Re: [Intel-wired-lan] [PATCH net] i40e: limit the DDP profile count returned by the firmware

[Linkui Xiao <xiaolinkui@126.com>]

On 2026/9/16 14:03, Loktionov, Aleksandr wrote:
> 
> 
>> -----Original Message-----
>> From: Linkui Xiao <xiaolinkui@126.com>
>> Sent: Tuesday, September 15, 2026 2:04 PM
>> To: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Kitszel,
>> Przemyslaw <przemyslaw.kitszel@intel.com>; andrew+netdev@lunn.ch;
>> davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
>> pabeni@redhat.com
>> Cc: intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org; linux-
>> kernel@vger.kernel.org; Linkui Xiao <xiaolinkui@kylinos.cn>
>> Subject: [Intel-wired-lan] [PATCH net] i40e: limit the DDP profile
>> count returned by the firmware
>>
>> From: Linkui Xiao <xiaolinkui@kylinos.cn>
>>
>> i40e_aq_get_ddp_list() writes into a I40E_PROFILE_LIST_SIZE buffer,
>> which is sized for I40E_MAX_PROFILE_NUM (16) i40e_profile_info entries
>> plus the 4 byte p_count header. i40e_ddp_does_profile_exist() and
>> i40e_ddp_does_profile_overlap() then loop over profile_list->p_count
>> without bounding it, so a firmware reporting more than 16 profiles
>> makes both helpers walk past the end of the on-stack buff[] and
>> compare against whatever happens to follow it on the stack.
>>
>> Clamp the count to the number of entries the buffer can actually hold
>> and make the loop counter unsigned to match the field type.
>>
>> Fixes: cdc594e00370 ("i40e: Implement DDP support in i40e driver")
>> Signed-off-by: Linkui Xiao <xiaolinkui@kylinos.cn>
>> ---
>>   drivers/net/ethernet/intel/i40e/i40e_ddp.c | 18 ++++++++++++++----
>>   1 file changed, 14 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/i40e/i40e_ddp.c
>> b/drivers/net/ethernet/intel/i40e/i40e_ddp.c
>> index daa9f2c42f70..1d6d8b3835a3 100644
>> --- a/drivers/net/ethernet/intel/i40e/i40e_ddp.c
>> +++ b/drivers/net/ethernet/intel/i40e/i40e_ddp.c
>> @@ -55,7 +55,7 @@ static int i40e_ddp_does_profile_exist(struct
>> i40e_hw *hw,
>>   	struct i40e_ddp_profile_list *profile_list;
>>   	u8 buff[I40E_PROFILE_LIST_SIZE];
>>   	int status;
>> -	int i;
>> +	u32 i, p_count;
> RCT please
> 
>>
>>   	status = i40e_aq_get_ddp_list(hw, buff, I40E_PROFILE_LIST_SIZE,
>> 0,
>>   				      NULL);
>> @@ -63,7 +63,12 @@ static int i40e_ddp_does_profile_exist(struct
>> i40e_hw *hw,
>>   		return -1;
>>
>>   	profile_list = (struct i40e_ddp_profile_list *)buff;
>> -	for (i = 0; i < profile_list->p_count; i++) {
>> +	/* Never walk past the end of buff[], the profile count
>> reported by
>> +	 * the firmware is not guaranteed to fit into the buffer we
>> gave it.
>> +	 */
>> +	p_count = min_t(u32, profile_list->p_count,
>> I40E_MAX_PROFILE_NUM);
>> +
>> +	for (i = 0; i < p_count; i++) {
>>   		if (i40e_ddp_profiles_eq(pinfo, &profile_list-
>>> p_info[i]))
>>   			return 1;
>>   	}
>> @@ -110,7 +115,7 @@ static int i40e_ddp_does_profile_overlap(struct
>> i40e_hw *hw,
>>   	struct i40e_ddp_profile_list *profile_list;
>>   	u8 buff[I40E_PROFILE_LIST_SIZE];
>>   	int status;
>> -	int i;
>> +	u32 i, p_count;
> RCT please
> 
> Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Hi,

Thanks for the review and for the Reviewed-by tag.

Just to make sure I understand the "RCT please" comments correctly: are 
they referring to the Reverse Christmas Tree (RCT) variable declaration 
ordering convention, i.e. the local variables should be declared in 
decreasing order of line length? In both functions I currently have:
         struct i40e_ddp_profile_list *profile_list;
         u8 buff[I40E_PROFILE_LIST_SIZE];
         int status;
         u32 i, p_count;

If I understand RCT correctly, the expected order would be something like:
         struct i40e_ddp_profile_list *profile_list;
         u8 buff[I40E_PROFILE_LIST_SIZE];
         u32 i, p_count;
         int status;

Could you please confirm whether that is what is being asked for, and 
whether there are any other RCT-related issues in this patch?
Also, should I send a v2 with the RCT ordering fixed, or would you 
prefer to keep the current version as-is since the change is purely 
stylistic? I am happy to send a v2 if that is preferred.

Thanks,
Linkui
> 
>>
>>   	status = i40e_aq_get_ddp_list(hw, buff, I40E_PROFILE_LIST_SIZE,
>> 0,
>>   				      NULL);
>> @@ -118,7 +123,12 @@ static int i40e_ddp_does_profile_overlap(struct
>> i40e_hw *hw,
>>   		return -EIO;
>>
>>   	profile_list = (struct i40e_ddp_profile_list *)buff;
>> -	for (i = 0; i < profile_list->p_count; i++) {
>> +	/* Never walk past the end of buff[], the profile count
>> reported by
>> +	 * the firmware is not guaranteed to fit into the buffer we
>> gave it.
>> +	 */
>> +	p_count = min_t(u32, profile_list->p_count,
>> I40E_MAX_PROFILE_NUM);
>> +
>> +	for (i = 0; i < p_count; i++) {
>>   		if (i40e_ddp_profiles_overlap(pinfo,
>>   					      &profile_list->p_info[i]))
>>   			return 1;
>> --
>> 2.25.1
>