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

[Luis Chamberlain <mcgrof@kernel.org>]

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