[GIT PULL] fsnotify fix for v5.1-rc8

[GIT PULL] fsnotify fix for v5.1-rc8

[Jan Kara]

  Hello Linus,

  could you please pull from

git://git.kernel.org/pub/scm/linux/kernel/git/jack/linux-fs.git fsnotify_for_v5.1-rc8

to get a fix of user trigerable NULL pointer dereference syzbot has
recently spotted. The problem has been introduced in rc1 so no CC stable is
needed.

Top of the tree is b1da6a51871c. The full shortlog is:

Jan Kara (1):
      fsnotify: Fix NULL ptr deref in fanotify_get_fsid()

The diffstat is

 fs/notify/fanotify/fanotify.c | 14 ++++++++++++--
 fs/notify/mark.c              | 12 ++++++------
 2 files changed, 18 insertions(+), 8 deletions(-)

Also I'm sorry but the tag is not signed as I'm at LSF/MM this week and
forgot my Yubikey at home...

							Thanks
								Honza

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

Re: [GIT PULL] fsnotify fix for v5.1-rc8

[Linus Torvalds]

On Tue, Apr 30, 2019 at 2:41 PM Jan Kara <jack@suse.cz> wrote:
>
> to get a fix of user trigerable NULL pointer dereference syzbot has
> recently spotted. The problem has been introduced in rc1 so no CC stable is
> needed.

Hmm. Pulled, but I thin kthe use of READ_ONCE/WRITE_ONCE is suspicious.

If we're reading a pointer like this locklessly, the proper sequence
is almost always something like "smp_store_release()" to write the
pointer, and "smp_load_acquire()" to read it.

Because that not only does the "access once" semantics, but it also
guarantees that when we actually look _through_ the pointer, we see
the data that was written to it. In contrast, code like this (from the
fix)

+       WRITE_ONCE(mark->connector, conn);

   ...

+               conn = READ_ONCE(iter_info->marks[type]->connector);
+               /* Mark is just getting destroyed or created? */
+               if (!conn)
+                       continue;
+               fsid = conn->fsid;

is rather suspicious, because there's no obvious guarantee that tjhe
"conn->fsid" part was written on one CPU before we read it on another.

There may well be barriers in place there that end up guaranteeing it
in practice, but I wanted to point out that the READ/WRITE_ONCE()
pattern tends to be a bit dodgy unless you have some other explicit
synchronization (and if that synchronization is a lock, then you
obviously shouldn't be using READ/WRITE_ONCE() at all).

                 Linus

Re: [GIT PULL] fsnotify fix for v5.1-rc8

[pr-tracker-bot]

The pull request you sent on Tue, 30 Apr 2019 23:41:49 +0200:

> git://git.kernel.org/pub/scm/linux/kernel/git/jack/linux-fs.git fsnotify_for_v5.1-rc8

has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/f2bc9c908dfe3f56fe4ca4d92e5c5be80963b973

Thank you!

-- 
Deet-doot-dot, I am a bot.
https://korg.wiki.kernel.org/userdoc/prtracker

Re: [GIT PULL] fsnotify fix for v5.1-rc8

[Jan Kara]

On Tue 30-04-19 15:10:30, Linus Torvalds wrote:
> On Tue, Apr 30, 2019 at 2:41 PM Jan Kara <jack@suse.cz> wrote:
> >
> > to get a fix of user trigerable NULL pointer dereference syzbot has
> > recently spotted. The problem has been introduced in rc1 so no CC stable is
> > needed.
> 
> Hmm. Pulled, but I thin kthe use of READ_ONCE/WRITE_ONCE is suspicious.
> 
> If we're reading a pointer like this locklessly, the proper sequence
> is almost always something like "smp_store_release()" to write the
> pointer, and "smp_load_acquire()" to read it.
> 
> Because that not only does the "access once" semantics, but it also
> guarantees that when we actually look _through_ the pointer, we see
> the data that was written to it. In contrast, code like this (from the
> fix)
> 
> +       WRITE_ONCE(mark->connector, conn);
> 
>    ...
> 
> +               conn = READ_ONCE(iter_info->marks[type]->connector);
> +               /* Mark is just getting destroyed or created? */
> +               if (!conn)
> +                       continue;
> +               fsid = conn->fsid;
> 
> is rather suspicious, because there's no obvious guarantee that tjhe
> "conn->fsid" part was written on one CPU before we read it on another.

Hum, you're right. The WRITE_ONCE(mark->connector, conn) still is not
enough. It needs to have a barrier so that the connector initialization is
guaranteed to be visible by RCU reader.
READ_ONCE(iter_info->marks[type]->connector) is safe as is already contains
smp_read_barrier_depends() which is all that should be needed once we have
write barrier before WRITE_ONCE().

Since I don't think this is a practical problem, I'll just queue the fix
for the merge window. Thanks for spotting this!

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

Re: [GIT PULL] fsnotify fix for v5.1-rc8

[Jan Kara]

On Wed 01-05-19 16:52:28, Jan Kara wrote:
> On Tue 30-04-19 15:10:30, Linus Torvalds wrote:
> > On Tue, Apr 30, 2019 at 2:41 PM Jan Kara <jack@suse.cz> wrote:
> > >
> > > to get a fix of user trigerable NULL pointer dereference syzbot has
> > > recently spotted. The problem has been introduced in rc1 so no CC stable is
> > > needed.
> > 
> > Hmm. Pulled, but I thin kthe use of READ_ONCE/WRITE_ONCE is suspicious.
> > 
> > If we're reading a pointer like this locklessly, the proper sequence
> > is almost always something like "smp_store_release()" to write the
> > pointer, and "smp_load_acquire()" to read it.
> > 
> > Because that not only does the "access once" semantics, but it also
> > guarantees that when we actually look _through_ the pointer, we see
> > the data that was written to it. In contrast, code like this (from the
> > fix)
> > 
> > +       WRITE_ONCE(mark->connector, conn);
> > 
> >    ...
> > 
> > +               conn = READ_ONCE(iter_info->marks[type]->connector);
> > +               /* Mark is just getting destroyed or created? */
> > +               if (!conn)
> > +                       continue;
> > +               fsid = conn->fsid;
> > 
> > is rather suspicious, because there's no obvious guarantee that tjhe
> > "conn->fsid" part was written on one CPU before we read it on another.
> 
> Hum, you're right. The WRITE_ONCE(mark->connector, conn) still is not
> enough. It needs to have a barrier so that the connector initialization is
> guaranteed to be visible by RCU reader.
> READ_ONCE(iter_info->marks[type]->connector) is safe as is already contains
> smp_read_barrier_depends() which is all that should be needed once we have
> write barrier before WRITE_ONCE().
> 
> Since I don't think this is a practical problem, I'll just queue the fix
> for the merge window. Thanks for spotting this!

And looking some more into this. I don't think the issue can happen at all.
The thing is that the "connector" gets allocated, initialized, and attached
to inode / mntpoint / sb using cmpxchg() which provides the barrier. Then
mark gets added to connector's list and mark->connector is set. So *mark*
changes happening in fsnotify_add_mark_list() can get reordered (but
there's just list addition there) but *connector* changes are safely
visible. But this certainly deserves a comment as even I got confused and
it was me who wrote this all ;)

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