On 6/18/26 6:02 AM, Gustavo A. R. Silva wrote:
>
>
> On 6/17/26 16:59, Gustavo A. R. Silva wrote:
>>
>>
>> On 6/17/26 16:01, Ilya Maximets wrote:
>>> 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.
>>
>> 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.
>
> Indeed. The execution stops here:
>
> fortify_memcpy_chk():
> 588 /*
> 589 * Always stop accesses beyond the struct that contains the
> 590 * field, when the buffer's remaining size is known.
> 591 * (The SIZE_MAX test is to optimize away checks where the buffer
> 592 * lengths are unknown.)
> 593 */
> 594 if (p_size != SIZE_MAX && p_size < size)
> 595 fortify_panic(func, FORTIFY_WRITE, p_size, size, true);
>
> with p_size = __builtin_dynamic_object_size(&new_md->u.tun_info, 0)
>
> The code never reaches the part where p_size_field
> (__bdos(&new_md->u.tun_info, 1))
> is checked at runtime because there is no need for that.
>
> So yep, this patch is okay as-is.
Ack. Thanks for looking into this!
Best regards, Ilya Maximets.
