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: <CAOQ4uxj01mrrPQMyygdyDAGpyA=K=SPH88E2tpY5RuSsqG9iiA@mail.gmail.com>
Date: Wed, 13 Nov 2024 15:36:33 +0100
From: Amir Goldstein <amir73il@...il.com>
To: Al Viro <viro@...iv.linux.org.uk>
Cc: Linus Torvalds <torvalds@...ux-foundation.org>, Josef Bacik <josef@...icpanda.com>, 
	kernel-team@...com, linux-fsdevel@...r.kernel.org, jack@...e.cz, 
	brauner@...nel.org, linux-xfs@...r.kernel.org, linux-btrfs@...r.kernel.org, 
	linux-mm@...ck.org, linux-ext4@...r.kernel.org
Subject: Re: [PATCH v7 05/18] fsnotify: introduce pre-content permission events

On Wed, Nov 13, 2024 at 5:30 AM Al Viro <viro@...iv.linux.org.uk> wrote:
>
> On Wed, Nov 13, 2024 at 01:19:54AM +0000, Al Viro wrote:
> > On Tue, Nov 12, 2024 at 04:38:42PM -0800, Linus Torvalds wrote:
> > > Looking at that locking code in fadvise() just for the f_mode use does
> > > make me think this would be a really good cleanup.
> > >
> > > I note that our fcntl code seems buggy as-is, because while it does
> > > use f_lock for assignments (good), it clearly does *not* use them for
> > > reading.
> > >
> > > So it looks like you can actually read inconsistent values.
> > >
> > > I get the feeling that f_flags would want WRITE_ONCE/READ_ONCE in
> > > _addition_ to the f_lock use it has.
> >
> > AFAICS, fasync logics is the fishy part - the rest should be sane.
> >
> > > The f_mode thing with fadvise() smells like the same bug. Just because
> > > the modifications are serialized wrt each other doesn't mean that
> > > readers are then automatically ok.
> >
> > Reads are also under ->f_lock in there, AFAICS...
> >
> > Another thing in the vicinity is ->f_mode modifications after the calls
> > of anon_inode_getfile() in several callers - probably ought to switch
> > those to anon_inode_getfile_fmode().  That had been discussed back in
> > April when the function got merged, but "convert to using it" followup
> > series hadn't materialized...
>
> While we are at it, there's is a couple of kludges I really hate -
> mixing __FMODE_NONOTIFY and __FMODE_EXEC with O_... flags.
>
> E.g. for __FMODE_NONOTIFY all it takes is switching fanotify from
> anon_inode_getfd() to anon_inode_getfile_fmode() and adding
> a dentry_open_nonotify() to be used by fanotify on the other path.
> That's it - no more weird shit in OPEN_FMODE(), etc.
>
> For __FMODE_EXEC it might get trickier (nfs is the main consumer),
> but I seriously suspect that something like "have path_openat()
> check op->acc_mode & MAY_EXEC and set FMODE_EXEC in ->f_mode
> right after struct file allocation" would make a good starting
> point; yes, it would affect uselib(2), but... I've no idea whether
> it wouldn't be the right thing to do; would be hard to test.
>
> Anyway, untested __FMODE_NONOTIFY side of it:
>
> diff --git a/fs/fcntl.c b/fs/fcntl.c
> index 22dd9dcce7ec..ebd1c82bfb6b 100644
> --- a/fs/fcntl.c
> +++ b/fs/fcntl.c
> @@ -1161,10 +1161,10 @@ static int __init fcntl_init(void)
>          * Exceptions: O_NONBLOCK is a two bit define on parisc; O_NDELAY
>          * is defined as O_NONBLOCK on some platforms and not on others.
>          */
> -       BUILD_BUG_ON(21 - 1 /* for O_RDONLY being 0 */ !=
> +       BUILD_BUG_ON(20 - 1 /* for O_RDONLY being 0 */ !=
>                 HWEIGHT32(
>                         (VALID_OPEN_FLAGS & ~(O_NONBLOCK | O_NDELAY)) |
> -                       __FMODE_EXEC | __FMODE_NONOTIFY));
> +                       __FMODE_EXEC));
>
>         fasync_cache = kmem_cache_create("fasync_cache",
>                                          sizeof(struct fasync_struct), 0,
> diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> index 9644bc72e457..43fbf29ef03a 100644
> --- a/fs/notify/fanotify/fanotify_user.c
> +++ b/fs/notify/fanotify/fanotify_user.c
> @@ -101,8 +101,7 @@ static void __init fanotify_sysctls_init(void)
>   *
>   * Internal and external open flags are stored together in field f_flags of
>   * struct file. Only external open flags shall be allowed in event_f_flags.
> - * Internal flags like FMODE_NONOTIFY, FMODE_EXEC, FMODE_NOCMTIME shall be
> - * excluded.
> + * Internal flags like FMODE_EXEC shall be excluded.
>   */
>  #define        FANOTIFY_INIT_ALL_EVENT_F_BITS                          ( \
>                 O_ACCMODE       | O_APPEND      | O_NONBLOCK    | \
> @@ -262,8 +261,8 @@ static int create_fd(struct fsnotify_group *group, const struct path *path,
>          * we need a new file handle for the userspace program so it can read even if it was
>          * originally opened O_WRONLY.
>          */
> -       new_file = dentry_open(path,
> -                              group->fanotify_data.f_flags | __FMODE_NONOTIFY,
> +       new_file = dentry_open_nonotify(path,
> +                              group->fanotify_data.f_flags,

I would make this a bit more generic helper and the comment above
is truly clueless:

        /*
-        * we need a new file handle for the userspace program so it
can read even if it was
-        * originally opened O_WRONLY.
+        * We provide an fd for the userspace program, so it could access the
+        * file without generating fanotify events itself.
         */
-       new_file = dentry_open(path,
-                              group->fanotify_data.f_flags | __FMODE_NONOTIFY,
-                              current_cred());
+       new_file = dentry_open_fmode(path, group->fanotify_data.f_flags,
+                                    FMODE_NONOTIFY, current_cred());



>                                current_cred());
>         if (IS_ERR(new_file)) {
>                 /*
> @@ -1404,6 +1403,7 @@ SYSCALL_DEFINE2(fanotify_init, unsigned int, flags, unsigned int, event_f_flags)
>         unsigned int fid_mode = flags & FANOTIFY_FID_BITS;
>         unsigned int class = flags & FANOTIFY_CLASS_BITS;
>         unsigned int internal_flags = 0;
> +       struct file *file;
>
>         pr_debug("%s: flags=%x event_f_flags=%x\n",
>                  __func__, flags, event_f_flags);
> @@ -1472,7 +1472,7 @@ SYSCALL_DEFINE2(fanotify_init, unsigned int, flags, unsigned int, event_f_flags)
>             (!(fid_mode & FAN_REPORT_NAME) || !(fid_mode & FAN_REPORT_FID)))
>                 return -EINVAL;
>
> -       f_flags = O_RDWR | __FMODE_NONOTIFY;
> +       f_flags = O_RDWR;
>         if (flags & FAN_CLOEXEC)
>                 f_flags |= O_CLOEXEC;
>         if (flags & FAN_NONBLOCK)
> @@ -1550,10 +1550,18 @@ SYSCALL_DEFINE2(fanotify_init, unsigned int, flags, unsigned int, event_f_flags)
>                         goto out_destroy_group;
>         }
>
> -       fd = anon_inode_getfd("[fanotify]", &fanotify_fops, group, f_flags);
> +       fd = get_unused_fd_flags(flags);

s/flags/f_flags

>         if (fd < 0)
>                 goto out_destroy_group;
>
> +       file = anon_inode_getfile_fmode("[fanotify]", &fanotify_fops, group,
> +                                       f_flags, FMODE_NONOTIFY);
> +       if (IS_ERR(file)) {
> +               fd = PTR_ERR(file);
> +               put_unused_fd(fd);
> +               goto out_destroy_group;
> +       }
> +       fd_install(fd, file);
>         return fd;
>
>  out_destroy_group:
> diff --git a/fs/open.c b/fs/open.c
> index acaeb3e25c88..04cb581528ff 100644
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -1118,6 +1118,23 @@ struct file *dentry_open(const struct path *path, int flags,
>  }
>  EXPORT_SYMBOL(dentry_open);
>
> +struct file *dentry_open_nonotify(const struct path *path, int flags,
> +                        const struct cred *cred)
> +{
> +       struct file *f = alloc_empty_file(flags, cred);
> +       if (!IS_ERR(f)) {
> +               int error;
> +
> +               f->f_mode |= FMODE_NONOTIFY;
> +               error = vfs_open(path, f);
> +               if (error) {
> +                       fput(f);
> +                       f = ERR_PTR(error);
> +               }
> +       }
> +       return f;
> +}
> +
>  /**
>   * dentry_create - Create and open a file
>   * @path: path to create
> @@ -1215,7 +1232,7 @@ inline struct open_how build_open_how(int flags, umode_t mode)
>  inline int build_open_flags(const struct open_how *how, struct open_flags *op)
>  {
>         u64 flags = how->flags;
> -       u64 strip = __FMODE_NONOTIFY | O_CLOEXEC;
> +       u64 strip = O_CLOEXEC;
>         int lookup_flags = 0;
>         int acc_mode = ACC_MODE(flags);
>

Get rid of another stale comment:

        /*
-        * Strip flags that either shouldn't be set by userspace like
-        * FMODE_NONOTIFY or that aren't relevant in determining struct
-        * open_flags like O_CLOEXEC.
+        * Strip flags that aren't relevant in determining struct open_flags.
         */

With these changed, you can add:

Reviewed-by: Amir Goldstein <amir73il@...il.com>

With the f_flags typo fixed, this passed LTP sanity tests, but I am
going to test the NONOTIFY functionally some more.

Thanks,
Amir.

Powered by blists - more mailing lists