Re: [PATCH bpf v4 2/3] bpf: Add validation for bpf_set_retval argument

[Yonghong Song]

On 6/4/26 6:04 AM, Xu Kuohai wrote:
> From: Xu Kuohai <xukuohai@xxxxxxxxxx>
>
> The bpf_set_retval() helper is used by cgroup BPF programs to set the
> return value of the target hook. The argument type for this helper is
> ARG_ANYTHING. This allows setting a positive value, which no cgroup
> hook expects and can cause issues, such as:
>
> - BPF_LSM_CGROUP: a positive value from bpf_lsm_socket_create bypasses
>    the err < 0 check in __sock_create(), leaving the socket object
>    unallocated. The positive return value is then propagated to the
>    syscall entry __sys_socket(), which also bypasses the IS_ERR() guard
>    and ultimately causes a NULL pointer dereference.
>
> - BPF_CGROUP_DEVICE: a positive value can be returned through cgroup
>    device bpf prog -> devcgroup_check_permission() -> bdev_permission()
>    -> bdev_file_open_by_dev(), where ERR_PTR(positive) produces a pointer
>    that IS_ERR() does not catch, leading to a wild pointer dereference.
>
> - BPF_CGROUP_SOCK: a positive value can be returned through cgroup sock
>    bpf prog -> __cgroup_bpf_run_filter_sk() -> inet_create() ->
>    __sock_create(), where inet_create() frees the newly allocated sk
>    via sk_common_release() and sets sock->sk = NULL on the non-zero
>    return, but __sock_create() only checks err < 0 for cleanup, so a
>    positive retval bypasses cleanup and returns a socket with NULL sk
>    to userspace, triggering a NULL pointer dereference on subsequent
>    socket operations.
>
> - BPF_CGROUP_SYSCTL: a positive value can be returned through the cgroup
>    bpf prog -> __cgroup_bpf_run_filter_sysctl() -> proc_sys_call_handler(),
>    where a non-zero return bypasses the normal sysctl proc_handler and is
>    returned directly to userspace as return value of read() or write()
>    syscall.

FYI, the following patch:
    https://lore.kernel.org/bpf/20260603105317.944304-4-dawei.feng@xxxxxxxxxx/
will change return value for BPF_CGROUP_SYSCTL from 1 to 0.

> So add validation for the argument of the bpf_set_retval() helper.
>
> For BPF_LSM_CGROUP, enforce the LSM hook specific range returned by
> bpf_lsm_get_retval_range().
>
> For all other cgroup program types, restrict the argument to
> [-MAX_ERRNO, 0], which matches the kernel convention of 0 for success
> and negative errno for error.
>
> BPF_CGROUP_GETSOCKOPT is an exception, since valid getsockopt
> implementations may return positive values, as allowed by commit
> c4dcfdd406aa ("bpf: Move getsockopt retval to struct bpf_cg_run_ctx").
>
> Also refine the return value range of bpf_get_retval() so that
> values returned by bpf_get_retval() can be passed directly to
> bpf_set_retval() without extra manual bounds checking.
>
> Fixes: b44123b4a3dc ("bpf: Add cgroup helpers bpf_{get,set}_retval to get/set syscall return value")
> Fixes: 69fd337a975c ("bpf: per-cgroup lsm flavor")
> Reported-by: Quan Sun <2022090917019@xxxxxxxxxxxxxxxx>
> Closes: https://lore.kernel.org/all/567d3206-74a5-44e5-99c6-779c425f399e@xxxxxxxxxxxxxxxx
> Signed-off-by: Xu Kuohai <xukuohai@xxxxxxxxxx>
> ---
>   kernel/bpf/verifier.c | 54 +++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 54 insertions(+)
[...]