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

[Casey Schaufler <casey@schaufler-ca.com>]

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?
>