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

From: Tariq Toukan <ttoukan.linux@gmail.com>
To: Kees Cook <kees@kernel.org>, Josef Oskera <joskera@redhat.com>,
	netdev@vger.kernel.org
Cc: Tariq Toukan <tariqt@nvidia.com>,
	"David S. Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>,
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
	Kees Cook <keescook@chromium.org>,
	linux-rdma@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH net] mlx4: supress fortify for inlined xmit
Date: Sun, 19 Feb 2023 11:16:54 +0200	[thread overview]
Message-ID: <febbc959-4cc7-a810-8000-db37f2de53cc@gmail.com> (raw)
In-Reply-To: <2E9A091B-ABA3-4B99-965A-EA893F402F25@kernel.org>

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