[GIT PULL] SELinux patches for v5.16

Started by Paul Moore on Nov 1, 2021 · 7 messages from 3 authors · Last reply Nov 2, 2021

open the first message on its own page

Message details and related content
Hi Linus,

Below is the SELinux pull request for v5.16 with a note about merge
conflicts following the highlight reel (you'll see something similar
on the audit pull request, and hopefully the io_uring and block/dm
trees but I have no idea if they track the LSM/audit work - likely
not).

** Highlights

- Add LSM/SELinux/Smack controls and auditing for io-uring.  As usual,
the individual commit descriptions have more detail, but we were
basically missing two things which we're adding here: establishment of
a proper audit context so that auditing of io-uring ops works
similarly to how it does for syscalls (with some io-uring additions
because io-uring ops are *not* syscalls), additional LSM hooks to
enable access control points for some of the more unusual io-uring
features, e.g. credential overrides.  The additional audit callouts
and LSM hooks were done in conjunction with the io-uring folks, based
on conversations and RFC patches earlier in the year.

- Fixup the binder credential handling so that the proper credentials
are used in the LSM hooks; the
commit description and the code comment which is removed in these
patches are helpful to understand the background and why this is the
proper fix.

- Enable SELinux genfscon policy support for securityfs, allowing
improved SELinux filesystem labeling for other subsystems which make
use of securityfs, e.g. IMA.

** Merge Notes

- I'm expecting three trees to add new audit record types during this
merge window: SELinux, block/device-mapper, and audit.  I've already
talked with the different maintainers and there shouldn't be any
duplicated values, but I expect you will see some merge conflicts in
include/uapi/linux/audit.h; the "correct" values should end up as:

  +#define AUDIT_URINGOP   1336 /* io_uring operation */

104 more lines

Message details and related content
On Mon, Nov 1, 2021 at 4:59 PM Paul Moore [off-list ref] wrote:
- Add LSM/SELinux/Smack controls and auditing for io-uring.
I started doing the merge resolution, and then I noted that there is
no sign that this has been discussed with the io_uring developers at
all.

Maybe there have been extensive discussions. I wouldn't know. There's
no acks, no links, no nothing in the commit messages.

So I ended up deciding not to pull at all after all.

You really can't just decide "let's add random audit hooks to this"
without talking to the maintainers.

And if you _did_ talk to maintainers, and got the go-ahead, why is
there absolutely zero sign of that in the commits?

              Linus
Message details and related content
On Mon, Nov 1, 2021 at 8:44 PM Linus Torvalds
[off-list ref] wrote:
On Mon, Nov 1, 2021 at 4:59 PM Paul Moore [off-list ref] wrote:
quoted
- Add LSM/SELinux/Smack controls and auditing for io-uring.
I started doing the merge resolution, and then I noted that there is
no sign that this has been discussed with the io_uring developers at
all.
I felt I addressed that in the pull request cover letter, although it
appears not in a way that you found adequate.  More on this below, but
here is the discussion history, with lore links:

*** Initial Draft (RFC) (May 2021)
https://lore.kernel.org/linux-security-module/162163367115.8379.8459012634106035341.stgit@sifl/

In the initial RFC you will see a lot of discussion with Jens Axboe
and Pavel Begunkov discussing the patchset and potential changes to
the solution.  Jens summarized his opinion on resolving this in the
message below, you'll note Jens approach is what was implemented and
sent to you via PR.

* Jens' Summary
https://lore.kernel.org/linux-security-module/46381e4e-a65d-f217-1d0d-43d1fa8a99aa@kernel.dk/

  "Sorry for the lack of response here, but to sum up my
   order of preference:

   1) It's probably better to just make the audit an opt-out
      in io_op_defs for each opcode, and avoid needing boiler
      plate code for each op handler. The opt-out would ensure
      that new opcodes get it by default it someone doesn't
      know what it is, and the io_op_defs addition would mean
      that it's in generic code rather then in the handlers.
      Yes it's a bit slower, but it's saner imho.

   2) With the above, I'm fine with adding this to io_uring.
      I don't think going the route of mutual exclusion in
      kconfig helps anyone, it'd be counter productive to
      both sides."

*** Second Revision (RFC) (Aug 2021)
https://lore.kernel.org/linux-security-module/162871480969.63873.9434591871437326374.stgit@olly/

This patchset implemented the approach described by Jens as well as
incorporated all of the feedback from the initial RFC.  There was some
additional discussion among the LSM/audit crowd but no additional
comments from the io-uring devs despite being on the To/CC line and

17 more lines

Maybe there have been extensive discussions. I wouldn't know. There's
no acks, no links, no nothing in the commit messages.

So I ended up deciding not to pull at all after all.

You really can't just decide "let's add random audit hooks to this"
without talking to the maintainers.
*sigh*

I can promise you I've never done that, nor would I ever consider it.
And if you _did_ talk to maintainers, and got the go-ahead, why is
there absolutely zero sign of that in the commits?
I felt the comment in the pull request was sufficient, however based
on your response it clearly isn't.  Would you like me to edit the
commits to add various discussion tags, is this follow-up sufficient,
or would you like me to do something else?  Just let me know what you
need to feel good about merging this pull request.

-- 
paul moore
www.paul-moore.com
Message details and related content
On Mon, Nov 1, 2021 at 8:13 PM Paul Moore [off-list ref] wrote:
I felt I addressed that in the pull request cover letter, although it
appears not in a way that you found adequate.
Yeah, it's actually quite adequate, but I wasn't seeing it.

Going back, I see that

  "The additional audit callouts and LSM hooks were done in
conjunction with the io-uring folks, based on conversations and RFC
patches earlier in the year"

So yeah, it was there, and I missed it. My bad.

It would have been good to have a link to said discussions in the
commits, or even just a "cc:" or whatever so that I see that the
proper people were aware of it.

Partly just for posterity, partly simply because that's actually what
I look at when doing conflict resolution.

I do obviously go back to the original email later to see if you then
had an example resolution (which I'll then compare against what I did
to see that I didn't miss anything), and to complete the commit
message. But in this case I didn't even get past the conflict when I
started going "but but but.."
I felt the comment in the pull request was sufficient, however based
on your response it clearly isn't.  Would you like me to edit the
commits to add various discussion tags, is this follow-up sufficient,
or would you like me to do something else?
This follow-up was sufficient. In fact, the original should have been
sufficient for me.

I just need to feel like I know that toes haven't been stepped on, and
that I don't have to fight a merge later..

             Linus
Message details and related content
On Mon, Nov 1, 2021 at 8:55 PM Linus Torvalds
[off-list ref] wrote:
This follow-up was sufficient. In fact, the original should have been
sufficient for me.
... and as you saw from the pr-tracker-bot, it's all merged now.

Sorry for missing that part of your original pull request.

            Linus
Message details and related content
On Tue, Nov 2, 2021 at 12:23 AM Linus Torvalds
[off-list ref] wrote:
On Mon, Nov 1, 2021 at 8:55 PM Linus Torvalds
[off-list ref] wrote:
quoted
This follow-up was sufficient. In fact, the original should have been
sufficient for me.
... and as you saw from the pr-tracker-bot, it's all merged now.

Sorry for missing that part of your original pull request.
The important part is that it is now in your tree, thanks.

-- 
paul moore
www.paul-moore.com
Keyboard shortcuts
hback out one level
jnext message in thread / next series-diff position
kprevious message in thread / previous series-diff position
ldrill in
Escclose help / fold thread tree
?toggle this help