On February 17, 2023 1:45:41 AM PST, Josef Oskera <joskera@xxxxxxxxxx> 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@xxxxxxxxxx>
>---
> 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...
-Kees
> } else {
>- skb_copy_from_linear_data(skb, inl + 1, spc);
>+ unsafe_memcpy(inl + 1, skb->data, spc,
>+ /* This copies data into inlined tx descriptor, which has
>+ * 104 bytes (MAX_INLINE) space for data.
>+ * Real structure of destination is in this case hidden for
>+ * the compiler
>+ * "spc" is compile-time known variable and can't be bigger
>+ * than 64 (MLX4_INLINE_ALIGN).
>+ * Bounds and other conditions are checked in current
>+ * function and earlier in mlx4_en_xmit()
>+ */);
> inl = (void *) (inl + 1) + spc;
> skb_copy_from_linear_data_offset(skb, spc, inl + 1,
> hlen - spc);
--
Kees Cook
