Re: general protection fault in fanotify_handle_event

[Jan Kara <jack@suse.cz>]

On Fri 19-04-19 12:33:02, Amir Goldstein wrote:
> On Thu, Apr 18, 2019 at 8:14 PM syzbot
> <syzbot+15927486a4f1bfcbaf91@syzkaller.appspotmail.com> wrote:
> >
> > syzbot has bisected this bug to:
> >
> > commit 77115225acc67d9ac4b15f04dd138006b9cd1ef2
> > Author: Amir Goldstein <amir73il@gmail.com>
> > Date:   Thu Jan 10 17:04:37 2019 +0000
> >
> >      fanotify: cache fsid in fsnotify_mark_connector
> >
> > bisection log:  https://syzkaller.appspot.com/x/bisect.txt?x=1627632d200000
> > start commit:   3f018f4a Add linux-next specific files for 20190418
> > git tree:       linux-next
> > final crash:    https://syzkaller.appspot.com/x/report.txt?x=1527632d200000
> > console output: https://syzkaller.appspot.com/x/log.txt?x=1127632d200000
> > kernel config:  https://syzkaller.appspot.com/x/.config?x=faa7bdc352fc157e
> > dashboard link: https://syzkaller.appspot.com/bug?extid=15927486a4f1bfcbaf91
> > syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=155543d3200000
> >
> > Reported-by: syzbot+15927486a4f1bfcbaf91@syzkaller.appspotmail.com
> > Fixes: 77115225acc6 ("fanotify: cache fsid in fsnotify_mark_connector")
> >

Hi Amir!

> It looks like lockless access to mark->connector is not safe as there is
> nothing preventing a reader from seeing a mark on object list without
> seeing the mark->connector assignment.

Yes, that seems to be possible.

> It made me wonder if (!mark->connector) check in fsnotify_put_mark() is safe.
> I couldn't find any call site where that would be a problem, but perhaps
> we should be more careful?

Why? That check is there really only to catch destruction of a mark that
was never attached (i.e., allocated but later never used due to some
error). Otherwise we should always have mark->connector set.

> Anyway, it seems that fsnotify_put_mark() uses the non NULL mark->connector
> as the indication that mark is on object list, so just assigning mark->connector
> before adding to object list won't do.
> 
> Since a reference of mark is our guaranty that mark->connector is not
> going away, I guess we could do opportunistic test for non NULL
> mark->connector from lockless path, if that fails, we check again
> with mark->lock held and if that fails something went wrong.
> 
> Another option is to teach fsnotify_first_mark() and fsnotify_next_mark()
> to skip over marks with NULL mark->connector.
> 
> What do you think? Did I over complicate this?

I'd prefer if we could make only fully initialized marks visible on
connector's list. Just to make things simple in the fast path of handling
events. So I'd just set mark->connector before adding mark to connector's
list and having smp_wmb() there to force ordering in
fsnotify_add_mark_list(). And we should use READ_ONCE() as a counter-part
to this in fanotify_get_fsid(). It is somewhat unfortunate that there are
three different ways how fsnotify_add_mark_list() can add mark to a list so
we may need to either repeat this in three different places or just use
some macro magic like:

#define fanotify_attach_obj_list(where, mark, conn, head) \
	do { \
		mark->connector = conn; \
		smp_wmb(); \
		hlist_add_##where##_rcu(&mark->obj_list, head); \
	} while (0)

And I would not really worry about fsnotify_put_mark() - if it ever sees
mark->connector set, mark really is on the connector's list and
fsnotify_put_mark() does the right thing (i.e. locks connector->lock if
needed).

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