[PATCH] lsm,io_uring: add LSM hooks to for the new uring_cmd file op

[PATCH] lsm,io_uring: add LSM hooks to for the new uring_cmd file op

[Luis Chamberlain]

io-uring cmd support was added through ee692a21e9bf ("fs,io_uring:
add infrastructure for uring-cmd"), this extended 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.

This was discussed long ago without no clear pointer for something
conclusive, so this enables LSMs to at least reject this new file
operation.

[0] https://lkml.kernel.org/r/8adf55db-7bab-f59d-d612-ed906b948d19@schaufler-ca.com

Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 include/linux/lsm_hook_defs.h | 1 +
 include/linux/lsm_hooks.h     | 3 +++
 include/linux/security.h      | 5 +++++
 io_uring/uring_cmd.c          | 5 +++++
 security/security.c           | 4 ++++
 5 files changed, 18 insertions(+)

diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index eafa1d2489fd..4e94755098f1 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -406,4 +406,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_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 91c8146649f5..b681cfce6190 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -1575,6 +1575,9 @@
  *      Check whether the current task is allowed to spawn a io_uring polling
  *      thread (IORING_SETUP_SQPOLL).
  *
+ * @uring_cmd:
+ *      Check whether the file_operations uring_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 4d0baf30266e..421856919b1e 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -2053,6 +2053,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_cmd(struct io_uring_cmd *ioucmd);
 #else
 static inline int security_uring_override_creds(const struct cred *new)
 {
@@ -2062,6 +2063,10 @@ static inline int security_uring_sqpoll(void)
 {
 	return 0;
 }
+static inline int security_uring_cmd(struct io_uring_cmd *ioucmd)
+{
+	return 0;
+}
 #endif /* CONFIG_SECURITY */
 #endif /* CONFIG_IO_URING */
 
diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c
index 0a421ed51e7e..5e666aa7edb8 100644
--- a/io_uring/uring_cmd.c
+++ b/io_uring/uring_cmd.c
@@ -3,6 +3,7 @@
 #include <linux/errno.h>
 #include <linux/file.h>
 #include <linux/io_uring.h>
+#include <linux/security.h>
 
 #include <uapi/linux/io_uring.h>
 
@@ -82,6 +83,10 @@ int io_uring_cmd(struct io_kiocb *req, unsigned int issue_flags)
 	struct file *file = req->file;
 	int ret;
 
+	ret = security_uring_cmd(ioucmd);
+	if (ret)
+		return ret;
+
 	if (!req->file->f_op->uring_cmd)
 		return -EOPNOTSUPP;
 
diff --git a/security/security.c b/security/security.c
index f85afb02ea1c..ad7d7229bd72 100644
--- a/security/security.c
+++ b/security/security.c
@@ -2655,4 +2655,8 @@ int security_uring_sqpoll(void)
 {
 	return call_int_hook(uring_sqpoll, 0);
 }
+int security_uring_cmd(struct io_uring_cmd *ioucmd)
+{
+	return call_int_hook(uring_cmd, 0, ioucmd);
+}
 #endif /* CONFIG_IO_URING */
-- 
2.35.1

Re: [PATCH] lsm,io_uring: add LSM hooks to for the new uring_cmd file op

[Casey Schaufler]

On 7/13/2022 5:05 PM, Luis Chamberlain wrote:
> io-uring cmd support was added through ee692a21e9bf ("fs,io_uring:
> add infrastructure for uring-cmd"), this extended 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.
>
> This was discussed long ago without no clear pointer for something
> conclusive, so this enables LSMs to at least reject this new file
> operation.

tl;dr - Yuck. Again.

You're passing the complexity of uring-cmd directly into each
and every security module. SELinux, AppArmor, Smack, BPF and
every other LSM now needs to know the gory details of everything
that might be in any arbitrary subsystem so that it can make a
wild guess about what to do. And I thought ioctl was hard to deal
with.

Look at what Paul Moore did for the existing io_uring code.
Carry that forward into your passthrough implementation.
No, I don't think that waving security away because we haven't
proposed a fix for your flawed design is acceptable. Sure, we
can help.

>
> [0] https://lkml.kernel.org/r/8adf55db-7bab-f59d-d612-ed906b948d19@schaufler-ca.com
>
> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
> ---
>  include/linux/lsm_hook_defs.h | 1 +
>  include/linux/lsm_hooks.h     | 3 +++
>  include/linux/security.h      | 5 +++++
>  io_uring/uring_cmd.c          | 5 +++++
>  security/security.c           | 4 ++++
>  5 files changed, 18 insertions(+)
>
> diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
> index eafa1d2489fd..4e94755098f1 100644
> --- a/include/linux/lsm_hook_defs.h
> +++ b/include/linux/lsm_hook_defs.h
> @@ -406,4 +406,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_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 91c8146649f5..b681cfce6190 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -1575,6 +1575,9 @@
>   *      Check whether the current task is allowed to spawn a io_uring polling
>   *      thread (IORING_SETUP_SQPOLL).
>   *
> + * @uring_cmd:
> + *      Check whether the file_operations uring_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 4d0baf30266e..421856919b1e 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -2053,6 +2053,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_cmd(struct io_uring_cmd *ioucmd);
>  #else
>  static inline int security_uring_override_creds(const struct cred *new)
>  {
> @@ -2062,6 +2063,10 @@ static inline int security_uring_sqpoll(void)
>  {
>  	return 0;
>  }
> +static inline int security_uring_cmd(struct io_uring_cmd *ioucmd)
> +{
> +	return 0;
> +}
>  #endif /* CONFIG_SECURITY */
>  #endif /* CONFIG_IO_URING */
>  
> diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c
> index 0a421ed51e7e..5e666aa7edb8 100644
> --- a/io_uring/uring_cmd.c
> +++ b/io_uring/uring_cmd.c
> @@ -3,6 +3,7 @@
>  #include <linux/errno.h>
>  #include <linux/file.h>
>  #include <linux/io_uring.h>
> +#include <linux/security.h>
>  
>  #include <uapi/linux/io_uring.h>
>  
> @@ -82,6 +83,10 @@ int io_uring_cmd(struct io_kiocb *req, unsigned int issue_flags)
>  	struct file *file = req->file;
>  	int ret;
>  
> +	ret = security_uring_cmd(ioucmd);
> +	if (ret)
> +		return ret;
> +
>  	if (!req->file->f_op->uring_cmd)
>  		return -EOPNOTSUPP;
>  
> diff --git a/security/security.c b/security/security.c
> index f85afb02ea1c..ad7d7229bd72 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -2655,4 +2655,8 @@ int security_uring_sqpoll(void)
>  {
>  	return call_int_hook(uring_sqpoll, 0);
>  }
> +int security_uring_cmd(struct io_uring_cmd *ioucmd)
> +{
> +	return call_int_hook(uring_cmd, 0, ioucmd);
> +}
>  #endif /* CONFIG_IO_URING */

Re: [PATCH] lsm,io_uring: add LSM hooks to for the new uring_cmd file op

[Paul Moore]

On Wed, Jul 13, 2022 at 8:05 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
>
> io-uring cmd support was added through ee692a21e9bf ("fs,io_uring:
> add infrastructure for uring-cmd"), this extended 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.
>
> This was discussed long ago without no clear pointer for something
> conclusive, so this enables LSMs to at least reject this new file
> operation.
>
> [0] https://lkml.kernel.org/r/8adf55db-7bab-f59d-d612-ed906b948d19@schaufler-ca.com

[NOTE: I now see that the IORING_OP_URING_CMD has made it into the
v5.19-rcX releases, I'm going to be honest and say that I'm
disappointed you didn't post the related LSM additions until
v5.19-rc6, especially given our earlier discussions.]

While the earlier discussion may not have offered a detailed approach
on how to solve this, I think it was rather conclusive in that the
approach used then (and reproduced here) did not provide enough
context to the LSMs to be able to make a decision.  There were similar
concerns when it came to auditing the command passthrough.  It appears
that most of my concerns in the original thread still apply to this
patch.

Given the LSM hook in this patch, it is very difficult (impossible?)
to determine the requested operation as these command opcodes are
device/subsystem specific.  The unfortunate result is that the LSMs
are likely going to either allow all, or none, of the commands for a
given device/subsystem, and I think we can all agree that is not a
good idea.

That is the critical bit of feedback on this patch, but there is more
feedback inline below.

> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
> ---
>  include/linux/lsm_hook_defs.h | 1 +
>  include/linux/lsm_hooks.h     | 3 +++
>  include/linux/security.h      | 5 +++++
>  io_uring/uring_cmd.c          | 5 +++++
>  security/security.c           | 4 ++++
>  5 files changed, 18 insertions(+)

...

> diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c
> index 0a421ed51e7e..5e666aa7edb8 100644
> --- a/io_uring/uring_cmd.c
> +++ b/io_uring/uring_cmd.c
> @@ -3,6 +3,7 @@
>  #include <linux/errno.h>
>  #include <linux/file.h>
>  #include <linux/io_uring.h>
> +#include <linux/security.h>
>
>  #include <uapi/linux/io_uring.h>
>
> @@ -82,6 +83,10 @@ int io_uring_cmd(struct io_kiocb *req, unsigned int issue_flags)
>         struct file *file = req->file;
>         int ret;
>
> +       ret = security_uring_cmd(ioucmd);
> +       if (ret)
> +               return ret;
> +
>         if (!req->file->f_op->uring_cmd)
>                 return -EOPNOTSUPP;
>

In order to be consistent with most of the other LSM hooks, the
'req->file->f_op->uring_cmd' check should come before the LSM hook
call.  The general approach used in most places is to first validate
the request and do any DAC based access checks before calling into the
LSM.

-- 
paul-moore.com

Re: [PATCH] lsm,io_uring: add LSM hooks to for the new uring_cmd file op

[Luis Chamberlain]

On Wed, Jul 13, 2022 at 05:38:42PM -0700, Casey Schaufler wrote:
> On 7/13/2022 5:05 PM, Luis Chamberlain wrote:
> > io-uring cmd support was added through ee692a21e9bf ("fs,io_uring:
> > add infrastructure for uring-cmd"), this extended 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.
> >
> > This was discussed long ago without no clear pointer for something
> > conclusive, so this enables LSMs to at least reject this new file
> > operation.
> 
> tl;dr - Yuck. Again.
> 
> You're passing the complexity of uring-cmd directly into each
> and every security module. SELinux, AppArmor, Smack, BPF and
> every other LSM now needs to know the gory details of everything
> that might be in any arbitrary subsystem so that it can make a
> wild guess about what to do. And I thought ioctl was hard to deal
> with.

Yes... I cannot agree anymore.

> Look at what Paul Moore did for the existing io_uring code.
> Carry that forward into your passthrough implementation.

Which one in particular? I didn't see any glaring obvious answers.

> No, I don't think that waving security away because we haven't
> proposed a fix for your flawed design is acceptable. Sure, we
> can help.

Hey if the answer was obvious it would have been implemented.

  Luis

Re: [PATCH] lsm,io_uring: add LSM hooks to for the new uring_cmd file op

[Luis Chamberlain]

On Wed, Jul 13, 2022 at 11:00:42PM -0400, Paul Moore wrote:
> On Wed, Jul 13, 2022 at 8:05 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
> >
> > io-uring cmd support was added through ee692a21e9bf ("fs,io_uring:
> > add infrastructure for uring-cmd"), this extended 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.
> >
> > This was discussed long ago without no clear pointer for something
> > conclusive, so this enables LSMs to at least reject this new file
> > operation.
> >
> > [0] https://lkml.kernel.org/r/8adf55db-7bab-f59d-d612-ed906b948d19@schaufler-ca.com
> 
> [NOTE: I now see that the IORING_OP_URING_CMD has made it into the
> v5.19-rcX releases, I'm going to be honest and say that I'm
> disappointed you didn't post the related LSM additions 

It does not mean I didn't ask for them too.

> until
> v5.19-rc6, especially given our earlier discussions.]

And hence since I don't see it either, it's on us now.

As important as I think LSMs are, I cannot convince everyone
to take them as serious as I do.

> While the earlier discussion may not have offered a detailed approach
> on how to solve this, I think it was rather conclusive in that the
> approach used then (and reproduced here) did not provide enough
> context to the LSMs to be able to make a decision.

Right...

> There were similar
> concerns when it came to auditing the command passthrough.  It appears
> that most of my concerns in the original thread still apply to this
> patch.
> 
> Given the LSM hook in this patch, it is very difficult (impossible?)
> to determine the requested operation as these command opcodes are
> device/subsystem specific.  The unfortunate result is that the LSMs
> are likely going to either allow all, or none, of the commands for a
> given device/subsystem, and I think we can all agree that is not a
> good idea.
> 
> That is the critical bit of feedback on this patch, but there is more
> feedback inline below.

Given a clear solution is not easily tangible at this point
I was hoping perhaps at least the abilility to enable LSMs to
reject uring-cmd would be better than nothing at this point.

> > Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
> > ---
> >  include/linux/lsm_hook_defs.h | 1 +
> >  include/linux/lsm_hooks.h     | 3 +++
> >  include/linux/security.h      | 5 +++++
> >  io_uring/uring_cmd.c          | 5 +++++
> >  security/security.c           | 4 ++++
> >  5 files changed, 18 insertions(+)
> 
> ...
> 
> > diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c
> > index 0a421ed51e7e..5e666aa7edb8 100644
> > --- a/io_uring/uring_cmd.c
> > +++ b/io_uring/uring_cmd.c
> > @@ -3,6 +3,7 @@
> >  #include <linux/errno.h>
> >  #include <linux/file.h>
> >  #include <linux/io_uring.h>
> > +#include <linux/security.h>
> >
> >  #include <uapi/linux/io_uring.h>
> >
> > @@ -82,6 +83,10 @@ int io_uring_cmd(struct io_kiocb *req, unsigned int issue_flags)
> >         struct file *file = req->file;
> >         int ret;
> >
> > +       ret = security_uring_cmd(ioucmd);
> > +       if (ret)
> > +               return ret;
> > +
> >         if (!req->file->f_op->uring_cmd)
> >                 return -EOPNOTSUPP;
> >
> 
> In order to be consistent with most of the other LSM hooks, the
> 'req->file->f_op->uring_cmd' check should come before the LSM hook
> call. 

Sure.

> The general approach used in most places is to first validate
> the request and do any DAC based access checks before calling into the
> LSM.

OK.

Let me know how you'd like to proceed given our current status.

  Luis

Re: [PATCH] lsm,io_uring: add LSM hooks to for the new uring_cmd file op

[Casey Schaufler]

On 7/14/2022 5:54 PM, Luis Chamberlain wrote:
> On Wed, Jul 13, 2022 at 05:38:42PM -0700, Casey Schaufler wrote:
>> On 7/13/2022 5:05 PM, Luis Chamberlain wrote:
>>> io-uring cmd support was added through ee692a21e9bf ("fs,io_uring:
>>> add infrastructure for uring-cmd"), this extended 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.
>>>
>>> This was discussed long ago without no clear pointer for something
>>> conclusive, so this enables LSMs to at least reject this new file
>>> operation.
>> tl;dr - Yuck. Again.
>>
>> You're passing the complexity of uring-cmd directly into each
>> and every security module. SELinux, AppArmor, Smack, BPF and
>> every other LSM now needs to know the gory details of everything
>> that might be in any arbitrary subsystem so that it can make a
>> wild guess about what to do. And I thought ioctl was hard to deal
>> with.
> Yes... I cannot agree anymore.
>
>> Look at what Paul Moore did for the existing io_uring code.
>> Carry that forward into your passthrough implementation.
> Which one in particular? I didn't see any glaring obvious answers.

Neither did I! I'm still playing catch-up on the initial io_uring
implementation. Smack's "Brutalist" support for io_uring isn't
especially satisfactory, and adding arbitrary sub-system defined
command behavior on top of what's already pretty mysterious
isn't going to make it any easier.

>
>> No, I don't think that waving security away because we haven't
>> proposed a fix for your flawed design is acceptable. Sure, we
>> can help.
> Hey if the answer was obvious it would have been implemented.

And it still needs to be implemented, even if it isn't obvious.
In the security world we don't get to say "Sure, the performance
sucks, but the optimization folks will take care of that later."
Throwing in a nebulous security hook and counting on the LSMs to
make rainbows out of it isn't going to fly either.

>
>   Luis

Re: [PATCH] lsm,io_uring: add LSM hooks to for the new uring_cmd file op

[Paul Moore]

On Thu, Jul 14, 2022 at 9:00 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
> On Wed, Jul 13, 2022 at 11:00:42PM -0400, Paul Moore wrote:
> > On Wed, Jul 13, 2022 at 8:05 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
> > >
> > > io-uring cmd support was added through ee692a21e9bf ("fs,io_uring:
> > > add infrastructure for uring-cmd"), this extended 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.
> > >
> > > This was discussed long ago without no clear pointer for something
> > > conclusive, so this enables LSMs to at least reject this new file
> > > operation.
> > >
> > > [0] https://lkml.kernel.org/r/8adf55db-7bab-f59d-d612-ed906b948d19@schaufler-ca.com
> >
> > [NOTE: I now see that the IORING_OP_URING_CMD has made it into the
> > v5.19-rcX releases, I'm going to be honest and say that I'm
> > disappointed you didn't post the related LSM additions
>
> It does not mean I didn't ask for them too.
>
> > until
> > v5.19-rc6, especially given our earlier discussions.]
>
> And hence since I don't see it either, it's on us now.

It looks like I owe you an apology, Luis.  While my frustration over
io_uring remains, along with my disappointment that the io_uring
developers continue to avoid discussing access controls with the LSM
community, you are not the author of the IORING_OP_URING_CMD.   You
are simply trying to do the right thing by adding the necessary LSM
controls and in my confusion I likely caused you a bit of frustration;
I'm sorry for that.

> As important as I think LSMs are, I cannot convince everyone
> to take them as serious as I do.

Yes, I think a lot of us are familiar with that feeling unfortunately :/

> > While the earlier discussion may not have offered a detailed approach
> > on how to solve this, I think it was rather conclusive in that the
> > approach used then (and reproduced here) did not provide enough
> > context to the LSMs to be able to make a decision.
>
> Right...
>
> > There were similar
> > concerns when it came to auditing the command passthrough.  It appears
> > that most of my concerns in the original thread still apply to this
> > patch.
> >
> > Given the LSM hook in this patch, it is very difficult (impossible?)
> > to determine the requested operation as these command opcodes are
> > device/subsystem specific.  The unfortunate result is that the LSMs
> > are likely going to either allow all, or none, of the commands for a
> > given device/subsystem, and I think we can all agree that is not a
> > good idea.
> >
> > That is the critical bit of feedback on this patch, but there is more
> > feedback inline below.
>
> Given a clear solution is not easily tangible at this point
> I was hoping perhaps at least the abilility to enable LSMs to
> reject uring-cmd would be better than nothing at this point.

Without any cooperation from the io_uring developers, that is likely
what we will have to do.  I know there was a lot of talk about this
functionality not being like another ioctl(), but from a LSM
perspective I think that is how we will need to treat it.

> > > Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
> > > ---
> > >  include/linux/lsm_hook_defs.h | 1 +
> > >  include/linux/lsm_hooks.h     | 3 +++
> > >  include/linux/security.h      | 5 +++++
> > >  io_uring/uring_cmd.c          | 5 +++++
> > >  security/security.c           | 4 ++++
> > >  5 files changed, 18 insertions(+)
> >
> > ...
> >
> > > diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c
> > > index 0a421ed51e7e..5e666aa7edb8 100644
> > > --- a/io_uring/uring_cmd.c
> > > +++ b/io_uring/uring_cmd.c
> > > @@ -3,6 +3,7 @@
> > >  #include <linux/errno.h>
> > >  #include <linux/file.h>
> > >  #include <linux/io_uring.h>
> > > +#include <linux/security.h>
> > >
> > >  #include <uapi/linux/io_uring.h>
> > >
> > > @@ -82,6 +83,10 @@ int io_uring_cmd(struct io_kiocb *req, unsigned int issue_flags)
> > >         struct file *file = req->file;
> > >         int ret;
> > >
> > > +       ret = security_uring_cmd(ioucmd);
> > > +       if (ret)
> > > +               return ret;
> > > +
> > >         if (!req->file->f_op->uring_cmd)
> > >                 return -EOPNOTSUPP;
> > >
> >
> > In order to be consistent with most of the other LSM hooks, the
> > 'req->file->f_op->uring_cmd' check should come before the LSM hook
> > call.
>
> Sure.
>
> > The general approach used in most places is to first validate
> > the request and do any DAC based access checks before calling into the
> > LSM.
>
> OK.
>
> Let me know how you'd like to proceed given our current status.

Well, we're at -rc6 right now which means IORING_OP_URING_CMD is
happening and it's unlikely the LSM folks are going to be able to
influence the design/implementation much at this point so we have to
do the best we can.  Given the existing constraints, I think your
patch is reasonable (although please do shift the hook call site down
a bit as discussed above), we just need to develop the LSM
implementations to go along with it.

Luis, can you respin and resend the patch with the requested changes?

Casey, it looks like Smack and SELinux are the only LSMs to implement
io_uring access controls.  Given the hook that Luis developed in this
patch, could you draft a patch for Smack to add the necessary checks?
I'll do the same for SELinux.  My initial thinking is that all we can
really do is check the access between the creds on the current task
(any overrides will have already taken place by the time the LSM hook
is called) with the io_uring_cmd:file label/creds; we won't be able to
provide much permission granularity for all the reasons previously
discussed, but I suspect that will be more of a SELinux problem than a
Smack problem (although I suspect Smack will need to treat this as
both a read and a write, which is likely less than ideal).

I think it's doubtful we will have all of this ready and tested in
time for v5.19, but I think we can have it ready shortly after that
and I'll mark all of the patches for -stable when I send them to
Linus.

I also think we should mark the patches with a 'Fixes:' line that
points at the IORING_OP_URING_CMD commit, ee692a21e9bf ("fs,io_uring:
add infrastructure for uring-cmd").

How does that sound to everyone?

-- 
paul-moore.com

Re: [PATCH] lsm,io_uring: add LSM hooks to for the new uring_cmd file op

[Luis Chamberlain]

On Fri, Jul 15, 2022 at 02:46:16PM -0400, Paul Moore wrote:
> It looks like I owe you an apology, Luis.  While my frustration over
> io_uring remains, along with my disappointment that the io_uring
> developers continue to avoid discussing access controls with the LSM
> community, you are not the author of the IORING_OP_URING_CMD.   You
> are simply trying to do the right thing by adding the necessary LSM
> controls and in my confusion I likely caused you a bit of frustration;
> I'm sorry for that.

No frustration caused, I get it.

> Well, we're at -rc6 right now which means IORING_OP_URING_CMD is
> happening and it's unlikely the LSM folks are going to be able to
> influence the design/implementation much at this point so we have to
> do the best we can.  Given the existing constraints, I think your
> patch is reasonable (although please do shift the hook call site down
> a bit as discussed above), we just need to develop the LSM
> implementations to go along with it.
> 
> Luis, can you respin and resend the patch with the requested changes?

Sure thing.

> I also think we should mark the patches with a 'Fixes:' line that
> points at the IORING_OP_URING_CMD commit, ee692a21e9bf ("fs,io_uring:
> add infrastructure for uring-cmd").

I'll do that.

  Luis

Re: [PATCH] lsm,io_uring: add LSM hooks to for the new uring_cmd file op

[Jens Axboe]

On 7/15/22 12:46 PM, Paul Moore wrote:
> On Thu, Jul 14, 2022 at 9:00 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
>> On Wed, Jul 13, 2022 at 11:00:42PM -0400, Paul Moore wrote:
>>> On Wed, Jul 13, 2022 at 8:05 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
>>>>
>>>> io-uring cmd support was added through ee692a21e9bf ("fs,io_uring:
>>>> add infrastructure for uring-cmd"), this extended 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.
>>>>
>>>> This was discussed long ago without no clear pointer for something
>>>> conclusive, so this enables LSMs to at least reject this new file
>>>> operation.
>>>>
>>>> [0] https://lkml.kernel.org/r/8adf55db-7bab-f59d-d612-ed906b948d19@schaufler-ca.com
>>>
>>> [NOTE: I now see that the IORING_OP_URING_CMD has made it into the
>>> v5.19-rcX releases, I'm going to be honest and say that I'm
>>> disappointed you didn't post the related LSM additions
>>
>> It does not mean I didn't ask for them too.
>>
>>> until
>>> v5.19-rc6, especially given our earlier discussions.]
>>
>> And hence since I don't see it either, it's on us now.
> 
> It looks like I owe you an apology, Luis.  While my frustration over
> io_uring remains, along with my disappointment that the io_uring
> developers continue to avoid discussing access controls with the LSM
> community, you are not the author of the IORING_OP_URING_CMD.   You
> are simply trying to do the right thing by adding the necessary LSM
> controls and in my confusion I likely caused you a bit of frustration;
> I'm sorry for that.

Maybe, just maybe, outbursts like this are why there's not a lot of
incentive to collaborate on this? I get why it can seem frustrating and
that you are being ignored, but I think it's more likely that people
just don't think of adding these hooks. I don't use any of the access
controls, nor do I really have a good idea which one exists and what
they do. None of the external developers or internal use cases we have
use any of this, and nobody outside of the developers of these kernel
features have ever brought it up...

I don't mind getting these added, but since I wasn't really part of
driving this particular feature, it wasn't on my radar.

>> Given a clear solution is not easily tangible at this point
>> I was hoping perhaps at least the abilility to enable LSMs to
>> reject uring-cmd would be better than nothing at this point.
> 
> Without any cooperation from the io_uring developers, that is likely
> what we will have to do.  I know there was a lot of talk about this
> functionality not being like another ioctl(), but from a LSM
> perspective I think that is how we will need to treat it.

Again this perceived ill intent. What are you looking for here?

>>>> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
 
> Well, we're at -rc6 right now which means IORING_OP_URING_CMD is
> happening and it's unlikely the LSM folks are going to be able to
> influence the design/implementation much at this point so we have to
> do the best we can.  Given the existing constraints, I think your
> patch is reasonable (although please do shift the hook call site down
> a bit as discussed above), we just need to develop the LSM
> implementations to go along with it.
> 
> Luis, can you respin and resend the patch with the requested changes?
> 
> Casey, it looks like Smack and SELinux are the only LSMs to implement
> io_uring access controls.  Given the hook that Luis developed in this
> patch, could you draft a patch for Smack to add the necessary checks?
> I'll do the same for SELinux.  My initial thinking is that all we can
> really do is check the access between the creds on the current task
> (any overrides will have already taken place by the time the LSM hook
> is called) with the io_uring_cmd:file label/creds; we won't be able to
> provide much permission granularity for all the reasons previously
> discussed, but I suspect that will be more of a SELinux problem than a
> Smack problem (although I suspect Smack will need to treat this as
> both a read and a write, which is likely less than ideal).
> 
> I think it's doubtful we will have all of this ready and tested in
> time for v5.19, but I think we can have it ready shortly after that
> and I'll mark all of the patches for -stable when I send them to
> Linus.
> 
> I also think we should mark the patches with a 'Fixes:' line that
> points at the IORING_OP_URING_CMD commit, ee692a21e9bf ("fs,io_uring:
> add infrastructure for uring-cmd").
> 
> How does that sound to everyone?

Let's do it the right way for 5.20, and then get it marked for a
backport. That will be trivial enough and will hit 5.19-stable shortly
as well. Rushing it now with 1 week before release will most likely
yield a worse long term result.

-- 
Jens Axboe

Re: [PATCH] lsm,io_uring: add LSM hooks to for the new uring_cmd file op

[Paul Moore]

On Fri, Jul 15, 2022 at 3:07 PM Jens Axboe <axboe@kernel.dk> wrote:
> On 7/15/22 12:46 PM, Paul Moore wrote:
> > On Thu, Jul 14, 2022 at 9:00 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
> >> On Wed, Jul 13, 2022 at 11:00:42PM -0400, Paul Moore wrote:
> >>> On Wed, Jul 13, 2022 at 8:05 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
> >>>>
> >>>> io-uring cmd support was added through ee692a21e9bf ("fs,io_uring:
> >>>> add infrastructure for uring-cmd"), this extended 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.
> >>>>
> >>>> This was discussed long ago without no clear pointer for something
> >>>> conclusive, so this enables LSMs to at least reject this new file
> >>>> operation.
> >>>>
> >>>> [0] https://lkml.kernel.org/r/8adf55db-7bab-f59d-d612-ed906b948d19@schaufler-ca.com
> >>>
> >>> [NOTE: I now see that the IORING_OP_URING_CMD has made it into the
> >>> v5.19-rcX releases, I'm going to be honest and say that I'm
> >>> disappointed you didn't post the related LSM additions
> >>
> >> It does not mean I didn't ask for them too.
> >>
> >>> until
> >>> v5.19-rc6, especially given our earlier discussions.]
> >>
> >> And hence since I don't see it either, it's on us now.
> >
> > It looks like I owe you an apology, Luis.  While my frustration over
> > io_uring remains, along with my disappointment that the io_uring
> > developers continue to avoid discussing access controls with the LSM
> > community, you are not the author of the IORING_OP_URING_CMD.   You
> > are simply trying to do the right thing by adding the necessary LSM
> > controls and in my confusion I likely caused you a bit of frustration;
> > I'm sorry for that.
>
> Maybe, just maybe, outbursts like this are why there's not a lot of
> incentive to collaborate on this? I get why it can seem frustrating and
> that you are being ignored, but I think it's more likely that people
> just don't think of adding these hooks. I don't use any of the access
> controls, nor do I really have a good idea which one exists and what
> they do. None of the external developers or internal use cases we have
> use any of this, and nobody outside of the developers of these kernel
> features have ever brought it up...

While my response may have been misdirected (once again, sorry Luis),
I feel that expressing frustration about the LSMs being routinely left
out of the discussion when new functionality is added to the kernel is
a reasonable response; especially when one considers the history of
this particular situation.  I was willing to attribute the initial
LSM/audit omission in io_uring to an honest oversight, and the fact
that we were able to work together to get something in place was a
good thing which gave me some hope.  However, the issue around
IORING_OP_URING_CMD was brought up earlier this year and many of us on
the LSM side expressed concern, only to see the code present in
v5.19-rcX with little heads-up given outside of Luis' patch a few days
ago.  You can call my comments an outburst if you like, but it seems
like an appropriate reaction in this case.

> I don't mind getting these added, but since I wasn't really part of
> driving this particular feature, it wasn't on my radar.

I generally don't care who authors a commit, it's that code itself
that matters, not who wrote it.  However, since you mentioned it I
went back to check, and it looks like you authored the basic
IORING_OP_URING_CMD infrastructure according to ee692a21e9bf
("fs,io_uring: add infrastructure for uring-cmd"); that seems like a
decent level of awareness to me.

> >> Given a clear solution is not easily tangible at this point
> >> I was hoping perhaps at least the abilility to enable LSMs to
> >> reject uring-cmd would be better than nothing at this point.
> >
> > Without any cooperation from the io_uring developers, that is likely
> > what we will have to do.  I know there was a lot of talk about this
> > functionality not being like another ioctl(), but from a LSM
> > perspective I think that is how we will need to treat it.
>
> Again this perceived ill intent. What are you looking for here?

We expressed concern earlier this year and were largely ignored, and
when the functionality was merged into mainline the LSM community was
not notified despite our previous comments.  Perhaps there is no ill
intent on the side of io_uring, but from my perspective it sure seems
like there was an effort to avoid the LSM community.

As far as what I'm looking for, I think basic consideration for
comments coming from the LSM community would be a good start.  We
obviously have had some success in the past with this, which is why
I'm a bit shocked that our IORING_OP_URING_CMD comments from earlier
this year appeared to fall on deaf ears.

> >>>> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
>
> > Well, we're at -rc6 right now which means IORING_OP_URING_CMD is
> > happening and it's unlikely the LSM folks are going to be able to
> > influence the design/implementation much at this point so we have to
> > do the best we can.  Given the existing constraints, I think your
> > patch is reasonable (although please do shift the hook call site down
> > a bit as discussed above), we just need to develop the LSM
> > implementations to go along with it.
> >
> > Luis, can you respin and resend the patch with the requested changes?
> >
> > Casey, it looks like Smack and SELinux are the only LSMs to implement
> > io_uring access controls.  Given the hook that Luis developed in this
> > patch, could you draft a patch for Smack to add the necessary checks?
> > I'll do the same for SELinux.  My initial thinking is that all we can
> > really do is check the access between the creds on the current task
> > (any overrides will have already taken place by the time the LSM hook
> > is called) with the io_uring_cmd:file label/creds; we won't be able to
> > provide much permission granularity for all the reasons previously
> > discussed, but I suspect that will be more of a SELinux problem than a
> > Smack problem (although I suspect Smack will need to treat this as
> > both a read and a write, which is likely less than ideal).
> >
> > I think it's doubtful we will have all of this ready and tested in
> > time for v5.19, but I think we can have it ready shortly after that
> > and I'll mark all of the patches for -stable when I send them to
> > Linus.
> >
> > I also think we should mark the patches with a 'Fixes:' line that
> > points at the IORING_OP_URING_CMD commit, ee692a21e9bf ("fs,io_uring:
> > add infrastructure for uring-cmd").
> >
> > How does that sound to everyone?
>
> Let's do it the right way for 5.20, and then get it marked for a
> backport. That will be trivial enough and will hit 5.19-stable shortly
> as well. Rushing it now with 1 week before release will most likely
> yield a worse long term result.

That is what I suggested above; it looks like we are on the same page
at least with the resolution.  I'll plan on bundling Luis' hook patch,
Casey's Smack patch, the SELinux patch and send them up to Linus once
they are ready.  If you, and/or other io_uring developers, could
review Luis' LSM hook patch from an io_uring perspective and add your
Ack/Review-by tag I would appreciate it.

-- 
paul-moore.com

Re: [PATCH] lsm,io_uring: add LSM hooks to for the new uring_cmd file op

[Paul Moore]

On Fri, Jul 15, 2022 at 3:02 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
> On Fri, Jul 15, 2022 at 02:46:16PM -0400, Paul Moore wrote:
> > It looks like I owe you an apology, Luis.  While my frustration over
> > io_uring remains, along with my disappointment that the io_uring
> > developers continue to avoid discussing access controls with the LSM
> > community, you are not the author of the IORING_OP_URING_CMD.   You
> > are simply trying to do the right thing by adding the necessary LSM
> > controls and in my confusion I likely caused you a bit of frustration;
> > I'm sorry for that.
>
> No frustration caused, I get it.

Thanks for your understanding, I appreciate it as well as your help in
this area.

> > Well, we're at -rc6 right now which means IORING_OP_URING_CMD is
> > happening and it's unlikely the LSM folks are going to be able to
> > influence the design/implementation much at this point so we have to
> > do the best we can.  Given the existing constraints, I think your
> > patch is reasonable (although please do shift the hook call site down
> > a bit as discussed above), we just need to develop the LSM
> > implementations to go along with it.
> >
> > Luis, can you respin and resend the patch with the requested changes?
>
> Sure thing.
>
> > I also think we should mark the patches with a 'Fixes:' line that
> > points at the IORING_OP_URING_CMD commit, ee692a21e9bf ("fs,io_uring:
> > add infrastructure for uring-cmd").
>
> I'll do that.

Great, thanks again for the help.

-- 
paul-moore.com

Re: [PATCH] lsm,io_uring: add LSM hooks to for the new uring_cmd file op

[Jens Axboe]

On 7/15/22 1:50 PM, Paul Moore wrote:
> On Fri, Jul 15, 2022 at 3:07 PM Jens Axboe <axboe@kernel.dk> wrote:
>> On 7/15/22 12:46 PM, Paul Moore wrote:
>>> On Thu, Jul 14, 2022 at 9:00 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
>>>> On Wed, Jul 13, 2022 at 11:00:42PM -0400, Paul Moore wrote:
>>>>> On Wed, Jul 13, 2022 at 8:05 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
>>>>>>
>>>>>> io-uring cmd support was added through ee692a21e9bf ("fs,io_uring:
>>>>>> add infrastructure for uring-cmd"), this extended 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.
>>>>>>
>>>>>> This was discussed long ago without no clear pointer for something
>>>>>> conclusive, so this enables LSMs to at least reject this new file
>>>>>> operation.
>>>>>>
>>>>>> [0] https://lkml.kernel.org/r/8adf55db-7bab-f59d-d612-ed906b948d19@schaufler-ca.com
>>>>>
>>>>> [NOTE: I now see that the IORING_OP_URING_CMD has made it into the
>>>>> v5.19-rcX releases, I'm going to be honest and say that I'm
>>>>> disappointed you didn't post the related LSM additions
>>>>
>>>> It does not mean I didn't ask for them too.
>>>>
>>>>> until
>>>>> v5.19-rc6, especially given our earlier discussions.]
>>>>
>>>> And hence since I don't see it either, it's on us now.
>>>
>>> It looks like I owe you an apology, Luis.  While my frustration over
>>> io_uring remains, along with my disappointment that the io_uring
>>> developers continue to avoid discussing access controls with the LSM
>>> community, you are not the author of the IORING_OP_URING_CMD.   You
>>> are simply trying to do the right thing by adding the necessary LSM
>>> controls and in my confusion I likely caused you a bit of frustration;
>>> I'm sorry for that.
>>
>> Maybe, just maybe, outbursts like this are why there's not a lot of
>> incentive to collaborate on this? I get why it can seem frustrating and
>> that you are being ignored, but I think it's more likely that people
>> just don't think of adding these hooks. I don't use any of the access
>> controls, nor do I really have a good idea which one exists and what
>> they do. None of the external developers or internal use cases we have
>> use any of this, and nobody outside of the developers of these kernel
>> features have ever brought it up...
> 
> While my response may have been misdirected (once again, sorry Luis),
> I feel that expressing frustration about the LSMs being routinely left
> out of the discussion when new functionality is added to the kernel is
> a reasonable response; especially when one considers the history of
> this particular situation.  I was willing to attribute the initial
> LSM/audit omission in io_uring to an honest oversight, and the fact
> that we were able to work together to get something in place was a
> good thing which gave me some hope.  However, the issue around
> IORING_OP_URING_CMD was brought up earlier this year and many of us on
> the LSM side expressed concern, only to see the code present in
> v5.19-rcX with little heads-up given outside of Luis' patch a few days
> ago.  You can call my comments an outburst if you like, but it seems
> like an appropriate reaction in this case.

I agree that it should've been part of the initial series. As mentioned
above, I wasn't much apart of that earlier discussion in the series, and
hence missed that it was missing. And as also mentioned, LSM isn't much
on my radar as nobody I know uses it. This will cause oversights, even
if they are unfortunate. My point is just that no ill intent should be
assumed here.

>> I don't mind getting these added, but since I wasn't really part of
>> driving this particular feature, it wasn't on my radar.
> 
> I generally don't care who authors a commit, it's that code itself
> that matters, not who wrote it.  However, since you mentioned it I
> went back to check, and it looks like you authored the basic
> IORING_OP_URING_CMD infrastructure according to ee692a21e9bf
> ("fs,io_uring: add infrastructure for uring-cmd"); that seems like a
> decent level of awareness to me.

I did author the basic framework of it, but Kanchan took over driving it
to completion and was the one doing the posting of it at that point.
It's not like I merge code I'm not aware of, we even discussed it at
LSFMM this year and nobody brought up the LSM oversight. Luis was there
too I believe.

The lack of awareness refers to the LSM side, not io_uring obviously.
I'm very much aware of what code I merge and have co-authored, goes
without saying.

>>>> Given a clear solution is not easily tangible at this point
>>>> I was hoping perhaps at least the abilility to enable LSMs to
>>>> reject uring-cmd would be better than nothing at this point.
>>>
>>> Without any cooperation from the io_uring developers, that is likely
>>> what we will have to do.  I know there was a lot of talk about this
>>> functionality not being like another ioctl(), but from a LSM
>>> perspective I think that is how we will need to treat it.
>>
>> Again this perceived ill intent. What are you looking for here?
> 
> We expressed concern earlier this year and were largely ignored, and
> when the functionality was merged into mainline the LSM community was
> not notified despite our previous comments.  Perhaps there is no ill
> intent on the side of io_uring, but from my perspective it sure seems
> like there was an effort to avoid the LSM community.
> 
> As far as what I'm looking for, I think basic consideration for
> comments coming from the LSM community would be a good start.  We
> obviously have had some success in the past with this, which is why
> I'm a bit shocked that our IORING_OP_URING_CMD comments from earlier
> this year appeared to fall on deaf ears.

I guess it's just somewhat lack of interest, since most of us don't have
to deal with anything that uses LSM. And then it mostly just gets in the
way and adds overhead, both from a runtime and maintainability point of
view, which further reduces the motivation.

I don't think it's an effort to avoid the LSM community.

For this particular case, I do agree that it should've been picked up
and been apart of the initial merge. I'll keep a closer eye on that in
the future.

>>>>>> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
>>
>>> Well, we're at -rc6 right now which means IORING_OP_URING_CMD is
>>> happening and it's unlikely the LSM folks are going to be able to
>>> influence the design/implementation much at this point so we have to
>>> do the best we can.  Given the existing constraints, I think your
>>> patch is reasonable (although please do shift the hook call site down
>>> a bit as discussed above), we just need to develop the LSM
>>> implementations to go along with it.
>>>
>>> Luis, can you respin and resend the patch with the requested changes?
>>>
>>> Casey, it looks like Smack and SELinux are the only LSMs to implement
>>> io_uring access controls.  Given the hook that Luis developed in this
>>> patch, could you draft a patch for Smack to add the necessary checks?
>>> I'll do the same for SELinux.  My initial thinking is that all we can
>>> really do is check the access between the creds on the current task
>>> (any overrides will have already taken place by the time the LSM hook
>>> is called) with the io_uring_cmd:file label/creds; we won't be able to
>>> provide much permission granularity for all the reasons previously
>>> discussed, but I suspect that will be more of a SELinux problem than a
>>> Smack problem (although I suspect Smack will need to treat this as
>>> both a read and a write, which is likely less than ideal).
>>>
>>> I think it's doubtful we will have all of this ready and tested in
>>> time for v5.19, but I think we can have it ready shortly after that
>>> and I'll mark all of the patches for -stable when I send them to
>>> Linus.
>>>
>>> I also think we should mark the patches with a 'Fixes:' line that
>>> points at the IORING_OP_URING_CMD commit, ee692a21e9bf ("fs,io_uring:
>>> add infrastructure for uring-cmd").
>>>
>>> How does that sound to everyone?
>>
>> Let's do it the right way for 5.20, and then get it marked for a
>> backport. That will be trivial enough and will hit 5.19-stable shortly
>> as well. Rushing it now with 1 week before release will most likely
>> yield a worse long term result.
> 
> That is what I suggested above; it looks like we are on the same page
> at least with the resolution.  I'll plan on bundling Luis' hook patch,
> Casey's Smack patch, the SELinux patch and send them up to Linus once
> they are ready.  If you, and/or other io_uring developers, could
> review Luis' LSM hook patch from an io_uring perspective and add your
> Ack/Review-by tag I would appreciate it.

Right, we agree on that, and I already did send an ack on the v2 of the
patch. Just be aware that it, as of now. depends on the io_uring tree
and won't apply cleanly to 5.19-rc, as mentioned in that reply too.

-- 
Jens Axboe

Re: [PATCH] lsm,io_uring: add LSM hooks to for the new uring_cmd file op

[Casey Schaufler]

On 7/15/2022 11:46 AM, Paul Moore wrote:
> On Thu, Jul 14, 2022 at 9:00 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
>> On Wed, Jul 13, 2022 at 11:00:42PM -0400, Paul Moore wrote:
>>> On Wed, Jul 13, 2022 at 8:05 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
>>>> io-uring cmd support was added through ee692a21e9bf ("fs,io_uring:
>>>> add infrastructure for uring-cmd"), this extended 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.
>>>>
>>>> This was discussed long ago without no clear pointer for something
>>>> conclusive, so this enables LSMs to at least reject this new file
>>>> operation.
>>>>
>>>> [0] https://lkml.kernel.org/r/8adf55db-7bab-f59d-d612-ed906b948d19@schaufler-ca.com
>>> [NOTE: I now see that the IORING_OP_URING_CMD has made it into the
>>> v5.19-rcX releases, I'm going to be honest and say that I'm
>>> disappointed you didn't post the related LSM additions
>> It does not mean I didn't ask for them too.
>>
>>> until
>>> v5.19-rc6, especially given our earlier discussions.]
>> And hence since I don't see it either, it's on us now.
> It looks like I owe you an apology, Luis.  While my frustration over
> io_uring remains, along with my disappointment that the io_uring
> developers continue to avoid discussing access controls with the LSM
> community, you are not the author of the IORING_OP_URING_CMD.   You
> are simply trying to do the right thing by adding the necessary LSM
> controls and in my confusion I likely caused you a bit of frustration;
> I'm sorry for that.
>
>> As important as I think LSMs are, I cannot convince everyone
>> to take them as serious as I do.
> Yes, I think a lot of us are familiar with that feeling unfortunately :/
>
>>> While the earlier discussion may not have offered a detailed approach
>>> on how to solve this, I think it was rather conclusive in that the
>>> approach used then (and reproduced here) did not provide enough
>>> context to the LSMs to be able to make a decision.
>> Right...
>>
>>> There were similar
>>> concerns when it came to auditing the command passthrough.  It appears
>>> that most of my concerns in the original thread still apply to this
>>> patch.
>>>
>>> Given the LSM hook in this patch, it is very difficult (impossible?)
>>> to determine the requested operation as these command opcodes are
>>> device/subsystem specific.  The unfortunate result is that the LSMs
>>> are likely going to either allow all, or none, of the commands for a
>>> given device/subsystem, and I think we can all agree that is not a
>>> good idea.
>>>
>>> That is the critical bit of feedback on this patch, but there is more
>>> feedback inline below.
>> Given a clear solution is not easily tangible at this point
>> I was hoping perhaps at least the abilility to enable LSMs to
>> reject uring-cmd would be better than nothing at this point.
> Without any cooperation from the io_uring developers, that is likely
> what we will have to do.  I know there was a lot of talk about this
> functionality not being like another ioctl(), but from a LSM
> perspective I think that is how we will need to treat it.
>
>>>> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
>>>> ---
>>>>  include/linux/lsm_hook_defs.h | 1 +
>>>>  include/linux/lsm_hooks.h     | 3 +++
>>>>  include/linux/security.h      | 5 +++++
>>>>  io_uring/uring_cmd.c          | 5 +++++
>>>>  security/security.c           | 4 ++++
>>>>  5 files changed, 18 insertions(+)
>>> ...
>>>
>>>> diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c
>>>> index 0a421ed51e7e..5e666aa7edb8 100644
>>>> --- a/io_uring/uring_cmd.c
>>>> +++ b/io_uring/uring_cmd.c
>>>> @@ -3,6 +3,7 @@
>>>>  #include <linux/errno.h>
>>>>  #include <linux/file.h>
>>>>  #include <linux/io_uring.h>
>>>> +#include <linux/security.h>
>>>>
>>>>  #include <uapi/linux/io_uring.h>
>>>>
>>>> @@ -82,6 +83,10 @@ int io_uring_cmd(struct io_kiocb *req, unsigned int issue_flags)
>>>>         struct file *file = req->file;
>>>>         int ret;
>>>>
>>>> +       ret = security_uring_cmd(ioucmd);
>>>> +       if (ret)
>>>> +               return ret;
>>>> +
>>>>         if (!req->file->f_op->uring_cmd)
>>>>                 return -EOPNOTSUPP;
>>>>
>>> In order to be consistent with most of the other LSM hooks, the
>>> 'req->file->f_op->uring_cmd' check should come before the LSM hook
>>> call.
>> Sure.
>>
>>> The general approach used in most places is to first validate
>>> the request and do any DAC based access checks before calling into the
>>> LSM.
>> OK.
>>
>> Let me know how you'd like to proceed given our current status.
> Well, we're at -rc6 right now which means IORING_OP_URING_CMD is
> happening and it's unlikely the LSM folks are going to be able to
> influence the design/implementation much at this point so we have to
> do the best we can.  Given the existing constraints, I think your
> patch is reasonable (although please do shift the hook call site down
> a bit as discussed above), we just need to develop the LSM
> implementations to go along with it.
>
> Luis, can you respin and resend the patch with the requested changes?
>
> Casey, it looks like Smack and SELinux are the only LSMs to implement
> io_uring access controls.  Given the hook that Luis developed in this
> patch, could you draft a patch for Smack to add the necessary checks?

Yes. I don't think it will be anything more sophisticated than the
existing "Brutalist" Smack support. It will also be tested to the
limited extent my resources and understanding of io_uring allow.

I am seriously concerned that without better integration between
LSM and io_uring development I'm going to end up in the same place
that led to Al Viro's comment regarding the Smack fcntl hooks:

	"I think I have an adequate flame, but it won't fit
	the maillist size limit..."

That came about because my understanding of fnctl() was incomplete.
I know a lot more about fnctl than I do about io_uring. I would
really like io_uring to work well in a Smack environment. It saddens
me that there isn't any reciporicol interest. But enough whinging.
On to the patch.

> I'll do the same for SELinux.  My initial thinking is that all we can
> really do is check the access between the creds on the current task
> (any overrides will have already taken place by the time the LSM hook
> is called) with the io_uring_cmd:file label/creds; we won't be able to
> provide much permission granularity for all the reasons previously
> discussed, but I suspect that will be more of a SELinux problem than a
> Smack problem (although I suspect Smack will need to treat this as
> both a read and a write, which is likely less than ideal).
>
> I think it's doubtful we will have all of this ready and tested in
> time for v5.19, but I think we can have it ready shortly after that
> and I'll mark all of the patches for -stable when I send them to
> Linus.
>
> I also think we should mark the patches with a 'Fixes:' line that
> points at the IORING_OP_URING_CMD commit, ee692a21e9bf ("fs,io_uring:
> add infrastructure for uring-cmd").
>
> How does that sound to everyone?
>

Re: [PATCH] lsm,io_uring: add LSM hooks to for the new uring_cmd file op

[Casey Schaufler]

On 7/15/2022 1:00 PM, Jens Axboe wrote:
> I agree that it should've been part of the initial series. As mentioned
> above, I wasn't much apart of that earlier discussion in the series, and
> hence missed that it was missing. And as also mentioned, LSM isn't much
> on my radar as nobody I know uses it.

There are well over 6 Billion systems deployed in the wild that use LSM.
Every Android device. Every Samsung TV, camera and watch. Chromebooks.
Data centers. AWS. HPC. Statistically, a system that does not use LSM is
extremely rare. The only systems that *don't* use LSM are the ones hand
configured by Linux developers for their own use.

>  This will cause oversights, even
> if they are unfortunate. My point is just that no ill intent should be
> assumed here.

I see no ill intent. And io_uring addresses an important issue.
It just needs to work for the majority of Linux systems, not just
the few that don't use LSM.

Re: [PATCH] lsm,io_uring: add LSM hooks to for the new uring_cmd file op

[Jens Axboe]

On 7/15/22 3:16 PM, Casey Schaufler wrote:
> On 7/15/2022 1:00 PM, Jens Axboe wrote:
>> I agree that it should've been part of the initial series. As mentioned
>> above, I wasn't much apart of that earlier discussion in the series, and
>> hence missed that it was missing. And as also mentioned, LSM isn't much
>> on my radar as nobody I know uses it.
> 
> There are well over 6 Billion systems deployed in the wild that use LSM.
> Every Android device. Every Samsung TV, camera and watch. Chromebooks.
> Data centers. AWS. HPC. Statistically, a system that does not use LSM is
> extremely rare. The only systems that *don't* use LSM are the ones hand
> configured by Linux developers for their own use.

I'm not talking about systems that only I use, but I believe you that
it's in wide use. Didn't mean to imply that it isn't, just that since I
don't come across it in my work or the people/systems that I've worked
with, it hasn't been much on my radar and nobody has asked for it.

>>  This will cause oversights, even
>> if they are unfortunate. My point is just that no ill intent should be
>> assumed here.
> 
> I see no ill intent. And io_uring addresses an important issue.
> It just needs to work for the majority of Linux systems, not just
> the few that don't use LSM.

Agree, and hopefully we can make sure that it does, going forward as
well.

-- 
Jens Axboe

Re: [PATCH] lsm,io_uring: add LSM hooks to for the new uring_cmd file op

[Luis Chamberlain]

On Fri, Jul 15, 2022 at 02:00:36PM -0600, Jens Axboe wrote:
> I did author the basic framework of it, but Kanchan took over driving it
> to completion and was the one doing the posting of it at that point.

And credit where due, that was a significant undertaking, and great
collaboration.

> It's not like I merge code I'm not aware of, we even discussed it at
> LSFMM this year and nobody brought up the LSM oversight. Luis was there
> too I believe.

I brought it up as a priority to Kanchan then. I cringed at not seeing it
addressed, but as with a lot of development, some things get punted for
'eventually'. What I think we need is more awareness of the importance of
addressing LSMs and making this a real top priority, not just, 'sure',
or 'eventually'. Without that wide awareness even those aware of its
importance cannot help make LSM considerations a tangible priority.

We can do this with ksummit, or whatever that's called these days,
because just doing this at security conferences is just getting people
preaching to the choir.

We can also do this through sessions at different companies.

  Luis

Re: [PATCH] lsm,io_uring: add LSM hooks to for the new uring_cmd file op

[Jens Axboe]

On 7/15/22 3:37 PM, Luis Chamberlain wrote:
> On Fri, Jul 15, 2022 at 02:00:36PM -0600, Jens Axboe wrote:
>> I did author the basic framework of it, but Kanchan took over driving it
>> to completion and was the one doing the posting of it at that point.
> 
> And credit where due, that was a significant undertaking, and great
> collaboration.

Definitely, the completion bit is usually the longest pole in the
endevaour.

>> It's not like I merge code I'm not aware of, we even discussed it at
>> LSFMM this year and nobody brought up the LSM oversight. Luis was there
>> too I believe.
> 
> I brought it up as a priority to Kanchan then. I cringed at not seeing it
> addressed, but as with a lot of development, some things get punted for
> 'eventually'. What I think we need is more awareness of the importance of
> addressing LSMs and making this a real top priority, not just, 'sure',
> or 'eventually'. Without that wide awareness even those aware of its
> importance cannot help make LSM considerations a tangible priority.

Not sure if this is a generic problem, or mostly on our side. uring_cmd
is a bit of an exception, since we don't really add a lot of non-syscall
accessible bits to begin with. But in general there's for sure more
action there than in other spots. I'm hopeful that this will be more on
top of our minds when the next time comes around.

For uring_cmd, extensions will most likely happen. At least I have some
in mind. We might want to make the control more finegrained at that
point, but let's deal with that when we get there.

> We can do this with ksummit, or whatever that's called these days,
> because just doing this at security conferences is just getting people
> preaching to the choir.

Don't think anyone disagrees that it needs to get done, and there's not
much process to hash out here other than one subsystem being aware of
another ones needs. Hence don't think the kernel summit or maintainers
summit is doing to be useful in that regard. Just my 2 cents.

-- 
Jens Axboe

Re: [PATCH] lsm,io_uring: add LSM hooks to for the new uring_cmd file op

[Casey Schaufler]

On 7/15/2022 1:50 PM, Casey Schaufler wrote:
> On 7/15/2022 11:46 AM, Paul Moore wrote:
>> On Thu, Jul 14, 2022 at 9:00 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
>>> On Wed, Jul 13, 2022 at 11:00:42PM -0400, Paul Moore wrote:
>>>> On Wed, Jul 13, 2022 at 8:05 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
>>>>> io-uring cmd support was added through ee692a21e9bf ("fs,io_uring:
>>>>> add infrastructure for uring-cmd"), this extended 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.
>>>>>
>>>>> This was discussed long ago without no clear pointer for something
>>>>> conclusive, so this enables LSMs to at least reject this new file
>>>>> operation.
>>>>>
>>>>> [0] https://lkml.kernel.org/r/8adf55db-7bab-f59d-d612-ed906b948d19@schaufler-ca.com
>>>> [NOTE: I now see that the IORING_OP_URING_CMD has made it into the
>>>> v5.19-rcX releases, I'm going to be honest and say that I'm
>>>> disappointed you didn't post the related LSM additions
>>> It does not mean I didn't ask for them too.
>>>
>>>> until
>>>> v5.19-rc6, especially given our earlier discussions.]
>>> And hence since I don't see it either, it's on us now.
>> It looks like I owe you an apology, Luis.  While my frustration over
>> io_uring remains, along with my disappointment that the io_uring
>> developers continue to avoid discussing access controls with the LSM
>> community, you are not the author of the IORING_OP_URING_CMD.   You
>> are simply trying to do the right thing by adding the necessary LSM
>> controls and in my confusion I likely caused you a bit of frustration;
>> I'm sorry for that.
>>
>>> As important as I think LSMs are, I cannot convince everyone
>>> to take them as serious as I do.
>> Yes, I think a lot of us are familiar with that feeling unfortunately :/
>>
>>>> While the earlier discussion may not have offered a detailed approach
>>>> on how to solve this, I think it was rather conclusive in that the
>>>> approach used then (and reproduced here) did not provide enough
>>>> context to the LSMs to be able to make a decision.
>>> Right...
>>>
>>>> There were similar
>>>> concerns when it came to auditing the command passthrough.  It appears
>>>> that most of my concerns in the original thread still apply to this
>>>> patch.
>>>>
>>>> Given the LSM hook in this patch, it is very difficult (impossible?)
>>>> to determine the requested operation as these command opcodes are
>>>> device/subsystem specific.  The unfortunate result is that the LSMs
>>>> are likely going to either allow all, or none, of the commands for a
>>>> given device/subsystem, and I think we can all agree that is not a
>>>> good idea.
>>>>
>>>> That is the critical bit of feedback on this patch, but there is more
>>>> feedback inline below.
>>> Given a clear solution is not easily tangible at this point
>>> I was hoping perhaps at least the abilility to enable LSMs to
>>> reject uring-cmd would be better than nothing at this point.
>> Without any cooperation from the io_uring developers, that is likely
>> what we will have to do.  I know there was a lot of talk about this
>> functionality not being like another ioctl(), but from a LSM
>> perspective I think that is how we will need to treat it.
>>
>>>>> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
>>>>> ---
>>>>>  include/linux/lsm_hook_defs.h | 1 +
>>>>>  include/linux/lsm_hooks.h     | 3 +++
>>>>>  include/linux/security.h      | 5 +++++
>>>>>  io_uring/uring_cmd.c          | 5 +++++
>>>>>  security/security.c           | 4 ++++
>>>>>  5 files changed, 18 insertions(+)
>>>> ...
>>>>
>>>>> diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c
>>>>> index 0a421ed51e7e..5e666aa7edb8 100644
>>>>> --- a/io_uring/uring_cmd.c
>>>>> +++ b/io_uring/uring_cmd.c
>>>>> @@ -3,6 +3,7 @@
>>>>>  #include <linux/errno.h>
>>>>>  #include <linux/file.h>
>>>>>  #include <linux/io_uring.h>
>>>>> +#include <linux/security.h>
>>>>>
>>>>>  #include <uapi/linux/io_uring.h>
>>>>>
>>>>> @@ -82,6 +83,10 @@ int io_uring_cmd(struct io_kiocb *req, unsigned int issue_flags)
>>>>>         struct file *file = req->file;
>>>>>         int ret;
>>>>>
>>>>> +       ret = security_uring_cmd(ioucmd);
>>>>> +       if (ret)
>>>>> +               return ret;
>>>>> +
>>>>>         if (!req->file->f_op->uring_cmd)
>>>>>                 return -EOPNOTSUPP;
>>>>>
>>>> In order to be consistent with most of the other LSM hooks, the
>>>> 'req->file->f_op->uring_cmd' check should come before the LSM hook
>>>> call.
>>> Sure.
>>>
>>>> The general approach used in most places is to first validate
>>>> the request and do any DAC based access checks before calling into the
>>>> LSM.
>>> OK.
>>>
>>> Let me know how you'd like to proceed given our current status.
>> Well, we're at -rc6 right now which means IORING_OP_URING_CMD is
>> happening and it's unlikely the LSM folks are going to be able to
>> influence the design/implementation much at this point so we have to
>> do the best we can.  Given the existing constraints, I think your
>> patch is reasonable (although please do shift the hook call site down
>> a bit as discussed above), we just need to develop the LSM
>> implementations to go along with it.
>>
>> Luis, can you respin and resend the patch with the requested changes?
>>
>> Casey, it looks like Smack and SELinux are the only LSMs to implement
>> io_uring access controls.  Given the hook that Luis developed in this
>> patch, could you draft a patch for Smack to add the necessary checks?
> Yes. I don't think it will be anything more sophisticated than the
> existing "Brutalist" Smack support. It will also be tested to the
> limited extent my resources and understanding of io_uring allow.
>
> I am seriously concerned that without better integration between
> LSM and io_uring development I'm going to end up in the same place
> that led to Al Viro's comment regarding the Smack fcntl hooks:
>
> 	"I think I have an adequate flame, but it won't fit
> 	the maillist size limit..."
>
> That came about because my understanding of fnctl() was incomplete.
> I know a lot more about fnctl than I do about io_uring. I would
> really like io_uring to work well in a Smack environment. It saddens
> me that there isn't any reciporicol interest. But enough whinging.
> On to the patch.

There isn't (as of this writing) a file io_uring/uring_cmd.c in
Linus' tree. What tree does this patch apply to?

Re: [PATCH] lsm,io_uring: add LSM hooks to for the new uring_cmd file op

[Jens Axboe]

On 7/15/22 5:03 PM, Casey Schaufler wrote:
> On 7/15/2022 1:50 PM, Casey Schaufler wrote:
>> On 7/15/2022 11:46 AM, Paul Moore wrote:
>>> On Thu, Jul 14, 2022 at 9:00 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
>>>> On Wed, Jul 13, 2022 at 11:00:42PM -0400, Paul Moore wrote:
>>>>> On Wed, Jul 13, 2022 at 8:05 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
>>>>>> io-uring cmd support was added through ee692a21e9bf ("fs,io_uring:
>>>>>> add infrastructure for uring-cmd"), this extended 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.
>>>>>>
>>>>>> This was discussed long ago without no clear pointer for something
>>>>>> conclusive, so this enables LSMs to at least reject this new file
>>>>>> operation.
>>>>>>
>>>>>> [0] https://lkml.kernel.org/r/8adf55db-7bab-f59d-d612-ed906b948d19@schaufler-ca.com
>>>>> [NOTE: I now see that the IORING_OP_URING_CMD has made it into the
>>>>> v5.19-rcX releases, I'm going to be honest and say that I'm
>>>>> disappointed you didn't post the related LSM additions
>>>> It does not mean I didn't ask for them too.
>>>>
>>>>> until
>>>>> v5.19-rc6, especially given our earlier discussions.]
>>>> And hence since I don't see it either, it's on us now.
>>> It looks like I owe you an apology, Luis.  While my frustration over
>>> io_uring remains, along with my disappointment that the io_uring
>>> developers continue to avoid discussing access controls with the LSM
>>> community, you are not the author of the IORING_OP_URING_CMD.   You
>>> are simply trying to do the right thing by adding the necessary LSM
>>> controls and in my confusion I likely caused you a bit of frustration;
>>> I'm sorry for that.
>>>
>>>> As important as I think LSMs are, I cannot convince everyone
>>>> to take them as serious as I do.
>>> Yes, I think a lot of us are familiar with that feeling unfortunately :/
>>>
>>>>> While the earlier discussion may not have offered a detailed approach
>>>>> on how to solve this, I think it was rather conclusive in that the
>>>>> approach used then (and reproduced here) did not provide enough
>>>>> context to the LSMs to be able to make a decision.
>>>> Right...
>>>>
>>>>> There were similar
>>>>> concerns when it came to auditing the command passthrough.  It appears
>>>>> that most of my concerns in the original thread still apply to this
>>>>> patch.
>>>>>
>>>>> Given the LSM hook in this patch, it is very difficult (impossible?)
>>>>> to determine the requested operation as these command opcodes are
>>>>> device/subsystem specific.  The unfortunate result is that the LSMs
>>>>> are likely going to either allow all, or none, of the commands for a
>>>>> given device/subsystem, and I think we can all agree that is not a
>>>>> good idea.
>>>>>
>>>>> That is the critical bit of feedback on this patch, but there is more
>>>>> feedback inline below.
>>>> Given a clear solution is not easily tangible at this point
>>>> I was hoping perhaps at least the abilility to enable LSMs to
>>>> reject uring-cmd would be better than nothing at this point.
>>> Without any cooperation from the io_uring developers, that is likely
>>> what we will have to do.  I know there was a lot of talk about this
>>> functionality not being like another ioctl(), but from a LSM
>>> perspective I think that is how we will need to treat it.
>>>
>>>>>> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
>>>>>> ---
>>>>>>  include/linux/lsm_hook_defs.h | 1 +
>>>>>>  include/linux/lsm_hooks.h     | 3 +++
>>>>>>  include/linux/security.h      | 5 +++++
>>>>>>  io_uring/uring_cmd.c          | 5 +++++
>>>>>>  security/security.c           | 4 ++++
>>>>>>  5 files changed, 18 insertions(+)
>>>>> ...
>>>>>
>>>>>> diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c
>>>>>> index 0a421ed51e7e..5e666aa7edb8 100644
>>>>>> --- a/io_uring/uring_cmd.c
>>>>>> +++ b/io_uring/uring_cmd.c
>>>>>> @@ -3,6 +3,7 @@
>>>>>>  #include <linux/errno.h>
>>>>>>  #include <linux/file.h>
>>>>>>  #include <linux/io_uring.h>
>>>>>> +#include <linux/security.h>
>>>>>>
>>>>>>  #include <uapi/linux/io_uring.h>
>>>>>>
>>>>>> @@ -82,6 +83,10 @@ int io_uring_cmd(struct io_kiocb *req, unsigned int issue_flags)
>>>>>>         struct file *file = req->file;
>>>>>>         int ret;
>>>>>>
>>>>>> +       ret = security_uring_cmd(ioucmd);
>>>>>> +       if (ret)
>>>>>> +               return ret;
>>>>>> +
>>>>>>         if (!req->file->f_op->uring_cmd)
>>>>>>                 return -EOPNOTSUPP;
>>>>>>
>>>>> In order to be consistent with most of the other LSM hooks, the
>>>>> 'req->file->f_op->uring_cmd' check should come before the LSM hook
>>>>> call.
>>>> Sure.
>>>>
>>>>> The general approach used in most places is to first validate
>>>>> the request and do any DAC based access checks before calling into the
>>>>> LSM.
>>>> OK.
>>>>
>>>> Let me know how you'd like to proceed given our current status.
>>> Well, we're at -rc6 right now which means IORING_OP_URING_CMD is
>>> happening and it's unlikely the LSM folks are going to be able to
>>> influence the design/implementation much at this point so we have to
>>> do the best we can.  Given the existing constraints, I think your
>>> patch is reasonable (although please do shift the hook call site down
>>> a bit as discussed above), we just need to develop the LSM
>>> implementations to go along with it.
>>>
>>> Luis, can you respin and resend the patch with the requested changes?
>>>
>>> Casey, it looks like Smack and SELinux are the only LSMs to implement
>>> io_uring access controls.  Given the hook that Luis developed in this
>>> patch, could you draft a patch for Smack to add the necessary checks?
>> Yes. I don't think it will be anything more sophisticated than the
>> existing "Brutalist" Smack support. It will also be tested to the
>> limited extent my resources and understanding of io_uring allow.
>>
>> I am seriously concerned that without better integration between
>> LSM and io_uring development I'm going to end up in the same place
>> that led to Al Viro's comment regarding the Smack fcntl hooks:
>>
>> 	"I think I have an adequate flame, but it won't fit
>> 	the maillist size limit..."
>>
>> That came about because my understanding of fnctl() was incomplete.
>> I know a lot more about fnctl than I do about io_uring. I would
>> really like io_uring to work well in a Smack environment. It saddens
>> me that there isn't any reciporicol interest. But enough whinging.
>> On to the patch.
> 
> There isn't (as of this writing) a file io_uring/uring_cmd.c in
> Linus' tree. What tree does this patch apply to?

It's the for-5.20 tree. See my reply to the v2 of the patch, including
suggestions on how to stage it.

-- 
Jens Axboe

Re: [PATCH] lsm,io_uring: add LSM hooks to for the new uring_cmd file op

[Casey Schaufler]

On 7/15/2022 4:05 PM, Jens Axboe wrote:
> On 7/15/22 5:03 PM, Casey Schaufler wrote:
>
>> There isn't (as of this writing) a file io_uring/uring_cmd.c in
>> Linus' tree. What tree does this patch apply to?
> It's the for-5.20 tree. See my reply to the v2 of the patch, including
> suggestions on how to stage it.

A URL for the io_uring tree would be REAL helpful.

Re: [PATCH] lsm,io_uring: add LSM hooks to for the new uring_cmd file op

[Jens Axboe]

On 7/15/22 5:14 PM, Casey Schaufler wrote:
> On 7/15/2022 4:05 PM, Jens Axboe wrote:
>> On 7/15/22 5:03 PM, Casey Schaufler wrote:
>>
>>> There isn't (as of this writing) a file io_uring/uring_cmd.c in
>>> Linus' tree. What tree does this patch apply to?
>> It's the for-5.20 tree. See my reply to the v2 of the patch, including
>> suggestions on how to stage it.
> 
> A URL for the io_uring tree would be REAL helpful.

https://git.kernel.dk/cgit/linux-block/log/?h=for-5.20/io_uring

That's the pending 5.20 tree. But there's really nothing exciting there
in terms of LSM etc, it's just that things have been broken up and
aren't in one big file anymore.

-- 
Jens Axboe

Re: [PATCH] lsm,io_uring: add LSM hooks to for the new uring_cmd file op

[Casey Schaufler]

On 7/15/2022 4:18 PM, Jens Axboe wrote:
> On 7/15/22 5:14 PM, Casey Schaufler wrote:
>> On 7/15/2022 4:05 PM, Jens Axboe wrote:
>>> On 7/15/22 5:03 PM, Casey Schaufler wrote:
>>>
>>>> There isn't (as of this writing) a file io_uring/uring_cmd.c in
>>>> Linus' tree. What tree does this patch apply to?
>>> It's the for-5.20 tree. See my reply to the v2 of the patch, including
>>> suggestions on how to stage it.
>> A URL for the io_uring tree would be REAL helpful.
> https://git.kernel.dk/cgit/linux-block/log/?h=for-5.20/io_uring

I'm sorry, I must be being extremely obtuse. I want to create the Smack
patch to go along with the patch under discussion. I would like to clone
the tree (with git clone <URL> ; git checkout <branch>) so I can build
the tree and then develop the code.  The URL you provided is a web front
end to the git tree, and does not provide the clone URL (that I can find).

>
> That's the pending 5.20 tree. But there's really nothing exciting there
> in terms of LSM etc, it's just that things have been broken up and
> aren't in one big file anymore.
>

Re: [PATCH] lsm,io_uring: add LSM hooks to for the new uring_cmd file op

[Jens Axboe]

On 7/15/22 5:31 PM, Casey Schaufler wrote:
> On 7/15/2022 4:18 PM, Jens Axboe wrote:
>> On 7/15/22 5:14 PM, Casey Schaufler wrote:
>>> On 7/15/2022 4:05 PM, Jens Axboe wrote:
>>>> On 7/15/22 5:03 PM, Casey Schaufler wrote:
>>>>
>>>>> There isn't (as of this writing) a file io_uring/uring_cmd.c in
>>>>> Linus' tree. What tree does this patch apply to?
>>>> It's the for-5.20 tree. See my reply to the v2 of the patch, including
>>>> suggestions on how to stage it.
>>> A URL for the io_uring tree would be REAL helpful.
>> https://git.kernel.dk/cgit/linux-block/log/?h=for-5.20/io_uring
> 
> I'm sorry, I must be being extremely obtuse. I want to create the Smack
> patch to go along with the patch under discussion. I would like to clone
> the tree (with git clone <URL> ; git checkout <branch>) so I can build
> the tree and then develop the code.  The URL you provided is a web front
> end to the git tree, and does not provide the clone URL (that I can find).

Just go one level back, out of the branch, and it'll tell you:

git://git.kernel.dk/linux-block

or

https://git.kernel.dk/linux-block

and the branch is for-5.20/io_uring as in the link.

-- 
Jens Axboe

Re: [PATCH] lsm,io_uring: add LSM hooks to for the new uring_cmd file op

[Kanchan Joshi]

[-- Attachment #1: Type: text/plain, Size: 1931 bytes --]

On Fri, Jul 15, 2022 at 02:46:16PM -0400, Paul Moore wrote:
>On Thu, Jul 14, 2022 at 9:00 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
>> On Wed, Jul 13, 2022 at 11:00:42PM -0400, Paul Moore wrote:
>> > On Wed, Jul 13, 2022 at 8:05 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
>> > >
>> > > io-uring cmd support was added through ee692a21e9bf ("fs,io_uring:
>> > > add infrastructure for uring-cmd"), this extended 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.
>> > >
>> > > This was discussed long ago without no clear pointer for something
>> > > conclusive, so this enables LSMs to at least reject this new file
>> > > operation.
>> > >
>> > > [0] https://lkml.kernel.org/r/8adf55db-7bab-f59d-d612-ed906b948d19@schaufler-ca.com
>> >
>> > [NOTE: I now see that the IORING_OP_URING_CMD has made it into the
>> > v5.19-rcX releases, I'm going to be honest and say that I'm
>> > disappointed you didn't post the related LSM additions
>>
>> It does not mean I didn't ask for them too.
>>
>> > until
>> > v5.19-rc6, especially given our earlier discussions.]
>>
>> And hence since I don't see it either, it's on us now.
>
>It looks like I owe you an apology, Luis.  While my frustration over
>io_uring remains, along with my disappointment that the io_uring
>developers continue to avoid discussing access controls with the LSM
>community, you are not the author of the IORING_OP_URING_CMD.   You

I am to be shot down here. Solely.
My LSM understanding has been awful. At a level that I am not clear
how to fix if someone says - your code lacks LSM consideration.
But nothing to justify, I fully understand this is not someone else's
problem but mine. I intend to get better at it.
And I owe apology (to you/LSM-folks, Luis, Jens) for the mess.

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] lsm,io_uring: add LSM hooks to for the new uring_cmd file op

[Paul Moore]

On Fri, Jul 15, 2022 at 11:26 PM Kanchan Joshi <joshi.k@samsung.com> wrote:
> On Fri, Jul 15, 2022 at 02:46:16PM -0400, Paul Moore wrote:
> >On Thu, Jul 14, 2022 at 9:00 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
> >> On Wed, Jul 13, 2022 at 11:00:42PM -0400, Paul Moore wrote:
> >> > On Wed, Jul 13, 2022 at 8:05 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
> >> > >
> >> > > io-uring cmd support was added through ee692a21e9bf ("fs,io_uring:
> >> > > add infrastructure for uring-cmd"), this extended 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.
> >> > >
> >> > > This was discussed long ago without no clear pointer for something
> >> > > conclusive, so this enables LSMs to at least reject this new file
> >> > > operation.
> >> > >
> >> > > [0] https://lkml.kernel.org/r/8adf55db-7bab-f59d-d612-ed906b948d19@schaufler-ca.com
> >> >
> >> > [NOTE: I now see that the IORING_OP_URING_CMD has made it into the
> >> > v5.19-rcX releases, I'm going to be honest and say that I'm
> >> > disappointed you didn't post the related LSM additions
> >>
> >> It does not mean I didn't ask for them too.
> >>
> >> > until
> >> > v5.19-rc6, especially given our earlier discussions.]
> >>
> >> And hence since I don't see it either, it's on us now.
> >
> >It looks like I owe you an apology, Luis.  While my frustration over
> >io_uring remains, along with my disappointment that the io_uring
> >developers continue to avoid discussing access controls with the LSM
> >community, you are not the author of the IORING_OP_URING_CMD.   You
>
> I am to be shot down here. Solely.
> My LSM understanding has been awful. At a level that I am not clear
> how to fix if someone says - your code lacks LSM consideration.
> But nothing to justify, I fully understand this is not someone else's
> problem but mine. I intend to get better at it.
> And I owe apology (to you/LSM-folks, Luis, Jens) for the mess.

Thanks for your honesty.  If it is any consolation, my understanding
of io_uring remains superficial at best, and it's one of the reasons
I've asked the io_uring devs to ack/review the LSM io_uring hooks and
their placement in the io_uring code.  Developing a deep understanding
of one kernel subsystem is often very difficult, doing the same across
multiple subsystems requires a *lot* of time and effort.  We have to
rely on our combined expertise to help each other fill in the gaps :)

If you are ever unsure about something in the LSM layer, or how a
change to io_uring (or any other subsystem) might impact the LSMs,
please don't hesitate to ask us.  It might take all of us a little
while to sort it out, but we can usually get it working in the end.

There shouldn't be harm in asking for help/clarification, the harm
usually comes when assumptions are made.

-- 
paul-moore.com