On 6/17/26 16:01, Ilya Maximets wrote:

On 6/17/26 10:08 PM, Gustavo A. R. Silva wrote:

quoted

Hi,

On 6/16/26 04:03, Ilya Maximets wrote:

quoted

kmalloc_flex() in metadata_dst_alloc() sets __counted_by for the
structure to the options_len, which is then initialized to zero.
Later, we're initializing the structure by copying the tunnel info
together with the options, and this triggers a warning for a potential
memcpy overflow, since the compiler estimates that the options can't
fit into the structure, even though the memory for them is actually
allocated.

   memcpy: detected buffer overflow: 104 byte write of buffer size 96
   WARNING: CPU: X PID: Y at lib/string_helpers.c:1036 __fortify_report
    skb_tunnel_info_unclone+0x179/0x190
    geneve_xmit+0x7fe/0xe00

This warning has nothing to do with counted_by. See below for more
comments.

quoted

The issue is triggered when built with clang and source fortification.

Fix that by doing the copy in two stages: first - the main data with
the options_len, then the options.  This way the correct length should
be known at the time of the copy.

It would be better if the options_len never changed after allocation,
but the allocation code is a little separate from the initialization
and it would be awkward and potentially dangerous to return a struct
with options_len set to a non-zero value from the metadata_dst_alloc().

Another option would be to use ip_tunnel_info_opts_set(), but it is
doing too many unnecessary operations for the use case here.

Fixes: 69050f8d6d07 ("treewide: Replace kmalloc with kmalloc_obj for non-scalar types")
Reported-by: Johan Thomsen <redacted>
Closes: https://lore.kernel.org/netdev/CAKv6aAM8_EWgXScnKmKYm_4SwGDVBK++dzfP+Y6msUXbp99QUw@mail.gmail.com/ (local)
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
---

Johan, if you can test this one in your setup as well, that would
be great.  Thanks.

   include/net/dst_metadata.h | 7 +++++--
   1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/include/net/dst_metadata.h b/include/net/dst_metadata.h
index 1fc2fb03ce3f..f45d1e3163f0 100644
--- a/include/net/dst_metadata.h
+++ b/include/net/dst_metadata.h

@@ -164,8 +164,11 @@ static inline struct metadata_dst *tun_dst_unclone(struct sk_buff *skb)
   	if (!new_md)
   		return ERR_PTR(-ENOMEM);
   
-	memcpy(&new_md->u.tun_info, &md_dst->u.tun_info,
-	       sizeof(struct ip_tunnel_info) + md_size);

What's going on here is that, internally, fortified memcpy() retrieves
the destination size via __builtin_dynamic_object_size() in mode 1.

That is:

__builtin_dynamic_object_size(&new_md->u.tun_info, 1)

For the above case, Clang returns sizeof(new_md->u.tun_info) == 96.

So the warning is reporting that 104 bytes don't fit in an object of
size 96 bytes, regardless of any counted_by annotation or allocation.

Hmm.  Does __builtin_dynamic_object_size(&new_md->u.tun_info, 1) return
104 when the options_len is 8?  If so, isn't that because it is counted
by that field?  Asking because the fortification doesn't complain if we
keep the full 104-byte copy as-is, but set the options_len beforehand,
as tested by Johan.

I see. If that is the case, then, internally, fortified memcpy() ends up
using mode 0 instead of mode 1. Something like this:

__builtin_dynamic_object_size(&new_md->u.tun_info, 0)

The above will effectively consider the allocation and counted_by because
it will interpret new_md->u.tun_info as an open-ended object due to the
flexible-array member (in struct ip_tunnel_info) whose size is determined
by counted_by.

I'm not entirely convinced we really want this.

-Gustavo

quoted

Of course, in this case, the write of 104 bytes into new_md->u.tun_info
is intentional and controlled, but what if it weren't?

On the other hand, for this same case, GCC currently returns SIZE_MAX,
which translates to -1 inside fortified memcpy(). Thus, bounds-checking
is bypassed, which is why this warning doesn't show up with GCC.

However, this is a bug in GCC. We're already looking into that.

I think we've had just a handful of cases like this across the whole
kernel tree. We can deal with them as you did here (by directly copying
the composite structure first, and then using memcpy() to copy into the
flexible-array member). If these cases ever become more common, we
could create some kind of helper to do both things at once. :)

quoted

+	/* Copy in two stages to keep the __counted_by happy. */

So based on my comments above, this code comment is not correct.

I feel like some comment is still needed, do you have some suggestions
for what would be a better wording?

quoted

quoted

+	new_md->u.tun_info = md_dst->u.tun_info;

This is fine.

quoted

+	memcpy(ip_tunnel_info_opts(&new_md->u.tun_info),
+	       ip_tunnel_info_opts(&md_dst->u.tun_info), md_size);

Is ip_tunnel_info_opts() really needed here?

Probably this works just fine:

memcpy(new_md->u.tun_info.options, md_dst->u.tun_info.options, md_size);

The logic here is: we have the access function, therefore we should use it.
It gives a bad example if we don't.

Best regards, Ilya Maximets.