On Wed, Jul 13, 2022 at 8:05 PM Luis Chamberlain <mcgrof@xxxxxxxxxx> wrote: > > io-uring cmd support was added through ee692a21e9bf ("fs,io_uring: > add infrastructure for uring-cmd"), this extended 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. > > This was discussed long ago without no clear pointer for something > conclusive, so this enables LSMs to at least reject this new file > operation. > > [0] https://lkml.kernel.org/r/8adf55db-7bab-f59d-d612-ed906b948d19@xxxxxxxxxxxxxxxx [NOTE: I now see that the IORING_OP_URING_CMD has made it into the v5.19-rcX releases, I'm going to be honest and say that I'm disappointed you didn't post the related LSM additions until v5.19-rc6, especially given our earlier discussions.] While the earlier discussion may not have offered a detailed approach on how to solve this, I think it was rather conclusive in that the approach used then (and reproduced here) did not provide enough context to the LSMs to be able to make a decision. There were similar concerns when it came to auditing the command passthrough. It appears that most of my concerns in the original thread still apply to this patch. Given the LSM hook in this patch, it is very difficult (impossible?) to determine the requested operation as these command opcodes are device/subsystem specific. The unfortunate result is that the LSMs are likely going to either allow all, or none, of the commands for a given device/subsystem, and I think we can all agree that is not a good idea. That is the critical bit of feedback on this patch, but there is more feedback inline below. > Signed-off-by: Luis Chamberlain <mcgrof@xxxxxxxxxx> > --- > include/linux/lsm_hook_defs.h | 1 + > include/linux/lsm_hooks.h | 3 +++ > include/linux/security.h | 5 +++++ > io_uring/uring_cmd.c | 5 +++++ > security/security.c | 4 ++++ > 5 files changed, 18 insertions(+) ... > diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c > index 0a421ed51e7e..5e666aa7edb8 100644 > --- a/io_uring/uring_cmd.c > +++ b/io_uring/uring_cmd.c > @@ -3,6 +3,7 @@ > #include <linux/errno.h> > #include <linux/file.h> > #include <linux/io_uring.h> > +#include <linux/security.h> > > #include <uapi/linux/io_uring.h> > > @@ -82,6 +83,10 @@ int io_uring_cmd(struct io_kiocb *req, unsigned int issue_flags) > struct file *file = req->file; > int ret; > > + ret = security_uring_cmd(ioucmd); > + if (ret) > + return ret; > + > if (!req->file->f_op->uring_cmd) > return -EOPNOTSUPP; > In order to be consistent with most of the other LSM hooks, the 'req->file->f_op->uring_cmd' check should come before the LSM hook call. The general approach used in most places is to first validate the request and do any DAC based access checks before calling into the LSM. -- paul-moore.com