Re: [PATCH net 5/6] e1000e: Fix out-of-bounds MMIO access by validating BAR0 size

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The patch checks the size of BAR0 only.

--

e1000e: Fix out-of-bounds MMIO access by validating BAR0 size

This commit makes e1000_probe() reject devices whose BAR0 is smaller than
SZ_64K, before ioremap() is called. It fixes a syzkaller-reported oops.
In that report a foreign device with a 4K BAR0 was bound through new_id,
and probe read EEMNGCTL past the end of the mapping.

> diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
> index 746a395869998..ad9b88c9af225 100644
> --- a/drivers/net/ethernet/intel/e1000e/netdev.c
> +++ b/drivers/net/ethernet/intel/e1000e/netdev.c
> @@ -7455,6 +7455,12 @@ static int e1000_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  	mmio_len = pci_resource_len(pdev, 0);
>  
>  	err = -EIO;
> +	/* Smallest BAR0 that covers every register the driver accesses */
> +	if (mmio_len < SZ_64K) {
> +		dev_err(&pdev->dev, "MMIO len is too small\n");
> +		goto err_ioremap;
> +	}
> +
>  	adapter->hw.hw_addr = ioremap(mmio_start, mmio_len);
>  	if (!adapter->hw.hw_addr)
>  		goto err_ioremap;

[Severity: Medium]
Does this check also cover the flash registers on ICH/PCH parts older
than SPT?

The commit message says:

  This accounts not only for standard registers up to E1000_SYSSTMPH,
  but also for flash registers mapped on ICH/PCH chipsets (up to offset
  0xE074 / ~57.1 KB). Since PCI BAR sizes are power-of-two aligned,
  SZ_64K is the minimum valid BAR0 size required to ensure all
  subsequent MMIO accesses remain strictly within the mapped boundary.

That seems true only for e1000_pch_spt and later. On those parts,
e1000_init_nvm_params_ich8lan() puts the flash registers inside BAR0:

	hw->flash_address = hw->hw_addr + E1000_FLASH_BASE_ADDR;

On ICH8/9/10, PCH, PCH2 and PCH_LPT the flash registers are in BAR1.
e1000_probe() maps BAR1 right after this hunk, with no minimum length:

	if ((adapter->flags & FLAG_HAS_FLASH) &&
	    (pci_resource_flags(pdev, 1) & IORESOURCE_MEM) &&
	    (hw->mac.type < e1000_pch_spt)) {
		flash_start = pci_resource_start(pdev, 1);
		flash_len = pci_resource_len(pdev, 1);
		adapter->hw.flash_address = ioremap(flash_start, flash_len);

Probe then reads and writes fixed offsets in that mapping. The second
step runs because FLAG_READ_ONLY_NVM is on by default for ICH parts:

e1000_probe()
  ei->get_variants()
    e1000_init_nvm_params_ich8lan()
      er32flash(ICH_FLASH_GFPREG)             /* 0x0 */
  e1000e_write_protect_nvm_ich8lan()
    er32flash(ICH_FLASH_PR0)                  /* 0x74 */
    ew32flash(ICH_FLASH_PR0, pr0.regval)
    er16flash(ICH_FLASH_HSFSTS)
    ew32flash(ICH_FLASH_HSFSTS, hsfsts.regval)

new_id_store() accepts a 7-field new_id. The driver_data can match any
existing table entry, such as board_ich8lan, board_pchlan or
board_pch_lpt. So the reproducer from the commit message can still be
used against a foreign device that has:

  - a BAR0 of at least 64K, which passes the new check
  - a memory BAR1 smaller than 0x78 bytes (PCI allows memory BARs as
    small as 16 bytes)

Would the PR0 read-modify-write at offset 0x74 then go past the end of
the BAR1 mapping? If the small BAR sits at the end of a page, this
could fault like the EEMNGCTL oops. Otherwise it could read and write
registers of whatever device decodes the next addresses.

The later "e1000e: add system to disable K1 list" patch in this series
does not appear to change this path.

Would BAR1 need its own minimum size check for the commit message's
claim to hold? That could be at least ICH_FLASH_PR0 + 4, or SZ_4K to
match real hardware.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261001222443.3500206-1-anthony.l.nguyen%40intel.com