| Message ID | 20241011203722.3749850-1-song@kernel.org (mailing list archive) |
|---|---|
| State | Under Review |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | fsnotify, lsm: Separate fsnotify_open_perm() and security_file_open() | expand |
On Oct 11, 2024 Song Liu <song@kernel.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@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(-) 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). 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. Acked-by: Paul Moore <paul@paul-moore.com> -- paul-moore.com
On Fri, Oct 11, 2024 at 11:42 PM Paul Moore <paul@paul-moore.com> wrote: > > On Oct 11, 2024 Song Liu <song@kernel.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@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 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@paul-moore.com> > Thanks, Amir.
Hi Amir, Thanks for the review. > On Oct 12, 2024, at 12:09 AM, Amir Goldstein <amir73il@gmail.com> wrote: > > On Fri, Oct 11, 2024 at 11:42 PM Paul Moore <paul@paul-moore.com> wrote: >> >> On Oct 11, 2024 Song Liu <song@kernel.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@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. > 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 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
On Fri 11-10-24 13:37:22, Song Liu 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@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]. Well, this is not a problem because CONFIG_FANOTIFY_ACCESS_PERMISSIONS has "depends on SECURITY". So fsnotify_open_perm() can have anything to do only if CONFIG_SECURITY is enabled. That being said I agree it makes sense to decouple security & fsnotify because there's no significant benefit and only confusion. So I like the patch but please update the changelog and also finish the split as Amir suggests. Honza > [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(-) > > diff --git a/fs/open.c b/fs/open.c > index acaeb3e25c88..6c4950f19cfb 100644 > --- a/fs/open.c > +++ b/fs/open.c > @@ -946,6 +946,10 @@ static int do_dentry_open(struct file *f, > if (error) > goto cleanup_all; > > + error = fsnotify_open_perm(f); > + if (error) > + goto cleanup_all; > + > error = break_lease(file_inode(f), f->f_flags); > if (error) > goto cleanup_all; > diff --git a/security/security.c b/security/security.c > index 6875eb4a59fc..a72cc62c0a07 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> > @@ -3102,13 +3101,7 @@ int security_file_receive(struct file *file) > */ > int security_file_open(struct file *file) > { > - int ret; > - > - ret = call_int_hook(file_open, file); > - if (ret) > - return ret; > - > - return fsnotify_open_perm(file); > + return call_int_hook(file_open, file); > } > > /** > -- > 2.43.5 >
diff --git a/fs/open.c b/fs/open.c index acaeb3e25c88..6c4950f19cfb 100644 --- a/fs/open.c +++ b/fs/open.c @@ -946,6 +946,10 @@ static int do_dentry_open(struct file *f, if (error) goto cleanup_all; + error = fsnotify_open_perm(f); + if (error) + goto cleanup_all; + error = break_lease(file_inode(f), f->f_flags); if (error) goto cleanup_all; diff --git a/security/security.c b/security/security.c index 6875eb4a59fc..a72cc62c0a07 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> @@ -3102,13 +3101,7 @@ int security_file_receive(struct file *file) */ int security_file_open(struct file *file) { - int ret; - - ret = call_int_hook(file_open, file); - if (ret) - return ret; - - return fsnotify_open_perm(file); + return call_int_hook(file_open, file); } /**
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(-)