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 > > --- > > 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 > > #include > > #include > > @@ -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 > > #include > > #include > > @@ -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 */ > >