linux-ext4 - Re: [PATCH v6 06/17] fsnotify: generate pre-content permission event on open

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: <CAOQ4uxihtPaKT02CSS37DW0JXTPFFnfaRrH781oKHzbpuBC8vw@mail.gmail.com>
Date: Tue, 12 Nov 2024 15:42:54 +0100
From: Amir Goldstein <amir73il@...il.com>
To: Jan Kara <jack@...e.cz>
Cc: Linus Torvalds <torvalds@...ux-foundation.org>, Josef Bacik <josef@...icpanda.com>, 
	kernel-team@...com, linux-fsdevel@...r.kernel.org, brauner@...nel.org, 
	linux-xfs@...r.kernel.org, linux-btrfs@...r.kernel.org, linux-mm@...ck.org, 
	linux-ext4@...r.kernel.org, kernel test robot <oliver.sang@...el.com>
Subject: Re: [PATCH v6 06/17] fsnotify: generate pre-content permission event
 on open

On Tue, Nov 12, 2024 at 2:55 PM Jan Kara <jack@...e.cz> wrote:
>
> On Tue 12-11-24 09:11:32, Amir Goldstein wrote:
> > On Tue, Nov 12, 2024 at 1:37 AM Linus Torvalds
> > <torvalds@...ux-foundation.org> wrote:
> > > On Mon, 11 Nov 2024 at 16:00, Amir Goldstein <amir73il@...il.com> wrote:
> > > >
> > > > I think that's a good idea for pre-content events, because it's fine
> > > > to say that if the sb/mount was not watched by a pre-content event listener
> > > > at the time of file open, then we do not care.
> > >
> > > Right.
> > >
> > > > The problem is that legacy inotify/fanotify watches can be added after
> > > > file is open, so that is allegedly why this optimization was not done for
> > > > fsnotify hooks in the past.
> > >
> > > So honestly, even if the legacy fsnotify hooks can't look at the file
> > > flag, they could damn well look at an inode flag.
> >
> > Legacy fanotify has a mount watch (FAN_MARK_MOUNT),
> > which is the common way for Anti-malware to set watches on
> > filesystems, so I am not sure what you are saying.
> >
> > > And I'm not even convinced that we couldn't fix them to just look at a
> > > file flag, and say "tough luck, somebody opened that file before you
> > > started watching, you don't get to see what they did".
> >
> > That would specifically break tail -f (for inotify) and probably many other
> > tools, but as long as we also look at the inode flags (i_fsnotify_mask)
> > and the dentry flags (DCACHE_FSNOTIFY_PARENT_WATCHED),
> > then I think we may be able to get away with changing the semantics
> > for open files on a fanotify mount watch.
>
> Yes, I agree we cannot afford to generate FS_MODIFY event only if the mark
> was placed after file open. There's too much stuff in userspace depending
> on this since this behavior dates back to inotify interface sometime in
> 2010 or so.
>
> > Specifically, I would really like to eliminate completely the cost of
> > FAN_ACCESS_PERM event, which could be gated on file flag, because
> > this is only for security/Anti-malware and I don't think this event is
> > practically
> > useful and it sure does not need to guarantee permission events to mount
> > watchers on already open files.
>
> For traditional fanotify permission events I agree generating them only if
> the mark was placed before open is likely fine but we'll have to try and
> see whether something breaks. For the new pre-content events I like the
> per-file flag as Linus suggested. That should indeed save us some cache
> misses in some fast paths.

FWIW, attached a patch that implements FMODE_NOTIFY_PERM
I have asked Oliver to run his performance tests to see if we can
observe an improvement with existing workloads, but is sure is going
to be useful for pre-content events.

For example, here is what the pre content helper looks like after
I adapted Josef's patches to use the flag:

static inline bool fsnotify_file_has_pre_content_watches(struct file *file)
{
        if (!(file->f_mode & FMODE_NOTIFY_PERM))
                return false;

        if (!(file_inode(file)->i_sb->s_iflags & SB_I_ALLOW_HSM))
                return false;

        return fsnotify_file_object_watched(file, FSNOTIFY_PRE_CONTENT_EVENTS);
}

Thanks,
Amir.

View attachment "0001-fsnotify-opt-in-for-permission-events-at-file_open_p.patch" of type "text/x-patch" (4545 bytes)