| Message ID | 1508920899-8115-3-git-send-email-mszeredi@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed 25-10-17 10:41:34, Miklos Szeredi wrote: > We may fail to pin one of the marks in fsnotify_prepare_user_wait() when > dropping the srcu read lock, resulting in use after free at the next > iteration. > > Solution is to store both marks in iter_info instead of just the one we'll > be sending the event for. I'm sorry but I'm not getting it. Where exactly is use-after-free happening? And how come because if fsnotify_prepare_user_wait() fails to pin some mark, it does not drop SRCU and bails out, doesn't it? Honza > diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c > index 0c4583b61717..48ec61f4c4d5 100644 > --- a/fs/notify/fsnotify.c > +++ b/fs/notify/fsnotify.c > @@ -336,6 +336,9 @@ int fsnotify(struct inode *to_tell, __u32 mask, const void *data, int data_is, > vfsmount_group = vfsmount_mark->group; > } > > + iter_info.inode_mark = inode_mark; > + iter_info.vfsmount_mark = vfsmount_mark; > + > if (inode_group && vfsmount_group) { > int cmp = fsnotify_compare_groups(inode_group, > vfsmount_group); > @@ -348,9 +351,6 @@ int fsnotify(struct inode *to_tell, __u32 mask, const void *data, int data_is, > } > } > > - iter_info.inode_mark = inode_mark; > - iter_info.vfsmount_mark = vfsmount_mark; > - > ret = send_to_group(to_tell, inode_mark, vfsmount_mark, mask, > data, data_is, cookie, file_name, > &iter_info); > -- > 2.5.5 >
On Mon, Oct 30, 2017 at 2:34 PM, Jan Kara <jack@suse.cz> wrote: > On Wed 25-10-17 10:41:34, Miklos Szeredi wrote: >> We may fail to pin one of the marks in fsnotify_prepare_user_wait() when >> dropping the srcu read lock, resulting in use after free at the next >> iteration. >> >> Solution is to store both marks in iter_info instead of just the one we'll >> be sending the event for. > > I'm sorry but I'm not getting it. Where exactly is use-after-free > happening? And how come because if fsnotify_prepare_user_wait() fails to > pin some mark, it does not drop SRCU and bails out, doesn't it? No no. It's the success case, when we do drop SRCU. Two marks non-NULL marks: inode_mark, vfsmount_mark. We select one with fsnotify_compare_groups(), e.g. cmp < 0: vfsmount_mark set to NULL. So we pin inode_mark but not vfsmount_mark. Then we find the next mark for inode_mark, and use non-NULL vfsmount_node to find the same vfsmount_mark that we started out previously. But that one was not pinned while SRCU was released -> use after free. Thanks, Miklos
On Mon 30-10-17 14:42:11, Miklos Szeredi wrote: > On Mon, Oct 30, 2017 at 2:34 PM, Jan Kara <jack@suse.cz> wrote: > > On Wed 25-10-17 10:41:34, Miklos Szeredi wrote: > >> We may fail to pin one of the marks in fsnotify_prepare_user_wait() when > >> dropping the srcu read lock, resulting in use after free at the next > >> iteration. > >> > >> Solution is to store both marks in iter_info instead of just the one we'll > >> be sending the event for. > > > > I'm sorry but I'm not getting it. Where exactly is use-after-free > > happening? And how come because if fsnotify_prepare_user_wait() fails to > > pin some mark, it does not drop SRCU and bails out, doesn't it? > > No no. It's the success case, when we do drop SRCU. > > Two marks non-NULL marks: inode_mark, vfsmount_mark. We select one > with fsnotify_compare_groups(), e.g. cmp < 0: vfsmount_mark set to > NULL. > > So we pin inode_mark but not vfsmount_mark. Then we find the next > mark for inode_mark, and use non-NULL vfsmount_node to find the same > vfsmount_mark that we started out previously. But that one was not > pinned while SRCU was released -> use after free. Got it. Thanks for explanation! But please add a comment that we need to protect both inode_node and vfsmount_node against freeing so that we can continue iteration at the place where iter_info.inode_mark and vfsmount_mark are set. Honza
diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c index 0c4583b61717..48ec61f4c4d5 100644 --- a/fs/notify/fsnotify.c +++ b/fs/notify/fsnotify.c @@ -336,6 +336,9 @@ int fsnotify(struct inode *to_tell, __u32 mask, const void *data, int data_is, vfsmount_group = vfsmount_mark->group; } + iter_info.inode_mark = inode_mark; + iter_info.vfsmount_mark = vfsmount_mark; + if (inode_group && vfsmount_group) { int cmp = fsnotify_compare_groups(inode_group, vfsmount_group); @@ -348,9 +351,6 @@ int fsnotify(struct inode *to_tell, __u32 mask, const void *data, int data_is, } } - iter_info.inode_mark = inode_mark; - iter_info.vfsmount_mark = vfsmount_mark; - ret = send_to_group(to_tell, inode_mark, vfsmount_mark, mask, data, data_is, cookie, file_name, &iter_info);
We may fail to pin one of the marks in fsnotify_prepare_user_wait() when dropping the srcu read lock, resulting in use after free at the next iteration. Solution is to store both marks in iter_info instead of just the one we'll be sending the event for. Signed-off-by: Miklos Szeredi <mszeredi@redhat.com> Fixes: 9385a84d7e1f ("fsnotify: Pass fsnotify_iter_info into handle_event handler") Cc: <stable@vger.kernel.org> # v4.12 --- fs/notify/fsnotify.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)