Patchwork [maverick,CVE,1/1] inotify: fix double free/corruption of stuct user

login
register
mail settings
Submitter Andy Whitcroft
Date Oct. 7, 2011, 12:27 p.m.
Message ID <1317990455-25069-2-git-send-email-apw@canonical.com>
Download mbox | patch
Permalink /patch/118312/
State New
Headers show

Comments

Andy Whitcroft - Oct. 7, 2011, 12:27 p.m.
From: Eric Paris <eparis@redhat.com>

On an error path in inotify_init1 a normal user can trigger a double
free of struct user.  This is a regression introduced by a2ae4cc9a16e
("inotify: stop kernel memory leak on file creation failure").

We fix this by making sure that if a group exists the user reference is
dropped when the group is cleaned up.  We should not explictly drop the
reference on error and also drop the reference when the group is cleaned
up.

The new lifetime rules are that an inotify group lives from
inotify_new_group to the last fsnotify_put_group.  Since the struct user
and inotify_devs are directly tied to this lifetime they are only
changed/updated in those two locations.  We get rid of all special
casing of struct user or user->inotify_devs.

Signed-off-by: Eric Paris <eparis@redhat.com>
Cc: stable@kernel.org (2.6.37 and up)
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

(cherry picked from commit d0de4dc584ec6aa3b26fffea320a8457827768fc)
CVE-2011-1479
BugLink: http://bugs.launchpad.net/bugs/869203
Signed-off-by: Andy Whitcroft <apw@canonical.com>
---
 fs/notify/inotify/inotify_fsnotify.c |    1 +
 fs/notify/inotify/inotify_user.c     |   39 +++++++++++----------------------
 2 files changed, 14 insertions(+), 26 deletions(-)
Stefan Bader - Oct. 7, 2011, 12:38 p.m.
On 07.10.2011 14:27, Andy Whitcroft wrote:
> From: Eric Paris <eparis@redhat.com>
> 
> On an error path in inotify_init1 a normal user can trigger a double
> free of struct user.  This is a regression introduced by a2ae4cc9a16e
> ("inotify: stop kernel memory leak on file creation failure").
> 
> We fix this by making sure that if a group exists the user reference is
> dropped when the group is cleaned up.  We should not explictly drop the
> reference on error and also drop the reference when the group is cleaned
> up.
> 
> The new lifetime rules are that an inotify group lives from
> inotify_new_group to the last fsnotify_put_group.  Since the struct user
> and inotify_devs are directly tied to this lifetime they are only
> changed/updated in those two locations.  We get rid of all special
> casing of struct user or user->inotify_devs.
> 
> Signed-off-by: Eric Paris <eparis@redhat.com>
> Cc: stable@kernel.org (2.6.37 and up)
> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> 
> (cherry picked from commit d0de4dc584ec6aa3b26fffea320a8457827768fc)
> CVE-2011-1479
> BugLink: http://bugs.launchpad.net/bugs/869203
> Signed-off-by: Andy Whitcroft <apw@canonical.com>
> ---
>  fs/notify/inotify/inotify_fsnotify.c |    1 +
>  fs/notify/inotify/inotify_user.c     |   39 +++++++++++----------------------
>  2 files changed, 14 insertions(+), 26 deletions(-)
> 
> diff --git a/fs/notify/inotify/inotify_fsnotify.c b/fs/notify/inotify/inotify_fsnotify.c
> index e27960c..518e1bc 100644
> --- a/fs/notify/inotify/inotify_fsnotify.c
> +++ b/fs/notify/inotify/inotify_fsnotify.c
> @@ -147,6 +147,7 @@ static void inotify_free_group_priv(struct fsnotify_group *group)
>  	idr_for_each(&group->inotify_data.idr, idr_callback, group);
>  	idr_remove_all(&group->inotify_data.idr);
>  	idr_destroy(&group->inotify_data.idr);
> +	atomic_dec(&group->inotify_data.user->inotify_devs);
>  	free_uid(group->inotify_data.user);
>  }
>  
> diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c
> index 72f8825..7b1a2c3 100644
> --- a/fs/notify/inotify/inotify_user.c
> +++ b/fs/notify/inotify/inotify_user.c
> @@ -290,15 +290,12 @@ static int inotify_fasync(int fd, struct file *file, int on)
>  static int inotify_release(struct inode *ignored, struct file *file)
>  {
>  	struct fsnotify_group *group = file->private_data;
> -	struct user_struct *user = group->inotify_data.user;
>  
>  	fsnotify_clear_marks_by_group(group);
>  
>  	/* free this group, matching get was inotify_init->fsnotify_obtain_group */
>  	fsnotify_put_group(group);
>  
> -	atomic_dec(&user->inotify_devs);
> -
>  	return 0;
>  }
>  
> @@ -616,7 +613,7 @@ retry:
>  	return ret;
>  }
>  
> -static struct fsnotify_group *inotify_new_group(struct user_struct *user, unsigned int max_events)
> +static struct fsnotify_group *inotify_new_group(unsigned int max_events)
>  {
>  	struct fsnotify_group *group;
>  	unsigned int grp_num;
> @@ -632,8 +629,14 @@ static struct fsnotify_group *inotify_new_group(struct user_struct *user, unsign
>  	spin_lock_init(&group->inotify_data.idr_lock);
>  	idr_init(&group->inotify_data.idr);
>  	group->inotify_data.last_wd = 0;
> -	group->inotify_data.user = user;
>  	group->inotify_data.fa = NULL;
> +	group->inotify_data.user = get_current_user();
> +
> +	if (atomic_inc_return(&group->inotify_data.user->inotify_devs) >
> +	    inotify_max_user_instances) {
> +		fsnotify_put_group(group);
> +		return ERR_PTR(-EMFILE);
> +	}
>  
>  	return group;
>  }
> @@ -643,7 +646,6 @@ static struct fsnotify_group *inotify_new_group(struct user_struct *user, unsign
>  SYSCALL_DEFINE1(inotify_init1, int, flags)
>  {
>  	struct fsnotify_group *group;
> -	struct user_struct *user;
>  	int ret;
>  
>  	/* Check the IN_* constants for consistency.  */
> @@ -653,31 +655,16 @@ SYSCALL_DEFINE1(inotify_init1, int, flags)
>  	if (flags & ~(IN_CLOEXEC | IN_NONBLOCK))
>  		return -EINVAL;
>  
> -	user = get_current_user();
> -	if (unlikely(atomic_read(&user->inotify_devs) >=
> -			inotify_max_user_instances)) {
> -		ret = -EMFILE;
> -		goto out_free_uid;
> -	}
> -
>  	/* fsnotify_obtain_group took a reference to group, we put this when we kill the file in the end */
> -	group = inotify_new_group(user, inotify_max_queued_events);
> -	if (IS_ERR(group)) {
> -		ret = PTR_ERR(group);
> -		goto out_free_uid;
> -	}
> -
> -	atomic_inc(&user->inotify_devs);
> +	group = inotify_new_group(inotify_max_queued_events);
> +	if (IS_ERR(group))
> +		return PTR_ERR(group);
>  
>  	ret = anon_inode_getfd("inotify", &inotify_fops, group,
>  				  O_RDONLY | flags);
> -	if (ret >= 0)
> -		return ret;
> +	if (ret < 0)
> +		fsnotify_put_group(group);
>  
> -	fsnotify_put_group(group);
> -	atomic_dec(&user->inotify_devs);
> -out_free_uid:
> -	free_uid(user);
>  	return ret;
>  }
>

Patch

diff --git a/fs/notify/inotify/inotify_fsnotify.c b/fs/notify/inotify/inotify_fsnotify.c
index e27960c..518e1bc 100644
--- a/fs/notify/inotify/inotify_fsnotify.c
+++ b/fs/notify/inotify/inotify_fsnotify.c
@@ -147,6 +147,7 @@  static void inotify_free_group_priv(struct fsnotify_group *group)
 	idr_for_each(&group->inotify_data.idr, idr_callback, group);
 	idr_remove_all(&group->inotify_data.idr);
 	idr_destroy(&group->inotify_data.idr);
+	atomic_dec(&group->inotify_data.user->inotify_devs);
 	free_uid(group->inotify_data.user);
 }
 
diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c
index 72f8825..7b1a2c3 100644
--- a/fs/notify/inotify/inotify_user.c
+++ b/fs/notify/inotify/inotify_user.c
@@ -290,15 +290,12 @@  static int inotify_fasync(int fd, struct file *file, int on)
 static int inotify_release(struct inode *ignored, struct file *file)
 {
 	struct fsnotify_group *group = file->private_data;
-	struct user_struct *user = group->inotify_data.user;
 
 	fsnotify_clear_marks_by_group(group);
 
 	/* free this group, matching get was inotify_init->fsnotify_obtain_group */
 	fsnotify_put_group(group);
 
-	atomic_dec(&user->inotify_devs);
-
 	return 0;
 }
 
@@ -616,7 +613,7 @@  retry:
 	return ret;
 }
 
-static struct fsnotify_group *inotify_new_group(struct user_struct *user, unsigned int max_events)
+static struct fsnotify_group *inotify_new_group(unsigned int max_events)
 {
 	struct fsnotify_group *group;
 	unsigned int grp_num;
@@ -632,8 +629,14 @@  static struct fsnotify_group *inotify_new_group(struct user_struct *user, unsign
 	spin_lock_init(&group->inotify_data.idr_lock);
 	idr_init(&group->inotify_data.idr);
 	group->inotify_data.last_wd = 0;
-	group->inotify_data.user = user;
 	group->inotify_data.fa = NULL;
+	group->inotify_data.user = get_current_user();
+
+	if (atomic_inc_return(&group->inotify_data.user->inotify_devs) >
+	    inotify_max_user_instances) {
+		fsnotify_put_group(group);
+		return ERR_PTR(-EMFILE);
+	}
 
 	return group;
 }
@@ -643,7 +646,6 @@  static struct fsnotify_group *inotify_new_group(struct user_struct *user, unsign
 SYSCALL_DEFINE1(inotify_init1, int, flags)
 {
 	struct fsnotify_group *group;
-	struct user_struct *user;
 	int ret;
 
 	/* Check the IN_* constants for consistency.  */
@@ -653,31 +655,16 @@  SYSCALL_DEFINE1(inotify_init1, int, flags)
 	if (flags & ~(IN_CLOEXEC | IN_NONBLOCK))
 		return -EINVAL;
 
-	user = get_current_user();
-	if (unlikely(atomic_read(&user->inotify_devs) >=
-			inotify_max_user_instances)) {
-		ret = -EMFILE;
-		goto out_free_uid;
-	}
-
 	/* fsnotify_obtain_group took a reference to group, we put this when we kill the file in the end */
-	group = inotify_new_group(user, inotify_max_queued_events);
-	if (IS_ERR(group)) {
-		ret = PTR_ERR(group);
-		goto out_free_uid;
-	}
-
-	atomic_inc(&user->inotify_devs);
+	group = inotify_new_group(inotify_max_queued_events);
+	if (IS_ERR(group))
+		return PTR_ERR(group);
 
 	ret = anon_inode_getfd("inotify", &inotify_fops, group,
 				  O_RDONLY | flags);
-	if (ret >= 0)
-		return ret;
+	if (ret < 0)
+		fsnotify_put_group(group);
 
-	fsnotify_put_group(group);
-	atomic_dec(&user->inotify_devs);
-out_free_uid:
-	free_uid(user);
 	return ret;
 }