diff mbox series

[v7,01/18] fsnotify: opt-in for permission events at file_open_perm() time

Message ID 8c8e9452d153a1918470cbe52a8eb6505c675911.1731433903.git.josef@toxicpanda.com (mailing list archive)
State New
Headers show
Series fanotify: add pre-content hooks | expand

Commit Message

Josef Bacik Nov. 12, 2024, 5:55 p.m. UTC 
From: Amir Goldstein <amir73il@gmail.com>

Legacy inotify/fanotify listeners can add watches for events on inode,
parent or mount and expect to get events (e.g. FS_MODIFY) on files that
were already open at the time of setting up the watches.

fanotify permission events are typically used by Anti-malware sofware,
that is watching the entire mount and it is not common to have more that
one Anti-malware engine installed on a system.

To reduce the overhead of the fsnotify_file_perm() hooks on every file
access, relax the semantics of the legacy FAN_OPEN_PERM event to generate
events only if there were *any* permission event listeners on the
filesystem at the time that the file was open.

The new semantics, implemented with the opt-in FMODE_NOTIFY_PERM flag
are also going to apply to the new fanotify pre-content event in order
to reduce the cost of the pre-content event vfs hooks.

Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Link: https://lore.kernel.org/linux-fsdevel/CAHk-=wj8L=mtcRTi=NECHMGfZQgXOp_uix1YVh04fEmrKaMnXA@mail.gmail.com/
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 include/linux/fs.h       |  3 ++-
 include/linux/fsnotify.h | 47 ++++++++++++++++++++++++++++------------
 2 files changed, 35 insertions(+), 15 deletions(-)

Comments

Linus Torvalds Nov. 12, 2024, 7:45 p.m. UTC | #1 
On Tue, 12 Nov 2024 at 09:56, Josef Bacik <josef@toxicpanda.com> wrote:
>
> @@ -119,14 +118,37 @@ static inline int fsnotify_file(struct file *file, __u32 mask)
>          * handle creation / destruction events and not "real" file events.
>          */
>         if (file->f_mode & (FMODE_NONOTIFY | FMODE_PATH))
> +               return false;
> +
> +       /* Permission events require that watches are set before FS_OPEN_PERM */
> +       if (mask & ALL_FSNOTIFY_PERM_EVENTS & ~FS_OPEN_PERM &&
> +           !(file->f_mode & FMODE_NOTIFY_PERM))
> +               return false;

This still all looks very strange.

As far as I can tell, there is exactly one user of FS_OPEN_PERM in
'mask', and that's fsnotify_open_perm(). Which is called in exactly
one place: security_file_open(), which is the wrong place to call it
anyway and is the only place where fsnotify is called from the
security layer.

In fact, that looks like an active bug: if you enable FSNOTIFY, but
you *don't* enable CONFIG_SECURITY, the whole fsnotify_open_perm()
will never be called at all.

And I just verified that yes, you can very much generate such a config.

So the whole FS_OPEN_PERM thing looks like a special case, called from
a (broken) special place, and now polluting this "fsnotify_file()"
logic for no actual reason and making it all look unnecessarily messy.

I'd suggest that the whole fsnotify_open_perm() simply be moved to
where it *should* be - in the open path - and not make a bad and
broken attempt at hiding inside the security layer, and not use this
"fsnotify_file()" logic at all.

The open-time logic is different. It shouldn't even attempt - badly -
to look like it's the same thing as some regular file access.

              Linus
Amir Goldstein Nov. 12, 2024, 10:37 p.m. UTC | #2 
On Tue, Nov 12, 2024 at 8:46 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Tue, 12 Nov 2024 at 09:56, Josef Bacik <josef@toxicpanda.com> wrote:
> >
> > @@ -119,14 +118,37 @@ static inline int fsnotify_file(struct file *file, __u32 mask)
> >          * handle creation / destruction events and not "real" file events.
> >          */
> >         if (file->f_mode & (FMODE_NONOTIFY | FMODE_PATH))
> > +               return false;
> > +
> > +       /* Permission events require that watches are set before FS_OPEN_PERM */
> > +       if (mask & ALL_FSNOTIFY_PERM_EVENTS & ~FS_OPEN_PERM &&
> > +           !(file->f_mode & FMODE_NOTIFY_PERM))
> > +               return false;
>
> This still all looks very strange.
>
> As far as I can tell, there is exactly one user of FS_OPEN_PERM in
> 'mask', and that's fsnotify_open_perm(). Which is called in exactly
> one place: security_file_open(), which is the wrong place to call it
> anyway and is the only place where fsnotify is called from the
> security layer.
>
> In fact, that looks like an active bug: if you enable FSNOTIFY, but
> you *don't* enable CONFIG_SECURITY, the whole fsnotify_open_perm()
> will never be called at all.
>
> And I just verified that yes, you can very much generate such a config.
>

See: 1cda52f1b461 fsnotify, lsm: Decouple fsnotify from lsm
in linux-next. This patch set is based on the fs-next branch.

> So the whole FS_OPEN_PERM thing looks like a special case, called from
> a (broken) special place, and now polluting this "fsnotify_file()"
> logic for no actual reason and making it all look unnecessarily messy.
>
> I'd suggest that the whole fsnotify_open_perm() simply be moved to
> where it *should* be - in the open path - and not make a bad and
> broken attempt at hiding inside the security layer, and not use this
> "fsnotify_file()" logic at all.
>
> The open-time logic is different. It shouldn't even attempt - badly -
> to look like it's the same thing as some regular file access.
>

OK, we can move setting the FMODE_NOTIFY_PERM to the open path.
I have considered that it may be better to unhide it, but wasn't sure.

Thanks,
Amir.
diff mbox series

Patch

diff --git a/include/linux/fs.h b/include/linux/fs.h
index 9c13222362f5..9b58e9887e4b 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -173,7 +173,8 @@  typedef int (dio_iodone_t)(struct kiocb *iocb, loff_t offset,
 
 #define	FMODE_NOREUSE		((__force fmode_t)(1 << 23))
 
-/* FMODE_* bit 24 */
+/* File may generate fanotify access permission events */
+#define FMODE_NOTIFY_PERM	((__force fmode_t)(1 << 24))
 
 /* File is embedded in backing_file object */
 #define FMODE_BACKING		((__force fmode_t)(1 << 25))
diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h
index 278620e063ab..f0fd3dcae654 100644
--- a/include/linux/fsnotify.h
+++ b/include/linux/fsnotify.h
@@ -108,10 +108,9 @@  static inline void fsnotify_dentry(struct dentry *dentry, __u32 mask)
 	fsnotify_parent(dentry, mask, dentry, FSNOTIFY_EVENT_DENTRY);
 }
 
-static inline int fsnotify_file(struct file *file, __u32 mask)
+/* Should events be generated on this open file regardless of watches? */
+static inline bool fsnotify_file_watchable(struct file *file, __u32 mask)
 {
-	const struct path *path;
-
 	/*
 	 * FMODE_NONOTIFY are fds generated by fanotify itself which should not
 	 * generate new events. We also don't want to generate events for
@@ -119,14 +118,37 @@  static inline int fsnotify_file(struct file *file, __u32 mask)
 	 * handle creation / destruction events and not "real" file events.
 	 */
 	if (file->f_mode & (FMODE_NONOTIFY | FMODE_PATH))
+		return false;
+
+	/* Permission events require that watches are set before FS_OPEN_PERM */
+	if (mask & ALL_FSNOTIFY_PERM_EVENTS & ~FS_OPEN_PERM &&
+	    !(file->f_mode & FMODE_NOTIFY_PERM))
+		return false;
+
+	return true;
+}
+
+static inline int fsnotify_file(struct file *file, __u32 mask)
+{
+	const struct path *path;
+
+	if (!fsnotify_file_watchable(file, mask))
 		return 0;
 
 	path = &file->f_path;
-	/* Permission events require group prio >= FSNOTIFY_PRIO_CONTENT */
-	if (mask & ALL_FSNOTIFY_PERM_EVENTS &&
-	    !fsnotify_sb_has_priority_watchers(path->dentry->d_sb,
-					       FSNOTIFY_PRIO_CONTENT))
-		return 0;
+	/*
+	 * Permission events require group prio >= FSNOTIFY_PRIO_CONTENT.
+	 * Unless permission event watchers exist at FS_OPEN_PERM time,
+	 * operations on file will not be generating any permission events.
+	 */
+	if (mask & ALL_FSNOTIFY_PERM_EVENTS) {
+		if (!fsnotify_sb_has_priority_watchers(path->dentry->d_sb,
+						       FSNOTIFY_PRIO_CONTENT))
+			return 0;
+
+		if (mask & FS_OPEN_PERM)
+			file->f_mode |= FMODE_NOTIFY_PERM;
+	}
 
 	return fsnotify_parent(path->dentry, mask, path, FSNOTIFY_EVENT_PATH);
 }
@@ -166,15 +188,12 @@  static inline int fsnotify_file_perm(struct file *file, int perm_mask)
  */
 static inline int fsnotify_open_perm(struct file *file)
 {
-	int ret;
+	int ret = fsnotify_file(file, FS_OPEN_PERM);
 
-	if (file->f_flags & __FMODE_EXEC) {
+	if (!ret && file->f_flags & __FMODE_EXEC)
 		ret = fsnotify_file(file, FS_OPEN_EXEC_PERM);
-		if (ret)
-			return ret;
-	}
 
-	return fsnotify_file(file, FS_OPEN_PERM);
+	return ret;
 }
 
 #else