From 13116dfd13c8c9d60ea04ece13419af2de8e2e37 Mon Sep 17 00:00:00 2001 From: Jan Kara Date: Tue, 28 Jan 2014 18:29:24 +0100 Subject: fanotify: Fix use after free in mask checking We cannot use the event structure returned from fsnotify_add_notify_event() because that event can be freed by the time that function returns. Use the mask argument passed into the event handler directly instead. This also fixes a possible problem when we could unnecessarily wait for permission response for a normal fanotify event which got merged with a permission event. We also disallow merging of permission event with any other event so that we know the permission event which we just created is the one on which we should wait for permission response. Reported-and-tested-by: Jiri Kosina Reported-and-tested-by: Dave Jones Signed-off-by: Jan Kara --- fs/notify/fanotify/fanotify.c | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) (limited to 'fs') diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c index 58772623f02..cc78e2fbc8e 100644 --- a/fs/notify/fanotify/fanotify.c +++ b/fs/notify/fanotify/fanotify.c @@ -16,12 +16,6 @@ static bool should_merge(struct fsnotify_event *old_fsn, { struct fanotify_event_info *old, *new; -#ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS - /* dont merge two permission events */ - if ((old_fsn->mask & FAN_ALL_PERM_EVENTS) && - (new_fsn->mask & FAN_ALL_PERM_EVENTS)) - return false; -#endif pr_debug("%s: old=%p new=%p\n", __func__, old_fsn, new_fsn); old = FANOTIFY_E(old_fsn); new = FANOTIFY_E(new_fsn); @@ -42,6 +36,16 @@ static struct fsnotify_event *fanotify_merge(struct list_head *list, pr_debug("%s: list=%p event=%p\n", __func__, list, event); +#ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS + /* + * Don't merge a permission event with any other event so that we know + * the event structure we have created in fanotify_handle_event() is the + * one we should check for permission response. + */ + if (event->mask & FAN_ALL_PERM_EVENTS) + return NULL; +#endif + list_for_each_entry_reverse(test_event, list, list) { if (should_merge(test_event, event)) { do_merge = true; @@ -195,13 +199,10 @@ static int fanotify_handle_event(struct fsnotify_group *group, fsnotify_destroy_event(group, fsn_event); if (IS_ERR(notify_fsn_event)) return PTR_ERR(notify_fsn_event); - /* We need to ask about a different events after a merge... */ - event = FANOTIFY_E(notify_fsn_event); - fsn_event = notify_fsn_event; } #ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS - if (fsn_event->mask & FAN_ALL_PERM_EVENTS) + if (mask & FAN_ALL_PERM_EVENTS) ret = fanotify_get_response_from_access(group, event); #endif return ret; -- cgit v1.2.3-18-g5258 From 83c0e1b442b488571f4fef4a91c2fe52eed6c705 Mon Sep 17 00:00:00 2001 From: Jan Kara Date: Tue, 28 Jan 2014 18:53:22 +0100 Subject: fsnotify: Do not return merged event from fsnotify_add_notify_event() The event returned from fsnotify_add_notify_event() cannot ever be used safely as the event may be freed by the time the function returns (after dropping notification_mutex). So change the prototype to just return whether the event was added or merged into some existing event. Reported-and-tested-by: Jiri Kosina Reported-and-tested-by: Dave Jones Signed-off-by: Jan Kara --- fs/notify/fanotify/fanotify.c | 18 +++++++----------- fs/notify/inotify/inotify_fsnotify.c | 19 +++++++------------ fs/notify/notification.c | 24 ++++++++++++------------ 3 files changed, 26 insertions(+), 35 deletions(-) (limited to 'fs') diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c index cc78e2fbc8e..c7e5e8f5474 100644 --- a/fs/notify/fanotify/fanotify.c +++ b/fs/notify/fanotify/fanotify.c @@ -28,8 +28,7 @@ static bool should_merge(struct fsnotify_event *old_fsn, } /* and the list better be locked by something too! */ -static struct fsnotify_event *fanotify_merge(struct list_head *list, - struct fsnotify_event *event) +static int fanotify_merge(struct list_head *list, struct fsnotify_event *event) { struct fsnotify_event *test_event; bool do_merge = false; @@ -43,7 +42,7 @@ static struct fsnotify_event *fanotify_merge(struct list_head *list, * one we should check for permission response. */ if (event->mask & FAN_ALL_PERM_EVENTS) - return NULL; + return 0; #endif list_for_each_entry_reverse(test_event, list, list) { @@ -54,10 +53,10 @@ static struct fsnotify_event *fanotify_merge(struct list_head *list, } if (!do_merge) - return NULL; + return 0; test_event->mask |= event->mask; - return test_event; + return 1; } #ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS @@ -153,7 +152,6 @@ static int fanotify_handle_event(struct fsnotify_group *group, int ret = 0; struct fanotify_event_info *event; struct fsnotify_event *fsn_event; - struct fsnotify_event *notify_fsn_event; BUILD_BUG_ON(FAN_ACCESS != FS_ACCESS); BUILD_BUG_ON(FAN_MODIFY != FS_MODIFY); @@ -192,13 +190,11 @@ static int fanotify_handle_event(struct fsnotify_group *group, event->response = 0; #endif - notify_fsn_event = fsnotify_add_notify_event(group, fsn_event, - fanotify_merge); - if (notify_fsn_event) { + ret = fsnotify_add_notify_event(group, fsn_event, fanotify_merge); + if (ret) { /* Our event wasn't used in the end. Free it. */ fsnotify_destroy_event(group, fsn_event); - if (IS_ERR(notify_fsn_event)) - return PTR_ERR(notify_fsn_event); + ret = 0; } #ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS diff --git a/fs/notify/inotify/inotify_fsnotify.c b/fs/notify/inotify/inotify_fsnotify.c index aad1a35e9af..d5ee56348bb 100644 --- a/fs/notify/inotify/inotify_fsnotify.c +++ b/fs/notify/inotify/inotify_fsnotify.c @@ -53,15 +53,13 @@ static bool event_compare(struct fsnotify_event *old_fsn, return false; } -static struct fsnotify_event *inotify_merge(struct list_head *list, - struct fsnotify_event *event) +static int inotify_merge(struct list_head *list, + struct fsnotify_event *event) { struct fsnotify_event *last_event; last_event = list_entry(list->prev, struct fsnotify_event, list); - if (!event_compare(last_event, event)) - return NULL; - return last_event; + return event_compare(last_event, event); } int inotify_handle_event(struct fsnotify_group *group, @@ -73,9 +71,8 @@ int inotify_handle_event(struct fsnotify_group *group, { struct inotify_inode_mark *i_mark; struct inotify_event_info *event; - struct fsnotify_event *added_event; struct fsnotify_event *fsn_event; - int ret = 0; + int ret; int len = 0; int alloc_len = sizeof(struct inotify_event_info); @@ -110,18 +107,16 @@ int inotify_handle_event(struct fsnotify_group *group, if (len) strcpy(event->name, file_name); - added_event = fsnotify_add_notify_event(group, fsn_event, inotify_merge); - if (added_event) { + ret = fsnotify_add_notify_event(group, fsn_event, inotify_merge); + if (ret) { /* Our event wasn't used in the end. Free it. */ fsnotify_destroy_event(group, fsn_event); - if (IS_ERR(added_event)) - ret = PTR_ERR(added_event); } if (inode_mark->mask & IN_ONESHOT) fsnotify_destroy_mark(inode_mark, group); - return ret; + return 0; } static void inotify_freeing_mark(struct fsnotify_mark *fsn_mark, struct fsnotify_group *group) diff --git a/fs/notify/notification.c b/fs/notify/notification.c index 952237b8e2d..18b3c4427dc 100644 --- a/fs/notify/notification.c +++ b/fs/notify/notification.c @@ -79,15 +79,15 @@ void fsnotify_destroy_event(struct fsnotify_group *group, /* * Add an event to the group notification queue. The group can later pull this - * event off the queue to deal with. If the event is successfully added to the - * group's notification queue, a reference is taken on event. + * event off the queue to deal with. The function returns 0 if the event was + * added to the queue, 1 if the event was merged with some other queued event. */ -struct fsnotify_event *fsnotify_add_notify_event(struct fsnotify_group *group, - struct fsnotify_event *event, - struct fsnotify_event *(*merge)(struct list_head *, - struct fsnotify_event *)) +int fsnotify_add_notify_event(struct fsnotify_group *group, + struct fsnotify_event *event, + int (*merge)(struct list_head *, + struct fsnotify_event *)) { - struct fsnotify_event *return_event = NULL; + int ret = 0; struct list_head *list = &group->notification_list; pr_debug("%s: group=%p event=%p\n", __func__, group, event); @@ -98,14 +98,14 @@ struct fsnotify_event *fsnotify_add_notify_event(struct fsnotify_group *group, /* Queue overflow event only if it isn't already queued */ if (list_empty(&group->overflow_event.list)) event = &group->overflow_event; - return_event = event; + ret = 1; } if (!list_empty(list) && merge) { - return_event = merge(list, event); - if (return_event) { + ret = merge(list, event); + if (ret) { mutex_unlock(&group->notification_mutex); - return return_event; + return ret; } } @@ -115,7 +115,7 @@ struct fsnotify_event *fsnotify_add_notify_event(struct fsnotify_group *group, wake_up(&group->notification_waitq); kill_fasync(&group->fsn_fa, SIGIO, POLL_IN); - return return_event; + return ret; } /* -- cgit v1.2.3-18-g5258 From 85816794240b9659e66e4d9b0df7c6e814e5f603 Mon Sep 17 00:00:00 2001 From: Jan Kara Date: Tue, 28 Jan 2014 21:38:06 +0100 Subject: fanotify: Fix use after free for permission events Currently struct fanotify_event_info has been destroyed immediately after reporting its contents to userspace. However that is wrong for permission events because those need to stay around until userspace provides response which is filled back in fanotify_event_info. So change to code to free permission events only after we have got the response from userspace. Reported-and-tested-by: Jiri Kosina Reported-and-tested-by: Dave Jones Signed-off-by: Jan Kara --- fs/notify/fanotify/fanotify.c | 5 ++++- fs/notify/fanotify/fanotify.h | 7 +++++++ fs/notify/fanotify/fanotify_user.c | 7 ++++++- 3 files changed, 17 insertions(+), 2 deletions(-) (limited to 'fs') diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c index c7e5e8f5474..0e792f5e314 100644 --- a/fs/notify/fanotify/fanotify.c +++ b/fs/notify/fanotify/fanotify.c @@ -192,14 +192,17 @@ static int fanotify_handle_event(struct fsnotify_group *group, ret = fsnotify_add_notify_event(group, fsn_event, fanotify_merge); if (ret) { + BUG_ON(mask & FAN_ALL_PERM_EVENTS); /* Our event wasn't used in the end. Free it. */ fsnotify_destroy_event(group, fsn_event); ret = 0; } #ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS - if (mask & FAN_ALL_PERM_EVENTS) + if (mask & FAN_ALL_PERM_EVENTS) { ret = fanotify_get_response_from_access(group, event); + fsnotify_destroy_event(group, fsn_event); + } #endif return ret; } diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h index 0e90174a116..32a2f034fb9 100644 --- a/fs/notify/fanotify/fanotify.h +++ b/fs/notify/fanotify/fanotify.h @@ -4,6 +4,13 @@ extern struct kmem_cache *fanotify_event_cachep; +/* + * Lifetime of the structure differs for normal and permission events. In both + * cases the structure is allocated in fanotify_handle_event(). For normal + * events the structure is freed immediately after reporting it to userspace. + * For permission events we free it only after we receive response from + * userspace. + */ struct fanotify_event_info { struct fsnotify_event fse; /* diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c index 1fd66abe574..b6175fa11bf 100644 --- a/fs/notify/fanotify/fanotify_user.c +++ b/fs/notify/fanotify/fanotify_user.c @@ -319,7 +319,12 @@ static ssize_t fanotify_read(struct file *file, char __user *buf, if (IS_ERR(kevent)) break; ret = copy_event_to_user(group, kevent, buf); - fsnotify_destroy_event(group, kevent); + /* + * Permission events get destroyed after we + * receive response + */ + if (!(kevent->mask & FAN_ALL_PERM_EVENTS)) + fsnotify_destroy_event(group, kevent); if (ret < 0) break; buf += ret; -- cgit v1.2.3-18-g5258