| Message ID | CAHC9VhRJ=fHzMHM6tt8JqkZa4bf0h72CAytSX9YrEs14Oaj8SA@mail.gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [GIT,PULL] SELinux patches for v5.16 | expand |
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
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.
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
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!
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
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.
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(-)