> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-boun...@osuosl.org> On Behalf Of
> Przemek Kitszel
> Sent: Wednesday, March 5, 2025 11:27 AM
> To: Kyungwook Boo <bookyungw...@gmail.com>
> Cc: intel-wired-...@lists.osuosl.org; linux-ker...@vger.kernel.org; Nguyen,
> Anthony L <anthony.l.ngu...@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
