* [PATCH 1/3] fanotify: Fix use after free in mask checking
2014-01-28 22:31 [PATCH 0/3] Fix fanotify regression Jan Kara
@ 2014-01-28 22:31 ` Jan Kara
2014-01-28 22:31 ` [PATCH 2/3] fsnotify: Do not return merged event from fsnotify_add_notify_event() Jan Kara
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Jan Kara @ 2014-01-28 22:31 UTC (permalink / raw)
To: linux-fsdevel; +Cc: Jiri Kosina, Dave Jones, Jan Kara
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.
Signed-off-by: Jan Kara <jack@suse.cz>
---
fs/notify/fanotify/fanotify.c | 21 +++++++++++----------
1 file changed, 11 insertions(+), 10 deletions(-)
diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index 58772623f02a..cc78e2fbc8e4 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;
--
1.8.1.4
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 2/3] fsnotify: Do not return merged event from fsnotify_add_notify_event()
2014-01-28 22:31 [PATCH 0/3] Fix fanotify regression Jan Kara
2014-01-28 22:31 ` [PATCH 1/3] fanotify: Fix use after free in mask checking Jan Kara
@ 2014-01-28 22:31 ` Jan Kara
2014-01-28 22:32 ` [PATCH 3/3] fanotify: Fix use after free for permission events Jan Kara
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Jan Kara @ 2014-01-28 22:31 UTC (permalink / raw)
To: linux-fsdevel; +Cc: Jiri Kosina, Dave Jones, Jan Kara
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.
Signed-off-by: Jan Kara <jack@suse.cz>
---
fs/notify/fanotify/fanotify.c | 18 +++++++-----------
fs/notify/inotify/inotify_fsnotify.c | 19 +++++++------------
fs/notify/notification.c | 24 ++++++++++++------------
include/linux/fsnotify_backend.h | 8 ++++----
4 files changed, 30 insertions(+), 39 deletions(-)
diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index cc78e2fbc8e4..c7e5e8f54748 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 aad1a35e9af1..d5ee56348bb8 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 952237b8e2d2..18b3c4427dca 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;
}
/*
diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
index 7d8d5e608594..3d286ff49ab0 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -322,10 +322,10 @@ extern int fsnotify_fasync(int fd, struct file *file, int on);
extern void fsnotify_destroy_event(struct fsnotify_group *group,
struct fsnotify_event *event);
/* attach the event to the group notification queue */
-extern 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 *));
+extern int fsnotify_add_notify_event(struct fsnotify_group *group,
+ struct fsnotify_event *event,
+ int (*merge)(struct list_head *,
+ struct fsnotify_event *));
/* true if the group notification queue is empty */
extern bool fsnotify_notify_queue_is_empty(struct fsnotify_group *group);
/* return, but do not dequeue the first event on the notification queue */
--
1.8.1.4
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 3/3] fanotify: Fix use after free for permission events
2014-01-28 22:31 [PATCH 0/3] Fix fanotify regression Jan Kara
2014-01-28 22:31 ` [PATCH 1/3] fanotify: Fix use after free in mask checking Jan Kara
2014-01-28 22:31 ` [PATCH 2/3] fsnotify: Do not return merged event from fsnotify_add_notify_event() Jan Kara
@ 2014-01-28 22:32 ` Jan Kara
2014-01-28 22:59 ` [PATCH 0/3] Fix fanotify regression Dave Jones
2014-01-29 12:00 ` Jiri Kosina
4 siblings, 0 replies; 6+ messages in thread
From: Jan Kara @ 2014-01-28 22:32 UTC (permalink / raw)
To: linux-fsdevel; +Cc: Jiri Kosina, Dave Jones, Jan Kara
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.
Signed-off-by: Jan Kara <jack@suse.cz>
---
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(-)
diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index c7e5e8f54748..0e792f5e3147 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 0e90174a116a..32a2f034fb94 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 57d7c083cb4b..3900255310b9 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;
--
1.8.1.4
^ permalink raw reply related [flat|nested] 6+ messages in thread