[RFC v2 0/1] RFC on how to include LSM hooks for io_uring commands

[RFC v2 0/1] RFC on how to include LSM hooks for io_uring commands

[Joel Granados]

The motivation for this patch is to continue the discussion around how to
include LSM callback hooks in the io_uring infrastructure. This is the
second version of the RFC and is meant to elicit discussion. I'll leave
general questions and the descriptions of the different approaches.
Comments are greatly appreciated

Approaches:
V2: I add a callback to the file_operations struct that will set a
security_uring structure with all the elements needed for LSMs to make a
decision. io_uring is still agnostic as it will just pass the callback
along and LSM can just focus on getting the data they need in the uring
security struct. When security is not defined in CONFIG the
security_uring_cmd can just be a noop (or itself be in an ifdef).

V1: I take the nvme io_uring passthrough and try to include it in the
already existing LSM infrastructure that is there for ioctl. This is far
from a general io_uring approach, but its a start :)

Questions:
1. Besides what is contained in the patch, would there be something
additional to plumb in LSM?

2. Is this general enough to fit all io_uring passthrough commands?

3. I'm trying to separate responsabilities. The LSM folks can take care of
LSM stuff and the io_uring users can take care of their specific domain.
Does this patch fulfill this?

4. Are there other approaches to solve this problem?

Joel Granados (1):
  Use a fs callback to set security specific data

 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(-)

-- 
2.30.2

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

[Joel Granados]

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*))
 #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 */
 
-- 
2.30.2

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

[Casey Schaufler]

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 */
>  

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

[Paul Moore]

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 */

-- 
paul-moore.com

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

[Joel Granados]

[-- Attachment #1: Type: text/plain, Size: 8583 bytes --]

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 */
> >  

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

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

[Joel Granados]

[-- Attachment #1: Type: text/plain, Size: 9579 bytes --]

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 */
> > >  



[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

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

[Joel Granados]

[-- Attachment #1: Type: text/plain, Size: 4758 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

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

[Christoph Hellwig]

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?

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

[Joel Granados]

[-- Attachment #1: Type: text/plain, Size: 2020 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]