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

[Zhu Yanjun <yanjun.zhu@linux.dev>]

在 2026/8/15 12:54, 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>

Thanks a lot.

Reviewed-by: Zhu Yanjun <yanjun.zhu@linux.dev>

Zhu Yanjun

> 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)

-- 
Best Regards,
Yanjun.Zhu