Re: [PATCH bpf v3 2/2] bpf: reject incompatible protocol changes

[bot+bpf-ci]

> bpf: reject incompatible protocol changes

TL;DR: The family revalidation looks incomplete: LWT ingress encap
(bpf_lwt_push_ip_encap, seg6) can still change L3 protocol and leave a
mismatched assigned socket. Extend the check, or narrow the commit message's
claim?

> diff --git a/net/core/filter.c b/net/core/filter.c
> index 173349bd2a3f..22e2ecb0cf2f 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c

[ ... ]

> @@ -3549,6 +3549,12 @@ static bool bpf_sk_assign_family_ok(const struct sk_buff *skb,
>  	return bpf_sk_assign_family_ok_proto(sk, skb_protocol(skb, true));
>  }
>
> +static bool bpf_skb_proto_change_sk_ok(struct sk_buff *skb, __be16 proto)
> +{
> +	return !skb_sk_is_prefetched(skb) ||
> +	       bpf_sk_assign_family_ok_proto(skb->sk, proto);
> +}
> +
>  BPF_CALL_3(bpf_skb_change_proto, struct sk_buff *, skb, __be16, proto,
>  	   u64, flags)
>  {

The commit message says "Without revalidation, this bypasses the
assignment-time family check. Reject incompatible protocol changes before
modifying the skb, including L3 encapsulation and decapsulation."

Is this revalidation complete?  It is only wired into bpf_skb_change_proto(),
bpf_skb_net_grow() and bpf_skb_net_shrink(), but another path can still change
the L3 protocol of an skb that keeps its bpf_sk_assign()ed socket.

Here is one concrete sequence:

1. A tc ingress program calls bpf_sk_assign() on an IPv4 packet with an
   AF_INET TCP socket.  bpf_sk_assign_family_ok() passes and the program
   returns TC_ACT_OK.

2. ip_rcv_core() keeps the prefetched socket:

    /* Must drop socket now because of tproxy. */
    if (!skb_sk_is_prefetched(skb))
        skb_orphan(skb);

3. ip_rcv_finish_core() skips early demux because skb->sk is set, then calls
   ip_route_input_noref().  A route with "encap bpf in" sets
   dst->input = lwtunnel_input.

4. bpf_input() runs the LWT_IN program, which calls
   bpf_lwt_in_push_encap(BPF_LWT_ENCAP_IP) with an IPv6 header.  That reaches
   bpf_lwt_push_ip_encap(..., ingress=true):

    } else {
        skb->protocol = htons(ETH_P_IPV6);
    }

   The skb->sk / sock_pfree assignment is left untouched.
   BPF_LWT_ENCAP_SEG6 behaves the same way.

5. The program returns BPF_LWT_REROUTE.  bpf_lwt_input_reroute() calls
   ip6_route_input() and dst_input(), so the packet goes through ip6_input
   and on to tcp_v6_rcv().

6. inet6_steal_sock() -> skb_steal_sock() returns the prefetched AF_INET
   socket for the IPv6 packet.  This is the family mismatch that
   bpf_skb_proto_change_sk_ok() is meant to prevent.

In-kernel IP tunnel decapsulation in the same netns keeps the prefetched
socket across a family change in the same way.  For example, with sit:

    ipip6_rcv()
        iptunnel_pull_header()    /* sets skb->protocol = ETH_P_IPV6 */
        skb_scrub_packet(skb, false)    /* does not orphan */
        netif_rx()

Should bpf_lwt_push_ip_encap() and bpf_push_seg6_encap() on ingress also call
bpf_skb_proto_change_sk_ok() (or orphan a prefetched socket)?  Or should the
commit message narrow its claim to the two tc helpers?

This is not introduced by this patch, and every step above needs
CAP_NET_ADMIN (tc and LWT programs).  But it limits the protection the patch
claims to provide.


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/36746681022