Re: [PATCH net 08/10] ipvs: avoid out-of-bounds write in ip_vs_nat_icmp

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
ipvs: avoid out-of-bounds write in ip_vs_nat_icmp

This passes the already-parsed ip_vs_iphdr down into ip_vs_nat_icmp() so
the embedded transport header offset comes from ciph->len instead of a
second read of the inner IPv4 ihl field.  The IPv4 header sanity checks in
ip_vs_out_icmp() and ip_vs_in_icmp() are also reworked to read ihl only
once.

Link: https://sashiko.dev/#/patchset/20260722101517.36313-1-ja%40ssi.bg

> diff --git a/net/netfilter/ipvs/ip_vs_core.c b/net/netfilter/ipvs/ip_vs_core.c
> index 6b79e0c4d9e28..0bdaeb4ed61e4 100644
> --- a/net/netfilter/ipvs/ip_vs_core.c
> +++ b/net/netfilter/ipvs/ip_vs_core.c
> @@ -925,28 +925,27 @@ static int ip_vs_route_me_harder(struct netns_ipvs *ipvs, int af,
>   */
>  void ip_vs_nat_icmp(struct sk_buff *skb, struct ip_vs_protocol *pp,
>  		    struct ip_vs_conn *cp, int inout, unsigned int toff,
> -		    bool has_ports)
> +		    bool has_ports, struct ip_vs_iphdr *ciph)
>  {
>  	struct iphdr *iph	 = ip_hdr(skb);
>  	struct icmphdr *icmph	 = (struct icmphdr *)(skb->data + toff);
> -	struct iphdr *ciph	 = (struct iphdr *)(icmph + 1);
> -	unsigned int coff __maybe_unused = toff + sizeof(struct icmphdr);
> +	struct iphdr *cih	 = (struct iphdr *)(icmph + 1);
>  
>  	if (inout) {
>  		iph->saddr = cp->vaddr.ip;
>  		ip_send_check(iph);
> -		ciph->daddr = cp->vaddr.ip;
> -		ip_send_check(ciph);
> +		cih->daddr = cp->vaddr.ip;
> +		ip_send_check(cih);
>  	} else {
>  		iph->daddr = cp->daddr.ip;
>  		ip_send_check(iph);
> -		ciph->saddr = cp->daddr.ip;
> -		ip_send_check(ciph);
> +		cih->saddr = cp->daddr.ip;
> +		ip_send_check(cih);
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but the two ip_send_check(cih)
calls here still read the inner ihl field a second time, which looks like
the same multi-read pattern the commit message describes fixing in this
function.

ip_send_check() takes its own fresh read of ihl and checksums 4 * ihl
bytes:

net/ipv4/ip_output.c:ip_send_check() {
	iph->check = 0;
	iph->check = ip_fast_csum((unsigned char *)iph, iph->ihl);
}

The only length the callers guarantee is the snapshot value ciph->len (plus
4 when ports are mangled):

net/netfilter/ipvs/ip_vs_core.c:handle_response_icmp() {
	unsigned int ctoff = ciph->len;
	...
		ctoff += 2 * sizeof(__u16);
	if (skb_ensure_writable(skb, ctoff))
		goto out;
	...
		ip_vs_nat_icmp(skb, pp, cp, 1, toff, has_ports, ciph);
}

ciph->len was computed earlier in ip_vs_fill_iph_skb_off() as
offset + iph->ihl * 4 from a skb_header_pointer() snapshot, and
skb_ensure_writable() -> pskb_may_pull() re-copies the bytes out of the
fragments, so the ihl byte ip_send_check() then reads can differ from the
one that sized the validated window.

If the fresh ihl is larger than the snapshot value, can ip_fast_csum() read
up to 40 bytes past what skb_ensure_writable() guaranteed, and fold
adjacent skb memory into the inner header checksum that is emitted?

If the fresh ihl ends up below 5 (for example the version/ihl byte going
from 0x45 to 0x40, compensated in tot_len so both the inner IP and ICMP
checksums stay valid), some architectures do not tolerate it:

arch/mips/include/asm/checksum.h:ip_fast_csum() {
	word += 4;
	do {
		csum += *word;
		carry = (csum < *word);
		csum += carry;
		word++;
	} while (word != stop);
}

with stop = iph + ihl, so for ihl <= 4 the loop termination is never hit.

arch/s390/include/asm/checksum.h:ip_fast_csum() {
	ihl -= 4;
	while (ihl--)
		csum += *ptr++;
}

where ihl == 0 underflows into roughly four billion forward word reads.

Would it make sense to checksum over the validated length
(ciph->len - ciph->off), or to re-validate version/ihl after
skb_ensure_writable(), matching what was done for the port offset?

The same sequence exists on the other call site:

net/netfilter/ipvs/ip_vs_xmit.c:ip_vs_icmp_xmit() {
	wlen = ciph->len;
	...
		wlen += 2 * sizeof(__u16); /* Also mangle ports */
	if (skb_ensure_writable(skb, wlen))
		goto tx_error;
	...
	ip_vs_nat_icmp(skb, pp, cp, 0, toff, has_ports, ciph);
}

>  
>  	/* the TCP/UDP/SCTP port */
>  	if (has_ports) {
> -		__be16 *ports = (void *)ciph + ciph->ihl*4;
> +		__be16 *ports = (void *)(skb->data + ciph->len);
>  
>  		if (inout)
>  			ports[1] = cp->vport;

[ ... ]

> @@ -1131,11 +1129,15 @@ static int ip_vs_out_icmp(struct netns_ipvs *ipvs, struct sk_buff *skb,
>  
>  	/* Now find the contained IP header */
>  	offset += sizeof(_icmph);
> +	if (!ip_vs_fill_iph_skb_icmp(AF_INET, skb, offset, true, &ciph))
> +		return NF_ACCEPT; /* The packet looks wrong, ignore */
> +
>  	cih = skb_header_pointer(skb, offset, sizeof(_ciph), &_ciph);
> -	if (!(cih && cih->version == 4 && cih->ihl >= 5))
> +	if (!(cih && cih->version == 4 &&
> +	      ciph.len - ciph.off >= sizeof(struct iphdr)))
>  		return NF_ACCEPT; /* The packet looks wrong, ignore */

This check validates the snapshot length only, so nothing revalidates
version or ihl after the data is pulled into the linear area and handed to
ip_send_check().  Was that intentional?

[ ... ]