sock_ops_convert_ctx_access() reads rtt_min without the is_locked_tcp_sock guard used for every other tcp_sock field. On request_sock-backed sock_ops callbacks, sk points at a tcp_request_sock and the converted load reads past the end of the allocation. Reuse SOCK_OPS_LOAD_TCP_SOCK_FIELD() for the rtt_min access and compute the offset with offsetof(struct minmax_sample, v). This leaves the byte addressed unchanged from the old sizeof_field(struct minmax_sample, t) expression, while making rtt_min consistent with every other tcp_sock field. This also picks up the same dst_reg == src_reg handling used by the other guarded field loads. Extend the sock_ops_get_sk selftest with an rtt_min subtest that checks request_sock-backed !fullsock callbacks read zero instead of leaking request_sock-adjacent memory. Found via AST-based call-graph analysis using sqry. Fixes: 44f0e43037d3 ("bpf: Add support for reading sk_state and more") Cc: stable@vger.kernel.org Signed-off-by: Werner Kasselman --- net/core/filter.c | 12 +++---- .../bpf/prog_tests/sock_ops_get_sk.c | 9 ++++++ .../selftests/bpf/progs/sock_ops_get_sk.c | 31 +++++++++++++++++++ 3 files changed, 45 insertions(+), 7 deletions(-) diff --git a/net/core/filter.c b/net/core/filter.c index e8ad062f63bc..9c43193a5c39 100644 --- a/net/core/filter.c +++ b/net/core/filter.c @@ -10827,14 +10827,12 @@ static u32 sock_ops_convert_ctx_access(enum bpf_access_type type, sizeof(struct minmax)); BUILD_BUG_ON(sizeof(struct minmax) < sizeof(struct minmax_sample)); + BUILD_BUG_ON(offsetof(struct tcp_sock, rtt_min) + + offsetof(struct minmax_sample, v) > S16_MAX); - *insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF( - struct bpf_sock_ops_kern, sk), - si->dst_reg, si->src_reg, - offsetof(struct bpf_sock_ops_kern, sk)); - *insn++ = BPF_LDX_MEM(BPF_W, si->dst_reg, si->dst_reg, - offsetof(struct tcp_sock, rtt_min) + - sizeof_field(struct minmax_sample, t)); + off = offsetof(struct tcp_sock, rtt_min) + + offsetof(struct minmax_sample, v); + SOCK_OPS_LOAD_TCP_SOCK_FIELD(BPF_W, off); break; case offsetof(struct bpf_sock_ops, bpf_sock_ops_cb_flags): diff --git a/tools/testing/selftests/bpf/prog_tests/sock_ops_get_sk.c b/tools/testing/selftests/bpf/prog_tests/sock_ops_get_sk.c index 343d92c4df30..1aea4c97d5d3 100644 --- a/tools/testing/selftests/bpf/prog_tests/sock_ops_get_sk.c +++ b/tools/testing/selftests/bpf/prog_tests/sock_ops_get_sk.c @@ -70,6 +70,15 @@ void test_ns_sock_ops_get_sk(void) ASSERT_EQ(skel->bss->diff_reg_bug_detected, 0, "diff_reg_bug_not_detected"); } + /* Test sock_ops rtt_min access in !fullsock callbacks */ + if (test__start_subtest("get_rtt_min")) { + run_sock_ops_test(cgroup_fd, + bpf_program__fd(skel->progs.sock_ops_get_rtt_min)); + ASSERT_EQ(skel->bss->rtt_min_null_seen, 1, "rtt_min_null_seen"); + ASSERT_EQ(skel->bss->rtt_min_bug_detected, 0, + "rtt_min_bug_not_detected"); + } + sock_ops_get_sk__destroy(skel); close_cgroup: close(cgroup_fd); diff --git a/tools/testing/selftests/bpf/progs/sock_ops_get_sk.c b/tools/testing/selftests/bpf/progs/sock_ops_get_sk.c index 3a0689f8ce7c..dee07da8901e 100644 --- a/tools/testing/selftests/bpf/progs/sock_ops_get_sk.c +++ b/tools/testing/selftests/bpf/progs/sock_ops_get_sk.c @@ -114,4 +114,35 @@ __naked void sock_ops_get_sk_diff_reg(void) : __clobber_all); } +/* sock_ops rtt_min access: different-register, is_locked_tcp_sock == 0 path (TCP_NEW_SYN_RECV). */ +int rtt_min_bug_detected; +int rtt_min_null_seen; + +SEC("sockops") +__naked void sock_ops_get_rtt_min(void) +{ + asm volatile ( + "r7 = *(u32 *)(r1 + %[is_fullsock_off]);" + "r2 = *(u32 *)(r1 + %[rtt_min_off]);" + "if r7 != 0 goto 2f;" + "if r2 == 0 goto 1f;" + "r1 = %[rtt_min_bug_detected] ll;" + "r3 = 1;" + "*(u32 *)(r1 + 0) = r3;" + "goto 2f;" + "1:" + "r1 = %[rtt_min_null_seen] ll;" + "r3 = 1;" + "*(u32 *)(r1 + 0) = r3;" + "2:" + "r0 = 1;" + "exit;" + : + : __imm_const(is_fullsock_off, offsetof(struct bpf_sock_ops, is_fullsock)), + __imm_const(rtt_min_off, offsetof(struct bpf_sock_ops, rtt_min)), + __imm_addr(rtt_min_bug_detected), + __imm_addr(rtt_min_null_seen) + : __clobber_all); +} + char _license[] SEC("license") = "GPL"; -- 2.43.0