[PATCH net 1/3] e1000e: fix incorrect modified flag check in e1000_read_nvm_spt()

[PATCH net 1/3] e1000e: fix incorrect modified flag check in e1000_read_nvm_spt()

[Tony Nguyen]

From: Ivy Lopez <skunkolee@gmail.com>

e1000_read_nvm_spt() reads two adjacent 16-bit NVM words as one
32-bit dword for efficiency. When deciding whether to use the
shadow RAM value for the high word (data[i + 1]), it incorrectly
checks the "modified" flag of the low word (shadow_ram[offset + i])
instead of the high word's own flag (shadow_ram[offset + i + 1]).

This can cause silent NVM shadow RAM corruption:
 - if only the high word is marked modified, its pending write is
   lost and the stale flash value is returned instead
 - if only the low word is marked modified, the high word is
   incorrectly overwritten with the low word's shadow value

This function is used on all PCH chips from SPT onward (SPT, CNP,
TGP, ADP, MTP, etc). The bug has been present since the dword-read
optimization was introduced for these chips.

Note: this was found via code review while investigating a separate
reproducible packet loss issue on I219-LM (SPT+ PCH). Testing showed
this fix does not resolve that particular symptom, but the flag
check is still a genuine logic bug that can corrupt shadow RAM
contents and should be fixed regardless.

Fixes: 79849ebc0e06 ("e1000e: initial support for i219")
Signed-off-by: Ivy Lopez <skunkolee@gmail.com>
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Reviewed-by: Dima Ruinskiy <dima.ruinskiy@intel.com>
Tested-by: Moriya Kadosh <moriyax.kadosh@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/e1000e/ich8lan.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.c b/drivers/net/ethernet/intel/e1000e/ich8lan.c
index aa90e0ce8aca..87cd1cdc6f10 100644
--- a/drivers/net/ethernet/intel/e1000e/ich8lan.c
+++ b/drivers/net/ethernet/intel/e1000e/ich8lan.c
@@ -3473,7 +3473,7 @@ static s32 e1000_read_nvm_spt(struct e1000_hw *hw, u16 offset, u16 words,
 				    dev_spec->shadow_ram[offset + i].value;
 			else
 				data[i] = (u16)(dword & 0xFFFF);
-			if (dev_spec->shadow_ram[offset + i].modified)
+			if (dev_spec->shadow_ram[offset + i + 1].modified)
 				data[i + 1] =
 				    dev_spec->shadow_ram[offset + i + 1].value;
 			else
-- 
2.47.1

Re: [PATCH net 1/3] e1000e: fix incorrect modified flag check in e1000_read_nvm_spt()

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by LLM and not proven at runtime). If it was triggered
   please include the symptoms, like the stack trace or kernel messages.

 - What hardware the change was tested on. For driver fixes please
   mention the device (and if relevant firmware version) used for
   testing, or say that the change was not tested on real hardware.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH net 1/3] e1000e: fix incorrect modified flag check in e1000_read_nvm_spt()

[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] The commit message overstates what the patch fixes.
- [Low] The second failure-mode bullet in the commit message is wrong.

--

e1000e: fix incorrect modified flag check in e1000_read_nvm_spt()

This patch changes the two-word branch of e1000_read_nvm_spt(). The high
word (data[i + 1]) is now taken from shadow RAM based on its own modified
flag, shadow_ram[offset + i + 1], instead of the low word's flag. The
commit message calls this a fix for silent NVM shadow RAM corruption on
SPT and later PCH parts.

> This can cause silent NVM shadow RAM corruption:
>  - if only the high word is marked modified, its pending write is
>    lost and the stale flash value is returned instead
>  - if only the low word is marked modified, the high word is
>    incorrectly overwritten with the low word's shadow value

[Severity: Low]
Is the second case described accurately? The pre-patch code was:

    if (dev_spec->shadow_ram[offset + i].modified)
        data[i + 1] =
            dev_spec->shadow_ram[offset + i + 1].value;

The assigned value comes from the high word's own entry
(offset + i + 1), not from the low word's entry.

Unmodified entries hold 0xFFFF. e1000_init_nvm_params_ich8lan() sets
them that way, and they are reset the same way after each commit:

    for (i = 0; i < nvm->word_size; i++) {
        dev_spec->shadow_ram[i].modified = false;
        dev_spec->shadow_ram[i].value = 0xFFFF;
    }

So when only the low word was modified, wouldn't the old code return
0xFFFF for the high word, not the low word's pending value?

> This function is used on all PCH chips from SPT onward (SPT, CNP,
> TGP, ADP, MTP, etc). The bug has been present since the dword-read
> optimization was introduced for these chips.
>
> Note: this was found via code review while investigating a separate
> reproducible packet loss issue on I219-LM (SPT+ PCH). Testing showed
> this fix does not resolve that particular symptom, but the flag
> check is still a genuine logic bug that can corrupt shadow RAM
> contents and should be fixed regardless.
>
> Fixes: 79849ebc0e06 ("e1000e: initial support for i219")

[Severity: Medium]
Can any in-tree caller reach the changed line on SPT+ parts?

The only way into e1000_read_nvm_spt() is hw->nvm.ops.read, through
e1000_read_nvm(). Every e1000_read_nvm() call in e1000e passes words=1,
except the one in e1000_get_eeprom():

    if (hw->nvm.type == e1000_nvm_eeprom_spi) {
        ret_val = e1000_read_nvm(hw, first_word,
                                 last_word - first_word + 1,
                                 eeprom_buff);
    } else {
        for (i = 0; i < last_word - first_word + 1; i++) {
            ret_val = e1000_read_nvm(hw, first_word + i, 1,
                                     &eeprom_buff[i]);

e1000_init_nvm_params_ich8lan() sets nvm->type = e1000_nvm_flash_sw for
every ICH/PCH/SPT+ part, so these parts never use the multi-word read.

With words=1, the loop in e1000_read_nvm_spt() runs once with i=0. It
takes the words - i == 1 branch, which already checks
shadow_ram[offset + i].modified correctly.

Also, e1000_read_nvm_spt() never writes shadow_ram. The only writers are
e1000_init_nvm_params_ich8lan(), e1000_write_nvm_ich8lan(), and the
resets after a commit. Even with a multi-word caller, wouldn't the worst
case be a wrong value returned to that caller, not corrupted shadow RAM?

The code change looks correct, but it fixes a latent bug. With the
Fixes: tag, stable will likely take it as an active data-integrity fix.

Could the commit message say that no in-tree path reaches the two-word
branch today? It could also drop the "silent NVM shadow RAM corruption"
and "can corrupt shadow RAM contents" wording.

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

[PATCH net 2/3] igb/igbvf: disable work items before device removal

[Tony Nguyen]

From: Xuanqiang Luo <luoxuanqiang@kylinos.cn>

A watchdog work item that is already running can queue the reset work after
the remove path has canceled it. The netdev Tx timeout handler can also
queue the reset work until unregister_netdev() shuts the interface down.
In igb, device reset interrupts provide another enqueue path. The reset
work may then run after free_netdev(), resulting in access to the freed
adapter memory.

Although igb_reset_task() checks __IGB_DOWN, checking the bit already
dereferences the adapter and therefore cannot protect its lifetime.

Disable the watchdog work first and the reset work second in both drivers.
Disabling the work items also prevents racing attempts to queue them during
device removal.

Fixes: 760141a53e5d ("igb[v],ixgbe: don't use flush_scheduled_work()")
Cc: stable@vger.kernel.org
Signed-off-by: Xuanqiang Luo <luoxuanqiang@kylinos.cn>
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Tested-by: Alexander Nowlin <alexander.nowlin@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/igb/igb_main.c | 4 ++--
 drivers/net/ethernet/intel/igbvf/netdev.c | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index d4a897a8c82c..933dd1c29950 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -3887,8 +3887,8 @@ static void igb_remove(struct pci_dev *pdev)
 	timer_delete_sync(&adapter->watchdog_timer);
 	timer_delete_sync(&adapter->phy_info_timer);
 
-	cancel_work_sync(&adapter->reset_task);
-	cancel_work_sync(&adapter->watchdog_task);
+	disable_work_sync(&adapter->watchdog_task);
+	disable_work_sync(&adapter->reset_task);
 
 #ifdef CONFIG_IGB_DCA
 	if (adapter->flags & IGB_FLAG_DCA_ENABLED) {
diff --git a/drivers/net/ethernet/intel/igbvf/netdev.c b/drivers/net/ethernet/intel/igbvf/netdev.c
index c686ee120a14..125876a18b5c 100644
--- a/drivers/net/ethernet/intel/igbvf/netdev.c
+++ b/drivers/net/ethernet/intel/igbvf/netdev.c
@@ -2901,8 +2901,8 @@ static void igbvf_remove(struct pci_dev *pdev)
 	set_bit(__IGBVF_DOWN, &adapter->state);
 	timer_delete_sync(&adapter->watchdog_timer);
 
-	cancel_work_sync(&adapter->reset_task);
-	cancel_work_sync(&adapter->watchdog_task);
+	disable_work_sync(&adapter->watchdog_task);
+	disable_work_sync(&adapter->reset_task);
 
 	unregister_netdev(netdev);
 
-- 
2.47.1

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

[Tony Nguyen]

From: Svyatoslav Nikolenko <nsvatoslav515@gmail.com>

In e1000_probe(), the error unwinding path at err_sw_init unconditionally
calls iounmap(hw->ce4100_gbe_mdio_base_virt). On non-CE4100 hardware,
hw->ce4100_gbe_mdio_base_virt remains NULL.
Passing NULL to iounmap() triggers a WARN_ON_ONCE on x86 architectures.
Furthermore, on CE4100 hardware, if probe fails before or during
MDIO mapping, ce4100_gbe_mdio_base_virt is also NULL.

Fix this by adding a NULL pointer check before calling iounmap() on
hw->ce4100_gbe_mdio_base_virt. iounmap(hw->hw_addr) remains unconditional
because every code path reaching err_sw_init occurs after hw->hw_addr has
been successfully mapped.

Reported-by: syzbot+ca1ef9e2e234b8d3599b@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=ca1ef9e2e234b8d3599b
Fixes: 13acde8fffc0 ("e1000: cleanup CE4100 MDIO registers access")
Cc: stable@vger.kernel.org
Signed-off-by: Svyatoslav Nikolenko <nsvatoslav515@gmail.com>
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/e1000/e1000_main.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/e1000/e1000_main.c b/drivers/net/ethernet/intel/e1000/e1000_main.c
index d7f5c6f16142..d7e279b739b8 100644
--- a/drivers/net/ethernet/intel/e1000/e1000_main.c
+++ b/drivers/net/ethernet/intel/e1000/e1000_main.c
@@ -1227,7 +1227,8 @@ 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);
+	if (hw->ce4100_gbe_mdio_base_virt)
+		iounmap(hw->ce4100_gbe_mdio_base_virt);
 	iounmap(hw->hw_addr);
 err_ioremap:
 	disable_dev = !test_and_set_bit(__E1000_DISABLED, &adapter->flags);
-- 
2.47.1

Re: [PATCH net 0/3][pull request] Intel Wired LAN Driver Updates 2026-11-12 (e1000e, igb, igbvf, e1000)

[Tony Nguyen]

On 11/12/2026 4:58 PM, Tony Nguyen wrote:
> Ivy Lopez corrects NVM read on e1000e which was not properly checking
> the high word's modified flag.
> 
> Xuanqiang Luo fixes igb and igbvf device removal by disabling watchdog and
> reset work to prevent work from being queued after cancellation.
> 
> Svyatoslav Nikolenko resolves an e1000 probe failure warning by checking
> that the CE4100 MDIO mapping is non-NULL before calling iounmap().

It looks like the cover didn't make it to the list [1], presumably, due 
to the Outlook outage. Did you want me to resend the series?

Thanks,
Tony

[1] 
https://lore.kernel.org/netdev/20261113005823.130021-2-anthony.l.nguyen@intel.com/

> The following are changes since commit bbcd02741c4cfbf9e1c59b1906f21ef772bd15eb:
>    net: mana: report RX checksum offload for IPv6
> and are available in the git repository at:
>    git://git.kernel.org/pub/scm/linux/kernel/git/tnguy/net-queue 1GbE
> 
> Ivy Lopez (1):
>    e1000e: fix incorrect modified flag check in e1000_read_nvm_spt()
> 
> Svyatoslav Nikolenko (1):
>    net: e1000: fix warning in iounmap on probe failure
> 
> Xuanqiang Luo (1):
>    igb/igbvf: disable work items before device removal
> 
>   drivers/net/ethernet/intel/e1000/e1000_main.c | 3 ++-
>   drivers/net/ethernet/intel/e1000e/ich8lan.c   | 2 +-
>   drivers/net/ethernet/intel/igb/igb_main.c     | 4 ++--
>   drivers/net/ethernet/intel/igbvf/netdev.c     | 4 ++--
>   4 files changed, 7 insertions(+), 6 deletions(-)
>