Re: [PATCH v5 6/8] crash_core: fold duplicated memblock arch hooks into the weak default

[sashiko-bot@kernel.org]

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

New issues:
- [Low] Leftover unused `#include <linux/memblock.h>` in `arch/riscv/kernel/machine_kexec_file.c`.

Pre-existing issues:
- [High] A time-of-check to time-of-use (TOCTOU) race condition in `crash_prepare_headers()` allows an out-of-bounds array write and Use-After-Free (UAF) during concurrent memory hotplug.
--

--- Patch [6]: [PATCH v5 6/8] crash_core: fold duplicated memblock arch hooks into the weak default ---
commit 1003e078862f2080b60e5f21352f7989a4d125c2
Author: Wandun Chen <chenwandun@lixiang.com>

crash_core: fold duplicated memblock arch hooks into the weak default

arm64, loongarch and riscv open-code the same memblock walk in
arch_get_system_nr_ranges() and arch_crash_populate_cmem(). Move it into
the __weak defaults in kernel/crash_core.c and delete the arch copies.

> diff --git a/arch/riscv/kernel/machine_kexec_file.c b/arch/riscv/kernel/machine_kexec_file.c
> index 808e872f40634..fdc6695e85882 100644
> --- a/arch/riscv/kernel/machine_kexec_file.c
> +++ b/arch/riscv/kernel/machine_kexec_file.c
> @@ -37,32 +37,6 @@ int arch_kimage_file_post_load_cleanup(struct kimage *image)
>  }
>  
>  #ifdef CONFIG_CRASH_DUMP

[Severity: Low]
This isn't a bug, but can we also drop the #include <linux/memblock.h>
from arch/riscv/kernel/machine_kexec_file.c?

The patch successfully relocates all memblock usage out of the riscv file,
and the include was correctly dropped for arm64 and loongarch, but it looks
like it was missed here.

> -unsigned int arch_get_system_nr_ranges(void)
> -{

[ ... ]

> diff --git a/kernel/crash_core.c b/kernel/crash_core.c
> index 2b36aa9fade06..77285ae3ce600 100644
> --- a/kernel/crash_core.c
> +++ b/kernel/crash_core.c

[ ... ]

> +int __weak arch_crash_populate_cmem(struct crash_mem *cmem)
> +{
> +	phys_addr_t start, end;
> +	u64 i;
> +
> +	for_each_mem_range(i, &start, &end) {
> +		cmem->ranges[cmem->nr_ranges].start = start;
> +		cmem->ranges[cmem->nr_ranges].end = end - 1;
> +		cmem->nr_ranges++;
> +	}

[Severity: High]
This is a pre-existing issue, but does this loop risk an out-of-bounds array
write during concurrent memory hotplug?

The loop unconditionally writes to cmem->ranges and increments the index
without verifying bounds against cmem->max_nr_ranges.

In crash_prepare_headers(), the code sequentially counts the regions,
allocates memory, and then calls arch_crash_populate_cmem():

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

Since neither kexec_file_load nor these functions hold the memory hotplug
read lock (get_online_mems()), couldn't memblock.memory be modified
concurrently between the check and the use? If regions are added or split,
this loop would write past the allocated array size.

> +	return 0;
> +}
>  int __weak arch_crash_exclude_ranges(struct crash_mem *cmem) { return 0; }

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