lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <e8551085-30b8-dce3-28b7-233b47a7ddc1@gmail.com> Date: Mon, 20 Feb 2023 10:30:15 +0200 From: Tariq Toukan <ttoukan.linux@...il.com> To: Kees Cook <kees@...nel.org>, Josef Oskera <joskera@...hat.com>, netdev@...r.kernel.org Cc: Tariq Toukan <tariqt@...dia.com>, "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, Kees Cook <keescook@...omium.org>, linux-rdma@...r.kernel.org, linux-kernel@...r.kernel.org Subject: Re: [PATCH net] mlx4: supress fortify for inlined xmit On 19/02/2023 11:16, Tariq Toukan wrote: > > > On 18/02/2023 18:26, Kees Cook wrote: >> On February 17, 2023 1:45:41 AM PST, Josef Oskera <joskera@...hat.com> >> wrote: >>> This call "skb_copy_from_linear_data(skb, inl + 1, spc)" triggers >>> FORTIFY memcpy() >>> warning on ppc64 platform. >>> >>> In function ‘fortify_memcpy_chk’, >>> inlined from ‘skb_copy_from_linear_data’ at >>> ./include/linux/skbuff.h:4029:2, >>> inlined from ‘build_inline_wqe’ at >>> drivers/net/ethernet/mellanox/mlx4/en_tx.c:722:4, >>> inlined from ‘mlx4_en_xmit’ at >>> drivers/net/ethernet/mellanox/mlx4/en_tx.c:1066:3: >>> ./include/linux/fortify-string.h:513:25: error: call to >>> ‘__write_overflow_field’ declared with attribute warning: detected >>> write beyond size of field (1st parameter); maybe use struct_group()? >>> [-Werror=attribute-warning] >>> 513 | __write_overflow_field(p_size_field, >>> size); >>> | >>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >>> >>> Same behaviour on x86 you can get if you use "__always_inline" >>> instead of >>> "inline" for skb_copy_from_linear_data() in skbuff.h >>> >>> The call here copies data into inlined tx destricptor, which has 104 >>> bytes >>> (MAX_INLINE) space for data payload. In this case "spc" is known in >>> compile-time >>> but the destination is used with hidden knowledge (real structure of >>> destination >>> is different from that the compiler can see). That cause the fortify >>> warning >>> because compiler can check bounds, but the real bounds are different. >>> "spc" can't be bigger than 64 bytes (MLX4_INLINE_ALIGN), so the data >>> can always >>> fit into inlined tx descriptor. >>> The fact that "inl" points into inlined tx descriptor is determined >>> earlier >>> in mlx4_en_xmit(). >>> >>> Fixes: f68f2ff91512c1 fortify: Detect struct member overflows in >>> memcpy() at compile-time >>> Signed-off-by: Josef Oskera <joskera@...hat.com> >>> --- >>> drivers/net/ethernet/mellanox/mlx4/en_tx.c | 11 ++++++++++- >>> 1 file changed, 10 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_tx.c >>> b/drivers/net/ethernet/mellanox/mlx4/en_tx.c >>> index c5758637b7bed6..f30ca9fe90e5b4 100644 >>> --- a/drivers/net/ethernet/mellanox/mlx4/en_tx.c >>> +++ b/drivers/net/ethernet/mellanox/mlx4/en_tx.c >>> @@ -719,7 +719,16 @@ static void build_inline_wqe(struct >>> mlx4_en_tx_desc *tx_desc, >>> inl = (void *) (inl + 1) + spc; >>> memcpy(((void *)(inl + 1)), fragptr, skb->len - spc); >> >> Using "unsafe" isn't the right solution here. What needs fixing is the >> "inl + 1" pattern which lacks any sense from the compilet's >> perspective. The struct of inl needs to end with a flex array, and it >> should be used for all the accesses. i.e. replace all the "inl + 1" >> instances with "inl->data". This makes it more readable for humans >> too. :) >> >> I can send a patch... >> > > Although expanding the mlx4_wqe_inline_seg struct with a flex array > sounds valid, I wouldn't go that way as it requires a larger change, > touching common and RDMA code as well, for a driver in it's end-of-life > stage. > > We already have such unsafe_memcpy usage in mlx5 driver, so I can accept > it here as well. > > Let's keep the change as contained as possible. > > Reviewed-by: Tariq Toukan <tariqt@...dia.com> Kees posted a contained alternative solution. Let's go with that one. Thanks.
Powered by blists - more mailing lists