> -----Original Message----- > From: Intel-wired-lan <intel-wired-lan-boun...@osuosl.org> On Behalf Of > Kyungwook Boo > Sent: Monday, March 3, 2025 11:20 AM > To: Nguyen, Anthony L <anthony.l.ngu...@intel.com>; Kitszel, Przemyslaw > <przemyslaw.kits...@intel.com> > Cc: intel-wired-...@lists.osuosl.org; linux-ker...@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
