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