Re: [PATCH] selinux: reorder hooks to make runtime disable less broken

From: Casey Schaufler <casey@schaufler-ca.com>
To: Stephen Smalley <sds@tycho.nsa.gov>,
	Ondrej Mosnacek <omosnace@redhat.com>,
	selinux@vger.kernel.org, Paul Moore <paul@paul-moore.com>,
	Casey Schaufler <casey@schaufler-ca.com>
Subject: Re: [PATCH] selinux: reorder hooks to make runtime disable less broken
Date: Mon, 9 Dec 2019 08:32:21 -0800	[thread overview]
Message-ID: <3285ac79-5d26-71bf-6fd7-dc1895ee7bbc@schaufler-ca.com> (raw)
In-Reply-To: <23671223-f841-564c-6ae8-0401bce0fa20@tycho.nsa.gov>

On 12/9/2019 5:21 AM, Stephen Smalley wrote:
> On 12/9/19 2:57 AM, Ondrej Mosnacek wrote:
>> Commit b1d9e6b0646d ("LSM: Switch to lists of hooks") switched the LSM
>> infrastructure to use per-hook lists, which meant that removing the
>> hooks for a given module was no longer atomic. Even though the commit
>> clearly documents that modules implementing runtime revmoval of hooks
>> (only SELinux attempts this madness) need to take special precautions to
>> avoid race conditions, SELinux has never addressed this.
>>
>> By inserting an artificial delay between the loop iterations of
>> security_delete_hooks() (I used 100 ms), booting to a state where
>> SELinux is enabled, but policy is not yet loaded, and running these
>> commands:
>>
>>      while true; do ping -c 1 <some IP>; done &
>>      echo -n 1 >/sys/fs/selinux/disable
>>      kill %1
>>      wait
>>
>> ...I was able to trigger NULL pointer dereferences in various places. I
>> also have a report of someone getting panics on a stock RHEL-8 kernel
>> after setting SELINUX=disabled in /etc/selinux/config and rebooting
>> (without adding "selinux=0" to kernel command-line).
>>
>> Reordering the SELinux hooks such that those that allocate structures
>> are removed last seems to prevent these panics. It is very much possible
>> that this doesn't make the runtime disable completely race-free, but at
>> least it makes the operation much less fragile.
>>
>> An alternative (and safer) solution would be to add NULL checks to each
>> hook, but doing this just to support the runtime disable hack doesn't
>> seem to be worth the effort...
>
> Personally, I would prefer to just get rid of runtime disable altogether; it also precludes making the hooks read-only after initialization.  IMHO, selinux=0 is the proper way to disable SELinux if necessary.  I believe there is an open bugzilla on Fedora related to this issue, since runtime disable was originally introduced for Fedora.

I agree completely with Stephen. There was significant discussion at the time
of switching to hook lists about this. My recollection is that there was consensus
that removing security_delete_hooks() was the way of the future. If anything was
to be done it would be removal. That was some time ago now, and something may
have changed in the usage patterns. We have "Linux Security Modules" instead of
"Loadable Security Modules" in large part due to the inherent difficulty in
ensuring that removing a security policy implementation on the fly can be
done safely, much less securely. 