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@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxx
>>>> [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@xxxxxxxxxx>
>>>>> ---
>>>>> 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?
