On Sat Apr 4, 2026 at 10:09 AM EDT, Jiayuan Chen wrote:
> When a BPF sock_ops program reads ctx->sk with dst_reg == src_reg
> (e.g., r1 = *(u64 *)(r1 + offsetof(sk))), the SOCK_OPS_GET_SK() macro
> fails to zero the destination register in the is_fullsock == 0 path.
>
> The macro saves/restores a temporary register and checks is_fullsock.
> When is_fullsock == 0 (e.g., TCP_NEW_SYN_RECV state with a request_sock),
> it should set dst_reg = 0 (NULL) so the verifier's PTR_TO_SOCKET_OR_NULL
> type is correct at runtime. Instead, dst_reg retains the original ctx
> pointer, which passes subsequent NULL checks and can be used as a bogus
> socket pointer, leading to stack-out-of-bounds access in helpers like
> bpf_skc_to_tcp6_sock().
>
> Fix by:
> - Changing JMP_A(1) to JMP_A(2) in the fullsock path to skip the
> added instruction.
> - Adding BPF_MOV64_IMM(si->dst_reg, 0) after the temp register
> restore in the !fullsock path, placed after the restore because
> dst_reg == src_reg means we need src_reg intact to read ctx->temp.
>
> Fixes: 84f44df664e9 ("bpf: sock_ops sk access may stomp registers when
> dst_reg = src_reg")
> Reported-by: Quan Sun <[email protected]>
> Reported-by: Yinhao Hu <[email protected]>
> Reported-by: Kaiyan Mei <[email protected]>
> Reported-by: Dongliang Mu <[email protected]>
> Closes:
> https://lore.kernel.org/bpf/[email protected]/T/#u
> Signed-off-by: Jiayuan Chen <[email protected]>
This patch only seems to fix the problem when dst_reg == src_reg.
Why is this not an issue when is_fullsock == 0, but dst_reg != src_reg?
In that case the dst_reg is unmodified by the whole macro but is still
marked as PTR_TO_SOCKET_OR_NULL. Isn't that a problem? Can you add
a test case for is_fullsock == 0 but dst_reg != src_reg in patch 2?
> ---
> Apologies for the Easter timing!
> ---
> net/core/filter.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 78b548158fb05..8fee00e6adef4 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -10618,10 +10618,11 @@ static u32 sock_ops_convert_ctx_access(enum
> bpf_access_type type,
> si->dst_reg, si->src_reg, \
> offsetof(struct bpf_sock_ops_kern, sk));\
> if (si->dst_reg == si->src_reg) { \
> - *insn++ = BPF_JMP_A(1); \
> + *insn++ = BPF_JMP_A(2); \
> *insn++ = BPF_LDX_MEM(BPF_DW, reg, si->src_reg, \
> offsetof(struct bpf_sock_ops_kern, \
> temp)); \
> + *insn++ = BPF_MOV64_IMM(si->dst_reg, 0); \
> } \
> } while (0)
>
