diff mbox series

[RFC,v2,1/1] Use a fs callback to set security specific data

Message ID 20221122103144.960752-2-j.granados@samsung.com (mailing list archive)
State New
Delegated to: Paul Moore
Headers show
Series RFC on how to include LSM hooks for io_uring commands | expand

Commit Message

Joel Granados Nov. 22, 2022, 10:31 a.m. UTC
Signed-off-by: Joel Granados <j.granados@samsung.com>
---
 drivers/nvme/host/core.c      | 10 ++++++++++
 include/linux/fs.h            |  2 ++
 include/linux/lsm_hook_defs.h |  3 ++-
 include/linux/security.h      | 16 ++++++++++++++--
 io_uring/uring_cmd.c          |  3 ++-
 security/security.c           |  5 +++--
 security/selinux/hooks.c      | 16 +++++++++++++++-
 7 files changed, 48 insertions(+), 7 deletions(-)

Comments

Casey Schaufler Nov. 22, 2022, 3:18 p.m. UTC | #1
On 11/22/2022 2:31 AM, Joel Granados wrote:
> Signed-off-by: Joel Granados <j.granados@samsung.com>
> ---
>  drivers/nvme/host/core.c      | 10 ++++++++++
>  include/linux/fs.h            |  2 ++
>  include/linux/lsm_hook_defs.h |  3 ++-
>  include/linux/security.h      | 16 ++++++++++++++--
>  io_uring/uring_cmd.c          |  3 ++-
>  security/security.c           |  5 +++--
>  security/selinux/hooks.c      | 16 +++++++++++++++-
>  7 files changed, 48 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index f94b05c585cb..275826fe3c9e 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -4,6 +4,7 @@
>   * Copyright (c) 2011-2014, Intel Corporation.
>   */
>  
> +#include "linux/security.h"
>  #include <linux/blkdev.h>
>  #include <linux/blk-mq.h>
>  #include <linux/blk-integrity.h>
> @@ -3308,6 +3309,13 @@ static int nvme_dev_release(struct inode *inode, struct file *file)
>  	return 0;
>  }
>  
> +int nvme_uring_cmd_sec(struct io_uring_cmd *ioucmd,  struct security_uring_cmd *sec)
> +{
> +	sec->flags = 0;
> +	sec->flags = SECURITY_URING_CMD_TYPE_IOCTL;
> +	return 0;
> +}
> +
>  static const struct file_operations nvme_dev_fops = {
>  	.owner		= THIS_MODULE,
>  	.open		= nvme_dev_open,
> @@ -3315,6 +3323,7 @@ static const struct file_operations nvme_dev_fops = {
>  	.unlocked_ioctl	= nvme_dev_ioctl,
>  	.compat_ioctl	= compat_ptr_ioctl,
>  	.uring_cmd	= nvme_dev_uring_cmd,
> +	.uring_cmd_sec	= nvme_uring_cmd_sec,
>  };
>  
>  static ssize_t nvme_sysfs_reset(struct device *dev,
> @@ -3982,6 +3991,7 @@ static const struct file_operations nvme_ns_chr_fops = {
>  	.compat_ioctl	= compat_ptr_ioctl,
>  	.uring_cmd	= nvme_ns_chr_uring_cmd,
>  	.uring_cmd_iopoll = nvme_ns_chr_uring_cmd_iopoll,
> +	.uring_cmd_sec	= nvme_uring_cmd_sec,
>  };
>  
>  static int nvme_add_ns_cdev(struct nvme_ns *ns)
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index e654435f1651..af743a2dd562 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2091,6 +2091,7 @@ struct dir_context {
>  
>  struct iov_iter;
>  struct io_uring_cmd;
> +struct security_uring_cmd;
>  
>  struct file_operations {
>  	struct module *owner;
> @@ -2136,6 +2137,7 @@ struct file_operations {
>  	int (*uring_cmd)(struct io_uring_cmd *ioucmd, unsigned int issue_flags);
>  	int (*uring_cmd_iopoll)(struct io_uring_cmd *, struct io_comp_batch *,
>  				unsigned int poll_flags);
> +	int (*uring_cmd_sec)(struct io_uring_cmd *ioucmd, struct security_uring_cmd*);
>  } __randomize_layout;
>  
>  struct inode_operations {
> diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
> index ec119da1d89b..6cef29bce373 100644
> --- a/include/linux/lsm_hook_defs.h
> +++ b/include/linux/lsm_hook_defs.h
> @@ -408,5 +408,6 @@ 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_cmd, struct io_uring_cmd *ioucmd)
> +LSM_HOOK(int, 0, uring_cmd, struct io_uring_cmd *ioucmd,
> +	int (*uring_cmd_sec)(struct io_uring_cmd *ioucmd, struct security_uring_cmd*))

I'm slow, and I'm sure the question has been covered elsewhere,
but I have real trouble understanding why you're sending a function
to fetch the security data rather than the data itself. Callbacks
are not usual for LSM interfaces. If multiple security modules have
uring_cmd hooks (e.g. SELinux and landlock) the callback has to be
called multiple times.

>  #endif /* CONFIG_IO_URING */
> diff --git a/include/linux/security.h b/include/linux/security.h
> index ca1b7109c0db..146b1bbdc2e0 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -2065,10 +2065,20 @@ static inline int security_perf_event_write(struct perf_event *event)
>  #endif /* CONFIG_PERF_EVENTS */
>  
>  #ifdef CONFIG_IO_URING
> +enum security_uring_cmd_type
> +{
> +	SECURITY_URING_CMD_TYPE_IOCTL,
> +};
> +
> +struct security_uring_cmd {
> +	u64 flags;
> +};
>  #ifdef CONFIG_SECURITY
>  extern int security_uring_override_creds(const struct cred *new);
>  extern int security_uring_sqpoll(void);
> -extern int security_uring_cmd(struct io_uring_cmd *ioucmd);
> +extern int security_uring_cmd(struct io_uring_cmd *ioucmd,
> +		int (*uring_cmd_sec)(struct io_uring_cmd *,
> +			struct security_uring_cmd*));
>  #else
>  static inline int security_uring_override_creds(const struct cred *new)
>  {
> @@ -2078,7 +2088,9 @@ static inline int security_uring_sqpoll(void)
>  {
>  	return 0;
>  }
> -static inline int security_uring_cmd(struct io_uring_cmd *ioucmd)
> +static inline int security_uring_cmd(struct io_uring_cmd *ioucmd,
> +		int (*uring_cmd_sec)(struct io_uring_cmd *,
> +			struct security_uring_cmd*))
>  {
>  	return 0;
>  }
> diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c
> index e50de0b6b9f8..2f650b346756 100644
> --- a/io_uring/uring_cmd.c
> +++ b/io_uring/uring_cmd.c
> @@ -108,10 +108,11 @@ int io_uring_cmd(struct io_kiocb *req, unsigned int issue_flags)
>  	struct file *file = req->file;
>  	int ret;
>  
> +	//req->file->f_op->owner->ei_funcs
>  	if (!req->file->f_op->uring_cmd)
>  		return -EOPNOTSUPP;
>  
> -	ret = security_uring_cmd(ioucmd);
> +	ret = security_uring_cmd(ioucmd, req->file->f_op->uring_cmd_sec);
>  	if (ret)
>  		return ret;
>  
> diff --git a/security/security.c b/security/security.c
> index 79d82cb6e469..d3360a32f971 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -2667,8 +2667,9 @@ int security_uring_sqpoll(void)
>  {
>  	return call_int_hook(uring_sqpoll, 0);
>  }
> -int security_uring_cmd(struct io_uring_cmd *ioucmd)
> +int security_uring_cmd(struct io_uring_cmd *ioucmd,
> +	int (*uring_cmd_sec)(struct io_uring_cmd *ioucmd, struct security_uring_cmd*))
>  {
> -	return call_int_hook(uring_cmd, 0, ioucmd);
> +	return call_int_hook(uring_cmd, 0, ioucmd, uring_cmd_sec);
>  }
>  #endif /* CONFIG_IO_URING */
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index f553c370397e..9fe3a230c671 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -21,6 +21,8 @@
>   *  Copyright (C) 2016 Mellanox Technologies
>   */
>  
> +#include "linux/nvme_ioctl.h"
> +#include "linux/security.h"
>  #include <linux/init.h>
>  #include <linux/kd.h>
>  #include <linux/kernel.h>
> @@ -6999,18 +7001,30 @@ static int selinux_uring_sqpoll(void)
>   * IORING_OP_URING_CMD against the device/file specified in @ioucmd.
>   *
>   */
> -static int selinux_uring_cmd(struct io_uring_cmd *ioucmd)
> +static int selinux_uring_cmd(struct io_uring_cmd *ioucmd,
> +	int (*uring_cmd_sec)(struct io_uring_cmd *ioucmd, struct security_uring_cmd*))
>  {
>  	struct file *file = ioucmd->file;
>  	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();
> +	struct security_uring_cmd sec_uring = {0};
> +	int ret;
>  
>  	ad.type = LSM_AUDIT_DATA_FILE;
>  	ad.u.file = file;
>  
> +	ret = uring_cmd_sec(ioucmd, &sec_uring);
> +	if (ret)
> +		return ret;
> +
> +	if (sec_uring.flags & SECURITY_URING_CMD_TYPE_IOCTL)
> +		return ioctl_has_perm(cred, file, FILE__IOCTL, (u16) ioucmd->cmd_op);
> +
>  	return avc_has_perm(&selinux_state, current_sid(), isec->sid,
>  			    SECCLASS_IO_URING, IO_URING__CMD, &ad);
> +
>  }
>  #endif /* CONFIG_IO_URING */
>
Paul Moore Nov. 23, 2022, 9:02 p.m. UTC | #2
On Tue, Nov 22, 2022 at 5:35 AM Joel Granados <j.granados@samsung.com> wrote:
>
> Signed-off-by: Joel Granados <j.granados@samsung.com>
> ---
>  drivers/nvme/host/core.c      | 10 ++++++++++
>  include/linux/fs.h            |  2 ++
>  include/linux/lsm_hook_defs.h |  3 ++-
>  include/linux/security.h      | 16 ++++++++++++++--
>  io_uring/uring_cmd.c          |  3 ++-
>  security/security.c           |  5 +++--
>  security/selinux/hooks.c      | 16 +++++++++++++++-
>  7 files changed, 48 insertions(+), 7 deletions(-)

...

> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index f553c370397e..9fe3a230c671 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -21,6 +21,8 @@
>   *  Copyright (C) 2016 Mellanox Technologies
>   */
>
> +#include "linux/nvme_ioctl.h"
> +#include "linux/security.h"
>  #include <linux/init.h>
>  #include <linux/kd.h>
>  #include <linux/kernel.h>
> @@ -6999,18 +7001,30 @@ static int selinux_uring_sqpoll(void)
>   * IORING_OP_URING_CMD against the device/file specified in @ioucmd.
>   *
>   */
> -static int selinux_uring_cmd(struct io_uring_cmd *ioucmd)
> +static int selinux_uring_cmd(struct io_uring_cmd *ioucmd,
> +       int (*uring_cmd_sec)(struct io_uring_cmd *ioucmd, struct security_uring_cmd*))
>  {

As we discussed in the previous thread, and Casey mentioned already,
passing a function pointer for the LSM to call isn't a great practice.
When it was proposed we hadn't really thought of any alternatives, but
if we can't find a good scalar value to compare somewhere, I think
inspecting the file_operations::owner::name string to determine the
target is preferable to the function pointer approach described here.

Although I really would like to see us find, or create, some sort of
scalar token ID we could use instead.  I fear that doing a lot of
strcmp()'s to identify the uring command target is going to be a
problem (one strcmp() for each possible target, multiplied by the
number of LSMs which implement a io_uring command hook).

>         struct file *file = ioucmd->file;
>         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();
> +       struct security_uring_cmd sec_uring = {0};
> +       int ret;
>
>         ad.type = LSM_AUDIT_DATA_FILE;
>         ad.u.file = file;
>
> +       ret = uring_cmd_sec(ioucmd, &sec_uring);
> +       if (ret)
> +               return ret;
> +
> +       if (sec_uring.flags & SECURITY_URING_CMD_TYPE_IOCTL)
> +               return ioctl_has_perm(cred, file, FILE__IOCTL, (u16) ioucmd->cmd_op);

As mentioned previously, we'll need a SELinux policy capability here
to preserve the SECCLASS_IO_URING/IO_URING__CMD access check for
existing users/policies.  I expect the logic would look something like
this (of course the details are dependent on how we identify the
target module/device/etc.):

  if (polcap_foo && uring_tgt) {
    switch (uring_tgt) {
    case NVME:
      return avc_has_perm(...);
    default:
      WARN();
      return avc_has_perm(SECCLASS_IO_URING, IO_URING__CMD);
    }
  } else
    return avc_has_perm(SECCLASS_IO_URING, IO_URING__CMD);

>         return avc_has_perm(&selinux_state, current_sid(), isec->sid,
>                             SECCLASS_IO_URING, IO_URING__CMD, &ad);
> +
>  }
>  #endif /* CONFIG_IO_URING */
Joel Granados Nov. 28, 2022, 8:19 a.m. UTC | #3
On Tue, Nov 22, 2022 at 07:18:24AM -0800, Casey Schaufler wrote:
> On 11/22/2022 2:31 AM, Joel Granados wrote:
> > Signed-off-by: Joel Granados <j.granados@samsung.com>
> > ---
> >  drivers/nvme/host/core.c      | 10 ++++++++++
> >  include/linux/fs.h            |  2 ++
> >  include/linux/lsm_hook_defs.h |  3 ++-
> >  include/linux/security.h      | 16 ++++++++++++++--
> >  io_uring/uring_cmd.c          |  3 ++-
> >  security/security.c           |  5 +++--
> >  security/selinux/hooks.c      | 16 +++++++++++++++-
> >  7 files changed, 48 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> > index f94b05c585cb..275826fe3c9e 100644
> > --- a/drivers/nvme/host/core.c
> > +++ b/drivers/nvme/host/core.c
> > @@ -4,6 +4,7 @@
> >   * Copyright (c) 2011-2014, Intel Corporation.
> >   */
> >  
> > +#include "linux/security.h"
> >  #include <linux/blkdev.h>
> >  #include <linux/blk-mq.h>
> >  #include <linux/blk-integrity.h>
> > @@ -3308,6 +3309,13 @@ static int nvme_dev_release(struct inode *inode, struct file *file)
> >  	return 0;
> >  }
> >  
> > +int nvme_uring_cmd_sec(struct io_uring_cmd *ioucmd,  struct security_uring_cmd *sec)
> > +{
> > +	sec->flags = 0;
> > +	sec->flags = SECURITY_URING_CMD_TYPE_IOCTL;
> > +	return 0;
> > +}
> > +
> >  static const struct file_operations nvme_dev_fops = {
> >  	.owner		= THIS_MODULE,
> >  	.open		= nvme_dev_open,
> > @@ -3315,6 +3323,7 @@ static const struct file_operations nvme_dev_fops = {
> >  	.unlocked_ioctl	= nvme_dev_ioctl,
> >  	.compat_ioctl	= compat_ptr_ioctl,
> >  	.uring_cmd	= nvme_dev_uring_cmd,
> > +	.uring_cmd_sec	= nvme_uring_cmd_sec,
> >  };
> >  
> >  static ssize_t nvme_sysfs_reset(struct device *dev,
> > @@ -3982,6 +3991,7 @@ static const struct file_operations nvme_ns_chr_fops = {
> >  	.compat_ioctl	= compat_ptr_ioctl,
> >  	.uring_cmd	= nvme_ns_chr_uring_cmd,
> >  	.uring_cmd_iopoll = nvme_ns_chr_uring_cmd_iopoll,
> > +	.uring_cmd_sec	= nvme_uring_cmd_sec,
> >  };
> >  
> >  static int nvme_add_ns_cdev(struct nvme_ns *ns)
> > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > index e654435f1651..af743a2dd562 100644
> > --- a/include/linux/fs.h
> > +++ b/include/linux/fs.h
> > @@ -2091,6 +2091,7 @@ struct dir_context {
> >  
> >  struct iov_iter;
> >  struct io_uring_cmd;
> > +struct security_uring_cmd;
> >  
> >  struct file_operations {
> >  	struct module *owner;
> > @@ -2136,6 +2137,7 @@ struct file_operations {
> >  	int (*uring_cmd)(struct io_uring_cmd *ioucmd, unsigned int issue_flags);
> >  	int (*uring_cmd_iopoll)(struct io_uring_cmd *, struct io_comp_batch *,
> >  				unsigned int poll_flags);
> > +	int (*uring_cmd_sec)(struct io_uring_cmd *ioucmd, struct security_uring_cmd*);
> >  } __randomize_layout;
> >  
> >  struct inode_operations {
> > diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
> > index ec119da1d89b..6cef29bce373 100644
> > --- a/include/linux/lsm_hook_defs.h
> > +++ b/include/linux/lsm_hook_defs.h
> > @@ -408,5 +408,6 @@ 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_cmd, struct io_uring_cmd *ioucmd)
> > +LSM_HOOK(int, 0, uring_cmd, struct io_uring_cmd *ioucmd,
> > +	int (*uring_cmd_sec)(struct io_uring_cmd *ioucmd, struct security_uring_cmd*))
> 
> I'm slow, and I'm sure the question has been covered elsewhere,
> but I have real trouble understanding why you're sending a function
> to fetch the security data rather than the data itself. Callbacks
> are not usual for LSM interfaces. If multiple security modules have
> uring_cmd hooks (e.g. SELinux and landlock) the callback has to be
> called multiple times.

No particular reason to have a callback, its just how it came out
initially. I think changing this to a LSM struct is not a deal breaker
for me. Especially if the callback might be called several times
unnecessarily.
TBH, I was expecting more pushback from including it in the file
opeartions struct directly. Do you see any issue with including LSM
specific pointers in the file opeartions?
> 
> >  #endif /* CONFIG_IO_URING */
> > diff --git a/include/linux/security.h b/include/linux/security.h
> > index ca1b7109c0db..146b1bbdc2e0 100644
> > --- a/include/linux/security.h
> > +++ b/include/linux/security.h
> > @@ -2065,10 +2065,20 @@ static inline int security_perf_event_write(struct perf_event *event)
> >  #endif /* CONFIG_PERF_EVENTS */
> >  
> >  #ifdef CONFIG_IO_URING
> > +enum security_uring_cmd_type
> > +{
> > +	SECURITY_URING_CMD_TYPE_IOCTL,
> > +};
> > +
> > +struct security_uring_cmd {
> > +	u64 flags;
> > +};
> >  #ifdef CONFIG_SECURITY
> >  extern int security_uring_override_creds(const struct cred *new);
> >  extern int security_uring_sqpoll(void);
> > -extern int security_uring_cmd(struct io_uring_cmd *ioucmd);
> > +extern int security_uring_cmd(struct io_uring_cmd *ioucmd,
> > +		int (*uring_cmd_sec)(struct io_uring_cmd *,
> > +			struct security_uring_cmd*));
> >  #else
> >  static inline int security_uring_override_creds(const struct cred *new)
> >  {
> > @@ -2078,7 +2088,9 @@ static inline int security_uring_sqpoll(void)
> >  {
> >  	return 0;
> >  }
> > -static inline int security_uring_cmd(struct io_uring_cmd *ioucmd)
> > +static inline int security_uring_cmd(struct io_uring_cmd *ioucmd,
> > +		int (*uring_cmd_sec)(struct io_uring_cmd *,
> > +			struct security_uring_cmd*))
> >  {
> >  	return 0;
> >  }
> > diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c
> > index e50de0b6b9f8..2f650b346756 100644
> > --- a/io_uring/uring_cmd.c
> > +++ b/io_uring/uring_cmd.c
> > @@ -108,10 +108,11 @@ int io_uring_cmd(struct io_kiocb *req, unsigned int issue_flags)
> >  	struct file *file = req->file;
> >  	int ret;
> >  
> > +	//req->file->f_op->owner->ei_funcs
> >  	if (!req->file->f_op->uring_cmd)
> >  		return -EOPNOTSUPP;
> >  
> > -	ret = security_uring_cmd(ioucmd);
> > +	ret = security_uring_cmd(ioucmd, req->file->f_op->uring_cmd_sec);
> >  	if (ret)
> >  		return ret;
> >  
> > diff --git a/security/security.c b/security/security.c
> > index 79d82cb6e469..d3360a32f971 100644
> > --- a/security/security.c
> > +++ b/security/security.c
> > @@ -2667,8 +2667,9 @@ int security_uring_sqpoll(void)
> >  {
> >  	return call_int_hook(uring_sqpoll, 0);
> >  }
> > -int security_uring_cmd(struct io_uring_cmd *ioucmd)
> > +int security_uring_cmd(struct io_uring_cmd *ioucmd,
> > +	int (*uring_cmd_sec)(struct io_uring_cmd *ioucmd, struct security_uring_cmd*))
> >  {
> > -	return call_int_hook(uring_cmd, 0, ioucmd);
> > +	return call_int_hook(uring_cmd, 0, ioucmd, uring_cmd_sec);
> >  }
> >  #endif /* CONFIG_IO_URING */
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index f553c370397e..9fe3a230c671 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -21,6 +21,8 @@
> >   *  Copyright (C) 2016 Mellanox Technologies
> >   */
> >  
> > +#include "linux/nvme_ioctl.h"
> > +#include "linux/security.h"
> >  #include <linux/init.h>
> >  #include <linux/kd.h>
> >  #include <linux/kernel.h>
> > @@ -6999,18 +7001,30 @@ static int selinux_uring_sqpoll(void)
> >   * IORING_OP_URING_CMD against the device/file specified in @ioucmd.
> >   *
> >   */
> > -static int selinux_uring_cmd(struct io_uring_cmd *ioucmd)
> > +static int selinux_uring_cmd(struct io_uring_cmd *ioucmd,
> > +	int (*uring_cmd_sec)(struct io_uring_cmd *ioucmd, struct security_uring_cmd*))
> >  {
> >  	struct file *file = ioucmd->file;
> >  	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();
> > +	struct security_uring_cmd sec_uring = {0};
> > +	int ret;
> >  
> >  	ad.type = LSM_AUDIT_DATA_FILE;
> >  	ad.u.file = file;
> >  
> > +	ret = uring_cmd_sec(ioucmd, &sec_uring);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (sec_uring.flags & SECURITY_URING_CMD_TYPE_IOCTL)
> > +		return ioctl_has_perm(cred, file, FILE__IOCTL, (u16) ioucmd->cmd_op);
> > +
> >  	return avc_has_perm(&selinux_state, current_sid(), isec->sid,
> >  			    SECCLASS_IO_URING, IO_URING__CMD, &ad);
> > +
> >  }
> >  #endif /* CONFIG_IO_URING */
> >
Joel Granados Nov. 28, 2022, 9:06 a.m. UTC | #4
On Mon, Nov 28, 2022 at 09:19:46AM +0100, Joel Granados wrote:
> On Tue, Nov 22, 2022 at 07:18:24AM -0800, Casey Schaufler wrote:
> > On 11/22/2022 2:31 AM, Joel Granados wrote:
> > > Signed-off-by: Joel Granados <j.granados@samsung.com>
> > > ---
> > >  drivers/nvme/host/core.c      | 10 ++++++++++
> > >  include/linux/fs.h            |  2 ++
> > >  include/linux/lsm_hook_defs.h |  3 ++-
> > >  include/linux/security.h      | 16 ++++++++++++++--
> > >  io_uring/uring_cmd.c          |  3 ++-
> > >  security/security.c           |  5 +++--
> > >  security/selinux/hooks.c      | 16 +++++++++++++++-
> > >  7 files changed, 48 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> > > index f94b05c585cb..275826fe3c9e 100644
> > > --- a/drivers/nvme/host/core.c
> > > +++ b/drivers/nvme/host/core.c
> > > @@ -4,6 +4,7 @@
> > >   * Copyright (c) 2011-2014, Intel Corporation.
> > >   */
> > >  
> > > +#include "linux/security.h"
> > >  #include <linux/blkdev.h>
> > >  #include <linux/blk-mq.h>
> > >  #include <linux/blk-integrity.h>
> > > @@ -3308,6 +3309,13 @@ static int nvme_dev_release(struct inode *inode, struct file *file)
> > >  	return 0;
> > >  }
> > >  
> > > +int nvme_uring_cmd_sec(struct io_uring_cmd *ioucmd,  struct security_uring_cmd *sec)
> > > +{
> > > +	sec->flags = 0;
> > > +	sec->flags = SECURITY_URING_CMD_TYPE_IOCTL;
> > > +	return 0;
> > > +}
> > > +
> > >  static const struct file_operations nvme_dev_fops = {
> > >  	.owner		= THIS_MODULE,
> > >  	.open		= nvme_dev_open,
> > > @@ -3315,6 +3323,7 @@ static const struct file_operations nvme_dev_fops = {
> > >  	.unlocked_ioctl	= nvme_dev_ioctl,
> > >  	.compat_ioctl	= compat_ptr_ioctl,
> > >  	.uring_cmd	= nvme_dev_uring_cmd,
> > > +	.uring_cmd_sec	= nvme_uring_cmd_sec,
> > >  };
> > >  
> > >  static ssize_t nvme_sysfs_reset(struct device *dev,
> > > @@ -3982,6 +3991,7 @@ static const struct file_operations nvme_ns_chr_fops = {
> > >  	.compat_ioctl	= compat_ptr_ioctl,
> > >  	.uring_cmd	= nvme_ns_chr_uring_cmd,
> > >  	.uring_cmd_iopoll = nvme_ns_chr_uring_cmd_iopoll,
> > > +	.uring_cmd_sec	= nvme_uring_cmd_sec,
> > >  };
> > >  
> > >  static int nvme_add_ns_cdev(struct nvme_ns *ns)
> > > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > > index e654435f1651..af743a2dd562 100644
> > > --- a/include/linux/fs.h
> > > +++ b/include/linux/fs.h
> > > @@ -2091,6 +2091,7 @@ struct dir_context {
> > >  
> > >  struct iov_iter;
> > >  struct io_uring_cmd;
> > > +struct security_uring_cmd;
> > >  
> > >  struct file_operations {
> > >  	struct module *owner;
> > > @@ -2136,6 +2137,7 @@ struct file_operations {
> > >  	int (*uring_cmd)(struct io_uring_cmd *ioucmd, unsigned int issue_flags);
> > >  	int (*uring_cmd_iopoll)(struct io_uring_cmd *, struct io_comp_batch *,
> > >  				unsigned int poll_flags);
> > > +	int (*uring_cmd_sec)(struct io_uring_cmd *ioucmd, struct security_uring_cmd*);
> > >  } __randomize_layout;
> > >  
> > >  struct inode_operations {
> > > diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
> > > index ec119da1d89b..6cef29bce373 100644
> > > --- a/include/linux/lsm_hook_defs.h
> > > +++ b/include/linux/lsm_hook_defs.h
> > > @@ -408,5 +408,6 @@ 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_cmd, struct io_uring_cmd *ioucmd)
> > > +LSM_HOOK(int, 0, uring_cmd, struct io_uring_cmd *ioucmd,
> > > +	int (*uring_cmd_sec)(struct io_uring_cmd *ioucmd, struct security_uring_cmd*))
> > 
> > I'm slow, and I'm sure the question has been covered elsewhere,
> > but I have real trouble understanding why you're sending a function
> > to fetch the security data rather than the data itself. Callbacks
> > are not usual for LSM interfaces. If multiple security modules have
> > uring_cmd hooks (e.g. SELinux and landlock) the callback has to be
> > called multiple times.
> 
> No particular reason to have a callback, its just how it came out
> initially. I think changing this to a LSM struct is not a deal breaker
> for me. Especially if the callback might be called several times
> unnecessarily.
> TBH, I was expecting more pushback from including it in the file
> opeartions struct directly. Do you see any issue with including LSM
> specific pointers in the file opeartions?

TBH, if we see that a callback is the way to go we can always have a
callback execute it in uring_cmd.c and pass a struct to the LSM infra.
This will avoid the double call the you are referring to.

One advantage of having a callback is that changes withing the uring
user/driver (like a access list changing the way the driver behaves with
certain users) can be updated on every call to uring_cmd_sec. The uring
user/driver can also decide to just return the same struct always.

> > 
> > >  #endif /* CONFIG_IO_URING */
> > > diff --git a/include/linux/security.h b/include/linux/security.h
> > > index ca1b7109c0db..146b1bbdc2e0 100644
> > > --- a/include/linux/security.h
> > > +++ b/include/linux/security.h
> > > @@ -2065,10 +2065,20 @@ static inline int security_perf_event_write(struct perf_event *event)
> > >  #endif /* CONFIG_PERF_EVENTS */
> > >  
> > >  #ifdef CONFIG_IO_URING
> > > +enum security_uring_cmd_type
> > > +{
> > > +	SECURITY_URING_CMD_TYPE_IOCTL,
> > > +};
> > > +
> > > +struct security_uring_cmd {
> > > +	u64 flags;
> > > +};
> > >  #ifdef CONFIG_SECURITY
> > >  extern int security_uring_override_creds(const struct cred *new);
> > >  extern int security_uring_sqpoll(void);
> > > -extern int security_uring_cmd(struct io_uring_cmd *ioucmd);
> > > +extern int security_uring_cmd(struct io_uring_cmd *ioucmd,
> > > +		int (*uring_cmd_sec)(struct io_uring_cmd *,
> > > +			struct security_uring_cmd*));
> > >  #else
> > >  static inline int security_uring_override_creds(const struct cred *new)
> > >  {
> > > @@ -2078,7 +2088,9 @@ static inline int security_uring_sqpoll(void)
> > >  {
> > >  	return 0;
> > >  }
> > > -static inline int security_uring_cmd(struct io_uring_cmd *ioucmd)
> > > +static inline int security_uring_cmd(struct io_uring_cmd *ioucmd,
> > > +		int (*uring_cmd_sec)(struct io_uring_cmd *,
> > > +			struct security_uring_cmd*))
> > >  {
> > >  	return 0;
> > >  }
> > > diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c
> > > index e50de0b6b9f8..2f650b346756 100644
> > > --- a/io_uring/uring_cmd.c
> > > +++ b/io_uring/uring_cmd.c
> > > @@ -108,10 +108,11 @@ int io_uring_cmd(struct io_kiocb *req, unsigned int issue_flags)
> > >  	struct file *file = req->file;
> > >  	int ret;
> > >  
> > > +	//req->file->f_op->owner->ei_funcs
> > >  	if (!req->file->f_op->uring_cmd)
> > >  		return -EOPNOTSUPP;
> > >  
> > > -	ret = security_uring_cmd(ioucmd);
> > > +	ret = security_uring_cmd(ioucmd, req->file->f_op->uring_cmd_sec);
> > >  	if (ret)
> > >  		return ret;
> > >  
> > > diff --git a/security/security.c b/security/security.c
> > > index 79d82cb6e469..d3360a32f971 100644
> > > --- a/security/security.c
> > > +++ b/security/security.c
> > > @@ -2667,8 +2667,9 @@ int security_uring_sqpoll(void)
> > >  {
> > >  	return call_int_hook(uring_sqpoll, 0);
> > >  }
> > > -int security_uring_cmd(struct io_uring_cmd *ioucmd)
> > > +int security_uring_cmd(struct io_uring_cmd *ioucmd,
> > > +	int (*uring_cmd_sec)(struct io_uring_cmd *ioucmd, struct security_uring_cmd*))
> > >  {
> > > -	return call_int_hook(uring_cmd, 0, ioucmd);
> > > +	return call_int_hook(uring_cmd, 0, ioucmd, uring_cmd_sec);
> > >  }
> > >  #endif /* CONFIG_IO_URING */
> > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > > index f553c370397e..9fe3a230c671 100644
> > > --- a/security/selinux/hooks.c
> > > +++ b/security/selinux/hooks.c
> > > @@ -21,6 +21,8 @@
> > >   *  Copyright (C) 2016 Mellanox Technologies
> > >   */
> > >  
> > > +#include "linux/nvme_ioctl.h"
> > > +#include "linux/security.h"
> > >  #include <linux/init.h>
> > >  #include <linux/kd.h>
> > >  #include <linux/kernel.h>
> > > @@ -6999,18 +7001,30 @@ static int selinux_uring_sqpoll(void)
> > >   * IORING_OP_URING_CMD against the device/file specified in @ioucmd.
> > >   *
> > >   */
> > > -static int selinux_uring_cmd(struct io_uring_cmd *ioucmd)
> > > +static int selinux_uring_cmd(struct io_uring_cmd *ioucmd,
> > > +	int (*uring_cmd_sec)(struct io_uring_cmd *ioucmd, struct security_uring_cmd*))
> > >  {
> > >  	struct file *file = ioucmd->file;
> > >  	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();
> > > +	struct security_uring_cmd sec_uring = {0};
> > > +	int ret;
> > >  
> > >  	ad.type = LSM_AUDIT_DATA_FILE;
> > >  	ad.u.file = file;
> > >  
> > > +	ret = uring_cmd_sec(ioucmd, &sec_uring);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	if (sec_uring.flags & SECURITY_URING_CMD_TYPE_IOCTL)
> > > +		return ioctl_has_perm(cred, file, FILE__IOCTL, (u16) ioucmd->cmd_op);
> > > +
> > >  	return avc_has_perm(&selinux_state, current_sid(), isec->sid,
> > >  			    SECCLASS_IO_URING, IO_URING__CMD, &ad);
> > > +
> > >  }
> > >  #endif /* CONFIG_IO_URING */
> > >
Joel Granados Nov. 28, 2022, 9:27 a.m. UTC | #5
On Wed, Nov 23, 2022 at 04:02:02PM -0500, Paul Moore wrote:
> On Tue, Nov 22, 2022 at 5:35 AM Joel Granados <j.granados@samsung.com> wrote:
> >
> > Signed-off-by: Joel Granados <j.granados@samsung.com>
> > ---
> >  drivers/nvme/host/core.c      | 10 ++++++++++
> >  include/linux/fs.h            |  2 ++
> >  include/linux/lsm_hook_defs.h |  3 ++-
> >  include/linux/security.h      | 16 ++++++++++++++--
> >  io_uring/uring_cmd.c          |  3 ++-
> >  security/security.c           |  5 +++--
> >  security/selinux/hooks.c      | 16 +++++++++++++++-
> >  7 files changed, 48 insertions(+), 7 deletions(-)
> 
> ...
> 
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index f553c370397e..9fe3a230c671 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -21,6 +21,8 @@
> >   *  Copyright (C) 2016 Mellanox Technologies
> >   */
> >
> > +#include "linux/nvme_ioctl.h"
> > +#include "linux/security.h"
> >  #include <linux/init.h>
> >  #include <linux/kd.h>
> >  #include <linux/kernel.h>
> > @@ -6999,18 +7001,30 @@ static int selinux_uring_sqpoll(void)
> >   * IORING_OP_URING_CMD against the device/file specified in @ioucmd.
> >   *
> >   */
> > -static int selinux_uring_cmd(struct io_uring_cmd *ioucmd)
> > +static int selinux_uring_cmd(struct io_uring_cmd *ioucmd,
> > +       int (*uring_cmd_sec)(struct io_uring_cmd *ioucmd, struct security_uring_cmd*))
> >  {
> 
> As we discussed in the previous thread, and Casey mentioned already,
> passing a function pointer for the LSM to call isn't a great practice.
> When it was proposed we hadn't really thought of any alternatives, but
> if we can't find a good scalar value to compare somewhere, I think
> inspecting the file_operations::owner::name string to determine the
> target is preferable to the function pointer approach described here.

We don't only need to determine the target; we need the target to
specify the current operation to the LSM infra so it can do its thing.

To me, if we just identify, we would need to have some logic in the
uring_cmd that goes through all the possible uring users to execute
their security specific functions. (Paul please correct me if I'm
misunderstanding you). Something like this

switch (uring_user_type(req->file->f_op->name)):
case nvme:
  nvme_specific_sec_call();
case ublk:
  ublk_specific_sec_call();
case user3:
  user3_specific_sec_call();
..... etc...

This is not scalable because there would need to be uring user code in
uring and that makes no sense as uring is agnostic to whatever is using
it.

> 
> Although I really would like to see us find, or create, some sort of
> scalar token ID we could use instead.  I fear that doing a lot of
> strcmp()'s to identify the uring command target is going to be a
> problem (one strcmp() for each possible target, multiplied by the
> number of LSMs which implement a io_uring command hook).
Agreed, depending on string compare does not scale.

> 
> >         struct file *file = ioucmd->file;
> >         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();
> > +       struct security_uring_cmd sec_uring = {0};
> > +       int ret;
> >
> >         ad.type = LSM_AUDIT_DATA_FILE;
> >         ad.u.file = file;
> >
> > +       ret = uring_cmd_sec(ioucmd, &sec_uring);
> > +       if (ret)
> > +               return ret;
> > +
> > +       if (sec_uring.flags & SECURITY_URING_CMD_TYPE_IOCTL)
> > +               return ioctl_has_perm(cred, file, FILE__IOCTL, (u16) ioucmd->cmd_op);
> 
> As mentioned previously, we'll need a SELinux policy capability here
> to preserve the SECCLASS_IO_URING/IO_URING__CMD access check for
> existing users/policies.  I expect the logic would look something like
> this (of course the details are dependent on how we identify the
> target module/device/etc.):
> 
>   if (polcap_foo && uring_tgt) {
>     switch (uring_tgt) {
>     case NVME:
>       return avc_has_perm(...);
>     default:
>       WARN();
>       return avc_has_perm(SECCLASS_IO_URING, IO_URING__CMD);
>     }
>   } else
>     return avc_has_perm(SECCLASS_IO_URING, IO_URING__CMD);
> 
This is selinux specific. right? I ask because I want to have it
strait in my head what is LSM generic and what is needed for a specific
implementation of an LSM.

> >         return avc_has_perm(&selinux_state, current_sid(), isec->sid,
> >                             SECCLASS_IO_URING, IO_URING__CMD, &ad);
> > +
> >  }
> >  #endif /* CONFIG_IO_URING */
> 
> -- 
> paul-moore.com
Christoph Hellwig Nov. 29, 2022, 2:24 p.m. UTC | #6
This seems to be missing any kind of changelog.  Also the
subject should say file_operations.  Most of the instances here are
not in a file system, and they most certainly aren't callbacks.

I think you've also missed a whole lot of maintainers.

> +#include "linux/security.h"

That's now how we include kernel-wide headers.

>  #include <linux/blkdev.h>
>  #include <linux/blk-mq.h>
>  #include <linux/blk-integrity.h>
> @@ -3308,6 +3309,13 @@ static int nvme_dev_release(struct inode *inode, struct file *file)
>  	return 0;
>  }
>  
> +int nvme_uring_cmd_sec(struct io_uring_cmd *ioucmd,  struct security_uring_cmd *sec)

Douple white space and overly long line.

> +{
> +	sec->flags = 0;
> +	sec->flags = SECURITY_URING_CMD_TYPE_IOCTL;

Double initialization of ->flags.  But how is this supposed to work
to start with?
Joel Granados Nov. 30, 2022, 9:29 p.m. UTC | #7
On Tue, Nov 29, 2022 at 06:24:00AM -0800, Christoph Hellwig wrote:
> This seems to be missing any kind of changelog.  Also the
> subject should say file_operations.  Most of the instances here are
> not in a file system, and they most certainly aren't callbacks.
> 
> I think you've also missed a whole lot of maintainers.
> 
> > +#include "linux/security.h"
> 
> That's now how we include kernel-wide headers.
> 
> >  #include <linux/blkdev.h>
> >  #include <linux/blk-mq.h>
> >  #include <linux/blk-integrity.h>
> > @@ -3308,6 +3309,13 @@ static int nvme_dev_release(struct inode *inode, struct file *file)
> >  	return 0;
> >  }
> >  
> > +int nvme_uring_cmd_sec(struct io_uring_cmd *ioucmd,  struct security_uring_cmd *sec)
> 
> Douple white space and overly long line.
> 
> > +{
> > +	sec->flags = 0;
> > +	sec->flags = SECURITY_URING_CMD_TYPE_IOCTL;
> 
> Double initialization of ->flags.  But how is this supposed to work
> to start with?

This RFC is meant to see how different solutions may play out. I'm not
trying to push anything through yet. Just testing the waters to see what
sticks and what people think about certain approaches. Should have
mentioned that in my cover letter.

My idea was to bring all relevant maintainers into the conversation once
I had a more clear idea on what needed to be done and how I would do it.

Since the patch is just a discussion piece, it is riddled with errors
like the ones you pointed out.

The idea with this second version is to add a security uring callback to
the already existing ones in the file_operations structure. This new
callback will fill in a security struct that will contain all the data
needed for the LSMs to do their thing. This callback can be protected by
an 'ifdef' for performance purposes.

There is a third proposal by Ming Lei that uses the io_uring_sqe struct
to embed io_uring type information. In my todo list I have a task to
implement this and present it as a third option.

best
Joel
diff mbox series

Patch

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index f94b05c585cb..275826fe3c9e 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -4,6 +4,7 @@ 
  * Copyright (c) 2011-2014, Intel Corporation.
  */
 
+#include "linux/security.h"
 #include <linux/blkdev.h>
 #include <linux/blk-mq.h>
 #include <linux/blk-integrity.h>
@@ -3308,6 +3309,13 @@  static int nvme_dev_release(struct inode *inode, struct file *file)
 	return 0;
 }
 
+int nvme_uring_cmd_sec(struct io_uring_cmd *ioucmd,  struct security_uring_cmd *sec)
+{
+	sec->flags = 0;
+	sec->flags = SECURITY_URING_CMD_TYPE_IOCTL;
+	return 0;
+}
+
 static const struct file_operations nvme_dev_fops = {
 	.owner		= THIS_MODULE,
 	.open		= nvme_dev_open,
@@ -3315,6 +3323,7 @@  static const struct file_operations nvme_dev_fops = {
 	.unlocked_ioctl	= nvme_dev_ioctl,
 	.compat_ioctl	= compat_ptr_ioctl,
 	.uring_cmd	= nvme_dev_uring_cmd,
+	.uring_cmd_sec	= nvme_uring_cmd_sec,
 };
 
 static ssize_t nvme_sysfs_reset(struct device *dev,
@@ -3982,6 +3991,7 @@  static const struct file_operations nvme_ns_chr_fops = {
 	.compat_ioctl	= compat_ptr_ioctl,
 	.uring_cmd	= nvme_ns_chr_uring_cmd,
 	.uring_cmd_iopoll = nvme_ns_chr_uring_cmd_iopoll,
+	.uring_cmd_sec	= nvme_uring_cmd_sec,
 };
 
 static int nvme_add_ns_cdev(struct nvme_ns *ns)
diff --git a/include/linux/fs.h b/include/linux/fs.h
index e654435f1651..af743a2dd562 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2091,6 +2091,7 @@  struct dir_context {
 
 struct iov_iter;
 struct io_uring_cmd;
+struct security_uring_cmd;
 
 struct file_operations {
 	struct module *owner;
@@ -2136,6 +2137,7 @@  struct file_operations {
 	int (*uring_cmd)(struct io_uring_cmd *ioucmd, unsigned int issue_flags);
 	int (*uring_cmd_iopoll)(struct io_uring_cmd *, struct io_comp_batch *,
 				unsigned int poll_flags);
+	int (*uring_cmd_sec)(struct io_uring_cmd *ioucmd, struct security_uring_cmd*);
 } __randomize_layout;
 
 struct inode_operations {
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index ec119da1d89b..6cef29bce373 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -408,5 +408,6 @@  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_cmd, struct io_uring_cmd *ioucmd)
+LSM_HOOK(int, 0, uring_cmd, struct io_uring_cmd *ioucmd,
+	int (*uring_cmd_sec)(struct io_uring_cmd *ioucmd, struct security_uring_cmd*))
 #endif /* CONFIG_IO_URING */
diff --git a/include/linux/security.h b/include/linux/security.h
index ca1b7109c0db..146b1bbdc2e0 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -2065,10 +2065,20 @@  static inline int security_perf_event_write(struct perf_event *event)
 #endif /* CONFIG_PERF_EVENTS */
 
 #ifdef CONFIG_IO_URING
+enum security_uring_cmd_type
+{
+	SECURITY_URING_CMD_TYPE_IOCTL,
+};
+
+struct security_uring_cmd {
+	u64 flags;
+};
 #ifdef CONFIG_SECURITY
 extern int security_uring_override_creds(const struct cred *new);
 extern int security_uring_sqpoll(void);
-extern int security_uring_cmd(struct io_uring_cmd *ioucmd);
+extern int security_uring_cmd(struct io_uring_cmd *ioucmd,
+		int (*uring_cmd_sec)(struct io_uring_cmd *,
+			struct security_uring_cmd*));
 #else
 static inline int security_uring_override_creds(const struct cred *new)
 {
@@ -2078,7 +2088,9 @@  static inline int security_uring_sqpoll(void)
 {
 	return 0;
 }
-static inline int security_uring_cmd(struct io_uring_cmd *ioucmd)
+static inline int security_uring_cmd(struct io_uring_cmd *ioucmd,
+		int (*uring_cmd_sec)(struct io_uring_cmd *,
+			struct security_uring_cmd*))
 {
 	return 0;
 }
diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c
index e50de0b6b9f8..2f650b346756 100644
--- a/io_uring/uring_cmd.c
+++ b/io_uring/uring_cmd.c
@@ -108,10 +108,11 @@  int io_uring_cmd(struct io_kiocb *req, unsigned int issue_flags)
 	struct file *file = req->file;
 	int ret;
 
+	//req->file->f_op->owner->ei_funcs
 	if (!req->file->f_op->uring_cmd)
 		return -EOPNOTSUPP;
 
-	ret = security_uring_cmd(ioucmd);
+	ret = security_uring_cmd(ioucmd, req->file->f_op->uring_cmd_sec);
 	if (ret)
 		return ret;
 
diff --git a/security/security.c b/security/security.c
index 79d82cb6e469..d3360a32f971 100644
--- a/security/security.c
+++ b/security/security.c
@@ -2667,8 +2667,9 @@  int security_uring_sqpoll(void)
 {
 	return call_int_hook(uring_sqpoll, 0);
 }
-int security_uring_cmd(struct io_uring_cmd *ioucmd)
+int security_uring_cmd(struct io_uring_cmd *ioucmd,
+	int (*uring_cmd_sec)(struct io_uring_cmd *ioucmd, struct security_uring_cmd*))
 {
-	return call_int_hook(uring_cmd, 0, ioucmd);
+	return call_int_hook(uring_cmd, 0, ioucmd, uring_cmd_sec);
 }
 #endif /* CONFIG_IO_URING */
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index f553c370397e..9fe3a230c671 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -21,6 +21,8 @@ 
  *  Copyright (C) 2016 Mellanox Technologies
  */
 
+#include "linux/nvme_ioctl.h"
+#include "linux/security.h"
 #include <linux/init.h>
 #include <linux/kd.h>
 #include <linux/kernel.h>
@@ -6999,18 +7001,30 @@  static int selinux_uring_sqpoll(void)
  * IORING_OP_URING_CMD against the device/file specified in @ioucmd.
  *
  */
-static int selinux_uring_cmd(struct io_uring_cmd *ioucmd)
+static int selinux_uring_cmd(struct io_uring_cmd *ioucmd,
+	int (*uring_cmd_sec)(struct io_uring_cmd *ioucmd, struct security_uring_cmd*))
 {
 	struct file *file = ioucmd->file;
 	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();
+	struct security_uring_cmd sec_uring = {0};
+	int ret;
 
 	ad.type = LSM_AUDIT_DATA_FILE;
 	ad.u.file = file;
 
+	ret = uring_cmd_sec(ioucmd, &sec_uring);
+	if (ret)
+		return ret;
+
+	if (sec_uring.flags & SECURITY_URING_CMD_TYPE_IOCTL)
+		return ioctl_has_perm(cred, file, FILE__IOCTL, (u16) ioucmd->cmd_op);
+
 	return avc_has_perm(&selinux_state, current_sid(), isec->sid,
 			    SECCLASS_IO_URING, IO_URING__CMD, &ad);
+
 }
 #endif /* CONFIG_IO_URING */