netdev - Re: [PATCH] i40e: fix MMIO write access to an invalid page in i40e_clear

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <aea2c7f8-995b-45bc-b2fb-d45e3fbe65b1@gmail.com>
Date: Fri, 7 Mar 2025 13:10:25 +0900
From: Kyungwook Boo <bookyungwook@...il.com>
To: "Loktionov, Aleksandr" <aleksandr.loktionov@...el.com>,
 "Kitszel, Przemyslaw" <przemyslaw.kitszel@...el.com>,
 "Nguyen, Anthony L" <anthony.l.nguyen@...el.com>
Cc: "intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>,
 "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH] i40e: fix MMIO write access to an invalid page in
 i40e_clear_hw

On 25. 3. 6. 20:13, Loktionov, Aleksandr wrote:
> 
> 
>> -----Original Message-----
>> From: Kyungwook Boo <bookyungwook@...il.com>
>> Sent: Thursday, March 6, 2025 6:26 AM
>> To: Loktionov, Aleksandr <aleksandr.loktionov@...el.com>; Kitszel,
>> Przemyslaw <przemyslaw.kitszel@...el.com>; Nguyen, Anthony L
>> <anthony.l.nguyen@...el.com>
>> Cc: intel-wired-lan@...ts.osuosl.org; netdev@...r.kernel.org
>> Subject: [PATCH] i40e: fix MMIO write access to an invalid page in
>> i40e_clear_hw
> Please follow the rules, add v2 to the patch

Hi, Loktionov,

Thank you for reviewing the patch.

Following your guidance, I will update the patch with the correct format and
also include v2.

>>
>> In i40e_clear_hw(), when the device sends a specific input(e.g., 0), an integer
>> underflow in the num_{pf,vf}_int variables can occur, leading to MMIO write
>> access to an invalid page.
>>
>> To fix this, we change the type of the unsigned integer variables
>> num_{pf,vf}_int to signed integers. Additionally, in the for-loop where the
>> integer underflow occurs, we also change the type of the loop variable i to a
>> signed integer.
> Please do follow the linux kernel 

If you are referring to the tone of the patch description, I will rewrite it in
the imperative mood.

>>
>> Signed-off-by: Kyungwook Boo <bookyungwook@...il.com>
>> Signed-off-by: Loktionov, Aleksandr <aleksandr.loktionov@...el.com>
>> Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@...el.com>
>> Link: https://lore.kernel.org/lkml/ffc91764-1142-4ba2-91b6-
>> 8c773f6f7095@...il.com/T/
>> ---
> Please up here versions history.

I have noted your request and will add the version history in the next update.

>>  drivers/net/ethernet/intel/i40e/i40e_common.c | 10 +++++-----
>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/i40e/i40e_common.c
>> b/drivers/net/ethernet/intel/i40e/i40e_common.c
>> index 370b4bddee44..9a73cb94dc5e 100644
>> --- a/drivers/net/ethernet/intel/i40e/i40e_common.c
>> +++ b/drivers/net/ethernet/intel/i40e/i40e_common.c
>> @@ -817,8 +817,8 @@ int i40e_pf_reset(struct i40e_hw *hw)  void
>> i40e_clear_hw(struct i40e_hw *hw)  {
>>  	u32 num_queues, base_queue;
>> -	u32 num_pf_int;
>> -	u32 num_vf_int;
>> +	s32 num_pf_int;
>> +	s32 num_vf_int;
>>  	u32 num_vfs;
>>  	u32 i, j;
> What about this vars? Are they used?

i, j are both used.
I think the relevant line to be considered is as follows:

if (val & I40E_PF_VT_PFALLOC_VALID_MASK && j >= i)
		num_vfs = (j - i) + 1;

After this, j is not used and 
i is used as index of several loops.

My current plan was to change only i to s32 since it is related to the bug.
However, i is also used outside the loop, as in the code above.

Should I proceed with the original plan, or would it be better to separate the
loop variable for clarity? I would appreciate your opinion on this.

>>  	u32 val;
>> @@ -848,18 +848,18 @@ void i40e_clear_hw(struct i40e_hw *hw)
>>  	/* stop all the interrupts */
>>  	wr32(hw, I40E_PFINT_ICR0_ENA, 0);
>>  	val = 0x3 << I40E_PFINT_DYN_CTLN_ITR_INDX_SHIFT;
>> -	for (i = 0; i < num_pf_int - 2; i++)
>> +	for (s32 i = 0; i < num_pf_int - 2; i++)
>>  		wr32(hw, I40E_PFINT_DYN_CTLN(i), val);
>>
>>  	/* Set the FIRSTQ_INDX field to 0x7FF in PFINT_LNKLSTx */
>>  	val = eol << I40E_PFINT_LNKLST0_FIRSTQ_INDX_SHIFT;
>>  	wr32(hw, I40E_PFINT_LNKLST0, val);
>> -	for (i = 0; i < num_pf_int - 2; i++)
>> +	for (s32 i = 0; i < num_pf_int - 2; i++)
>>  		wr32(hw, I40E_PFINT_LNKLSTN(i), val);
>>  	val = eol << I40E_VPINT_LNKLST0_FIRSTQ_INDX_SHIFT;
>>  	for (i = 0; i < num_vfs; i++)
>>  		wr32(hw, I40E_VPINT_LNKLST0(i), val);
>> -	for (i = 0; i < num_vf_int - 2; i++)
>> +	for (s32 i = 0; i < num_vf_int - 2; i++)
>>  		wr32(hw, I40E_VPINT_LNKLSTN(i), val);
>>
>>  	/* warn the HW of the coming Tx disables */
>> --
>> 2.25.1

Best,
Kyungwook Boo