| Message ID | 20220308152105.309618-4-joshi.k@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | io_uring passthru over nvme | expand |
On Tue, Mar 08, 2022 at 08:50:51PM +0530, Kanchan Joshi wrote: > From: Jens Axboe <axboe@kernel.dk> > > This is a file private kind of request. io_uring doesn't know what's > in this command type, it's for the file_operations->async_cmd() > handler to deal with. > > Signed-off-by: Jens Axboe <axboe@kernel.dk> > Signed-off-by: Kanchan Joshi <joshi.k@samsung.com> > --- <-- snip --> > +static int io_uring_cmd(struct io_kiocb *req, unsigned int issue_flags) > +{ > + struct file *file = req->file; > + int ret; > + struct io_uring_cmd *ioucmd = &req->uring_cmd; > + > + ioucmd->flags |= issue_flags; > + ret = file->f_op->async_cmd(ioucmd); I think we're going to have to add a security_file_async_cmd() check before this call here. Because otherwise we're enabling to, for example, bypass security_file_ioctl() for example using the new iouring-cmd interface. Or is this already thought out with the existing security_uring_*() stuff? Luis
On 3/10/22 6:51 PM, Luis Chamberlain wrote: > On Tue, Mar 08, 2022 at 08:50:51PM +0530, Kanchan Joshi wrote: >> From: Jens Axboe <axboe@kernel.dk> >> >> This is a file private kind of request. io_uring doesn't know what's >> in this command type, it's for the file_operations->async_cmd() >> handler to deal with. >> >> Signed-off-by: Jens Axboe <axboe@kernel.dk> >> Signed-off-by: Kanchan Joshi <joshi.k@samsung.com> >> --- > > <-- snip --> > >> +static int io_uring_cmd(struct io_kiocb *req, unsigned int issue_flags) >> +{ >> + struct file *file = req->file; >> + int ret; >> + struct io_uring_cmd *ioucmd = &req->uring_cmd; >> + >> + ioucmd->flags |= issue_flags; >> + ret = file->f_op->async_cmd(ioucmd); > > I think we're going to have to add a security_file_async_cmd() check > before this call here. Because otherwise we're enabling to, for > example, bypass security_file_ioctl() for example using the new > iouring-cmd interface. > > Or is this already thought out with the existing security_uring_*() stuff? Unless the request sets .audit_skip, it'll be included already in terms of logging. But I'd prefer not to lodge this in with ioctls, unless we're going to be doing actual ioctls. But definitely something to keep in mind and make sure that we're under the right umbrella in terms of auditing and security.
On Thu, Mar 10, 2022 at 07:43:04PM -0700, Jens Axboe wrote: > On 3/10/22 6:51 PM, Luis Chamberlain wrote: > > On Tue, Mar 08, 2022 at 08:50:51PM +0530, Kanchan Joshi wrote: > >> From: Jens Axboe <axboe@kernel.dk> > >> > >> This is a file private kind of request. io_uring doesn't know what's > >> in this command type, it's for the file_operations->async_cmd() > >> handler to deal with. > >> > >> Signed-off-by: Jens Axboe <axboe@kernel.dk> > >> Signed-off-by: Kanchan Joshi <joshi.k@samsung.com> > >> --- > > > > <-- snip --> > > > >> +static int io_uring_cmd(struct io_kiocb *req, unsigned int issue_flags) > >> +{ > >> + struct file *file = req->file; > >> + int ret; > >> + struct io_uring_cmd *ioucmd = &req->uring_cmd; > >> + > >> + ioucmd->flags |= issue_flags; > >> + ret = file->f_op->async_cmd(ioucmd); > > > > I think we're going to have to add a security_file_async_cmd() check > > before this call here. Because otherwise we're enabling to, for > > example, bypass security_file_ioctl() for example using the new > > iouring-cmd interface. > > > > Or is this already thought out with the existing security_uring_*() stuff? > > Unless the request sets .audit_skip, it'll be included already in terms > of logging. Neat. > But I'd prefer not to lodge this in with ioctls, unless > we're going to be doing actual ioctls. Oh sure, I have been an advocate to ensure folks don't conflate async_cmd with ioctl. However it *can* enable subsystems to enable ioctl passthrough, but each of those subsystems need to vet for this on their own terms. I'd hate to see / hear some LSM surprises later. > But definitely something to keep in mind and make sure that we're under > the right umbrella in terms of auditing and security. Paul, how about something like this for starters (and probably should be squashed into this series so its not a separate commit) ? From f3ddbe822374cc1c7002bd795c1ae486d370cbd1 Mon Sep 17 00:00:00 2001 From: Luis Chamberlain <mcgrof@kernel.org> Date: Fri, 11 Mar 2022 08:55:50 -0800 Subject: [PATCH] lsm,io_uring: add LSM hooks to for the new async_cmd file op io-uring is extending the struct file_operations to allow a new command which each subsystem can use to enable command passthrough. Add an LSM specific for the command passthrough which enables LSMs to inspect the command details. Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> --- fs/io_uring.c | 5 +++++ include/linux/lsm_hook_defs.h | 1 + include/linux/lsm_hooks.h | 3 +++ include/linux/security.h | 5 +++++ security/security.c | 4 ++++ 5 files changed, 18 insertions(+) diff --git a/fs/io_uring.c b/fs/io_uring.c index 3f6eacc98e31..1c4e6b2cb61a 100644 --- a/fs/io_uring.c +++ b/fs/io_uring.c @@ -4190,6 +4190,11 @@ static int io_uring_cmd_prep(struct io_kiocb *req, struct io_ring_ctx *ctx = req->ctx; struct io_uring_cmd *ioucmd = &req->uring_cmd; u32 ucmd_flags = READ_ONCE(sqe->uring_cmd_flags); + int ret; + + ret = security_uring_async_cmd(ioucmd); + if (ret) + return ret; if (!req->file->f_op->async_cmd) return -EOPNOTSUPP; diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h index 819ec92dc2a8..4a20f8e6b295 100644 --- a/include/linux/lsm_hook_defs.h +++ b/include/linux/lsm_hook_defs.h @@ -404,4 +404,5 @@ 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_async_cmd, struct io_uring_cmd *ioucmd) #endif /* CONFIG_IO_URING */ diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h index 3bf5c658bc44..21b18cf138c2 100644 --- a/include/linux/lsm_hooks.h +++ b/include/linux/lsm_hooks.h @@ -1569,6 +1569,9 @@ * Check whether the current task is allowed to spawn a io_uring polling * thread (IORING_SETUP_SQPOLL). * + * @uring_async_cmd: + * Check whether the file_operations async_cmd is allowed to run. + * */ union security_list_options { #define LSM_HOOK(RET, DEFAULT, NAME, ...) RET (*NAME)(__VA_ARGS__); diff --git a/include/linux/security.h b/include/linux/security.h index 6d72772182c8..4d7f72813d75 100644 --- a/include/linux/security.h +++ b/include/linux/security.h @@ -2041,6 +2041,7 @@ static inline int security_perf_event_write(struct perf_event *event) #ifdef CONFIG_SECURITY extern int security_uring_override_creds(const struct cred *new); extern int security_uring_sqpoll(void); +extern int security_uring_async_cmd(struct io_uring_cmd *ioucmd); #else static inline int security_uring_override_creds(const struct cred *new) { @@ -2050,6 +2051,10 @@ static inline int security_uring_sqpoll(void) { return 0; } +static inline int security_uring_async_cmd(struct io_uring_cmd *ioucmd) +{ + return 0; +} #endif /* CONFIG_SECURITY */ #endif /* CONFIG_IO_URING */ diff --git a/security/security.c b/security/security.c index 22261d79f333..ef96be2f953a 100644 --- a/security/security.c +++ b/security/security.c @@ -2640,4 +2640,8 @@ int security_uring_sqpoll(void) { return call_int_hook(uring_sqpoll, 0); } +int security_uring_async_cmd(struct io_uring_cmd *ioucmd) +{ + return call_int_hook(uring_async_cmd, 0, ioucmd); +} #endif /* CONFIG_IO_URING */
On Fri, Mar 11, 2022 at 12:11 PM Luis Chamberlain <mcgrof@kernel.org> wrote: > On Thu, Mar 10, 2022 at 07:43:04PM -0700, Jens Axboe wrote: > > On 3/10/22 6:51 PM, Luis Chamberlain wrote: > > > On Tue, Mar 08, 2022 at 08:50:51PM +0530, Kanchan Joshi wrote: > > >> From: Jens Axboe <axboe@kernel.dk> > > >> > > >> This is a file private kind of request. io_uring doesn't know what's > > >> in this command type, it's for the file_operations->async_cmd() > > >> handler to deal with. > > >> > > >> Signed-off-by: Jens Axboe <axboe@kernel.dk> > > >> Signed-off-by: Kanchan Joshi <joshi.k@samsung.com> > > >> --- > > > > > > <-- snip --> > > > > > >> +static int io_uring_cmd(struct io_kiocb *req, unsigned int issue_flags) > > >> +{ > > >> + struct file *file = req->file; > > >> + int ret; > > >> + struct io_uring_cmd *ioucmd = &req->uring_cmd; > > >> + > > >> + ioucmd->flags |= issue_flags; > > >> + ret = file->f_op->async_cmd(ioucmd); > > > > > > I think we're going to have to add a security_file_async_cmd() check > > > before this call here. Because otherwise we're enabling to, for > > > example, bypass security_file_ioctl() for example using the new > > > iouring-cmd interface. > > > > > > Or is this already thought out with the existing security_uring_*() stuff? > > > > Unless the request sets .audit_skip, it'll be included already in terms > > of logging. > > Neat. [NOTE: added the audit and SELinux lists to the To/CC line] Neat, but I think we will need to augment things to support this new passthrough mechanism. The issue is that folks who look at audit logs need to be able to piece together what happened on the system using just what they have in the logs themselves. As things currently stand with this patchset, the only bit of information they would have to go on would be "uring_op=<IORING_OP_URING_CMD>" which isn't very informative :) You'll see a similar issue in the newly proposed LSM hook below, we need to be able to record information about not only the passthrough command, e.g. io_uring_cmd::cmd_op, but also the underlying device/handler so that we can put the passthrough command in the right context (as far as I can tell io_uring_cmd::cmd_op is specific to the device). We might be able to leverage file_operations::owner::name for this, e.g. "uring_passthru_dev=nvme uring_passthru_op=<NVME_IOCTL_IO64_CMD>". > > But I'd prefer not to lodge this in with ioctls, unless > > we're going to be doing actual ioctls. > > Oh sure, I have been an advocate to ensure folks don't conflate async_cmd > with ioctl. However it *can* enable subsystems to enable ioctl > passthrough, but each of those subsystems need to vet for this on their > own terms. I'd hate to see / hear some LSM surprises later. Same :) Thanks for bringing this up with us while the patches are still in-progress/under-review, I think it makes for a much more pleasant experience for everyone. > > But definitely something to keep in mind and make sure that we're under > > the right umbrella in terms of auditing and security. > > Paul, how about something like this for starters (and probably should > be squashed into this series so its not a separate commit) ? > > From f3ddbe822374cc1c7002bd795c1ae486d370cbd1 Mon Sep 17 00:00:00 2001 > From: Luis Chamberlain <mcgrof@kernel.org> > Date: Fri, 11 Mar 2022 08:55:50 -0800 > Subject: [PATCH] lsm,io_uring: add LSM hooks to for the new async_cmd file op > > io-uring is extending the struct file_operations to allow a new > command which each subsystem can use to enable command passthrough. > Add an LSM specific for the command passthrough which enables LSMs > to inspect the command details. > > Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> > --- > fs/io_uring.c | 5 +++++ > include/linux/lsm_hook_defs.h | 1 + > include/linux/lsm_hooks.h | 3 +++ > include/linux/security.h | 5 +++++ > security/security.c | 4 ++++ > 5 files changed, 18 insertions(+) > > diff --git a/fs/io_uring.c b/fs/io_uring.c > index 3f6eacc98e31..1c4e6b2cb61a 100644 > --- a/fs/io_uring.c > +++ b/fs/io_uring.c > @@ -4190,6 +4190,11 @@ static int io_uring_cmd_prep(struct io_kiocb *req, > struct io_ring_ctx *ctx = req->ctx; > struct io_uring_cmd *ioucmd = &req->uring_cmd; > u32 ucmd_flags = READ_ONCE(sqe->uring_cmd_flags); > + int ret; > + > + ret = security_uring_async_cmd(ioucmd); > + if (ret) > + return ret; As a quick aside, for the LSM/audit folks the lore link for the full patchset is here: https://lore.kernel.org/io-uring/CA+1E3rJ17F0Rz5UKUnW-LPkWDfPHXG5aeq-ocgNxHfGrxYtAuw@mail.gmail.com/T/#m605e2fb7caf33e8880683fe6b57ade4093ed0643 Similar to what was discussed above with respect to auditing, I think we need to do some extra work here to make it easier for a LSM to put the IO request in the proper context. We have io_uring_cmd::cmd_op via the @ioucmd parameter, which is good, but we need to be able to associate that with a driver to make sense of it. In the case of audit we could simply use the module name string, which is probably ideal as we would want a string anyway, but LSMs will likely want something more machine friendly. That isn't to say we couldn't do a strcmp() on the module name string, but for something that aims to push performance as much as possible, doing a strcmp() on each operation seems a little less than optimal ;)
On Fri, Mar 11, 2022 at 01:47:51PM -0500, Paul Moore wrote: > On Fri, Mar 11, 2022 at 12:11 PM Luis Chamberlain <mcgrof@kernel.org> wrote: > > On Thu, Mar 10, 2022 at 07:43:04PM -0700, Jens Axboe wrote: > > > On 3/10/22 6:51 PM, Luis Chamberlain wrote: > > > > On Tue, Mar 08, 2022 at 08:50:51PM +0530, Kanchan Joshi wrote: > > > >> From: Jens Axboe <axboe@kernel.dk> > > > >> > > > >> This is a file private kind of request. io_uring doesn't know what's > > > >> in this command type, it's for the file_operations->async_cmd() > > > >> handler to deal with. > > > >> > > > >> Signed-off-by: Jens Axboe <axboe@kernel.dk> > > > >> Signed-off-by: Kanchan Joshi <joshi.k@samsung.com> > > > >> --- > > > > > > > > <-- snip --> > > > > > > > >> +static int io_uring_cmd(struct io_kiocb *req, unsigned int issue_flags) > > > >> +{ > > > >> + struct file *file = req->file; > > > >> + int ret; > > > >> + struct io_uring_cmd *ioucmd = &req->uring_cmd; > > > >> + > > > >> + ioucmd->flags |= issue_flags; > > > >> + ret = file->f_op->async_cmd(ioucmd); > > > > > > > > I think we're going to have to add a security_file_async_cmd() check > > > > before this call here. Because otherwise we're enabling to, for > > > > example, bypass security_file_ioctl() for example using the new > > > > iouring-cmd interface. > > > > > > > > Or is this already thought out with the existing security_uring_*() stuff? > > > > > > Unless the request sets .audit_skip, it'll be included already in terms > > > of logging. > > > > Neat. > > [NOTE: added the audit and SELinux lists to the To/CC line] > > Neat, but I think we will need to augment things to support this new > passthrough mechanism. That's what my spidey instincts told me. > The issue is that folks who look at audit logs need to be able to > piece together what happened on the system using just what they have > in the logs themselves. As things currently stand with this patchset, > the only bit of information they would have to go on would be > "uring_op=<IORING_OP_URING_CMD>" which isn't very informative :) > > You'll see a similar issue in the newly proposed LSM hook below, we > need to be able to record information about not only the passthrough > command, e.g. io_uring_cmd::cmd_op, but also the underlying > device/handler so that we can put the passthrough command in the right > context (as far as I can tell io_uring_cmd::cmd_op is specific to the > device). We might be able to leverage file_operations::owner::name > for this, e.g. "uring_passthru_dev=nvme > uring_passthru_op=<NVME_IOCTL_IO64_CMD>". OK... > > > But I'd prefer not to lodge this in with ioctls, unless > > > we're going to be doing actual ioctls. > > > > Oh sure, I have been an advocate to ensure folks don't conflate async_cmd > > with ioctl. However it *can* enable subsystems to enable ioctl > > passthrough, but each of those subsystems need to vet for this on their > > own terms. I'd hate to see / hear some LSM surprises later. > > Same :) Thanks for bringing this up with us while the patches are > still in-progress/under-review, I think it makes for a much more > pleasant experience for everyone. Sure thing. > > > But definitely something to keep in mind and make sure that we're under > > > the right umbrella in terms of auditing and security. > > > > Paul, how about something like this for starters (and probably should > > be squashed into this series so its not a separate commit) ? > > > > From f3ddbe822374cc1c7002bd795c1ae486d370cbd1 Mon Sep 17 00:00:00 2001 > > From: Luis Chamberlain <mcgrof@kernel.org> > > Date: Fri, 11 Mar 2022 08:55:50 -0800 > > Subject: [PATCH] lsm,io_uring: add LSM hooks to for the new async_cmd file op > > > > io-uring is extending the struct file_operations to allow a new > > command which each subsystem can use to enable command passthrough. > > Add an LSM specific for the command passthrough which enables LSMs > > to inspect the command details. > > > > Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> > > --- > > fs/io_uring.c | 5 +++++ > > include/linux/lsm_hook_defs.h | 1 + > > include/linux/lsm_hooks.h | 3 +++ > > include/linux/security.h | 5 +++++ > > security/security.c | 4 ++++ > > 5 files changed, 18 insertions(+) > > > > diff --git a/fs/io_uring.c b/fs/io_uring.c > > index 3f6eacc98e31..1c4e6b2cb61a 100644 > > --- a/fs/io_uring.c > > +++ b/fs/io_uring.c > > @@ -4190,6 +4190,11 @@ static int io_uring_cmd_prep(struct io_kiocb *req, > > struct io_ring_ctx *ctx = req->ctx; > > struct io_uring_cmd *ioucmd = &req->uring_cmd; > > u32 ucmd_flags = READ_ONCE(sqe->uring_cmd_flags); > > + int ret; > > + > > + ret = security_uring_async_cmd(ioucmd); > > + if (ret) > > + return ret; > > As a quick aside, for the LSM/audit folks the lore link for the full > patchset is here: > https://lore.kernel.org/io-uring/CA+1E3rJ17F0Rz5UKUnW-LPkWDfPHXG5aeq-ocgNxHfGrxYtAuw@mail.gmail.com/T/#m605e2fb7caf33e8880683fe6b57ade4093ed0643 > > Similar to what was discussed above with respect to auditing, I think > we need to do some extra work here to make it easier for a LSM to put > the IO request in the proper context. We have io_uring_cmd::cmd_op > via the @ioucmd parameter, which is good, but we need to be able to > associate that with a driver to make sense of it. It may not always be a driver, it can be built-in stuff. > In the case of > audit we could simply use the module name string, which is probably > ideal as we would want a string anyway, but LSMs will likely want > something more machine friendly. That isn't to say we couldn't do a > strcmp() on the module name string, but for something that aims to > push performance as much as possible, doing a strcmp() on each > operation seems a little less than optimal ;) Yes this is a super hot path... Luis
On Fri, Mar 11, 2022 at 3:57 PM Luis Chamberlain <mcgrof@kernel.org> wrote: > On Fri, Mar 11, 2022 at 01:47:51PM -0500, Paul Moore wrote: ... > > Similar to what was discussed above with respect to auditing, I think > > we need to do some extra work here to make it easier for a LSM to put > > the IO request in the proper context. We have io_uring_cmd::cmd_op > > via the @ioucmd parameter, which is good, but we need to be able to > > associate that with a driver to make sense of it. > > It may not always be a driver, it can be built-in stuff. Good point, but I believe the argument still applies. LSMs are going to need some way to put the cmd_op token in the proper context so that security policy can be properly enforced.
On 3/11/2022 9:11 AM, Luis Chamberlain wrote: > On Thu, Mar 10, 2022 at 07:43:04PM -0700, Jens Axboe wrote: >> On 3/10/22 6:51 PM, Luis Chamberlain wrote: >>> On Tue, Mar 08, 2022 at 08:50:51PM +0530, Kanchan Joshi wrote: >>>> From: Jens Axboe <axboe@kernel.dk> >>>> >>>> This is a file private kind of request. io_uring doesn't know what's >>>> in this command type, it's for the file_operations->async_cmd() >>>> handler to deal with. >>>> >>>> Signed-off-by: Jens Axboe <axboe@kernel.dk> >>>> Signed-off-by: Kanchan Joshi <joshi.k@samsung.com> >>>> --- >>> <-- snip --> >>> >>>> +static int io_uring_cmd(struct io_kiocb *req, unsigned int issue_flags) >>>> +{ >>>> + struct file *file = req->file; >>>> + int ret; >>>> + struct io_uring_cmd *ioucmd = &req->uring_cmd; >>>> + >>>> + ioucmd->flags |= issue_flags; >>>> + ret = file->f_op->async_cmd(ioucmd); >>> I think we're going to have to add a security_file_async_cmd() check >>> before this call here. Because otherwise we're enabling to, for >>> example, bypass security_file_ioctl() for example using the new >>> iouring-cmd interface. >>> >>> Or is this already thought out with the existing security_uring_*() stuff? >> Unless the request sets .audit_skip, it'll be included already in terms >> of logging. > Neat. > >> But I'd prefer not to lodge this in with ioctls, unless >> we're going to be doing actual ioctls. > Oh sure, I have been an advocate to ensure folks don't conflate async_cmd > with ioctl. However it *can* enable subsystems to enable ioctl > passthrough, but each of those subsystems need to vet for this on their > own terms. I'd hate to see / hear some LSM surprises later. > >> But definitely something to keep in mind and make sure that we're under >> the right umbrella in terms of auditing and security. > Paul, how about something like this for starters (and probably should > be squashed into this series so its not a separate commit) ? > > >From f3ddbe822374cc1c7002bd795c1ae486d370cbd1 Mon Sep 17 00:00:00 2001 > From: Luis Chamberlain <mcgrof@kernel.org> > Date: Fri, 11 Mar 2022 08:55:50 -0800 > Subject: [PATCH] lsm,io_uring: add LSM hooks to for the new async_cmd file op > > io-uring is extending the struct file_operations to allow a new > command which each subsystem can use to enable command passthrough. > Add an LSM specific for the command passthrough which enables LSMs > to inspect the command details. > > Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> > --- > fs/io_uring.c | 5 +++++ > include/linux/lsm_hook_defs.h | 1 + > include/linux/lsm_hooks.h | 3 +++ > include/linux/security.h | 5 +++++ > security/security.c | 4 ++++ > 5 files changed, 18 insertions(+) > > diff --git a/fs/io_uring.c b/fs/io_uring.c > index 3f6eacc98e31..1c4e6b2cb61a 100644 > --- a/fs/io_uring.c > +++ b/fs/io_uring.c > @@ -4190,6 +4190,11 @@ static int io_uring_cmd_prep(struct io_kiocb *req, > struct io_ring_ctx *ctx = req->ctx; > struct io_uring_cmd *ioucmd = &req->uring_cmd; > u32 ucmd_flags = READ_ONCE(sqe->uring_cmd_flags); > + int ret; > + > + ret = security_uring_async_cmd(ioucmd); > + if (ret) > + return ret; > > if (!req->file->f_op->async_cmd) > return -EOPNOTSUPP; > diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h > index 819ec92dc2a8..4a20f8e6b295 100644 > --- a/include/linux/lsm_hook_defs.h > +++ b/include/linux/lsm_hook_defs.h > @@ -404,4 +404,5 @@ 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_async_cmd, struct io_uring_cmd *ioucmd) > #endif /* CONFIG_IO_URING */ > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h > index 3bf5c658bc44..21b18cf138c2 100644 > --- a/include/linux/lsm_hooks.h > +++ b/include/linux/lsm_hooks.h > @@ -1569,6 +1569,9 @@ > * Check whether the current task is allowed to spawn a io_uring polling > * thread (IORING_SETUP_SQPOLL). > * > + * @uring_async_cmd: > + * Check whether the file_operations async_cmd is allowed to run. > + * > */ > union security_list_options { > #define LSM_HOOK(RET, DEFAULT, NAME, ...) RET (*NAME)(__VA_ARGS__); > diff --git a/include/linux/security.h b/include/linux/security.h > index 6d72772182c8..4d7f72813d75 100644 > --- a/include/linux/security.h > +++ b/include/linux/security.h > @@ -2041,6 +2041,7 @@ static inline int security_perf_event_write(struct perf_event *event) > #ifdef CONFIG_SECURITY > extern int security_uring_override_creds(const struct cred *new); > extern int security_uring_sqpoll(void); > +extern int security_uring_async_cmd(struct io_uring_cmd *ioucmd); > #else > static inline int security_uring_override_creds(const struct cred *new) > { > @@ -2050,6 +2051,10 @@ static inline int security_uring_sqpoll(void) > { > return 0; > } > +static inline int security_uring_async_cmd(struct io_uring_cmd *ioucmd) > +{ > + return 0; > +} > #endif /* CONFIG_SECURITY */ > #endif /* CONFIG_IO_URING */ > > diff --git a/security/security.c b/security/security.c > index 22261d79f333..ef96be2f953a 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -2640,4 +2640,8 @@ int security_uring_sqpoll(void) > { > return call_int_hook(uring_sqpoll, 0); > } > +int security_uring_async_cmd(struct io_uring_cmd *ioucmd) > +{ > + return call_int_hook(uring_async_cmd, 0, ioucmd); I don't have a good understanding of what information is in ioucmd. I am afraid that there may not be enough for a security module to make appropriate decisions in all cases. I am especially concerned about the modules that use path hooks, but based on the issues we've always had with ioctl and the like I fear for all cases. > +} > #endif /* CONFIG_IO_URING */
On Mon, Mar 14, 2022 at 09:25:35AM -0700, Casey Schaufler wrote: > On 3/11/2022 9:11 AM, Luis Chamberlain wrote: > > On Thu, Mar 10, 2022 at 07:43:04PM -0700, Jens Axboe wrote: > > > On 3/10/22 6:51 PM, Luis Chamberlain wrote: > > > > On Tue, Mar 08, 2022 at 08:50:51PM +0530, Kanchan Joshi wrote: > > > > > From: Jens Axboe <axboe@kernel.dk> > > > > > > > > > > This is a file private kind of request. io_uring doesn't know what's > > > > > in this command type, it's for the file_operations->async_cmd() > > > > > handler to deal with. > > > > > > > > > > Signed-off-by: Jens Axboe <axboe@kernel.dk> > > > > > Signed-off-by: Kanchan Joshi <joshi.k@samsung.com> > > > > > --- > > > > <-- snip --> > > > > > > > > > +static int io_uring_cmd(struct io_kiocb *req, unsigned int issue_flags) > > > > > +{ > > > > > + struct file *file = req->file; > > > > > + int ret; > > > > > + struct io_uring_cmd *ioucmd = &req->uring_cmd; > > > > > + > > > > > + ioucmd->flags |= issue_flags; > > > > > + ret = file->f_op->async_cmd(ioucmd); > > > > I think we're going to have to add a security_file_async_cmd() check > > > > before this call here. Because otherwise we're enabling to, for > > > > example, bypass security_file_ioctl() for example using the new > > > > iouring-cmd interface. > > > > > > > > Or is this already thought out with the existing security_uring_*() stuff? > > > Unless the request sets .audit_skip, it'll be included already in terms > > > of logging. > > Neat. > > > > > But I'd prefer not to lodge this in with ioctls, unless > > > we're going to be doing actual ioctls. > > Oh sure, I have been an advocate to ensure folks don't conflate async_cmd > > with ioctl. However it *can* enable subsystems to enable ioctl > > passthrough, but each of those subsystems need to vet for this on their > > own terms. I'd hate to see / hear some LSM surprises later. > > > > > But definitely something to keep in mind and make sure that we're under > > > the right umbrella in terms of auditing and security. > > Paul, how about something like this for starters (and probably should > > be squashed into this series so its not a separate commit) ? > > > > >From f3ddbe822374cc1c7002bd795c1ae486d370cbd1 Mon Sep 17 00:00:00 2001 > > From: Luis Chamberlain <mcgrof@kernel.org> > > Date: Fri, 11 Mar 2022 08:55:50 -0800 > > Subject: [PATCH] lsm,io_uring: add LSM hooks to for the new async_cmd file op > > > > io-uring is extending the struct file_operations to allow a new > > command which each subsystem can use to enable command passthrough. > > Add an LSM specific for the command passthrough which enables LSMs > > to inspect the command details. > > > > Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> > > --- > > fs/io_uring.c | 5 +++++ > > include/linux/lsm_hook_defs.h | 1 + > > include/linux/lsm_hooks.h | 3 +++ > > include/linux/security.h | 5 +++++ > > security/security.c | 4 ++++ > > 5 files changed, 18 insertions(+) > > > > diff --git a/fs/io_uring.c b/fs/io_uring.c > > index 3f6eacc98e31..1c4e6b2cb61a 100644 > > --- a/fs/io_uring.c > > +++ b/fs/io_uring.c > > @@ -4190,6 +4190,11 @@ static int io_uring_cmd_prep(struct io_kiocb *req, > > struct io_ring_ctx *ctx = req->ctx; > > struct io_uring_cmd *ioucmd = &req->uring_cmd; > > u32 ucmd_flags = READ_ONCE(sqe->uring_cmd_flags); > > + int ret; > > + > > + ret = security_uring_async_cmd(ioucmd); > > + if (ret) > > + return ret; > > if (!req->file->f_op->async_cmd) > > return -EOPNOTSUPP; > > diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h > > index 819ec92dc2a8..4a20f8e6b295 100644 > > --- a/include/linux/lsm_hook_defs.h > > +++ b/include/linux/lsm_hook_defs.h > > @@ -404,4 +404,5 @@ 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_async_cmd, struct io_uring_cmd *ioucmd) > > #endif /* CONFIG_IO_URING */ > > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h > > index 3bf5c658bc44..21b18cf138c2 100644 > > --- a/include/linux/lsm_hooks.h > > +++ b/include/linux/lsm_hooks.h > > @@ -1569,6 +1569,9 @@ > > * Check whether the current task is allowed to spawn a io_uring polling > > * thread (IORING_SETUP_SQPOLL). > > * > > + * @uring_async_cmd: > > + * Check whether the file_operations async_cmd is allowed to run. > > + * > > */ > > union security_list_options { > > #define LSM_HOOK(RET, DEFAULT, NAME, ...) RET (*NAME)(__VA_ARGS__); > > diff --git a/include/linux/security.h b/include/linux/security.h > > index 6d72772182c8..4d7f72813d75 100644 > > --- a/include/linux/security.h > > +++ b/include/linux/security.h > > @@ -2041,6 +2041,7 @@ static inline int security_perf_event_write(struct perf_event *event) > > #ifdef CONFIG_SECURITY > > extern int security_uring_override_creds(const struct cred *new); > > extern int security_uring_sqpoll(void); > > +extern int security_uring_async_cmd(struct io_uring_cmd *ioucmd); > > #else > > static inline int security_uring_override_creds(const struct cred *new) > > { > > @@ -2050,6 +2051,10 @@ static inline int security_uring_sqpoll(void) > > { > > return 0; > > } > > +static inline int security_uring_async_cmd(struct io_uring_cmd *ioucmd) > > +{ > > + return 0; > > +} > > #endif /* CONFIG_SECURITY */ > > #endif /* CONFIG_IO_URING */ > > diff --git a/security/security.c b/security/security.c > > index 22261d79f333..ef96be2f953a 100644 > > --- a/security/security.c > > +++ b/security/security.c > > @@ -2640,4 +2640,8 @@ int security_uring_sqpoll(void) > > { > > return call_int_hook(uring_sqpoll, 0); > > } > > +int security_uring_async_cmd(struct io_uring_cmd *ioucmd) > > +{ > > + return call_int_hook(uring_async_cmd, 0, ioucmd); > > I don't have a good understanding of what information is in ioucmd. > I am afraid that there may not be enough for a security module to > make appropriate decisions in all cases. I am especially concerned > about the modules that use path hooks, but based on the issues we've > always had with ioctl and the like I fear for all cases. As Paul pointed out, this particular LSM hook would not be needed if we can somehow ensure users of the cmd path use their respective LSMs there. It is not easy to force users to have the LSM hook to be used, one idea might be to have a registration mechanism which allows users to also specify the LSM hook, but these can vary in arguments, so perhaps then what is needed is the LSM type in enum form, and internally we have a mapping of these. That way we slowly itemize which cmds we *do* allow for, thus vetting at the same time a respective LSM hook. Thoughts? Luis
On 3/14/2022 9:32 AM, Luis Chamberlain wrote: > On Mon, Mar 14, 2022 at 09:25:35AM -0700, Casey Schaufler wrote: >> On 3/11/2022 9:11 AM, Luis Chamberlain wrote: >>> On Thu, Mar 10, 2022 at 07:43:04PM -0700, Jens Axboe wrote: >>>> On 3/10/22 6:51 PM, Luis Chamberlain wrote: >>>>> On Tue, Mar 08, 2022 at 08:50:51PM +0530, Kanchan Joshi wrote: >>>>>> From: Jens Axboe <axboe@kernel.dk> >>>>>> >>>>>> This is a file private kind of request. io_uring doesn't know what's >>>>>> in this command type, it's for the file_operations->async_cmd() >>>>>> handler to deal with. >>>>>> >>>>>> Signed-off-by: Jens Axboe <axboe@kernel.dk> >>>>>> Signed-off-by: Kanchan Joshi <joshi.k@samsung.com> >>>>>> --- >>>>> <-- snip --> >>>>> >>>>>> +static int io_uring_cmd(struct io_kiocb *req, unsigned int issue_flags) >>>>>> +{ >>>>>> + struct file *file = req->file; >>>>>> + int ret; >>>>>> + struct io_uring_cmd *ioucmd = &req->uring_cmd; >>>>>> + >>>>>> + ioucmd->flags |= issue_flags; >>>>>> + ret = file->f_op->async_cmd(ioucmd); >>>>> I think we're going to have to add a security_file_async_cmd() check >>>>> before this call here. Because otherwise we're enabling to, for >>>>> example, bypass security_file_ioctl() for example using the new >>>>> iouring-cmd interface. >>>>> >>>>> Or is this already thought out with the existing security_uring_*() stuff? >>>> Unless the request sets .audit_skip, it'll be included already in terms >>>> of logging. >>> Neat. >>> >>>> But I'd prefer not to lodge this in with ioctls, unless >>>> we're going to be doing actual ioctls. >>> Oh sure, I have been an advocate to ensure folks don't conflate async_cmd >>> with ioctl. However it *can* enable subsystems to enable ioctl >>> passthrough, but each of those subsystems need to vet for this on their >>> own terms. I'd hate to see / hear some LSM surprises later. >>> >>>> But definitely something to keep in mind and make sure that we're under >>>> the right umbrella in terms of auditing and security. >>> Paul, how about something like this for starters (and probably should >>> be squashed into this series so its not a separate commit) ? >>> >>> >From f3ddbe822374cc1c7002bd795c1ae486d370cbd1 Mon Sep 17 00:00:00 2001 >>> From: Luis Chamberlain <mcgrof@kernel.org> >>> Date: Fri, 11 Mar 2022 08:55:50 -0800 >>> Subject: [PATCH] lsm,io_uring: add LSM hooks to for the new async_cmd file op >>> >>> io-uring is extending the struct file_operations to allow a new >>> command which each subsystem can use to enable command passthrough. >>> Add an LSM specific for the command passthrough which enables LSMs >>> to inspect the command details. >>> >>> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> >>> --- >>> fs/io_uring.c | 5 +++++ >>> include/linux/lsm_hook_defs.h | 1 + >>> include/linux/lsm_hooks.h | 3 +++ >>> include/linux/security.h | 5 +++++ >>> security/security.c | 4 ++++ >>> 5 files changed, 18 insertions(+) >>> >>> diff --git a/fs/io_uring.c b/fs/io_uring.c >>> index 3f6eacc98e31..1c4e6b2cb61a 100644 >>> --- a/fs/io_uring.c >>> +++ b/fs/io_uring.c >>> @@ -4190,6 +4190,11 @@ static int io_uring_cmd_prep(struct io_kiocb *req, >>> struct io_ring_ctx *ctx = req->ctx; >>> struct io_uring_cmd *ioucmd = &req->uring_cmd; >>> u32 ucmd_flags = READ_ONCE(sqe->uring_cmd_flags); >>> + int ret; >>> + >>> + ret = security_uring_async_cmd(ioucmd); >>> + if (ret) >>> + return ret; >>> if (!req->file->f_op->async_cmd) >>> return -EOPNOTSUPP; >>> diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h >>> index 819ec92dc2a8..4a20f8e6b295 100644 >>> --- a/include/linux/lsm_hook_defs.h >>> +++ b/include/linux/lsm_hook_defs.h >>> @@ -404,4 +404,5 @@ 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_async_cmd, struct io_uring_cmd *ioucmd) >>> #endif /* CONFIG_IO_URING */ >>> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h >>> index 3bf5c658bc44..21b18cf138c2 100644 >>> --- a/include/linux/lsm_hooks.h >>> +++ b/include/linux/lsm_hooks.h >>> @@ -1569,6 +1569,9 @@ >>> * Check whether the current task is allowed to spawn a io_uring polling >>> * thread (IORING_SETUP_SQPOLL). >>> * >>> + * @uring_async_cmd: >>> + * Check whether the file_operations async_cmd is allowed to run. >>> + * >>> */ >>> union security_list_options { >>> #define LSM_HOOK(RET, DEFAULT, NAME, ...) RET (*NAME)(__VA_ARGS__); >>> diff --git a/include/linux/security.h b/include/linux/security.h >>> index 6d72772182c8..4d7f72813d75 100644 >>> --- a/include/linux/security.h >>> +++ b/include/linux/security.h >>> @@ -2041,6 +2041,7 @@ static inline int security_perf_event_write(struct perf_event *event) >>> #ifdef CONFIG_SECURITY >>> extern int security_uring_override_creds(const struct cred *new); >>> extern int security_uring_sqpoll(void); >>> +extern int security_uring_async_cmd(struct io_uring_cmd *ioucmd); >>> #else >>> static inline int security_uring_override_creds(const struct cred *new) >>> { >>> @@ -2050,6 +2051,10 @@ static inline int security_uring_sqpoll(void) >>> { >>> return 0; >>> } >>> +static inline int security_uring_async_cmd(struct io_uring_cmd *ioucmd) >>> +{ >>> + return 0; >>> +} >>> #endif /* CONFIG_SECURITY */ >>> #endif /* CONFIG_IO_URING */ >>> diff --git a/security/security.c b/security/security.c >>> index 22261d79f333..ef96be2f953a 100644 >>> --- a/security/security.c >>> +++ b/security/security.c >>> @@ -2640,4 +2640,8 @@ int security_uring_sqpoll(void) >>> { >>> return call_int_hook(uring_sqpoll, 0); >>> } >>> +int security_uring_async_cmd(struct io_uring_cmd *ioucmd) >>> +{ >>> + return call_int_hook(uring_async_cmd, 0, ioucmd); >> I don't have a good understanding of what information is in ioucmd. >> I am afraid that there may not be enough for a security module to >> make appropriate decisions in all cases. I am especially concerned >> about the modules that use path hooks, but based on the issues we've >> always had with ioctl and the like I fear for all cases. > As Paul pointed out, this particular LSM hook would not be needed if we can > somehow ensure users of the cmd path use their respective LSMs there. It > is not easy to force users to have the LSM hook to be used, one idea > might be to have a registration mechanism which allows users to also > specify the LSM hook, but these can vary in arguments, so perhaps then > what is needed is the LSM type in enum form, and internally we have a > mapping of these. That way we slowly itemize which cmds we *do* allow > for, thus vetting at the same time a respective LSM hook. Thoughts? tl;dr - Yuck. I don't see how your registration mechanism would be easier than getting "users of the cmd path" to use the LSM mechanism the way everyone else does. What it would do is pass responsibility for dealing with LSM to the io_uring core team. Experience has shown that dealing with the security issues after the fact is much harder than doing it up front, even when developers wail about the burden. Sure, LSM is an unpleasant interface/mechanism, but so is locking, and no one gets away without addressing that. My $0.02. > > Luis
On Mon, Mar 14, 2022 at 11:05:41AM -0700, Casey Schaufler wrote: > On 3/14/2022 9:32 AM, Luis Chamberlain wrote: > > On Mon, Mar 14, 2022 at 09:25:35AM -0700, Casey Schaufler wrote: > > > On 3/11/2022 9:11 AM, Luis Chamberlain wrote: > > > > On Thu, Mar 10, 2022 at 07:43:04PM -0700, Jens Axboe wrote: > > > > > On 3/10/22 6:51 PM, Luis Chamberlain wrote: > > > > > > On Tue, Mar 08, 2022 at 08:50:51PM +0530, Kanchan Joshi wrote: > > > > > > > From: Jens Axboe <axboe@kernel.dk> > > > > > > > > > > > > > > This is a file private kind of request. io_uring doesn't know what's > > > > > > > in this command type, it's for the file_operations->async_cmd() > > > > > > > handler to deal with. > > > > > > > > > > > > > > Signed-off-by: Jens Axboe <axboe@kernel.dk> > > > > > > > Signed-off-by: Kanchan Joshi <joshi.k@samsung.com> > > > > > > > --- > > > > > > <-- snip --> > > > > > > > > > > > > > +static int io_uring_cmd(struct io_kiocb *req, unsigned int issue_flags) > > > > > > > +{ > > > > > > > + struct file *file = req->file; > > > > > > > + int ret; > > > > > > > + struct io_uring_cmd *ioucmd = &req->uring_cmd; > > > > > > > + > > > > > > > + ioucmd->flags |= issue_flags; > > > > > > > + ret = file->f_op->async_cmd(ioucmd); > > > > > > I think we're going to have to add a security_file_async_cmd() check > > > > > > before this call here. Because otherwise we're enabling to, for > > > > > > example, bypass security_file_ioctl() for example using the new > > > > > > iouring-cmd interface. > > > > > > > > > > > > Or is this already thought out with the existing security_uring_*() stuff? > > > > > Unless the request sets .audit_skip, it'll be included already in terms > > > > > of logging. > > > > Neat. > > > > > > > > > But I'd prefer not to lodge this in with ioctls, unless > > > > > we're going to be doing actual ioctls. > > > > Oh sure, I have been an advocate to ensure folks don't conflate async_cmd > > > > with ioctl. However it *can* enable subsystems to enable ioctl > > > > passthrough, but each of those subsystems need to vet for this on their > > > > own terms. I'd hate to see / hear some LSM surprises later. > > > > > > > > > But definitely something to keep in mind and make sure that we're under > > > > > the right umbrella in terms of auditing and security. > > > > Paul, how about something like this for starters (and probably should > > > > be squashed into this series so its not a separate commit) ? > > > > > > > > >From f3ddbe822374cc1c7002bd795c1ae486d370cbd1 Mon Sep 17 00:00:00 2001 > > > > From: Luis Chamberlain <mcgrof@kernel.org> > > > > Date: Fri, 11 Mar 2022 08:55:50 -0800 > > > > Subject: [PATCH] lsm,io_uring: add LSM hooks to for the new async_cmd file op > > > > > > > > io-uring is extending the struct file_operations to allow a new > > > > command which each subsystem can use to enable command passthrough. > > > > Add an LSM specific for the command passthrough which enables LSMs > > > > to inspect the command details. > > > > > > > > Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> > > > > --- > > > > fs/io_uring.c | 5 +++++ > > > > include/linux/lsm_hook_defs.h | 1 + > > > > include/linux/lsm_hooks.h | 3 +++ > > > > include/linux/security.h | 5 +++++ > > > > security/security.c | 4 ++++ > > > > 5 files changed, 18 insertions(+) > > > > > > > > diff --git a/fs/io_uring.c b/fs/io_uring.c > > > > index 3f6eacc98e31..1c4e6b2cb61a 100644 > > > > --- a/fs/io_uring.c > > > > +++ b/fs/io_uring.c > > > > @@ -4190,6 +4190,11 @@ static int io_uring_cmd_prep(struct io_kiocb *req, > > > > struct io_ring_ctx *ctx = req->ctx; > > > > struct io_uring_cmd *ioucmd = &req->uring_cmd; > > > > u32 ucmd_flags = READ_ONCE(sqe->uring_cmd_flags); > > > > + int ret; > > > > + > > > > + ret = security_uring_async_cmd(ioucmd); > > > > + if (ret) > > > > + return ret; > > > > if (!req->file->f_op->async_cmd) > > > > return -EOPNOTSUPP; > > > > diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h > > > > index 819ec92dc2a8..4a20f8e6b295 100644 > > > > --- a/include/linux/lsm_hook_defs.h > > > > +++ b/include/linux/lsm_hook_defs.h > > > > @@ -404,4 +404,5 @@ 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_async_cmd, struct io_uring_cmd *ioucmd) > > > > #endif /* CONFIG_IO_URING */ > > > > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h > > > > index 3bf5c658bc44..21b18cf138c2 100644 > > > > --- a/include/linux/lsm_hooks.h > > > > +++ b/include/linux/lsm_hooks.h > > > > @@ -1569,6 +1569,9 @@ > > > > * Check whether the current task is allowed to spawn a io_uring polling > > > > * thread (IORING_SETUP_SQPOLL). > > > > * > > > > + * @uring_async_cmd: > > > > + * Check whether the file_operations async_cmd is allowed to run. > > > > + * > > > > */ > > > > union security_list_options { > > > > #define LSM_HOOK(RET, DEFAULT, NAME, ...) RET (*NAME)(__VA_ARGS__); > > > > diff --git a/include/linux/security.h b/include/linux/security.h > > > > index 6d72772182c8..4d7f72813d75 100644 > > > > --- a/include/linux/security.h > > > > +++ b/include/linux/security.h > > > > @@ -2041,6 +2041,7 @@ static inline int security_perf_event_write(struct perf_event *event) > > > > #ifdef CONFIG_SECURITY > > > > extern int security_uring_override_creds(const struct cred *new); > > > > extern int security_uring_sqpoll(void); > > > > +extern int security_uring_async_cmd(struct io_uring_cmd *ioucmd); > > > > #else > > > > static inline int security_uring_override_creds(const struct cred *new) > > > > { > > > > @@ -2050,6 +2051,10 @@ static inline int security_uring_sqpoll(void) > > > > { > > > > return 0; > > > > } > > > > +static inline int security_uring_async_cmd(struct io_uring_cmd *ioucmd) > > > > +{ > > > > + return 0; > > > > +} > > > > #endif /* CONFIG_SECURITY */ > > > > #endif /* CONFIG_IO_URING */ > > > > diff --git a/security/security.c b/security/security.c > > > > index 22261d79f333..ef96be2f953a 100644 > > > > --- a/security/security.c > > > > +++ b/security/security.c > > > > @@ -2640,4 +2640,8 @@ int security_uring_sqpoll(void) > > > > { > > > > return call_int_hook(uring_sqpoll, 0); > > > > } > > > > +int security_uring_async_cmd(struct io_uring_cmd *ioucmd) > > > > +{ > > > > + return call_int_hook(uring_async_cmd, 0, ioucmd); > > > I don't have a good understanding of what information is in ioucmd. > > > I am afraid that there may not be enough for a security module to > > > make appropriate decisions in all cases. I am especially concerned > > > about the modules that use path hooks, but based on the issues we've > > > always had with ioctl and the like I fear for all cases. > > As Paul pointed out, this particular LSM hook would not be needed if we can > > somehow ensure users of the cmd path use their respective LSMs there. It > > is not easy to force users to have the LSM hook to be used, one idea > > might be to have a registration mechanism which allows users to also > > specify the LSM hook, but these can vary in arguments, so perhaps then > > what is needed is the LSM type in enum form, and internally we have a > > mapping of these. That way we slowly itemize which cmds we *do* allow > > for, thus vetting at the same time a respective LSM hook. Thoughts? > > tl;dr - Yuck. > > I don't see how your registration mechanism would be easier than > getting "users of the cmd path" to use the LSM mechanism the way > everyone else does. What it would do is pass responsibility for > dealing with LSM to the io_uring core team. Agreed, I was just trying to be proactive to help with the LSM stuff. But indeed, that path would be complicated and I agree probably not the most practical one. > Experience has shown > that dealing with the security issues after the fact is much > harder than doing it up front, even when developers wail about > the burden. Sure, LSM is an unpleasant interface/mechanism, but > so is locking, and no one gets away without addressing that. > My $0.02. So putting the onus on those file_operations which embrace async_cmd to take into account LSMs seems to be the way to go then, which seems to align with what Paul was suggesting. Luis
diff --git a/fs/io_uring.c b/fs/io_uring.c index 241ba1cd6dcf..1f228a79e68f 100644 --- a/fs/io_uring.c +++ b/fs/io_uring.c @@ -200,13 +200,6 @@ struct io_rings { struct io_uring_cqe cqes[] ____cacheline_aligned_in_smp; }; -enum io_uring_cmd_flags { - IO_URING_F_COMPLETE_DEFER = 1, - IO_URING_F_UNLOCKED = 2, - /* int's last bit, sign checks are usually faster than a bit test */ - IO_URING_F_NONBLOCK = INT_MIN, -}; - struct io_mapped_ubuf { u64 ubuf; u64 ubuf_end; @@ -860,6 +853,7 @@ struct io_kiocb { struct io_mkdir mkdir; struct io_symlink symlink; struct io_hardlink hardlink; + struct io_uring_cmd uring_cmd; }; u8 opcode; @@ -1110,6 +1104,9 @@ static const struct io_op_def io_op_defs[] = { [IORING_OP_MKDIRAT] = {}, [IORING_OP_SYMLINKAT] = {}, [IORING_OP_LINKAT] = {}, + [IORING_OP_URING_CMD] = { + .needs_file = 1, + }, }; /* requests with any of those set should undergo io_disarm_next() */ @@ -2464,6 +2461,22 @@ static void io_req_task_submit(struct io_kiocb *req, bool *locked) io_req_complete_failed(req, -EFAULT); } +static void io_uring_cmd_work(struct io_kiocb *req, bool *locked) +{ + req->uring_cmd.driver_cb(&req->uring_cmd); +} + +void io_uring_cmd_complete_in_task(struct io_uring_cmd *ioucmd, + void (*driver_cb)(struct io_uring_cmd *)) +{ + struct io_kiocb *req = container_of(ioucmd, struct io_kiocb, uring_cmd); + + req->uring_cmd.driver_cb = driver_cb; + req->io_task_work.func = io_uring_cmd_work; + io_req_task_work_add(req, !!(req->ctx->flags & IORING_SETUP_SQPOLL)); +} +EXPORT_SYMBOL_GPL(io_uring_cmd_complete_in_task); + static void io_req_task_queue_fail(struct io_kiocb *req, int ret) { req->result = ret; @@ -4109,6 +4122,51 @@ static int io_linkat(struct io_kiocb *req, unsigned int issue_flags) return 0; } +/* + * Called by consumers of io_uring_cmd, if they originally returned + * -EIOCBQUEUED upon receiving the command. + */ +void io_uring_cmd_done(struct io_uring_cmd *ioucmd, ssize_t ret) +{ + struct io_kiocb *req = container_of(ioucmd, struct io_kiocb, uring_cmd); + + if (ret < 0) + req_set_fail(req); + io_req_complete(req, ret); +} +EXPORT_SYMBOL_GPL(io_uring_cmd_done); + +static int io_uring_cmd_prep(struct io_kiocb *req, + const struct io_uring_sqe *sqe) +{ + struct io_uring_cmd *ioucmd = &req->uring_cmd; + + if (!req->file->f_op->async_cmd || !(req->ctx->flags & IORING_SETUP_SQE128)) + return -EOPNOTSUPP; + if (req->ctx->flags & IORING_SETUP_IOPOLL) + return -EOPNOTSUPP; + ioucmd->cmd = (void *) &sqe->cmd; + ioucmd->cmd_op = READ_ONCE(sqe->cmd_op); + ioucmd->cmd_len = READ_ONCE(sqe->cmd_len); + ioucmd->flags = 0; + return 0; +} + +static int io_uring_cmd(struct io_kiocb *req, unsigned int issue_flags) +{ + struct file *file = req->file; + int ret; + struct io_uring_cmd *ioucmd = &req->uring_cmd; + + ioucmd->flags |= issue_flags; + ret = file->f_op->async_cmd(ioucmd); + /* queued async, consumer will call io_uring_cmd_done() when complete */ + if (ret == -EIOCBQUEUED) + return 0; + io_uring_cmd_done(ioucmd, ret); + return 0; +} + static int io_shutdown_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe) { @@ -6588,6 +6646,8 @@ static int io_req_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe) return io_symlinkat_prep(req, sqe); case IORING_OP_LINKAT: return io_linkat_prep(req, sqe); + case IORING_OP_URING_CMD: + return io_uring_cmd_prep(req, sqe); } printk_once(KERN_WARNING "io_uring: unhandled opcode %d\n", @@ -6871,6 +6931,9 @@ static int io_issue_sqe(struct io_kiocb *req, unsigned int issue_flags) case IORING_OP_LINKAT: ret = io_linkat(req, issue_flags); break; + case IORING_OP_URING_CMD: + ret = io_uring_cmd(req, issue_flags); + break; default: ret = -EINVAL; break; @@ -11215,6 +11278,8 @@ static int __init io_uring_init(void) BUILD_BUG_ON(ARRAY_SIZE(io_op_defs) != IORING_OP_LAST); BUILD_BUG_ON(__REQ_F_LAST_BIT > 8 * sizeof(int)); + BUILD_BUG_ON(sizeof(struct io_uring_cmd) > 64); + req_cachep = KMEM_CACHE(io_kiocb, SLAB_HWCACHE_ALIGN | SLAB_PANIC | SLAB_ACCOUNT); return 0; diff --git a/include/linux/io_uring.h b/include/linux/io_uring.h index 649a4d7c241b..cedc68201469 100644 --- a/include/linux/io_uring.h +++ b/include/linux/io_uring.h @@ -5,7 +5,29 @@ #include <linux/sched.h> #include <linux/xarray.h> +enum io_uring_cmd_flags { + IO_URING_F_COMPLETE_DEFER = 1, + IO_URING_F_UNLOCKED = 2, + /* int's last bit, sign checks are usually faster than a bit test */ + IO_URING_F_NONBLOCK = INT_MIN, +}; + +struct io_uring_cmd { + struct file *file; + void *cmd; + /* for irq-completion - if driver requires doing stuff in task-context*/ + void (*driver_cb)(struct io_uring_cmd *cmd); + u32 flags; + u32 cmd_op; + u16 cmd_len; + u16 unused; + u8 pdu[28]; /* available inline for free use */ +}; + #if defined(CONFIG_IO_URING) +void io_uring_cmd_done(struct io_uring_cmd *cmd, ssize_t ret); +void io_uring_cmd_complete_in_task(struct io_uring_cmd *ioucmd, + void (*driver_cb)(struct io_uring_cmd *)); struct sock *io_uring_get_socket(struct file *file); void __io_uring_cancel(bool cancel_all); void __io_uring_free(struct task_struct *tsk); @@ -26,6 +48,13 @@ static inline void io_uring_free(struct task_struct *tsk) __io_uring_free(tsk); } #else +static inline void io_uring_cmd_done(struct io_uring_cmd *cmd, ssize_t ret) +{ +} +static inline void io_uring_cmd_complete_in_task(struct io_uring_cmd *ioucmd, + void (*driver_cb)(struct io_uring_cmd *)) +{ +} static inline struct sock *io_uring_get_socket(struct file *file) { return NULL; diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h index c5db68433ca5..9bf1d6c0ed7f 100644 --- a/include/uapi/linux/io_uring.h +++ b/include/uapi/linux/io_uring.h @@ -22,10 +22,12 @@ struct io_uring_sqe { union { __u64 off; /* offset into file */ __u64 addr2; + __u32 cmd_op; }; union { __u64 addr; /* pointer to buffer or iovecs */ __u64 splice_off_in; + __u16 cmd_len; }; __u32 len; /* buffer size or number of iovecs */ union { @@ -60,8 +62,10 @@ struct io_uring_sqe { __s32 splice_fd_in; __u32 file_index; }; - __u64 __pad2[2]; - + union { + __u64 __pad2[2]; + __u64 cmd; + }; /* * If the ring is initializefd with IORING_SETUP_SQE128, then this field * contains 64-bytes of padding, doubling the size of the SQE. @@ -150,6 +154,7 @@ enum { IORING_OP_MKDIRAT, IORING_OP_SYMLINKAT, IORING_OP_LINKAT, + IORING_OP_URING_CMD, /* this goes last, obviously */ IORING_OP_LAST,