Re: [PATCH 1/1] bpf: Check sk_fullsock() in bpf_tcp_*_syncookie

> Fixes: c0df236e1394 ("bpf: Change bpf_tcp_*_syncookie to accept ARG_PTR_TO_BTF_ID_SOCK_COMMON")
> Cc: [email protected]
> Reported-by: Vega <[email protected]>
> Assisted-by: Codex:gpt-5.4
> Signed-off-by: Luxiao Xu <[email protected]>
> Signed-off-by: Ren Wei <[email protected]>

Does the Fixes: tag point to the commit that introduced the bug?

Looking at the history, c0df236e1394 (v5.10-rc1) widened arg1_type from
ARG_PTR_TO_SOCK_COMMON to ARG_PTR_TO_BTF_ID_SOCK_COMMON so that
bpf_skc_to_*() results could also be accepted. However, passing a
mini-socket was already possible with the old ARG_PTR_TO_SOCK_COMMON type:

At 399040847084 (v5.2-rc1, which added bpf_tcp_check_syncookie):

  kernel/bpf/verifier.c accepted any sk pointer for ARG_PTR_TO_SOCK_COMMON:

      } else if (arg_type == ARG_PTR_TO_SOCK_COMMON) {
              expected_type = PTR_TO_SOCK_COMMON;
              /* Any sk pointer can be ARG_PTR_TO_SOCK_COMMON */
              if (!type_is_sk_pointer(type))
                      goto err_type;

  and type_is_sk_pointer() included PTR_TO_SOCK_COMMON.

  bpf_skc_lookup_tcp_proto already had .ret_type =
  RET_PTR_TO_SOCK_COMMON_OR_NULL, yielding PTR_TO_SOCK_COMMON after the
  NULL check.

  bpf_skc_lookup_tcp() calls __bpf_skc_lookup() which walks ehash via
  __inet_lookup_established(). ehash holds TCP_TIME_WAIT socks
  (inet_twsk_hashdance) and TCP_NEW_SYN_RECV request socks
  (inet_ehash_insert(req_to_sk(req), NULL) in inet_connection_sock.c).
  The returned sock_common can be a request_sock or inet_timewait_sock.

  The commit that added bpf_tcp_check_syncookie (399040847084) exposed
  both BPF_FUNC_skc_lookup_tcp and BPF_FUNC_tcp_check_syncookie in the
  same program types (tc_cls_act_func_proto and xdp_func_proto), so a
  program could chain them from day one.

At 399040847084, sk_protocol was still a bitfield inside struct sock
(unsigned int sk_protocol : 8), far past the end of struct sock_common,
so the slab-out-of-bounds read described in the commit message was
already reachable.

Should this instead be two separate Fixes: tags, one per helper?

  Fixes: 399040847084 ("bpf: add helper to check for a valid SYN cookie")
  Fixes: 70d66244317e ("bpf: add bpf_tcp_gen_syncookie helper")

The Cc: [email protected] line will cause stable tooling to backport
this patch only as far as v5.10, but the out-of-bounds read exists back to
v5.2 (bpf_tcp_check_syncookie) and v5.4 (bpf_tcp_gen_syncookie).


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