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

[netdev-bot+sashiko@kernel.org]

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?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261102100451.2505200-2-steffen.klassert%40secunet.com