Re: [PATCH v7 28/28] sfc: support pio mapping based on cxl

[Alejandro Lucero Palau <alucerop@amd.com>]

On 12/12/24 21:22, Simon Horman wrote:
> On Mon, Dec 09, 2024 at 06:54:29PM +0000, alejandro.lucero-palau@amd.com wrote:
>> From: Alejandro Lucero <alucerop@amd.com>
>>
>> With a device supporting CXL and successfully initialised, use the cxl
>> region to map the memory range and use this mapping for PIO buffers.
>>
>> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
>> ---
>>   drivers/net/ethernet/sfc/ef10.c       | 48 +++++++++++++++++++++++----
>>   drivers/net/ethernet/sfc/efx_cxl.c    | 19 ++++++++++-
>>   drivers/net/ethernet/sfc/net_driver.h |  2 ++
>>   drivers/net/ethernet/sfc/nic.h        |  3 ++
>>   4 files changed, 65 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/sfc/ef10.c b/drivers/net/ethernet/sfc/ef10.c
>> index 452009ed7a43..4587ca884c03 100644
>> --- a/drivers/net/ethernet/sfc/ef10.c
>> +++ b/drivers/net/ethernet/sfc/ef10.c
>> @@ -24,6 +24,7 @@
>>   #include <linux/wait.h>
>>   #include <linux/workqueue.h>
>>   #include <net/udp_tunnel.h>
>> +#include "efx_cxl.h"
>>   
>>   /* Hardware control for EF10 architecture including 'Huntington'. */
>>   
>> @@ -177,6 +178,12 @@ static int efx_ef10_init_datapath_caps(struct efx_nic *efx)
>>   			  efx->num_mac_stats);
>>   	}
> Hi Alejandro,
>
> Earlier in efx_ef10_init_datapath_caps, outbuf is declared using:
>
> 	MCDI_DECLARE_BUF(outbuf, MC_CMD_GET_CAPABILITIES_V4_OUT_LEN);
>
> This will result in the following declaration:
>
> 	efx_dword_t _name[DIV_ROUND_UP(MC_CMD_GET_CAPABILITIES_V4_OUT_LEN, 4)]
>
> Where MC_CMD_GET_CAPABILITIES_V4_OUT_LEN is defined as 78.
> So outbuf will be an array with DIV_ROUND_UP(78, 4) == 20 elements.
>
>>   
>> +	if (outlen < MC_CMD_GET_CAPABILITIES_V7_OUT_LEN)
>> +		nic_data->datapath_caps3 = 0;
>> +	else
>> +		nic_data->datapath_caps3 = MCDI_DWORD(outbuf,
>> +						      GET_CAPABILITIES_V7_OUT_FLAGS3);
>> +
>>   	return 0;
>>   }
>>   
> MC_CMD_GET_CAPABILITIES_V7_OUT_FLAGS3_OFST is defined as 148.
> And the above will result in an access to element 148 / 4 == 37 of
> outbuf. A buffer overflow.


Hi Simon,


This is, obviously, quite serious, although being the first and only 
flag in that MCDI extension explains why has gone hidden and harmless 
(as it is a read).


I'll definitely fix it.


Thanks!


>
> Flagged by gcc-14 W=1 allmodconfig builds as:
>
> In file included from drivers/net/ethernet/sfc/net_driver.h:33,
>                   from drivers/net/ethernet/sfc/ef10.c:7:
> drivers/net/ethernet/sfc/ef10.c: In function 'efx_ef10_init_datapath_caps':
> drivers/net/ethernet/sfc/bitfield.h:167:35: warning: array subscript 37 is above array bounds of 'efx_dword_t[20]' {aka 'union efx_dword[20]'} [-Warray-bounds=]
>    167 |         (EFX_EXTRACT32((dword).u32[0], 0, 31, low, high) &      \
> drivers/net/ethernet/sfc/bitfield.h:129:11: note: in definition of macro 'EFX_EXTRACT_NATIVE'
>    129 |          (native_element) << ((min) - (low)))
>        |           ^~~~~~~~~~~~~~
> ./include/linux/byteorder/generic.h:89:21: note: in expansion of macro '__le32_to_cpu'
>     89 | #define le32_to_cpu __le32_to_cpu
>        |                     ^~~~~~~~~~~~~
> drivers/net/ethernet/sfc/bitfield.h:167:10: note: in expansion of macro 'EFX_EXTRACT32'
>    167 |         (EFX_EXTRACT32((dword).u32[0], 0, 31, low, high) &      \
>        |          ^~~~~~~~~~~~~
> drivers/net/ethernet/sfc/bitfield.h:187:9: note: in expansion of macro 'EFX_EXTRACT_DWORD'
>    187 |         EFX_EXTRACT_DWORD(dword, EFX_LOW_BIT(field),            \
>        |         ^~~~~~~~~~~~~~~~~
> drivers/net/ethernet/sfc/mcdi.h:257:9: note: in expansion of macro 'EFX_DWORD_FIELD'
>    257 |         EFX_DWORD_FIELD(*_MCDI_DWORD(_buf, _field), EFX_DWORD_0)
>        |         ^~~~~~~~~~~~~~~
> drivers/net/ethernet/sfc/ef10.c:184:44: note: in expansion of macro 'MCDI_DWORD'
>    184 |                 nic_data->datapath_caps3 = MCDI_DWORD(outbuf,
>        |                                            ^~~~~~~~~~
> In file included from drivers/net/ethernet/sfc/ef10.c:12:
> drivers/net/ethernet/sfc/ef10.c:110:26: note: while referencing 'outbuf'
>    110 |         MCDI_DECLARE_BUF(outbuf, MC_CMD_GET_CAPABILITIES_V4_OUT_LEN);
>        |                          ^~~~~~
> drivers/net/ethernet/sfc/mcdi.h:187:21: note: in definition of macro '_MCDI_DECLARE_BUF'
>    187 |         efx_dword_t _name[DIV_ROUND_UP(_len, 4)]
>        |                     ^~~~~
> drivers/net/ethernet/sfc/ef10.c:110:9: note: in expansion of macro 'MCDI_DECLARE_BUF'
>    110 |         MCDI_DECLARE_BUF(outbuf, MC_CMD_GET_CAPABILITIES_V4_OUT_LEN);
>        |         ^~~~~~~~~~~~~~~~
>
> ...
>