| Message ID | 20221122103144.960752-2-j.granados@samsung.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC,v2,1/1] Use a fs callback to set security specific data | expand |
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 */ >
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 */
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 */ > >
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 */ > > >
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
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?
On Tue, Nov 29, 2022 at 06:24:00AM -0800, Christoph Hellwig wrote: > This seems to be missing any kind of changelog. Also the > subject should say file_operations. Most of the instances here are > not in a file system, and they most certainly aren't callbacks. > > I think you've also missed a whole lot of maintainers. > > > +#include "linux/security.h" > > That's now how we include kernel-wide headers. > > > #include <linux/blkdev.h> > > #include <linux/blk-mq.h> > > #include <linux/blk-integrity.h> > > @@ -3308,6 +3309,13 @@ static int nvme_dev_release(struct inode *inode, struct file *file) > > return 0; > > } > > > > +int nvme_uring_cmd_sec(struct io_uring_cmd *ioucmd, struct security_uring_cmd *sec) > > Douple white space and overly long line. > > > +{ > > + sec->flags = 0; > > + sec->flags = SECURITY_URING_CMD_TYPE_IOCTL; > > Double initialization of ->flags. But how is this supposed to work > to start with? This RFC is meant to see how different solutions may play out. I'm not trying to push anything through yet. Just testing the waters to see what sticks and what people think about certain approaches. Should have mentioned that in my cover letter. My idea was to bring all relevant maintainers into the conversation once I had a more clear idea on what needed to be done and how I would do it. Since the patch is just a discussion piece, it is riddled with errors like the ones you pointed out. The idea with this second version is to add a security uring callback to the already existing ones in the file_operations structure. This new callback will fill in a security struct that will contain all the data needed for the LSMs to do their thing. This callback can be protected by an 'ifdef' for performance purposes. There is a third proposal by Ming Lei that uses the io_uring_sqe struct to embed io_uring type information. In my todo list I have a task to implement this and present it as a third option. best Joel
diff --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 */
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(-)