Re: [PATCH net 4/7] ipvs: revalidate ihl before icmp_send

[Jakub Kicinski <kuba@kernel.org>]

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: revalidate ihl before icmp_send

This adds a check in ip_vs_in_icmp() that drops the packet when the
embedded IP header's ihl no longer matches the length that was validated
from the frag copy, sets the transport header before icmp_send(), and
passes the tunnel protocol to ipv4_update_pmtu() instead of 0.

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

A few questions below.

> diff --git a/net/netfilter/ipvs/ip_vs_core.c b/net/netfilter/ipvs/ip_vs_core.c
> index ba0957798bad0..c41fac3ed873e 100644
> --- a/net/netfilter/ipvs/ip_vs_core.c
> +++ b/net/netfilter/ipvs/ip_vs_core.c
> @@ -1934,7 +1934,8 @@ ip_vs_in_icmp(struct netns_ipvs *ipvs, struct sk_buff *skb, int *related,
>  			IP_VS_DBG(12, "ICMP for %s %pI4->%pI4: mtu=%u\n",
>  				  outer_proto, &ip_hdr(skb)->saddr,
>  				  &ip_hdr(skb)->daddr, mtu);
> -			ipv4_update_pmtu(skb, ipvs->net, mtu, 0, 0);
> +			ipv4_update_pmtu(skb, ipvs->net, mtu, 0,
> +					 ip_hdr(skb)->protocol);

[Severity: Medium]

The new protocol argument becomes fl4.flowi4_proto inside
ipv4_update_pmtu()->__build_flow_key(), and it is used both by
__ip_route_output_key() and by the fib_lookup() in
__ip_rt_update_pmtu().

The route IPVS actually transmits these tunnel packets on is looked up
and cached by do_output_route4(), which leaves flowi4_proto at zero:

net/netfilter/ipvs/ip_vs_xmit.c:do_output_route4() {
	memset(&fl4, 0, sizeof(fl4));
	fl4.daddr = daddr;
	fl4.flowi4_flags = (rt_mode & IP_VS_RT_MODE_KNOWN_NH) ?
			   FLOWI_FLAG_KNOWN_NH : 0;
	...
}

and fib4_rule_match() keys on that field:

net/ipv4/fib_rules.c:fib4_rule_match() {
	...
	if (rule->ip_proto && (rule->ip_proto != fl4->flowi4_proto))
		return 0;
	...
}

With an FRA_IP_PROTO rule configured, can the two lookups now land in
different tables, so the exception ends up on a nexthop IPVS never
consults?  The read-back immediately below would then still return the
unreduced MTU:

			if (dest) {
				struct ip_vs_dest_dst *dest_dst;

				dest_dst = rcu_dereference(dest->dest_dst);
				if (dest_dst)
					mtu = dst_mtu(dest_dst->dst_cache);
			}

and the ICMP relayed to the client would advertise the stale larger
value, leaving the passive PMTUD added by f2edb9f7706d ineffective for
such setups.

The multipath variant of this does not seem to apply, since
__ip_rt_update_pmtu() installs the exception on every path when
fib_info_num_path(res.fi) > 1.

Other in-tree callers of ipv4_update_pmtu() pass the real protocol, but
they also set flowi4_proto on their transmit lookup; IPVS does not.
Would it make sense to set flowi4_proto in do_output_route4() as well,
so the two keys agree?

[Severity: Medium]

Under the same assumption the commit message makes, that a local process
can mutate the bytes between the frag read and the pull, is the protocol
byte read here trusted?

Everything that authorized this tunnel was checked on the on-stack copy
taken earlier in ip_vs_in_icmp():

	cih = skb_header_pointer(skb, offset, sizeof(_ciph), &_ciph);
	if (!cih)
		return NF_ACCEPT;	/* The packet looks wrong, ignore */
	hlen_ipip = cih->ihl * 4;
	if (!(cih->version == 4 && hlen_ipip >= sizeof(struct iphdr)))
		return NF_ACCEPT;	/* The packet looks wrong, ignore */
	raddr = (union nf_inet_addr *)&cih->daddr;
	...
	if (cih->protocol == IPPROTO_IPIP) {
	...
	} else if ((cih->protocol == IPPROTO_UDP ||
		    cih->protocol == IPPROTO_GRE) &&

whereas ip_hdr(skb)->protocol here is re-read from the linear area after
pskb_may_pull(skb, hlen_ipip).  If those bytes can change, the value fed
to ipv4_update_pmtu(), and hence fl4.flowi4_proto for
__ip_route_output_key() and update_or_create_fnhe(), is not necessarily
the IPIP/UDP/GRE value that was validated, which works against the
"provide correct protocol to ipv4_update_pmtu()" goal.

Would saving cih->protocol into a local at validation time and passing
that local here be preferable to re-reading the header?

For what it is worth, the saddr/daddr/tos that __build_flow_key() reads
from skb->data at this call site have the same exposure, but that part
predates this patch.

>  			/* Client uses PMTUD? */
>  			if (!(frag_off & htons(IP_DF)))
>  				goto ignore_tunnel;
> @@ -1960,6 +1961,12 @@ ip_vs_in_icmp(struct netns_ipvs *ipvs, struct sk_buff *skb, int *related,
>  		/* Ensure the IP header is present in headroom */
>  		if (!pskb_may_pull(skb, hlen_orig))
>  			goto ignore_tunnel;
> +		skb_set_transport_header(skb, hlen_orig);
> +		/* Before now we may used ihl from skb frag, revalidate it after
> +		 * copying it into skb head to prevent out-of-bounds access
> +		 */
> +		if (ip_hdr(skb)->ihl * 4 != hlen_orig)
> +			goto ignore_tunnel;

[Severity: Medium]

Is the "out-of-bounds access" wording in this comment, and in the commit
message, accurate for the current tree?

The commit message says:

    "we must be careful and revalidate the embedded headers after
    reading them from the skb frags to prevent out-of-bounds
    access"

and

    "local process can change the ihl field and after
    pskb_may_pull() we can see larger value which is a problem
    for the icmp_send() call."

Every ihl-derived access reachable from this icmp_send() call looks
bounds-checked.  The tracepoint in include/trace/events/icmp.h reads the
quoted header through skb_header_pointer() and copes with a NULL return:

	if (iph->protocol == IPPROTO_UDP)
		uh = skb_header_pointer(skb,
					skb_network_offset(skb) +
					(iph->ihl << 2),
					sizeof(_uh), &_uh);

	if (!uh) {
	...

__icmp_send() does the same for its inner probe:

net/ipv4/icmp.c:__icmp_send() {
	...
		itp = skb_header_pointer(skb_in,
					 skb_network_header(skb_in) +
					 (iph->ihl << 2) + ...
		if (!itp)
			goto out;
	...
}

and __ip_options_echo() works on IPCB(skb)->opt, which is zeroed just
above the pull:

		memset(&(IPCB(skb)->opt), 0, sizeof(IPCB(skb)->opt));

so it returns immediately on optlen == 0.  With that, does a mutated ihl
today produce anything worse than wrong or zeroed tracepoint fields?

Related question on the Fixes tag:

    Fixes: 7fcc2fe39fed ("net: icmp: avoid invalid transport header access in icmp_send tracepoint")

That commit replaced the skb_transport_header() dereference in the
tracepoint with the bounds-checked skb_header_pointer() read shown above,
so it introduced the ihl-based offset but not an out-of-bounds access.
Since there is no remaining skb_transport_header() consumer in the
icmp_send() path, what does the added skb_set_transport_header(skb,
hlen_orig) feed?  Could the tag and the comment be reworded to describe
this as hardening plus a transport-header fixup, rather than a fix for an
out-of-bounds read?

>  		IP_VS_DBG(12, "Sending ICMP for %pI4->%pI4: t=%u, c=%u, i=%u\n",
>  			&ip_hdr(skb)->saddr, &ip_hdr(skb)->daddr,
>  			type, code, ntohl(info));