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

[Stefan Bader]

On 07.10.2011 14:27, Andy Whitcroft wrote:
> From: Eric Paris <eparis at 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 at redhat.com>
> Cc: stable at kernel.org (2.6.37 and up)
> Signed-off-by: Linus Torvalds <torvalds at linux-foundation.org>
> 
> (cherry picked from commit d0de4dc584ec6aa3b26fffea320a8457827768fc)
> CVE-2011-1479
> BugLink: http://bugs.launchpad.net/bugs/869203
> Signed-off-by: Andy Whitcroft <apw at 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;
>  }
>