Re: [RFC PATCH 10/20] kho: extended scratch

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Missing error check for `kho_get_mem_map_phys()` return value
- [High] Missing static inline stub for `kho_extend_scratch()` causes build failures
- [High] 32-bit integer overflow in `kho_ext_mark_scratch()` causes incorrect physical address calculations
- [Medium] Uninitialized stack variable `radix` passed to `kho_radix_init_tree()`
- [Medium] Nested acquisition of mutexes from the same lockdep class
--

commit 69c3d241e681519286b4c5b53a9ed0d4287fb2b8
Author: Pratyush Yadav (Google) <pratyush@kernel.org>

kho: extended scratch

The scratch space is allocated by the first kernel in the KHO chain and reused.
This commit introduces extended scratch areas discovered at boot by walking the
preserved memory radix tree.

> diff --git a/include/linux/kexec_handover.h b/include/linux/kexec_handover.h
> index 8968c56d2d73e..6ce46f36ed992 100644
> --- a/include/linux/kexec_handover.h
> +++ b/include/linux/kexec_handover.h
> @@ -37,6 +37,7 @@ void kho_remove_subtree(void *blob);
>  int kho_retrieve_subtree(const char *name, phys_addr_t *phys, size_t *size);
>  
>  void kho_memory_init(void);
> +void kho_extend_scratch(void);

[Severity: High]
Does this missing static inline stub cause build failures when
CONFIG_KEXEC_HANDOVER=n?

kho_extend_scratch() is called unconditionally from mm_core_init_early().
However, its prototype here is only declared when CONFIG_KEXEC_HANDOVER is
enabled. Compiling the kernel with CONFIG_KEXEC_HANDOVER=n will result in an
implicit function declaration error.

[ ... ]

> diff --git a/kernel/liveupdate/kexec_handover.c b/kernel/liveupdate/kexec_handover.c
> index b2d1572808ebf..a006a883ee94a 100644
> --- a/kernel/liveupdate/kexec_handover.c
> +++ b/kernel/liveupdate/kexec_handover.c
[ ... ]
> @@ -825,6 +842,120 @@ static void __init kho_reserve_scratch(void)
>  	kho_enable = false;
>  }
>  
> +#define KHO_EXT_SHIFT 30 /* 1 GiB */
> +
> +static int __init kho_ext_walk_key(unsigned long key, void *data)
> +{
> +	struct kho_radix_tree *tree = data;
> +	phys_addr_t start, end;
> +	unsigned int order;
> +	int err;
> +
> +	start = kho_decode_radix_key(key, &order);
> +	end = start + (1UL << (order + PAGE_SHIFT));
> +
> +	while (start < end) {
> +		err = kho_radix_add_key(tree, start >> KHO_EXT_SHIFT);

[Severity: Medium]
Will this trigger a lockdep warning due to nested acquisition of mutexes from
the same lockdep class?

When kho_radix_walk_tree() is called on kho_in.radix_tree, it acquires its
lock and then calls this walk callback. This callback calls kho_radix_add_key()
on the new radix tree, which acquires its lock. Because both locks are
initialized by kho_radix_init_tree(), they share the same lockdep class.
Acquiring a second lock of the same class without using mutex_lock_nested()
can trigger a "possible recursive locking detected" lockdep warning.

> +		if (err)
> +			return err;
> +
> +		start += (1UL << KHO_EXT_SHIFT);
> +	}
> +
> +	return 0;
> +}

[ ... ]

> +static int __init kho_ext_mark_scratch(unsigned long key, void *data)
> +{
> +	phys_addr_t *prev_end = data;
> +	phys_addr_t start = key << KHO_EXT_SHIFT;

[Severity: High]
Can this cause a 32-bit integer overflow on 32-bit architectures?

The key variable is of type unsigned long, which is 32-bit on 32-bit
architectures. Since KHO_EXT_SHIFT is 30, any key >= 4 (representing memory at
or above 4 GiB) will overflow the 32-bit shift and result in a truncated
address. Should key be cast to phys_addr_t before shifting?

> +	int err;
> +
> +	if (start > *prev_end) {
> +		err = memblock_mark_kho_scratch_ext(*prev_end, start - *prev_end);
> +		if (err)
> +			return err;
> +	}
> +
> +	*prev_end = start + (1UL << KHO_EXT_SHIFT);
> +	return 0;
> +}

[ ... ]

> +void __init kho_extend_scratch(void)
> +{
> +	const struct kho_radix_walk_cb kho_cb = {
> +		.key = kho_ext_walk_key,
> +		.table = kho_ext_walk_table,
> +	};
> +	const struct kho_radix_walk_cb ext_cb = {
> +		.key = kho_ext_mark_scratch,
> +	};
> +	struct kho_radix_tree radix;

[Severity: Medium]
Is it safe to pass this uninitialized stack variable radix to
kho_radix_init_tree()?

It is allocated on the stack but not zero-initialized. If kho_radix_init_tree()
conditionally initializes its fields (e.g., checking if tree->root is non-NULL
to avoid double initialization for global trees), it could read garbage stack
memory and return without initializing radix.lock. Subsequent operations would
then dereference uninitialized pointers.

> +	phys_addr_t prev_end = 0, mem_map_phys;
> +	int err = 0;
> +
> +	if (!is_kho_boot())
> +		return;
> +
> +	/* Make sure the KHO radix tree is initialized. */
> +	mem_map_phys = kho_get_mem_map_phys(kho_get_fdt());
> +	err = kho_radix_init_tree(&kho_in.radix_tree, phys_to_virt(mem_map_phys));

[Severity: High]
Does this code miss checking the return value of kho_get_mem_map_phys()?

kho_get_mem_map_phys() returns 0 on failure (e.g., missing FDT property),
and this return value is passed directly to phys_to_virt() without validation.
If the map is missing, the code will initialize the KHO radix tree using
physical address 0x0 as the root, leading to arbitrary memory being parsed
as tree nodes, which can cause memory corruption or a kernel panic during
early boot.

> +	if (err)
> +		goto print;
> +
> +	err = kho_radix_init_tree(&radix, NULL);
> +	if (err)
> +		goto print;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260528004204.1484584-1-jloeser@linux.microsoft.com?part=10