Re: [PATCH net v2] net: e1000: fix warning in iounmap on probe failure

[netdev-bot+sashiko@kernel.org]

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

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

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] Commit message misdescribes the defect and half of the added
  code is dead.
- [Low] The submission carries `Reported-by:
  syzbot+ca1ef9e2e234b8d3599b@syzkaller.appspotmail.com` but the next…

--

net: e1000: fix warning in iounmap on probe failure

This adds NULL checks before the two iounmap() calls in the e1000_probe()
error unwind path, for hw->ce4100_gbe_mdio_base_virt and hw->hw_addr, in
response to a syzbot report of a WARN from iounmap().

> During a probe failure, the e1000 driver error handling path calls
> iounmap() on hw->ce4100_gbe_mdio_base_virt and hw->hw_addr without
> checking if they were previously mapped. This triggers a kernel warning
> (WARN) rather than a fatal crash when passed an uninitialized pointer.

[Severity: Medium]
Is the hw->hw_addr half of this description accurate?  hw->hw_addr has a
single assignment in the whole driver, in e1000_probe():

	err = -EIO;
	hw->hw_addr = pci_ioremap_bar(pdev, BAR_0);
	if (!hw->hw_addr)
		goto err_ioremap;

The err_ioremap label sits below the iounmap block being patched, and
nothing ever resets hw->hw_addr to NULL.  Every goto that lands in the
err_sw_init / err_dma / err_mdio_ioremap region (e1000_init_hw_struct()
failure, dma_set_mask_and_coherent() failure, e1000_sw_init() failure,
the ce4100 ioremap() failure) happens after hw_addr was successfully
mapped.  So can the new if (hw->hw_addr) test ever be false?

The pointer that can legitimately be NULL there is
hw->ce4100_gbe_mdio_base_virt, which is only mapped for one MAC type:

	if (hw->mac_type == e1000_ce4100) {
		hw->ce4100_gbe_mdio_base_virt =
					ioremap(pci_resource_start(pdev, BAR_1),
						pci_resource_len(pdev, BAR_1));

		if (!hw->ce4100_gbe_mdio_base_virt)
			goto err_mdio_ioremap;
	}

and x86's iounmap() does:

	if (WARN_ON_ONCE(!is_ioremap_addr((void __force *)addr)))
		return;

which looks like the warning syzbot hit.

Also, the pointer is not uninitialized: the adapter is zeroed by
alloc_etherdev(), so ce4100_gbe_mdio_base_virt is a well defined NULL.
Could the commit message be reworded to say that, and could the second
Fixes: tag (1dc329180fe22 "e1000: Use hw, er32, and ew32") be dropped
since the hw_addr case does not appear to be reachable?

One more thought on form: e1000_remove() already guards the same two
pointers the other way around:

	if (hw->mac_type == e1000_ce4100)
		iounmap(hw->ce4100_gbe_mdio_base_virt);
	iounmap(hw->hw_addr);

Would matching that idiom here keep the two paths consistent?

> diff --git a/drivers/net/ethernet/intel/e1000/e1000_main.c b/drivers/net/ethernet/intel/e1000/e1000_main.c
> index d7f5c6f161420..156903089fa6b 100644
> --- a/drivers/net/ethernet/intel/e1000/e1000_main.c
> +++ b/drivers/net/ethernet/intel/e1000/e1000_main.c
> @@ -1227,8 +1227,10 @@ static int e1000_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  	kfree(adapter->rx_ring);
>  err_dma:
>  err_sw_init:
> -	iounmap(hw->ce4100_gbe_mdio_base_virt);
> -	iounmap(hw->hw_addr);
> +	if (hw->ce4100_gbe_mdio_base_virt)
> +		iounmap(hw->ce4100_gbe_mdio_base_virt);
> +	if (hw->hw_addr)
> +		iounmap(hw->hw_addr);
>  err_ioremap:
>  	disable_dev = !test_and_set_bit(__E1000_DISABLED, &adapter->flags);
>  	free_netdev(netdev);

On the tag block:

> Reported-by: syzbot+ca1ef9e2e234b8d3599b@syzkaller.appspotmail.com
> Fixes: 13acde8fffc0a ("e1000: cleanup CE4100 MDIO registers access")

[Severity: Low]
Could a Closes: or Link: line with the syzkaller dashboard URL be added
immediately after the Reported-by: line?  scripts/checkpatch.pl checks
for this:

	# check if Reported-by: is followed by a Closes: tag
			if ($sign_off =~ /^reported(?:|-and-tested)-by:$/i) {

This matters here because the body defers all reproduction evidence to
the dashboard:

> This issue was found by Syzbot. The exact kernel configuration (.config)
> and the C reproducer used to trigger this warning are available on the
> Syzbot dashboard.

Without the URL there is no way to check which configuration reaches the
e1000_probe() error path with hw->ce4100_gbe_mdio_base_virt == NULL.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260922155238.67445-1-nsvatoslav515%40gmail.com