[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <C4165EAD-23A7-4D62-BB31-26D872AC78EF@fb.com>
Date: Sat, 12 Oct 2024 17:26:51 +0000
From: Song Liu <songliubraving@...a.com>
To: Amir Goldstein <amir73il@...il.com>
CC: Paul Moore <paul@...l-moore.com>, Song Liu <song@...nel.org>,
Linux-Fsdevel <linux-fsdevel@...r.kernel.org>,
LKML
<linux-kernel@...r.kernel.org>,
LSM List
<linux-security-module@...r.kernel.org>,
Al Viro <viro@...iv.linux.org.uk>,
Christian Brauner <brauner@...nel.org>, Jan Kara <jack@...e.cz>,
James Morris
<jmorris@...ei.org>,
"Serge E. Hallyn" <serge@...lyn.com>,
Kernel Team
<kernel-team@...a.com>
Subject: Re: [PATCH] fsnotify, lsm: Separate fsnotify_open_perm() and
security_file_open()
Hi Amir,
Thanks for the review.
> On Oct 12, 2024, at 12:09 AM, Amir Goldstein <amir73il@...il.com> wrote:
>
> 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
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
Powered by blists - more mailing lists