[PATCH v3 1/2] io_uring: refactor io_uring_allowed()

[PATCH v3 1/2] io_uring: refactor io_uring_allowed()

[Hamza Mahfooz]

Have io_uring_allowed() return an error code directly instead of
true/false. This is needed for follow-up work to guard io_uring_setup()
with LSM.

Cc: Jens Axboe <axboe@kernel.dk>
Signed-off-by: Hamza Mahfooz <hamzamahfooz@linux.microsoft.com>
---
 io_uring/io_uring.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
index 7bfbc7c22367..c2d8bd4c2cfc 100644
--- a/io_uring/io_uring.c
+++ b/io_uring/io_uring.c
@@ -3789,29 +3789,36 @@ static long io_uring_setup(u32 entries, struct io_uring_params __user *params)
 	return io_uring_create(entries, &p, params);
 }
 
-static inline bool io_uring_allowed(void)
+static inline int io_uring_allowed(void)
 {
 	int disabled = READ_ONCE(sysctl_io_uring_disabled);
 	kgid_t io_uring_group;
 
 	if (disabled == 2)
-		return false;
+		return -EPERM;
 
 	if (disabled == 0 || capable(CAP_SYS_ADMIN))
-		return true;
+		goto allowed_lsm;
 
 	io_uring_group = make_kgid(&init_user_ns, sysctl_io_uring_group);
 	if (!gid_valid(io_uring_group))
-		return false;
+		return -EPERM;
+
+	if (!in_group_p(io_uring_group))
+		return -EPERM;
 
-	return in_group_p(io_uring_group);
+allowed_lsm:
+	return 0;
 }
 
 SYSCALL_DEFINE2(io_uring_setup, u32, entries,
 		struct io_uring_params __user *, params)
 {
-	if (!io_uring_allowed())
-		return -EPERM;
+	int ret;
+
+	ret = io_uring_allowed();
+	if (ret)
+		return ret;
 
 	return io_uring_setup(entries, params);
 }
-- 
2.47.1

[PATCH v3 2/2] lsm,io_uring: add LSM hooks for io_uring_setup()

[Hamza Mahfooz]

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

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, {} }
 };
-- 
2.47.1

Re: [PATCH v3 2/2] lsm,io_uring: add LSM hooks for io_uring_setup()

[Casey Schaufler]

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, {} }
>  };

Re: [PATCH v3 2/2] lsm,io_uring: add LSM hooks for io_uring_setup()

[Paul Moore]

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.

-- 
paul-moore.com

Re: [PATCH v3 2/2] lsm,io_uring: add LSM hooks for io_uring_setup()

[Casey Schaufler]

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.

Re: [PATCH v3 2/2] lsm,io_uring: add LSM hooks for io_uring_setup()

[Paul Moore]

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.

-- 
paul-moore.com

Re: [PATCH v3 2/2] lsm,io_uring: add LSM hooks for io_uring_setup()

[Casey Schaufler]

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.

Re: [PATCH v3 2/2] lsm,io_uring: add LSM hooks for io_uring_setup()

[Paul Moore]

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.

-- 
paul-moore.com