Re: [PATCH v5 7/8] crash_core: replace for_each_mem_range() with for_each_mem_region()

[sashiko-bot@kernel.org]

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

New issues:
- [High] Use-after-free and wild pointer iteration in `for_each_mem_region` due to concurrent memory hotplug.

Pre-existing issues:
- [High] TOCTOU race between counting and populating memblock regions leading to an out-of-bounds write.
--

--- Patch [7]: [PATCH v5 7/8] crash_core: replace for_each_mem_range() with for_each_mem_region() ---
commit 50852c55d127f622d99b3ad7a91bdd709c53a6eb
Author: Wandun Chen <chenwandun@lixiang.com>

crash_core: replace for_each_mem_range() with for_each_mem_region()

This patch replaces the use of for_each_mem_range() with
for_each_mem_region() in the weak defaults for arch_get_system_nr_ranges()
and arch_crash_populate_cmem(). It also introduces a
crash_should_skip_region() helper to explicitly filter out MEMBLOCK_NOMAP
regions.

> diff --git a/kernel/crash_core.c b/kernel/crash_core.c
> index 77285ae3ce600..fb78b7814f188 100644
> --- a/kernel/crash_core.c
> +++ b/kernel/crash_core.c
[ ... ]
> @@ -284,22 +290,25 @@ static struct crash_mem *alloc_cmem(unsigned int nr_ranges)
>  unsigned int __weak arch_get_system_nr_ranges(void)
>  {
>  	unsigned int nr_ranges = 2 + crashk_cma_cnt; /* crashk_res + crashk_low_res, +CMA splits */
> -	phys_addr_t start, end;
> -	u64 i;
> +	struct memblock_region *reg;
>  
> -	for_each_mem_range(i, &start, &end)
> +	for_each_mem_region(reg) {
[Severity: High]
Is it possible for a concurrent memory hotplug to cause a use-after-free and
unbounded iteration here?

If sys_kexec_file_load() calls this path without holding mem_hotplug_lock, a
concurrent memory hotplug (e.g., ACPI memory add) could add a new region via
memblock_add_node().

This could trigger memblock_double_array(), which frees the old
memblock.memory.regions array.

Because the for_each_mem_region(reg) macro initialized the reg pointer once,
would it now point to freed memory? Also, if the loop condition compares
against the new array's bounds, and the new array is at a higher address,
could the loop run out of bounds?

> +		if (crash_should_skip_region(reg))
> +			continue;
>  		nr_ranges++;
> +	}
>  	return nr_ranges;
>  }
>  
>  int __weak arch_crash_populate_cmem(struct crash_mem *cmem)
>  {
> -	phys_addr_t start, end;
> -	u64 i;
> +	struct memblock_region *reg;
>  
> -	for_each_mem_range(i, &start, &end) {
> -		cmem->ranges[cmem->nr_ranges].start = start;
> -		cmem->ranges[cmem->nr_ranges].end = end - 1;
> +	for_each_mem_region(reg) {
> +		if (crash_should_skip_region(reg))
> +			continue;
> +		cmem->ranges[cmem->nr_ranges].start = reg->base;
> +		cmem->ranges[cmem->nr_ranges].end = reg->base + reg->size - 1;
>  		cmem->nr_ranges++;
[Severity: High]
This is a pre-existing issue, but can this out-of-bounds write corrupt the
heap if a region is added concurrently?

crash_prepare_headers() performs two lockless passes over the regions:

	max_nr_ranges = arch_get_system_nr_ranges();
	if (!max_nr_ranges)
		return -ENOMEM;

	cmem = alloc_cmem(max_nr_ranges);
	if (!cmem)
		return -ENOMEM;

	ret = arch_crash_populate_cmem(cmem);

If memory hotplug adds a new region concurrently between these two passes,
the region count increases. Since the array assignment here lacks bounds
checking against cmem->max_nr_ranges, would the second pass iterate more
times than the allocated capacity and overwrite adjacent memory?

>  	}
>  	return 0;

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