[PATCH 0/3] Fix fanotify regression

[PATCH 0/3] Fix fanotify regression

[Jan Kara]

  Hello,

  as Dave and Jiri noticed my latest changes to notification framework
caused some use-after-free issues with fanotify (I've been too eager to
remove the use count in the event without realizing all the implications
with permission events). These three patches fix the issues. Dave, Jiri,
can you give a final testing round to this cleaned up set of patches?
Thanks! If they pass, I'll send a pull request with the patches to
Linus.

								Honza

[PATCH 1/3] fanotify: Fix use after free in mask checking

[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

[PATCH 2/3] fsnotify: Do not return merged event from fsnotify_add_notify_event()

[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

[PATCH 3/3] fanotify: Fix use after free for permission events

[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

Re: [PATCH 0/3] Fix fanotify regression

[Dave Jones]

On Tue, Jan 28, 2014 at 11:31:57PM +0100, Jan Kara wrote:
 > 
 >   Hello,
 > 
 >   as Dave and Jiri noticed my latest changes to notification framework
 > caused some use-after-free issues with fanotify (I've been too eager to
 > remove the use count in the event without realizing all the implications
 > with permission events). These three patches fix the issues. Dave, Jiri,
 > can you give a final testing round to this cleaned up set of patches?

Looks good to me!

Tested-by: Dave Jones <davej@fedoraproject.org>

Re: [PATCH 0/3] Fix fanotify regression

[Jiri Kosina]

On Tue, 28 Jan 2014, Jan Kara wrote:

>   as Dave and Jiri noticed my latest changes to notification framework
> caused some use-after-free issues with fanotify (I've been too eager to
> remove the use count in the event without realizing all the implications
> with permission events). These three patches fix the issues. Dave, Jiri,
> can you give a final testing round to this cleaned up set of patches?
> Thanks! If they pass, I'll send a pull request with the patches to
> Linus.

Yup, it works.

	Reported-and-tested-by: Jiri Kosina <jkosina@suse.cz>

Thanks,

-- 
Jiri Kosina
SUSE Labs