Re: fanotify use after free.

[Jan Kara]

On Wed 22-01-14 10:20:01, Linus Torvalds wrote:
> On Tue, Jan 21, 2014 at 10:27 PM, Dave Jones <davej@xxxxxxxxxx> wrote:
> >
> > BUG fanotify_event_info (Not tainted): Poison overwritten
> 
> Looking at the poison data, it seems that is is the
> 
>         u32 response;
> 
> field that has been overwritten (with all zero).
> 
> That doesn't really help me guess where the bug is, though. That code
> is crazy. For example, looking at one place where it is set, we have:
> 
>  - process_access_response():
> 
>         re->event->response = response;
> 
>         wake_up(&group->fanotify_data.access_waitq);
> 
>         kmem_cache_free(fanotify_response_event_cache, re);
> 
> which looks buggy in *so* many ways. In particular, we're doing a
> kmem_cache_free() on "re", but what happened to "re->event" that we
> just used? There was no release of that anywhere. Wut?
> 
> So it seems that the lifetime of these "fanotify_event_info"
> structures is completely buggered. I don't even see any *attempt* to
> maintain reference counts or other lifetime info. People free the
> containers that point to them without doing anything at all about the
> fsnotify_event that contains the fanotify_event_info that they point
> to.
> 
> Jan - how is the lifetime of the fanotify_event_info tied to the
> lifetime of the fanotify_response_event structure that contains
> pointers into it? Because I don't see it.
  Yeah, I messed that up. They aren't tied in any way - well, in fact they
end up being tied but in a wrong way. fanotify_event_info lives from the
moment event happens to the moment user reads the event. At that moment the
fanotify_response_event gets created (for those special permission events),
pointing to fanotify_event_info which will get freed just several lines
further :-|

But refcounting seems like an overkill for this - there is exactly one
fanotify_response_event structure iff it is a permission event. So
something like the (completely untested) attached patch should fix the
problem. But I agree it's a bit ugly so we might want something different.
I'll try to think about something better tomorrow.

> And considering that it's the response field that gets overwritten, it
> really sounds like *that* is the exact issue at play here - there is
> some fanotify_response_event structure holding a pointer to the
> fanotify_event that is embedded into a fanotify_event_info that has
> been freed.

								Honza
-- 
Jan Kara <jack@xxxxxxx>
SUSE Labs, CR
diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index 58772623f02a..756e9b047e27 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -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, event);
+	}
 #endif
 	return ret;
 }
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;