mbox series

[GIT,PULL] SELinux patches for v5.16

Message ID CAHC9VhRJ=fHzMHM6tt8JqkZa4bf0h72CAytSX9YrEs14Oaj8SA@mail.gmail.com (mailing list archive)
State New, archived
Headers show
Series [GIT,PULL] SELinux patches for v5.16 | expand

Pull-request

git://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git tags/selinux-pr-20211101

Message

Paul Moore Nov. 1, 2021, 11:59 p.m. UTC
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 */
  +#define AUDIT_OPENAT2   1337 /* Record showing openat2 how args */
  +#define AUDIT_DM_CTRL   1338 /* Device Mapper target control */
  +#define AUDIT_DM_EVENT  1339 /* Device Mapper events */

- Based on your tree from this afternoon you will see a merge conflict
in fs/io-wq.c, but it's just an include collision, the fixup is
trivial.

- Based on your tree from this afternoon you will see a merge conflict
in fs/io_uring.c in the io_op_def struct definition; the fixup is also
pretty easy, just make sure the "audit_skip" field is added to the
struct.

Thanks,
-Paul

--
The following changes since commit 6880fa6c56601bb8ed59df6c30fd390cc5f6dd8f:

 Linux 5.15-rc1 (2021-09-12 16:28:37 -0700)

are available in the Git repository at:

 git://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git
   tags/selinux-pr-20211101

for you to fetch changes up to 15bf32398ad488c0df1cbaf16431422c87e4feea:

 security: Return xattr name from security_dentry_init_security()
   (2021-10-20 08:17:08 -0400)

----------------------------------------------------------------
selinux/stable-5.16 PR 20211101

----------------------------------------------------------------
Casey Schaufler (1):
     Smack: Brutalist io_uring support

Christian Göttsche (1):
     selinux: enable genfscon labeling for securityfs

Florian Westphal (1):
     selinux: remove unneeded ipv6 hook wrappers

Kees Cook (1):
     LSM: Avoid warnings about potentially unused hook variables

Ondrej Mosnacek (1):
     selinux: fix race condition when computing ocontext SIDs

Paul Moore (11):
     audit: prepare audit_context for use in calling contexts beyond syscalls
     audit,io_uring,io-wq: add some basic audit support to io_uring
     audit: add filtering for io_uring records
     fs: add anon_inode_getfile_secure() similar to anon_inode_getfd_secure()
     io_uring: convert io_uring to the secure anon inode interface
     lsm,io_uring: add LSM hooks to io_uring
     selinux: add support for the io_uring access controls
     selinux: remove the SELinux lockdown implementation
     selinux: make better use of the nf_hook_state passed to the NF hooks
     selinux: fix all of the W=1 build warnings
     selinux: fix a sock regression in selinux_ip_postroute_compat()

Todd Kjos (3):
     binder: use euid from cred instead of using task
     binder: use cred instead of task for selinux checks
     binder: use cred instead of task for getsecid

Vivek Goyal (1):
     security: Return xattr name from security_dentry_init_security()

drivers/android/binder.c            |  27 +--
drivers/android/binder_internal.h   |   4 +
fs/anon_inodes.c                    |  29 +++
fs/ceph/xattr.c                     |   3 +-
fs/io-wq.c                          |   4 +
fs/io_uring.c                       |  69 +++++-
fs/nfs/nfs4proc.c                   |   3 +-
include/linux/anon_inodes.h         |   4 +
include/linux/audit.h               |  26 ++
include/linux/lsm_hook_defs.h       |  22 +-
include/linux/lsm_hooks.h           |  30 ++-
include/linux/security.h            |  55 +++--
include/uapi/linux/audit.h          |   4 +-
kernel/audit.h                      |   7 +-
kernel/audit_tree.c                 |   3 +-
kernel/audit_watch.c                |   3 +-
kernel/auditfilter.c                |  15 +-
kernel/auditsc.c                    | 468 +++++++++++++++++++++++++-----
security/security.c                 |  35 ++-
security/selinux/avc.c              |  13 +-
security/selinux/hooks.c            | 239 +++++++-----------
security/selinux/include/classmap.h |   4 +-
security/selinux/netlabel.c         |   7 +-
security/selinux/netport.c          |   2 +-
security/selinux/ss/hashtab.c       |   1 +
security/selinux/ss/mls.c           |   4 +
security/selinux/ss/services.c      | 176 +++++++-------
security/smack/smack_lsm.c          |  46 ++++
28 files changed, 882 insertions(+), 421 deletions(-)

Comments

Linus Torvalds Nov. 2, 2021, 12:44 a.m. UTC | #1
On Mon, Nov 1, 2021 at 4:59 PM Paul Moore <paul@paul-moore.com> 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
Paul Moore Nov. 2, 2021, 3:13 a.m. UTC | #2
On Mon, Nov 1, 2021 at 8:44 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Mon, Nov 1, 2021 at 4:59 PM Paul Moore <paul@paul-moore.com> 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.

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
the cover letter explicitly asking for their ACKs.

*** Third Revision (Sept 2021)
https://lore.kernel.org/linux-security-module/163159032713.470089.11728103630366176255.stgit@olly/

The third revision only had minor changes compared to the second, no
direct comments to this revision although related comments continued
to be made on prevision revisions.  The io-uring developers continue
to be on the To/CC line, with no comments.

*** Fourth Revision (Sept 2021)
https://lore.kernel.org/linux-security-module/163172413301.88001.16054830862146685573.stgit@olly/

The fourth revision also only had minor changes.  The patchset
continued to keep the io_uring devs on the To/CC line and there was an
explicit plea to ask for their review/ACK/etc. but none was ever sent.

> 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.
Linus Torvalds Nov. 2, 2021, 3:55 a.m. UTC | #3
On Mon, Nov 1, 2021 at 8:13 PM Paul Moore <paul@paul-moore.com> 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
pr-tracker-bot@kernel.org Nov. 2, 2021, 4:21 a.m. UTC | #4
The pull request you sent on Mon, 1 Nov 2021 19:59:02 -0400:

> git://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git tags/selinux-pr-20211101

has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/cdab10bf3285ee354e8f50254aa799631b7a95e0

Thank you!
Linus Torvalds Nov. 2, 2021, 4:22 a.m. UTC | #5
On Mon, Nov 1, 2021 at 8:55 PM Linus Torvalds
<torvalds@linux-foundation.org> 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
Paul Moore Nov. 2, 2021, 12:58 p.m. UTC | #6
On Tue, Nov 2, 2021 at 12:23 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Mon, Nov 1, 2021 at 8:55 PM Linus Torvalds
> <torvalds@linux-foundation.org> 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.

The important part is that it is now in your tree, thanks.