diff mbox series

MMIO write access to an invalid page in i40e_clear_hw()

Message ID ffc91764-1142-4ba2-91b6-8c773f6f7095@gmail.com
State RFC
Headers show
Series MMIO write access to an invalid page in i40e_clear_hw() | expand

Commit Message

Kyungwook Boo March 3, 2025, 10:19 a.m. UTC 
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:



Could you check this?

Best regards,
Kyungwook Boo

Comments

Loktionov, Aleksandr March 5, 2025, 10:13 a.m. UTC | #1 
> -----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
Przemek Kitszel March 5, 2025, 10:27 a.m. UTC | #2 
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
Loktionov, Aleksandr March 5, 2025, 12:11 p.m. UTC | #3 
> -----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
Kyungwook Boo March 6, 2025, 12:19 a.m. UTC | #4 
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.
Kyungwook Boo March 6, 2025, 12:41 a.m. UTC | #5 
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
diff mbox series

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) {
+		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++) {