On May 15, 2024 KP Singh <kpsingh@xxxxxxxxxx> wrote:
>
> BPF LSM hooks have side-effects (even when a default value's returned)
> as some hooks end up behaving differently due to the very presence of
> the hook.
>
> The static keys guarding the BPF LSM hooks are disabled by default and
> enabled only when a BPF program is attached implementing the hook
> logic. This avoids the issue of the side-effects and also the minor
> overhead associated with the empty callback.
>
> security_file_ioctl:
> 0xff...0e30 <+0>: endbr64
> 0xff...0e34 <+4>: nopl 0x0(%rax,%rax,1)
> 0xff...0e39 <+9>: push %rbp
> 0xff...0e3a <+10>: push %r14
> 0xff...0e3c <+12>: push %rbx
> 0xff...0e3d <+13>: mov %rdx,%rbx
> 0xff...0e40 <+16>: mov %esi,%ebp
> 0xff...0e42 <+18>: mov %rdi,%r14
> 0xff...0e45 <+21>: jmp 0xff...0e57 <security_file_ioctl+39>
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> Static key enabled for SELinux
>
> 0xff...0e47 <+23>: xchg %ax,%ax
> ^^^^^^^^^^^^^^
>
> Static key disabled for BPF. This gets patched when a BPF LSM
> program is attached
>
> 0xff...0e49 <+25>: xor %eax,%eax
> 0xff...0e4b <+27>: xchg %ax,%ax
> 0xff...0e4d <+29>: pop %rbx
> 0xff...0e4e <+30>: pop %r14
> 0xff...0e50 <+32>: pop %rbp
> 0xff...0e51 <+33>: cs jmp 0xff...0000 <__x86_return_thunk>
> 0xff...0e57 <+39>: endbr64
> 0xff...0e5b <+43>: mov %r14,%rdi
> 0xff...0e5e <+46>: mov %ebp,%esi
> 0xff...0e60 <+48>: mov %rbx,%rdx
> 0xff...0e63 <+51>: call 0xff...33c0 <selinux_file_ioctl>
> 0xff...0e68 <+56>: test %eax,%eax
> 0xff...0e6a <+58>: jne 0xff...0e4d <security_file_ioctl+29>
> 0xff...0e6c <+60>: jmp 0xff...0e47 <security_file_ioctl+23>
> 0xff...0e6e <+62>: endbr64
> 0xff...0e72 <+66>: mov %r14,%rdi
> 0xff...0e75 <+69>: mov %ebp,%esi
> 0xff...0e77 <+71>: mov %rbx,%rdx
> 0xff...0e7a <+74>: call 0xff...e3b0 <bpf_lsm_file_ioctl>
> 0xff...0e7f <+79>: test %eax,%eax
> 0xff...0e81 <+81>: jne 0xff...0e4d <security_file_ioctl+29>
> 0xff...0e83 <+83>: jmp 0xff...0e49 <security_file_ioctl+25>
> 0xff...0e85 <+85>: endbr64
> 0xff...0e89 <+89>: mov %r14,%rdi
> 0xff...0e8c <+92>: mov %ebp,%esi
> 0xff...0e8e <+94>: mov %rbx,%rdx
> 0xff...0e91 <+97>: pop %rbx
> 0xff...0e92 <+98>: pop %r14
> 0xff...0e94 <+100>: pop %rbp
> 0xff...0e95 <+101>: ret
>
> This patch enables this by providing a LSM_HOOK_INIT_RUNTIME variant
> that allows the LSMs to opt-in to hooks which can be toggled at runtime
> which with security_toogle_hook.
>
> Reviewed-by: Kees Cook <keescook@xxxxxxxxxxxx>
> Acked-by: Casey Schaufler <casey@xxxxxxxxxxxxxxxx>
> Signed-off-by: KP Singh <kpsingh@xxxxxxxxxx>
> ---
> include/linux/lsm_hooks.h | 30 ++++++++++++++++++++++++++++-
> kernel/bpf/trampoline.c | 40 +++++++++++++++++++++++++++++++++++----
> security/bpf/hooks.c | 2 +-
> security/security.c | 35 +++++++++++++++++++++++++++++++++-
> 4 files changed, 100 insertions(+), 7 deletions(-)
...
> diff --git a/security/security.c b/security/security.c
> index 9654ca074aed..2f8bcacf1fb4 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -885,6 +887,37 @@ int lsm_fill_user_ctx(struct lsm_ctx __user *uctx, u32 *uctx_len,
> return rc;
> }
>
> +/**
> + * 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);
GCC (v14.1.1 if that matters) is complaining about casting randomized
structs. Looking quickly at the two structs, lsm_static_call and
lsm_static_calls_table, I suspect the cast is harmless even if the
randstruct case, but I would like to see some sort of fix for this so
I don't get spammed by GCC every time I do a build. On the other hand,
if this cast really is a problem in the randstruct case we obviously
need to fix that.
Either way, resolve this and make sure you test with GCC/randstruct
enabled.
> + 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 || !scalls[i].hl->runtime)
> + 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;
> +}
> +
> /*
> * The default value of the LSM hook is defined in linux/lsm_hook_defs.h and
> * can be accessed with:
> --
> 2.45.0.rc1.225.g2a3ae87e7f-goog
--
paul-moore.com
