Re: [PATCH v1 12/13] hyperv: add MSHV Dom0 root-partition boot enablement (EFI HvLoader)

Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
- [Critical] The x86 EFI stub stores the command line in a `.bss` static buffer which is wiped by the kernel decompressor.
- [High] Unconditional increment of `cmdline_len` with `snprintf` return value leads to buffer overflow.
- [High] Incorrect linked list traversal causes truncation of the existing `setup_data` chain.
- [Medium] Missing NULL pointer check before calling `strlen` on the command line.
- [Medium] Skipping invalid e820 indices leaves uninitialized holes in `hv_mshv_res`, breaking resource registration.
- [Medium] MSHV root partition boot enablement is completely unreachable on ARM64.
- [Low] Memory leak of `mshv_reserved` array in x86 EFI stub.
- [Low] Memory leak of `mem_map` in ARM64 EFI stub.
- [Low] Parsing error in `hyperv_resvd_new` bypasses state updates, silencing previous valid ranges.
--

--- Patch [12]: [PATCH v1 12/13] hyperv: add MSHV Dom0 root-partition boot enablement (EFI HvLoader) ---
commit fcbf57de7557f7a5bf335c4003de65861d33a7c5
Author: Wei Hu <[email protected]>

hyperv: add MSHV Dom0 root-partition boot enablement (EFI HvLoader)

This commit ports the EFI HvLoader protocol handshake and the hyperv_resvd_new
memory reservation. It provides the Linux-side enablement necessary to boot an
upstream kernel as a Microsoft Hypervisor (MSHV) root partition.

> --- a/arch/x86/kernel/cpu/mshyperv.c
> +++ b/arch/x86/kernel/cpu/mshyperv.c
[ ... ]
> @@ -496,6 +499,75 @@ EXPORT_SYMBOL_GPL(hv_get_hypervisor_version);
> +static void __init hv_resv_mshv_memory(void)
> +{
> +	u64 start, end, size;
> +	int i, idx, max = ARRAY_SIZE(hv_resvd_ranges);
> +
> +	for (i = 0; i < max && hv_resvd_ranges[i] != -1; i++) {
> +		idx = hv_resvd_ranges[i];
> +		if (idx < 0 || idx >= e820_table->nr_entries) {
> +			pr_info("Hyper-V: invalid resvd idx %d\n", idx);
> +			continue;

[Severity: Medium]
Does skipping the invalid index here leave the hv_mshv_res array
zero-initialized for this slot?

Later on, hv_mark_resources() iterates over this array using:
    for (i = 0; i < max && hv_mshv_res[i].end; i++)

Could this cause the registration loop to terminate prematurely when it hits
the hole, silently dropping all subsequent valid memory ranges?

> +		}
> +
> +		start = e820_table->entries[idx].addr;

> --- /dev/null
> +++ b/drivers/firmware/efi/libstub/arm64-efi-mshv.c
> @@ -0,0 +1,41 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include <linux/efi.h>
> +#include <asm/efi.h>
> +#include <asm/setup.h>
> +
> +#include "efistub.h"
> +#include "efi-mshv.h"
> +
> +efi_status_t mshv_efi_setup(char **cmdline_ptr)
> +{

[Severity: Medium]
Is this function ever invoked? It appears to be defined here but never hooked
into the ARM64 EFI stub entry point, making the ARM64 MSHV boot enablement
unreachable code.

> +	efi_status_t status;
> +	efi_memory_desc_t *mem_map;
> +	unsigned long map_sz, desc_sz, new_cmdline_addr;
> +
> +	status = mshv_efi_init();
> +	if (status == EFI_NOT_FOUND) // we are in a standard Linux boot
> +		return EFI_SUCCESS;
> +
> +	map_sz = 0;
> +	mshv_get_hv_ranges((void *)&mem_map, &map_sz, &desc_sz);
> +
> +	status = efi_bs_call(allocate_pool, EFI_LOADER_DATA, COMMAND_LINE_SIZE,
> +			     (void **)&new_cmdline_addr);

[Severity: Low]
Does this code leak the mem_map pool allocation? Unlike the x86 implementation
in this patch, it doesn't appear to be freed before returning EFI_SUCCESS.

> +	if (status != EFI_SUCCESS)

> --- /dev/null
> +++ b/drivers/firmware/efi/libstub/efi-mshv-common.c
[ ... ]
> +void mshv_efi_update_cmdline(efi_memory_desc_t *mem_map,
> +			     unsigned long map_sz, unsigned long desc_sz, char *old_cmdline,
> +		char *buf, unsigned long buf_sz)
> +{
> +	int i, cmdline_len, nr_desc;
> +
> +	cmdline_len = strlen(old_cmdline);

[Severity: Medium]
Can old_cmdline be NULL if the kernel is booted without a command line?
If so, passing it directly to strlen() would lead to a NULL pointer
dereference.

> +	memcpy(buf, old_cmdline, cmdline_len + 1);
> +
> +	cmdline_len += snprintf(buf + cmdline_len,
> +				buf_sz - cmdline_len,
> +				" hyperv_resvd_new=");
> +
> +	nr_desc = map_sz / desc_sz;
> +	for (i = 0; i < nr_desc; ++i) {
> +		efi_memory_desc_t *d;
> +		u64 start, end, sz;
> +
> +		d = efi_memdesc_ptr(mem_map, desc_sz, i);
> +		start = d->phys_addr;
> +		sz = d->num_pages << PAGE_SHIFT;
> +		end = start + sz - 1;
> +
> +		cmdline_len += snprintf(buf + cmdline_len,
> +					buf_sz - cmdline_len,
> +					"%s0x%llx!0x%llx", i > 0 ? "," : "",
> +					sz, start);

[Severity: High]
Could this result in a buffer overflow?

If the appended string exceeds buf_sz, snprintf returns the number of
characters that would have been written, not the number actually written. 
Adding this value to cmdline_len could cause buf_sz - cmdline_len to underflow
into a very large unsigned integer.

Will the next loop iteration then write out of bounds via buf + cmdline_len?

> +	}

> --- /dev/null
> +++ b/drivers/firmware/efi/libstub/x86-efi-mshv.c
[ ... ]
> +static efi_status_t mshv_populate_ranges(struct boot_params *boot_params,
> +					 efi_memory_desc_t *mem_map, unsigned long map_sz,
> +			unsigned long desc_sz)
> +{
> +	unsigned long cmdline_ptr;
> +	u32 cmdline_size;
> +	static u8 mshv_cmdline[COMMAND_LINE_SIZE];

[Severity: Critical]
Will this static buffer be placed in the .bss segment and subsequently
destroyed?

When the EFI stub hands over control to the kernel decompressor
(e.g. startup_64), the decompressor zeroes its .bss segment before the command
line is parsed. Using a static buffer here might cause the entire kernel
command line to be lost.

> +
> +	memset(mshv_cmdline, 0, sizeof(mshv_cmdline));
> +
> +	cmdline_ptr = boot_params->hdr.cmd_line_ptr;
> +	cmdline_ptr |= (u64)boot_params->ext_cmd_line_ptr << 32;
> +	cmdline_size = boot_params->hdr.cmdline_size;
> +
> +	mshv_efi_update_cmdline(mem_map, map_sz, desc_sz,
> +				(char *)cmdline_ptr,
> +				(char *)mshv_cmdline, COMMAND_LINE_SIZE);
> +
> +	boot_params->hdr.cmd_line_ptr = (u32)((unsigned long)mshv_cmdline);
[ ... ]
> +efi_status_t mshv_efi_setup(struct boot_params *boot_params)
> +{
[ ... ]
> +	/*
> +	 * Build an array of kernel 'struct resource' objects that contain mshv
> +	 * reserved ranges. This array is populated via a command line parameter
> +	 * called 'hyperv_resvd_new'.
> +	 */
> +
> +	status = mshv_realloc_ranges(&mshv_reserved,
> +				     &mshv_reserved_sz,
> +				MSHV_RESERVED_RANGES_COUNT);

[Severity: Low]
Does this code leak the mshv_reserved array memory? It is dynamically
allocated here and possibly reallocated during the loop, but it doesn't appear
to be freed at the end of the function.

[ ... ]
> +	setup_data_itr = (struct setup_data **)&boot_params->hdr.setup_data;
> +
> +	while (*setup_data_itr && (*setup_data_itr)->next)
> +		setup_data_itr = (struct setup_data **)&(*setup_data_itr)->next;
> +
> +	*setup_data_itr = (struct setup_data *)sd_block;

[Severity: High]
Does this loop terminate one node early?

Because it checks (*setup_data_itr)->next in the while condition, it exits
when pointing to the last node. Overwriting *setup_data_itr here seems to
overwrite the pointer to the last element rather than appending to it, which
could drop critical boot parameters passed by the bootloader.

> +
> +	for (i = 0; i < nr_ranges; i++) {

> --- a/drivers/hv/hv_common.c
> +++ b/drivers/hv/hv_common.c
[ ... ]
> @@ -863,3 +867,81 @@ const char *hv_result_to_string(u64 status)
> +static int __init hv_parse_hyperv_resvd_new(char *arg)
> +{
> +	int i = 0;
> +
> +	if (is_kdump_kernel())
> +		return 0;
> +
> +	while (arg && *arg) {
> +		unsigned long long region_start, region_sz;
> +		char *range, *addr_str;
> +
> +		if (i >= HV_MAX_RESVD_RANGES) {
> +			pr_err("Hyper-V: too many hyperv_resvd_new ranges specified\n");
> +			return 0;
> +		}
> +
> +		/* Each range is "<size>!<address>", ranges separated by ','. */
> +		range = strsep(&arg, ",");
> +		addr_str = strchr(range, '!');
> +		if (!addr_str) {
> +			pr_err("Hyper-V: invalid format for hyperv_resvd_new: %s\n", range);
> +			return 0;

[Severity: Low]
If an invalid format is encountered, does this early return bypass updating
the global ranges_nr counter?

By returning early instead of breaking the loop, ranges_nr = i is skipped,
which might cause any previously parsed valid memory ranges to be silently
ignored by hv_dump_mshv_memory() later.

> +		}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=12