[RFC 0/1] RFC on how to include LSM hooks for io_uring commands

[RFC 0/1] RFC on how to include LSM hooks for io_uring commands

[Joel Granados]

The motivation for this patch is to continue the discussion around how to
include LSM callback hooks in the io_uring infrastructure. To begin I take
the nvme io_uring passthrough and try to include it in the already existing
LSM infrastructure that is there for ioctl. This is far from a general
io_uring approach, but its a start :)

You are very welcome to comment on the patch, but I have specific questions
in mind:

1. The nvme io_uring are governed by ioctl numbers. In this patch I have
passed this number directly to the ioctl_has_perm function in selinux. For
the io_uring commands that follow such a pattern, is it enough to forward
the call? or do we need to plumb something else? @Paul: really interested
in hearing your thoughts.

2. Could we use something similar for commands that are not structured as
an ioctl? Does ublk structure its commands after ioctl, or does it use
another system? @David would like to hear your thoughts on
this.

3. Finally, Is there anything preventing us from gathering all these
io_uring commands under a common LSM infrastructure like the one that
already exists for ioctl?

Comments are greatly appreciated

Joel Granados (1):
  Use ioctl selinux callback io_uring commands that implement the ioctl
    op convention

 security/selinux/hooks.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

-- 
2.30.2

[RFC 1/1] Use ioctl selinux callback io_uring commands that implement the ioctl op convention

[Joel Granados]

Signed-off-by: Joel Granados <j.granados@samsung.com>
---
 security/selinux/hooks.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index f553c370397e..a3f37ae5a980 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -21,6 +21,7 @@
  *  Copyright (C) 2016 Mellanox Technologies
  */
 
+#include "linux/nvme_ioctl.h"
 #include <linux/init.h>
 #include <linux/kd.h>
 #include <linux/kernel.h>
@@ -7005,12 +7006,22 @@ static int selinux_uring_cmd(struct io_uring_cmd *ioucmd)
 	struct inode *inode = file_inode(file);
 	struct inode_security_struct *isec = selinux_inode(inode);
 	struct common_audit_data ad;
+	const struct cred *cred = current_cred();
 
 	ad.type = LSM_AUDIT_DATA_FILE;
 	ad.u.file = file;
 
-	return avc_has_perm(&selinux_state, current_sid(), isec->sid,
-			    SECCLASS_IO_URING, IO_URING__CMD, &ad);
+	switch (ioucmd->cmd_op) {
+	case NVME_URING_CMD_IO:
+	case NVME_URING_CMD_IO_VEC:
+	case NVME_URING_CMD_ADMIN:
+	case NVME_URING_CMD_ADMIN_VEC:
+		return ioctl_has_perm(cred, file, FILE__IOCTL, (u16) ioucmd->cmd_op);
+	default:
+		return avc_has_perm(&selinux_state, current_sid(), isec->sid,
+				    SECCLASS_IO_URING, IO_URING__CMD, &ad);
+	}
+
 }
 #endif /* CONFIG_IO_URING */
 
-- 
2.30.2

Re: [RFC 1/1] Use ioctl selinux callback io_uring commands that implement the ioctl op convention

[Kanchan Joshi]

[-- Attachment #1: Type: text/plain, Size: 2701 bytes --]

On Wed, Nov 16, 2022 at 01:50:51PM +0100, Joel Granados wrote:
>Signed-off-by: Joel Granados <j.granados@samsung.com>
>---
> security/selinux/hooks.c | 15 +++++++++++++--
> 1 file changed, 13 insertions(+), 2 deletions(-)
>
>diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>index f553c370397e..a3f37ae5a980 100644
>--- a/security/selinux/hooks.c
>+++ b/security/selinux/hooks.c
>@@ -21,6 +21,7 @@
>  *  Copyright (C) 2016 Mellanox Technologies
>  */
>
>+#include "linux/nvme_ioctl.h"
> #include <linux/init.h>
> #include <linux/kd.h>
> #include <linux/kernel.h>
>@@ -7005,12 +7006,22 @@ static int selinux_uring_cmd(struct io_uring_cmd *ioucmd)
> 	struct inode *inode = file_inode(file);
> 	struct inode_security_struct *isec = selinux_inode(inode);
> 	struct common_audit_data ad;
>+	const struct cred *cred = current_cred();
>
> 	ad.type = LSM_AUDIT_DATA_FILE;
> 	ad.u.file = file;
>
>-	return avc_has_perm(&selinux_state, current_sid(), isec->sid,
>-			    SECCLASS_IO_URING, IO_URING__CMD, &ad);
>+	switch (ioucmd->cmd_op) {
>+	case NVME_URING_CMD_IO:
>+	case NVME_URING_CMD_IO_VEC:
>+	case NVME_URING_CMD_ADMIN:
>+	case NVME_URING_CMD_ADMIN_VEC:

We do not have to spell out these opcodes here.
How about this instead:

+       /*
+        * nvme uring-cmd continue to follow the ioctl format, so reuse what
+        * we do for ioctl.
+        */
+       if(_IOC_TYPE(ioucmd->cmd_op) == 'N')
+               return ioctl_has_perm(cred, file, FILE__IOCTL, (u16) ioucmd->cmd_op);
+       else
+               return avc_has_perm(&selinux_state, current_sid(), isec->sid,
+                                   SECCLASS_IO_URING, IO_URING__CMD, &ad);
+       }
+

Now, if we write the above fragment this way -

if (__IOC_TYPE(ioucmd->cmd_op) != 0)
	reuse_what_is_done_for_ioctl;
else
	current_check;

That will be bit more generic and can support more opcodes than nvme.
ublk will continue to fall into else case, but something else (of
future) may go into the if-part and be as fine-granular as ioctl hook
has been.
Although we defined new nvme opcodes to be used with uring-cmd, it is
also possible that some other provider decides to work with existing
ioctl-opcode packaged inside uring-cmd and turns it async. It's just
another implmentation choice.

Not so nice with the above could be that driver-type being 0 seems
under conflict already. The table in this page:
https://www.kernel.org/doc/html/latest/userspace-api/ioctl/ioctl-number.html
But that is first four out of many others. So those four will fall into
else-part (if ever we get there) and everything else will go into the
if-part.

Let's see whether Paul considers all this an improvement from what is
present now.

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [RFC 1/1] Use ioctl selinux callback io_uring commands that implement the ioctl op convention

[Paul Moore]

On Wed, Nov 16, 2022 at 12:49 PM Kanchan Joshi <joshi.k@samsung.com> wrote:
> On Wed, Nov 16, 2022 at 01:50:51PM +0100, Joel Granados wrote:
> >Signed-off-by: Joel Granados <j.granados@samsung.com>
> >---
> > security/selinux/hooks.c | 15 +++++++++++++--
> > 1 file changed, 13 insertions(+), 2 deletions(-)
> >
> >diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> >index f553c370397e..a3f37ae5a980 100644
> >--- a/security/selinux/hooks.c
> >+++ b/security/selinux/hooks.c
> >@@ -21,6 +21,7 @@
> >  *  Copyright (C) 2016 Mellanox Technologies
> >  */
> >
> >+#include "linux/nvme_ioctl.h"
> > #include <linux/init.h>
> > #include <linux/kd.h>
> > #include <linux/kernel.h>
> >@@ -7005,12 +7006,22 @@ static int selinux_uring_cmd(struct io_uring_cmd *ioucmd)
> >       struct inode *inode = file_inode(file);
> >       struct inode_security_struct *isec = selinux_inode(inode);
> >       struct common_audit_data ad;
> >+      const struct cred *cred = current_cred();
> >
> >       ad.type = LSM_AUDIT_DATA_FILE;
> >       ad.u.file = file;
> >
> >-      return avc_has_perm(&selinux_state, current_sid(), isec->sid,
> >-                          SECCLASS_IO_URING, IO_URING__CMD, &ad);
> >+      switch (ioucmd->cmd_op) {
> >+      case NVME_URING_CMD_IO:
> >+      case NVME_URING_CMD_IO_VEC:
> >+      case NVME_URING_CMD_ADMIN:
> >+      case NVME_URING_CMD_ADMIN_VEC:
>
> We do not have to spell out these opcodes here.
> How about this instead:
>
> +       /*
> +        * nvme uring-cmd continue to follow the ioctl format, so reuse what
> +        * we do for ioctl.
> +        */
> +       if(_IOC_TYPE(ioucmd->cmd_op) == 'N')
> +               return ioctl_has_perm(cred, file, FILE__IOCTL, (u16) ioucmd->cmd_op);
> +       else
> +               return avc_has_perm(&selinux_state, current_sid(), isec->sid,
> +                                   SECCLASS_IO_URING, IO_URING__CMD, &ad);
> +       }
> +
>
> Now, if we write the above fragment this way -
>
> if (__IOC_TYPE(ioucmd->cmd_op) != 0)
>         reuse_what_is_done_for_ioctl;
> else
>         current_check;
>
> That will be bit more generic and can support more opcodes than nvme.
> ublk will continue to fall into else case, but something else (of
> future) may go into the if-part and be as fine-granular as ioctl hook
> has been.
> Although we defined new nvme opcodes to be used with uring-cmd, it is
> also possible that some other provider decides to work with existing
> ioctl-opcode packaged inside uring-cmd and turns it async. It's just
> another implmentation choice.
>
> Not so nice with the above could be that driver-type being 0 seems
> under conflict already. The table in this page:
> https://www.kernel.org/doc/html/latest/userspace-api/ioctl/ioctl-number.html
> But that is first four out of many others. So those four will fall into
> else-part (if ever we get there) and everything else will go into the
> if-part.
>
> Let's see whether Paul considers all this an improvement from what is
> present now.

There are a two things that need consideration:

* The current access control enforces the SELinux io_uring/cmd
permission on all io_uring command passthrough operations, that would
need to be preserved using something we call "policy capabilities".
The quick summary is that policy capabilities are a way for the
SELinux policy to signal to the kernel that it is aware of a breaking
change and the policy is written to take this change into account;
when the kernel loads this newly capable policy it sets a flag which
triggers a different behavior in the kernel.  A simple example can be
found in selinux_file_ioctl(FIONCLEX)/selinux_policycap_ioctl_skip_cloexec(),
but we can talk more about this later if/when we resolve the other
issue.

* As we discussed previously, the real problem is the fact that we are
missing the necessary context in the LSM hook to separate the
different types of command targets.  With traditional ioctls we can
look at the ioctl number and determine both the type of
device/subsystem/etc. as well as the operation being requested; there
is no such information available with the io_uring command
passthrough.  In this sense, the io_uring command passthrough is
actually worse than traditional ioctls from an access control
perspective.  Until we have an easy(ish)[1] way to determine the
io_uring command target type, changes like the one suggested here are
going to be doomed as each target type is free to define their own
io_uring commands.

[1] Yes, one could theoretically make some determination of the target
type by inspecting io_uring_cmd::file::f_op (or similar), but checking
file_operations' function pointers is both a pretty awful layering
violation and downright ugly; I don't want to have to maintain that
long-term in a LSM.

--
paul-moore.com

Re: [RFC 1/1] Use ioctl selinux callback io_uring commands that implement the ioctl op convention

[Joel Granados]

[-- Attachment #1: Type: text/plain, Size: 5689 bytes --]

On Wed, Nov 16, 2022 at 02:21:14PM -0500, Paul Moore wrote:
> On Wed, Nov 16, 2022 at 12:49 PM Kanchan Joshi <joshi.k@samsung.com> wrote:
> > On Wed, Nov 16, 2022 at 01:50:51PM +0100, Joel Granados wrote:
> > >Signed-off-by: Joel Granados <j.granados@samsung.com>
> > >---
> > > security/selinux/hooks.c | 15 +++++++++++++--
> > > 1 file changed, 13 insertions(+), 2 deletions(-)
> > >
> > >diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > >index f553c370397e..a3f37ae5a980 100644
> > >--- a/security/selinux/hooks.c
> > >+++ b/security/selinux/hooks.c
> > >@@ -21,6 +21,7 @@
> > >  *  Copyright (C) 2016 Mellanox Technologies
> > >  */
> > >
> > >+#include "linux/nvme_ioctl.h"
> > > #include <linux/init.h>
> > > #include <linux/kd.h>
> > > #include <linux/kernel.h>
> > >@@ -7005,12 +7006,22 @@ static int selinux_uring_cmd(struct io_uring_cmd *ioucmd)
> > >       struct inode *inode = file_inode(file);
> > >       struct inode_security_struct *isec = selinux_inode(inode);
> > >       struct common_audit_data ad;
> > >+      const struct cred *cred = current_cred();
> > >
> > >       ad.type = LSM_AUDIT_DATA_FILE;
> > >       ad.u.file = file;
> > >
> > >-      return avc_has_perm(&selinux_state, current_sid(), isec->sid,
> > >-                          SECCLASS_IO_URING, IO_URING__CMD, &ad);
> > >+      switch (ioucmd->cmd_op) {
> > >+      case NVME_URING_CMD_IO:
> > >+      case NVME_URING_CMD_IO_VEC:
> > >+      case NVME_URING_CMD_ADMIN:
> > >+      case NVME_URING_CMD_ADMIN_VEC:
> >
> > We do not have to spell out these opcodes here.
> > How about this instead:
> >
> > +       /*
> > +        * nvme uring-cmd continue to follow the ioctl format, so reuse what
> > +        * we do for ioctl.
> > +        */
> > +       if(_IOC_TYPE(ioucmd->cmd_op) == 'N')
> > +               return ioctl_has_perm(cred, file, FILE__IOCTL, (u16) ioucmd->cmd_op);
> > +       else
> > +               return avc_has_perm(&selinux_state, current_sid(), isec->sid,
> > +                                   SECCLASS_IO_URING, IO_URING__CMD, &ad);
> > +       }
> > +
> >
> > Now, if we write the above fragment this way -
> >
> > if (__IOC_TYPE(ioucmd->cmd_op) != 0)
> >         reuse_what_is_done_for_ioctl;
> > else
> >         current_check;
> >
> > That will be bit more generic and can support more opcodes than nvme.
> > ublk will continue to fall into else case, but something else (of
> > future) may go into the if-part and be as fine-granular as ioctl hook
> > has been.
> > Although we defined new nvme opcodes to be used with uring-cmd, it is
> > also possible that some other provider decides to work with existing
> > ioctl-opcode packaged inside uring-cmd and turns it async. It's just
> > another implmentation choice.
> >
> > Not so nice with the above could be that driver-type being 0 seems
> > under conflict already. The table in this page:
> > https://www.kernel.org/doc/html/latest/userspace-api/ioctl/ioctl-number.html
> > But that is first four out of many others. So those four will fall into
> > else-part (if ever we get there) and everything else will go into the
> > if-part.
> >
> > Let's see whether Paul considers all this an improvement from what is
> > present now.
> 
> There are a two things that need consideration:
> 
> * The current access control enforces the SELinux io_uring/cmd
> permission on all io_uring command passthrough operations, that would
> need to be preserved using something we call "policy capabilities".
> The quick summary is that policy capabilities are a way for the
> SELinux policy to signal to the kernel that it is aware of a breaking
> change and the policy is written to take this change into account;
> when the kernel loads this newly capable policy it sets a flag which
> triggers a different behavior in the kernel.  A simple example can be
> found in selinux_file_ioctl(FIONCLEX)/selinux_policycap_ioctl_skip_cloexec(),
> but we can talk more about this later if/when we resolve the other
> issue.
Guess we can tackle this after we see how we can get the necessary
context.

> 
> * As we discussed previously, the real problem is the fact that we are
> missing the necessary context in the LSM hook to separate the
> different types of command targets.  With traditional ioctls we can
> look at the ioctl number and determine both the type of
> device/subsystem/etc. as well as the operation being requested; there
> is no such information available with the io_uring command
> passthrough.  In this sense, the io_uring command passthrough is
> actually worse than traditional ioctls from an access control
> perspective.  Until we have an easy(ish)[1] way to determine the
> io_uring command target type, changes like the one suggested here are
> going to be doomed as each target type is free to define their own
> io_uring commands.
The only thing that comes immediately to mind is that we can have
io_uring users define a function that is then passed to the LSM
infrastructure. This function will have all the logic to give relative
context to LSM. It would be general enough to fit all the possible commands
and the logic would be implemented in the "drivers" side so there is no
need for LSM folks to know all io_uring users.

> 
> [1] Yes, one could theoretically make some determination of the target
> type by inspecting io_uring_cmd::file::f_op (or similar), but checking
> file_operations' function pointers is both a pretty awful layering
> violation and downright ugly; I don't want to have to maintain that
> long-term in a LSM.
> 
> --
> paul-moore.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [RFC 1/1] Use ioctl selinux callback io_uring commands that implement the ioctl op convention

[Paul Moore]

On Thu, Nov 17, 2022 at 4:40 AM Joel Granados <j.granados@samsung.com> wrote:
> On Wed, Nov 16, 2022 at 02:21:14PM -0500, Paul Moore wrote:

...

> > * As we discussed previously, the real problem is the fact that we are
> > missing the necessary context in the LSM hook to separate the
> > different types of command targets.  With traditional ioctls we can
> > look at the ioctl number and determine both the type of
> > device/subsystem/etc. as well as the operation being requested; there
> > is no such information available with the io_uring command
> > passthrough.  In this sense, the io_uring command passthrough is
> > actually worse than traditional ioctls from an access control
> > perspective.  Until we have an easy(ish)[1] way to determine the
> > io_uring command target type, changes like the one suggested here are
> > going to be doomed as each target type is free to define their own
> > io_uring commands.
>
> The only thing that comes immediately to mind is that we can have
> io_uring users define a function that is then passed to the LSM
> infrastructure. This function will have all the logic to give relative
> context to LSM. It would be general enough to fit all the possible commands
> and the logic would be implemented in the "drivers" side so there is no
> need for LSM folks to know all io_uring users.

Passing a function pointer to the LSM to fetch, what will likely be
just a constant value, seems kinda ugly, but I guess we only have ugly
options at this point.  I imagine we could cache the result in the
inode's security blob, assuming the device type remained constant (I
can't think of why it would change); still ugly but at least it
doesn't require multiple indirect calls.

It's probably worth throwing together a quick patch so we can discuss
this further.

--
paul-moore.com

Re: [RFC 1/1] Use ioctl selinux callback io_uring commands that implement the ioctl op convention

[Luis Chamberlain]

On Thu, Nov 17, 2022 at 05:10:07PM -0500, Paul Moore wrote:
> On Thu, Nov 17, 2022 at 4:40 AM Joel Granados <j.granados@samsung.com> wrote:
> > On Wed, Nov 16, 2022 at 02:21:14PM -0500, Paul Moore wrote:
> 
> ...
> 
> > > * As we discussed previously, the real problem is the fact that we are
> > > missing the necessary context in the LSM hook to separate the
> > > different types of command targets.  With traditional ioctls we can
> > > look at the ioctl number and determine both the type of
> > > device/subsystem/etc. as well as the operation being requested; there
> > > is no such information available with the io_uring command
> > > passthrough.  In this sense, the io_uring command passthrough is
> > > actually worse than traditional ioctls from an access control
> > > perspective.  Until we have an easy(ish)[1] way to determine the
> > > io_uring command target type, changes like the one suggested here are
> > > going to be doomed as each target type is free to define their own
> > > io_uring commands.
> >
> > The only thing that comes immediately to mind is that we can have
> > io_uring users define a function that is then passed to the LSM
> > infrastructure. This function will have all the logic to give relative
> > context to LSM. It would be general enough to fit all the possible commands
> > and the logic would be implemented in the "drivers" side so there is no
> > need for LSM folks to know all io_uring users.
> 
> Passing a function pointer to the LSM to fetch, what will likely be
> just a constant value, seems kinda ugly, but I guess we only have ugly
> options at this point.

I am not sure if this helps yet, but queued on modules-next we now have
an improvement in speed of about 1500x for kallsyms_lookup_name(), and
so symbol lookups are now fast. Makes me wonder if a type of special
export could be drawn up for specific calls which follow a structure
and so the respective lsm could be inferred by a prefix instead of
placing the calls in-place. Then it would not mattter where a call is
used, so long as it would follow a specific pattern / structure with
all the crap you need on it.

  Luis

Re: [RFC 1/1] Use ioctl selinux callback io_uring commands that implement the ioctl op convention

[Paul Moore]

On Mon, Nov 21, 2022 at 2:53 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
> On Thu, Nov 17, 2022 at 05:10:07PM -0500, Paul Moore wrote:
> > On Thu, Nov 17, 2022 at 4:40 AM Joel Granados <j.granados@samsung.com> wrote:
> > > On Wed, Nov 16, 2022 at 02:21:14PM -0500, Paul Moore wrote:
> >
> > ...
> >
> > > > * As we discussed previously, the real problem is the fact that we are
> > > > missing the necessary context in the LSM hook to separate the
> > > > different types of command targets.  With traditional ioctls we can
> > > > look at the ioctl number and determine both the type of
> > > > device/subsystem/etc. as well as the operation being requested; there
> > > > is no such information available with the io_uring command
> > > > passthrough.  In this sense, the io_uring command passthrough is
> > > > actually worse than traditional ioctls from an access control
> > > > perspective.  Until we have an easy(ish)[1] way to determine the
> > > > io_uring command target type, changes like the one suggested here are
> > > > going to be doomed as each target type is free to define their own
> > > > io_uring commands.
> > >
> > > The only thing that comes immediately to mind is that we can have
> > > io_uring users define a function that is then passed to the LSM
> > > infrastructure. This function will have all the logic to give relative
> > > context to LSM. It would be general enough to fit all the possible commands
> > > and the logic would be implemented in the "drivers" side so there is no
> > > need for LSM folks to know all io_uring users.
> >
> > Passing a function pointer to the LSM to fetch, what will likely be
> > just a constant value, seems kinda ugly, but I guess we only have ugly
> > options at this point.
>
> I am not sure if this helps yet, but queued on modules-next we now have
> an improvement in speed of about 1500x for kallsyms_lookup_name(), and
> so symbol lookups are now fast. Makes me wonder if a type of special
> export could be drawn up for specific calls which follow a structure
> and so the respective lsm could be inferred by a prefix instead of
> placing the calls in-place. Then it would not mattter where a call is
> used, so long as it would follow a specific pattern / structure with
> all the crap you need on it.

I suspect we may be talking about different things here, I don't think
the issue is which LSM(s) may be enabled, as the call is to
security_uring_cmd() regardless.  I believe the issue is more of how
do the LSMs determine the target of the io_uring command, e.g. nvme or
ublk.

My understanding is that Joel was suggesting a change to the LSM hook
to add a function specific pointer (presumably defined as part of the
file_operations struct) that could be called by the LSM to determine
the target.

Although now that I'm looking again at the file_operations struct, I
wonder if we would be better off having the LSMs inspect the
file_operations::owner field, potentially checking the module::name
field.  It's a little painful in the sense that it is potentially
multiple strcmp() calls for each security_uring_cmd() call, but I'm
not sure the passed function approach would be much better.  Do we
have a consistent per-module scalar value we can use instead of a
character string?

--
paul-moore.com

Re: [RFC 1/1] Use ioctl selinux callback io_uring commands that implement the ioctl op convention

[Joel Granados]

[-- Attachment #1: Type: text/plain, Size: 3906 bytes --]

On Mon, Nov 21, 2022 at 04:05:37PM -0500, Paul Moore wrote:
> On Mon, Nov 21, 2022 at 2:53 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
> > On Thu, Nov 17, 2022 at 05:10:07PM -0500, Paul Moore wrote:
> > > On Thu, Nov 17, 2022 at 4:40 AM Joel Granados <j.granados@samsung.com> wrote:
> > > > On Wed, Nov 16, 2022 at 02:21:14PM -0500, Paul Moore wrote:
> > >
> > > ...
> > >
> > > > > * As we discussed previously, the real problem is the fact that we are
> > > > > missing the necessary context in the LSM hook to separate the
> > > > > different types of command targets.  With traditional ioctls we can
> > > > > look at the ioctl number and determine both the type of
> > > > > device/subsystem/etc. as well as the operation being requested; there
> > > > > is no such information available with the io_uring command
> > > > > passthrough.  In this sense, the io_uring command passthrough is
> > > > > actually worse than traditional ioctls from an access control
> > > > > perspective.  Until we have an easy(ish)[1] way to determine the
> > > > > io_uring command target type, changes like the one suggested here are
> > > > > going to be doomed as each target type is free to define their own
> > > > > io_uring commands.
> > > >
> > > > The only thing that comes immediately to mind is that we can have
> > > > io_uring users define a function that is then passed to the LSM
> > > > infrastructure. This function will have all the logic to give relative
> > > > context to LSM. It would be general enough to fit all the possible commands
> > > > and the logic would be implemented in the "drivers" side so there is no
> > > > need for LSM folks to know all io_uring users.
> > >
> > > Passing a function pointer to the LSM to fetch, what will likely be
> > > just a constant value, seems kinda ugly, but I guess we only have ugly
> > > options at this point.
> >
> > I am not sure if this helps yet, but queued on modules-next we now have
> > an improvement in speed of about 1500x for kallsyms_lookup_name(), and
> > so symbol lookups are now fast. Makes me wonder if a type of special
> > export could be drawn up for specific calls which follow a structure
> > and so the respective lsm could be inferred by a prefix instead of
> > placing the calls in-place. Then it would not mattter where a call is
> > used, so long as it would follow a specific pattern / structure with
> > all the crap you need on it.
> 
> I suspect we may be talking about different things here, I don't think
> the issue is which LSM(s) may be enabled, as the call is to
> security_uring_cmd() regardless.  I believe the issue is more of how
> do the LSMs determine the target of the io_uring command, e.g. nvme or
> ublk.
I agree, but we might be able to use kallsysms_lookup_name to execute a
callback once we know where the call comes from.

> 
> My understanding is that Joel was suggesting a change to the LSM hook
> to add a function specific pointer (presumably defined as part of the
> file_operations struct) that could be called by the LSM to determine
> the target.
Indeed. I just sent out the RFC. Its at an idea stage and would be great
to hear what you think

> 
> Although now that I'm looking again at the file_operations struct, I
> wonder if we would be better off having the LSMs inspect the
> file_operations::owner field, potentially checking the module::name
> field.  It's a little painful in the sense that it is potentially
> multiple strcmp() calls for each security_uring_cmd() call, but I'm
> not sure the passed function approach would be much better.  Do we
> have a consistent per-module scalar value we can use instead of a
> character string?
This is also a possibility. And with that we might just be able to call
some sort of callback with kallsysms_lookup_name or whatever makes
sense.
> 
> --
> paul-moore.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [RFC 1/1] Use ioctl selinux callback io_uring commands that implement the ioctl op convention

[Ming Lei]

On Mon, Nov 21, 2022 at 04:05:37PM -0500, Paul Moore wrote:
> On Mon, Nov 21, 2022 at 2:53 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
> > On Thu, Nov 17, 2022 at 05:10:07PM -0500, Paul Moore wrote:
> > > On Thu, Nov 17, 2022 at 4:40 AM Joel Granados <j.granados@samsung.com> wrote:
> > > > On Wed, Nov 16, 2022 at 02:21:14PM -0500, Paul Moore wrote:
> > >
> > > ...
> > >
> > > > > * As we discussed previously, the real problem is the fact that we are
> > > > > missing the necessary context in the LSM hook to separate the
> > > > > different types of command targets.  With traditional ioctls we can
> > > > > look at the ioctl number and determine both the type of
> > > > > device/subsystem/etc. as well as the operation being requested; there
> > > > > is no such information available with the io_uring command
> > > > > passthrough.  In this sense, the io_uring command passthrough is
> > > > > actually worse than traditional ioctls from an access control
> > > > > perspective.  Until we have an easy(ish)[1] way to determine the
> > > > > io_uring command target type, changes like the one suggested here are
> > > > > going to be doomed as each target type is free to define their own
> > > > > io_uring commands.
> > > >
> > > > The only thing that comes immediately to mind is that we can have
> > > > io_uring users define a function that is then passed to the LSM
> > > > infrastructure. This function will have all the logic to give relative
> > > > context to LSM. It would be general enough to fit all the possible commands
> > > > and the logic would be implemented in the "drivers" side so there is no
> > > > need for LSM folks to know all io_uring users.
> > >
> > > Passing a function pointer to the LSM to fetch, what will likely be
> > > just a constant value, seems kinda ugly, but I guess we only have ugly
> > > options at this point.
> >
> > I am not sure if this helps yet, but queued on modules-next we now have
> > an improvement in speed of about 1500x for kallsyms_lookup_name(), and
> > so symbol lookups are now fast. Makes me wonder if a type of special
> > export could be drawn up for specific calls which follow a structure
> > and so the respective lsm could be inferred by a prefix instead of
> > placing the calls in-place. Then it would not mattter where a call is
> > used, so long as it would follow a specific pattern / structure with
> > all the crap you need on it.
> 
> I suspect we may be talking about different things here, I don't think
> the issue is which LSM(s) may be enabled, as the call is to
> security_uring_cmd() regardless.  I believe the issue is more of how
> do the LSMs determine the target of the io_uring command, e.g. nvme or
> ublk.
> 
> My understanding is that Joel was suggesting a change to the LSM hook
> to add a function specific pointer (presumably defined as part of the
> file_operations struct) that could be called by the LSM to determine
> the target.
> 
> Although now that I'm looking again at the file_operations struct, I
> wonder if we would be better off having the LSMs inspect the
> file_operations::owner field, potentially checking the module::name
> field.  It's a little painful in the sense that it is potentially
> multiple strcmp() calls for each security_uring_cmd() call, but I'm
> not sure the passed function approach would be much better.  Do we
> have a consistent per-module scalar value we can use instead of a
> character string?

In future there may be more uring_cmd kernel users, maybe we need to
consider to use one reserved field in io_uring_sqe for describing the
target type if it is one must for security subsystem, and this way
will be similar with traditional ioctl which encodes this kind of
info in command type.


Thanks,
Ming

Re: [RFC 1/1] Use ioctl selinux callback io_uring commands that implement the ioctl op convention

[Joel Granados]

[-- Attachment #1: Type: text/plain, Size: 4351 bytes --]

On Tue, Nov 22, 2022 at 10:04:03PM +0800, Ming Lei wrote:
> On Mon, Nov 21, 2022 at 04:05:37PM -0500, Paul Moore wrote:
> > On Mon, Nov 21, 2022 at 2:53 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
> > > On Thu, Nov 17, 2022 at 05:10:07PM -0500, Paul Moore wrote:
> > > > On Thu, Nov 17, 2022 at 4:40 AM Joel Granados <j.granados@samsung.com> wrote:
> > > > > On Wed, Nov 16, 2022 at 02:21:14PM -0500, Paul Moore wrote:
> > > >
> > > > ...
> > > >
> > > > > > * As we discussed previously, the real problem is the fact that we are
> > > > > > missing the necessary context in the LSM hook to separate the
> > > > > > different types of command targets.  With traditional ioctls we can
> > > > > > look at the ioctl number and determine both the type of
> > > > > > device/subsystem/etc. as well as the operation being requested; there
> > > > > > is no such information available with the io_uring command
> > > > > > passthrough.  In this sense, the io_uring command passthrough is
> > > > > > actually worse than traditional ioctls from an access control
> > > > > > perspective.  Until we have an easy(ish)[1] way to determine the
> > > > > > io_uring command target type, changes like the one suggested here are
> > > > > > going to be doomed as each target type is free to define their own
> > > > > > io_uring commands.
> > > > >
> > > > > The only thing that comes immediately to mind is that we can have
> > > > > io_uring users define a function that is then passed to the LSM
> > > > > infrastructure. This function will have all the logic to give relative
> > > > > context to LSM. It would be general enough to fit all the possible commands
> > > > > and the logic would be implemented in the "drivers" side so there is no
> > > > > need for LSM folks to know all io_uring users.
> > > >
> > > > Passing a function pointer to the LSM to fetch, what will likely be
> > > > just a constant value, seems kinda ugly, but I guess we only have ugly
> > > > options at this point.
> > >
> > > I am not sure if this helps yet, but queued on modules-next we now have
> > > an improvement in speed of about 1500x for kallsyms_lookup_name(), and
> > > so symbol lookups are now fast. Makes me wonder if a type of special
> > > export could be drawn up for specific calls which follow a structure
> > > and so the respective lsm could be inferred by a prefix instead of
> > > placing the calls in-place. Then it would not mattter where a call is
> > > used, so long as it would follow a specific pattern / structure with
> > > all the crap you need on it.
> > 
> > I suspect we may be talking about different things here, I don't think
> > the issue is which LSM(s) may be enabled, as the call is to
> > security_uring_cmd() regardless.  I believe the issue is more of how
> > do the LSMs determine the target of the io_uring command, e.g. nvme or
> > ublk.
> > 
> > My understanding is that Joel was suggesting a change to the LSM hook
> > to add a function specific pointer (presumably defined as part of the
> > file_operations struct) that could be called by the LSM to determine
> > the target.
> > 
> > Although now that I'm looking again at the file_operations struct, I
> > wonder if we would be better off having the LSMs inspect the
> > file_operations::owner field, potentially checking the module::name
> > field.  It's a little painful in the sense that it is potentially
> > multiple strcmp() calls for each security_uring_cmd() call, but I'm
> > not sure the passed function approach would be much better.  Do we
> > have a consistent per-module scalar value we can use instead of a
> > character string?
> 
> In future there may be more uring_cmd kernel users, maybe we need to
> consider to use one reserved field in io_uring_sqe for describing the
> target type if it is one must for security subsystem, and this way
> will be similar with traditional ioctl which encodes this kind of
> info in command type.
This is of course another option. I was a bit reluctant to start the
discussion with this implementation, but now that you have brought it up
I can come up with an initial RFC and we can add it to the mix of
possibilities.

Would you just add it to the end of the struct? or what reserved field
are you referring to?
> 
> 
> Thanks,
> Ming
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [RFC 1/1] Use ioctl selinux callback io_uring commands that implement the ioctl op convention

[Ming Lei]

On Mon, Nov 28, 2022 at 11:13:29AM +0100, Joel Granados wrote:
> On Tue, Nov 22, 2022 at 10:04:03PM +0800, Ming Lei wrote:
> > On Mon, Nov 21, 2022 at 04:05:37PM -0500, Paul Moore wrote:
> > > On Mon, Nov 21, 2022 at 2:53 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
> > > > On Thu, Nov 17, 2022 at 05:10:07PM -0500, Paul Moore wrote:
> > > > > On Thu, Nov 17, 2022 at 4:40 AM Joel Granados <j.granados@samsung.com> wrote:
> > > > > > On Wed, Nov 16, 2022 at 02:21:14PM -0500, Paul Moore wrote:
> > > > >
> > > > > ...
> > > > >
> > > > > > > * As we discussed previously, the real problem is the fact that we are
> > > > > > > missing the necessary context in the LSM hook to separate the
> > > > > > > different types of command targets.  With traditional ioctls we can
> > > > > > > look at the ioctl number and determine both the type of
> > > > > > > device/subsystem/etc. as well as the operation being requested; there
> > > > > > > is no such information available with the io_uring command
> > > > > > > passthrough.  In this sense, the io_uring command passthrough is
> > > > > > > actually worse than traditional ioctls from an access control
> > > > > > > perspective.  Until we have an easy(ish)[1] way to determine the
> > > > > > > io_uring command target type, changes like the one suggested here are
> > > > > > > going to be doomed as each target type is free to define their own
> > > > > > > io_uring commands.
> > > > > >
> > > > > > The only thing that comes immediately to mind is that we can have
> > > > > > io_uring users define a function that is then passed to the LSM
> > > > > > infrastructure. This function will have all the logic to give relative
> > > > > > context to LSM. It would be general enough to fit all the possible commands
> > > > > > and the logic would be implemented in the "drivers" side so there is no
> > > > > > need for LSM folks to know all io_uring users.
> > > > >
> > > > > Passing a function pointer to the LSM to fetch, what will likely be
> > > > > just a constant value, seems kinda ugly, but I guess we only have ugly
> > > > > options at this point.
> > > >
> > > > I am not sure if this helps yet, but queued on modules-next we now have
> > > > an improvement in speed of about 1500x for kallsyms_lookup_name(), and
> > > > so symbol lookups are now fast. Makes me wonder if a type of special
> > > > export could be drawn up for specific calls which follow a structure
> > > > and so the respective lsm could be inferred by a prefix instead of
> > > > placing the calls in-place. Then it would not mattter where a call is
> > > > used, so long as it would follow a specific pattern / structure with
> > > > all the crap you need on it.
> > > 
> > > I suspect we may be talking about different things here, I don't think
> > > the issue is which LSM(s) may be enabled, as the call is to
> > > security_uring_cmd() regardless.  I believe the issue is more of how
> > > do the LSMs determine the target of the io_uring command, e.g. nvme or
> > > ublk.
> > > 
> > > My understanding is that Joel was suggesting a change to the LSM hook
> > > to add a function specific pointer (presumably defined as part of the
> > > file_operations struct) that could be called by the LSM to determine
> > > the target.
> > > 
> > > Although now that I'm looking again at the file_operations struct, I
> > > wonder if we would be better off having the LSMs inspect the
> > > file_operations::owner field, potentially checking the module::name
> > > field.  It's a little painful in the sense that it is potentially
> > > multiple strcmp() calls for each security_uring_cmd() call, but I'm
> > > not sure the passed function approach would be much better.  Do we
> > > have a consistent per-module scalar value we can use instead of a
> > > character string?
> > 
> > In future there may be more uring_cmd kernel users, maybe we need to
> > consider to use one reserved field in io_uring_sqe for describing the
> > target type if it is one must for security subsystem, and this way
> > will be similar with traditional ioctl which encodes this kind of
> > info in command type.
> This is of course another option. I was a bit reluctant to start the
> discussion with this implementation, but now that you have brought it up
> I can come up with an initial RFC and we can add it to the mix of
> possibilities.
> 
> Would you just add it to the end of the struct? or what reserved field
> are you referring to?

io_uring_sqe is uapi, so you can't add any field to sqe, and '__pad1'
could be best field for carrying this info, given it is close to 'cmd_op',
and 'u8' should be enough for storing target type info.


thanks,
Ming

Re: [RFC 1/1] Use ioctl selinux callback io_uring commands that implement the ioctl op convention

[Joel Granados]

[-- Attachment #1: Type: text/plain, Size: 2812 bytes --]

On Mon, Nov 21, 2022 at 11:53:17AM -0800, Luis Chamberlain wrote:
> On Thu, Nov 17, 2022 at 05:10:07PM -0500, Paul Moore wrote:
> > On Thu, Nov 17, 2022 at 4:40 AM Joel Granados <j.granados@samsung.com> wrote:
> > > On Wed, Nov 16, 2022 at 02:21:14PM -0500, Paul Moore wrote:
> > 
> > ...
> > 
> > > > * As we discussed previously, the real problem is the fact that we are
> > > > missing the necessary context in the LSM hook to separate the
> > > > different types of command targets.  With traditional ioctls we can
> > > > look at the ioctl number and determine both the type of
> > > > device/subsystem/etc. as well as the operation being requested; there
> > > > is no such information available with the io_uring command
> > > > passthrough.  In this sense, the io_uring command passthrough is
> > > > actually worse than traditional ioctls from an access control
> > > > perspective.  Until we have an easy(ish)[1] way to determine the
> > > > io_uring command target type, changes like the one suggested here are
> > > > going to be doomed as each target type is free to define their own
> > > > io_uring commands.
> > >
> > > The only thing that comes immediately to mind is that we can have
> > > io_uring users define a function that is then passed to the LSM
> > > infrastructure. This function will have all the logic to give relative
> > > context to LSM. It would be general enough to fit all the possible commands
> > > and the logic would be implemented in the "drivers" side so there is no
> > > need for LSM folks to know all io_uring users.
> > 
> > Passing a function pointer to the LSM to fetch, what will likely be
> > just a constant value, seems kinda ugly, but I guess we only have ugly
> > options at this point.
> 
> I am not sure if this helps yet, but queued on modules-next we now have
> an improvement in speed of about 1500x for kallsyms_lookup_name(), and
> so symbol lookups are now fast. Makes me wonder if a type of special
> export could be drawn up for specific calls which follow a structure
> and so the respective lsm could be inferred by a prefix instead of
> placing the calls in-place. Then it would not mattter where a call is
> used, so long as it would follow a specific pattern / structure with
> all the crap you need on it.

This is very good to know. Thx for putting this in the foreground.
This problems CAN be seen as two:
1. Knowing where the io_uring is coming from (user, file, drivers...)
2. Calling the appropriate callback (ublk callback or nvme callback...)

In this sense we might be able to use kallsysms_lookup_name() to do the
callback to a relevant driver.

I have posted a version 2 of the RFC without using
kallsysms_lookup_name, but I'll keep it in the back of my mind.

Best
> 
>   Luis

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [RFC 1/1] Use ioctl selinux callback io_uring commands that implement the ioctl op convention

[Joel Granados]

[-- Attachment #1: Type: text/plain, Size: 3356 bytes --]

On Wed, Nov 16, 2022 at 11:08:21PM +0530, Kanchan Joshi wrote:
> On Wed, Nov 16, 2022 at 01:50:51PM +0100, Joel Granados wrote:
> > Signed-off-by: Joel Granados <j.granados@samsung.com>
> > ---
> > security/selinux/hooks.c | 15 +++++++++++++--
> > 1 file changed, 13 insertions(+), 2 deletions(-)
> > 
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index f553c370397e..a3f37ae5a980 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -21,6 +21,7 @@
> >  *  Copyright (C) 2016 Mellanox Technologies
> >  */
> > 
> > +#include "linux/nvme_ioctl.h"
> > #include <linux/init.h>
> > #include <linux/kd.h>
> > #include <linux/kernel.h>
> > @@ -7005,12 +7006,22 @@ static int selinux_uring_cmd(struct io_uring_cmd *ioucmd)
> > 	struct inode *inode = file_inode(file);
> > 	struct inode_security_struct *isec = selinux_inode(inode);
> > 	struct common_audit_data ad;
> > +	const struct cred *cred = current_cred();
> > 
> > 	ad.type = LSM_AUDIT_DATA_FILE;
> > 	ad.u.file = file;
> > 
> > -	return avc_has_perm(&selinux_state, current_sid(), isec->sid,
> > -			    SECCLASS_IO_URING, IO_URING__CMD, &ad);
> > +	switch (ioucmd->cmd_op) {
> > +	case NVME_URING_CMD_IO:
> > +	case NVME_URING_CMD_IO_VEC:
> > +	case NVME_URING_CMD_ADMIN:
> > +	case NVME_URING_CMD_ADMIN_VEC:
> 
> We do not have to spell out these opcodes here.
> How about this instead:
> 
> +       /*
> +        * nvme uring-cmd continue to follow the ioctl format, so reuse what
> +        * we do for ioctl.
> +        */
> +       if(_IOC_TYPE(ioucmd->cmd_op) == 'N')
> +               return ioctl_has_perm(cred, file, FILE__IOCTL, (u16) ioucmd->cmd_op);
> +       else
> +               return avc_has_perm(&selinux_state, current_sid(), isec->sid,
> +                                   SECCLASS_IO_URING, IO_URING__CMD, &ad);
> +       }
> +
> 
> Now, if we write the above fragment this way -
> 
> if (__IOC_TYPE(ioucmd->cmd_op) != 0)
> 	reuse_what_is_done_for_ioctl;
> else
> 	current_check;

This is even cleaner. I really like this solution.
> 
> That will be bit more generic and can support more opcodes than nvme.
> ublk will continue to fall into else case, but something else (of
> future) may go into the if-part and be as fine-granular as ioctl hook
> has been.
I also see that this is the case. Since the io_uring command does not
have a predefined structure another solution for the non ioctl io_uring
commands needs to be found.

> Although we defined new nvme opcodes to be used with uring-cmd, it is
> also possible that some other provider decides to work with existing
> ioctl-opcode packaged inside uring-cmd and turns it async. It's just
> another implmentation choice.
> 
> Not so nice with the above could be that driver-type being 0 seems
> under conflict already. The table in this page:
> https://www.kernel.org/doc/html/latest/userspace-api/ioctl/ioctl-number.html
> But that is first four out of many others. So those four will fall into
> else-part (if ever we get there) and everything else will go into the
> if-part.
Agreed, the fact that we might have these crashes also seems to be
another CON for using the ioctl approach

> 
> Let's see whether Paul considers all this an improvement from what is
> present now.



[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]