linux-ext4 - Re: [PATCH v8 10/19] fanotify: introduce FAN_PRE

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: <CAOQ4uxjSQQr88W9z7xK5LbotvpiPon8nk=L33J5Z5RamEGHNCw@mail.gmail.com>
Date: Wed, 27 Nov 2024 13:20:52 +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 10/19] fanotify: introduce FAN_PRE_ACCESS permission event

On Wed, Nov 27, 2024 at 1:18 PM Jan Kara <jack@...e.cz> wrote:
>
> On Fri 22-11-24 14:51:23, Amir Goldstein wrote:
> > On Fri, Nov 22, 2024 at 1:42 PM Jan Kara <jack@...e.cz> wrote:
> > >
> > > On Thu 21-11-24 19:37:43, Amir Goldstein wrote:
> > > > On Thu, Nov 21, 2024 at 7:31 PM Amir Goldstein <amir73il@...il.com> wrote:
> > > > > On Thu, Nov 21, 2024 at 5:36 PM Jan Kara <jack@...e.cz> wrote:
> > > > > > On Thu 21-11-24 15:18:36, Amir Goldstein wrote:
> > > > > > > On Thu, Nov 21, 2024 at 11:44 AM Jan Kara <jack@...e.cz> wrote:
> > > > > > > and also always emitted ACCESS_PERM.
> > > > > >
> > > > > > I know that and it's one of those mostly useless events AFAICT.
> > > > > >
> > > > > > > my POC is using that PRE_ACCESS to populate
> > > > > > > directories on-demand, although the functionality is incomplete without the
> > > > > > > "populate on lookup" event.
> > > > > >
> > > > > > Exactly. Without "populate on lookup" doing "populate on readdir" is ok for
> > > > > > a demo but not really usable in practice because you can get spurious
> > > > > > ENOENT from a lookup.
> > > > > >
> > > > > > > > avoid the mistake of original fanotify which had some events available on
> > > > > > > > directories but they did nothing and then you have to ponder hard whether
> > > > > > > > you're going to break userspace if you actually start emitting them...
> > > > > > >
> > > > > > > But in any case, the FAN_ONDIR built-in filter is applicable to PRE_ACCESS.
> > > > > >
> > > > > > Well, I'm not so concerned about filtering out uninteresting events. I'm
> > > > > > more concerned about emitting the event now and figuring out later that we
> > > > > > need to emit it in different places or with some other info when actual
> > > > > > production users appear.
> > > > > >
> > > > > > But I've realized we must allow pre-content marks to be placed on dirs so
> > > > > > that such marks can be placed on parents watching children. What we'd need
> > > > > > to forbid is a combination of FAN_ONDIR and FAN_PRE_ACCESS, wouldn't we?
> > > > >
> > > > > Yes, I think that can work well for now.
> > > > >
> > > >
> > > > Only it does not require only check at API time that both flags are not
> > > > set, because FAN_ONDIR can be set earlier and then FAN_PRE_ACCESS
> > > > can be added later and vice versa, so need to do this in
> > > > fanotify_may_update_existing_mark() AFAICT.
> > >
> > > I have now something like:
> > >
> > > @@ -1356,7 +1356,7 @@ static int fanotify_group_init_error_pool(struct fsnotify_group *group)
> > >  }
> > >
> > >  static int fanotify_may_update_existing_mark(struct fsnotify_mark *fsn_mark,
> > > -                                             unsigned int fan_flags)
> > > +                                            __u32 mask, unsigned int fan_flags)
> > >  {
> > >         /*
> > >          * Non evictable mark cannot be downgraded to evictable mark.
> > > @@ -1383,6 +1383,11 @@ static int fanotify_may_update_existing_mark(struct fsnotify_mark *fsn_mark,
> > >             fsn_mark->flags & FSNOTIFY_MARK_FLAG_IGNORED_SURV_MODIFY)
> > >                 return -EEXIST;
> > >
> > > +       /* For now pre-content events are not generated for directories */
> > > +       mask |= fsn_mark->mask;
> > > +       if (mask & FANOTIFY_PRE_CONTENT_EVENTS && mask & FAN_ONDIR)
> > > +               return -EEXIST;
> > > +
> >
> > EEXIST is going to be confusing if there was never any mark.
> > Either return -EINVAL here or also check this condition on the added mask
> > itself before calling fanotify_add_mark() and return -EINVAL there.
> >
> > I prefer two distinct errors, but probably one is also good enough.
>
> That's actually a good point. My previous change allowed setting
> FAN_PRE_ACCESS | FAN_ONDIR on a new mark because that doesn't get to
> fanotify_may_update_existing_mark(). So I now have:
>
> diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> index 0919ea735f4a..38a46865408e 100644
> --- a/fs/notify/fanotify/fanotify_user.c
> +++ b/fs/notify/fanotify/fanotify_user.c
> @@ -1356,7 +1356,7 @@ static int fanotify_group_init_error_pool(struct fsnotify_group *group)
>  }
>
>  static int fanotify_may_update_existing_mark(struct fsnotify_mark *fsn_mark,
> -                                             unsigned int fan_flags)
> +                                            __u32 mask, unsigned int fan_flags)
>  {
>         /*
>          * Non evictable mark cannot be downgraded to evictable mark.
> @@ -1383,6 +1383,11 @@ static int fanotify_may_update_existing_mark(struct fsnotify_mark *fsn_mark,
>             fsn_mark->flags & FSNOTIFY_MARK_FLAG_IGNORED_SURV_MODIFY)
>                 return -EEXIST;
>
> +       /* For now pre-content events are not generated for directories */
> +       mask |= fsn_mark->mask;
> +       if (mask & FANOTIFY_PRE_CONTENT_EVENTS && mask & FAN_ONDIR)
> +               return -EEXIST;
> +
>         return 0;
>  }
>
> @@ -1409,7 +1414,7 @@ static int fanotify_add_mark(struct fsnotify_group *group,
>         /*
>          * Check if requested mark flags conflict with an existing mark flags.
>          */
> -       ret = fanotify_may_update_existing_mark(fsn_mark, fan_flags);
> +       ret = fanotify_may_update_existing_mark(fsn_mark, mask, fan_flags);
>         if (ret)
>                 goto out;
>
> @@ -1905,6 +1910,10 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,
>         if (mask & FAN_RENAME && !(fid_mode & FAN_REPORT_NAME))
>                 goto fput_and_out;
>
> +       /* Pre-content events are not currently generated for directories. */
> +       if (mask & FANOTIFY_PRE_CONTENT_EVENTS && mask & FAN_ONDIR)
> +               goto fput_and_out;
> +
>         if (mark_cmd == FAN_MARK_FLUSH) {
>                 ret = 0;
>                 if (mark_type == FAN_MARK_MOUNT)
> --
> 2.35.3
>

Looks good.

Thanks,
Amir.