| Message ID | 20250127155723.67711-2-hamzamahfooz@linux.microsoft.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v3,1/2] io_uring: refactor io_uring_allowed() | expand |
On 1/27/2025 7:57 AM, Hamza Mahfooz wrote: > It is desirable to allow LSM to configure accessibility to io_uring > because it is a coarse yet very simple way to restrict access to it. So, > add an LSM for io_uring_allowed() to guard access to io_uring. I don't like this at all at all. It looks like you're putting in a hook so that io_uring can easily deflect any responsibility for safely interacting with LSMs. > > Cc: Paul Moore <paul@paul-moore.com> > Signed-off-by: Hamza Mahfooz <hamzamahfooz@linux.microsoft.com> > --- > include/linux/lsm_hook_defs.h | 1 + > include/linux/security.h | 5 +++++ > io_uring/io_uring.c | 2 +- > security/security.c | 12 ++++++++++++ > security/selinux/hooks.c | 14 ++++++++++++++ > security/selinux/include/classmap.h | 2 +- > 6 files changed, 34 insertions(+), 2 deletions(-) > > diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h > index e2f1ce37c41e..9eb313bd0c93 100644 > --- a/include/linux/lsm_hook_defs.h > +++ b/include/linux/lsm_hook_defs.h > @@ -455,6 +455,7 @@ LSM_HOOK(int, 0, perf_event_write, struct perf_event *event) > LSM_HOOK(int, 0, uring_override_creds, const struct cred *new) > LSM_HOOK(int, 0, uring_sqpoll, void) > LSM_HOOK(int, 0, uring_cmd, struct io_uring_cmd *ioucmd) > +LSM_HOOK(int, 0, uring_allowed, void) > #endif /* CONFIG_IO_URING */ > > LSM_HOOK(void, LSM_RET_VOID, initramfs_populated, void) > diff --git a/include/linux/security.h b/include/linux/security.h > index 980b6c207cad..3e68f8468a22 100644 > --- a/include/linux/security.h > +++ b/include/linux/security.h > @@ -2362,6 +2362,7 @@ static inline int security_perf_event_write(struct perf_event *event) > extern int security_uring_override_creds(const struct cred *new); > extern int security_uring_sqpoll(void); > extern int security_uring_cmd(struct io_uring_cmd *ioucmd); > +extern int security_uring_allowed(void); > #else > static inline int security_uring_override_creds(const struct cred *new) > { > @@ -2375,6 +2376,10 @@ static inline int security_uring_cmd(struct io_uring_cmd *ioucmd) > { > return 0; > } > +extern int security_uring_allowed(void) > +{ > + return 0; > +} > #endif /* CONFIG_SECURITY */ > #endif /* CONFIG_IO_URING */ > > diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c > index c2d8bd4c2cfc..9df7b3b556ef 100644 > --- a/io_uring/io_uring.c > +++ b/io_uring/io_uring.c > @@ -3808,7 +3808,7 @@ static inline int io_uring_allowed(void) > return -EPERM; > > allowed_lsm: > - return 0; > + return security_uring_allowed(); > } > > SYSCALL_DEFINE2(io_uring_setup, u32, entries, > diff --git a/security/security.c b/security/security.c > index 143561ebc3e8..c9fae447327e 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -5999,6 +5999,18 @@ int security_uring_cmd(struct io_uring_cmd *ioucmd) > { > return call_int_hook(uring_cmd, ioucmd); > } > + > +/** > + * security_uring_allowed() - Check if io_uring_setup() is allowed > + * > + * Check whether the current task is allowed to call io_uring_setup(). > + * > + * Return: Returns 0 if permission is granted. > + */ > +int security_uring_allowed(void) > +{ > + return call_int_hook(uring_allowed); > +} > #endif /* CONFIG_IO_URING */ > > /** > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index 7b867dfec88b..fb37e87df226 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -7137,6 +7137,19 @@ static int selinux_uring_cmd(struct io_uring_cmd *ioucmd) > return avc_has_perm(current_sid(), isec->sid, > SECCLASS_IO_URING, IO_URING__CMD, &ad); > } > + > +/** > + * selinux_uring_allowed - check if io_uring_setup() can be called > + * > + * Check to see if the current task is allowed to call io_uring_setup(). > + */ > +static int selinux_uring_allowed(void) > +{ > + u32 sid = current_sid(); > + > + return avc_has_perm(sid, sid, SECCLASS_IO_URING, IO_URING__ALLOWED, > + NULL); > +} > #endif /* CONFIG_IO_URING */ > > static const struct lsm_id selinux_lsmid = { > @@ -7390,6 +7403,7 @@ static struct security_hook_list selinux_hooks[] __ro_after_init = { > LSM_HOOK_INIT(uring_override_creds, selinux_uring_override_creds), > LSM_HOOK_INIT(uring_sqpoll, selinux_uring_sqpoll), > LSM_HOOK_INIT(uring_cmd, selinux_uring_cmd), > + LSM_HOOK_INIT(uring_allowed, selinux_uring_allowed), > #endif > > /* > diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h > index 03e82477dce9..8a8f3908aac8 100644 > --- a/security/selinux/include/classmap.h > +++ b/security/selinux/include/classmap.h > @@ -177,7 +177,7 @@ const struct security_class_mapping secclass_map[] = { > { "perf_event", > { "open", "cpu", "kernel", "tracepoint", "read", "write", NULL } }, > { "anon_inode", { COMMON_FILE_PERMS, NULL } }, > - { "io_uring", { "override_creds", "sqpoll", "cmd", NULL } }, > + { "io_uring", { "override_creds", "sqpoll", "cmd", "allowed", NULL } }, > { "user_namespace", { "create", NULL } }, > /* last one */ { NULL, {} } > };
On Mon, Jan 27, 2025 at 12:18 PM Casey Schaufler <casey@schaufler-ca.com> wrote: > On 1/27/2025 7:57 AM, Hamza Mahfooz wrote: > > It is desirable to allow LSM to configure accessibility to io_uring > > because it is a coarse yet very simple way to restrict access to it. So, > > add an LSM for io_uring_allowed() to guard access to io_uring. > > I don't like this at all at all. It looks like you're putting in a hook > so that io_uring can easily deflect any responsibility for safely > interacting with LSMs. That's not how this works Casey, unless you're seeing something different? This is an additional access control point for io_uring, largely to simplify what a LSM would need to do to help control a process' access to io_uring, it does not replace any of the io_uring LSM hooks or access control points.
On 1/27/2025 1:23 PM, Paul Moore wrote: > On Mon, Jan 27, 2025 at 12:18 PM Casey Schaufler <casey@schaufler-ca.com> wrote: >> On 1/27/2025 7:57 AM, Hamza Mahfooz wrote: >>> It is desirable to allow LSM to configure accessibility to io_uring >>> because it is a coarse yet very simple way to restrict access to it. So, >>> add an LSM for io_uring_allowed() to guard access to io_uring. >> I don't like this at all at all. It looks like you're putting in a hook >> so that io_uring can easily deflect any responsibility for safely >> interacting with LSMs. > That's not how this works Casey, unless you're seeing something different? Yes, it's just me being paranoid. When a mechanism is introduced that makes it easy to disable a system feature in the LSM environment I start hearing voices saying "You can't use security and the cool thing together", and the developers of "the cool thing" wave hands and say "just disable it" and it never gets properly integrated. I have seen this so many times it makes me wonder how anything ever does get made to work in multiple configurations. > > This is an additional access control point for io_uring, largely to > simplify what a LSM would need to do to help control a process' access > to io_uring, it does not replace any of the io_uring LSM hooks or > access control points. What I see is "LSM xyzzy has an issue with io_uring? Just create a io_uring_allowed() hook that always disables io_uring." LSM xyzzy never gets attention from the io_uring developers because, as far as they care, the problem is solved.
On Mon, Jan 27, 2025 at 7:23 PM Casey Schaufler <casey@schaufler-ca.com> wrote: > On 1/27/2025 1:23 PM, Paul Moore wrote: > > On Mon, Jan 27, 2025 at 12:18 PM Casey Schaufler <casey@schaufler-ca.com> wrote: > >> On 1/27/2025 7:57 AM, Hamza Mahfooz wrote: > >>> It is desirable to allow LSM to configure accessibility to io_uring > >>> because it is a coarse yet very simple way to restrict access to it. So, > >>> add an LSM for io_uring_allowed() to guard access to io_uring. > >> I don't like this at all at all. It looks like you're putting in a hook > >> so that io_uring can easily deflect any responsibility for safely > >> interacting with LSMs. > > That's not how this works Casey, unless you're seeing something different? > > Yes, it's just me being paranoid. When a mechanism is introduced that makes > it easy to disable a system feature in the LSM environment I start hearing > voices saying "You can't use security and the cool thing together", and the > developers of "the cool thing" wave hands and say "just disable it" and it > never gets properly integrated. I have seen this so many times it makes me > wonder how anything ever does get made to work in multiple configurations. While there is plenty of precedent regarding paranoia over kernel changes outside the LSM, and yes, I do agree that there are likely some configurations that aren't tested (or make little sense for that matter), I don't believe that to be the case here. The proposed LSM hook is simply another access control, and it makes it much easier for LSMs without full and clear anonymous inode controls to apply access controls to io_uring. > > This is an additional access control point for io_uring, largely to > > simplify what a LSM would need to do to help control a process' access > > to io_uring, it does not replace any of the io_uring LSM hooks or > > access control points. > > What I see is "LSM xyzzy has an issue with io_uring? Just create a > io_uring_allowed() hook that always disables io_uring." LSM xyzzy never > gets attention from the io_uring developers because, as far as they care, > the problem is solved. To be honest, I wouldn't expect the io_uring developers (or any kernel subsystem maintainer outside the LSMs) to worry about a specific LSM. The io_uring developers should be focused on ensuring that the LSM hooks for io_uring are updated as necessary and called from all of the right locations as io_uring continues to evolve. If there is a problem with LSM xyzzy because it provides a buggy LSM callback for the io_uring LSM hooks, that is a xyzzy issue not an io_uring issue.
On 1/28/2025 2:35 PM, Paul Moore wrote: > On Mon, Jan 27, 2025 at 7:23 PM Casey Schaufler <casey@schaufler-ca.com> wrote: >> On 1/27/2025 1:23 PM, Paul Moore wrote: >>> On Mon, Jan 27, 2025 at 12:18 PM Casey Schaufler <casey@schaufler-ca.com> wrote: >>>> On 1/27/2025 7:57 AM, Hamza Mahfooz wrote: >>>>> It is desirable to allow LSM to configure accessibility to io_uring >>>>> because it is a coarse yet very simple way to restrict access to it. So, >>>>> add an LSM for io_uring_allowed() to guard access to io_uring. >>>> I don't like this at all at all. It looks like you're putting in a hook >>>> so that io_uring can easily deflect any responsibility for safely >>>> interacting with LSMs. >>> That's not how this works Casey, unless you're seeing something different? >> Yes, it's just me being paranoid. When a mechanism is introduced that makes >> it easy to disable a system feature in the LSM environment I start hearing >> voices saying "You can't use security and the cool thing together", and the >> developers of "the cool thing" wave hands and say "just disable it" and it >> never gets properly integrated. I have seen this so many times it makes me >> wonder how anything ever does get made to work in multiple configurations. > While there is plenty of precedent regarding paranoia over kernel > changes outside the LSM, and yes, I do agree that there are likely > some configurations that aren't tested (or make little sense for that > matter), I don't believe that to be the case here. The proposed LSM > hook is simply another access control, and it makes it much easier for > LSMs without full and clear anonymous inode controls to apply access > controls to io_uring. I can't say I agree that it's an access control because although it is specific to a process it isn't specific to an object. You can still access the set of objects using other means. It is a mechanism control, preventing use of io_uring entirely. >>> This is an additional access control point for io_uring, largely to >>> simplify what a LSM would need to do to help control a process' access >>> to io_uring, it does not replace any of the io_uring LSM hooks or >>> access control points. >> What I see is "LSM xyzzy has an issue with io_uring? Just create a >> io_uring_allowed() hook that always disables io_uring." LSM xyzzy never >> gets attention from the io_uring developers because, as far as they care, >> the problem is solved. > To be honest, I wouldn't expect the io_uring developers (or any kernel > subsystem maintainer outside the LSMs) to worry about a specific LSM. > The io_uring developers should be focused on ensuring that the LSM > hooks for io_uring are updated as necessary and called from all of the > right locations as io_uring continues to evolve. If there is a > problem with LSM xyzzy because it provides a buggy LSM callback for > the io_uring LSM hooks, that is a xyzzy issue not an io_uring issue. I'm much more concerned about bugs in io_uring than in xyzzy. The io_uring people have been pretty good about addressing LSM issues, so it's not a huge deal, but I never like seeing switches to turn off features because security is active. If no one else shares my concern you can put my comments down to the ravings of the lunatic fringe and ignore them.
On Tue, Jan 28, 2025 at 7:02 PM Casey Schaufler <casey@schaufler-ca.com> wrote: > > I can't say I agree that it's an access control because although it is > specific to a process it isn't specific to an object. You can still access > the set of objects using other means. It is a mechanism control, preventing > use of io_uring entirely. I see your argument and raise you "capabilities". Granted, we could have a fairly lively debate about the merits of capabilities, which I'm not encouraging here, I'm only mentioning it as a counterpoint and evidence that there is precedent for things like this as "access control". > I'm much more concerned about bugs in io_uring than in xyzzy. The io_uring > people have been pretty good about addressing LSM issues, so it's not > a huge deal, but I never like seeing switches to turn off features because > security is active. > > If no one else shares my concern you can put my comments down to the > ravings of the lunatic fringe and ignore them. Fair enough. FWIW, I appreciate the discussion, even if we didn't quite reach consensus this time around.
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h index e2f1ce37c41e..9eb313bd0c93 100644 --- a/include/linux/lsm_hook_defs.h +++ b/include/linux/lsm_hook_defs.h @@ -455,6 +455,7 @@ LSM_HOOK(int, 0, perf_event_write, struct perf_event *event) LSM_HOOK(int, 0, uring_override_creds, const struct cred *new) LSM_HOOK(int, 0, uring_sqpoll, void) LSM_HOOK(int, 0, uring_cmd, struct io_uring_cmd *ioucmd) +LSM_HOOK(int, 0, uring_allowed, void) #endif /* CONFIG_IO_URING */ LSM_HOOK(void, LSM_RET_VOID, initramfs_populated, void) diff --git a/include/linux/security.h b/include/linux/security.h index 980b6c207cad..3e68f8468a22 100644 --- a/include/linux/security.h +++ b/include/linux/security.h @@ -2362,6 +2362,7 @@ static inline int security_perf_event_write(struct perf_event *event) extern int security_uring_override_creds(const struct cred *new); extern int security_uring_sqpoll(void); extern int security_uring_cmd(struct io_uring_cmd *ioucmd); +extern int security_uring_allowed(void); #else static inline int security_uring_override_creds(const struct cred *new) { @@ -2375,6 +2376,10 @@ static inline int security_uring_cmd(struct io_uring_cmd *ioucmd) { return 0; } +extern int security_uring_allowed(void) +{ + return 0; +} #endif /* CONFIG_SECURITY */ #endif /* CONFIG_IO_URING */ diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c index c2d8bd4c2cfc..9df7b3b556ef 100644 --- a/io_uring/io_uring.c +++ b/io_uring/io_uring.c @@ -3808,7 +3808,7 @@ static inline int io_uring_allowed(void) return -EPERM; allowed_lsm: - return 0; + return security_uring_allowed(); } SYSCALL_DEFINE2(io_uring_setup, u32, entries, diff --git a/security/security.c b/security/security.c index 143561ebc3e8..c9fae447327e 100644 --- a/security/security.c +++ b/security/security.c @@ -5999,6 +5999,18 @@ int security_uring_cmd(struct io_uring_cmd *ioucmd) { return call_int_hook(uring_cmd, ioucmd); } + +/** + * security_uring_allowed() - Check if io_uring_setup() is allowed + * + * Check whether the current task is allowed to call io_uring_setup(). + * + * Return: Returns 0 if permission is granted. + */ +int security_uring_allowed(void) +{ + return call_int_hook(uring_allowed); +} #endif /* CONFIG_IO_URING */ /** diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 7b867dfec88b..fb37e87df226 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -7137,6 +7137,19 @@ static int selinux_uring_cmd(struct io_uring_cmd *ioucmd) return avc_has_perm(current_sid(), isec->sid, SECCLASS_IO_URING, IO_URING__CMD, &ad); } + +/** + * selinux_uring_allowed - check if io_uring_setup() can be called + * + * Check to see if the current task is allowed to call io_uring_setup(). + */ +static int selinux_uring_allowed(void) +{ + u32 sid = current_sid(); + + return avc_has_perm(sid, sid, SECCLASS_IO_URING, IO_URING__ALLOWED, + NULL); +} #endif /* CONFIG_IO_URING */ static const struct lsm_id selinux_lsmid = { @@ -7390,6 +7403,7 @@ static struct security_hook_list selinux_hooks[] __ro_after_init = { LSM_HOOK_INIT(uring_override_creds, selinux_uring_override_creds), LSM_HOOK_INIT(uring_sqpoll, selinux_uring_sqpoll), LSM_HOOK_INIT(uring_cmd, selinux_uring_cmd), + LSM_HOOK_INIT(uring_allowed, selinux_uring_allowed), #endif /* diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h index 03e82477dce9..8a8f3908aac8 100644 --- a/security/selinux/include/classmap.h +++ b/security/selinux/include/classmap.h @@ -177,7 +177,7 @@ const struct security_class_mapping secclass_map[] = { { "perf_event", { "open", "cpu", "kernel", "tracepoint", "read", "write", NULL } }, { "anon_inode", { COMMON_FILE_PERMS, NULL } }, - { "io_uring", { "override_creds", "sqpoll", "cmd", NULL } }, + { "io_uring", { "override_creds", "sqpoll", "cmd", "allowed", NULL } }, { "user_namespace", { "create", NULL } }, /* last one */ { NULL, {} } };
It is desirable to allow LSM to configure accessibility to io_uring because it is a coarse yet very simple way to restrict access to it. So, add an LSM for io_uring_allowed() to guard access to io_uring. Cc: Paul Moore <paul@paul-moore.com> Signed-off-by: Hamza Mahfooz <hamzamahfooz@linux.microsoft.com> --- include/linux/lsm_hook_defs.h | 1 + include/linux/security.h | 5 +++++ io_uring/io_uring.c | 2 +- security/security.c | 12 ++++++++++++ security/selinux/hooks.c | 14 ++++++++++++++ security/selinux/include/classmap.h | 2 +- 6 files changed, 34 insertions(+), 2 deletions(-)