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

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

[Song Liu]

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(-)

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

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

[Paul Moore]

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

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

[Amir Goldstein]

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.

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

[Song Liu]

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

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

[Jan Kara]

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
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR