Re: [PATCH 03/17] io_uring: add infra and support for IORING_OP_URING_CMD

Re: [PATCH 03/17] io_uring: add infra and support for IORING_OP_URING_CMD

[Luis Chamberlain]

On Tue, Mar 08, 2022 at 08:50:51PM +0530, Kanchan Joshi wrote:
> From: Jens Axboe <axboe@kernel.dk>
> 
> This is a file private kind of request. io_uring doesn't know what's
> in this command type, it's for the file_operations->async_cmd()
> handler to deal with.
> 
> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> Signed-off-by: Kanchan Joshi <joshi.k@samsung.com>
> ---

<-- snip -->

> +static int io_uring_cmd(struct io_kiocb *req, unsigned int issue_flags)
> +{
> +	struct file *file = req->file;
> +	int ret;
> +	struct io_uring_cmd *ioucmd = &req->uring_cmd;
> +
> +	ioucmd->flags |= issue_flags;
> +	ret = file->f_op->async_cmd(ioucmd);

I think we're going to have to add a security_file_async_cmd() check
before this call here. Because otherwise we're enabling to, for
example, bypass security_file_ioctl() for example using the new
iouring-cmd interface.

Or is this already thought out with the existing security_uring_*() stuff?

  Luis

Re: [PATCH 03/17] io_uring: add infra and support for IORING_OP_URING_CMD

[Jens Axboe]

On 3/10/22 6:51 PM, Luis Chamberlain wrote:
> On Tue, Mar 08, 2022 at 08:50:51PM +0530, Kanchan Joshi wrote:
>> From: Jens Axboe <axboe@kernel.dk>
>>
>> This is a file private kind of request. io_uring doesn't know what's
>> in this command type, it's for the file_operations->async_cmd()
>> handler to deal with.
>>
>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>> Signed-off-by: Kanchan Joshi <joshi.k@samsung.com>
>> ---
> 
> <-- snip -->
> 
>> +static int io_uring_cmd(struct io_kiocb *req, unsigned int issue_flags)
>> +{
>> +	struct file *file = req->file;
>> +	int ret;
>> +	struct io_uring_cmd *ioucmd = &req->uring_cmd;
>> +
>> +	ioucmd->flags |= issue_flags;
>> +	ret = file->f_op->async_cmd(ioucmd);
> 
> I think we're going to have to add a security_file_async_cmd() check
> before this call here. Because otherwise we're enabling to, for
> example, bypass security_file_ioctl() for example using the new
> iouring-cmd interface.
> 
> Or is this already thought out with the existing security_uring_*() stuff?

Unless the request sets .audit_skip, it'll be included already in terms
of logging. But I'd prefer not to lodge this in with ioctls, unless
we're going to be doing actual ioctls.

But definitely something to keep in mind and make sure that we're under
the right umbrella in terms of auditing and security.

-- 
Jens Axboe

Re: [PATCH 03/17] io_uring: add infra and support for IORING_OP_URING_CMD

[Luis Chamberlain]

On Thu, Mar 10, 2022 at 07:43:04PM -0700, Jens Axboe wrote:
> On 3/10/22 6:51 PM, Luis Chamberlain wrote:
> > On Tue, Mar 08, 2022 at 08:50:51PM +0530, Kanchan Joshi wrote:
> >> From: Jens Axboe <axboe@kernel.dk>
> >>
> >> This is a file private kind of request. io_uring doesn't know what's
> >> in this command type, it's for the file_operations->async_cmd()
> >> handler to deal with.
> >>
> >> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> >> Signed-off-by: Kanchan Joshi <joshi.k@samsung.com>
> >> ---
> > 
> > <-- snip -->
> > 
> >> +static int io_uring_cmd(struct io_kiocb *req, unsigned int issue_flags)
> >> +{
> >> +	struct file *file = req->file;
> >> +	int ret;
> >> +	struct io_uring_cmd *ioucmd = &req->uring_cmd;
> >> +
> >> +	ioucmd->flags |= issue_flags;
> >> +	ret = file->f_op->async_cmd(ioucmd);
> > 
> > I think we're going to have to add a security_file_async_cmd() check
> > before this call here. Because otherwise we're enabling to, for
> > example, bypass security_file_ioctl() for example using the new
> > iouring-cmd interface.
> > 
> > Or is this already thought out with the existing security_uring_*() stuff?
> 
> Unless the request sets .audit_skip, it'll be included already in terms
> of logging.

Neat.

> But I'd prefer not to lodge this in with ioctls, unless
> we're going to be doing actual ioctls.

Oh sure, I have been an advocate to ensure folks don't conflate async_cmd
with ioctl. However it *can* enable subsystems to enable ioctl
passthrough, but each of those subsystems need to vet for this on their
own terms. I'd hate to see / hear some LSM surprises later.

> But definitely something to keep in mind and make sure that we're under
> the right umbrella in terms of auditing and security.

Paul, how about something like this for starters (and probably should
be squashed into this series so its not a separate commit) ?

From f3ddbe822374cc1c7002bd795c1ae486d370cbd1 Mon Sep 17 00:00:00 2001
From: Luis Chamberlain <mcgrof@kernel.org>
Date: Fri, 11 Mar 2022 08:55:50 -0800
Subject: [PATCH] lsm,io_uring: add LSM hooks to for the new async_cmd file op

io-uring is extending the struct file_operations to allow a new
command which each subsystem can use to enable command passthrough.
Add an LSM specific for the command passthrough which enables LSMs
to inspect the command details.

Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 fs/io_uring.c                 | 5 +++++
 include/linux/lsm_hook_defs.h | 1 +
 include/linux/lsm_hooks.h     | 3 +++
 include/linux/security.h      | 5 +++++
 security/security.c           | 4 ++++
 5 files changed, 18 insertions(+)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 3f6eacc98e31..1c4e6b2cb61a 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -4190,6 +4190,11 @@ static int io_uring_cmd_prep(struct io_kiocb *req,
 	struct io_ring_ctx *ctx = req->ctx;
 	struct io_uring_cmd *ioucmd = &req->uring_cmd;
 	u32 ucmd_flags = READ_ONCE(sqe->uring_cmd_flags);
+	int ret;
+
+	ret = security_uring_async_cmd(ioucmd);
+	if (ret)
+		return ret;
 
 	if (!req->file->f_op->async_cmd)
 		return -EOPNOTSUPP;
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 819ec92dc2a8..4a20f8e6b295 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -404,4 +404,5 @@ LSM_HOOK(int, 0, perf_event_write, struct perf_event *event)
 #ifdef CONFIG_IO_URING
 LSM_HOOK(int, 0, uring_override_creds, const struct cred *new)
 LSM_HOOK(int, 0, uring_sqpoll, void)
+LSM_HOOK(int, 0, uring_async_cmd, struct io_uring_cmd *ioucmd)
 #endif /* CONFIG_IO_URING */
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 3bf5c658bc44..21b18cf138c2 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -1569,6 +1569,9 @@
  *      Check whether the current task is allowed to spawn a io_uring polling
  *      thread (IORING_SETUP_SQPOLL).
  *
+ * @uring_async_cmd:
+ *      Check whether the file_operations async_cmd is allowed to run.
+ *
  */
 union security_list_options {
 	#define LSM_HOOK(RET, DEFAULT, NAME, ...) RET (*NAME)(__VA_ARGS__);
diff --git a/include/linux/security.h b/include/linux/security.h
index 6d72772182c8..4d7f72813d75 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -2041,6 +2041,7 @@ static inline int security_perf_event_write(struct perf_event *event)
 #ifdef CONFIG_SECURITY
 extern int security_uring_override_creds(const struct cred *new);
 extern int security_uring_sqpoll(void);
+extern int security_uring_async_cmd(struct io_uring_cmd *ioucmd);
 #else
 static inline int security_uring_override_creds(const struct cred *new)
 {
@@ -2050,6 +2051,10 @@ static inline int security_uring_sqpoll(void)
 {
 	return 0;
 }
+static inline int security_uring_async_cmd(struct io_uring_cmd *ioucmd)
+{
+	return 0;
+}
 #endif /* CONFIG_SECURITY */
 #endif /* CONFIG_IO_URING */
 
diff --git a/security/security.c b/security/security.c
index 22261d79f333..ef96be2f953a 100644
--- a/security/security.c
+++ b/security/security.c
@@ -2640,4 +2640,8 @@ int security_uring_sqpoll(void)
 {
 	return call_int_hook(uring_sqpoll, 0);
 }
+int security_uring_async_cmd(struct io_uring_cmd *ioucmd)
+{
+	return call_int_hook(uring_async_cmd, 0, ioucmd);
+}
 #endif /* CONFIG_IO_URING */
-- 
2.34.1

Re: [PATCH 05/17] nvme: wire-up support for async-passthru on char-device.

[Luis Chamberlain]

On Tue, Mar 08, 2022 at 08:50:53PM +0530, Kanchan Joshi wrote:
> diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c
> index 5c9cd9695519..1df270b47af5 100644
> --- a/drivers/nvme/host/ioctl.c
> +++ b/drivers/nvme/host/ioctl.c
> @@ -369,6 +469,33 @@ long nvme_ns_chr_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>  	return __nvme_ioctl(ns, cmd, (void __user *)arg);
>  }
>  
> +static int nvme_ns_async_ioctl(struct nvme_ns *ns, struct io_uring_cmd *ioucmd)
> +{
> +	int ret;
> +
> +	BUILD_BUG_ON(sizeof(struct nvme_uring_cmd_pdu) > sizeof(ioucmd->pdu));
> +
> +	switch (ioucmd->cmd_op) {
> +	case NVME_IOCTL_IO64_CMD:
> +		ret = nvme_user_cmd64(ns->ctrl, ns, NULL, ioucmd);
> +		break;
> +	default:
> +		ret = -ENOTTY;
> +	}
> +
> +	if (ret >= 0)
> +		ret = -EIOCBQUEUED;
> +	return ret;
> +}

And here I think we'll need something like this:

diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c
index ddb7e5864be6..83529adf130d 100644
--- a/drivers/nvme/host/ioctl.c
+++ b/drivers/nvme/host/ioctl.c
@@ -5,6 +5,7 @@
  */
 #include <linux/ptrace.h>	/* for force_successful_syscall_return */
 #include <linux/nvme_ioctl.h>
+#include <linux/security.h>
 #include "nvme.h"
 
 /*
@@ -524,6 +525,11 @@ static int nvme_ns_async_ioctl(struct nvme_ns *ns, struct io_uring_cmd *ioucmd)
 
 	BUILD_BUG_ON(sizeof(struct nvme_uring_cmd_pdu) > sizeof(ioucmd->pdu));
 
+	ret = security_file_ioctl(ioucmd->file, ioucmd->cmd_op,
+				  (unsigned long) ioucmd->cmd);
+	if (ret)
+		return ret;
+
 	switch (ioucmd->cmd_op) {
 	case NVME_IOCTL_IO64_CMD:
 		ret = nvme_user_cmd64(ns->ctrl, ns, NULL, ioucmd);

Re: [PATCH 03/17] io_uring: add infra and support for IORING_OP_URING_CMD

[Paul Moore]

On Fri, Mar 11, 2022 at 12:11 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
> On Thu, Mar 10, 2022 at 07:43:04PM -0700, Jens Axboe wrote:
> > On 3/10/22 6:51 PM, Luis Chamberlain wrote:
> > > On Tue, Mar 08, 2022 at 08:50:51PM +0530, Kanchan Joshi wrote:
> > >> From: Jens Axboe <axboe@kernel.dk>
> > >>
> > >> This is a file private kind of request. io_uring doesn't know what's
> > >> in this command type, it's for the file_operations->async_cmd()
> > >> handler to deal with.
> > >>
> > >> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> > >> Signed-off-by: Kanchan Joshi <joshi.k@samsung.com>
> > >> ---
> > >
> > > <-- snip -->
> > >
> > >> +static int io_uring_cmd(struct io_kiocb *req, unsigned int issue_flags)
> > >> +{
> > >> +  struct file *file = req->file;
> > >> +  int ret;
> > >> +  struct io_uring_cmd *ioucmd = &req->uring_cmd;
> > >> +
> > >> +  ioucmd->flags |= issue_flags;
> > >> +  ret = file->f_op->async_cmd(ioucmd);
> > >
> > > I think we're going to have to add a security_file_async_cmd() check
> > > before this call here. Because otherwise we're enabling to, for
> > > example, bypass security_file_ioctl() for example using the new
> > > iouring-cmd interface.
> > >
> > > Or is this already thought out with the existing security_uring_*() stuff?
> >
> > Unless the request sets .audit_skip, it'll be included already in terms
> > of logging.
>
> Neat.

[NOTE: added the audit and SELinux lists to the To/CC line]

Neat, but I think we will need to augment things to support this new
passthrough mechanism.

The issue is that folks who look at audit logs need to be able to
piece together what happened on the system using just what they have
in the logs themselves.  As things currently stand with this patchset,
the only bit of information they would have to go on would be
"uring_op=<IORING_OP_URING_CMD>" which isn't very informative :)

You'll see a similar issue in the newly proposed LSM hook below, we
need to be able to record information about not only the passthrough
command, e.g. io_uring_cmd::cmd_op, but also the underlying
device/handler so that we can put the passthrough command in the right
context (as far as I can tell io_uring_cmd::cmd_op is specific to the
device).  We might be able to leverage file_operations::owner::name
for this, e.g. "uring_passthru_dev=nvme
uring_passthru_op=<NVME_IOCTL_IO64_CMD>".

> > But I'd prefer not to lodge this in with ioctls, unless
> > we're going to be doing actual ioctls.
>
> Oh sure, I have been an advocate to ensure folks don't conflate async_cmd
> with ioctl. However it *can* enable subsystems to enable ioctl
> passthrough, but each of those subsystems need to vet for this on their
> own terms. I'd hate to see / hear some LSM surprises later.

Same :)  Thanks for bringing this up with us while the patches are
still in-progress/under-review, I think it makes for a much more
pleasant experience for everyone.

> > But definitely something to keep in mind and make sure that we're under
> > the right umbrella in terms of auditing and security.
>
> Paul, how about something like this for starters (and probably should
> be squashed into this series so its not a separate commit) ?
>
> From f3ddbe822374cc1c7002bd795c1ae486d370cbd1 Mon Sep 17 00:00:00 2001
> From: Luis Chamberlain <mcgrof@kernel.org>
> Date: Fri, 11 Mar 2022 08:55:50 -0800
> Subject: [PATCH] lsm,io_uring: add LSM hooks to for the new async_cmd file op
>
> io-uring is extending the struct file_operations to allow a new
> command which each subsystem can use to enable command passthrough.
> Add an LSM specific for the command passthrough which enables LSMs
> to inspect the command details.
>
> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
> ---
>  fs/io_uring.c                 | 5 +++++
>  include/linux/lsm_hook_defs.h | 1 +
>  include/linux/lsm_hooks.h     | 3 +++
>  include/linux/security.h      | 5 +++++
>  security/security.c           | 4 ++++
>  5 files changed, 18 insertions(+)
>
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index 3f6eacc98e31..1c4e6b2cb61a 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -4190,6 +4190,11 @@ static int io_uring_cmd_prep(struct io_kiocb *req,
>         struct io_ring_ctx *ctx = req->ctx;
>         struct io_uring_cmd *ioucmd = &req->uring_cmd;
>         u32 ucmd_flags = READ_ONCE(sqe->uring_cmd_flags);
> +       int ret;
> +
> +       ret = security_uring_async_cmd(ioucmd);
> +       if (ret)
> +               return ret;

As a quick aside, for the LSM/audit folks the lore link for the full
patchset is here:
https://lore.kernel.org/io-uring/CA+1E3rJ17F0Rz5UKUnW-LPkWDfPHXG5aeq-ocgNxHfGrxYtAuw@mail.gmail.com/T/#m605e2fb7caf33e8880683fe6b57ade4093ed0643

Similar to what was discussed above with respect to auditing, I think
we need to do some extra work here to make it easier for a LSM to put
the IO request in the proper context.  We have io_uring_cmd::cmd_op
via the @ioucmd parameter, which is good, but we need to be able to
associate that with a driver to make sense of it.  In the case of
audit we could simply use the module name string, which is probably
ideal as we would want a string anyway, but LSMs will likely want
something more machine friendly.  That isn't to say we couldn't do a
strcmp() on the module name string, but for something that aims to
push performance as much as possible, doing a strcmp() on each
operation seems a little less than optimal ;)

-- 
paul-moore.com

Re: [PATCH 05/17] nvme: wire-up support for async-passthru on char-device.

[Paul Moore]

On Fri, Mar 11, 2022 at 12:56 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
>
> On Tue, Mar 08, 2022 at 08:50:53PM +0530, Kanchan Joshi wrote:
> > diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c
> > index 5c9cd9695519..1df270b47af5 100644
> > --- a/drivers/nvme/host/ioctl.c
> > +++ b/drivers/nvme/host/ioctl.c
> > @@ -369,6 +469,33 @@ long nvme_ns_chr_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> >       return __nvme_ioctl(ns, cmd, (void __user *)arg);
> >  }
> >
> > +static int nvme_ns_async_ioctl(struct nvme_ns *ns, struct io_uring_cmd *ioucmd)
> > +{
> > +     int ret;
> > +
> > +     BUILD_BUG_ON(sizeof(struct nvme_uring_cmd_pdu) > sizeof(ioucmd->pdu));
> > +
> > +     switch (ioucmd->cmd_op) {
> > +     case NVME_IOCTL_IO64_CMD:
> > +             ret = nvme_user_cmd64(ns->ctrl, ns, NULL, ioucmd);
> > +             break;
> > +     default:
> > +             ret = -ENOTTY;
> > +     }
> > +
> > +     if (ret >= 0)
> > +             ret = -EIOCBQUEUED;
> > +     return ret;
> > +}
>
> And here I think we'll need something like this:

If we can promise that we will have a LSM hook for all of the
file_operations::async_cmd implementations that are security relevant
we could skip the LSM passthrough hook at the io_uring layer.  It
would potentially make life easier in that we don't have to worry
about putting the passthrough op in the right context, but risks
missing a LSM hook control point (it will happen at some point and
*boom* CVE).

> diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c
> index ddb7e5864be6..83529adf130d 100644
> --- a/drivers/nvme/host/ioctl.c
> +++ b/drivers/nvme/host/ioctl.c
> @@ -5,6 +5,7 @@
>   */
>  #include <linux/ptrace.h>      /* for force_successful_syscall_return */
>  #include <linux/nvme_ioctl.h>
> +#include <linux/security.h>
>  #include "nvme.h"
>
>  /*
> @@ -524,6 +525,11 @@ static int nvme_ns_async_ioctl(struct nvme_ns *ns, struct io_uring_cmd *ioucmd)
>
>         BUILD_BUG_ON(sizeof(struct nvme_uring_cmd_pdu) > sizeof(ioucmd->pdu));
>
> +       ret = security_file_ioctl(ioucmd->file, ioucmd->cmd_op,
> +                                 (unsigned long) ioucmd->cmd);
> +       if (ret)
> +               return ret;
> +
>         switch (ioucmd->cmd_op) {
>         case NVME_IOCTL_IO64_CMD:
>                 ret = nvme_user_cmd64(ns->ctrl, ns, NULL, ioucmd);

-- 
paul-moore.com

Re: [PATCH 03/17] io_uring: add infra and support for IORING_OP_URING_CMD

[Luis Chamberlain]

On Fri, Mar 11, 2022 at 01:47:51PM -0500, Paul Moore wrote:
> On Fri, Mar 11, 2022 at 12:11 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
> > On Thu, Mar 10, 2022 at 07:43:04PM -0700, Jens Axboe wrote:
> > > On 3/10/22 6:51 PM, Luis Chamberlain wrote:
> > > > On Tue, Mar 08, 2022 at 08:50:51PM +0530, Kanchan Joshi wrote:
> > > >> From: Jens Axboe <axboe@kernel.dk>
> > > >>
> > > >> This is a file private kind of request. io_uring doesn't know what's
> > > >> in this command type, it's for the file_operations->async_cmd()
> > > >> handler to deal with.
> > > >>
> > > >> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> > > >> Signed-off-by: Kanchan Joshi <joshi.k@samsung.com>
> > > >> ---
> > > >
> > > > <-- snip -->
> > > >
> > > >> +static int io_uring_cmd(struct io_kiocb *req, unsigned int issue_flags)
> > > >> +{
> > > >> +  struct file *file = req->file;
> > > >> +  int ret;
> > > >> +  struct io_uring_cmd *ioucmd = &req->uring_cmd;
> > > >> +
> > > >> +  ioucmd->flags |= issue_flags;
> > > >> +  ret = file->f_op->async_cmd(ioucmd);
> > > >
> > > > I think we're going to have to add a security_file_async_cmd() check
> > > > before this call here. Because otherwise we're enabling to, for
> > > > example, bypass security_file_ioctl() for example using the new
> > > > iouring-cmd interface.
> > > >
> > > > Or is this already thought out with the existing security_uring_*() stuff?
> > >
> > > Unless the request sets .audit_skip, it'll be included already in terms
> > > of logging.
> >
> > Neat.
> 
> [NOTE: added the audit and SELinux lists to the To/CC line]
> 
> Neat, but I think we will need to augment things to support this new
> passthrough mechanism.

That's what my spidey instincts told me.

> The issue is that folks who look at audit logs need to be able to
> piece together what happened on the system using just what they have
> in the logs themselves.  As things currently stand with this patchset,
> the only bit of information they would have to go on would be
> "uring_op=<IORING_OP_URING_CMD>" which isn't very informative :)
> 
> You'll see a similar issue in the newly proposed LSM hook below, we
> need to be able to record information about not only the passthrough
> command, e.g. io_uring_cmd::cmd_op, but also the underlying
> device/handler so that we can put the passthrough command in the right
> context (as far as I can tell io_uring_cmd::cmd_op is specific to the
> device).  We might be able to leverage file_operations::owner::name
> for this, e.g. "uring_passthru_dev=nvme
> uring_passthru_op=<NVME_IOCTL_IO64_CMD>".

OK...

> > > But I'd prefer not to lodge this in with ioctls, unless
> > > we're going to be doing actual ioctls.
> >
> > Oh sure, I have been an advocate to ensure folks don't conflate async_cmd
> > with ioctl. However it *can* enable subsystems to enable ioctl
> > passthrough, but each of those subsystems need to vet for this on their
> > own terms. I'd hate to see / hear some LSM surprises later.
> 
> Same :)  Thanks for bringing this up with us while the patches are
> still in-progress/under-review, I think it makes for a much more
> pleasant experience for everyone.

Sure thing.

> > > But definitely something to keep in mind and make sure that we're under
> > > the right umbrella in terms of auditing and security.
> >
> > Paul, how about something like this for starters (and probably should
> > be squashed into this series so its not a separate commit) ?
> >
> > From f3ddbe822374cc1c7002bd795c1ae486d370cbd1 Mon Sep 17 00:00:00 2001
> > From: Luis Chamberlain <mcgrof@kernel.org>
> > Date: Fri, 11 Mar 2022 08:55:50 -0800
> > Subject: [PATCH] lsm,io_uring: add LSM hooks to for the new async_cmd file op
> >
> > io-uring is extending the struct file_operations to allow a new
> > command which each subsystem can use to enable command passthrough.
> > Add an LSM specific for the command passthrough which enables LSMs
> > to inspect the command details.
> >
> > Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
> > ---
> >  fs/io_uring.c                 | 5 +++++
> >  include/linux/lsm_hook_defs.h | 1 +
> >  include/linux/lsm_hooks.h     | 3 +++
> >  include/linux/security.h      | 5 +++++
> >  security/security.c           | 4 ++++
> >  5 files changed, 18 insertions(+)
> >
> > diff --git a/fs/io_uring.c b/fs/io_uring.c
> > index 3f6eacc98e31..1c4e6b2cb61a 100644
> > --- a/fs/io_uring.c
> > +++ b/fs/io_uring.c
> > @@ -4190,6 +4190,11 @@ static int io_uring_cmd_prep(struct io_kiocb *req,
> >         struct io_ring_ctx *ctx = req->ctx;
> >         struct io_uring_cmd *ioucmd = &req->uring_cmd;
> >         u32 ucmd_flags = READ_ONCE(sqe->uring_cmd_flags);
> > +       int ret;
> > +
> > +       ret = security_uring_async_cmd(ioucmd);
> > +       if (ret)
> > +               return ret;
> 
> As a quick aside, for the LSM/audit folks the lore link for the full
> patchset is here:
> https://lore.kernel.org/io-uring/CA+1E3rJ17F0Rz5UKUnW-LPkWDfPHXG5aeq-ocgNxHfGrxYtAuw@mail.gmail.com/T/#m605e2fb7caf33e8880683fe6b57ade4093ed0643
> 
> Similar to what was discussed above with respect to auditing, I think
> we need to do some extra work here to make it easier for a LSM to put
> the IO request in the proper context.  We have io_uring_cmd::cmd_op
> via the @ioucmd parameter, which is good, but we need to be able to
> associate that with a driver to make sense of it.

It may not always be a driver, it can be built-in stuff.

> In the case of
> audit we could simply use the module name string, which is probably
> ideal as we would want a string anyway, but LSMs will likely want
> something more machine friendly.  That isn't to say we couldn't do a
> strcmp() on the module name string, but for something that aims to
> push performance as much as possible, doing a strcmp() on each
> operation seems a little less than optimal ;)

Yes this is a super hot path...

  Luis

Re: [PATCH 05/17] nvme: wire-up support for async-passthru on char-device.

[Luis Chamberlain]

On Fri, Mar 11, 2022 at 01:53:03PM -0500, Paul Moore wrote:
> On Fri, Mar 11, 2022 at 12:56 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
> >
> > On Tue, Mar 08, 2022 at 08:50:53PM +0530, Kanchan Joshi wrote:
> > > diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c
> > > index 5c9cd9695519..1df270b47af5 100644
> > > --- a/drivers/nvme/host/ioctl.c
> > > +++ b/drivers/nvme/host/ioctl.c
> > > @@ -369,6 +469,33 @@ long nvme_ns_chr_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> > >       return __nvme_ioctl(ns, cmd, (void __user *)arg);
> > >  }
> > >
> > > +static int nvme_ns_async_ioctl(struct nvme_ns *ns, struct io_uring_cmd *ioucmd)
> > > +{
> > > +     int ret;
> > > +
> > > +     BUILD_BUG_ON(sizeof(struct nvme_uring_cmd_pdu) > sizeof(ioucmd->pdu));
> > > +
> > > +     switch (ioucmd->cmd_op) {
> > > +     case NVME_IOCTL_IO64_CMD:
> > > +             ret = nvme_user_cmd64(ns->ctrl, ns, NULL, ioucmd);
> > > +             break;
> > > +     default:
> > > +             ret = -ENOTTY;
> > > +     }
> > > +
> > > +     if (ret >= 0)
> > > +             ret = -EIOCBQUEUED;
> > > +     return ret;
> > > +}
> >
> > And here I think we'll need something like this:
> 
> If we can promise that we will have a LSM hook for all of the
> file_operations::async_cmd implementations that are security relevant
> we could skip the LSM passthrough hook at the io_uring layer.

There is no way to ensure this unless perhaps we cake that into
the API somehow... Or have a registration system for setting the
respctive file ops / LSM.

> It
> would potentially make life easier in that we don't have to worry
> about putting the passthrough op in the right context, but risks
> missing a LSM hook control point (it will happen at some point and
> *boom* CVE).

Precicely my concern. So we either open code this and ask folks
to do this or I think we do a registration and require both ops
and the the LSM hook at registration.

I think this should be enough information to get Kanchan rolling
on the LSM side.

  Luis

Re: [PATCH 03/17] io_uring: add infra and support for IORING_OP_URING_CMD

[Paul Moore]

On Fri, Mar 11, 2022 at 3:57 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
> On Fri, Mar 11, 2022 at 01:47:51PM -0500, Paul Moore wrote:

...

> > Similar to what was discussed above with respect to auditing, I think
> > we need to do some extra work here to make it easier for a LSM to put
> > the IO request in the proper context.  We have io_uring_cmd::cmd_op
> > via the @ioucmd parameter, which is good, but we need to be able to
> > associate that with a driver to make sense of it.
>
> It may not always be a driver, it can be built-in stuff.

Good point, but I believe the argument still applies.  LSMs are going
to need some way to put the cmd_op token in the proper context so that
security policy can be properly enforced.

-- 
paul-moore.com

Re: [PATCH 03/17] io_uring: add infra and support for IORING_OP_URING_CMD

[Casey Schaufler]

On 3/11/2022 9:11 AM, Luis Chamberlain wrote:
> On Thu, Mar 10, 2022 at 07:43:04PM -0700, Jens Axboe wrote:
>> On 3/10/22 6:51 PM, Luis Chamberlain wrote:
>>> On Tue, Mar 08, 2022 at 08:50:51PM +0530, Kanchan Joshi wrote:
>>>> From: Jens Axboe <axboe@kernel.dk>
>>>>
>>>> This is a file private kind of request. io_uring doesn't know what's
>>>> in this command type, it's for the file_operations->async_cmd()
>>>> handler to deal with.
>>>>
>>>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>>>> Signed-off-by: Kanchan Joshi <joshi.k@samsung.com>
>>>> ---
>>> <-- snip -->
>>>
>>>> +static int io_uring_cmd(struct io_kiocb *req, unsigned int issue_flags)
>>>> +{
>>>> +	struct file *file = req->file;
>>>> +	int ret;
>>>> +	struct io_uring_cmd *ioucmd = &req->uring_cmd;
>>>> +
>>>> +	ioucmd->flags |= issue_flags;
>>>> +	ret = file->f_op->async_cmd(ioucmd);
>>> I think we're going to have to add a security_file_async_cmd() check
>>> before this call here. Because otherwise we're enabling to, for
>>> example, bypass security_file_ioctl() for example using the new
>>> iouring-cmd interface.
>>>
>>> Or is this already thought out with the existing security_uring_*() stuff?
>> Unless the request sets .audit_skip, it'll be included already in terms
>> of logging.
> Neat.
>
>> But I'd prefer not to lodge this in with ioctls, unless
>> we're going to be doing actual ioctls.
> Oh sure, I have been an advocate to ensure folks don't conflate async_cmd
> with ioctl. However it *can* enable subsystems to enable ioctl
> passthrough, but each of those subsystems need to vet for this on their
> own terms. I'd hate to see / hear some LSM surprises later.
>
>> But definitely something to keep in mind and make sure that we're under
>> the right umbrella in terms of auditing and security.
> Paul, how about something like this for starters (and probably should
> be squashed into this series so its not a separate commit) ?
>
> >From f3ddbe822374cc1c7002bd795c1ae486d370cbd1 Mon Sep 17 00:00:00 2001
> From: Luis Chamberlain <mcgrof@kernel.org>
> Date: Fri, 11 Mar 2022 08:55:50 -0800
> Subject: [PATCH] lsm,io_uring: add LSM hooks to for the new async_cmd file op
>
> io-uring is extending the struct file_operations to allow a new
> command which each subsystem can use to enable command passthrough.
> Add an LSM specific for the command passthrough which enables LSMs
> to inspect the command details.
>
> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
> ---
>   fs/io_uring.c                 | 5 +++++
>   include/linux/lsm_hook_defs.h | 1 +
>   include/linux/lsm_hooks.h     | 3 +++
>   include/linux/security.h      | 5 +++++
>   security/security.c           | 4 ++++
>   5 files changed, 18 insertions(+)
>
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index 3f6eacc98e31..1c4e6b2cb61a 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -4190,6 +4190,11 @@ static int io_uring_cmd_prep(struct io_kiocb *req,
>   	struct io_ring_ctx *ctx = req->ctx;
>   	struct io_uring_cmd *ioucmd = &req->uring_cmd;
>   	u32 ucmd_flags = READ_ONCE(sqe->uring_cmd_flags);
> +	int ret;
> +
> +	ret = security_uring_async_cmd(ioucmd);
> +	if (ret)
> +		return ret;
>   
>   	if (!req->file->f_op->async_cmd)
>   		return -EOPNOTSUPP;
> diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
> index 819ec92dc2a8..4a20f8e6b295 100644
> --- a/include/linux/lsm_hook_defs.h
> +++ b/include/linux/lsm_hook_defs.h
> @@ -404,4 +404,5 @@ LSM_HOOK(int, 0, perf_event_write, struct perf_event *event)
>   #ifdef CONFIG_IO_URING
>   LSM_HOOK(int, 0, uring_override_creds, const struct cred *new)
>   LSM_HOOK(int, 0, uring_sqpoll, void)
> +LSM_HOOK(int, 0, uring_async_cmd, struct io_uring_cmd *ioucmd)
>   #endif /* CONFIG_IO_URING */
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 3bf5c658bc44..21b18cf138c2 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -1569,6 +1569,9 @@
>    *      Check whether the current task is allowed to spawn a io_uring polling
>    *      thread (IORING_SETUP_SQPOLL).
>    *
> + * @uring_async_cmd:
> + *      Check whether the file_operations async_cmd is allowed to run.
> + *
>    */
>   union security_list_options {
>   	#define LSM_HOOK(RET, DEFAULT, NAME, ...) RET (*NAME)(__VA_ARGS__);
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 6d72772182c8..4d7f72813d75 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -2041,6 +2041,7 @@ static inline int security_perf_event_write(struct perf_event *event)
>   #ifdef CONFIG_SECURITY
>   extern int security_uring_override_creds(const struct cred *new);
>   extern int security_uring_sqpoll(void);
> +extern int security_uring_async_cmd(struct io_uring_cmd *ioucmd);
>   #else
>   static inline int security_uring_override_creds(const struct cred *new)
>   {
> @@ -2050,6 +2051,10 @@ static inline int security_uring_sqpoll(void)
>   {
>   	return 0;
>   }
> +static inline int security_uring_async_cmd(struct io_uring_cmd *ioucmd)
> +{
> +	return 0;
> +}
>   #endif /* CONFIG_SECURITY */
>   #endif /* CONFIG_IO_URING */
>   
> diff --git a/security/security.c b/security/security.c
> index 22261d79f333..ef96be2f953a 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -2640,4 +2640,8 @@ int security_uring_sqpoll(void)
>   {
>   	return call_int_hook(uring_sqpoll, 0);
>   }
> +int security_uring_async_cmd(struct io_uring_cmd *ioucmd)
> +{
> +	return call_int_hook(uring_async_cmd, 0, ioucmd);

I don't have a good understanding of what information is in ioucmd.
I am afraid that there may not be enough for a security module to
make appropriate decisions in all cases. I am especially concerned
about the modules that use path hooks, but based on the issues we've
always had with ioctl and the like I fear for all cases.

> +}
>   #endif /* CONFIG_IO_URING */

Re: [PATCH 03/17] io_uring: add infra and support for IORING_OP_URING_CMD

[Luis Chamberlain]

On Mon, Mar 14, 2022 at 09:25:35AM -0700, Casey Schaufler wrote:
> On 3/11/2022 9:11 AM, Luis Chamberlain wrote:
> > On Thu, Mar 10, 2022 at 07:43:04PM -0700, Jens Axboe wrote:
> > > On 3/10/22 6:51 PM, Luis Chamberlain wrote:
> > > > On Tue, Mar 08, 2022 at 08:50:51PM +0530, Kanchan Joshi wrote:
> > > > > From: Jens Axboe <axboe@kernel.dk>
> > > > > 
> > > > > This is a file private kind of request. io_uring doesn't know what's
> > > > > in this command type, it's for the file_operations->async_cmd()
> > > > > handler to deal with.
> > > > > 
> > > > > Signed-off-by: Jens Axboe <axboe@kernel.dk>
> > > > > Signed-off-by: Kanchan Joshi <joshi.k@samsung.com>
> > > > > ---
> > > > <-- snip -->
> > > > 
> > > > > +static int io_uring_cmd(struct io_kiocb *req, unsigned int issue_flags)
> > > > > +{
> > > > > +	struct file *file = req->file;
> > > > > +	int ret;
> > > > > +	struct io_uring_cmd *ioucmd = &req->uring_cmd;
> > > > > +
> > > > > +	ioucmd->flags |= issue_flags;
> > > > > +	ret = file->f_op->async_cmd(ioucmd);
> > > > I think we're going to have to add a security_file_async_cmd() check
> > > > before this call here. Because otherwise we're enabling to, for
> > > > example, bypass security_file_ioctl() for example using the new
> > > > iouring-cmd interface.
> > > > 
> > > > Or is this already thought out with the existing security_uring_*() stuff?
> > > Unless the request sets .audit_skip, it'll be included already in terms
> > > of logging.
> > Neat.
> > 
> > > But I'd prefer not to lodge this in with ioctls, unless
> > > we're going to be doing actual ioctls.
> > Oh sure, I have been an advocate to ensure folks don't conflate async_cmd
> > with ioctl. However it *can* enable subsystems to enable ioctl
> > passthrough, but each of those subsystems need to vet for this on their
> > own terms. I'd hate to see / hear some LSM surprises later.
> > 
> > > But definitely something to keep in mind and make sure that we're under
> > > the right umbrella in terms of auditing and security.
> > Paul, how about something like this for starters (and probably should
> > be squashed into this series so its not a separate commit) ?
> > 
> > >From f3ddbe822374cc1c7002bd795c1ae486d370cbd1 Mon Sep 17 00:00:00 2001
> > From: Luis Chamberlain <mcgrof@kernel.org>
> > Date: Fri, 11 Mar 2022 08:55:50 -0800
> > Subject: [PATCH] lsm,io_uring: add LSM hooks to for the new async_cmd file op
> > 
> > io-uring is extending the struct file_operations to allow a new
> > command which each subsystem can use to enable command passthrough.
> > Add an LSM specific for the command passthrough which enables LSMs
> > to inspect the command details.
> > 
> > Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
> > ---
> >   fs/io_uring.c                 | 5 +++++
> >   include/linux/lsm_hook_defs.h | 1 +
> >   include/linux/lsm_hooks.h     | 3 +++
> >   include/linux/security.h      | 5 +++++
> >   security/security.c           | 4 ++++
> >   5 files changed, 18 insertions(+)
> > 
> > diff --git a/fs/io_uring.c b/fs/io_uring.c
> > index 3f6eacc98e31..1c4e6b2cb61a 100644
> > --- a/fs/io_uring.c
> > +++ b/fs/io_uring.c
> > @@ -4190,6 +4190,11 @@ static int io_uring_cmd_prep(struct io_kiocb *req,
> >   	struct io_ring_ctx *ctx = req->ctx;
> >   	struct io_uring_cmd *ioucmd = &req->uring_cmd;
> >   	u32 ucmd_flags = READ_ONCE(sqe->uring_cmd_flags);
> > +	int ret;
> > +
> > +	ret = security_uring_async_cmd(ioucmd);
> > +	if (ret)
> > +		return ret;
> >   	if (!req->file->f_op->async_cmd)
> >   		return -EOPNOTSUPP;
> > diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
> > index 819ec92dc2a8..4a20f8e6b295 100644
> > --- a/include/linux/lsm_hook_defs.h
> > +++ b/include/linux/lsm_hook_defs.h
> > @@ -404,4 +404,5 @@ LSM_HOOK(int, 0, perf_event_write, struct perf_event *event)
> >   #ifdef CONFIG_IO_URING
> >   LSM_HOOK(int, 0, uring_override_creds, const struct cred *new)
> >   LSM_HOOK(int, 0, uring_sqpoll, void)
> > +LSM_HOOK(int, 0, uring_async_cmd, struct io_uring_cmd *ioucmd)
> >   #endif /* CONFIG_IO_URING */
> > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> > index 3bf5c658bc44..21b18cf138c2 100644
> > --- a/include/linux/lsm_hooks.h
> > +++ b/include/linux/lsm_hooks.h
> > @@ -1569,6 +1569,9 @@
> >    *      Check whether the current task is allowed to spawn a io_uring polling
> >    *      thread (IORING_SETUP_SQPOLL).
> >    *
> > + * @uring_async_cmd:
> > + *      Check whether the file_operations async_cmd is allowed to run.
> > + *
> >    */
> >   union security_list_options {
> >   	#define LSM_HOOK(RET, DEFAULT, NAME, ...) RET (*NAME)(__VA_ARGS__);
> > diff --git a/include/linux/security.h b/include/linux/security.h
> > index 6d72772182c8..4d7f72813d75 100644
> > --- a/include/linux/security.h
> > +++ b/include/linux/security.h
> > @@ -2041,6 +2041,7 @@ static inline int security_perf_event_write(struct perf_event *event)
> >   #ifdef CONFIG_SECURITY
> >   extern int security_uring_override_creds(const struct cred *new);
> >   extern int security_uring_sqpoll(void);
> > +extern int security_uring_async_cmd(struct io_uring_cmd *ioucmd);
> >   #else
> >   static inline int security_uring_override_creds(const struct cred *new)
> >   {
> > @@ -2050,6 +2051,10 @@ static inline int security_uring_sqpoll(void)
> >   {
> >   	return 0;
> >   }
> > +static inline int security_uring_async_cmd(struct io_uring_cmd *ioucmd)
> > +{
> > +	return 0;
> > +}
> >   #endif /* CONFIG_SECURITY */
> >   #endif /* CONFIG_IO_URING */
> > diff --git a/security/security.c b/security/security.c
> > index 22261d79f333..ef96be2f953a 100644
> > --- a/security/security.c
> > +++ b/security/security.c
> > @@ -2640,4 +2640,8 @@ int security_uring_sqpoll(void)
> >   {
> >   	return call_int_hook(uring_sqpoll, 0);
> >   }
> > +int security_uring_async_cmd(struct io_uring_cmd *ioucmd)
> > +{
> > +	return call_int_hook(uring_async_cmd, 0, ioucmd);
> 
> I don't have a good understanding of what information is in ioucmd.
> I am afraid that there may not be enough for a security module to
> make appropriate decisions in all cases. I am especially concerned
> about the modules that use path hooks, but based on the issues we've
> always had with ioctl and the like I fear for all cases.

As Paul pointed out, this particular LSM hook would not be needed if we can
somehow ensure users of the cmd path use their respective LSMs there. It
is not easy to force users to have the LSM hook to be used, one idea
might be to have a registration mechanism which allows users to also
specify the LSM hook, but these can vary in arguments, so perhaps then
what is needed is the LSM type in enum form, and internally we have a
mapping of these. That way we slowly itemize which cmds we *do* allow
for, thus vetting at the same time a respective LSM hook. Thoughts?

  Luis

Re: [PATCH 03/17] io_uring: add infra and support for IORING_OP_URING_CMD

[Casey Schaufler]

On 3/14/2022 9:32 AM, Luis Chamberlain wrote:
> On Mon, Mar 14, 2022 at 09:25:35AM -0700, Casey Schaufler wrote:
>> On 3/11/2022 9:11 AM, Luis Chamberlain wrote:
>>> On Thu, Mar 10, 2022 at 07:43:04PM -0700, Jens Axboe wrote:
>>>> On 3/10/22 6:51 PM, Luis Chamberlain wrote:
>>>>> On Tue, Mar 08, 2022 at 08:50:51PM +0530, Kanchan Joshi wrote:
>>>>>> From: Jens Axboe <axboe@kernel.dk>
>>>>>>
>>>>>> This is a file private kind of request. io_uring doesn't know what's
>>>>>> in this command type, it's for the file_operations->async_cmd()
>>>>>> handler to deal with.
>>>>>>
>>>>>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>>>>>> Signed-off-by: Kanchan Joshi <joshi.k@samsung.com>
>>>>>> ---
>>>>> <-- snip -->
>>>>>
>>>>>> +static int io_uring_cmd(struct io_kiocb *req, unsigned int issue_flags)
>>>>>> +{
>>>>>> +	struct file *file = req->file;
>>>>>> +	int ret;
>>>>>> +	struct io_uring_cmd *ioucmd = &req->uring_cmd;
>>>>>> +
>>>>>> +	ioucmd->flags |= issue_flags;
>>>>>> +	ret = file->f_op->async_cmd(ioucmd);
>>>>> I think we're going to have to add a security_file_async_cmd() check
>>>>> before this call here. Because otherwise we're enabling to, for
>>>>> example, bypass security_file_ioctl() for example using the new
>>>>> iouring-cmd interface.
>>>>>
>>>>> Or is this already thought out with the existing security_uring_*() stuff?
>>>> Unless the request sets .audit_skip, it'll be included already in terms
>>>> of logging.
>>> Neat.
>>>
>>>> But I'd prefer not to lodge this in with ioctls, unless
>>>> we're going to be doing actual ioctls.
>>> Oh sure, I have been an advocate to ensure folks don't conflate async_cmd
>>> with ioctl. However it *can* enable subsystems to enable ioctl
>>> passthrough, but each of those subsystems need to vet for this on their
>>> own terms. I'd hate to see / hear some LSM surprises later.
>>>
>>>> But definitely something to keep in mind and make sure that we're under
>>>> the right umbrella in terms of auditing and security.
>>> Paul, how about something like this for starters (and probably should
>>> be squashed into this series so its not a separate commit) ?
>>>
>>> >From f3ddbe822374cc1c7002bd795c1ae486d370cbd1 Mon Sep 17 00:00:00 2001
>>> From: Luis Chamberlain <mcgrof@kernel.org>
>>> Date: Fri, 11 Mar 2022 08:55:50 -0800
>>> Subject: [PATCH] lsm,io_uring: add LSM hooks to for the new async_cmd file op
>>>
>>> io-uring is extending the struct file_operations to allow a new
>>> command which each subsystem can use to enable command passthrough.
>>> Add an LSM specific for the command passthrough which enables LSMs
>>> to inspect the command details.
>>>
>>> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
>>> ---
>>>    fs/io_uring.c                 | 5 +++++
>>>    include/linux/lsm_hook_defs.h | 1 +
>>>    include/linux/lsm_hooks.h     | 3 +++
>>>    include/linux/security.h      | 5 +++++
>>>    security/security.c           | 4 ++++
>>>    5 files changed, 18 insertions(+)
>>>
>>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>>> index 3f6eacc98e31..1c4e6b2cb61a 100644
>>> --- a/fs/io_uring.c
>>> +++ b/fs/io_uring.c
>>> @@ -4190,6 +4190,11 @@ static int io_uring_cmd_prep(struct io_kiocb *req,
>>>    	struct io_ring_ctx *ctx = req->ctx;
>>>    	struct io_uring_cmd *ioucmd = &req->uring_cmd;
>>>    	u32 ucmd_flags = READ_ONCE(sqe->uring_cmd_flags);
>>> +	int ret;
>>> +
>>> +	ret = security_uring_async_cmd(ioucmd);
>>> +	if (ret)
>>> +		return ret;
>>>    	if (!req->file->f_op->async_cmd)
>>>    		return -EOPNOTSUPP;
>>> diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
>>> index 819ec92dc2a8..4a20f8e6b295 100644
>>> --- a/include/linux/lsm_hook_defs.h
>>> +++ b/include/linux/lsm_hook_defs.h
>>> @@ -404,4 +404,5 @@ LSM_HOOK(int, 0, perf_event_write, struct perf_event *event)
>>>    #ifdef CONFIG_IO_URING
>>>    LSM_HOOK(int, 0, uring_override_creds, const struct cred *new)
>>>    LSM_HOOK(int, 0, uring_sqpoll, void)
>>> +LSM_HOOK(int, 0, uring_async_cmd, struct io_uring_cmd *ioucmd)
>>>    #endif /* CONFIG_IO_URING */
>>> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
>>> index 3bf5c658bc44..21b18cf138c2 100644
>>> --- a/include/linux/lsm_hooks.h
>>> +++ b/include/linux/lsm_hooks.h
>>> @@ -1569,6 +1569,9 @@
>>>     *      Check whether the current task is allowed to spawn a io_uring polling
>>>     *      thread (IORING_SETUP_SQPOLL).
>>>     *
>>> + * @uring_async_cmd:
>>> + *      Check whether the file_operations async_cmd is allowed to run.
>>> + *
>>>     */
>>>    union security_list_options {
>>>    	#define LSM_HOOK(RET, DEFAULT, NAME, ...) RET (*NAME)(__VA_ARGS__);
>>> diff --git a/include/linux/security.h b/include/linux/security.h
>>> index 6d72772182c8..4d7f72813d75 100644
>>> --- a/include/linux/security.h
>>> +++ b/include/linux/security.h
>>> @@ -2041,6 +2041,7 @@ static inline int security_perf_event_write(struct perf_event *event)
>>>    #ifdef CONFIG_SECURITY
>>>    extern int security_uring_override_creds(const struct cred *new);
>>>    extern int security_uring_sqpoll(void);
>>> +extern int security_uring_async_cmd(struct io_uring_cmd *ioucmd);
>>>    #else
>>>    static inline int security_uring_override_creds(const struct cred *new)
>>>    {
>>> @@ -2050,6 +2051,10 @@ static inline int security_uring_sqpoll(void)
>>>    {
>>>    	return 0;
>>>    }
>>> +static inline int security_uring_async_cmd(struct io_uring_cmd *ioucmd)
>>> +{
>>> +	return 0;
>>> +}
>>>    #endif /* CONFIG_SECURITY */
>>>    #endif /* CONFIG_IO_URING */
>>> diff --git a/security/security.c b/security/security.c
>>> index 22261d79f333..ef96be2f953a 100644
>>> --- a/security/security.c
>>> +++ b/security/security.c
>>> @@ -2640,4 +2640,8 @@ int security_uring_sqpoll(void)
>>>    {
>>>    	return call_int_hook(uring_sqpoll, 0);
>>>    }
>>> +int security_uring_async_cmd(struct io_uring_cmd *ioucmd)
>>> +{
>>> +	return call_int_hook(uring_async_cmd, 0, ioucmd);
>> I don't have a good understanding of what information is in ioucmd.
>> I am afraid that there may not be enough for a security module to
>> make appropriate decisions in all cases. I am especially concerned
>> about the modules that use path hooks, but based on the issues we've
>> always had with ioctl and the like I fear for all cases.
> As Paul pointed out, this particular LSM hook would not be needed if we can
> somehow ensure users of the cmd path use their respective LSMs there. It
> is not easy to force users to have the LSM hook to be used, one idea
> might be to have a registration mechanism which allows users to also
> specify the LSM hook, but these can vary in arguments, so perhaps then
> what is needed is the LSM type in enum form, and internally we have a
> mapping of these. That way we slowly itemize which cmds we *do* allow
> for, thus vetting at the same time a respective LSM hook. Thoughts?

tl;dr - Yuck.

I don't see how your registration mechanism would be easier than
getting "users of the cmd path" to use the LSM mechanism the way
everyone else does. What it would do is pass responsibility for
dealing with LSM to the io_uring core team. Experience has shown
that dealing with the security issues after the fact is much
harder than doing it up front, even when developers wail about
the burden. Sure, LSM is an unpleasant interface/mechanism, but
so is locking, and no one gets away without addressing that.
My $0.02.

>
>    Luis

Re: [PATCH 03/17] io_uring: add infra and support for IORING_OP_URING_CMD

[Luis Chamberlain]

On Mon, Mar 14, 2022 at 11:05:41AM -0700, Casey Schaufler wrote:
> On 3/14/2022 9:32 AM, Luis Chamberlain wrote:
> > On Mon, Mar 14, 2022 at 09:25:35AM -0700, Casey Schaufler wrote:
> > > On 3/11/2022 9:11 AM, Luis Chamberlain wrote:
> > > > On Thu, Mar 10, 2022 at 07:43:04PM -0700, Jens Axboe wrote:
> > > > > On 3/10/22 6:51 PM, Luis Chamberlain wrote:
> > > > > > On Tue, Mar 08, 2022 at 08:50:51PM +0530, Kanchan Joshi wrote:
> > > > > > > From: Jens Axboe <axboe@kernel.dk>
> > > > > > > 
> > > > > > > This is a file private kind of request. io_uring doesn't know what's
> > > > > > > in this command type, it's for the file_operations->async_cmd()
> > > > > > > handler to deal with.
> > > > > > > 
> > > > > > > Signed-off-by: Jens Axboe <axboe@kernel.dk>
> > > > > > > Signed-off-by: Kanchan Joshi <joshi.k@samsung.com>
> > > > > > > ---
> > > > > > <-- snip -->
> > > > > > 
> > > > > > > +static int io_uring_cmd(struct io_kiocb *req, unsigned int issue_flags)
> > > > > > > +{
> > > > > > > +	struct file *file = req->file;
> > > > > > > +	int ret;
> > > > > > > +	struct io_uring_cmd *ioucmd = &req->uring_cmd;
> > > > > > > +
> > > > > > > +	ioucmd->flags |= issue_flags;
> > > > > > > +	ret = file->f_op->async_cmd(ioucmd);
> > > > > > I think we're going to have to add a security_file_async_cmd() check
> > > > > > before this call here. Because otherwise we're enabling to, for
> > > > > > example, bypass security_file_ioctl() for example using the new
> > > > > > iouring-cmd interface.
> > > > > > 
> > > > > > Or is this already thought out with the existing security_uring_*() stuff?
> > > > > Unless the request sets .audit_skip, it'll be included already in terms
> > > > > of logging.
> > > > Neat.
> > > > 
> > > > > But I'd prefer not to lodge this in with ioctls, unless
> > > > > we're going to be doing actual ioctls.
> > > > Oh sure, I have been an advocate to ensure folks don't conflate async_cmd
> > > > with ioctl. However it *can* enable subsystems to enable ioctl
> > > > passthrough, but each of those subsystems need to vet for this on their
> > > > own terms. I'd hate to see / hear some LSM surprises later.
> > > > 
> > > > > But definitely something to keep in mind and make sure that we're under
> > > > > the right umbrella in terms of auditing and security.
> > > > Paul, how about something like this for starters (and probably should
> > > > be squashed into this series so its not a separate commit) ?
> > > > 
> > > > >From f3ddbe822374cc1c7002bd795c1ae486d370cbd1 Mon Sep 17 00:00:00 2001
> > > > From: Luis Chamberlain <mcgrof@kernel.org>
> > > > Date: Fri, 11 Mar 2022 08:55:50 -0800
> > > > Subject: [PATCH] lsm,io_uring: add LSM hooks to for the new async_cmd file op
> > > > 
> > > > io-uring is extending the struct file_operations to allow a new
> > > > command which each subsystem can use to enable command passthrough.
> > > > Add an LSM specific for the command passthrough which enables LSMs
> > > > to inspect the command details.
> > > > 
> > > > Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
> > > > ---
> > > >    fs/io_uring.c                 | 5 +++++
> > > >    include/linux/lsm_hook_defs.h | 1 +
> > > >    include/linux/lsm_hooks.h     | 3 +++
> > > >    include/linux/security.h      | 5 +++++
> > > >    security/security.c           | 4 ++++
> > > >    5 files changed, 18 insertions(+)
> > > > 
> > > > diff --git a/fs/io_uring.c b/fs/io_uring.c
> > > > index 3f6eacc98e31..1c4e6b2cb61a 100644
> > > > --- a/fs/io_uring.c
> > > > +++ b/fs/io_uring.c
> > > > @@ -4190,6 +4190,11 @@ static int io_uring_cmd_prep(struct io_kiocb *req,
> > > >    	struct io_ring_ctx *ctx = req->ctx;
> > > >    	struct io_uring_cmd *ioucmd = &req->uring_cmd;
> > > >    	u32 ucmd_flags = READ_ONCE(sqe->uring_cmd_flags);
> > > > +	int ret;
> > > > +
> > > > +	ret = security_uring_async_cmd(ioucmd);
> > > > +	if (ret)
> > > > +		return ret;
> > > >    	if (!req->file->f_op->async_cmd)
> > > >    		return -EOPNOTSUPP;
> > > > diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
> > > > index 819ec92dc2a8..4a20f8e6b295 100644
> > > > --- a/include/linux/lsm_hook_defs.h
> > > > +++ b/include/linux/lsm_hook_defs.h
> > > > @@ -404,4 +404,5 @@ LSM_HOOK(int, 0, perf_event_write, struct perf_event *event)
> > > >    #ifdef CONFIG_IO_URING
> > > >    LSM_HOOK(int, 0, uring_override_creds, const struct cred *new)
> > > >    LSM_HOOK(int, 0, uring_sqpoll, void)
> > > > +LSM_HOOK(int, 0, uring_async_cmd, struct io_uring_cmd *ioucmd)
> > > >    #endif /* CONFIG_IO_URING */
> > > > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> > > > index 3bf5c658bc44..21b18cf138c2 100644
> > > > --- a/include/linux/lsm_hooks.h
> > > > +++ b/include/linux/lsm_hooks.h
> > > > @@ -1569,6 +1569,9 @@
> > > >     *      Check whether the current task is allowed to spawn a io_uring polling
> > > >     *      thread (IORING_SETUP_SQPOLL).
> > > >     *
> > > > + * @uring_async_cmd:
> > > > + *      Check whether the file_operations async_cmd is allowed to run.
> > > > + *
> > > >     */
> > > >    union security_list_options {
> > > >    	#define LSM_HOOK(RET, DEFAULT, NAME, ...) RET (*NAME)(__VA_ARGS__);
> > > > diff --git a/include/linux/security.h b/include/linux/security.h
> > > > index 6d72772182c8..4d7f72813d75 100644
> > > > --- a/include/linux/security.h
> > > > +++ b/include/linux/security.h
> > > > @@ -2041,6 +2041,7 @@ static inline int security_perf_event_write(struct perf_event *event)
> > > >    #ifdef CONFIG_SECURITY
> > > >    extern int security_uring_override_creds(const struct cred *new);
> > > >    extern int security_uring_sqpoll(void);
> > > > +extern int security_uring_async_cmd(struct io_uring_cmd *ioucmd);
> > > >    #else
> > > >    static inline int security_uring_override_creds(const struct cred *new)
> > > >    {
> > > > @@ -2050,6 +2051,10 @@ static inline int security_uring_sqpoll(void)
> > > >    {
> > > >    	return 0;
> > > >    }
> > > > +static inline int security_uring_async_cmd(struct io_uring_cmd *ioucmd)
> > > > +{
> > > > +	return 0;
> > > > +}
> > > >    #endif /* CONFIG_SECURITY */
> > > >    #endif /* CONFIG_IO_URING */
> > > > diff --git a/security/security.c b/security/security.c
> > > > index 22261d79f333..ef96be2f953a 100644
> > > > --- a/security/security.c
> > > > +++ b/security/security.c
> > > > @@ -2640,4 +2640,8 @@ int security_uring_sqpoll(void)
> > > >    {
> > > >    	return call_int_hook(uring_sqpoll, 0);
> > > >    }
> > > > +int security_uring_async_cmd(struct io_uring_cmd *ioucmd)
> > > > +{
> > > > +	return call_int_hook(uring_async_cmd, 0, ioucmd);
> > > I don't have a good understanding of what information is in ioucmd.
> > > I am afraid that there may not be enough for a security module to
> > > make appropriate decisions in all cases. I am especially concerned
> > > about the modules that use path hooks, but based on the issues we've
> > > always had with ioctl and the like I fear for all cases.
> > As Paul pointed out, this particular LSM hook would not be needed if we can
> > somehow ensure users of the cmd path use their respective LSMs there. It
> > is not easy to force users to have the LSM hook to be used, one idea
> > might be to have a registration mechanism which allows users to also
> > specify the LSM hook, but these can vary in arguments, so perhaps then
> > what is needed is the LSM type in enum form, and internally we have a
> > mapping of these. That way we slowly itemize which cmds we *do* allow
> > for, thus vetting at the same time a respective LSM hook. Thoughts?
> 
> tl;dr - Yuck.
> 
> I don't see how your registration mechanism would be easier than
> getting "users of the cmd path" to use the LSM mechanism the way
> everyone else does. What it would do is pass responsibility for
> dealing with LSM to the io_uring core team.

Agreed, I was just trying to be proactive to help with the LSM stuff.
But indeed, that path would be complicated and I agree probably not
the most practical one.

> Experience has shown
> that dealing with the security issues after the fact is much
> harder than doing it up front, even when developers wail about
> the burden. Sure, LSM is an unpleasant interface/mechanism, but
> so is locking, and no one gets away without addressing that.
> My $0.02.

So putting the onus on those file_operations which embrace async_cmd to
take into account LSMs seems to be the way to go then, which seems to
align with what Paul was suggesting.

  Luis