Re: [PATCH net] mlx4: supress fortify for inlined xmit

[Tariq Toukan]

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@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...
>
> 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@xxxxxxxxxx>

Kees posted a contained alternative solution.
Let's go with that one. Thanks.