Re: [PATCH bpf v6 1/2] bpf: tcp: Reject non-TCP skb in bpf_sk_assign_tcp_reqsk()

[Martin KaFai Lau]

On Tue, Apr 07, 2026 at 09:25:06PM -0700, Kuniyuki Iwashima wrote:
> > > sashiko has flagged a similar issue with larger scope.
> > > Please take a look. Thanks.
> > >
> > > https://sashiko.dev/#/patchset/20260403015851.148209-1-jiayuan.chen%40linux.dev
> >
> >
> > Thanks a lot Martin, sashiko actually dug into a deeper issue here.
> >
> > Eric and Kuniyuki,
> >
> > I think the AI review has a point. Since BPF can modify skb fields, the
> > following sequence still bypasses the protocol check in
> > bpf_sk_assign_tcp_reqsk():
> >
> >     // for a UDP skb
> >     iph->protocol = TCP
> >     bpf_sk_assign_tcp_reqsk()
> >     iph->protocol = UDP
> >
> > On top of that, bpf_sk_assign() already has the same problem — it doesn't
> > validate L4 protocol at all.
> 
> Sigh... honestly it does not make sense to me to add changes
> in the common fast path to protect someone with bpf capability
> shooting oneself in the foot.
> 
> On top of L4 validation in bpf_sk_assign() and bpf_sk_assign_tcp_reqsk(),
> can't we mark such an skb immutable after the helpers and catch
> subsequent writes to skb->data on the verifier ?

Clearing the skb->sk in a helper like bpf_skb_store_bytes or
rejecting direct writes to skb->data could break existing
bpf program.

I suspect adding a simple iph->protocol/ip6h->nexthdr check to
the helper (e.g. bpf_sk_assign) could also break some
tunneling use cases (e.g. ipip) also.

> 
> 
> >
> > So I think we should add a check matching skb against sk in
> > skb_steal_sock() instead of adding check in bpf helper.

Maybe limit the check to the '*prefetched' case in skb_steal_sock().

FWIW, in the early days of bpf_sk_assign, a tc bpf program could only
get hold of a tcp_sock. Later, bpf_map_lookup_elem(&sock_map) was
allowed in tc, and then udp/unix sock support was also added to sock_map.

There have been discussions on tc bpf programs being able to do
bpf_map_lookup_elem(&sock_map) to get a unix_sock. AFAIK, this
looked-up unix_sock can be used in bpf_sk_assign. It probably
makes sense for bpf_sk_assign to reject all non-tcp/non-udp sk.