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.