[PATCH net] mlx4: supress fortify for inlined xmit

[PATCH net] mlx4: supress fortify for inlined xmit

[Josef Oskera]

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);
 		} 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);
-- 
2.39.0

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

[Kees Cook]

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

-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

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

[Tariq Toukan]

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
> 
>> 		} 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);
> 
> 

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