I've tried Kees's patch and it works for me. I am able to compile mlx4 without the fortify warning. ne 19. 2. 2023 v 10:43 odesílatel Tariq Toukan <ttoukan.linux@xxxxxxxxx> napsal: > > > > On 18/02/2023 20:38, Kees Cook wrote: > > The call "skb_copy_from_linear_data(skb, inl + 1, spc)" triggers a 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(). > > > > Avoid confusing the compiler with "inl + 1" constructions to get to past > > the inl header by introducing a flexible array "data" to the struct so > > that the compiler can see that we are not dealing with an array of inl > > structs, but rather, arbitrary data following the structure. There are > > no changes to the structure layout reported by pahole, and the resulting > > machine code is actually smaller. > > > > Reported-by: Josef Oskera <joskera@xxxxxxxxxx> > > Link: https://lore.kernel.org/lkml/20230217094541.2362873-1-joskera@xxxxxxxxxx > > Fixes: f68f2ff91512 ("fortify: Detect struct member overflows in memcpy() at compile-time") > > Cc: Tariq Toukan <tariqt@xxxxxxxxxx> > > Cc: "David S. Miller" <davem@xxxxxxxxxxxxx> > > Cc: Eric Dumazet <edumazet@xxxxxxxxxx> > > Cc: Jakub Kicinski <kuba@xxxxxxxxxx> > > Cc: Paolo Abeni <pabeni@xxxxxxxxxx> > > Cc: Yishai Hadas <yishaih@xxxxxxxxxx> > > Cc: netdev@xxxxxxxxxxxxxxx > > Cc: linux-rdma@xxxxxxxxxxxxxxx > > Signed-off-by: Kees Cook <keescook@xxxxxxxxxxxx> > > --- > > drivers/net/ethernet/mellanox/mlx4/en_tx.c | 22 +++++++++++----------- > > include/linux/mlx4/qp.h | 1 + > > 2 files changed, 12 insertions(+), 11 deletions(-) > > > > Just saw your patch now, after commenting on the other thread. :) > > So you choose not to fix similar usages in RDMA driver > drivers/infiniband/hw/mlx4/qp.c, like: > > 3204 spc = MLX4_INLINE_ALIGN - > 3205 ((unsigned long) (inl + 1) & (MLX4_INLINE_ALIGN - 1)); > 3206 if (header_size <= spc) { > 3207 inl->byte_count = cpu_to_be32(1 << 31 | header_size); > 3208 memcpy(inl + 1, sqp->header_buf, header_size); > 3209 i = 1; > 3210 } else { > 3211 inl->byte_count = cpu_to_be32(1 << 31 | spc); > 3212 memcpy(inl + 1, sqp->header_buf, spc); > 3213 > 3214 inl = (void *) (inl + 1) + spc; > 3215 memcpy(inl + 1, sqp->header_buf + spc, header_size > - spc); > > This keeps the patch minimal indeed. > > Did you repro the issue and test this solution? > Maybe Josef can also verify it works for him? > > > diff --git a/drivers/net/ethernet/mellanox/mlx4/en_tx.c b/drivers/net/ethernet/mellanox/mlx4/en_tx.c > > index c5758637b7be..2f79378fbf6e 100644 > > --- a/drivers/net/ethernet/mellanox/mlx4/en_tx.c > > +++ b/drivers/net/ethernet/mellanox/mlx4/en_tx.c > > @@ -699,32 +699,32 @@ static void build_inline_wqe(struct mlx4_en_tx_desc *tx_desc, > > inl->byte_count = cpu_to_be32(1 << 31 | skb->len); > > } else { > > inl->byte_count = cpu_to_be32(1 << 31 | MIN_PKT_LEN); > > - memset(((void *)(inl + 1)) + skb->len, 0, > > + memset(inl->data + skb->len, 0, > > MIN_PKT_LEN - skb->len); > > } > > - skb_copy_from_linear_data(skb, inl + 1, hlen); > > + skb_copy_from_linear_data(skb, inl->data, hlen); > > if (shinfo->nr_frags) > > - memcpy(((void *)(inl + 1)) + hlen, fragptr, > > + memcpy(inl->data + hlen, fragptr, > > skb_frag_size(&shinfo->frags[0])); > > > > } else { > > inl->byte_count = cpu_to_be32(1 << 31 | spc); > > if (hlen <= spc) { > > - skb_copy_from_linear_data(skb, inl + 1, hlen); > > + skb_copy_from_linear_data(skb, inl->data, hlen); > > if (hlen < spc) { > > - memcpy(((void *)(inl + 1)) + hlen, > > + memcpy(inl->data + hlen, > > fragptr, spc - hlen); > > fragptr += spc - hlen; > > } > > - inl = (void *) (inl + 1) + spc; > > - memcpy(((void *)(inl + 1)), fragptr, skb->len - spc); > > + inl = (void *)inl->data + spc; > > + memcpy(inl->data, fragptr, skb->len - spc); > > } else { > > - skb_copy_from_linear_data(skb, inl + 1, spc); > > - inl = (void *) (inl + 1) + spc; > > - skb_copy_from_linear_data_offset(skb, spc, inl + 1, > > + skb_copy_from_linear_data(skb, inl->data, spc); > > + inl = (void *)inl->data + spc; > > No need now for all these (void *) castings. > > > + skb_copy_from_linear_data_offset(skb, spc, inl->data, > > hlen - spc); > > if (shinfo->nr_frags) > > - memcpy(((void *)(inl + 1)) + hlen - spc, > > + memcpy(inl->data + hlen - spc, > > fragptr, > > skb_frag_size(&shinfo->frags[0])); > > } > > diff --git a/include/linux/mlx4/qp.h b/include/linux/mlx4/qp.h > > index c78b90f2e9a1..b9a7b1319f5d 100644 > > --- a/include/linux/mlx4/qp.h > > +++ b/include/linux/mlx4/qp.h > > @@ -446,6 +446,7 @@ enum { > > > > struct mlx4_wqe_inline_seg { > > __be32 byte_count; > > + __u8 data[]; > > }; > > > > enum mlx4_update_qp_attr { >
