Re: [PATCH V2] bpf: security enhancement by limiting the offensive eBPF helpers

[Daniel Borkmann]

On 1/18/23 1:54 AM, Yi He wrote:
> The bpf_send_singal, bpf_send_singal_thread and bpf_override_return
> is similar to bpf_write_user and can affect userspace processes.
> Thus, these three helpers should also be restricted by security lockdown.
>
> Signed-off-by: Yi He <clangllvm@xxxxxxx>
> ---
>   V1 -> V2: add security lockdown to bpf_send_singal_thread and remove
> 	the unused LOCKDOWN_OFFENSIVE_BPF_MAX.
>
>   include/linux/security.h | 2 ++
>   kernel/trace/bpf_trace.c | 9 ++++++---
>   2 files changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 5b67f208f..42420e620 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -123,6 +123,8 @@ enum lockdown_reason {
>   	LOCKDOWN_DEBUGFS,
>   	LOCKDOWN_XMON_WR,
>   	LOCKDOWN_BPF_WRITE_USER,
> +	LOCKDOWN_BPF_SEND_SIGNAL,
> +	LOCKDOWN_BPF_OVERRIDE_RETURN,
>   	LOCKDOWN_DBG_WRITE_KERNEL,
>   	LOCKDOWN_RTAS_ERROR_INJECTION,
>   	LOCKDOWN_INTEGRITY_MAX,

I'm not applying this.. i) this means by default you effectively remove these
helpers from existing users in the wild given integrity mode is default for
secure boot, but also ii) should we lock-down and remove the ability for other
privileged entities like processes to send signals, seccomp to ret_kill, ptrace,
etc given they all "can affect userspace processes". For the other one, check
out already existing FUNCTION_ERROR_INJECTION kernel config.

> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index 3bbd3f0c8..fdb94868d 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -1463,9 +1463,11 @@ bpf_tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
>   		return &bpf_cgrp_storage_delete_proto;
>   #endif
>   	case BPF_FUNC_send_signal:
> -		return &bpf_send_signal_proto;
> +		return security_locked_down(LOCKDOWN_BPF_SEND_SIGNAL) < 0 ?
> +		       NULL : &bpf_send_signal_proto;
>   	case BPF_FUNC_send_signal_thread:
> -		return &bpf_send_signal_thread_proto;
> +		return security_locked_down(LOCKDOWN_BPF_SEND_SIGNAL) < 0 ?
> +		       NULL : &bpf_send_signal_thread_proto;
>   	case BPF_FUNC_perf_event_read_value:
>   		return &bpf_perf_event_read_value_proto;
>   	case BPF_FUNC_get_ns_current_pid_tgid:
> @@ -1531,7 +1533,8 @@ kprobe_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
>   		return &bpf_get_stack_proto;
>   #ifdef CONFIG_BPF_KPROBE_OVERRIDE
>   	case BPF_FUNC_override_return:
> -		return &bpf_override_return_proto;
> +		return security_locked_down(LOCKDOWN_BPF_OVERRIDE_RETURN) < 0 ?
> +		       NULL : &bpf_override_return_proto;
>   #endif
>   	case BPF_FUNC_get_func_ip:
>   		return prog->expected_attach_type == BPF_TRACE_KPROBE_MULTI ?