* [PATCH net-next] net: Silence false field-spanning write warning in ip_tunnel_info_opts_set() memcpy
@ 2025-01-07 16:55 Gal Pressman
2025-01-07 23:28 ` Kees Cook
0 siblings, 1 reply; 6+ messages in thread
From: Gal Pressman @ 2025-01-07 16:55 UTC (permalink / raw)
To: David S. Miller, Jakub Kicinski
Cc: netdev, Eric Dumazet, Paolo Abeni, David Ahern, Simon Horman,
Kees Cook, linux-hardening, Gal Pressman, Cosmin Ratiu
When metadata_dst struct is allocated (using metadata_dst_alloc()), it
reserves room for options at the end of the struct.
Similar to [1], change the memcpy() to unsafe_memcpy() as it is
guaranteed that enough room (md_size bytes) was allocated and the
field-spanning write is intentional.
This resolves the following warning:
memcpy: detected field-spanning write (size 8) of single field "_Generic(info, const struct ip_tunnel_info * : ((const void *)((info) + 1)), struct ip_tunnel_info * : ((void *)((info) + 1)) )" at include/net/ip_tunnels.h:662 (size 0)
WARNING: CPU: 0 PID: 19184 at include/net/ip_tunnels.h:662 validate_and_copy_set_tun+0x2f0/0x308 [openvswitch]
Modules linked in: act_csum act_pedit act_tunnel_key geneve ip6_udp_tunnel udp_tunnel nf_conntrack_netlink act_skbedit act_mirred sbsa_gwdt ipmi_devintf ipmi_msghandler nvme_fabrics nvme_core overlay optee cls_matchall nfnetlink_cttimeout act_gact cls_flower sch_ingress openvswitch nsh nf_conncount nft_chain_nat xt_MASQUERADE nf_nat xt_tcpmss ib_srp scsi_transport_srp xt_NFLOG nfnetlink_log xt_recent xt_hashlimit xt_state xt_conntrack nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 xt_mark xt_comment ipt_REJECT nf_reject_ipv4 rpcrdma nft_compat rdma_ucm ib_umad ib_iser binfmt_misc rdma_cm ib_ipoib iw_cm nf_tables libiscsi scsi_transport_iscsi nfnetlink ib_cm mlx5_ib ib_uverbs ib_core uio_pdrv_genirq uio mlxbf_pmc mlxbf_bootctl bluefield_edac sch_fq_codel dm_multipath fuse efi_pstore ip_tables crct10dif_ce mlx5_core mlxfw psample i2c_mlxbf pwr_mlxbf gpio_mlxbf2 mlxbf_gige mlxbf_tmfifo ipv6 crc_ccitt
CPU: 0 UID: 0 PID: 19184 Comm: handler2 Not tainted 6.12.0-for-upstream-bluefield-2024-11-29-01-33 #1
Hardware name: https://www.mellanox.com BlueField SoC/BlueField SoC, BIOS 4.9.0.13378 Oct 30 2024
pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
pc : validate_and_copy_set_tun+0x2f0/0x308 [openvswitch]
lr : validate_and_copy_set_tun+0x2f0/0x308 [openvswitch]
sp : ffffffc0aac6b420
x29: ffffffc0aac6b420 x28: 0000000000000040 x27: 0000000000000001
x26: 0000000000000008 x25: 0000000000000001 x24: 0000000000000800
x23: ffffff8082ab9c00 x22: ffffffc0aac6b480 x21: 0000000000000008
x20: ffffffc0aac6b830 x19: 0000000000000000 x18: ffffffffffffffff
x17: 1514131211100000 x16: ffffffea66bab8d8 x15: 2b20296f666e6928
x14: 28292a2064696f76 x13: 6c636e6920746120 x12: ffffffea68a6e5d0
x11: 0000000000000001 x10: 0000000000000001 x9 : ffffffea66c399b8
x8 : c0000000ffffefff x7 : ffffffea68a163e0 x6 : 0000000000000001
x5 : ffffff83fdeae488 x4 : 0000000000000000 x3 : 0000000000000027
x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffffff80979f2280
Call trace:
validate_and_copy_set_tun+0x2f0/0x308 [openvswitch] (P)
validate_and_copy_set_tun+0x2f0/0x308 [openvswitch] (L)
validate_set.constprop.0+0x2dc/0x438 [openvswitch]
__ovs_nla_copy_actions+0x404/0xd48 [openvswitch]
ovs_nla_copy_actions+0xb4/0x160 [openvswitch]
ovs_packet_cmd_execute+0x1bc/0x2f0 [openvswitch]
genl_family_rcv_msg_doit+0xd0/0x140
genl_rcv_msg+0x1f0/0x280
netlink_rcv_skb+0x64/0x138
genl_rcv+0x40/0x60
netlink_unicast+0x2e8/0x348
netlink_sendmsg+0x1ac/0x400
__sock_sendmsg+0x64/0xc0
____sys_sendmsg+0x26c/0x2f0
___sys_sendmsg+0x88/0xf0
__sys_sendmsg+0x88/0x100
__arm64_sys_sendmsg+0x2c/0x40
invoke_syscall+0x50/0x120
el0_svc_common.constprop.0+0x48/0xf0
do_el0_svc+0x24/0x38
el0_svc+0x34/0xf0
el0t_64_sync_handler+0x10c/0x138
el0t_64_sync+0x1ac/0x1b0
[1] Commit 13cfd6a6d7ac ("net: Silence false field-spanning write warning in metadata_dst memcpy")
Reviewed-by: Cosmin Ratiu <cratiu@nvidia.com>
Signed-off-by: Gal Pressman <gal@nvidia.com>
---
include/net/ip_tunnels.h | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/include/net/ip_tunnels.h b/include/net/ip_tunnels.h
index 1aa31bdb2b31..d5e163eba234 100644
--- a/include/net/ip_tunnels.h
+++ b/include/net/ip_tunnels.h
@@ -659,7 +659,10 @@ static inline void ip_tunnel_info_opts_set(struct ip_tunnel_info *info,
{
info->options_len = len;
if (len > 0) {
- memcpy(ip_tunnel_info_opts(info), from, len);
+ unsafe_memcpy(ip_tunnel_info_opts(info), from, len,
+ /* metadata_dst_alloc() reserves room (md_size bytes)
+ * for options right after the ip_tunnel_info struct.
+ */);
ip_tunnel_flags_or(info->key.tun_flags, info->key.tun_flags,
flags);
}
--
2.40.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH net-next] net: Silence false field-spanning write warning in ip_tunnel_info_opts_set() memcpy
2025-01-07 16:55 [PATCH net-next] net: Silence false field-spanning write warning in ip_tunnel_info_opts_set() memcpy Gal Pressman
@ 2025-01-07 23:28 ` Kees Cook
2025-01-08 6:56 ` Gal Pressman
2025-01-09 9:00 ` Gal Pressman
0 siblings, 2 replies; 6+ messages in thread
From: Kees Cook @ 2025-01-07 23:28 UTC (permalink / raw)
To: Gal Pressman, David S. Miller, Jakub Kicinski
Cc: netdev, Eric Dumazet, Paolo Abeni, David Ahern, Simon Horman,
linux-hardening, Cosmin Ratiu
On January 7, 2025 8:55:09 AM PST, Gal Pressman <gal@nvidia.com> wrote:
>When metadata_dst struct is allocated (using metadata_dst_alloc()), it
>reserves room for options at the end of the struct.
>
>Similar to [1], change the memcpy() to unsafe_memcpy() as it is
>guaranteed that enough room (md_size bytes) was allocated and the
>field-spanning write is intentional.
Why not just add an "options" flex array to struct ip_tunnel_info?
E.g.:
struct ip_tunnel_info {
struct ip_tunnel_key key;
struct ip_tunnel_encap encap;
#ifdef CONFIG_DST_CACHE
struct dst_cache dst_cache;
#endif
u8 options_len;
u8 mode;
u8 options[] __counted_by(options_len);
};
>
>This resolves the following warning:
> memcpy: detected field-spanning write (size 8) of single field "_Generic(info, const struct ip_tunnel_info * : ((const void *)((info) + 1)), struct ip_tunnel_info * : ((void *)((info) + 1)) )" at include/net/ip_tunnels.h:662 (size 0)
Then you can drop this macro and just use: info->options
Looks like you'd need to do it for all the types in struct metadata_dst, but at least you could stop hiding it from the compiler. :)
-Kees
--
Kees Cook
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next] net: Silence false field-spanning write warning in ip_tunnel_info_opts_set() memcpy
2025-01-07 23:28 ` Kees Cook
@ 2025-01-08 6:56 ` Gal Pressman
2025-01-09 9:00 ` Gal Pressman
1 sibling, 0 replies; 6+ messages in thread
From: Gal Pressman @ 2025-01-08 6:56 UTC (permalink / raw)
To: Kees Cook, David S. Miller, Jakub Kicinski
Cc: netdev, Eric Dumazet, Paolo Abeni, David Ahern, Simon Horman,
linux-hardening, Cosmin Ratiu
On 08/01/2025 1:28, Kees Cook wrote:
>
>
> On January 7, 2025 8:55:09 AM PST, Gal Pressman <gal@nvidia.com> wrote:
>> When metadata_dst struct is allocated (using metadata_dst_alloc()), it
>> reserves room for options at the end of the struct.
>>
>> Similar to [1], change the memcpy() to unsafe_memcpy() as it is
>> guaranteed that enough room (md_size bytes) was allocated and the
>> field-spanning write is intentional.
>
> Why not just add an "options" flex array to struct ip_tunnel_info?
>
> E.g.:
>
> struct ip_tunnel_info {
> struct ip_tunnel_key key;
> struct ip_tunnel_encap encap;
> #ifdef CONFIG_DST_CACHE
> struct dst_cache dst_cache;
> #endif
> u8 options_len;
> u8 mode;
> u8 options[] __counted_by(options_len);
> };
>
>>
>> This resolves the following warning:
>> memcpy: detected field-spanning write (size 8) of single field "_Generic(info, const struct ip_tunnel_info * : ((const void *)((info) + 1)), struct ip_tunnel_info * : ((void *)((info) + 1)) )" at include/net/ip_tunnels.h:662 (size 0)
>
> Then you can drop this macro and just use: info->options
>
> Looks like you'd need to do it for all the types in struct metadata_dst, but at least you could stop hiding it from the compiler. :)
>
> -Kees
>
>
Thanks for the review Kees, I'll take a look into that.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next] net: Silence false field-spanning write warning in ip_tunnel_info_opts_set() memcpy
2025-01-07 23:28 ` Kees Cook
2025-01-08 6:56 ` Gal Pressman
@ 2025-01-09 9:00 ` Gal Pressman
2025-01-09 16:52 ` Kees Cook
1 sibling, 1 reply; 6+ messages in thread
From: Gal Pressman @ 2025-01-09 9:00 UTC (permalink / raw)
To: Kees Cook, David S. Miller, Jakub Kicinski
Cc: netdev, Eric Dumazet, Paolo Abeni, David Ahern, Simon Horman,
linux-hardening, Cosmin Ratiu
On 08/01/2025 1:28, Kees Cook wrote:
>> This resolves the following warning:
>> memcpy: detected field-spanning write (size 8) of single field "_Generic(info, const struct ip_tunnel_info * : ((const void *)((info) + 1)), struct ip_tunnel_info * : ((void *)((info) + 1)) )" at include/net/ip_tunnels.h:662 (size 0)
>
> Then you can drop this macro and just use: info->options
>
> Looks like you'd need to do it for all the types in struct metadata_dst, but at least you could stop hiding it from the compiler. :)
Can you please explain the "do it for all the types in struct
metadata_dst" part?
AFAICT, struct ip_tunnel_info is the only one that's extendable, I don't
think others need to be modified.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next] net: Silence false field-spanning write warning in ip_tunnel_info_opts_set() memcpy
2025-01-09 9:00 ` Gal Pressman
@ 2025-01-09 16:52 ` Kees Cook
2025-01-12 6:37 ` Gal Pressman
0 siblings, 1 reply; 6+ messages in thread
From: Kees Cook @ 2025-01-09 16:52 UTC (permalink / raw)
To: Gal Pressman
Cc: David S. Miller, Jakub Kicinski, netdev, Eric Dumazet,
Paolo Abeni, David Ahern, Simon Horman, linux-hardening,
Cosmin Ratiu
On Thu, Jan 09, 2025 at 11:00:24AM +0200, Gal Pressman wrote:
> On 08/01/2025 1:28, Kees Cook wrote:
> >> This resolves the following warning:
> >> memcpy: detected field-spanning write (size 8) of single field "_Generic(info, const struct ip_tunnel_info * : ((const void *)((info) + 1)), struct ip_tunnel_info * : ((void *)((info) + 1)) )" at include/net/ip_tunnels.h:662 (size 0)
> >
> > Then you can drop this macro and just use: info->options
> >
> > Looks like you'd need to do it for all the types in struct metadata_dst, but at least you could stop hiding it from the compiler. :)
>
> Can you please explain the "do it for all the types in struct
> metadata_dst" part?
> AFAICT, struct ip_tunnel_info is the only one that's extendable, I don't
> think others need to be modified.
Ah, sorry. If that's the case, then just ip_tunnel_info is fine. (Is all
of the metadata_dst trailing byte allocation logic just for
ip_tunnel_info?)
-Kees
--
Kees Cook
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next] net: Silence false field-spanning write warning in ip_tunnel_info_opts_set() memcpy
2025-01-09 16:52 ` Kees Cook
@ 2025-01-12 6:37 ` Gal Pressman
0 siblings, 0 replies; 6+ messages in thread
From: Gal Pressman @ 2025-01-12 6:37 UTC (permalink / raw)
To: Kees Cook
Cc: David S. Miller, Jakub Kicinski, netdev, Eric Dumazet,
Paolo Abeni, David Ahern, Simon Horman, linux-hardening,
Cosmin Ratiu
On 09/01/2025 18:52, Kees Cook wrote:
> On Thu, Jan 09, 2025 at 11:00:24AM +0200, Gal Pressman wrote:
>> On 08/01/2025 1:28, Kees Cook wrote:
>>>> This resolves the following warning:
>>>> memcpy: detected field-spanning write (size 8) of single field "_Generic(info, const struct ip_tunnel_info * : ((const void *)((info) + 1)), struct ip_tunnel_info * : ((void *)((info) + 1)) )" at include/net/ip_tunnels.h:662 (size 0)
>>>
>>> Then you can drop this macro and just use: info->options
>>>
>>> Looks like you'd need to do it for all the types in struct metadata_dst, but at least you could stop hiding it from the compiler. :)
>>
>> Can you please explain the "do it for all the types in struct
>> metadata_dst" part?
>> AFAICT, struct ip_tunnel_info is the only one that's extendable, I don't
>> think others need to be modified.
>
> Ah, sorry. If that's the case, then just ip_tunnel_info is fine. (Is all
> of the metadata_dst trailing byte allocation logic just for
> ip_tunnel_info?)
Yes, thanks again!
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-01-12 6:37 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-01-07 16:55 [PATCH net-next] net: Silence false field-spanning write warning in ip_tunnel_info_opts_set() memcpy Gal Pressman
2025-01-07 23:28 ` Kees Cook
2025-01-08 6:56 ` Gal Pressman
2025-01-09 9:00 ` Gal Pressman
2025-01-09 16:52 ` Kees Cook
2025-01-12 6:37 ` Gal Pressman
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).