On Sun Apr 5, 2026 at 7:49 PM EDT, Emil Tsalapatis wrote:
> 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?
Sorry for the double post, but also check sashiko.dev:
SOSK_OPTS_GET_FIELD seems to have the same issue as the
SOCK_OPTS_GET_SK. Can you add the same fix to it?
>
>> ---
>> 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)
>>
