Re: [PATCH bpf v1 1/2] bpf: Fix SOCK_OPS_GET_SK same-register OOB read in sock_ops

[Jiayuan Chen <jiayuan.chen@linux.dev>]

On 4/6/26 7:54 AM, Emil Tsalapatis wrote:
> 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 <2022090917019@std.uestc.edu.cn>
>>> Reported-by: Yinhao Hu <dddddd@hust.edu.cn>
>>> Reported-by: Kaiyan Mei <M202472210@hust.edu.cn>
>>> Reported-by: Dongliang Mu <dzm91@hust.edu.cn>
>>> Closes: https://lore.kernel.org/bpf/6fe1243e-149b-4d3b-99c7-fcc9e2f75787@std.uestc.edu.cn/T/#u
>>> Signed-off-by: Jiayuan Chen <jiayuan.chen@linux.dev>
>> 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?
>

Thanks for the review!

The AI reviewer's observation about SOCK_OPS_GET_FIELD() is correct —
it has the same bug when dst_reg == src_reg and is_locked_tcp_sock == 0.
I've folded that fix into patch 1 in v2.

Regarding dst_reg != src_reg: this case is actually safe. When
dst_reg != src_reg, fullsock_reg is dst_reg itself, and the generated
sequence is:

LDX_MEM   dst_reg = is_fullsock
JEQ       dst_reg == 0, +jmp
LDX_MEM   dst_reg = sk

The JEQ only branches when dst_reg == 0, so dst_reg is naturally
zeroed on that path — no extra MOV_IMM needed. The same-register bug
exists precisely because dst_reg == src_reg forces the macro to borrow
a temporary register for the is_fullsock check, leaving dst_reg (the
ctx pointer) untouched.

I will add a get_sk_diff_reg subtest in v2.

The other suggestions (moving the detailed comment to the BPF program
file, avoiding vague "the fix" wording) are good points — addressed
in v2 as well.