linux-kernel - Re: [PATCH] fsnotify, lsm: Separate fsnotify_open_perm() and security_file

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: <CAOQ4uxhg8zEZ4iOpUigv1paHMXvZNCFv_qTNfg-1CcehjE+7oA@mail.gmail.com>
Date: Sat, 12 Oct 2024 09:09:07 +0200
From: Amir Goldstein <amir73il@...il.com>
To: Paul Moore <paul@...l-moore.com>
Cc: Song Liu <song@...nel.org>, linux-fsdevel@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-security-module@...r.kernel.org, 
	viro@...iv.linux.org.uk, brauner@...nel.org, jack@...e.cz, jmorris@...ei.org, 
	serge@...lyn.com, kernel-team@...com
Subject: Re: [PATCH] fsnotify, lsm: Separate fsnotify_open_perm() and security_file_open()

On Fri, Oct 11, 2024 at 11:42 PM Paul Moore <paul@...l-moore.com> wrote:
>
> On Oct 11, 2024 Song Liu <song@...nel.org> wrote:
> >
> > Currently, fsnotify_open_perm() is called from security_file_open(). This
> > is not right for CONFIG_SECURITY=n and CONFIG_FSNOTIFY=y case, as
> > security_file_open() in this combination will be a no-op and not call
> > fsnotify_open_perm(). Fix this by calling fsnotify_open_perm() directly.
> >
> > Signed-off-by: Song Liu <song@...nel.org>
> > ---
> > PS: I didn't included a Fixes tag. This issue was probably introduced 15
> > years ago in [1]. If we want to back port this to stable, we will need
> > another version for older kernel because of [2].
> >
> > [1] c4ec54b40d33 ("fsnotify: new fsnotify hooks and events types for access decisions")
> > [2] 36e28c42187c ("fsnotify: split fsnotify_perm() into two hooks")
> > ---
> >  fs/open.c           | 4 ++++
> >  security/security.c | 9 +--------
> >  2 files changed, 5 insertions(+), 8 deletions(-)

Nice cleanup, but please finish off the coupling of lsm/fsnotify altogether.
I would either change the title to "decouple fsnotify from lsm" or
submit an additional patch with that title.

diff --git a/fs/notify/fanotify/Kconfig b/fs/notify/fanotify/Kconfig
index a511f9d8677b..0e36aaf379b7 100644
--- a/fs/notify/fanotify/Kconfig
+++ b/fs/notify/fanotify/Kconfig
@@ -15,7 +15,6 @@ config FANOTIFY
 config FANOTIFY_ACCESS_PERMISSIONS
        bool "fanotify permissions checking"
        depends on FANOTIFY
-       depends on SECURITY
        default n
        help
           Say Y here is you want fanotify listeners to be able to
make permissions
diff --git a/security/security.c b/security/security.c
index 6875eb4a59fc..8d238ffdeb4a 100644
--- a/security/security.c
+++ b/security/security.c
@@ -19,7 +19,6 @@
 #include <linux/kernel.h>
 #include <linux/kernel_read_file.h>
 #include <linux/lsm_hooks.h>
-#include <linux/fsnotify.h>
 #include <linux/mman.h>
 #include <linux/mount.h>
 #include <linux/personality.h>

>
> This looks fine to me, if we can get an ACK from the VFS folks I can
> merge this into the lsm/stable-6.12 tree and send it to Linus, or the
> VFS folks can do it if they prefer (my ACK is below just in case).

My preference would be to take this via the vfs or fsnotify tree.

>
> As far as stable prior to v6.8 is concerned, once this hits Linus'
> tree you can submit an adjusted backport for the older kernels to the
> stable team.

Please do NOT submit an adjustable backport.
Instead please include the following tags for the decoupling patch:

Depends-on: 36e28c42187c fsnotify: split fsnotify_perm() into two hooks
Depends-on: d9e5d31084b0 fsnotify: optionally pass access range in
file permission hooks

>
> Acked-by: Paul Moore <paul@...l-moore.com>
>

Thanks,
Amir.