Thread (5 messages) 5 messages, 5 authors, 2024-10-14

Re: [PATCH] fsnotify, lsm: Separate fsnotify_open_perm() and security_file_open()

From: Song Liu <hidden>
Date: 2024-10-12 17:26:56
Also in: linux-fsdevel, lkml

Hi Amir, 

Thanks for the review. 
quoted hunk ↗ jump to hunk
On Oct 12, 2024, at 12:09 AM, Amir Goldstein [off-list ref] wrote:

On Fri, Oct 11, 2024 at 11:42 PM Paul Moore [off-list ref] wrote:
quoted
On Oct 11, 2024 Song Liu [off-list ref] wrote:
quoted
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@kernel.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
I will send v2 with this change. 
quoted hunk ↗ jump to hunk
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>
quoted
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.
quoted
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
IIUC, FANOTIFY_ACCESS_PERMISSIONS is the only user of FS_OPEN_EXEC_PERM
and FS_OPEN_PERM. In this case, I think we don't need to back port this
to stable, because there is no user of fsnotify_open_perm without 
CONFIG_SECURITY. Did I miss something?

Thanks,
Song

Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help