Message ID | 1317990455-25069-2-git-send-email-apw@canonical.com |
---|---|
State | New |
Headers | show |
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; > } >
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; }