Re: [PATCH v14 07/17] riscv: kexec_file: Fix potential buffer overflow in prepare_elf_headers()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] `get_online_mems()` does not serialize `walk_system_ram_res()` against memory hotplug, failing to eliminate the TOCTOU race.
- [High] Reachable TOCTOU race condition inappropriately triggers `WARN_ON_ONCE`, allowing intentional dmesg spam and potential panics.
- [Medium] The commit message fabricates a non-existent deadlock scenario in RISC-V to justify the lock's placement.
--

commit 223e3ce35bfbaf2615822d5aa0bcebc0234cf741
Author: Jinjie Ruan <ruanjinjie@huawei.com>

riscv: kexec_file: Fix potential buffer overflow in prepare_elf_headers()

This commit attempts to fix a TOCTOU race in prepare_elf_headers() by
moving get_online_mems() to the top-level architecture image loaders,
and adding a bounds check in prepare_elf64_ram_headers_callback().

> Directly introducing get_online_mems() inside prepare_elf_headers() would
> trigger an immediate recursive read-after-write deadlock when invoked by
> the runtime hotplug notification path (which already holds the hotplug
> write lock).
> 
> To eliminate the TOCTOU window safely without deadlock risks, move the
> get_online_mems() read lock to the top-level architecture image loaders.
> Since these top-level loaders are strictly executed on the initial system
> call path and are never re-entered by the runtime hotplug notifier, this
> approach physically isolates the locking contexts. The system memory ranges
> are forced to be statically frozen during the entire layout generation,
> eradicating the buffer overflow vulnerability.

[Severity: Medium]
Is this deadlock scenario accurate?

RISC-V currently does not select CONFIG_CRASH_HOTPLUG or
ARCH_SUPPORTS_CRASH_HOTPLUG. This means there is no runtime hotplug
notification path that dynamically updates kexec images or calls
prepare_elf_headers() on RISC-V.

Could the commit message be updated to clarify what this lock placement
actually protects against, given that the described runtime hotplug
notification path doesn't apply to this architecture?

> diff --git a/arch/riscv/kernel/machine_kexec_file.c b/arch/riscv/kernel/machine_kexec_file.c
> index 3f7766057cac3..a88362dfbd893 100644
> --- a/arch/riscv/kernel/machine_kexec_file.c
> +++ b/arch/riscv/kernel/machine_kexec_file.c
[ ... ]
> @@ -48,6 +49,9 @@ static int prepare_elf64_ram_headers_callback(struct resource *res, void *arg)
>  {
>  	struct crash_mem *cmem = arg;
>  
> +	if (WARN_ON_ONCE(cmem->nr_ranges >= cmem->max_nr_ranges))
> +		return -EAGAIN;

[Severity: High]
Is it safe to use a WARN_ON_ONCE() here?

Because get_online_mems() (added below) fails to prevent the Time-of-Check to
Time-of-Use race, a concurrent memory hot-add can legitimately cause
cmem->nr_ranges >= cmem->max_nr_ranges.

By repeatedly invoking the kexec_file_load syscall concurrently with memory
hotplug, a privileged user could intentionally hit this branch. This allows
them to pollute dmesg with stack traces or potentially trigger a kernel
panic on systems configured with panic_on_warn=1.

> +
>  	cmem->ranges[cmem->nr_ranges].start = res->start;
>  	cmem->ranges[cmem->nr_ranges].end = res->end;
>  	cmem->nr_ranges++;
[ ... ]
> @@ -281,12 +285,15 @@ int load_extra_segments(struct kimage *image, unsigned long kernel_start,
>  	if (image->type == KEXEC_TYPE_CRASH) {
>  		void *headers;
>  		unsigned long headers_sz;
> +		get_online_mems();
>  		ret = prepare_elf_headers(&headers, &headers_sz);

[Severity: High]
Does get_online_mems() properly serialize walk_system_ram_res() against
memory hotplug?

get_online_mems() only acquires the mem_hotplug_lock as a reader. During
memory hotplug, __add_memory() calls register_memory_resource() to add the
new memory range to the iomem_resource tree (temporarily acquiring
resource_lock) before it calls add_memory_resource() to acquire the
mem_hotplug_lock as a writer.

Because walk_system_ram_res() iterates over the iomem_resource tree, it will
see newly added memory regions even while get_online_mems() is held.

Does this render the intended synchronization ineffective and leave the
Time-of-Check to Time-of-Use window open?

>  		if (ret) {
> +			put_online_mems();
>  			pr_err("Preparing elf core header failed\n");
>  			goto out;
>  		}
>  
> +		put_online_mems();
>  		kbuf.buffer = headers;
>  		kbuf.bufsz = headers_sz;
>  		kbuf.mem = KEXEC_BUF_MEM_UNKNOWN;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260525084932.934910-1-ruanjinjie@huawei.com?part=7