[PATCH net 0/2] forcedeth: two register-window bounds fixes

[PATCH net 0/2] forcedeth: two register-window bounds fixes

[Marek Czernohous]

From: Marek Czernohous <marek@czernohous.de>

Two bounds fixes in forcedeth, both in the same shape: a loop that walks
the register window one step too far. They are independent of each other
and touch different functions.

1/2 nv_suspend() and nv_resume() save and restore the non-PCI config
    space with i <= register_size/sizeof(u32). On a VER3 device that is
    exactly the length of saved_config_space[], so the last iteration
    reads and writes one element past the array, and on resume it
    writel()s that element one dword past the length the driver mapped.
    UBSAN catches it.

2/2 nv_tx_timeout() dumps the window in rows of eight dwords but only
    bounds the row's starting offset, so the final row reads between 12
    and 28 bytes past register_size, on every one of the three supported
    window sizes.

Neither is a regression. Both are long standing, and 1/2 in particular
is not new to the list:

  - The identical off-by-one in nv_get_regs() was fixed by commit
    ba9aa134287f ("forcedeth: fix buffer overflow") in 2012. The two
    loops in this patch were missed at the time.
  - The suspend and resume side was then reported on LKML in September
    2013 by Marc Weber, with the same analysis and the same
    one-character fix. Sergei Shtylyov replied asking for the patch
    inline rather than attached, and the thread ended there.

So this is not a new discovery. It is the same bug at the two sites the
2012 fix did not reach, finally sent in the form the list asks for.

How bad is it, stated plainly

  1/2 writes one u32 past the end of a declared array, on a suspend
  path, on every suspend of a VER3 device. That is an out-of-bounds
  store, it is what UBSAN reports, and with CONFIG_UBSAN_TRAP=y it is a
  trap that aborts the running kernel code. That is the stable case, and
  I think it stands on its own: memory safety, reproduced on hardware,
  one character to fix, no behavioural change for anyone else.

  What I will not claim is drama beyond that. The element it lands in is
  np->name_rx, a scratch string that nv_request_irq() rewrites with
  sprintf() before it is ever used, so on a kernel without UBSAN_TRAP
  nothing observable is corrupted. The patch says which member and why,
  so you can judge the severity yourself instead of taking my word.

  The MMIO side of both patches is milder still. ioremap() rounds the
  requested length up to page granularity, so these accesses stay inside
  the page the CPU has mapped and no fault is expected on any
  architecture with PAGE_SIZE >= 4K. What they leave is the window the
  driver asked for. 2/2 is only that, and carries no stable tag.

Behaviour change in 2/2, so it is not buried in the patch

  The partial trailing row of the debug dump is no longer printed: 16
  bytes for VER1, 20 for VER2, 4 for VER3. That is a deliberate trade
  against open-coding a second, narrower dump in a debug-only path. If
  you would rather keep those registers, a short remainder loop on top
  is the obvious follow-up.

Testing

  Reference hardware: Apple Macmini3,1 (MCP79 chipset), forcedeth
  driving the onboard NIC.

  1/2 is reproduced and fixed on that machine. One point of method
  first: UBSAN reports each source location only once per module load,
  so a quiet second suspend proves nothing. Both runs below are the
  first S3 cycle after a fresh load of the module in question.

    stock module,   first S3 after load:  2 splats, one per loop
    patched module, first S3 after load:  none

  The patched module was built, stripped, installed and reloaded, with
  the md5 of the running module checked against the installed one. The
  link came back, the DHCP lease was restored and ping showed no loss.
  That measurement was taken on 2026-08-04 on a 7.1.6 based kernel. The
  stock half has since been reproduced again on 7.1.8, most recently on
  2026-08-13, reporting line 6225 from pci_pm_suspend and line 6240
  from pci_pm_resume.

  I have not repeated the patched half on net/main itself. The runtime
  measurements come from a distro kernel on the reference hardware,
  which is the only machine I have with this NIC; the series itself is
  based on and built against net/main.

  2/2 has no runtime test. Its path sits behind the debug_tx_timeout
  module parameter and needs a genuine TX timeout, which I cannot force
  safely on this machine. It rests on the arithmetic in the patch and
  on the build below.

  Build: allmodconfig with W=1 on x86_64, whole tree, zero compiler
  warnings and zero errors; forcedeth.c specifically produces none.
  That took about 30 hours on the two cores I have, which is why I say
  it plainly rather than in passing.

  I have not run allyesconfig. If you want that too, say so and I will
  queue it before reposting rather than claim a build I did not do.

Two checkpatch notes on 1/2, both deliberate

  "Prefer a maximum 75 chars per line" fires on a line that is quoted
  UBSAN output. The splat is trimmed, and 1/2 says what was cut, but I
  did not rewrap the lines that remain: reflowing diagnostic output to
  satisfy a heuristic makes it harder to match against a real log.

  Two "spaces preferred around that '/'" CHECKs fire on
  register_size/sizeof(u32). That spacing is what the file already uses,
  including in nv_get_regs(), which is otherwise the same loop. Adding
  spaces would leave the two lines I touch inconsistent with their
  neighbourhood, so I kept the change to the one character that is
  wrong. Happy to do it the other way round if you prefer.

AI assistance

  Per Documentation/process/coding-assistants.rst: this work is AI
  assisted. I use Claude (claude-opus-5) as a coding and analysis
  assistant. Both patches carry an Assisted-by trailer accordingly, and
  no Signed-off-by is added by the tool.

  Nature of the assistance: the assistant did the code archaeology and
  most of the drafting. I described the symptom, asked for the mechanism
  to be traced in the source rather than guessed, and asked for each
  claim to be backed by a file and a line. The UBSAN output and the S3
  measurements are from the machine, not model output.

  It is also what found the 2012 fix and the 2013 report above, on a
  second pass over an earlier draft of this posting that claimed the bug
  had never been reported. That claim was wrong and would have wasted
  your time, so it seems worth saying that the checking pass is part of
  the process here and not a flourish. I reviewed the result, I
  understand the code, and I take responsibility for it.

Marek Czernohous (2):
  forcedeth: fix off-by-one when saving/restoring non-PCI config space
  forcedeth: stop the tx_timeout register dump past the requested window

 drivers/net/ethernet/nvidia/forcedeth.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)


base-commit: 9006c116dd111d457bf5d074990210f70a4ad2c8
-- 
2.54.0

[PATCH net 1/2] forcedeth: fix off-by-one when saving/restoring non-PCI config space

[Marek Czernohous]

From: Marek Czernohous <marek@czernohous.de>

nv_suspend() and nv_resume() walk the non-PCI configuration space with

	for (i = 0; i <= np->register_size/sizeof(u32); i++)

which runs one iteration too many. saved_config_space is declared as

	u32 saved_config_space[NV_PCI_REGSZ_MAX/4];

and NV_PCI_REGSZ_VER3 is equal to NV_PCI_REGSZ_MAX (0x604), so on a VER3
device register_size/sizeof(u32) is exactly the array length and the last
iteration addresses one element past the end.

The element it lands on is np->name_rx[0..3]: saved_config_space[] is
followed immediately by char name_rx[IFNAMSIZ + 3], and char needs no
padding. Nothing observable is corrupted by that, because nv_request_irq()
rewrites name_rx with sprintf() before it is ever passed to request_irq().
The bug is the out-of-bounds access itself, which UBSAN reports and which
CONFIG_UBSAN_TRAP=y turns into a trap that aborts the running kernel code,
plus an MMIO read and, on resume, an MMIO writel() to base + 0x604, one
dword past the range the driver mapped:

	np->base = ioremap(addr, np->register_size);

VER1 and VER2 devices stay inside the array, but they too get the stray
read and the stray write one dword past their own window.

Caught by UBSAN on an Apple Macmini3,1 (MCP79) during a deep S3 cycle.
The splat below is trimmed: the build path in the file name, the CPU
and taint lines, the Workqueue line, the "?" hint frames, and the
frames below device_suspend are all cut. The kernel was tainted, with
an out-of-tree nouveau and CPU_OUT_OF_SPEC; forcedeth itself was the
stock module.

  UBSAN: array-index-out-of-bounds in drivers/net/ethernet/nvidia/forcedeth.c:6225:25
  index 385 is out of range for type 'u32 [385]'
  Call Trace:
   dump_stack_lvl+0x5d/0x80
   ubsan_epilogue+0x5/0x2b
   __ubsan_handle_out_of_bounds.cold+0x54/0x59
   __this_module+0xe398c/0xe9010 [forcedeth]
   pci_pm_suspend+0x80/0x170
   dpm_run_callback+0x51/0x160
   device_suspend+0x1a2/0x4a0
   ...

Both loops are hit. UBSAN reports each source location only once per module
load (__ubsan_handle_out_of_bounds() calls suppress_report(), which does
test_and_set_bit(REPORTED_BIT, ...) on the struct source_location), so the
two splats land in the first S3 cycle after the module is loaded and later
cycles are silent even though the access still runs off the end every time.
In that first cycle line 6225 is reported from pci_pm_suspend and line 6240
from pci_pm_resume.

The same off-by-one was fixed in nv_get_regs() by commit ba9aa134287f
("forcedeth: fix buffer overflow") in 2012; these two loops were missed.
The suspend and resume side was reported on LKML in September 2013 by Marc
Weber, with the same analysis and the same one-character fix, but the patch
was attached rather than sent inline and the thread ended there.

Use < instead of <=, which saves and restores exactly register_size bytes.

Fixes: 1a1ca86158ee ("[netdrvr] forcedeth: save/restore device configuration space")
Cc: stable@vger.kernel.org
Signed-off-by: Marek Czernohous <marek@czernohous.de>
Assisted-by: Claude:claude-opus-5
---
 drivers/net/ethernet/nvidia/forcedeth.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/nvidia/forcedeth.c b/drivers/net/ethernet/nvidia/forcedeth.c
index 58d3e55def48..dc804e111564 100644
--- a/drivers/net/ethernet/nvidia/forcedeth.c
+++ b/drivers/net/ethernet/nvidia/forcedeth.c
@@ -6221,7 +6221,7 @@ static int nv_suspend(struct device *device)
 	netif_device_detach(dev);
 
 	/* save non-pci configuration space */
-	for (i = 0; i <= np->register_size/sizeof(u32); i++)
+	for (i = 0; i < np->register_size/sizeof(u32); i++)
 		np->saved_config_space[i] = readl(base + i*sizeof(u32));
 
 	return 0;
@@ -6236,7 +6236,7 @@ static int nv_resume(struct device *device)
 	int i, rc = 0;
 
 	/* restore non-pci configuration space */
-	for (i = 0; i <= np->register_size/sizeof(u32); i++)
+	for (i = 0; i < np->register_size/sizeof(u32); i++)
 		writel(np->saved_config_space[i], base+i*sizeof(u32));
 
 	if (np->driver_data & DEV_NEED_MSI_FIX)
-- 
2.54.0

[PATCH net 2/2] forcedeth: stop the tx_timeout register dump past the requested window

[Marek Czernohous]

From: Marek Czernohous <marek@czernohous.de>

nv_tx_timeout() dumps the register window in rows of eight dwords:

	for (i = 0; i <= np->register_size; i += 32) {
		netdev_info(dev, "%3x: %08x ... %08x\n", i,
			    readl(base + i + 0), ..., readl(base + i + 28));

The loop bound only checks the row's starting offset, so the final row
reads a full 32 bytes from a position that is below the end of the window
but too close to it. base is mapped with exactly that length:

	np->base = ioremap(addr, np->register_size);

so the tail of that row is read from beyond the length the driver asked
for. Per variant, the last iteration reads past register_size by:

	NV_PCI_REGSZ_VER1 (0x270): row 0x260 reads to 0x27f, 16 bytes over
	NV_PCI_REGSZ_VER2 (0x2d4): row 0x2c0 reads to 0x2df, 12 bytes over
	NV_PCI_REGSZ_VER3 (0x604): row 0x600 reads to 0x61f, 28 bytes over

This happens on every supported device, not just one of them. Note that
it is not a consequence of the sizes being odd: with i <= register_size
the offending row is reached whatever the size, and a size that were a
multiple of 32 would overrun by a full row rather than by a remainder.

To be precise about the severity: the reads stay inside the BAR. Memory
BAR sizes are powers of two, the driver only accepts a region with
pci_resource_len() >= register_size (forcedeth.c:5757-5762), and the
next power of two at or above each register_size already covers the
offending row: 0x400 for 0x270 and 0x2d4, 0x800 for 0x604. ioremap()
also rounds the mapped length up to page granularity, so the reads land
inside the mapping the CPU has as well. What they leave is the window
the driver asked for, not the BAR and not the mapping. That is still a
driver reading registers it did not ask for, and it is trivial to
avoid, but nobody should expect a fault from it.

Changing <= to < is not enough: register_size is a length and every size
above is larger than its last row start, so i still reaches the offending
row. Check that the whole row fits instead.

The trade-off is that a partial trailing row is no longer dumped: 16 bytes
for VER1, 20 for VER2, 4 for VER3. That seemed preferable to reading
outside the requested window, and to open-coding a second, narrower dump
for the remainder in what is a debug-only path. Extending the dump to
cover the tail can be done on top if anyone misses those registers.

Only reachable with the debug_tx_timeout module parameter, which defaults
to false. It has not been observed at runtime: forcing a genuine TX
timeout on the reference machine is not something I can do safely, so this
rests on the arithmetic above and on a build test, not on a reproduction.
UBSAN does not catch it either, since these are MMIO reads rather than an
array access. It was found by reading the function while fixing the
saved_config_space off-by-one in nv_suspend() and nv_resume().

The dump was introduced with a fixed 0x400 bound while ioremap() mapped
only NV_PCI_REGSZ (0x270), so it read about 0x190 bytes too far from the
start. Commit 86a0f04387bf ("[PATCH] forcedeth: fix initialization")
later replaced 0x400 with np->register_size, which shrank the overrun to
the remainder but did not remove it.

Fixes: c2dba06dae7d ("[PATCH] forcedeth: rewritten tx irq handling")
Signed-off-by: Marek Czernohous <marek@czernohous.de>
Assisted-by: Claude:claude-opus-5
---
 drivers/net/ethernet/nvidia/forcedeth.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/nvidia/forcedeth.c b/drivers/net/ethernet/nvidia/forcedeth.c
index dc804e111564..f0218a0eab5c 100644
--- a/drivers/net/ethernet/nvidia/forcedeth.c
+++ b/drivers/net/ethernet/nvidia/forcedeth.c
@@ -2740,7 +2740,7 @@ static void nv_tx_timeout(struct net_device *dev, unsigned int txqueue)
 
 		netdev_info(dev, "Ring at %lx\n", (unsigned long)np->ring_addr);
 		netdev_info(dev, "Dumping tx registers\n");
-		for (i = 0; i <= np->register_size; i += 32) {
+		for (i = 0; i + 32 <= np->register_size; i += 32) {
 			netdev_info(dev,
 				    "%3x: %08x %08x %08x %08x "
 				    "%08x %08x %08x %08x\n",
-- 
2.54.0