Re: [PATCH] xfrm: validate ihl in xfrm4_transport_output()

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 (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).

- [High] net/xfrm/xfrm_output.c:69-70, xfrm4_transport_output(): the new
  guard validates the attacker-controlled IPv4 header length against…

Pre-existing issues:
- [Medium] net/xfrm/xfrm_output.c:66-81, xfrm4_transport_output(): only
  an upper bound on ihl is added, so iph->ihl values 0..4 (ihl = 0..16…

--

xfrm: validate ihl in xfrm4_transport_output()

This adds a bounds check on the IPv4 header length read from the packet in
xfrm4_transport_output(), rejecting the packet with -EINVAL when ihl exceeds
skb->len, before the header relocation done by __skb_pull() and memmove().
The stated goal is to close a local denial of service reachable by injecting
frames into an xfrm interface, for example with AF_PACKET on an xfrmi device.

> diff --git a/net/xfrm/xfrm_output.c b/net/xfrm/xfrm_output.c
> index e305ba32e356b..3abb21d113a45 100644
> --- a/net/xfrm/xfrm_output.c
> +++ b/net/xfrm/xfrm_output.c
> @@ -66,6 +66,9 @@ static int xfrm4_transport_output(struct xfrm_state *x, struct sk_buff *skb)
>  	struct iphdr *iph = ip_hdr(skb);
>  	int ihl = iph->ihl * 4;
>  
> +	if (ihl > skb->len)
> +		return -EINVAL;
> +

[Severity: High]
Is skb->len the right bound here?  skb->len covers the paged fragment data
too, but the two operations further down in xfrm4_transport_output() need
the linear head length:

net/xfrm/xfrm_output.c:xfrm4_transport_output()
	skb->transport_header = skb->network_header + ihl;
	__skb_pull(skb, ihl);
	memmove(skb_network_header(skb), iph, ihl);

include/linux/skbuff.h:__skb_pull()
	skb->len -= len;
	if (unlikely(skb->len < skb->data_len)) {
		...
		BUG();
	}

So for any skb where skb_headlen(skb) < ihl <= skb->len, the new check
passes and __skb_pull() still drives skb->len below skb->data_len and
BUG()s, and the memmove() source range still runs past skb->tail.

Can the injection vector named in the changelog produce exactly such an
skb?  In packet_snd() the linear length is attacker controlled:

net/packet/af_packet.c:packet_snd()
	linear = __virtio16_to_cpu(vio_le(), vnet_hdr.hdr_len);
	linear = max(linear, min_t(int, len, hard_header_len));

xfrmi_dev_setup() never calls ether_setup(), so hard_header_len is 0 and
linear stays at hdr_len, and packet_alloc_skb() keeps the rest paged:

net/packet/af_packet.c:packet_alloc_skb()
	if (prepad + len < PAGE_SIZE || !linear)
		linear = len;
	...
	skb_put(skb, linear);
	skb->data_len = len - linear;

With hdr_len = 1 and len larger than PAGE_SIZE, skb_headlen(skb) is 1 and
the single linear byte is the attacker's version/ihl byte.

Does anything linearize the skb before the pull?  xfrmi_dev_init() sets
NETIF_F_SG | NETIF_F_FRAGLIST, so skb_needs_linearize() is false, and
xfrm_output_one() only calls xfrm_skb_check_space() (pskb_expand_head(),
which does not linearize) before xfrm_outer_mode_output().

Would using the linear bound instead work, i.e.

	if (ihl > skb_headlen(skb))
		return -EINVAL;

or

	if (!pskb_may_pull(skb, ihl))
		return -EINVAL;

which also makes the memmove() source in-bounds?

[Severity: Medium]
This isn't a bug introduced by this patch, but since the check being added
here is the ihl validation, should it also enforce a lower bound?  Only the
upper bound is checked, so iph->ihl values 0..4 (0..16 bytes, less than
sizeof(struct iphdr)) are still accepted.

The changelog points at the validation in ip_rcv_core(), which rejects both
directions:

net/ipv4/ip_input.c:ip_rcv_core()
	if (iph->ihl < 5 || iph->version != 4)
		goto inhdr_error;

and sibling xfrm4 code already assumes ihl >= 5:

net/xfrm/xfrm_inout.h:xfrm4_extract_header()
	XFRM_MODE_SKB_CB(skb)->optlen = iph->ihl * 4 - sizeof(*iph);

Can a short ihl leak uninitialized headroom?  In
xfrm4_transport_output() mac_header is placed at a fixed offset while only
ihl bytes are relocated:

	skb->mac_header = skb->network_header +
			  offsetof(struct iphdr, protocol);
	...
	memmove(skb_network_header(skb), iph, ihl);

offsetof(struct iphdr, protocol) is 9, so for iph->ihl <= 2 the byte at
skb_mac_header() is never written, and esp_output() reads it:

net/ipv4/esp4.c:esp_output()
	esp.proto = *skb_mac_header(skb);
	*skb_mac_header(skb) = IPPROTO_ESP;

esp_output_head() then puts that value on the wire:

	esp_output_fill_trailer(tail, esp->tfclen, esp->plen, esp->proto);

For iph->ihl == 2 the emitted outer header bytes covering check/saddr/daddr
are similarly never-initialized headroom, apart from tot_len and check
being rewritten by __ip_local_out()/ip_send_check().

On reachability, the IP_HDRINCL raw path cannot deliver this because
raw_send_hdrinc() rejects it:

net/ipv4/raw.c:raw_send_hdrinc()
	if (iphlen > length || iphlen < sizeof(*iph))
		goto error_free;

and for the AF_PACKET/xfrmi path the flow dissector bails out on short ihl:

net/core/flow_dissector.c:__skb_flow_dissect()
		if (!iph || iph->ihl < 5) {
			fdret = FLOW_DISSECT_RET_OUT_BAD;

which leaves decode_session4() with an all-zero flowi4, so a wildcard
(0.0.0.0) transport-mode policy and SA would additionally have to be
selected.  I could not establish that such a state cannot be installed, so
would adding an ihl < sizeof(struct iphdr) rejection alongside the new
check be worthwhile?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260917072226.80788-1-q.h.hack.winter%40gmail.com