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

On 10/3/26 3:24 AM, Jean-Paul Sergent wrote:

memcmp: detected buffer overflow: 108 byte read of buffer size 96
 WARNING: CPU: 0 PID: 15 at lib/string_helpers.c:1036 __fortify_report+0x45/0x60
  __fortify_panic+0x9/0x10
  skb_metadata_dst_cmp+0x11b/0x120
  dev_gro_receive+0x303/0x620
  gro_receive_skb+0xc5/0x230
  gro_cell_poll+0x67/0xa0

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, and the combined
struct+options memcmp trips CONFIG_FORTIFY_SOURCE when built with
clang. Observed live on 6.18.54-talos with Cilium geneve, in
gro_cell_poll (gro_cells GRO on the geneve device) under sustained
cross-node RX; the warning is followed by a fatal Oops
(kernel BUG at lib/string_helpers.c:1043).

This doesn't make sense to me.  The problem in tun_dst_unclone was
at the initialization time, where we couldn't write the options_len
together with the options while the currently stored value is zero.

Here the function just compares two blocks and they must be already
fully initialized and have options_len properly set.  If they have
options, but the length is zero, that's a bug somewhere else.

While here, fix a related overread: the memcmp length uses
a->u.tun_info.options_len for BOTH sides, so when b carries fewer
options than a the comparison reads past b's allocation. Pre-check
that both sides carry the same options_len

This makes sense and may be the real bug here?  If options actually
have different length for some reason, then the memcmp will rightly
trigger the fortification check as it should.

However, someone more familiar with GRO should probably look at this
to see how the comparison should behave when options are different as
it sounds a little weird that they are.

and compare the options
through ip_tunnel_info_opts() so the counted_by view matches the read
length (the same two-stage shape the unclone fix uses).

This makes no sense.  Single memcmp should work just fine as long as the
compared size doesn't exceed the actual size of both memory regions.

Do you still see the fortification issue trigger with just the length
comparison change?

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

This is likely wrong and should point to the commit that added the
tunnel info comparison.

Cc: stable@vger.kernel.org
Reported-by: Jean-Paul Sergent <redacted>

If you are the author you need a sign-off instead of a reported-by.

Also, you're missing a lot of maintainers in the Cc list.

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

There is no point linking the same thread where you're posting a patch,
it will be linked anyway on commit.

Assisted-by: LLM

v2: fix subject prefix to [PATCH net]; no code changes.

This should not be in the commit message.  Also, there should be a link
to the previous version here.

This is obvious, drop the comment.

This part of the change doesn't make much sense, but anyway, when asking
LLMs to write comments, please ask them to be concise.  There is too much
stuff in there that makes no sense in the context of the code, e.g. the
mentioning of the "tun_dst_unclone fix", and the comment is generally way
too long for what it tries to accomplish.  It should be 2 lines at most
in this particular case.

Same applies to the commit message, there is too much fluff in there that
makes it harder to read.

So, please, do some quality control before sending patches, read what
you're sending.  Don't just shoot out AI slop.  Next person may not be
that kind in their replies.

Best regards, Ilya Maximets.