Re: [PATCH bpf-next v10 5/5] bpf: Only enable BPF LSM hooks when an LSM program is attached

[Kees Cook <keescook@chromium.org>]

On Wed, May 08, 2024 at 12:10:45AM +0200, KP Singh wrote:
> [...]
> +/**
> + * security_toggle_hook - Toggle the state of the LSM hook.
> + * @hook_addr: The address of the hook to be toggled.
> + * @state: Whether to enable for disable the hook.
> + *
> + * Returns 0 on success, -EINVAL if the address is not found.
> + */
> +int security_toggle_hook(void *hook_addr, bool state)
> +{
> +	struct lsm_static_call *scalls = ((void *)&static_calls_table);
> +	unsigned long num_entries =
> +		(sizeof(static_calls_table) / sizeof(struct lsm_static_call));
> +	int i;
> +
> +	for (i = 0; i < num_entries; i++) {
> +		if (!scalls[i].hl)
> +			continue;
> +
> +		if (scalls[i].hl->hook.lsm_func_addr != hook_addr)
> +			continue;
> +
> +		if (state)
> +			static_branch_enable(scalls[i].active);
> +		else
> +			static_branch_disable(scalls[i].active);
> +		return 0;
> +	}
> +	return -EINVAL;
> +}

First of all: patches 1-4 are great. They have a measurable performance
benefit; let's get those in.

But here I come to patch 5 where I will suggest the exact opposite of
what Paul said in v9 for patch 5. :P

I don't want to have a global function that can be used to disable LSMs.
We got an entire distro (RedHat) to change their SELinux configurations
to get rid of CONFIG_SECURITY_SELINUX_DISABLE (and therefore
CONFIG_SECURITY_WRITABLE_HOOKS), via commit f22f9aaf6c3d ("selinux:
remove the runtime disable functionality"). We cannot reintroduce that,
and I'm hoping Paul will agree, given this reminder of LSM history. :)

Run-time hook changing should be BPF_LSM specific, if it exists at all.

-- 
Kees Cook