Re: [PATCH bpf-next v4 14/15] selftests/bpf: Test attaching struct_ops to a cgroup

[bot+bpf-ci@kernel.org]

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

> diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ops.c b/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ops.c
> new file mode 100644
> index 0000000000000..6435ed1c2cf6d
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ops.c

[ ... ]

> +/* Must match progs/bpf_tcp_ops.c */
> +#define OPS_RETVAL1	11
> +#define OPS_RETVAL2	22
> +
> +/*
> + * Attach three struct_ops implementing timeout_init to the same cgroup; they
> + * run in attach order [retval1, retval2, retval3]. timeout_init's return value
> + * is chained: the first prog reads the kernel seed via bpf_get_retval() (0,
> + * since no legacy sockops prog is attached) and returns OPS_RETVAL1; each
> + * subsequent prog must then observe the previous prog's return value. This
> + * proves the trampoline inherits the retval across an array of struct_ops.
> + */
> +static void test_retval(int cgroup_fd, struct bpf_tcp_ops *skel)
> +{
> +	struct bpf_link *link1 = NULL, *link2 = NULL, *link3 = NULL;
> +
> +	skel->bss->retval_saw1 = -1;
> +	skel->bss->retval_saw2 = -1;
> +	skel->bss->retval_saw3 = -1;
> +
> +	link1 = bpf_map__attach_cgroup_opts(skel->maps.tcp_ops_retval1, cgroup_fd, NULL);
> +	if (!ASSERT_OK_PTR(link1, "attach_retval1"))
> +		goto done;
> +
> +	link2 = bpf_map__attach_cgroup_opts(skel->maps.tcp_ops_retval2, cgroup_fd, NULL);
> +	if (!ASSERT_OK_PTR(link2, "attach_retval2"))
> +		goto done;
> +
> +	link3 = bpf_map__attach_cgroup_opts(skel->maps.tcp_ops_retval3, cgroup_fd, NULL);
> +	if (!ASSERT_OK_PTR(link3, "attach_retval3"))
> +		goto done;
> +
> +	do_listen_connect(AF_INET6);
> +
> +	/* First prog inherits the kernel seed (no legacy sockops -> 0). */
> +	ASSERT_EQ(skel->bss->retval_saw1, 0, "retval_saw1");
> +	/* Each subsequent prog inherits the previous prog's return value. */
> +	ASSERT_EQ(skel->bss->retval_saw2, OPS_RETVAL1, "retval_saw2");
> +	ASSERT_EQ(skel->bss->retval_saw3, OPS_RETVAL2, "retval_saw3");

The test expects retval_saw2 to be 11 and retval_saw3 to be 22, both
outside the range the verifier believes bpf_get_retval() can return.

> diff --git a/tools/testing/selftests/bpf/progs/bpf_tcp_ops.c b/tools/testing/selftests/bpf/progs/bpf_tcp_ops.c
> new file mode 100644
> index 0000000000000..94a7f52573d51
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/bpf_tcp_ops.c

[ ... ]

> +#define OPS_RETVAL1	11
> +#define OPS_RETVAL2	22
> +#define OPS_RETVAL3	33
> +
> +int retval_saw1;
> +int retval_saw2;
> +int retval_saw3;
> +
> +SEC("struct_ops")
> +int BPF_PROG(tcp_ops_retval1_timeout_init, struct sock *sk, struct request_sock *req)
> +{
> +	retval_saw1 = bpf_get_retval();
> +	return OPS_RETVAL1;
> +}
> +
> +SEC(".struct_ops.link")
> +struct bpf_tcp_ops tcp_ops_retval1 = {
> +	.timeout_init = (void *)tcp_ops_retval1_timeout_init,
> +};
> +
> +SEC("struct_ops")
> +int BPF_PROG(tcp_ops_retval2_timeout_init, struct sock *sk, struct request_sock *req)
> +{
> +	retval_saw2 = bpf_get_retval();
> +	return OPS_RETVAL2;
> +}
> +
> +SEC(".struct_ops.link")
> +struct bpf_tcp_ops tcp_ops_retval2 = {
> +	.timeout_init = (void *)tcp_ops_retval2_timeout_init,
> +};
> +
> +SEC("struct_ops")
> +int BPF_PROG(tcp_ops_retval3_timeout_init, struct sock *sk, struct request_sock *req)
> +{
> +	retval_saw3 = bpf_get_retval();
> +	return OPS_RETVAL3;
> +}

Does bpf_get_retval() have the correct verifier range for struct_ops
programs?

This test is the first user of bpf_get_retval() from a struct_ops
program. The verifier refines the helper's return value in
do_refine_retval_range() (kernel/bpf/verifier.c):

  case BPF_FUNC_get_retval:
      if (prog_type == BPF_PROG_TYPE_CGROUP_SOCKOPT && ...) break;
      if (prog_type == BPF_PROG_TYPE_LSM && ...) { ... } else {
          range.minval = -MAX_ERRNO;
          range.maxval = 0;
      }
      reg_set_srange64(ret_reg, range.minval, range.maxval);

BPF_PROG_TYPE_STRUCT_OPS takes the else branch, so after calling
bpf_get_retval() the verifier believes the value is in [-MAX_ERRNO, 0].
reg_set_srange64() unconditionally assigns that range rather than
intersecting with the actual possible values.

But that claim does not hold for bpf_tcp_ops. bpf_tcp_ops_get_retval()
(net/ipv4/bpf_tcp_ops.c) returns the chained value from the previous
struct_ops program's return, which is unconstrained: check_return_code()
bails out early for struct_ops members whose return type is not a
pointer, and .timeout_init returns plain int with no range check.

The test's own assertions prove the divergence: the code above expects
retval_saw2 == 11 and retval_saw3 == 22, both outside [-MAX_ERRNO, 0].

The test itself is harmless because it only stores the value to a .bss
int. But the incorrect range could allow unsafe code elsewhere:

  int v = bpf_get_retval();   /* verifier: [-4095, 0] */
  arr[-v] = 1;                /* verifier: index in [0, 4095], accepts */

A preceding bpf_tcp_ops in the same cgroup could return an arbitrary int
(say -1000000), producing an out-of-bounds access the verifier accepted.
The incorrect range also lets the verifier prune 'if (bpf_get_retval() >
0)' as dead code, so a program could silently behave differently from
what it says.

This gap was introduced in commit 5c9c5dca9cbb1 ("bpf: tcp: Support
selected sock_ops callbacks as struct_ops"), which added
bpf_tcp_ops_get_retval_proto with .ret_type = RET_INTEGER without
teaching do_refine_retval_range() that struct_ops/bpf_tcp_ops has no
-MAX_ERRNO..0 restriction. A forward search over the provided range
bf0fde93fa8f6..d499152306e78 found only one later commit
d499152306e78, which does not address this; the gap is unfixed at the
end of the series.

Either do_refine_retval_range() needs a struct_ops case that leaves the
range unrefined (as CGROUP_GETSOCKOPT does), or the chained struct_ops
return value has to be clamped to [-MAX_ERRNO, 0] where
bpf_tcp_ops_call_int() stores it.


---
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/35271288839