Re: [PATCH bpf-next 1/2] bpf: Support cgroup {get,set}sockopt hooks for compat syscalls

[bot+bpf-ci@kernel.org]

[-- Attachment #1: Type: text/plain, Size: 3876 bytes --]

> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index ffd96e8b920bd..15f712b5b1645 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -7616,6 +7616,7 @@ struct bpf_sockopt {
>  	__s32	optname;
>  	__s32	optlen;
>  	__s32	retval;
> +	__s32	is_compat;
>  };

[ ... ]

> diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
> index 8fbc942a1cc3a..910878e991a04 100644
> --- a/kernel/bpf/cgroup.c
> +++ b/kernel/bpf/cgroup.c

[ ... ]

> @@ -2705,6 +2707,9 @@ static u32 cg_sockopt_convert_ctx_access(enum bpf_access_type type,
>  		else
>  			*insn++ = CG_SOCKOPT_READ_FIELD(optlen);
>  		break;
> +	case offsetof(struct bpf_sockopt, is_compat):
> +		*insn++ = CG_SOCKOPT_READ_FIELD(is_compat);
> +		break;
>  	case offsetof(struct bpf_sockopt, retval):
>  		BUILD_BUG_ON(offsetof(struct bpf_cg_run_ctx, run_ctx) != 0);

Appending __s32 is_compat to struct bpf_sockopt grows the UAPI structure
from 40 to 48 bytes due to 8-byte alignment required by the __bpf_md_ptr()
unions. This creates 4 bytes of unoccupied tail padding at offsets 44-47.

Can the verifier accept a 4-byte load at ctx offset 44?

Looking at cg_sockopt_is_valid_access() in kernel/bpf/cgroup.c, the bounds
check allows any offset under sizeof(struct bpf_sockopt), with alignment:

	if (off < 0 || off >= sizeof(struct bpf_sockopt))
		return false;
	if (off % size != 0)
		return false;

A 4-byte load at offset 44 passes (44 < 48, 44 % 4 == 0), but
cg_sockopt_convert_ctx_access() above has no case for offset 44 and no
default arm. When bpf_convert_ctx_accesses() sees zero instructions
returned, it calls verifier_bug() and triggers WARN_ONCE under
CONFIG_DEBUG_KERNEL, letting any user with cgroup-sockopt load permission
trigger a kernel warning via a BPF_PROG_TYPE_CGROUP_SOCKOPT program with
r0 = *(u32 *)(r1 + 44).

Before this patch every accepted offset had a matching conversion case:
offsets 0-23 (pointer fields) require size 8, and 24, 28, 32, 36 (level,
optname, optlen, retval) were all handled; sizeof was exactly 40 with no
tail padding.

Should the hole be reserved in the UAPI struct (e.g. __u32 :32 or a named
reserved field) and rejected in is_valid_access, or should
cg_sockopt_convert_ctx_access() add an explicit case list or default arm
to reject non-enumerated offsets?

[ ... ]

The patch updates __cgroup_bpf_run_filter_getsockopt() to accept compat
and populate ctx.is_compat, but __cgroup_bpf_run_filter_getsockopt_kern()
was not updated.

This matters because __cgroup_bpf_run_filter_getsockopt_kern() is the only
hook that runs for getsockopt(TCP_ZEROCOPY_RECEIVE). The outer
BPF_CGROUP_RUN_PROG_GETSOCKOPT in do_sock_getsockopt() is bypassed for
that optname via tcp_bpf_bypass_getsockopt(), and do_tcp_getsockopt()
instead calls BPF_CGROUP_RUN_PROG_GETSOCKOPT_KERN() at net/ipv4/tcp.c.
Unlike the two hooks this patch fixes, that kernel-side hook was never
gated on compat, so it already ran for 32-bit callers.

Now it reports ctx->is_compat == 0 for them, violating the documented
contract that the field is "1 for compat syscalls and 0 otherwise." A BPF
program that keys its optval layout decisions off ctx->is_compat will
misparse the compat struct tcp_zerocopy_receive request from a 32-bit task.

Should compat be threaded into
__cgroup_bpf_run_filter_getsockopt_kern() as well, or at minimum should the
documentation clarify that is_compat is unreliable on the kern hook?

[ ... ]

This looks like a bug fix for code introduced in 55db9c0e8534 ("net: remove
compat_sys_{get,set}sockopt"). Should this include:

  Fixes: 55db9c0e8534 ("net: remove compat_sys_{get,set}sockopt")


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/31696027015