From: Stephen Smalley <firstname.lastname@example.org> To: Paul Moore <email@example.com> Cc: firstname.lastname@example.org, email@example.com, firstname.lastname@example.org, email@example.com, James Morris <firstname.lastname@example.org>, email@example.com Subject: Re: [PATCH] selinux: fix residual uses of current_security() for the SELinux blob Date: Thu, 5 Sep 2019 16:01:17 -0400 Message-ID: <firstname.lastname@example.org> (raw) In-Reply-To: <CAHC9VhTdv2C2MbNb7p-cPAW8ZeY+tmcz1c77qXmvowSxrWbw4g@mail.gmail.com> On 9/4/19 6:50 PM, Paul Moore wrote: > On Wed, Sep 4, 2019 at 10:32 AM Stephen Smalley <email@example.com> wrote: >> We need to use selinux_cred() to fetch the SELinux cred blob instead >> of directly using current->security or current_security(). There >> were a couple of lingering uses of current_security() in the SELinux code >> that were apparently missed during the earlier conversions. IIUC, this >> would only manifest as a bug if multiple security modules including >> SELinux are enabled and SELinux is not first in the lsm order. After >> this change, there appear to be no other users of current_security() >> in-tree; perhaps we should remove it altogether. >> >> Fixes: bbd3662a8348 ("Infrastructure management of the cred security blob") >> Signed-off-by: Stephen Smalley <firstname.lastname@example.org> >> --- >> security/selinux/hooks.c | 2 +- >> security/selinux/include/objsec.h | 20 ++++++++++---------- >> 2 files changed, 11 insertions(+), 11 deletions(-) > > Thanks Stephen, and everyone who reviewed/commented on the patch. > This looks fine to me too, and while it is a little late, I think > there is value in getting this into the next merge window so I've gone > ahead and merged this into selinux/next. > > As far as removing current_security is concerned, I also agree that > removing it is probably a good idea. Does anyone object if I merge a > follow-up patch via the SELinux tree (patch coming shortly)? Obviously no objection to the follow-up patch. I had another concern though raised by this bug that I wanted to note. AFAICS, no SELinux maintainer ever acked or reviewed the commit that introduced the bug, or the other patches in that series. In fact that commit and many others in the series only have a single Reviewed-by line and no Acked-by lines at all. I guess the SELinux maintainers (self included) could/should have been more proactive but maybe there should have been a final poke at each of the security module maintainers to provide at least an ack before it was merged or at least sent up to Linus? I've also seen a tendency to assume that passing the selinux-testsuite suffices for ensuring that the patch is correct wrt SELinux. If it wasn't already clear, the selinux-testsuite is by no means complete in its coverage (contributions welcome; known gaps captured as open issues in github), so passing the testsuite is no substitute for code review. For the next and any future rounds of stacking support, I'm hoping we can be a bit more rigorous in our code review and testing requirements.
next prev parent reply index Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-09-04 14:32 Stephen Smalley 2019-09-04 15:16 ` Casey Schaufler 2019-09-04 15:31 ` Stephen Smalley 2019-09-04 16:46 ` John Johansen 2019-09-04 19:35 ` James Morris 2019-09-04 22:50 ` Paul Moore 2019-09-05 20:01 ` Stephen Smalley [this message] 2019-09-05 20:55 ` James Morris
Reply instructions: You may reply publically to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
Linux-Security-Module Archive on lore.kernel.org Archives are clonable: git clone --mirror https://lore.kernel.org/linux-security-module/0 linux-security-module/git/0.git # If you have public-inbox 1.1+ installed, you may # initialize and index your mirror using the following commands: public-inbox-init -V2 linux-security-module linux-security-module/ https://lore.kernel.org/linux-security-module \ firstname.lastname@example.org public-inbox-index linux-security-module Example config snippet for mirrors Newsgroup available over NNTP: nntp://nntp.lore.kernel.org/org.kernel.vger.linux-security-module AGPL code for this site: git clone https://public-inbox.org/public-inbox.git