|
| 1 | +From 4c6d43db2a4d2cef3921e885cf34798f790d34ea Mon Sep 17 00:00:00 2001 |
| 2 | +From: Ilya Maximets <i.maximets@ovn.org> |
| 3 | +Date: Tue, 16 Jun 2026 12:03:29 +0200 |
| 4 | +Subject: net: dst_metadata: fix false-positive memcpy overflow in |
| 5 | + tun_dst_unclone |
| 6 | + |
| 7 | +kmalloc_flex() in metadata_dst_alloc() sets __counted_by for the |
| 8 | +structure to the options_len, which is then initialized to zero. |
| 9 | +Later, we're initializing the structure by copying the tunnel info |
| 10 | +together with the options, and this triggers a warning for a potential |
| 11 | +memcpy overflow, since the compiler estimates that the options can't |
| 12 | +fit into the structure, even though the memory for them is actually |
| 13 | +allocated. |
| 14 | + |
| 15 | + memcpy: detected buffer overflow: 104 byte write of buffer size 96 |
| 16 | + WARNING: CPU: X PID: Y at lib/string_helpers.c:1036 __fortify_report |
| 17 | + skb_tunnel_info_unclone+0x179/0x190 |
| 18 | + geneve_xmit+0x7fe/0xe00 |
| 19 | + |
| 20 | +The issue is triggered when built with clang and source fortification. |
| 21 | + |
| 22 | +Fix that by doing the copy in two stages: first - the main data with |
| 23 | +the options_len, then the options. This way the correct length should |
| 24 | +be known at the time of the copy. |
| 25 | + |
| 26 | +It would be better if the options_len never changed after allocation, |
| 27 | +but the allocation code is a little separate from the initialization |
| 28 | +and it would be awkward and potentially dangerous to return a struct |
| 29 | +with options_len set to a non-zero value from the metadata_dst_alloc(). |
| 30 | + |
| 31 | +Another option would be to use ip_tunnel_info_opts_set(), but it is |
| 32 | +doing too many unnecessary operations for the use case here. |
| 33 | + |
| 34 | +Fixes: 69050f8d6d07 ("treewide: Replace kmalloc with kmalloc_obj for non-scalar types") |
| 35 | +Reported-by: Johan Thomsen <write@ownrisk.dk> |
| 36 | +Closes: https://lore.kernel.org/netdev/CAKv6aAM8_EWgXScnKmKYm_4SwGDVBK++dzfP+Y6msUXbp99QUw@mail.gmail.com/ |
| 37 | +Signed-off-by: Ilya Maximets <i.maximets@ovn.org> |
| 38 | +Link: https://patch.msgid.link/20260616100332.1308294-1-i.maximets@ovn.org |
| 39 | +Signed-off-by: Jakub Kicinski <kuba@kernel.org> |
| 40 | +--- |
| 41 | + include/net/dst_metadata.h | 7 +++++-- |
| 42 | + 1 file changed, 5 insertions(+), 2 deletions(-) |
| 43 | + |
| 44 | +diff --git a/include/net/dst_metadata.h b/include/net/dst_metadata.h |
| 45 | +index 1fc2fb03ce3f9..f45d1e3163f00 100644 |
| 46 | +--- a/include/net/dst_metadata.h |
| 47 | ++++ b/include/net/dst_metadata.h |
| 48 | +@@ -164,8 +164,11 @@ static inline struct metadata_dst *tun_dst_unclone(struct sk_buff *skb) |
| 49 | + if (!new_md) |
| 50 | + return ERR_PTR(-ENOMEM); |
| 51 | + |
| 52 | +- memcpy(&new_md->u.tun_info, &md_dst->u.tun_info, |
| 53 | +- sizeof(struct ip_tunnel_info) + md_size); |
| 54 | ++ /* Copy in two stages to keep the __counted_by happy. */ |
| 55 | ++ new_md->u.tun_info = md_dst->u.tun_info; |
| 56 | ++ memcpy(ip_tunnel_info_opts(&new_md->u.tun_info), |
| 57 | ++ ip_tunnel_info_opts(&md_dst->u.tun_info), md_size); |
| 58 | ++ |
| 59 | + #ifdef CONFIG_DST_CACHE |
| 60 | + /* Unclone the dst cache if there is one */ |
| 61 | + if (new_md->u.tun_info.dst_cache.cache) { |
| 62 | +-- |
| 63 | +cgit 1.3-korg |
0 commit comments