| Message ID | 20241013002248.3984442-1-song@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] fsnotify, lsm: Decouple fsnotify from lsm | expand |
On Sun, Oct 13, 2024 at 2:23 AM 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. Maybe I am missing something. I like cleaner interfaces, but if it is a report of a problem then I do not understand what the problem is. IOW, what does "This is not right" mean? > > After this, CONFIG_FANOTIFY_ACCESS_PERMISSIONS does not require > CONFIG_SECURITY any more. Remove the dependency in the config. > > Signed-off-by: Song Liu <song@kernel.org> > Acked-by: Paul Moore <paul@paul-moore.com> > > --- > > v1: https://lore.kernel.org/linux-fsdevel/20241011203722.3749850-1-song@kernel.org/ > > As far as I can tell, it is necessary to back port this to stable. Because > CONFIG_FANOTIFY_ACCESS_PERMISSIONS is the only user of fsnotify_open_perm, > and CONFIG_FANOTIFY_ACCESS_PERMISSIONS depends on CONFIG_SECURITY. > Therefore, the following tags are not necessary. But I include here as > these are discussed in v1. I did not understand why you claim that the tags are or not necessary. The dependency is due to removal of the fsnotify.h include. Anyway, I don't think it is critical to backport this fix. The dependencies would probably fail to apply cleanly to older kernels, so unless somebody cares, it would stay this way. > > Fixes: c4ec54b40d33 ("fsnotify: new fsnotify hooks and events types for access decisions") Because I am not sure what the problem is, I am not sure that a Fixes: tag is called for. > Depends-on: 36e28c42187c ("fsnotify: split fsnotify_perm() into two hooks") > Depends-on: d9e5d31084b0 ("fsnotify: optionally pass access range in file permission hooks") These need to be in the commit message in case AUTOSEL or a developer would decide to backport your change. Thanks, Amir. > --- > fs/notify/fanotify/Kconfig | 1 - > fs/open.c | 4 ++++ > security/security.c | 9 +-------- > 3 files changed, 5 insertions(+), 9 deletions(-) > > 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/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 > >
Hi Amir, > On Oct 13, 2024, at 2:38 AM, Amir Goldstein <amir73il@gmail.com> wrote: > > On Sun, Oct 13, 2024 at 2:23 AM 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. > > Maybe I am missing something. > I like cleaner interfaces, but if it is a report of a problem then > I do not understand what the problem is. > IOW, what does "This is not right" mean? With existing code, CONFIG_FANOTIFY_ACCESS_PERMISSIONS depends on CONFIG_SECURITY, but CONFIG_FSNOTIFY does not depend on CONFIG_SECURITY. So CONFIG_SECURITY=n and CONFIG_FSNOTIFY=y is a valid combination. fsnotify_open_perm() is an fsnotify API, so I think it is not right to skip the API call for this config. > >> >> After this, CONFIG_FANOTIFY_ACCESS_PERMISSIONS does not require >> CONFIG_SECURITY any more. Remove the dependency in the config. >> >> Signed-off-by: Song Liu <song@kernel.org> >> Acked-by: Paul Moore <paul@paul-moore.com> >> >> --- >> >> v1: https://lore.kernel.org/linux-fsdevel/20241011203722.3749850-1-song@kernel.org/ >> >> As far as I can tell, it is necessary to back port this to stable. Because >> CONFIG_FANOTIFY_ACCESS_PERMISSIONS is the only user of fsnotify_open_perm, >> and CONFIG_FANOTIFY_ACCESS_PERMISSIONS depends on CONFIG_SECURITY. >> Therefore, the following tags are not necessary. But I include here as >> these are discussed in v1. > > I did not understand why you claim that the tags are or not necessary. > The dependency is due to removal of the fsnotify.h include. I think the Fixes tag is also not necessary, not just the two Depends-on tags. This is because while fsnotify_open_perm() is a fsnotify API, only CONFIG_FANOTIFY_ACCESS_PERMISSIONS really uses (if I understand correctly). > > Anyway, I don't think it is critical to backport this fix. > The dependencies would probably fail to apply cleanly to older kernels, > so unless somebody cares, it would stay this way. I agree it is not critical to back port this fix. I put the Fixes tag below "---" for this reason. Does this answer your question? Thanks, Song > >> >> Fixes: c4ec54b40d33 ("fsnotify: new fsnotify hooks and events types for access decisions") > > Because I am not sure what the problem is, I am not sure that a Fixes: > tag is called for. > >> Depends-on: 36e28c42187c ("fsnotify: split fsnotify_perm() into two hooks") >> Depends-on: d9e5d31084b0 ("fsnotify: optionally pass access range in file permission hooks") > > These need to be in the commit message in case AUTOSEL or a developer > would decide to backport your change. > > Thanks, > Amir.
On Sun, Oct 13, 2024 at 4:46 PM Song Liu <songliubraving@meta.com> wrote: > > Hi Amir, > > > On Oct 13, 2024, at 2:38 AM, Amir Goldstein <amir73il@gmail.com> wrote: > > > > On Sun, Oct 13, 2024 at 2:23 AM 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. > > > > Maybe I am missing something. > > I like cleaner interfaces, but if it is a report of a problem then > > I do not understand what the problem is. > > IOW, what does "This is not right" mean? > > With existing code, CONFIG_FANOTIFY_ACCESS_PERMISSIONS depends on > CONFIG_SECURITY, but CONFIG_FSNOTIFY does not depend on > CONFIG_SECURITY. So CONFIG_SECURITY=n and CONFIG_FSNOTIFY=y is a > valid combination. fsnotify_open_perm() is an fsnotify API, so I > think it is not right to skip the API call for this config. > > > > >> > >> After this, CONFIG_FANOTIFY_ACCESS_PERMISSIONS does not require > >> CONFIG_SECURITY any more. Remove the dependency in the config. > >> > >> Signed-off-by: Song Liu <song@kernel.org> > >> Acked-by: Paul Moore <paul@paul-moore.com> > >> > >> --- > >> > >> v1: https://lore.kernel.org/linux-fsdevel/20241011203722.3749850-1-song@kernel.org/ > >> > >> As far as I can tell, it is necessary to back port this to stable. Because > >> CONFIG_FANOTIFY_ACCESS_PERMISSIONS is the only user of fsnotify_open_perm, > >> and CONFIG_FANOTIFY_ACCESS_PERMISSIONS depends on CONFIG_SECURITY. > >> Therefore, the following tags are not necessary. But I include here as > >> these are discussed in v1. > > > > I did not understand why you claim that the tags are or not necessary. > > The dependency is due to removal of the fsnotify.h include. > > I think the Fixes tag is also not necessary, not just the two > Depends-on tags. This is because while fsnotify_open_perm() is a > fsnotify API, only CONFIG_FANOTIFY_ACCESS_PERMISSIONS really uses > (if I understand correctly). > That is correct. > > > > Anyway, I don't think it is critical to backport this fix. > > The dependencies would probably fail to apply cleanly to older kernels, > > so unless somebody cares, it would stay this way. > > I agree it is not critical to back port this fix. I put the > Fixes tag below "---" for this reason. > > Does this answer your question? > Yes, I agree with not including any of the tags and not targeting stable. Jan, Christian, do you agree with the wording of the commit message, or think that it needs to be clarified? Would you prefer this to go via the fsnotify tree or vfs tree? Thanks, Amir.
On Sun 13-10-24 16:51:35, Amir Goldstein wrote: > On Sun, Oct 13, 2024 at 4:46 PM Song Liu <songliubraving@meta.com> wrote: > > > > Hi Amir, > > > > > On Oct 13, 2024, at 2:38 AM, Amir Goldstein <amir73il@gmail.com> wrote: > > > > > > On Sun, Oct 13, 2024 at 2:23 AM 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. > > > > > > Maybe I am missing something. > > > I like cleaner interfaces, but if it is a report of a problem then > > > I do not understand what the problem is. > > > IOW, what does "This is not right" mean? > > > > With existing code, CONFIG_FANOTIFY_ACCESS_PERMISSIONS depends on > > CONFIG_SECURITY, but CONFIG_FSNOTIFY does not depend on > > CONFIG_SECURITY. So CONFIG_SECURITY=n and CONFIG_FSNOTIFY=y is a > > valid combination. fsnotify_open_perm() is an fsnotify API, so I > > think it is not right to skip the API call for this config. > > > > > > > >> > > >> After this, CONFIG_FANOTIFY_ACCESS_PERMISSIONS does not require > > >> CONFIG_SECURITY any more. Remove the dependency in the config. > > >> > > >> Signed-off-by: Song Liu <song@kernel.org> > > >> Acked-by: Paul Moore <paul@paul-moore.com> > > >> > > >> --- > > >> > > >> v1: https://lore.kernel.org/linux-fsdevel/20241011203722.3749850-1-song@kernel.org/ > > >> > > >> As far as I can tell, it is necessary to back port this to stable. Because > > >> CONFIG_FANOTIFY_ACCESS_PERMISSIONS is the only user of fsnotify_open_perm, > > >> and CONFIG_FANOTIFY_ACCESS_PERMISSIONS depends on CONFIG_SECURITY. > > >> Therefore, the following tags are not necessary. But I include here as > > >> these are discussed in v1. > > > > > > I did not understand why you claim that the tags are or not necessary. > > > The dependency is due to removal of the fsnotify.h include. > > > > I think the Fixes tag is also not necessary, not just the two > > Depends-on tags. This is because while fsnotify_open_perm() is a > > fsnotify API, only CONFIG_FANOTIFY_ACCESS_PERMISSIONS really uses > > (if I understand correctly). > > > > That is correct. > > > > > > > Anyway, I don't think it is critical to backport this fix. > > > The dependencies would probably fail to apply cleanly to older kernels, > > > so unless somebody cares, it would stay this way. > > > > I agree it is not critical to back port this fix. I put the > > Fixes tag below "---" for this reason. > > > > Does this answer your question? > > > > Yes, I agree with not including any of the tags and not targeting stable. > > Jan, Christian, > > do you agree with the wording of the commit message, or think > that it needs to be clarified? > > Would you prefer this to go via the fsnotify tree or vfs tree? I guess I'll take this through fsnotify tree after updating the changelog a bit. Honza
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/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); } /**