Re: [PATCH net] net: dst_metadata: fix false-positive memcpy overflow in tun_dst_unclone

["Gustavo A. R. Silva" <gustavo@embeddedor.com>]

Hi,

On 6/16/26 04:03, Ilya Maximets wrote:
> 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.

> 
> 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 <write@ownrisk.dk>
> Closes: https://lore.kernel.org/netdev/CAKv6aAM8_EWgXScnKmKYm_4SwGDVBK++dzfP+Y6msUXbp99QUw@mail.gmail.com/
> 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.

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. :)

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

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

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

This is fine.

> +	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);

-Gustavo