Re: [PATCH v5 5/8] riscv: build crash_mem ranges from memblock instead of resource tree

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Lockless iteration over memblock regions introduces a Use-After-Free against concurrent memory hotplug.
- [Medium] for_each_mem_range() fails to filter MEMBLOCK_NODUMP regions, breaking the patch's core feature.

Pre-existing issues:
- [High] Time-of-Check to Time-of-Use (TOCTOU) race condition during lockless double-iteration causes an out-of-bounds array write.
--

--- Patch [5]: [PATCH v5 5/8] riscv: build crash_mem ranges from memblock instead of resource tree ---
commit 6f60d7eb990c57ea1aef75ef70d369b6c9300b5a
Author: Wandun Chen <chenwandun@lixiang.com>

riscv: build crash_mem ranges from memblock instead of resource tree

This commit replaces walk_system_ram_res() with for_each_mem_range() to build
crash_mem ranges from memblock. It also updates the ARCH_KEEP_MEMBLOCK
config to keep memblock around when kexec file and crash dump are enabled,
allowing the exclusion of MEMBLOCK_NODUMP regions from the vmcore.

> diff --git a/arch/riscv/kernel/machine_kexec_file.c b/arch/riscv/kernel/machine_kexec_file.c
> index 26cd2a8bd0cd3..808e872f40634 100644
> --- a/arch/riscv/kernel/machine_kexec_file.c
> +++ b/arch/riscv/kernel/machine_kexec_file.c
[ ... ]
>  unsigned int arch_get_system_nr_ranges(void)
>  {
>  	unsigned int nr_ranges = 2 + crashk_cma_cnt; /* For exclusion of crashkernel region */
> +	u64 i;
> +	phys_addr_t start, end;
>  
> -	walk_system_ram_res(0, -1, &nr_ranges, get_nr_ram_ranges_callback);
> +	for_each_mem_range(i, &start, &end)
> +		nr_ranges++;

[Severity: High]
This isn't a bug introduced by this patch, but by performing a lockless count
here and then populating the array in arch_crash_populate_cmem() later,
couldn't a concurrent memory hotplug add regions in between the two calls?

If memblock regions are added after the cmem array is allocated but before it
is populated, the second loop will yield more regions than were counted,
leading to an out-of-bounds write in arch_crash_populate_cmem() since there
are no bounds checks on cmem->nr_ranges.

>  
>  	return nr_ranges;
>  }
>  
[ ... ]
> -int arch_crash_populate_cmem(struct crash_mem *cmem)
> -{
> -	return walk_system_ram_res(0, -1, cmem, prepare_elf64_ram_headers_callback);
> -}
> -
> +int arch_crash_populate_cmem(struct crash_mem *cmem)
> +{
> +	u64 i;
> +	phys_addr_t start, end;
>  
> +	for_each_mem_range(i, &start, &end) {

[Severity: Medium]
The commit message mentions that "only by walking memblock can the
reserved-memory regions marked MEMBLOCK_NODUMP be excluded". Does
for_each_mem_range() actually filter out MEMBLOCK_NODUMP regions?

Looking at for_each_mem_range(), it delegates to should_skip_region(), which
doesn't appear to have any logic to filter MEMBLOCK_NODUMP. Will this loop
end up blindly yielding all regions and failing to exclude the targeted memory
from the vmcore?

[Severity: High]
Since for_each_mem_range() iterates over memblock.memory.regions locklessly,
is it safe against concurrent memory hotplug?

If a memory hotplug operation runs concurrently and adds new memory regions,
memblock_add_node() might exceed the current region limit and trigger
memblock_double_array(). This allocates a new array and frees the old one,
potentially causing a use-after-free if this lockless iteration is still
accessing the freed array.

> +		cmem->ranges[cmem->nr_ranges].start = start;
> +		cmem->ranges[cmem->nr_ranges].end = end - 1;
> +		cmem->nr_ranges++;
> +	}
>  
>  	return 0;
>  }

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