On 6/17/26 10:08 PM, Gustavo A. R. Silva wrote:
> 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 <[email protected]>
>> Closes:
>> https://lore.kernel.org/netdev/cakv6aam8_ewgxscnkmkym_4swgdvbk++dzfp+y6msuxbp99...@mail.gmail.com/
>> Signed-off-by: Ilya Maximets <[email protected]>
>> ---
>>
>> 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.
>
> 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.
I feel like some comment is still needed, do you have some suggestions
for what would be a better wording?
>
>> + 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);
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.
