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
