MMIO write access to an invalid page in i40e_clear_hw()

MMIO write access to an invalid page in i40e_clear_hw()

[Kyungwook Boo]

Hello,

It seems that there are invalid page MMIO write access in i40e_clear_hw()
due to an integer underflow from num_pf_int(also num_vf_int seems possible).

The following is a sample code in i40e_clear_hw():

val = rd32(hw, I40E_GLPCI_CNF2); // (1)
num_pf_int = FIELD_GET(I40E_GLPCI_CNF2_MSI_X_PF_N_MASK, val); // (2)
num_vf_int = FIELD_GET(I40E_GLPCI_CNF2_MSI_X_VF_N_MASK, val);
...
for (i = 0; i < num_pf_int - 2; i++)      // (3)
	wr32(hw, I40E_PFINT_DYN_CTLN(i), val);  // (4)
...
for (i = 0; i < num_pf_int - 2; i++)			// (5)
	wr32(hw, I40E_PFINT_LNKLSTN(i), val);
...
for (i = 0; i < num_vf_int - 2; i++)			// (6)
	wr32(hw, I40E_VPINT_LNKLSTN(i), val);

An example scenario for num_pf_int:
(1) val = 0 (if MMIO read value was 0)
(2) num_pf_int = 0 (also zero after bit field extraction from val)
(3) An integer underflow occurs (num_pf_int - 2 == 0xfffffffe)
(4) Out-of-bounds MMIO write access if access address exceeds the expected
range.

From above example scenario, the maximum access offset value can be around
0x4000347f8(=172G) which seems like this underflow is not intended(also there
are masking operations like (2) for num_pf_int), so I report this issue.

I think similar issue also could happen at (5) and (6).

The following is the patch method I propose:

diff --git a/drivers/net/ethernet/intel/i40e/i40e_common.c b/drivers/net/ethernet/intel/i40e/i40e_common.c
index 370b4bddee44..97ef79be39b3 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_common.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_common.c
@@ -848,19 +848,25 @@ 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++)
-		wr32(hw, I40E_PFINT_DYN_CTLN(i), val);
+	if (num_pf_int > 1) {
+		for (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++)
-		wr32(hw, I40E_PFINT_LNKLSTN(i), val);
+	if (num_pf_int > 1) {
+		for (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++)
-		wr32(hw, I40E_VPINT_LNKLSTN(i), val);
+	if (num_vf_int > 1) {
+		for (i = 0; i < num_vf_int - 2; i++)
+			wr32(hw, I40E_VPINT_LNKLSTN(i), val);
+	}
 
 	/* warn the HW of the coming Tx disables */
 	for (i = 0; i < num_queues; i++) {


Could you check this?

Best regards,
Kyungwook Boo

RE: [Intel-wired-lan] MMIO write access to an invalid page in i40e_clear_hw()

[Loktionov, Aleksandr]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Kyungwook Boo
> Sent: Monday, March 3, 2025 11:20 AM
> To: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Kitszel, Przemyslaw
> <przemyslaw.kitszel@intel.com>
> Cc: intel-wired-lan@lists.osuosl.org; linux-kernel@vger.kernel.org
> Subject: [Intel-wired-lan] MMIO write access to an invalid page in
> i40e_clear_hw()
> 
Please start commit title with 'fix' to explicitly tell what your patch do i.e. :
Ice: fix MMIO write access to an invalid page in  i40e_clear_hw


> Hello,
> 
> It seems that there are invalid page MMIO write access in i40e_clear_hw() due
> to an integer underflow from num_pf_int(also num_vf_int seems possible).
> 
> The following is a sample code in i40e_clear_hw():
> 
> val = rd32(hw, I40E_GLPCI_CNF2); // (1)
> num_pf_int = FIELD_GET(I40E_GLPCI_CNF2_MSI_X_PF_N_MASK, val); // (2)
> num_vf_int = FIELD_GET(I40E_GLPCI_CNF2_MSI_X_VF_N_MASK, val); ...
> for (i = 0; i < num_pf_int - 2; i++)      // (3)
> 	wr32(hw, I40E_PFINT_DYN_CTLN(i), val);  // (4) ...
> for (i = 0; i < num_pf_int - 2; i++)			// (5)
> 	wr32(hw, I40E_PFINT_LNKLSTN(i), val);
> ...
> for (i = 0; i < num_vf_int - 2; i++)			// (6)
> 	wr32(hw, I40E_VPINT_LNKLSTN(i), val);
> 
> An example scenario for num_pf_int:
> (1) val = 0 (if MMIO read value was 0)
> (2) num_pf_int = 0 (also zero after bit field extraction from val)
> (3) An integer underflow occurs (num_pf_int - 2 == 0xfffffffe)
> (4) Out-of-bounds MMIO write access if access address exceeds the expected
> range.
> 
> From above example scenario, the maximum access offset value can be
> around
> 0x4000347f8(=172G) which seems like this underflow is not intended(also
> there are masking operations like (2) for num_pf_int), so I report this issue.
> 
> I think similar issue also could happen at (5) and (6).
> 
> The following is the patch method I propose:
> 
Please add Fixes: tag  https://www.kernel.org/doc/html/latest/process/submitting-patches.html


> diff --git a/drivers/net/ethernet/intel/i40e/i40e_common.c
> b/drivers/net/ethernet/intel/i40e/i40e_common.c
> index 370b4bddee44..97ef79be39b3 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_common.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_common.c
> @@ -848,19 +848,25 @@ 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++)
> -		wr32(hw, I40E_PFINT_DYN_CTLN(i), val);
> +	if (num_pf_int > 1) {
> +		for (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++)
> -		wr32(hw, I40E_PFINT_LNKLSTN(i), val);
> +	if (num_pf_int > 1) {
> +		for (i = 0; i < num_pf_int - 2; i++)
> +			wr32(hw, I40E_PFINT_LNKLSTN(i), val);
> +	}
Can you consider moving this if upper and use it once instead of duplicating the code?
I think it can help to maintain the code. What do you think?

>  	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++)
> -		wr32(hw, I40E_VPINT_LNKLSTN(i), val);
> +	if (num_vf_int > 1) {
> +		for (i = 0; i < num_vf_int - 2; i++)
> +			wr32(hw, I40E_VPINT_LNKLSTN(i), val);
> +	}
> 
>  	/* warn the HW of the coming Tx disables */
>  	for (i = 0; i < num_queues; i++) {
> 
> 
> Could you check this?
> 
> Best regards,
> Kyungwook Boo

Re: [Intel-wired-lan] MMIO write access to an invalid page in i40e_clear_hw()

[Kyungwook Boo]

On 25. 3. 5. 19:13, Loktionov, Aleksandr wrote:
> 
> 
>> -----Original Message-----
>> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
>> Kyungwook Boo
>> Sent: Monday, March 3, 2025 11:20 AM
>> To: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Kitszel, Przemyslaw
>> <przemyslaw.kitszel@intel.com>
>> Cc: intel-wired-lan@lists.osuosl.org; linux-kernel@vger.kernel.org
>> Subject: [Intel-wired-lan] MMIO write access to an invalid page in
>> i40e_clear_hw()
>>
> Please start commit title with 'fix' to explicitly tell what your patch do i.e. :
> Ice: fix MMIO write access to an invalid page in  i40e_clear_hw
> 
> Please add Fixes: tag  https://www.kernel.org/doc/html/latest/process/submitting-patches.html


Thanks for the guidance. I'll follow your advice and send the patch accordingly.
I'll also read the linked documentation and will try the patch is properly
formatted.

Re: MMIO write access to an invalid page in i40e_clear_hw()

[Przemek Kitszel]

On 3/3/25 11:19, Kyungwook Boo wrote:
> Hello,
> 
> It seems that there are invalid page MMIO write access in i40e_clear_hw()

Hi,

is this something that actually occurred, or just a theoretical bug?
(depending on that we will apply it to different tree)

please send a proper patch anyway, as it looks legit to don't go bananas
when HW gives you 0

(and CC netdev instead of generic kernel ML, perhaps that's the reason
this mail was tagged as spam for me)

> due to an integer underflow from num_pf_int(also num_vf_int seems possible).
> 
> The following is a sample code in i40e_clear_hw():
> 
> val = rd32(hw, I40E_GLPCI_CNF2); // (1)
> num_pf_int = FIELD_GET(I40E_GLPCI_CNF2_MSI_X_PF_N_MASK, val); // (2)
> num_vf_int = FIELD_GET(I40E_GLPCI_CNF2_MSI_X_VF_N_MASK, val);
> ...
> for (i = 0; i < num_pf_int - 2; i++)      // (3)
> 	wr32(hw, I40E_PFINT_DYN_CTLN(i), val);  // (4)
> ...
> for (i = 0; i < num_pf_int - 2; i++)			// (5)
> 	wr32(hw, I40E_PFINT_LNKLSTN(i), val);
> ...
> for (i = 0; i < num_vf_int - 2; i++)			// (6)
> 	wr32(hw, I40E_VPINT_LNKLSTN(i), val);
> 
> An example scenario for num_pf_int:
> (1) val = 0 (if MMIO read value was 0)
> (2) num_pf_int = 0 (also zero after bit field extraction from val)
> (3) An integer underflow occurs (num_pf_int - 2 == 0xfffffffe)
> (4) Out-of-bounds MMIO write access if access address exceeds the expected
> range.
> 
>  From above example scenario, the maximum access offset value can be around
> 0x4000347f8(=172G) which seems like this underflow is not intended(also there
> are masking operations like (2) for num_pf_int), so I report this issue.
> 
> I think similar issue also could happen at (5) and (6).
> 
> The following is the patch method I propose:
> 
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_common.c b/drivers/net/ethernet/intel/i40e/i40e_common.c
> index 370b4bddee44..97ef79be39b3 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_common.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_common.c
> @@ -848,19 +848,25 @@ 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++)
> -		wr32(hw, I40E_PFINT_DYN_CTLN(i), val);
> +	if (num_pf_int > 1) {

instead of adding if conditions, I would simply change the type
to be signed

> +		for (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++)
> -		wr32(hw, I40E_PFINT_LNKLSTN(i), val);
> +	if (num_pf_int > 1) {
> +		for (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++)
> -		wr32(hw, I40E_VPINT_LNKLSTN(i), val);
> +	if (num_vf_int > 1) {
> +		for (i = 0; i < num_vf_int - 2; i++)
> +			wr32(hw, I40E_VPINT_LNKLSTN(i), val);
> +	}
>   
>   	/* warn the HW of the coming Tx disables */
>   	for (i = 0; i < num_queues; i++) {
> 
> 
> Could you check this?
> 
> Best regards,
> Kyungwook Boo

RE: [Intel-wired-lan] MMIO write access to an invalid page in i40e_clear_hw()

[Loktionov, Aleksandr]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Przemek Kitszel
> Sent: Wednesday, March 5, 2025 11:27 AM
> To: Kyungwook Boo <bookyungwook@gmail.com>
> Cc: intel-wired-lan@lists.osuosl.org; linux-kernel@vger.kernel.org; Nguyen,
> Anthony L <anthony.l.nguyen@intel.com>
> Subject: Re: [Intel-wired-lan] MMIO write access to an invalid page in
> i40e_clear_hw()
> 
> On 3/3/25 11:19, Kyungwook Boo wrote:
> > Hello,
> >
> > It seems that there are invalid page MMIO write access in
> > i40e_clear_hw()
> 
> Hi,
> 
> is this something that actually occurred, or just a theoretical bug?
> (depending on that we will apply it to different tree)
> 
> please send a proper patch anyway, as it looks legit to don't go bananas when
> HW gives you 0
> 
> (and CC netdev instead of generic kernel ML, perhaps that's the reason this
> mail was tagged as spam for me)
> 
> > due to an integer underflow from num_pf_int(also num_vf_int seems
> possible).
> >
> > The following is a sample code in i40e_clear_hw():
> >
> > val = rd32(hw, I40E_GLPCI_CNF2); // (1) num_pf_int =
> > FIELD_GET(I40E_GLPCI_CNF2_MSI_X_PF_N_MASK, val); // (2) num_vf_int =
> > FIELD_GET(I40E_GLPCI_CNF2_MSI_X_VF_N_MASK, val); ...
> > for (i = 0; i < num_pf_int - 2; i++)      // (3)
> > 	wr32(hw, I40E_PFINT_DYN_CTLN(i), val);  // (4) ...
> > for (i = 0; i < num_pf_int - 2; i++)			// (5)
> > 	wr32(hw, I40E_PFINT_LNKLSTN(i), val); ...
> > for (i = 0; i < num_vf_int - 2; i++)			// (6)
> > 	wr32(hw, I40E_VPINT_LNKLSTN(i), val);
> >
> > An example scenario for num_pf_int:
> > (1) val = 0 (if MMIO read value was 0)
> > (2) num_pf_int = 0 (also zero after bit field extraction from val)
> > (3) An integer underflow occurs (num_pf_int - 2 == 0xfffffffe)
> > (4) Out-of-bounds MMIO write access if access address exceeds the
> > expected range.
> >
> >  From above example scenario, the maximum access offset value can be
> > around
> > 0x4000347f8(=172G) which seems like this underflow is not
> > intended(also there are masking operations like (2) for num_pf_int), so I
> report this issue.
> >
> > I think similar issue also could happen at (5) and (6).
> >
> > The following is the patch method I propose:
> >
> > diff --git a/drivers/net/ethernet/intel/i40e/i40e_common.c
> > b/drivers/net/ethernet/intel/i40e/i40e_common.c
> > index 370b4bddee44..97ef79be39b3 100644
> > --- a/drivers/net/ethernet/intel/i40e/i40e_common.c
> > +++ b/drivers/net/ethernet/intel/i40e/i40e_common.c
> > @@ -848,19 +848,25 @@ 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++)
> > -		wr32(hw, I40E_PFINT_DYN_CTLN(i), val);
> > +	if (num_pf_int > 1) {
> 
> instead of adding if conditions, I would simply change the type to be signed
Agree, but don't forget to make I signed too!

> > +		for (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++)
> > -		wr32(hw, I40E_PFINT_LNKLSTN(i), val);
> > +	if (num_pf_int > 1) {
> > +		for (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++)
> > -		wr32(hw, I40E_VPINT_LNKLSTN(i), val);
> > +	if (num_vf_int > 1) {
> > +		for (i = 0; i < num_vf_int - 2; i++)
> > +			wr32(hw, I40E_VPINT_LNKLSTN(i), val);
> > +	}
> >
> >   	/* warn the HW of the coming Tx disables */
> >   	for (i = 0; i < num_queues; i++) {
> >
> >
> > Could you check this?
> >
> > Best regards,
> > Kyungwook Boo

Re: [Intel-wired-lan] MMIO write access to an invalid page in i40e_clear_hw()

[Kyungwook Boo]

On 25. 3. 5. 21:11, Loktionov, Aleksandr wrote:
> 
> 
>> -----Original Message-----
>> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
>> Przemek Kitszel
>> Sent: Wednesday, March 5, 2025 11:27 AM
>> To: Kyungwook Boo <bookyungwook@gmail.com>
>> Cc: intel-wired-lan@lists.osuosl.org; linux-kernel@vger.kernel.org; Nguyen,
>> Anthony L <anthony.l.nguyen@intel.com>
>> Subject: Re: [Intel-wired-lan] MMIO write access to an invalid page in
>> i40e_clear_hw()
>>
>> On 3/3/25 11:19, Kyungwook Boo wrote:
>>> Hello,
>>>
>>> It seems that there are invalid page MMIO write access in
>>> i40e_clear_hw()
>>
>> Hi,
>>
>> is this something that actually occurred, or just a theoretical bug?
>> (depending on that we will apply it to different tree)
>>
>> please send a proper patch anyway, as it looks legit to don't go bananas when
>> HW gives you 0
>>
>> (and CC netdev instead of generic kernel ML, perhaps that's the reason this
>> mail was tagged as spam for me)

Hi,

thank you for your response.

If you’re asking whether this happened on a physical device, it did not, but it
is something that can be triggered through virtual device with fuzzed input.

Also, I'll follow your guidance when sending the proper patch.

>>> diff --git a/drivers/net/ethernet/intel/i40e/i40e_common.c
>>> b/drivers/net/ethernet/intel/i40e/i40e_common.c
>>> index 370b4bddee44..97ef79be39b3 100644
>>> --- a/drivers/net/ethernet/intel/i40e/i40e_common.c
>>> +++ b/drivers/net/ethernet/intel/i40e/i40e_common.c
>>> @@ -848,19 +848,25 @@ 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++)
>>> -		wr32(hw, I40E_PFINT_DYN_CTLN(i), val);
>>> +	if (num_pf_int > 1) {
>>
>> instead of adding if conditions, I would simply change the type to be signed

I’ll incorporate the suggested approach when sending the patch.

Best,
Kyungwook Boo