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

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

[Svyatoslav Nikolenko]

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.

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.

Fix this by adding NULL checks before calling iounmap().

Reported-by: syzbot+ca1ef9e2e234b8d3599b@syzkaller.appspotmail.com
Fixes: 13acde8fffc0a ("e1000: cleanup CE4100 MDIO registers access")
Fixes: 1dc329180fe22 ("e1000: Use hw, er32, and ew32")
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Signed-off-by: Svyatoslav Nikolenko <nsvatoslav515@gmail.com>
---
v2:
  - Expanded commit message to answer reviewer questions (reproduction details)
  - Added appropriate Fixes tags
  - Added Reviewed-by tag from Aleksandr Loktionov

 drivers/net/ethernet/intel/e1000/e1000_main.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/e1000/e1000_main.c b/drivers/net/ethernet/intel/e1000/e1000_main.c
index d7f5c6f16142..156903089fa6 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);
-- 
2.47.3

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

[netdev-bot+sashiko]

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] Half of the patch is provably dead code and the commit message
  over-claims the defect.
- [Low] The commit carries `Reported-by:
  syzbot+ca1ef9e2e234b8d3599b@syzkaller.appspotmail.com` and asserts…

--

net: e1000: fix warning in iounmap on probe failure

This patch adds NULL checks before the two iounmap() calls in the
err_sw_init/err_dma error unwind path of e1000_probe(), so that a probe
failure does not pass a NULL pointer to iounmap().  It carries two Fixes:
tags and a syzbot Reported-by:.

No Link: tags were present in the commit header.

Two questions below, one on the commit message tags and one on the diff.

> net: e1000: fix warning in iounmap on probe failure
>
> 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.
>
> 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.
>
> Fix this by adding NULL checks before calling iounmap().
>
> Reported-by: syzbot+ca1ef9e2e234b8d3599b@syzkaller.appspotmail.com
> Fixes: 13acde8fffc0a ("e1000: cleanup CE4100 MDIO registers access")

[Severity: Low]
The Reported-by: line is followed directly by a Fixes: line, with no
Closes: or Link: URL pointing at the syzbot report.  Could that URL be
added?

Documentation/process/submitting-patches.rst asks for it:

    The tag should be followed by a Closes: tag pointing to the report,
    unless the report is not available on the web

and scripts/checkpatch.pl warns on this pattern:

    BAD_REPORTED_BY_LINK
    "Reported-by: should be immediately followed by Closes: or Link:
     with a URL to the report"

The commit message says the .config and the C reproducer "are available on
the Syzbot dashboard", but without the link there is no way to check which
error path syzbot actually reached, which is relevant to the question
below.

Also, the description of the value handed to iounmap() as an
"uninitialized pointer" does not look accurate: the adapter private area
comes from alloc_etherdev()/alloc_netdev_mqs(), which zero the allocation,
so hw->ce4100_gbe_mdio_base_virt is deterministically NULL rather than
garbage.  Could that wording be adjusted?

> 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:

[Severity: Medium]
Can the new "if (hw->hw_addr)" test ever be false?

hw->hw_addr is assigned in exactly one place in the 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(hw->hw_addr) that is being
guarded here, so the only path that leaves hw_addr NULL skips this code
entirely.  Every other goto that lands on this label (err_sw_init,
err_dma, err_mdio_ioremap, err_eeprom, err_register) is reachable only
after pci_ioremap_bar() already returned a valid mapping.

If that reading is right, the hw_addr half of the change is dead code, and
the second tag

	Fixes: 1dc329180fe22 ("e1000: Use hw, er32, and ew32")

refers to a defect that is not present.  Should that tag be dropped, along
with the mention of hw->hw_addr in the commit message, so a stable
backport is not aimed at a path that cannot trigger?

The ce4100_gbe_mdio_base_virt half does look like a real fix, since that
mapping is only created 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));

so on every other device the field stays NULL while the unwind path still
calls iounmap() on it, and x86 iounmap() does:

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

Would it be clearer to describe only that case in the changelog?

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

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

[netdev-bot+sashiko]

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

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

[Svyatoslav Nikolenko]

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.

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.

Fix this by adding NULL checks before calling iounmap().

Reported-by: syzbot+ca1ef9e2e234b8d3599b@syzkaller.appspotmail.com
Fixes: 13acde8fffc0a ("e1000: cleanup CE4100 MDIO registers access")
Fixes: 1dc329180fe22 ("e1000: Use hw, er32, and ew32")
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Signed-off-by: Svyatoslav Nikolenko <nsvatoslav515@gmail.com>
---
v2:
  - Expanded commit message to answer reviewer questions (reproduction details)
  - Added appropriate Fixes tags
  - Added Reviewed-by tag from Aleksandr Loktionov

 drivers/net/ethernet/intel/e1000/e1000_main.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/e1000/e1000_main.c b/drivers/net/ethernet/intel/e1000/e1000_main.c
index d7f5c6f16142..156903089fa6 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);
-- 
2.47.3