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

[Tariq Toukan <ttoukan.linux@gmail.com>]

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@redhat.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@redhat.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@nvidia.com>

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