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

[Jiayuan Chen]

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

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.