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@xxxxxxxxxxx>
> > ---
> > 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:
signature.asc
Description: PGP signature
