Re: [PATCH] selinux: fix residual uses of current_security() for the SELinux blob

[Stephen Smalley <sds@tycho.nsa.gov>]

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.