Linux-Security-Module Archive on lore.kernel.org
 help / color / Atom feed
From: Stephen Smalley <sds@tycho.nsa.gov>
To: Paul Moore <paul@paul-moore.com>
Cc: keescook@chromium.org, casey@schaufler-ca.com,
	selinux@vger.kernel.org, linux-security-module@vger.kernel.org,
	James Morris <jmorris@namei.org>,
	dhowells@redhat.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: <b5705661-089a-cb9c-53b5-21b855937638@tycho.nsa.gov> (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 <sds@tycho.nsa.gov> 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 <sds@tycho.nsa.gov>
>> ---
>>   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.

  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 \
    --in-reply-to=b5705661-089a-cb9c-53b5-21b855937638@tycho.nsa.gov \
    --to=sds@tycho.nsa.gov \
    --cc=casey@schaufler-ca.com \
    --cc=dhowells@redhat.com \
    --cc=jmorris@namei.org \
    --cc=keescook@chromium.org \
    --cc=linux-security-module@vger.kernel.org \
    --cc=paul@paul-moore.com \
    --cc=selinux@vger.kernel.org \
    /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 \
		linux-security-module@vger.kernel.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