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

[Simon Horman]

On Sat, Aug 15, 2026 at 09:54:38PM +0200, Marek Czernohous wrote:
> From: Marek Czernohous <marek@xxxxxxxxxxxxx>
> 
> 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@xxxxxxxxxxxxx>
> Assisted-by: Claude:claude-opus-5

Reviewed-by: Simon Horman <horms@xxxxxxxxxx>