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

[Simon Horman <horms@kernel.org>]

On Sat, Aug 15, 2026 at 09:54:38PM +0200, Marek Czernohous wrote:
> 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

Reviewed-by: Simon Horman <horms@kernel.org>