Re: [PATCH net 01/15] 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] Commit "ipvs: avoid out-of-bounds write in ip_vs_nat_icmp_v6" has
  only `Fixes: 63dca2c0b0e7`.
- [Low] The commit message says "This fix does not depend on patch 2".

--

ipvs: avoid out-of-bounds write in ip_vs_nat_icmp_v6

This patch changes both ICMPv6 handlers in IPVS. When ipv6_find_hdr()
stops at a non-first fragment and leaves ciph.len == ciph.off, the
embedded IPv6 header is now added to ciph.len. The skb_ensure_writable()
range used by the NAT paths then covers the addresses that
ip_vs_nat_icmp_v6() rewrites.

> This fix does not depend on patch 2. Stable kernels without commit
> 342e24a339b9 ("ipvs: do not mangle ICMP replies for non-first fragments")
> also need that commit to handle incoming ICMPv6 errors quoting
> non-first ESP fragments.
>
> Fixes: 63dca2c0b0e7 ("ipvs: Fix faulty IPv6 extension header handling in IPVS")

[Severity: Low]
The incoming half of this bug only seems to have become reachable with
342e24a339b9. That path is
ip_vs_in_icmp_v6()->ip_vs_icmp_xmit_v6()->ip_vs_nat_icmp_v6().

Before that commit, ip_vs_in_icmp_v6() had:

	if (ciph.fragoffs)
		return NF_ACCEPT;

342e24a339b9 changed it to:

	if (ciph.fragoffs && !pp->dont_defrag)
		return NF_ACCEPT;

ESP sets .dont_defrag = 1 in ip_vs_proto_ah_esp.c, so non-first ESP
fragments now reach ip_vs_icmp_xmit_v6(). There, wlen = ciph->len sizes
skb_ensure_writable(), and without this fix ciph->len equals ciph->off.
ip_vs_nat_icmp_v6() then writes the embedded addresses past that range.

The paragraph above reads as if 342e24a339b9 is only a functional
prerequisite. Should it say that 342e24a339b9 exposed the incoming-path
write, and add a second Fixes: 342e24a339b9 tag? The existing
Fixes: 63dca2c0b0e7 tag plus Cc: stable already covers backports, so
this is mostly about traceability.

> diff --git a/net/netfilter/ipvs/ip_vs_core.c b/net/netfilter/ipvs/ip_vs_core.c
> index 8a3c880fdb266..76213159404f0 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
> +	 */
> +	if (ciph.len == ciph.off)
> +		ciph.len += sizeof(struct ipv6hdr);

[Severity: Low]
This isn't a bug, but is "so that ESP can pass" accurate for the IPv6
paths? A len vs off comparison only exists on the IPv4 side, for example
in ip_vs_nat_icmp():

	if (cih->ihl * 4 != ciph->len - ciph->off)
		return false;

None of these compare ciph.len with ciph.off:

- ip_vs_out_icmp_v6()
- ip_vs_in_icmp_v6()
- handle_response_icmp()
- ip_vs_icmp_xmit_v6()
- ip_vs_nat_icmp_v6()
- the AH/ESP conn_in_get/conn_out_get handlers

So the only effect seems to be the larger skb_ensure_writable() range.

Later in the series, "ipv6: update *offset for non-first fragments in
ipv6_find_hdr()" makes ipv6_find_hdr() do this for non-first fragments:

	*offset = start + sizeof(struct frag_hdr);

After that change, the first clause of this comment is no longer true.
The ciph.len == ciph.off check also can't fire any more, here or in the
copy added to ip_vs_in_icmp_v6().

Separately, the commit message says "This fix does not depend on patch
2." Once merged, "patch 2" won't point to anything in git history. Could
it name "ipv6: update *offset for non-first fragments in ipv6_find_hdr()"
instead?

> +
>  	pp = ip_vs_proto_get(ciph.protocol);
>  	if (!pp)
>  		return NF_ACCEPT;

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261113011447.2357709-2-pablo%40netfilter.org