Re: [PATCH net v2] net: dst_metadata: fix fortify trap + overread in skb_metadata_dst_cmp

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 2 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The commit message and the new code comment give the wrong
  root cause, while the code change itself works.
- [Medium] The Fixes: tag points at 69050f8d6d07 ("treewide: Replace
  kmalloc with kmalloc_obj for non-scalar types", Feb 2026, first in…
- [Low] The commit has no Signed-off-by:.

--

net: dst_metadata: fix fortify trap + overread in skb_metadata_dst_cmp

This patch changes the METADATA_IP_TUNNEL case of skb_metadata_dst_cmp()
to address a FORTIFY_SOURCE memcmp() panic in the GRO path with geneve.
Dsts with different options_len values are now rejected up front.
The struct and the options are then compared with two separate memcmp()
calls.

Closes: https://lore.kernel.org/netdev/20261003005449.2675.1@jpsergent.gmail.com/

> Fixes: 69050f8d6d07 ("treewide: Replace kmalloc with kmalloc_obj for non-scalar types")

[Severity: Medium]

Is this the right Fixes: target? 69050f8d6d07 first appears in v7.0, but
the panic was reproduced on 6.18.54. In v6.18, metadata_dst_alloc() still
uses plain kmalloc(struct_size(md_dst, u.tun_info.options, optslen),
flags).

FORTIFY can size both objects here because of the __counted_by
annotation added by bb5e62f2d547 ("net: Add options as a flexible array
to struct ip_tunnel_info"), which is in v6.15:

	u8			options[] __aligned_largest __counted_by(options_len);

The memcmp() itself goes back to 3fcece12bc1b (2017).

With the current tag, would stable tooling skip 6.15 through 6.18,
including the 6.18 kernel where this was observed?

> Cc: stable@vger.kernel.org
> Reported-by: Jean-Paul Sergent <jpsergent@gmail.com>
> Closes: https://lore.kernel.org/netdev/20261003005449.2675.1@jpsergent.gmail.com/
> Assisted-by: LLM
>
> v2: fix subject prefix to [PATCH net]; no code changes.

[Severity: Low]

The commit has no Signed-off-by: line.
Documentation/process/coding-assistants.rst says the human submitter
must add their own Signed-off-by to certify the DCO. Can the patch be
applied without it?

The v2 changelog is also in the commit body, not below the --- line, so
git am would keep it in the permanent history. Could it be moved below
the ---?

> diff --git a/include/net/dst_metadata.h b/include/net/dst_metadata.h
> index f45d1e3163f00..60878ea1eb50a 100644
> --- a/include/net/dst_metadata.h
> +++ b/include/net/dst_metadata.h
> @@ -115,10 +115,30 @@ static inline int skb_metadata_dst_cmp(const struct sk_buff *skb_a,
>  	case METADATA_HW_PORT_MUX:
>  		return memcmp(&a->u.port_info, &b->u.port_info,
>  			      sizeof(a->u.port_info));
> -	case METADATA_IP_TUNNEL:
> -		return memcmp(&a->u.tun_info, &b->u.tun_info,
> -			      sizeof(a->u.tun_info) +
> -					 a->u.tun_info.options_len);
> +	case METADATA_IP_TUNNEL: {
> +		int ret;
> +
> +		/* Options lengths must match, or the options memcmp below
> +		 * would read past b's allocation when b carries fewer
> +		 * options than a.
> +		 */
> +		if (a->u.tun_info.options_len != b->u.tun_info.options_len)
> +			return 1;
> +		ret = memcmp(&a->u.tun_info, &b->u.tun_info,
> +			     sizeof(a->u.tun_info));
> +		if (ret)
> +			return ret;
> +		/* Compare the options through the flex-array member so the
> +		 * compiler's __counted_by(options_len) view stays consistent
> +		 * with the read length (same shape as the tun_dst_unclone
> +		 * fix); a single memcmp of struct+options trips
> +		 * CONFIG_FORTIFY_SOURCE when options_len is still 0 from
> +		 * allocation time.
> +		 */

[Severity: Medium]

Is this the right explanation for the trap? The commit message says the
same thing:

  Same disease as the tun_dst_unclone fix (4c6d43db2a4d), on the RX
  sibling: kmalloc_flex() in metadata_dst_alloc() sets __counted_by for
  the structure to options_len, which is then initialized to zero, so
  the compiler's view of the metadata_dst tail is 96 bytes at the time
  of the access. Geneve carries 108 bytes of options, ...

The old code computed the read length from the current value:
sizeof(a->u.tun_info) + a->u.tun_info.options_len. sizeof(struct
ip_tunnel_info) is 96. A 108 byte read therefore means
a->u.tun_info.options_len was 12 when the call ran. So geneve carried
12 bytes of options, not 108.

The fortified memcmp() checks the first buffer before the second:

include/linux/fortify-string.h:memcmp() {
    ...
	if (p_size < size)
		fortify_panic(FORTIFY_FUNC_memcmp, FORTIFY_READ, p_size, size, INT_MIN);
	else if (q_size < size)
		fortify_panic(FORTIFY_FUNC_memcmp, FORTIFY_READ, q_size, size, INT_MIN);
    ...
}

For a, p_size is 96 + 12 = 108, so the reported 96 has to be q_size.
That means b->u.tun_info.options_len was 0.

On RX, neither dst still holds its allocation-time zero when
gro_list_prepare() calls skb_metadata_dst_cmp(). geneve_rx() fills in
the length right after udp_tun_rx_dst():

drivers/net/geneve.c:geneve_rx() {
    ...
		ip_tunnel_info_opts_set(&tun_dst->u.tun_info,
					gnvh->options, gnvh->opt_len * 4,
					flags);
    ...
}

If options_len really were 0 at compare time, the read length would be
96 and nothing would trap.

So isn't FORTIFY correctly catching a compare of a (12 bytes of options)
against b (no options), with a's length used for both buffers? If so,
the options_len pre-check, which the commit message calls a "While here"
fix, is the actual fix.

Once the lengths are known to match, a single struct+options memcmp()
fits both counted_by views. Is the split into two memcmp() calls needed?

options_len sits at offset 88 and options[] at offset 96. The old
memcmp() would hit the options_len difference before it reached b's
missing options. So the user-visible failure looks like the FORTIFY
panic, not a real overread past b's allocation.

Could the commit message and this comment describe the mismatched
options_len case instead of an allocation-time zero?

> +		return memcmp(ip_tunnel_info_opts(&a->u.tun_info),
> +			      ip_tunnel_info_opts(&b->u.tun_info),
> +			      a->u.tun_info.options_len);
> +	}

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261003005449.2675.3%40jpsergent.gmail.com