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

[Julian Anastasov <ja@ssi.bg>]

	Hello,

On Tue, 4 Aug 2026, Jakub Kicinski wrote:

> 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?

	New version of this patch was delayed on friday with
some hours, so now I have a separate fix that revalidates ihl,
ready to be applied on top of this patch...

Regards

--
Julian Anastasov <ja@ssi.bg>