Re: [PATCH v5 4/8] of: reserved_mem: mark /memreserve/ entries as MEMBLOCK_NODUMP

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] 64-bit FDT memreserve base and size values bypass overlap checks due to type mismatch and implicit truncation to 32-bit.
- [High] The overlap check between the memreserve region and dumpable reserved memory regions fails due to 32-bit arithmetic overflow.
--

--- Patch [4]: [PATCH v5 4/8] of: reserved_mem: mark /memreserve/ entries as MEMBLOCK_NODUMP ---
commit 224ae2a2bcff895e0e857319035a55e95d869a02
Author: Wandun Chen <chenwandun@lixiang.com>

of: reserved_mem: mark /memreserve/ entries as MEMBLOCK_NODUMP
    
The /memreserve/ entries are memory reservations made by the bootloader
or firmware, their contents are not needed for kernel crash analysis,
so mark them MEMBLOCK_NODUMP to have kdump omit them from the vmcore.

Entries that overlap a dumpable region (such as CMA) are left unmarked,
because the pages of a dumpable region may carry crash-relevant data.

> diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
> index f38d11df6f152..f0683b5d77b30 100644
> --- a/drivers/of/of_reserved_mem.c
> +++ b/drivers/of/of_reserved_mem.c
[ ... ]
> @@ -251,6 +251,38 @@ static void __init __rmem_check_for_overlap(void)
>  	}
>  }
>  
> +void __init fdt_mark_memreserve_nodump(void)
> +{
> +	u64 base, size;
> +	int n;
> +	const void *fdt = initial_boot_params;
> +
> +	if (!IS_ENABLED(CONFIG_CRASH_DUMP))
> +		return;
> +
> +	if (!fdt)
> +		return;
> +
> +	for (n = 0; ; n++) {
> +		int i;
> +
> +		if (fdt_get_mem_rsv(fdt, n, &base, &size))
> +			break;
> +		if (!size)
> +			break;
> +
> +		for (i = 0; i < reserved_mem_count; i++) {
> +			struct reserved_mem *rmem = &reserved_mem[i];
> +
> +			if (rmem->dumpable && base < rmem->base + rmem->size &&
> +			    rmem->base < base + size)

[Severity: High]
Does this overlap check suffer from a 32-bit arithmetic overflow?

Both rmem->base and rmem->size are phys_addr_t. On 32-bit systems, their
addition evaluates as a 32-bit operation. If a dumpable memory region extends
exactly to the end of the 32-bit address space, rmem->base + rmem->size could
overflow and wrap to 0.

The wrapped value 0 would then be zero-extended to u64 for the comparison
against the u64 base. The comparison base < 0ULL will always evaluate to false,
causing the overlap check to silently fail and incorrectly mark the memreserve
as MEMBLOCK_NODUMP.

> +				break;
> +		}
> +		if (i == reserved_mem_count)
> +			memblock_mark_nodump(base, size);

[Severity: High]
Could this implicitly truncate the 64-bit base and size values to 32-bit?

The base and size variables are u64, but memblock_mark_nodump() expects 32-bit
phys_addr_t parameters on 32-bit systems without PAE.

If an FDT contains a memory reservation with a base address above 4GB, the
64-bit value passes the overlap check above but gets implicitly truncated here.
This modifies the truncated 32-bit address instead, completely bypassing the
overlap check for the address actually being modified, and could erroneously
mark valid dumpable memory regions as MEMBLOCK_NODUMP.

> +	}
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260806093519.2907771-1-chenwandun1@gmail.com?part=4