Re: [PATCH] net/mlx4_en: Introduce flexible array to silence overflow warning
From: Josef Oskera
Date: Sun Feb 19 2023 - 21:10:58 EST
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 {
>