linux-kernel - Re: commit 0790303ec869 leads to cpu stall without CONFIG_FANOTIFY_ACCESS

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] [day] [month] [year] [list]

Message-ID: <20241210112026.7v74ig2rrmceam5o@quack3>
Date: Tue, 10 Dec 2024 12:20:26 +0100
From: Jan Kara <jack@...e.cz>
To: Amir Goldstein <amir73il@...il.com>
Cc: Jan Kara <jack@...e.cz>, Bert Karwatzki <spasswolf@....de>,
	Josef Bacik <josef@...icpanda.com>, linux-kernel@...r.kernel.org,
	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: commit 0790303ec869 leads to cpu stall without
 CONFIG_FANOTIFY_ACCESS_PERMISSIONS=y

On Mon 09-12-24 17:23:24, Amir Goldstein wrote:
> On Mon, Dec 9, 2024 at 1:26 PM Jan Kara <jack@...e.cz> wrote:
> >
> > On Mon 09-12-24 13:11:04, Jan Kara wrote:
> > > > Then I took a closer look at the function called in the problematic code
> > > > and noticed that fsnotify_file_area_perm(), is a NOOP when
> > > > CONFIG_FANOTIFY_ACCESS_PERMISSIONS is not set (which was the case in my
> > > > .config). This also explains why this was not found before, as
> > > > distributional .config file have this option enabled.  Setting the option
> > > > to y solves the issue, too
> > >
> > > Well, I agree with you on all the points but the real question is, how come
> > > the test FMODE_FSNOTIFY_HSM(file->f_mode) was true on our kernel when you
> > > clearly don't run HSM software, even more so with
> > > CONFIG_FANOTIFY_ACCESS_PERMISSIONS disabled. That's the real cause of this
> > > problem. Something fishy is going on here... checking...
> > >
> > > Ah, because I've botched out file_set_fsnotify_mode() in case
> > > CONFIG_FANOTIFY_ACCESS_PERMISSIONS is disabled. This should fix the
> > > problem:
> > >
> > > index 1a9ef8f6784d..778a88fcfddc 100644
> > > --- a/include/linux/fsnotify.h
> > > +++ b/include/linux/fsnotify.h
> > > @@ -215,6 +215,7 @@ static inline int fsnotify_open_perm(struct file *file)
> > >  #else
> > >  static inline void file_set_fsnotify_mode(struct file *file)
> > >  {
> > > +       file->f_mode |= FMODE_NONOTIFY_PERM;
> > >  }
> > >
> > > I'm going to test this with CONFIG_FANOTIFY_ACCESS_PERMISSIONS disabled and
> > > push out a fixed version. Thanks again for the report and analysis!
> >
> > So this was not enough, What we need is:
> > index 1a9ef8f6784d..778a88fcfddc 100644
> > --- a/include/linux/fsnotify.h
> > +++ b/include/linux/fsnotify.h
> > @@ -215,6 +215,10 @@ static inline int fsnotify_open_perm(struct file *file)
> >  #else
> >  static inline void file_set_fsnotify_mode(struct file *file)
> >  {
> > +       /* Is it a file opened by fanotify? */
> > +       if (FMODE_FSNOTIFY_NONE(file->f_mode))
> > +               return;
> > +       file->f_mode |= FMODE_NONOTIFY_PERM;
> >  }
> >
> > This passes testing for me so I've pushed it out and the next linux-next
> > build should have this fix.
> 
> This fix is not obvious to the code reviewer (especially when that is
> reviewer Linus...)
> Perhaps it would be safer and less hidden to do:
> 
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -211,11 +211,16 @@ typedef int (dio_iodone_t)(struct kiocb *iocb,
> loff_t offset,
> 
>  #define FMODE_FSNOTIFY_NONE(mode) \
>         ((mode & FMODE_FSNOTIFY_MASK) == FMODE_NONOTIFY)
> +#ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS
>  #define FMODE_FSNOTIFY_PERM(mode) \
>         ((mode & FMODE_FSNOTIFY_MASK) == 0 || \
>          (mode & FMODE_FSNOTIFY_MASK) == (FMODE_NONOTIFY | FMODE_NONOTIFY_PERM))
>  #define FMODE_FSNOTIFY_HSM(mode) \
>         ((mode & FMODE_FSNOTIFY_MASK) == 0)
> +#else
> +#define FMODE_FSNOTIFY_PERM(mode)      0
> +#define FMODE_FSNOTIFY_HSM(mode)       0
> +#endif

I agree this is a nicer way to achieve the same. Updated, tested & pushed
out.

								Honza
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR