linux-ext4 - Re: [PATCH v3 12/15] fanotify: Introduce FAN_FS

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]

Date:   Thu, 1 Jul 2021 09:37:26 +0300
From:   Amir Goldstein <amir73il@...il.com>
To:     Gabriel Krisman Bertazi <krisman@...labora.com>
Cc:     "Darrick J. Wong" <djwong@...nel.org>,
        Theodore Tso <tytso@....edu>,
        Dave Chinner <david@...morbit.com>, Jan Kara <jack@...e.com>,
        David Howells <dhowells@...hat.com>,
        Khazhismel Kumykov <khazhy@...gle.com>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        Ext4 <linux-ext4@...r.kernel.org>, kernel@...labora.com
Subject: Re: [PATCH v3 12/15] fanotify: Introduce FAN_FS_ERROR event

On Wed, Jun 30, 2021 at 8:43 PM Gabriel Krisman Bertazi
<krisman@...labora.com> wrote:
>
> Amir Goldstein <amir73il@...il.com> writes:
>
> >> +       fee->fsid = fee->mark->connector->fsid;
> >> +
> >> +       fsnotify_get_mark(fee->mark);
> >> +
> >> +       /*
> >> +        * Error reporting needs to happen in atomic context.  If this
> >> +        * inode's file handler is more than we initially predicted,
> >> +        * there is nothing better we can do than report the error with
> >> +        * a bad FH.
> >> +        */
> >> +       fh_len = fanotify_encode_fh_len(inode);
> >> +       if (WARN_ON(fh_len > fee->max_fh_len))
> >
> > WARN_ON() is not acceptable for things that can logically happen
> > if you think this is important you could use pr_warn_ratelimited()
> > like we do in fanotify_encode_fh(),
> > but since fs-monitor will observe the lack of FID anyway, I think
> > there is little point in reporting this to kmsg.
>
> Hi Amir,
>
> Thanks for all the review so far.
>
> Consider that fh_len > max_fh_len can happen only if the filesystem
> requires a longer handler for the failed inode than it requires for the
> root inode.  Looking at the FH types, I don't think this would be
> possible to happen currently, but this WARN_ON is trying to catch future
> problems.
>

Don't get confused by FH types. A filesystem is not obliged to
return a uniform and single handle_type nor uniform handle_size.
Overlayfs FH size depends on the FH size of the fs in the layer
the file is on, which may be different for different files.

> Notice this would not be a fs-monitor misuse of the uAPI,  but an actual
> kernel bug. The FH size we predicted when allocating the static error
> slot is not large enough for at least one FH of this filesystem.  So I
> think a WARN_ON or a pr_warn is desired.  I will change it to a
> pr_warn_ratelimited as you suggested.
>

It would be a very minor kernel bug.
It would mean that there is a filesystem that matters in practice
for error reporting with different sizes of FH which you did not
take into account.

There is also a solution, but I think it is an overkill -
If you follow my suggestion to recreate the mark error event
on dequeue, you can update max_fh_len and re-created the
next event with larger buffer size.

In that case, admin will only see a few  pr_warn_ratelimited()
messages until fs-monitors reads the overflowed error event.

Also, I think it would be wise to use the NULL-FID convention
with different handle_types to report the different cases of:
- Failed encode (FILEID_INVALID)
- No inode (FILEID_ROOT)

Also, better use FANOTIFY_INLINE_FH_LEN as mimimum
for error event buffer size.

Thanks,
Amir.