Re: [PATCH nf 1/2] ipvs: avoid out-of-bounds write in ip_vs_nat_icmp_v6

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] Three problems with the comment and commit message; the code
  change itself is correct.

--

ipvs: avoid out-of-bounds write in ip_vs_nat_icmp_v6

When ipv6_find_hdr() stops at a non-first fragment and leaves len equal
to off, this patch adds sizeof(struct ipv6hdr) to ciph.len in both
ip_vs_out_icmp_v6() and ip_vs_in_icmp_v6(). With that, the
skb_ensure_writable() calls in the ICMPv6 NAT paths cover the embedded
addresses that ip_vs_nat_icmp_v6() rewrites.

> diff --git a/net/netfilter/ipvs/ip_vs_core.c b/net/netfilter/ipvs/ip_vs_core.c
> index fd503f0efb578..cfd193196a5bc 100644
> --- a/net/netfilter/ipvs/ip_vs_core.c
> +++ b/net/netfilter/ipvs/ip_vs_core.c
> @@ -1206,6 +1206,13 @@ static int ip_vs_out_icmp_v6(struct netns_ipvs *ipvs, struct sk_buff *skb,
>  				     true, &ciph))
>  		return NF_ACCEPT; /* The packet looks wrong, ignore */
>  
> +	/* ipv6_find_hdr() does not include the embedded header for
> +	 * non-first fragments, add it so that ESP can pass and the
> +	 * NAT writable checks cover the rewritten addresses
> +	 */

[Severity: Low]
The code change looks correct. Is the "so that ESP can pass" part of
this comment accurate, though?

I couldn't find anything that blocks an embedded IPv6 ESP fragment based
on ciph.len:

  - Neither ip_vs_out_icmp_v6() nor ip_vs_in_icmp_v6() compares ciph.len
    with ciph.off.
  - The only such check is the "ciph->len - ciph->off" ihl check, and it
    exists only on the IPv4 path.
  - The ESP lookup reads only the addresses:

net/netfilter/ipvs/ip_vs_proto_ah_esp.c:ah_esp_conn_fill_param_proto() {
	if (likely(!ip_vs_iph_inverse(iph)))
		ip_vs_conn_fill_param(ipvs, af, IPPROTO_UDP,
				      &iph->saddr, htons(PORT_ISAKMP),
				      &iph->daddr, htons(PORT_ISAKMP), p);
	...
}

So non-first ESP fragments already reached connection lookup and NAT
before this patch. That is how the out-of-bounds write happened.

The new value seems to be used only as ctoff in handle_response_icmp()
and as wlen in ip_vs_icmp_xmit_v6():

net/netfilter/ipvs/ip_vs_xmit.c:ip_vs_icmp_xmit_v6() {
	...
	wlen = ciph->len;
	...
}

Would the comment be clearer if it said that len must cover the embedded
IPv6 header that ip_vs_nat_icmp_v6() rewrites? The same comment appears
again in ip_vs_in_icmp_v6().

The commit message also says:

    This fix does not depend on patch 2.

Once this is applied, "patch 2" has no meaning in the git history. Could
the message name the companion commit instead: "ipv6: update *offset for
non-first fragments in ipv6_find_hdr()"?

That companion commit makes ipv6_find_hdr() set
*offset = start + sizeof(struct frag_hdr) on the non-first-fragment
early return. After it lands, can ciph.len == ciph.off still be true in
either function?

If not, the new branch can never be reached in mainline. The comment's
premise, "ipv6_find_hdr() does not include the embedded header for
non-first fragments", would then be false. Should the comment say it
applies only to kernels where ipv6_find_hdr() leaves *offset unchanged?

> +	if (ciph.len == ciph.off)
> +		ciph.len += sizeof(struct ipv6hdr);
> +
>  	pp = ip_vs_proto_get(ciph.protocol);
>  	if (!pp)
>  		return NF_ACCEPT;

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260925141155.17603-1-axel.mierczuk%401password.com