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

[Emil Tsalapatis]

On Sun Apr 5, 2026 at 10:58 PM EDT, Jiayuan Chen wrote:
>
> 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@xxxxxxxxxxxxxxxx>
>>>> Reported-by: Yinhao Hu <dddddd@xxxxxxxxxxx>
>>>> Reported-by: Kaiyan Mei <M202472210@xxxxxxxxxxx>
>>>> Reported-by: Dongliang Mu <dzm91@xxxxxxxxxxx>
>>>> Closes: https://lore.kernel.org/bpf/6fe1243e-149b-4d3b-99c7-fcc9e2f75787@xxxxxxxxxxxxxxxx/T/#u
>>>> Signed-off-by: Jiayuan Chen <jiayuan.chen@xxxxxxxxx>
>>> 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
>

Yes, I missed the dst is the is_fullsock_reg assignment. v1 looks
correct then.

> 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.

With the changes, feel free to add to both patches:

Reviewed-by: Emil Tsalapatis <emil@xxxxxxxxxxxxxxx>