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
| ||
|
Message-ID: <CAOQ4uxiiFsu-cG89i_PA+kqUp8ycmewhuD9xJBgpuBy5AahG5Q@mail.gmail.com> Date: Wed, 13 Nov 2024 00:06:07 +0100 From: Amir Goldstein <amir73il@...il.com> To: Linus Torvalds <torvalds@...ux-foundation.org> Cc: 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 Tue, Nov 12, 2024 at 9:12 PM Linus Torvalds <torvalds@...ux-foundation.org> wrote: > > On Tue, 12 Nov 2024 at 09:56, Josef Bacik <josef@...icpanda.com> wrote: > > > > #ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS > > +static inline int fsnotify_pre_content(struct file *file) > > +{ > > + struct inode *inode = file_inode(file); > > + > > + /* > > + * Pre-content events are only reported for regular files and dirs > > + * if there are any pre-content event watchers on this sb. > > + */ > > + if ((!S_ISDIR(inode->i_mode) && !S_ISREG(inode->i_mode)) || > > + !(inode->i_sb->s_iflags & SB_I_ALLOW_HSM) || > > + !fsnotify_sb_has_priority_watchers(inode->i_sb, > > + FSNOTIFY_PRIO_PRE_CONTENT)) > > + return 0; > > + > > + return fsnotify_file(file, FS_PRE_ACCESS); > > +} > > Yeah, no. > > None of this should check inode->i_sb->s_iflags at any point. > > The "is there a pre-content" thing should check one thing, and one > thing only: that "is this file watched" flag. > The whole indecipherable mess of inline functions that do random > things in <linux/fsnotify.h> needs to be cleaned up, not made even > more indecipherable. > > I'm NAKing this whole series until this is all sane and cleaned up, > and I don't want to see a new hacky version being sent out tomorrow > with just another layer of new hacks, with random new inline functions > that call other inline functions and have complex odd conditionals > that make no sense. > > Really. If the new hooks don't have that *SINGLE* bit test, they will > not get merged. > > And that *SINGLE* bit test had better not be hidden under multiple > layers of odd inline functions. > > You DO NOT get to use the same old broken complex function for the new > hooks that then mix these odd helpers. > > This whole "add another crazy inline function using another crazy > helper needs to STOP. Later on in the patch series you do > > +/* > + * fsnotify_truncate_perm - permission hook before file truncate > + */ > +static inline int fsnotify_truncate_perm(const struct path *path, > loff_t length) > +{ > + return fsnotify_pre_content(path, &length, 0); > +} > > or things like this: > > +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); > +} > > and no, NONE of that should be tested at runtime. > > I repeat: you should have *ONE* inline function that basically does > > static inline bool fsnotify_file_watched(struct file *file) > { > return file && unlikely(file->f_mode & FMODE_NOTIFY_PERM); > } > > and absolutely nothing else. If that file is set, the file has > notification events, and you go to an out-of-line slow case. You don't > inline the unlikely cases after that. > > And you make sure that you only set that special bit on files and > filesystems that support it. You most definitely don't check for > SB_I_ALLOW_HSM kind of flags at runtime in critical code. I understand your point. It makes sense. But it requires using another FMODE_HSM flag, because FMODE_NOTIFY_PERM covers also the legacy FS_ACCESS_PERM event, which has different semantics that I consider broken, but it is what it is. I am fine not optimizing out the legacy FS_ACCESS_PERM event and just making sure not to add new bad code, if that is what you prefer and I also am fine with using two FMODE_ flags if that is prefered. Thanks, Amir.
Powered by blists - more mailing lists