| Message ID | 20250107165509.3008505-1-gal@nvidia.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [net-next] net: Silence false field-spanning write warning in ip_tunnel_info_opts_set() memcpy | expand |
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
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.
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.
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
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!
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); }
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(-)