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

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

[Ilya Maximets]

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

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);
+	/* Copy in two stages to keep the __counted_by happy. */
+	new_md->u.tun_info = md_dst->u.tun_info;
+	memcpy(ip_tunnel_info_opts(&new_md->u.tun_info),
+	       ip_tunnel_info_opts(&md_dst->u.tun_info), md_size);
+
 #ifdef CONFIG_DST_CACHE
 	/* Unclone the dst cache if there is one */
 	if (new_md->u.tun_info.dst_cache.cache) {
-- 
2.54.0

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

[Gustavo A. R. Silva]

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

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

[Ilya Maximets]

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

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.

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

[Gustavo A. R. Silva]

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

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

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

[Gustavo A. R. Silva]

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

Thanks
-Gustavo

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

[Ilya Maximets]

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

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

[Johan Thomsen]

> 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);
> +       /* Copy in two stages to keep the __counted_by happy. */
> +       new_md->u.tun_info = md_dst->u.tun_info;
> +       memcpy(ip_tunnel_info_opts(&new_md->u.tun_info),
> +              ip_tunnel_info_opts(&md_dst->u.tun_info), md_size);
> +
>  #ifdef CONFIG_DST_CACHE
>         /* Unclone the dst cache if there is one */
>         if (new_md->u.tun_info.dst_cache.cache) {

Hi Ilya,

Sure. Just stressed it for 24 hours and - I cannot trigger the bug
with this patch applied.

BR
Johan

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

[Ilya Maximets]

On 6/18/26 1:43 PM, Johan Thomsen wrote:
>> 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);
>> +       /* Copy in two stages to keep the __counted_by happy. */
>> +       new_md->u.tun_info = md_dst->u.tun_info;
>> +       memcpy(ip_tunnel_info_opts(&new_md->u.tun_info),
>> +              ip_tunnel_info_opts(&md_dst->u.tun_info), md_size);
>> +
>>  #ifdef CONFIG_DST_CACHE
>>         /* Unclone the dst cache if there is one */
>>         if (new_md->u.tun_info.dst_cache.cache) {
> 
> Hi Ilya,
> 
> Sure. Just stressed it for 24 hours and - I cannot trigger the bug
> with this patch applied.

Thanks, Johan!

Best regards, Ilya Maximets.

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

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Tue, 16 Jun 2026 12:03:29 +0200 you 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.
> 
> [...]

Here is the summary with links:
  - [net] net: dst_metadata: fix false-positive memcpy overflow in tun_dst_unclone
    https://git.kernel.org/netdev/net/c/4c6d43db2a4d

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html