linux-ext4 - Re: [PATCH v8 02/19] fsnotify: opt-in for permission events at file open time

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: <CAOQ4uxgjOZN_=BM3DuLLZ8Vzdh-q7NYKhMnF0p_NveYd=e7vdA@mail.gmail.com>
Date: Wed, 20 Nov 2024 17:12:21 +0100
From: Amir Goldstein <amir73il@...il.com>
To: Jan Kara <jack@...e.cz>
Cc: Josef Bacik <josef@...icpanda.com>, kernel-team@...com, linux-fsdevel@...r.kernel.org, 
	brauner@...nel.org, torvalds@...ux-foundation.org, viro@...iv.linux.org.uk, 
	linux-xfs@...r.kernel.org, linux-btrfs@...r.kernel.org, linux-mm@...ck.org, 
	linux-ext4@...r.kernel.org
Subject: Re: [PATCH v8 02/19] fsnotify: opt-in for permission events at file
 open time

On Wed, Nov 20, 2024 at 4:53 PM Jan Kara <jack@...e.cz> wrote:
>
> On Fri 15-11-24 10:30:15, Josef Bacik wrote:
> > From: Amir Goldstein <amir73il@...il.com>
> >
> > Legacy inotify/fanotify listeners can add watches for events on inode,
> > parent or mount and expect to get events (e.g. FS_MODIFY) on files that
> > were already open at the time of setting up the watches.
> >
> > fanotify permission events are typically used by Anti-malware sofware,
> > that is watching the entire mount and it is not common to have more that
> > one Anti-malware engine installed on a system.
> >
> > To reduce the overhead of the fsnotify_file_perm() hooks on every file
> > access, relax the semantics of the legacy FAN_ACCESS_PERM event to generate
> > events only if there were *any* permission event listeners on the
> > filesystem at the time that the file was opened.
> >
> > The new semantic is implemented by extending the FMODE_NONOTIFY bit into
> > two FMODE_NONOTIFY_* bits, that are used to store a mode for which of the
> > events types to report.
> >
> > This is going to apply to the new fanotify pre-content events in order
> > to reduce the cost of the new pre-content event vfs hooks.
> >
> > Suggested-by: Linus Torvalds <torvalds@...ux-foundation.org>
> > Link: https://lore.kernel.org/linux-fsdevel/CAHk-=wj8L=mtcRTi=NECHMGfZQgXOp_uix1YVh04fEmrKaMnXA@mail.gmail.com/
> > Signed-off-by: Amir Goldstein <amir73il@...il.com>
>
> FWIW I've ended up somewhat massaging this patch (see below).
>
> > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > index 23bd058576b1..8e5c783013d2 100644
> > --- a/include/linux/fs.h
> > +++ b/include/linux/fs.h
> > @@ -173,13 +173,14 @@ typedef int (dio_iodone_t)(struct kiocb *iocb, loff_t offset,
> >
> >  #define      FMODE_NOREUSE           ((__force fmode_t)(1 << 23))
> >
> > -/* FMODE_* bit 24 */
> > -
> >  /* File is embedded in backing_file object */
> > -#define FMODE_BACKING                ((__force fmode_t)(1 << 25))
> > +#define FMODE_BACKING                ((__force fmode_t)(1 << 24))
> >
> > -/* File was opened by fanotify and shouldn't generate fanotify events */
> > -#define FMODE_NONOTIFY               ((__force fmode_t)(1 << 26))
> > +/* File shouldn't generate fanotify pre-content events */
> > +#define FMODE_NONOTIFY_HSM   ((__force fmode_t)(1 << 25))
> > +
> > +/* File shouldn't generate fanotify permission events */
> > +#define FMODE_NONOTIFY_PERM  ((__force fmode_t)(1 << 26))
>
> Firstly, I've kept FMODE_NONOTIFY to stay a single bit instead of two bit
> constant. I've seen too many bugs caused by people expecting the constant
> has a single bit set when it actually had more in my life. So I've ended up
> with:
>
> +/*
> + * Together with FMODE_NONOTIFY_PERM defines which fsnotify events shouldn't be
> + * generated (see below)
> + */
> +#define FMODE_NONOTIFY         ((__force fmode_t)(1 << 25))
> +
> +/*
> + * Together with FMODE_NONOTIFY defines which fsnotify events shouldn't be
> + * generated (see below)
> + */
> +#define FMODE_NONOTIFY_PERM    ((__force fmode_t)(1 << 26))
>
> and
>
> +/*
> + * The two FMODE_NONOTIFY* define which fsnotify events should not be generated
> + * for a file. These are the possible values of (f->f_mode &
> + * FMODE_FSNOTIFY_MASK) and their meaning:
> + *
> + * FMODE_NONOTIFY - suppress all (incl. non-permission) events.
> + * FMODE_NONOTIFY_PERM - suppress permission (incl. pre-content) events.
> + * FMODE_NONOTIFY | FMODE_NONOTIFY_PERM - suppress only pre-content events.
> + */
> +#define FMODE_FSNOTIFY_MASK \
> +       (FMODE_NONOTIFY | FMODE_NONOTIFY_PERM)
> +
> +#define FMODE_FSNOTIFY_NONE(mode) \
> +       ((mode & FMODE_FSNOTIFY_MASK) == FMODE_NONOTIFY)
> +#define FMODE_FSNOTIFY_PERM(mode) \
> +       (!(mode & FMODE_NONOTIFY_PERM))

That looks incorrect -
It gives the wrong value for FMODE_NONOTIFY | FMODE_NONOTIFY_PERM

should be:
!= FMODE_NONOTIFY_PERM &&
!= FMODE_NONOTIFY

The simplicity of the single bit test is for permission events
is why I chose my model, but I understand your reasoning.

> +#define FMODE_FSNOTIFY_HSM(mode) \
> +       ((mode & FMODE_FSNOTIFY_MASK) == 0)
>
> Also I've moved file_set_fsnotify_mode() out of line into fsnotify.c. The
> function gets quite big and the call is not IMO so expensive to warrant
> inlining. Furthermore it saves exporting some fsnotify internals to modules
> (in later patches).

Sounds good.
Since you wanted to refrain from defining a two bit constant,
I wonder how you annotated for NONOTIFY_HSM case

   return FMODE_NONOTIFY | FMODE_NONOTIFY_PERM;


Thanks,
Amir.