Re: fanotify use after free.

[Jan Kara <jack@suse.cz>]

[-- Attachment #1: Type: text/plain, Size: 1000 bytes --]

On Fri 24-01-14 08:26:45, Jiri Kosina wrote:
> On Fri, 24 Jan 2014, Jan Kara wrote:
> 
> >   Strange. I've installed systemd system (openSUSE 13.1) and it boots 
> > with the latest Linus' kernel just fine (and I have at least FANOTIFY 
> > and SLAB debugging set the same way as you). But it was only a KVM 
> > guest. I'll try tomorrow with a physical machine I guess.
> 
> FWIW the system I am reliably able to reproduce this on is opensuse 12.3 
> with this systemd version: 
> 
> Version     : 195
> Release     : 13.18.1
  Hum, still no luck with reproduction (either on physical machine or with
KVM). Anyway, I've looked at the code again and the previous patch had a
stupid bug (passing different pointer to fsnotify_destroy_event() than we
should have), plus also the merging function in fanotify was too
aggressive. Can you try the attached patch? It boots for me but that means
nothing since I cannot reproduce the issue... Thanks!

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

[-- Attachment #2: fanotify.diff --]
[-- Type: text/x-patch, Size: 1737 bytes --]

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index 58772623f02a..1b3dd9de8518 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -18,7 +18,7 @@ static bool should_merge(struct fsnotify_event *old_fsn,
 
 #ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS
 	/* dont merge two permission events */
-	if ((old_fsn->mask & FAN_ALL_PERM_EVENTS) &&
+	if ((old_fsn->mask & FAN_ALL_PERM_EVENTS) ||
 	    (new_fsn->mask & FAN_ALL_PERM_EVENTS))
 		return false;
 #endif
@@ -201,8 +201,10 @@ static int fanotify_handle_event(struct fsnotify_group *group,
 	}
 
 #ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS
-	if (fsn_event->mask & FAN_ALL_PERM_EVENTS)
+	if (fsn_event->mask & FAN_ALL_PERM_EVENTS) {
 		ret = fanotify_get_response_from_access(group, event);
+		fsnotify_destroy_event(group, fsn_event);
+	}
 #endif
 	return ret;
 }
@@ -221,7 +223,8 @@ static void fanotify_free_event(struct fsnotify_event *fsn_event)
 	struct fanotify_event_info *event;
 
 	event = FANOTIFY_E(fsn_event);
-	path_put(&event->path);
+	if (event->path.mnt)
+		path_put(&event->path);
 	put_pid(event->tgid);
 	kmem_cache_free(fanotify_event_cachep, event);
 }
diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index 57d7c083cb4b..d493c72c71fd 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -319,7 +319,8 @@ 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);
+			if (!(kevent->mask & FAN_ALL_PERM_EVENTS))
+				fsnotify_destroy_event(group, kevent);
 			if (ret < 0)
 				break;
 			buf += ret;