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

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

[Ondrej Mosnacek]

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...

Fixes: b1d9e6b0646d ("LSM: Switch to lists of hooks")
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
 security/selinux/hooks.c | 97 +++++++++++++++++++++++++++-------------
 1 file changed, 66 insertions(+), 31 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 116b4d644f68..5075be8eea2a 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -6864,6 +6864,21 @@ static int selinux_perf_event_write(struct perf_event *event)
 }
 #endif
 
+/*
+ * IMPORTANT NOTE: When adding new hooks, please be careful to keep this order:
+ * 1. any hooks that don't belong to (2.) or (3.) below,
+ * 2. hooks that both access structures allocated by other hooks, and allocate
+ *    structures that can be later accessed by other hooks (mostly "cloning"
+ *    hooks),
+ * 3. hooks that only allocate structures that can be later accessed by other
+ *    hooks ("allocating" hooks).
+ *
+ * Please follow block comment delimiters in the list to keep this order.
+ *
+ * This ordering is needed for SELinux runtime disable to work at least somewhat
+ * safely. Breaking the ordering rules above might lead to NULL pointer derefs
+ * when disabling SELinux at runtime.
+ */
 static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
 	LSM_HOOK_INIT(binder_set_context_mgr, selinux_binder_set_context_mgr),
 	LSM_HOOK_INIT(binder_transaction, selinux_binder_transaction),
@@ -6886,12 +6901,7 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
 	LSM_HOOK_INIT(bprm_committing_creds, selinux_bprm_committing_creds),
 	LSM_HOOK_INIT(bprm_committed_creds, selinux_bprm_committed_creds),
 
-	LSM_HOOK_INIT(fs_context_dup, selinux_fs_context_dup),
-	LSM_HOOK_INIT(fs_context_parse_param, selinux_fs_context_parse_param),
-
-	LSM_HOOK_INIT(sb_alloc_security, selinux_sb_alloc_security),
 	LSM_HOOK_INIT(sb_free_security, selinux_sb_free_security),
-	LSM_HOOK_INIT(sb_eat_lsm_opts, selinux_sb_eat_lsm_opts),
 	LSM_HOOK_INIT(sb_free_mnt_opts, selinux_free_mnt_opts),
 	LSM_HOOK_INIT(sb_remount, selinux_sb_remount),
 	LSM_HOOK_INIT(sb_kern_mount, selinux_sb_kern_mount),
@@ -6901,12 +6911,10 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
 	LSM_HOOK_INIT(sb_umount, selinux_umount),
 	LSM_HOOK_INIT(sb_set_mnt_opts, selinux_set_mnt_opts),
 	LSM_HOOK_INIT(sb_clone_mnt_opts, selinux_sb_clone_mnt_opts),
-	LSM_HOOK_INIT(sb_add_mnt_opt, selinux_add_mnt_opt),
 
 	LSM_HOOK_INIT(dentry_init_security, selinux_dentry_init_security),
 	LSM_HOOK_INIT(dentry_create_files_as, selinux_dentry_create_files_as),
 
-	LSM_HOOK_INIT(inode_alloc_security, selinux_inode_alloc_security),
 	LSM_HOOK_INIT(inode_free_security, selinux_inode_free_security),
 	LSM_HOOK_INIT(inode_init_security, selinux_inode_init_security),
 	LSM_HOOK_INIT(inode_create, selinux_inode_create),
@@ -6978,21 +6986,15 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
 	LSM_HOOK_INIT(ipc_permission, selinux_ipc_permission),
 	LSM_HOOK_INIT(ipc_getsecid, selinux_ipc_getsecid),
 
-	LSM_HOOK_INIT(msg_msg_alloc_security, selinux_msg_msg_alloc_security),
-
-	LSM_HOOK_INIT(msg_queue_alloc_security,
-			selinux_msg_queue_alloc_security),
 	LSM_HOOK_INIT(msg_queue_associate, selinux_msg_queue_associate),
 	LSM_HOOK_INIT(msg_queue_msgctl, selinux_msg_queue_msgctl),
 	LSM_HOOK_INIT(msg_queue_msgsnd, selinux_msg_queue_msgsnd),
 	LSM_HOOK_INIT(msg_queue_msgrcv, selinux_msg_queue_msgrcv),
 
-	LSM_HOOK_INIT(shm_alloc_security, selinux_shm_alloc_security),
 	LSM_HOOK_INIT(shm_associate, selinux_shm_associate),
 	LSM_HOOK_INIT(shm_shmctl, selinux_shm_shmctl),
 	LSM_HOOK_INIT(shm_shmat, selinux_shm_shmat),
 
-	LSM_HOOK_INIT(sem_alloc_security, selinux_sem_alloc_security),
 	LSM_HOOK_INIT(sem_associate, selinux_sem_associate),
 	LSM_HOOK_INIT(sem_semctl, selinux_sem_semctl),
 	LSM_HOOK_INIT(sem_semop, selinux_sem_semop),
@@ -7003,13 +7005,11 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
 	LSM_HOOK_INIT(setprocattr, selinux_setprocattr),
 
 	LSM_HOOK_INIT(ismaclabel, selinux_ismaclabel),
-	LSM_HOOK_INIT(secid_to_secctx, selinux_secid_to_secctx),
 	LSM_HOOK_INIT(secctx_to_secid, selinux_secctx_to_secid),
 	LSM_HOOK_INIT(release_secctx, selinux_release_secctx),
 	LSM_HOOK_INIT(inode_invalidate_secctx, selinux_inode_invalidate_secctx),
 	LSM_HOOK_INIT(inode_notifysecctx, selinux_inode_notifysecctx),
 	LSM_HOOK_INIT(inode_setsecctx, selinux_inode_setsecctx),
-	LSM_HOOK_INIT(inode_getsecctx, selinux_inode_getsecctx),
 
 	LSM_HOOK_INIT(unix_stream_connect, selinux_socket_unix_stream_connect),
 	LSM_HOOK_INIT(unix_may_send, selinux_socket_unix_may_send),
@@ -7032,7 +7032,6 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
 	LSM_HOOK_INIT(socket_getpeersec_stream,
 			selinux_socket_getpeersec_stream),
 	LSM_HOOK_INIT(socket_getpeersec_dgram, selinux_socket_getpeersec_dgram),
-	LSM_HOOK_INIT(sk_alloc_security, selinux_sk_alloc_security),
 	LSM_HOOK_INIT(sk_free_security, selinux_sk_free_security),
 	LSM_HOOK_INIT(sk_clone_security, selinux_sk_clone_security),
 	LSM_HOOK_INIT(sk_getsecid, selinux_sk_getsecid),
@@ -7047,7 +7046,6 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
 	LSM_HOOK_INIT(secmark_refcount_inc, selinux_secmark_refcount_inc),
 	LSM_HOOK_INIT(secmark_refcount_dec, selinux_secmark_refcount_dec),
 	LSM_HOOK_INIT(req_classify_flow, selinux_req_classify_flow),
-	LSM_HOOK_INIT(tun_dev_alloc_security, selinux_tun_dev_alloc_security),
 	LSM_HOOK_INIT(tun_dev_free_security, selinux_tun_dev_free_security),
 	LSM_HOOK_INIT(tun_dev_create, selinux_tun_dev_create),
 	LSM_HOOK_INIT(tun_dev_attach_queue, selinux_tun_dev_attach_queue),
@@ -7057,17 +7055,11 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
 	LSM_HOOK_INIT(ib_pkey_access, selinux_ib_pkey_access),
 	LSM_HOOK_INIT(ib_endport_manage_subnet,
 		      selinux_ib_endport_manage_subnet),
-	LSM_HOOK_INIT(ib_alloc_security, selinux_ib_alloc_security),
 	LSM_HOOK_INIT(ib_free_security, selinux_ib_free_security),
 #endif
 #ifdef CONFIG_SECURITY_NETWORK_XFRM
-	LSM_HOOK_INIT(xfrm_policy_alloc_security, selinux_xfrm_policy_alloc),
-	LSM_HOOK_INIT(xfrm_policy_clone_security, selinux_xfrm_policy_clone),
 	LSM_HOOK_INIT(xfrm_policy_free_security, selinux_xfrm_policy_free),
 	LSM_HOOK_INIT(xfrm_policy_delete_security, selinux_xfrm_policy_delete),
-	LSM_HOOK_INIT(xfrm_state_alloc, selinux_xfrm_state_alloc),
-	LSM_HOOK_INIT(xfrm_state_alloc_acquire,
-			selinux_xfrm_state_alloc_acquire),
 	LSM_HOOK_INIT(xfrm_state_free_security, selinux_xfrm_state_free),
 	LSM_HOOK_INIT(xfrm_state_delete_security, selinux_xfrm_state_delete),
 	LSM_HOOK_INIT(xfrm_policy_lookup, selinux_xfrm_policy_lookup),
@@ -7077,14 +7069,12 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
 #endif
 
 #ifdef CONFIG_KEYS
-	LSM_HOOK_INIT(key_alloc, selinux_key_alloc),
 	LSM_HOOK_INIT(key_free, selinux_key_free),
 	LSM_HOOK_INIT(key_permission, selinux_key_permission),
 	LSM_HOOK_INIT(key_getsecurity, selinux_key_getsecurity),
 #endif
 
 #ifdef CONFIG_AUDIT
-	LSM_HOOK_INIT(audit_rule_init, selinux_audit_rule_init),
 	LSM_HOOK_INIT(audit_rule_known, selinux_audit_rule_known),
 	LSM_HOOK_INIT(audit_rule_match, selinux_audit_rule_match),
 	LSM_HOOK_INIT(audit_rule_free, selinux_audit_rule_free),
@@ -7094,19 +7084,64 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
 	LSM_HOOK_INIT(bpf, selinux_bpf),
 	LSM_HOOK_INIT(bpf_map, selinux_bpf_map),
 	LSM_HOOK_INIT(bpf_prog, selinux_bpf_prog),
-	LSM_HOOK_INIT(bpf_map_alloc_security, selinux_bpf_map_alloc),
-	LSM_HOOK_INIT(bpf_prog_alloc_security, selinux_bpf_prog_alloc),
 	LSM_HOOK_INIT(bpf_map_free_security, selinux_bpf_map_free),
 	LSM_HOOK_INIT(bpf_prog_free_security, selinux_bpf_prog_free),
 #endif
 
 #ifdef CONFIG_PERF_EVENTS
 	LSM_HOOK_INIT(perf_event_open, selinux_perf_event_open),
-	LSM_HOOK_INIT(perf_event_alloc, selinux_perf_event_alloc),
 	LSM_HOOK_INIT(perf_event_free, selinux_perf_event_free),
 	LSM_HOOK_INIT(perf_event_read, selinux_perf_event_read),
 	LSM_HOOK_INIT(perf_event_write, selinux_perf_event_write),
 #endif
+
+	/*
+	 * PUT "CLONING" (ACCESSING + ALLOCATING) HOOKS HERE
+	 */
+	LSM_HOOK_INIT(fs_context_dup, selinux_fs_context_dup),
+	LSM_HOOK_INIT(fs_context_parse_param, selinux_fs_context_parse_param),
+	LSM_HOOK_INIT(sb_eat_lsm_opts, selinux_sb_eat_lsm_opts),
+	LSM_HOOK_INIT(sb_add_mnt_opt, selinux_add_mnt_opt),
+#ifdef CONFIG_SECURITY_NETWORK_XFRM
+	LSM_HOOK_INIT(xfrm_policy_clone_security, selinux_xfrm_policy_clone),
+#endif
+
+	/*
+	 * PUT "ALLOCATING" HOOKS HERE
+	 */
+	LSM_HOOK_INIT(msg_msg_alloc_security, selinux_msg_msg_alloc_security),
+	LSM_HOOK_INIT(msg_queue_alloc_security,
+		      selinux_msg_queue_alloc_security),
+	LSM_HOOK_INIT(shm_alloc_security, selinux_shm_alloc_security),
+	LSM_HOOK_INIT(sb_alloc_security, selinux_sb_alloc_security),
+	LSM_HOOK_INIT(inode_alloc_security, selinux_inode_alloc_security),
+	LSM_HOOK_INIT(sem_alloc_security, selinux_sem_alloc_security),
+	LSM_HOOK_INIT(secid_to_secctx, selinux_secid_to_secctx),
+	LSM_HOOK_INIT(inode_getsecctx, selinux_inode_getsecctx),
+	LSM_HOOK_INIT(sk_alloc_security, selinux_sk_alloc_security),
+	LSM_HOOK_INIT(tun_dev_alloc_security, selinux_tun_dev_alloc_security),
+#ifdef CONFIG_SECURITY_INFINIBAND
+	LSM_HOOK_INIT(ib_alloc_security, selinux_ib_alloc_security),
+#endif
+#ifdef CONFIG_SECURITY_NETWORK_XFRM
+	LSM_HOOK_INIT(xfrm_policy_alloc_security, selinux_xfrm_policy_alloc),
+	LSM_HOOK_INIT(xfrm_state_alloc, selinux_xfrm_state_alloc),
+	LSM_HOOK_INIT(xfrm_state_alloc_acquire,
+		      selinux_xfrm_state_alloc_acquire),
+#endif
+#ifdef CONFIG_KEYS
+	LSM_HOOK_INIT(key_alloc, selinux_key_alloc),
+#endif
+#ifdef CONFIG_AUDIT
+	LSM_HOOK_INIT(audit_rule_init, selinux_audit_rule_init),
+#endif
+#ifdef CONFIG_BPF_SYSCALL
+	LSM_HOOK_INIT(bpf_map_alloc_security, selinux_bpf_map_alloc),
+	LSM_HOOK_INIT(bpf_prog_alloc_security, selinux_bpf_prog_alloc),
+#endif
+#ifdef CONFIG_PERF_EVENTS
+	LSM_HOOK_INIT(perf_event_alloc, selinux_perf_event_alloc),
+#endif
 };
 
 static __init int selinux_init(void)
@@ -7287,14 +7322,14 @@ int selinux_disable(struct selinux_state *state)
 
 	selinux_enabled = 0;
 
+	/* Unregister netfilter hooks. */
+	selinux_nf_ip_exit();
+
 	security_delete_hooks(selinux_hooks, ARRAY_SIZE(selinux_hooks));
 
 	/* Try to destroy the avc node cache */
 	avc_disable();
 
-	/* Unregister netfilter hooks. */
-	selinux_nf_ip_exit();
-
 	/* Unregister selinuxfs. */
 	exit_sel_fs();
 
-- 
2.23.0

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

[Ondrej Mosnacek]

On Mon, Dec 9, 2019 at 8:57 AM Ondrej Mosnacek <omosnace@redhat.com> 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...
>
> Fixes: b1d9e6b0646d ("LSM: Switch to lists of hooks")
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> ---
>  security/selinux/hooks.c | 97 +++++++++++++++++++++++++++-------------
>  1 file changed, 66 insertions(+), 31 deletions(-)

Please note that this patch applies on top of Linus' tree instead of
selinux/next, since there are some new hooks there that aren't in
next.

-- 
Ondrej Mosnacek <omosnace at redhat dot com>
Software Engineer, Security Technologies
Red Hat, Inc.

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

[Stephen Smalley]

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.

> 
> Fixes: b1d9e6b0646d ("LSM: Switch to lists of hooks")
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> ---
>   security/selinux/hooks.c | 97 +++++++++++++++++++++++++++-------------
>   1 file changed, 66 insertions(+), 31 deletions(-)
> 
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 116b4d644f68..5075be8eea2a 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -6864,6 +6864,21 @@ static int selinux_perf_event_write(struct perf_event *event)
>   }
>   #endif
>   
> +/*
> + * IMPORTANT NOTE: When adding new hooks, please be careful to keep this order:
> + * 1. any hooks that don't belong to (2.) or (3.) below,
> + * 2. hooks that both access structures allocated by other hooks, and allocate
> + *    structures that can be later accessed by other hooks (mostly "cloning"
> + *    hooks),
> + * 3. hooks that only allocate structures that can be later accessed by other
> + *    hooks ("allocating" hooks).
> + *
> + * Please follow block comment delimiters in the list to keep this order.
> + *
> + * This ordering is needed for SELinux runtime disable to work at least somewhat
> + * safely. Breaking the ordering rules above might lead to NULL pointer derefs
> + * when disabling SELinux at runtime.
> + */
>   static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
>   	LSM_HOOK_INIT(binder_set_context_mgr, selinux_binder_set_context_mgr),
>   	LSM_HOOK_INIT(binder_transaction, selinux_binder_transaction),
> @@ -6886,12 +6901,7 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
>   	LSM_HOOK_INIT(bprm_committing_creds, selinux_bprm_committing_creds),
>   	LSM_HOOK_INIT(bprm_committed_creds, selinux_bprm_committed_creds),
>   
> -	LSM_HOOK_INIT(fs_context_dup, selinux_fs_context_dup),
> -	LSM_HOOK_INIT(fs_context_parse_param, selinux_fs_context_parse_param),
> -
> -	LSM_HOOK_INIT(sb_alloc_security, selinux_sb_alloc_security),
>   	LSM_HOOK_INIT(sb_free_security, selinux_sb_free_security),
> -	LSM_HOOK_INIT(sb_eat_lsm_opts, selinux_sb_eat_lsm_opts),
>   	LSM_HOOK_INIT(sb_free_mnt_opts, selinux_free_mnt_opts),
>   	LSM_HOOK_INIT(sb_remount, selinux_sb_remount),
>   	LSM_HOOK_INIT(sb_kern_mount, selinux_sb_kern_mount),
> @@ -6901,12 +6911,10 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
>   	LSM_HOOK_INIT(sb_umount, selinux_umount),
>   	LSM_HOOK_INIT(sb_set_mnt_opts, selinux_set_mnt_opts),
>   	LSM_HOOK_INIT(sb_clone_mnt_opts, selinux_sb_clone_mnt_opts),
> -	LSM_HOOK_INIT(sb_add_mnt_opt, selinux_add_mnt_opt),
>   
>   	LSM_HOOK_INIT(dentry_init_security, selinux_dentry_init_security),
>   	LSM_HOOK_INIT(dentry_create_files_as, selinux_dentry_create_files_as),
>   
> -	LSM_HOOK_INIT(inode_alloc_security, selinux_inode_alloc_security),
>   	LSM_HOOK_INIT(inode_free_security, selinux_inode_free_security),
>   	LSM_HOOK_INIT(inode_init_security, selinux_inode_init_security),
>   	LSM_HOOK_INIT(inode_create, selinux_inode_create),
> @@ -6978,21 +6986,15 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
>   	LSM_HOOK_INIT(ipc_permission, selinux_ipc_permission),
>   	LSM_HOOK_INIT(ipc_getsecid, selinux_ipc_getsecid),
>   
> -	LSM_HOOK_INIT(msg_msg_alloc_security, selinux_msg_msg_alloc_security),
> -
> -	LSM_HOOK_INIT(msg_queue_alloc_security,
> -			selinux_msg_queue_alloc_security),
>   	LSM_HOOK_INIT(msg_queue_associate, selinux_msg_queue_associate),
>   	LSM_HOOK_INIT(msg_queue_msgctl, selinux_msg_queue_msgctl),
>   	LSM_HOOK_INIT(msg_queue_msgsnd, selinux_msg_queue_msgsnd),
>   	LSM_HOOK_INIT(msg_queue_msgrcv, selinux_msg_queue_msgrcv),
>   
> -	LSM_HOOK_INIT(shm_alloc_security, selinux_shm_alloc_security),
>   	LSM_HOOK_INIT(shm_associate, selinux_shm_associate),
>   	LSM_HOOK_INIT(shm_shmctl, selinux_shm_shmctl),
>   	LSM_HOOK_INIT(shm_shmat, selinux_shm_shmat),
>   
> -	LSM_HOOK_INIT(sem_alloc_security, selinux_sem_alloc_security),
>   	LSM_HOOK_INIT(sem_associate, selinux_sem_associate),
>   	LSM_HOOK_INIT(sem_semctl, selinux_sem_semctl),
>   	LSM_HOOK_INIT(sem_semop, selinux_sem_semop),
> @@ -7003,13 +7005,11 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
>   	LSM_HOOK_INIT(setprocattr, selinux_setprocattr),
>   
>   	LSM_HOOK_INIT(ismaclabel, selinux_ismaclabel),
> -	LSM_HOOK_INIT(secid_to_secctx, selinux_secid_to_secctx),
>   	LSM_HOOK_INIT(secctx_to_secid, selinux_secctx_to_secid),
>   	LSM_HOOK_INIT(release_secctx, selinux_release_secctx),
>   	LSM_HOOK_INIT(inode_invalidate_secctx, selinux_inode_invalidate_secctx),
>   	LSM_HOOK_INIT(inode_notifysecctx, selinux_inode_notifysecctx),
>   	LSM_HOOK_INIT(inode_setsecctx, selinux_inode_setsecctx),
> -	LSM_HOOK_INIT(inode_getsecctx, selinux_inode_getsecctx),
>   
>   	LSM_HOOK_INIT(unix_stream_connect, selinux_socket_unix_stream_connect),
>   	LSM_HOOK_INIT(unix_may_send, selinux_socket_unix_may_send),
> @@ -7032,7 +7032,6 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
>   	LSM_HOOK_INIT(socket_getpeersec_stream,
>   			selinux_socket_getpeersec_stream),
>   	LSM_HOOK_INIT(socket_getpeersec_dgram, selinux_socket_getpeersec_dgram),
> -	LSM_HOOK_INIT(sk_alloc_security, selinux_sk_alloc_security),
>   	LSM_HOOK_INIT(sk_free_security, selinux_sk_free_security),
>   	LSM_HOOK_INIT(sk_clone_security, selinux_sk_clone_security),
>   	LSM_HOOK_INIT(sk_getsecid, selinux_sk_getsecid),
> @@ -7047,7 +7046,6 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
>   	LSM_HOOK_INIT(secmark_refcount_inc, selinux_secmark_refcount_inc),
>   	LSM_HOOK_INIT(secmark_refcount_dec, selinux_secmark_refcount_dec),
>   	LSM_HOOK_INIT(req_classify_flow, selinux_req_classify_flow),
> -	LSM_HOOK_INIT(tun_dev_alloc_security, selinux_tun_dev_alloc_security),
>   	LSM_HOOK_INIT(tun_dev_free_security, selinux_tun_dev_free_security),
>   	LSM_HOOK_INIT(tun_dev_create, selinux_tun_dev_create),
>   	LSM_HOOK_INIT(tun_dev_attach_queue, selinux_tun_dev_attach_queue),
> @@ -7057,17 +7055,11 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
>   	LSM_HOOK_INIT(ib_pkey_access, selinux_ib_pkey_access),
>   	LSM_HOOK_INIT(ib_endport_manage_subnet,
>   		      selinux_ib_endport_manage_subnet),
> -	LSM_HOOK_INIT(ib_alloc_security, selinux_ib_alloc_security),
>   	LSM_HOOK_INIT(ib_free_security, selinux_ib_free_security),
>   #endif
>   #ifdef CONFIG_SECURITY_NETWORK_XFRM
> -	LSM_HOOK_INIT(xfrm_policy_alloc_security, selinux_xfrm_policy_alloc),
> -	LSM_HOOK_INIT(xfrm_policy_clone_security, selinux_xfrm_policy_clone),
>   	LSM_HOOK_INIT(xfrm_policy_free_security, selinux_xfrm_policy_free),
>   	LSM_HOOK_INIT(xfrm_policy_delete_security, selinux_xfrm_policy_delete),
> -	LSM_HOOK_INIT(xfrm_state_alloc, selinux_xfrm_state_alloc),
> -	LSM_HOOK_INIT(xfrm_state_alloc_acquire,
> -			selinux_xfrm_state_alloc_acquire),
>   	LSM_HOOK_INIT(xfrm_state_free_security, selinux_xfrm_state_free),
>   	LSM_HOOK_INIT(xfrm_state_delete_security, selinux_xfrm_state_delete),
>   	LSM_HOOK_INIT(xfrm_policy_lookup, selinux_xfrm_policy_lookup),
> @@ -7077,14 +7069,12 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
>   #endif
>   
>   #ifdef CONFIG_KEYS
> -	LSM_HOOK_INIT(key_alloc, selinux_key_alloc),
>   	LSM_HOOK_INIT(key_free, selinux_key_free),
>   	LSM_HOOK_INIT(key_permission, selinux_key_permission),
>   	LSM_HOOK_INIT(key_getsecurity, selinux_key_getsecurity),
>   #endif
>   
>   #ifdef CONFIG_AUDIT
> -	LSM_HOOK_INIT(audit_rule_init, selinux_audit_rule_init),
>   	LSM_HOOK_INIT(audit_rule_known, selinux_audit_rule_known),
>   	LSM_HOOK_INIT(audit_rule_match, selinux_audit_rule_match),
>   	LSM_HOOK_INIT(audit_rule_free, selinux_audit_rule_free),
> @@ -7094,19 +7084,64 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
>   	LSM_HOOK_INIT(bpf, selinux_bpf),
>   	LSM_HOOK_INIT(bpf_map, selinux_bpf_map),
>   	LSM_HOOK_INIT(bpf_prog, selinux_bpf_prog),
> -	LSM_HOOK_INIT(bpf_map_alloc_security, selinux_bpf_map_alloc),
> -	LSM_HOOK_INIT(bpf_prog_alloc_security, selinux_bpf_prog_alloc),
>   	LSM_HOOK_INIT(bpf_map_free_security, selinux_bpf_map_free),
>   	LSM_HOOK_INIT(bpf_prog_free_security, selinux_bpf_prog_free),
>   #endif
>   
>   #ifdef CONFIG_PERF_EVENTS
>   	LSM_HOOK_INIT(perf_event_open, selinux_perf_event_open),
> -	LSM_HOOK_INIT(perf_event_alloc, selinux_perf_event_alloc),
>   	LSM_HOOK_INIT(perf_event_free, selinux_perf_event_free),
>   	LSM_HOOK_INIT(perf_event_read, selinux_perf_event_read),
>   	LSM_HOOK_INIT(perf_event_write, selinux_perf_event_write),
>   #endif
> +
> +	/*
> +	 * PUT "CLONING" (ACCESSING + ALLOCATING) HOOKS HERE
> +	 */
> +	LSM_HOOK_INIT(fs_context_dup, selinux_fs_context_dup),
> +	LSM_HOOK_INIT(fs_context_parse_param, selinux_fs_context_parse_param),
> +	LSM_HOOK_INIT(sb_eat_lsm_opts, selinux_sb_eat_lsm_opts),
> +	LSM_HOOK_INIT(sb_add_mnt_opt, selinux_add_mnt_opt),
> +#ifdef CONFIG_SECURITY_NETWORK_XFRM
> +	LSM_HOOK_INIT(xfrm_policy_clone_security, selinux_xfrm_policy_clone),
> +#endif
> +
> +	/*
> +	 * PUT "ALLOCATING" HOOKS HERE
> +	 */
> +	LSM_HOOK_INIT(msg_msg_alloc_security, selinux_msg_msg_alloc_security),
> +	LSM_HOOK_INIT(msg_queue_alloc_security,
> +		      selinux_msg_queue_alloc_security),
> +	LSM_HOOK_INIT(shm_alloc_security, selinux_shm_alloc_security),
> +	LSM_HOOK_INIT(sb_alloc_security, selinux_sb_alloc_security),
> +	LSM_HOOK_INIT(inode_alloc_security, selinux_inode_alloc_security),
> +	LSM_HOOK_INIT(sem_alloc_security, selinux_sem_alloc_security),
> +	LSM_HOOK_INIT(secid_to_secctx, selinux_secid_to_secctx),
> +	LSM_HOOK_INIT(inode_getsecctx, selinux_inode_getsecctx),
> +	LSM_HOOK_INIT(sk_alloc_security, selinux_sk_alloc_security),
> +	LSM_HOOK_INIT(tun_dev_alloc_security, selinux_tun_dev_alloc_security),
> +#ifdef CONFIG_SECURITY_INFINIBAND
> +	LSM_HOOK_INIT(ib_alloc_security, selinux_ib_alloc_security),
> +#endif
> +#ifdef CONFIG_SECURITY_NETWORK_XFRM
> +	LSM_HOOK_INIT(xfrm_policy_alloc_security, selinux_xfrm_policy_alloc),
> +	LSM_HOOK_INIT(xfrm_state_alloc, selinux_xfrm_state_alloc),
> +	LSM_HOOK_INIT(xfrm_state_alloc_acquire,
> +		      selinux_xfrm_state_alloc_acquire),
> +#endif
> +#ifdef CONFIG_KEYS
> +	LSM_HOOK_INIT(key_alloc, selinux_key_alloc),
> +#endif
> +#ifdef CONFIG_AUDIT
> +	LSM_HOOK_INIT(audit_rule_init, selinux_audit_rule_init),
> +#endif
> +#ifdef CONFIG_BPF_SYSCALL
> +	LSM_HOOK_INIT(bpf_map_alloc_security, selinux_bpf_map_alloc),
> +	LSM_HOOK_INIT(bpf_prog_alloc_security, selinux_bpf_prog_alloc),
> +#endif
> +#ifdef CONFIG_PERF_EVENTS
> +	LSM_HOOK_INIT(perf_event_alloc, selinux_perf_event_alloc),
> +#endif
>   };
>   
>   static __init int selinux_init(void)
> @@ -7287,14 +7322,14 @@ int selinux_disable(struct selinux_state *state)
>   
>   	selinux_enabled = 0;
>   
> +	/* Unregister netfilter hooks. */
> +	selinux_nf_ip_exit();
> +
>   	security_delete_hooks(selinux_hooks, ARRAY_SIZE(selinux_hooks));
>   
>   	/* Try to destroy the avc node cache */
>   	avc_disable();
>   
> -	/* Unregister netfilter hooks. */
> -	selinux_nf_ip_exit();
> -
>   	/* Unregister selinuxfs. */
>   	exit_sel_fs();
>   
> 

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

[Stephen Smalley]

On 12/9/19 8: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.

Also, if we have to retain this support, it seems like this ought to be 
fixed in the LSM framework especially since it was a change there that 
broke the SELinux implementation.

> 
>>
>> Fixes: b1d9e6b0646d ("LSM: Switch to lists of hooks")
>> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
>> ---
>>   security/selinux/hooks.c | 97 +++++++++++++++++++++++++++-------------
>>   1 file changed, 66 insertions(+), 31 deletions(-)
>>
>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>> index 116b4d644f68..5075be8eea2a 100644
>> --- a/security/selinux/hooks.c
>> +++ b/security/selinux/hooks.c
>> @@ -6864,6 +6864,21 @@ static int selinux_perf_event_write(struct 
>> perf_event *event)
>>   }
>>   #endif
>> +/*
>> + * IMPORTANT NOTE: When adding new hooks, please be careful to keep 
>> this order:
>> + * 1. any hooks that don't belong to (2.) or (3.) below,
>> + * 2. hooks that both access structures allocated by other hooks, and 
>> allocate
>> + *    structures that can be later accessed by other hooks (mostly 
>> "cloning"
>> + *    hooks),
>> + * 3. hooks that only allocate structures that can be later accessed 
>> by other
>> + *    hooks ("allocating" hooks).
>> + *
>> + * Please follow block comment delimiters in the list to keep this 
>> order.
>> + *
>> + * This ordering is needed for SELinux runtime disable to work at 
>> least somewhat
>> + * safely. Breaking the ordering rules above might lead to NULL 
>> pointer derefs
>> + * when disabling SELinux at runtime.
>> + */
>>   static struct security_hook_list selinux_hooks[] __lsm_ro_after_init 
>> = {
>>       LSM_HOOK_INIT(binder_set_context_mgr, 
>> selinux_binder_set_context_mgr),
>>       LSM_HOOK_INIT(binder_transaction, selinux_binder_transaction),
>> @@ -6886,12 +6901,7 @@ static struct security_hook_list 
>> selinux_hooks[] __lsm_ro_after_init = {
>>       LSM_HOOK_INIT(bprm_committing_creds, 
>> selinux_bprm_committing_creds),
>>       LSM_HOOK_INIT(bprm_committed_creds, selinux_bprm_committed_creds),
>> -    LSM_HOOK_INIT(fs_context_dup, selinux_fs_context_dup),
>> -    LSM_HOOK_INIT(fs_context_parse_param, 
>> selinux_fs_context_parse_param),
>> -
>> -    LSM_HOOK_INIT(sb_alloc_security, selinux_sb_alloc_security),
>>       LSM_HOOK_INIT(sb_free_security, selinux_sb_free_security),
>> -    LSM_HOOK_INIT(sb_eat_lsm_opts, selinux_sb_eat_lsm_opts),
>>       LSM_HOOK_INIT(sb_free_mnt_opts, selinux_free_mnt_opts),
>>       LSM_HOOK_INIT(sb_remount, selinux_sb_remount),
>>       LSM_HOOK_INIT(sb_kern_mount, selinux_sb_kern_mount),
>> @@ -6901,12 +6911,10 @@ static struct security_hook_list 
>> selinux_hooks[] __lsm_ro_after_init = {
>>       LSM_HOOK_INIT(sb_umount, selinux_umount),
>>       LSM_HOOK_INIT(sb_set_mnt_opts, selinux_set_mnt_opts),
>>       LSM_HOOK_INIT(sb_clone_mnt_opts, selinux_sb_clone_mnt_opts),
>> -    LSM_HOOK_INIT(sb_add_mnt_opt, selinux_add_mnt_opt),
>>       LSM_HOOK_INIT(dentry_init_security, selinux_dentry_init_security),
>>       LSM_HOOK_INIT(dentry_create_files_as, 
>> selinux_dentry_create_files_as),
>> -    LSM_HOOK_INIT(inode_alloc_security, selinux_inode_alloc_security),
>>       LSM_HOOK_INIT(inode_free_security, selinux_inode_free_security),
>>       LSM_HOOK_INIT(inode_init_security, selinux_inode_init_security),
>>       LSM_HOOK_INIT(inode_create, selinux_inode_create),
>> @@ -6978,21 +6986,15 @@ static struct security_hook_list 
>> selinux_hooks[] __lsm_ro_after_init = {
>>       LSM_HOOK_INIT(ipc_permission, selinux_ipc_permission),
>>       LSM_HOOK_INIT(ipc_getsecid, selinux_ipc_getsecid),
>> -    LSM_HOOK_INIT(msg_msg_alloc_security, 
>> selinux_msg_msg_alloc_security),
>> -
>> -    LSM_HOOK_INIT(msg_queue_alloc_security,
>> -            selinux_msg_queue_alloc_security),
>>       LSM_HOOK_INIT(msg_queue_associate, selinux_msg_queue_associate),
>>       LSM_HOOK_INIT(msg_queue_msgctl, selinux_msg_queue_msgctl),
>>       LSM_HOOK_INIT(msg_queue_msgsnd, selinux_msg_queue_msgsnd),
>>       LSM_HOOK_INIT(msg_queue_msgrcv, selinux_msg_queue_msgrcv),
>> -    LSM_HOOK_INIT(shm_alloc_security, selinux_shm_alloc_security),
>>       LSM_HOOK_INIT(shm_associate, selinux_shm_associate),
>>       LSM_HOOK_INIT(shm_shmctl, selinux_shm_shmctl),
>>       LSM_HOOK_INIT(shm_shmat, selinux_shm_shmat),
>> -    LSM_HOOK_INIT(sem_alloc_security, selinux_sem_alloc_security),
>>       LSM_HOOK_INIT(sem_associate, selinux_sem_associate),
>>       LSM_HOOK_INIT(sem_semctl, selinux_sem_semctl),
>>       LSM_HOOK_INIT(sem_semop, selinux_sem_semop),
>> @@ -7003,13 +7005,11 @@ static struct security_hook_list 
>> selinux_hooks[] __lsm_ro_after_init = {
>>       LSM_HOOK_INIT(setprocattr, selinux_setprocattr),
>>       LSM_HOOK_INIT(ismaclabel, selinux_ismaclabel),
>> -    LSM_HOOK_INIT(secid_to_secctx, selinux_secid_to_secctx),
>>       LSM_HOOK_INIT(secctx_to_secid, selinux_secctx_to_secid),
>>       LSM_HOOK_INIT(release_secctx, selinux_release_secctx),
>>       LSM_HOOK_INIT(inode_invalidate_secctx, 
>> selinux_inode_invalidate_secctx),
>>       LSM_HOOK_INIT(inode_notifysecctx, selinux_inode_notifysecctx),
>>       LSM_HOOK_INIT(inode_setsecctx, selinux_inode_setsecctx),
>> -    LSM_HOOK_INIT(inode_getsecctx, selinux_inode_getsecctx),
>>       LSM_HOOK_INIT(unix_stream_connect, 
>> selinux_socket_unix_stream_connect),
>>       LSM_HOOK_INIT(unix_may_send, selinux_socket_unix_may_send),
>> @@ -7032,7 +7032,6 @@ static struct security_hook_list selinux_hooks[] 
>> __lsm_ro_after_init = {
>>       LSM_HOOK_INIT(socket_getpeersec_stream,
>>               selinux_socket_getpeersec_stream),
>>       LSM_HOOK_INIT(socket_getpeersec_dgram, 
>> selinux_socket_getpeersec_dgram),
>> -    LSM_HOOK_INIT(sk_alloc_security, selinux_sk_alloc_security),
>>       LSM_HOOK_INIT(sk_free_security, selinux_sk_free_security),
>>       LSM_HOOK_INIT(sk_clone_security, selinux_sk_clone_security),
>>       LSM_HOOK_INIT(sk_getsecid, selinux_sk_getsecid),
>> @@ -7047,7 +7046,6 @@ static struct security_hook_list selinux_hooks[] 
>> __lsm_ro_after_init = {
>>       LSM_HOOK_INIT(secmark_refcount_inc, selinux_secmark_refcount_inc),
>>       LSM_HOOK_INIT(secmark_refcount_dec, selinux_secmark_refcount_dec),
>>       LSM_HOOK_INIT(req_classify_flow, selinux_req_classify_flow),
>> -    LSM_HOOK_INIT(tun_dev_alloc_security, 
>> selinux_tun_dev_alloc_security),
>>       LSM_HOOK_INIT(tun_dev_free_security, 
>> selinux_tun_dev_free_security),
>>       LSM_HOOK_INIT(tun_dev_create, selinux_tun_dev_create),
>>       LSM_HOOK_INIT(tun_dev_attach_queue, selinux_tun_dev_attach_queue),
>> @@ -7057,17 +7055,11 @@ static struct security_hook_list 
>> selinux_hooks[] __lsm_ro_after_init = {
>>       LSM_HOOK_INIT(ib_pkey_access, selinux_ib_pkey_access),
>>       LSM_HOOK_INIT(ib_endport_manage_subnet,
>>                 selinux_ib_endport_manage_subnet),
>> -    LSM_HOOK_INIT(ib_alloc_security, selinux_ib_alloc_security),
>>       LSM_HOOK_INIT(ib_free_security, selinux_ib_free_security),
>>   #endif
>>   #ifdef CONFIG_SECURITY_NETWORK_XFRM
>> -    LSM_HOOK_INIT(xfrm_policy_alloc_security, 
>> selinux_xfrm_policy_alloc),
>> -    LSM_HOOK_INIT(xfrm_policy_clone_security, 
>> selinux_xfrm_policy_clone),
>>       LSM_HOOK_INIT(xfrm_policy_free_security, selinux_xfrm_policy_free),
>>       LSM_HOOK_INIT(xfrm_policy_delete_security, 
>> selinux_xfrm_policy_delete),
>> -    LSM_HOOK_INIT(xfrm_state_alloc, selinux_xfrm_state_alloc),
>> -    LSM_HOOK_INIT(xfrm_state_alloc_acquire,
>> -            selinux_xfrm_state_alloc_acquire),
>>       LSM_HOOK_INIT(xfrm_state_free_security, selinux_xfrm_state_free),
>>       LSM_HOOK_INIT(xfrm_state_delete_security, 
>> selinux_xfrm_state_delete),
>>       LSM_HOOK_INIT(xfrm_policy_lookup, selinux_xfrm_policy_lookup),
>> @@ -7077,14 +7069,12 @@ static struct security_hook_list 
>> selinux_hooks[] __lsm_ro_after_init = {
>>   #endif
>>   #ifdef CONFIG_KEYS
>> -    LSM_HOOK_INIT(key_alloc, selinux_key_alloc),
>>       LSM_HOOK_INIT(key_free, selinux_key_free),
>>       LSM_HOOK_INIT(key_permission, selinux_key_permission),
>>       LSM_HOOK_INIT(key_getsecurity, selinux_key_getsecurity),
>>   #endif
>>   #ifdef CONFIG_AUDIT
>> -    LSM_HOOK_INIT(audit_rule_init, selinux_audit_rule_init),
>>       LSM_HOOK_INIT(audit_rule_known, selinux_audit_rule_known),
>>       LSM_HOOK_INIT(audit_rule_match, selinux_audit_rule_match),
>>       LSM_HOOK_INIT(audit_rule_free, selinux_audit_rule_free),
>> @@ -7094,19 +7084,64 @@ static struct security_hook_list 
>> selinux_hooks[] __lsm_ro_after_init = {
>>       LSM_HOOK_INIT(bpf, selinux_bpf),
>>       LSM_HOOK_INIT(bpf_map, selinux_bpf_map),
>>       LSM_HOOK_INIT(bpf_prog, selinux_bpf_prog),
>> -    LSM_HOOK_INIT(bpf_map_alloc_security, selinux_bpf_map_alloc),
>> -    LSM_HOOK_INIT(bpf_prog_alloc_security, selinux_bpf_prog_alloc),
>>       LSM_HOOK_INIT(bpf_map_free_security, selinux_bpf_map_free),
>>       LSM_HOOK_INIT(bpf_prog_free_security, selinux_bpf_prog_free),
>>   #endif
>>   #ifdef CONFIG_PERF_EVENTS
>>       LSM_HOOK_INIT(perf_event_open, selinux_perf_event_open),
>> -    LSM_HOOK_INIT(perf_event_alloc, selinux_perf_event_alloc),
>>       LSM_HOOK_INIT(perf_event_free, selinux_perf_event_free),
>>       LSM_HOOK_INIT(perf_event_read, selinux_perf_event_read),
>>       LSM_HOOK_INIT(perf_event_write, selinux_perf_event_write),
>>   #endif
>> +
>> +    /*
>> +     * PUT "CLONING" (ACCESSING + ALLOCATING) HOOKS HERE
>> +     */
>> +    LSM_HOOK_INIT(fs_context_dup, selinux_fs_context_dup),
>> +    LSM_HOOK_INIT(fs_context_parse_param, 
>> selinux_fs_context_parse_param),
>> +    LSM_HOOK_INIT(sb_eat_lsm_opts, selinux_sb_eat_lsm_opts),
>> +    LSM_HOOK_INIT(sb_add_mnt_opt, selinux_add_mnt_opt),
>> +#ifdef CONFIG_SECURITY_NETWORK_XFRM
>> +    LSM_HOOK_INIT(xfrm_policy_clone_security, 
>> selinux_xfrm_policy_clone),
>> +#endif
>> +
>> +    /*
>> +     * PUT "ALLOCATING" HOOKS HERE
>> +     */
>> +    LSM_HOOK_INIT(msg_msg_alloc_security, 
>> selinux_msg_msg_alloc_security),
>> +    LSM_HOOK_INIT(msg_queue_alloc_security,
>> +              selinux_msg_queue_alloc_security),
>> +    LSM_HOOK_INIT(shm_alloc_security, selinux_shm_alloc_security),
>> +    LSM_HOOK_INIT(sb_alloc_security, selinux_sb_alloc_security),
>> +    LSM_HOOK_INIT(inode_alloc_security, selinux_inode_alloc_security),
>> +    LSM_HOOK_INIT(sem_alloc_security, selinux_sem_alloc_security),
>> +    LSM_HOOK_INIT(secid_to_secctx, selinux_secid_to_secctx),
>> +    LSM_HOOK_INIT(inode_getsecctx, selinux_inode_getsecctx),
>> +    LSM_HOOK_INIT(sk_alloc_security, selinux_sk_alloc_security),
>> +    LSM_HOOK_INIT(tun_dev_alloc_security, 
>> selinux_tun_dev_alloc_security),
>> +#ifdef CONFIG_SECURITY_INFINIBAND
>> +    LSM_HOOK_INIT(ib_alloc_security, selinux_ib_alloc_security),
>> +#endif
>> +#ifdef CONFIG_SECURITY_NETWORK_XFRM
>> +    LSM_HOOK_INIT(xfrm_policy_alloc_security, 
>> selinux_xfrm_policy_alloc),
>> +    LSM_HOOK_INIT(xfrm_state_alloc, selinux_xfrm_state_alloc),
>> +    LSM_HOOK_INIT(xfrm_state_alloc_acquire,
>> +              selinux_xfrm_state_alloc_acquire),
>> +#endif
>> +#ifdef CONFIG_KEYS
>> +    LSM_HOOK_INIT(key_alloc, selinux_key_alloc),
>> +#endif
>> +#ifdef CONFIG_AUDIT
>> +    LSM_HOOK_INIT(audit_rule_init, selinux_audit_rule_init),
>> +#endif
>> +#ifdef CONFIG_BPF_SYSCALL
>> +    LSM_HOOK_INIT(bpf_map_alloc_security, selinux_bpf_map_alloc),
>> +    LSM_HOOK_INIT(bpf_prog_alloc_security, selinux_bpf_prog_alloc),
>> +#endif
>> +#ifdef CONFIG_PERF_EVENTS
>> +    LSM_HOOK_INIT(perf_event_alloc, selinux_perf_event_alloc),
>> +#endif
>>   };
>>   static __init int selinux_init(void)
>> @@ -7287,14 +7322,14 @@ int selinux_disable(struct selinux_state *state)
>>       selinux_enabled = 0;
>> +    /* Unregister netfilter hooks. */
>> +    selinux_nf_ip_exit();
>> +
>>       security_delete_hooks(selinux_hooks, ARRAY_SIZE(selinux_hooks));
>>       /* Try to destroy the avc node cache */
>>       avc_disable();
>> -    /* Unregister netfilter hooks. */
>> -    selinux_nf_ip_exit();
>> -
>>       /* Unregister selinuxfs. */
>>       exit_sel_fs();
>>
> 

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

[Casey Schaufler]

On 12/9/2019 5:58 AM, Stephen Smalley wrote:
> On 12/9/19 8: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.
>
> Also, if we have to retain this support, it seems like this ought to be fixed in the LSM framework especially since it was a change there that broke the SELinux implementation.

Agreed, mostly. Deleting an LSM is fundamentally something the infrastructure
should handle *if* we allow it. Should we decide at some point to allow loadable
modules, as Tetsuo has advocated from time to time, we would need a general
solution. We don't have a general solution now because only SELinux wants it.
The previous implementation was under #ifdef for SELinux. At the time I understood
that there was no interest in investing in it. The implementation passed tests
at the time.

I propose that until such time as someone decides to seriously investigate
loadable security modules* the sole user of the deletion mechanism is
welcome to invest whatever they like in their special case, and I will be
happy to lend whatever assistance I can.

---
* I do not plan to propose an implementation of loadable modules.
  I leave that as an exercise for the next generation.

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

[Ondrej Mosnacek]

On Mon, Dec 9, 2019 at 6:20 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 12/9/2019 5:58 AM, Stephen Smalley wrote:
> > On 12/9/19 8: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.
> >
> > Also, if we have to retain this support, it seems like this ought to be fixed in the LSM framework especially since it was a change there that broke the SELinux implementation.
>
> Agreed, mostly. Deleting an LSM is fundamentally something the infrastructure
> should handle *if* we allow it. Should we decide at some point to allow loadable
> modules, as Tetsuo has advocated from time to time, we would need a general
> solution. We don't have a general solution now because only SELinux wants it.
> The previous implementation was under #ifdef for SELinux. At the time I understood
> that there was no interest in investing in it. The implementation passed tests
> at the time.
>
> I propose that until such time as someone decides to seriously investigate
> loadable security modules* the sole user of the deletion mechanism is
> welcome to invest whatever they like in their special case, and I will be
> happy to lend whatever assistance I can.

On my way to lunch I came up with another relatively simple solution
that should address this problem at the infrastructure level. Let me
try to write it up into a patch, hopefully it will work...

>
> ---
> * I do not plan to propose an implementation of loadable modules.
>   I leave that as an exercise for the next generation.
>
>

-- 
Ondrej Mosnacek <omosnace at redhat dot com>
Software Engineer, Security Technologies
Red Hat, Inc.

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

[Casey Schaufler]

On 12/10/2019 3:27 AM, Ondrej Mosnacek wrote:
> On Mon, Dec 9, 2019 at 6:20 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>> On 12/9/2019 5:58 AM, Stephen Smalley wrote:
>>> On 12/9/19 8: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.
>>> Also, if we have to retain this support, it seems like this ought to be fixed in the LSM framework especially since it was a change there that broke the SELinux implementation.
>> Agreed, mostly. Deleting an LSM is fundamentally something the infrastructure
>> should handle *if* we allow it. Should we decide at some point to allow loadable
>> modules, as Tetsuo has advocated from time to time, we would need a general
>> solution. We don't have a general solution now because only SELinux wants it.
>> The previous implementation was under #ifdef for SELinux. At the time I understood
>> that there was no interest in investing in it. The implementation passed tests
>> at the time.
>>
>> I propose that until such time as someone decides to seriously investigate
>> loadable security modules* the sole user of the deletion mechanism is
>> welcome to invest whatever they like in their special case, and I will be
>> happy to lend whatever assistance I can.
> On my way to lunch I came up with another relatively simple solution
> that should address this problem at the infrastructure level. Let me
> try to write it up into a patch, hopefully it will work...

I await your proposal with keen interest.

>
>> ---
>> * I do not plan to propose an implementation of loadable modules.
>>   I leave that as an exercise for the next generation.
>>
>>

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

[Casey Schaufler]

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. 

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

[Casey Schaufler]

On 12/9/2019 8:40 AM, Wenhui Zhang wrote:
> As Casey mentioned.
> When changing this order, would you take some default stackable modules in consideration as well?

The order in which another module sets things up ought not
affect your module in any way. 

> I personally is writing a module for my course work, and I am using Selinux and Integrity implementations as a basis, while stacking some new hook implementations on top of this.
> If the underlying Selinux and Integrity module changes the order, would it affect my stacked one please?
> Maybe a centralized memory security blob garbage collection module should be written, such as garbage collection by refcount of the security blob?

The security module infrastructure does not do garbage collection.
Modules are encouraged to not create garbage.

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

[Ondrej Mosnacek]

On Mon, Dec 9, 2019 at 2:21 PM Stephen Smalley <sds@tycho.nsa.gov> 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, too, would like to see it gone, but removing it immediately would
likely cause issues for existing users (see [1]). So considering the
removal is going to take some time, I'd prefer to at least stop the
kernel from crashing in the meantime.

I do plan to seriously look at removing the runtime disable entirely,
but that is a longer-term goal. Restoring the logical hook order would
be trivial once the removal is done.

[1] https://bugzilla.redhat.com/show_bug.cgi?id=1430944#c2

--
Ondrej Mosnacek <omosnace at redhat dot com>
Software Engineer, Security Technologies
Red Hat, Inc.

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

[Paul Moore]

On Tue, Dec 10, 2019 at 6:19 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> On Mon, Dec 9, 2019 at 2:21 PM Stephen Smalley <sds@tycho.nsa.gov> 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, too, would like to see it gone, but removing it immediately would
> likely cause issues for existing users (see [1]) ...
>
> [1] https://bugzilla.redhat.com/show_bug.cgi?id=1430944#c2

For the record, and for those who didn't click on the RHBZ link above,
I'm a big fan of getting rid of SELinux's runtime disable but concede
that it must be done in such a way to as not horribly break userspace.

-- 
paul moore
www.paul-moore.com

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

[Casey Schaufler]

On 12/10/2019 11:29 AM, Paul Moore wrote:
> On Tue, Dec 10, 2019 at 6:19 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>> On Mon, Dec 9, 2019 at 2:21 PM Stephen Smalley <sds@tycho.nsa.gov> 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, too, would like to see it gone, but removing it immediately would
>> likely cause issues for existing users (see [1]) ...
>>
>> [1] https://bugzilla.redhat.com/show_bug.cgi?id=1430944#c2
> For the record, and for those who didn't click on the RHBZ link above,
> I'm a big fan of getting rid of SELinux's runtime disable but concede
> that it must be done in such a way to as not horribly break userspace.

Is there some reason that changing the "disable SELinux" option
has to remove the hooks? Why can't it set selinux_enabled to 0
and be done with it?

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

[Stephen Smalley]

On 12/10/19 2:43 PM, Casey Schaufler wrote:
> On 12/10/2019 11:29 AM, Paul Moore wrote:
>> On Tue, Dec 10, 2019 at 6:19 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>>> On Mon, Dec 9, 2019 at 2:21 PM Stephen Smalley <sds@tycho.nsa.gov> 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, too, would like to see it gone, but removing it immediately would
>>> likely cause issues for existing users (see [1]) ...
>>>
>>> [1] https://bugzilla.redhat.com/show_bug.cgi?id=1430944#c2
>> For the record, and for those who didn't click on the RHBZ link above,
>> I'm a big fan of getting rid of SELinux's runtime disable but concede
>> that it must be done in such a way to as not horribly break userspace.
> 
> Is there some reason that changing the "disable SELinux" option
> has to remove the hooks? Why can't it set selinux_enabled to 0
> and be done with it?

selinux_enabled is only used during initialization to deal with 
selinux=0 across the different components of SELinux.  It isn't checked 
by the hooks themselves.  And if we were to add a if (!selinux_enabled) 
return 0 to the start of every hook, then that's just another easy 
target for kernel exploits to leverage.

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

[Casey Schaufler]

On 12/10/2019 11:50 AM, Stephen Smalley wrote:
> On 12/10/19 2:43 PM, Casey Schaufler wrote:
>> On 12/10/2019 11:29 AM, Paul Moore wrote:
>>> On Tue, Dec 10, 2019 at 6:19 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>>>> On Mon, Dec 9, 2019 at 2:21 PM Stephen Smalley <sds@tycho.nsa.gov> 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, too, would like to see it gone, but removing it immediately would
>>>> likely cause issues for existing users (see [1]) ...
>>>>
>>>> [1] https://bugzilla.redhat.com/show_bug.cgi?id=1430944#c2
>>> For the record, and for those who didn't click on the RHBZ link above,
>>> I'm a big fan of getting rid of SELinux's runtime disable but concede
>>> that it must be done in such a way to as not horribly break userspace.
>>
>> Is there some reason that changing the "disable SELinux" option
>> has to remove the hooks? Why can't it set selinux_enabled to 0
>> and be done with it?
>
> selinux_enabled is only used during initialization to deal with selinux=0 across the different components of SELinux.  It isn't checked by the hooks themselves.  And if we were to add a if (!selinux_enabled) return 0 to the start of every hook, then that's just another easy target for kernel exploits to leverage.

That's what I expected, but I wanted to see it explicitly stated. Thanks.