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@xxxxxxxxxxx>
> > > ---
> > > 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:
signature.asc
Description: PGP signature
