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

[Jens Axboe <axboe@kernel.dk>]

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