Re: [PATCH 02/14] xfrm: interface: validate the IP header on xmit

[Steffen Klassert <steffen.klassert@secunet.com>]

On Tue, Nov 03, 2026 at 08:05:00AM +0000, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 1 · Low: 0
> 
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
> 
> - [Medium] In xfrmi_xmit() (net/xfrm/xfrm_interface_core.c), the new
>   header checks look only at skb->protocol.
> 
> --
> 
> xfrm: interface: validate the IP header on xmit
> 
> This patch adds a pskb_inet_may_pull() call to xfrmi_xmit(). It also
> drops packets whose IP version does not match skb->protocol, and IPv4
> packets with a malformed ihl. The aim is to stop xfrm4_transport_output()
> from working on an unvalidated header when packets are injected into an
> xfrmi device.
> 
> > diff --git a/net/xfrm/xfrm_interface_core.c b/net/xfrm/xfrm_interface_core.c
> > index 688306bf62c5a..9835fbe63ccb8 100644
> > --- a/net/xfrm/xfrm_interface_core.c
> > +++ b/net/xfrm/xfrm_interface_core.c
> > @@ -531,8 +531,13 @@ static netdev_tx_t xfrmi_xmit(struct sk_buff *skb, struct net_device *dev)
> >  
> >  	memset(&fl, 0, sizeof(fl));
> >  
> > +	if (!pskb_inet_may_pull(skb))
> > +		goto tx_err;
> > +
> >  	switch (skb->protocol) {
> >  	case htons(ETH_P_IPV6):
> > +		if (ipv6_hdr(skb)->version != 6)
> > +			goto tx_err;
> >  		memset(IP6CB(skb), 0, sizeof(*IP6CB(skb)));
> >  		xfrm_decode_session(dev_net(dev), skb, &fl, AF_INET6);
> >  		if (!dst) {
> 
> [Severity: Medium]
> Can an skb marked ETH_P_IPV6 pass this check and still reach
> xfrm4_transport_output()?
> 
> The new check in xfrmi_xmit() looks only at skb->protocol. The transform
> that actually runs is picked by the dst family, and then by
> x->props.family.
> 
> When skb_dst(skb) is already set, xfrmi_xmit() skips the route lookup.
> The dst family is never compared with the header that was just checked.
> 
> In collect_md mode, xfrmi_xmit2() also swaps in a different dst after
> this check:
> 
> net/xfrm/xfrm_interface_core.c:xfrmi_xmit2() {
>     ...
> 		if (md_info->dst_orig) {
> 			struct dst_entry *tmp_dst = dst;
> 
> 			dst = md_info->dst_orig;
> 			skb_dst_set(skb, dst);
>     ...
> }
> 
> xfrm_lookup_with_ifid() then takes the policy/state family from that
> dst, not from the packet:
> 
> net/xfrm/xfrm_policy.c:xfrm_lookup_with_ifid() {
>     ...
> 	u16 family = dst_orig->ops->family;
>     ...
> }
> 
> A tc BPF program could reach this with the following sequence:
> 
> bpf_skb_set_xfrm_info()
>     info->dst_orig = skb_dst(skb);    /* IPv4 rtable */
>     attaches the per-cpu metadata dst
> bpf_skb_change_proto(skb, ETH_P_IPV6)
>     skb_valid_dst() is false for DST_METADATA, so the metadata dst
>     and the IPv4 dst_orig are both kept
> program writes an IPv6 header with version 6
> xfrmi_xmit()                  /* dst != NULL, only version check runs */
>   xfrmi_xmit2()               /* collect_md swaps in IPv4 dst_orig */
>     xfrm_lookup_with_ifid()   /* family is AF_INET */
>     ...
>     xfrm_outer_mode_output()
>       xfrm4_transport_output()
> 
> A plain IPv4 rtable does not survive bpf_skb_change_proto(), because it
> drops non-metadata dsts. So this appears limited to the metadata dst
> ordering above.
> 
> With an AF_INET transport mode state, xfrm4_transport_output() takes ihl
> from the low nibble of the IPv6 header's first byte. That nibble is the
> top four bits of the traffic class, so it can be anything from 0 to 15:
> 
> net/xfrm/xfrm_output.c:xfrm4_transport_output() {
> 	struct iphdr *iph = ip_hdr(skb);
> 	int ihl = iph->ihl * 4;
>     ...
> 	__skb_pull(skb, ihl);
> 	memmove(skb_network_header(skb), iph, ihl);
>     ...
> }
> 
> pskb_inet_may_pull() only made 40 bytes linear for IPv6. For a small
> packet, skb->len can wrap and the memmove() can read past the end of the
> data. If the skb has paged data and a linear length under 60 bytes,
> __skb_pull() hits BUG() instead.
> 
> One step has not been confirmed end to end. It is not certain that an
> AF_INET policy/state lookup matches a flowi that xfrm_decode_session()
> filled in as IPv6. It looks plausible with a 0/0 selector, because the
> ip4 address fields overlay ip6 address bytes that the sender controls.
> 
> This path also needs CAP_BPF/CAP_NET_ADMIN, a collect_md xfrmi, and an
> AF_INET transport mode SA and policy.
> 
> The commit message says:
> 
>   "xfrm4_transport_output() uses iph->ihl for __skb_pull() and memmove()
>   and expects the header to be valid."
> 
> Would it make sense to also drop packets whose final dst family (after
> the collect_md dst_orig swap) does not match skb->protocol, so this path
> is covered too?

Not a regression of this patch, just a missing path.
Can be fixed by a followup patch.