linux-ext4 - Re: [PATCH v8 11/32] fsnotify: Protect fsnotify_handle_inode

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20211019100106.GG3255@quack2.suse.cz>
Date:   Tue, 19 Oct 2021 12:01:06 +0200
From:   Jan Kara <jack@...e.cz>
To:     Amir Goldstein <amir73il@...il.com>
Cc:     Gabriel Krisman Bertazi <krisman@...labora.com>,
        Jan Kara <jack@...e.com>,
        "Darrick J. Wong" <djwong@...nel.org>,
        Theodore Tso <tytso@....edu>,
        Dave Chinner <david@...morbit.com>,
        David Howells <dhowells@...hat.com>,
        Khazhismel Kumykov <khazhy@...gle.com>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        Ext4 <linux-ext4@...r.kernel.org>,
        Linux API <linux-api@...r.kernel.org>, kernel@...labora.com
Subject: Re: [PATCH v8 11/32] fsnotify: Protect fsnotify_handle_inode_event
 from no-inode events

On Tue 19-10-21 08:34:41, Amir Goldstein wrote:
> On Tue, Oct 19, 2021 at 3:01 AM Gabriel Krisman Bertazi
> <krisman@...labora.com> wrote:
> >
> > FAN_FS_ERROR allows events without inodes - i.e. for file system-wide
> > errors.  Even though fsnotify_handle_inode_event is not currently used
> > by fanotify, this patch protects this path to handle this new case.
> >
> > Suggested-by: Amir Goldstein <amir73il@...il.com>
> > Signed-off-by: Gabriel Krisman Bertazi <krisman@...labora.com>
> > ---
> >  fs/notify/fsnotify.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
> > index fde3a1115a17..47f931fb571c 100644
> > --- a/fs/notify/fsnotify.c
> > +++ b/fs/notify/fsnotify.c
> > @@ -252,6 +252,9 @@ static int fsnotify_handle_inode_event(struct fsnotify_group *group,
> >         if (WARN_ON_ONCE(!ops->handle_inode_event))
> >                 return 0;
> >
> > +       if (!inode)
> > +               return 0;
> > +
> 
> Sigh.. the plot thickens.
> There are three in-tree backends that implement the ->handle_inode_event()
> interface.
> 
> inotify and dnotify can take NULL inode and the above will make the CREATE
> events on kernfs vanish, so we cannot do that.
> Sorry for not noticing this earlier when I asked for this change.
> 
> nfsd_file_fsnotify_handle_event() can most certainly not take NULL inode,
> but nfsd does not watch for CREATE events.

And furthermore you cannot really export kernfs :)

> I think what we need to do is (Jan please correct me if you think otherwise):
> 1. Document the handle_inode_event() interface that either inode or dir
>     must be non-NULL
> 2. WARN_ON_ONCE(!inode && !dir) instead of just (!inode) above

Yeah, like:
	if (WARN_ON_ONCE(!inode && !dir))
		return 0;

> 3. Add WARN_ON_ONCE(!inode) before trace_nfsd_file_fsnotify_handle_event()
>     in nfsd_file_fsnotify_handle_event()

And:
	if (WARN_ON_ONCE(!inode))
		return 0;

Sounds like a good plan to me.

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