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

[Marek Czernohous <mczernohous@gmail.com>]

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