* [RFC v2 0/1] RFC on how to include LSM hooks for io_uring commands [not found] <CGME20221122103536eucas1p2a0bc5ebdf063715f063e5b6254d0b058@eucas1p2.samsung.com> @ 2022-11-22 10:31 ` Joel Granados [not found] ` <CGME20221122103536eucas1p28f1c88f2300e49942c789721fe70c428@eucas1p2.samsung.com> 0 siblings, 1 reply; 9+ messages in thread From: Joel Granados @ 2022-11-22 10:31 UTC (permalink / raw) To: mcgrof, ddiss, joshi.k, paul Cc: ming.lei, linux-security-module, axboe, io-uring, 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
[parent not found: <CGME20221122103536eucas1p28f1c88f2300e49942c789721fe70c428@eucas1p2.samsung.com>]
* [RFC v2 1/1] Use a fs callback to set security specific data [not found] ` <CGME20221122103536eucas1p28f1c88f2300e49942c789721fe70c428@eucas1p2.samsung.com> @ 2022-11-22 10:31 ` Joel Granados 2022-11-22 15:18 ` Casey Schaufler ` (2 more replies) 0 siblings, 3 replies; 9+ messages in thread From: Joel Granados @ 2022-11-22 10:31 UTC (permalink / raw) To: mcgrof, ddiss, joshi.k, paul Cc: ming.lei, linux-security-module, axboe, io-uring, 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [RFC v2 1/1] Use a fs callback to set security specific data 2022-11-22 10:31 ` [RFC v2 1/1] Use a fs callback to set security specific data Joel Granados @ 2022-11-22 15:18 ` Casey Schaufler 2022-11-28 8:19 ` Joel Granados 2022-11-23 21:02 ` Paul Moore 2022-11-29 14:24 ` Christoph Hellwig 2 siblings, 1 reply; 9+ messages in thread From: Casey Schaufler @ 2022-11-22 15:18 UTC (permalink / raw) To: Joel Granados, mcgrof, ddiss, joshi.k, paul Cc: ming.lei, linux-security-module, axboe, io-uring, casey 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 */ > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC v2 1/1] Use a fs callback to set security specific data 2022-11-22 15:18 ` Casey Schaufler @ 2022-11-28 8:19 ` Joel Granados 2022-11-28 9:06 ` Joel Granados 0 siblings, 1 reply; 9+ messages in thread From: Joel Granados @ 2022-11-28 8:19 UTC (permalink / raw) To: Casey Schaufler Cc: mcgrof, ddiss, joshi.k, paul, ming.lei, linux-security-module, axboe, io-uring [-- 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 --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC v2 1/1] Use a fs callback to set security specific data 2022-11-28 8:19 ` Joel Granados @ 2022-11-28 9:06 ` Joel Granados 0 siblings, 0 replies; 9+ messages in thread From: Joel Granados @ 2022-11-28 9:06 UTC (permalink / raw) To: Casey Schaufler Cc: mcgrof, ddiss, joshi.k, paul, ming.lei, linux-security-module, axboe, io-uring [-- 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 --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC v2 1/1] Use a fs callback to set security specific data 2022-11-22 10:31 ` [RFC v2 1/1] Use a fs callback to set security specific data Joel Granados 2022-11-22 15:18 ` Casey Schaufler @ 2022-11-23 21:02 ` Paul Moore 2022-11-28 9:27 ` Joel Granados 2022-11-29 14:24 ` Christoph Hellwig 2 siblings, 1 reply; 9+ messages in thread From: Paul Moore @ 2022-11-23 21:02 UTC (permalink / raw) To: Joel Granados Cc: mcgrof, ddiss, joshi.k, ming.lei, linux-security-module, axboe, io-uring 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC v2 1/1] Use a fs callback to set security specific data 2022-11-23 21:02 ` Paul Moore @ 2022-11-28 9:27 ` Joel Granados 0 siblings, 0 replies; 9+ messages in thread From: Joel Granados @ 2022-11-28 9:27 UTC (permalink / raw) To: Paul Moore Cc: mcgrof, ddiss, joshi.k, ming.lei, linux-security-module, axboe, io-uring [-- 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 --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC v2 1/1] Use a fs callback to set security specific data 2022-11-22 10:31 ` [RFC v2 1/1] Use a fs callback to set security specific data Joel Granados 2022-11-22 15:18 ` Casey Schaufler 2022-11-23 21:02 ` Paul Moore @ 2022-11-29 14:24 ` Christoph Hellwig 2022-11-30 21:29 ` Joel Granados 2 siblings, 1 reply; 9+ messages in thread From: Christoph Hellwig @ 2022-11-29 14:24 UTC (permalink / raw) To: Joel Granados Cc: mcgrof, ddiss, joshi.k, paul, ming.lei, linux-security-module, axboe, io-uring 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? ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC v2 1/1] Use a fs callback to set security specific data 2022-11-29 14:24 ` Christoph Hellwig @ 2022-11-30 21:29 ` Joel Granados 0 siblings, 0 replies; 9+ messages in thread From: Joel Granados @ 2022-11-30 21:29 UTC (permalink / raw) To: Christoph Hellwig Cc: mcgrof, ddiss, joshi.k, paul, ming.lei, linux-security-module, axboe, io-uring [-- 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 --] ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-11-30 21:29 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <CGME20221122103536eucas1p2a0bc5ebdf063715f063e5b6254d0b058@eucas1p2.samsung.com>
2022-11-22 10:31 ` [RFC v2 0/1] RFC on how to include LSM hooks for io_uring commands Joel Granados
[not found] ` <CGME20221122103536eucas1p28f1c88f2300e49942c789721fe70c428@eucas1p2.samsung.com>
2022-11-22 10:31 ` [RFC v2 1/1] Use a fs callback to set security specific data Joel Granados
2022-11-22 15:18 ` Casey Schaufler
2022-11-28 8:19 ` Joel Granados
2022-11-28 9:06 ` Joel Granados
2022-11-23 21:02 ` Paul Moore
2022-11-28 9:27 ` Joel Granados
2022-11-29 14:24 ` Christoph Hellwig
2022-11-30 21:29 ` Joel Granados
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).