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

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [Critical] Secure Boot bypass and arbitrary memory overwrite via `hyperv_resvd=` early parameter.
- [High] Uninitialized stack variable `ret` used across trust boundary in `mshv_launch`.
- [High] EFI stub passes unmerged hypervisor memory ranges to the kernel, overflowing command-line and parser limits.
- [High] Logic error in `hv_parse_hyperv_resvd_new` rejects valid physical address 0.
- [Medium] Incorrect variadic argument forwarding in `mshv_efi_reboot`.
- [Medium] State corruption in `hv_parse_hyperv_resvd_new` overwriting head without clearing the tail.
- [Low] Incomplete implementation/Dead code: `SETUP_MSHV` is built but never parsed.
--

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

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

Port the EFI HvLoader protocol handshake (efi-mshv stub + SETUP_MSHV) and
the hyperv_resvd_new memory reservation so an upstream kernel can boot as
a Microsoft Hypervisor (MSHV) root partition.

> diff --git a/arch/x86/kernel/cpu/mshyperv.c b/arch/x86/kernel/cpu/mshyperv.c
> index 185d4f677ec0f..63504b22b5ba8 100644
> --- a/arch/x86/kernel/cpu/mshyperv.c
> +++ b/arch/x86/kernel/cpu/mshyperv.c
[ ... ]
> +static int __init hv_parse_hyperv_resvd(char *arg)
> +{
> +	int idx, max = ARRAY_SIZE(hv_resvd_ranges);
> +	int i = 0;
[ ... ]
> +	while (get_option(&arg, &idx)) {
> +		if (i >= max) {
> +			pr_err("Hyper-V: resvd ranges tbl full %d\n", idx);
> +			break;
> +		}
> +
> +		hv_resvd_ranges[i++] = idx;
> +	}
> +
> +	return 0;
> +}
> +early_param("hyperv_resvd", hv_parse_hyperv_resvd);
> +
> +/*
> + * Reserve memory that the hypervisor is using early on. The ranges are marked
> + * reserved by a custom bootloader, change that to usable and reserve that
> + * range. Note, the bootloader sanitizes the e820 before passing on here.
> + */
> +static void __init hv_resv_mshv_memory(void)
> +{
> +	u64 start, end, size;
> +	int i, idx, max = ARRAY_SIZE(hv_resvd_ranges);
> +	int resource_index = 0;
> +
> +	for (i = 0; i < max && hv_resvd_ranges[i] != -1; i++) {
> +		idx = hv_resvd_ranges[i];
[ ... ]
> +		start = e820_table->entries[idx].addr;
> +		size = e820_table->entries[idx].size;
> +		end = start + size - 1;
> +
> +		memblock_reserve(start, size);
> +		e820_table->entries[idx].type = E820_TYPE_RAM;
> +		pr_info("Hyper-V reserve [mem %#018Lx-%#018Lx]\n", start, end);
> +
> +		hv_mshv_res[resource_index].name = "Hypervisor Code and Data";
> +		hv_mshv_res[resource_index].flags = IORESOURCE_BUSY | IORESOURCE_SYSTEM_RAM;
> +		hv_mshv_res[resource_index].start = start;
> +		hv_mshv_res[resource_index++].end = end;
> +	}
> +}

[Severity: Critical]
Does this allow bypassing kernel lockdown?

The hv_parse_hyperv_resvd() function takes an unauthenticated index from the
kernel command line. Then hv_resv_mshv_memory() blindly converts that e820
entry to E820_TYPE_RAM and IORESOURCE_SYSTEM_RAM without bounds checks.

Could a user with access to the bootloader command line use this to target
reserved ACPI or firmware regions, map them as System RAM, and overwrite them
via kexec_file_load?

> diff --git a/drivers/firmware/efi/libstub/efi-mshv-common.c b/drivers/firmware/efi/libstub/efi-mshv-common.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/firmware/efi/libstub/efi-mshv-common.c
[ ... ]
> +efi_status_t mshv_launch(void)
> +{
> +	struct hvl_return_data ret;
> +
> +	if (!efi_mshv)
> +		return EFI_SUCCESS;
> +
> +	efi_mshv->launch_hv(NULL, &ret);
> +	/* TODO: Where/how do we dump the hv loader logs? */
> +	if (ret.launch_data.launch_status != 0)
> +		efi_rt_call(reset_system, EFI_RESET_COLD, EFI_ABORTED, 0, NULL);
> +	return EFI_SUCCESS;
> +}

[Severity: High]
Can this result in evaluating uninitialized stack memory?

The ret structure is allocated on the stack but not zeroed before being
passed to efi_mshv->launch_hv(). If the hypervisor loader protocol fails
early or neglects to fully populate the structure, does checking
ret.launch_data.launch_status read stack garbage and trigger an unexpected
cold reboot?

> diff --git a/drivers/firmware/efi/libstub/efi-mshv.h b/drivers/firmware/efi/libstub/efi-mshv.h
> new file mode 100644
> --- /dev/null
> +++ b/drivers/firmware/efi/libstub/efi-mshv.h
[ ... ]
> +static inline void mshv_efi_reboot(const char *fmt, ...)
> +{
> +	va_list args;
> +
> +	va_start(args, fmt);
> +	efi_printk(fmt, args);
> +	va_end(args);
> +
> +	efi_bs_call(stall, 5 * EFI_USEC_PER_SEC);
> +	efi_rt_call(reset_system, EFI_RESET_COLD, EFI_ABORTED, 0, NULL);
> +}

[Severity: Medium]
Is args correctly passed to efi_printk() here?

efi_printk() is a variadic function rather than a vprintf-style function.
Passing a va_list directly to it as the first argument will cause it to
interpret the va_list structure itself as the format argument, which prints
garbage memory values instead of the actual error codes.

> diff --git a/drivers/firmware/efi/libstub/x86-efi-mshv.c b/drivers/firmware/efi/libstub/x86-efi-mshv.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/firmware/efi/libstub/x86-efi-mshv.c
[ ... ]
> +efi_status_t mshv_efi_setup(struct boot_params *boot_params)
> +{
[ ... ]
> +	for (i = 0, nr_ranges = 0; i < nr_desc; i++) {
[ ... ]
> +		mshv_range->name = "Hypervisor Code and Data";
> +		mshv_range->flags = IORESOURCE_BUSY | IORESOURCE_SYSTEM_RAM;
> +		mshv_range->start = d->phys_addr;
> +		mshv_range->end = d->phys_addr + (d->num_pages << PAGE_SHIFT) - 1;
> +
> +		prev = mshv_range++;
> +		nr_ranges++;
[ ... ]
> +	}
> +
> +	status = mshv_populate_ranges(boot_params, mem_map, map_sz, desc_sz);

[Severity: High]
Does this pass the unmerged EFI memory map to the command line?

The code successfully merges adjacent EFI descriptors into the mshv_reserved
array above, but then passes the raw, unmerged mem_map array into
mshv_populate_ranges().

Since mshv_efi_update_cmdline() serializes every descriptor, could this
exceed the 32-range limit (HV_MAX_RESVD_RANGES) in hv_parse_hyperv_resvd_new()
if the unmerged map is highly fragmented, causing the kernel to silently
drop the remaining ranges?

> +	if (status != EFI_SUCCESS)
> +		mshv_efi_reboot("failed to update command line with code %d",
> +				status);
> +
> +	/* Build an indirect setup_data for each mshv reserved range. */
> +	status = efi_bs_call(allocate_pool, EFI_LOADER_DATA,
> +			     nr_ranges * sizeof(struct mshv_setup_data),
> +				(void **)&sd_block);
> +	if (status != EFI_SUCCESS)
> +		mshv_efi_reboot("failed to allocate space for hv ranges: error code %d", status);
[ ... ]
> +	for (i = 0; i < nr_ranges; i++) {
> +		start = mshv_reserved[i].start;
> +		end = mshv_reserved[i].end;
> +
> +		sd_block[i].sd.type = SETUP_INDIRECT;
> +		sd_block[i].sd.len  = sizeof(struct setup_indirect);
> +		sd_block[i].sd.next = (__u64)&sd_block[i + 1];
> +
> +		sd_block[i].si.type = SETUP_MSHV;
> +		sd_block[i].si.reserved = 0;
> +		sd_block[i].si.len = end - start + 1;
> +		sd_block[i].si.addr = start;
> +	}

[Severity: Low]
Is the SETUP_MSHV data parsed by the kernel later?

The stub builds a SETUP_INDIRECT chain wrapping SETUP_MSHV elements, but it
doesn't look like there is any logic added in the kernel to parse SETUP_MSHV.
Does this leave unused structures in boot memory?

> diff --git a/drivers/hv/hv_common.c b/drivers/hv/hv_common.c
> index 6b67ac6167891..ab301dd602fbc 100644
> --- a/drivers/hv/hv_common.c
> +++ b/drivers/hv/hv_common.c
[ ... ]
> +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 (kstrtoull(range, 16, &region_sz) || !region_sz ||
> +		    kstrtoull(addr_str, 16, &region_start) || !region_start) {

[Severity: High]
Does the !region_start check reject physical address 0?

If the hypervisor reserves a range starting at the beginning of physical
address space (page 0), region_start will be 0. This check will treat it
as a validation failure and break out of the parsing loop.

> +			pr_err("Hyper-V: invalid format for hyperv_resvd_new\n");
> +			break;
> +		}
> +
> +		memblock_reserve(region_start, region_sz);
> +
> +		hv_mshv_res[i].name = "Hypervisor Code and Data";
> +		hv_mshv_res[i].flags = IORESOURCE_BUSY | IORESOURCE_SYSTEM_RAM;
> +		hv_mshv_res[i].start = region_start;
> +		hv_mshv_res[i].end = region_start + region_sz - 1;
> +
> +		++i;
> +	}
> +
> +	ranges_nr = i;
> +
> +	return 0;
> +}
> +early_param("hyperv_resvd_new", hv_parse_hyperv_resvd_new);
[ ... ]
> +void __init hv_mark_resources(void)
> +{
> +	int i, max = ARRAY_SIZE(hv_mshv_res);
> +
> +	for (i = 0; i < max && hv_mshv_res[i].end; i++)

[Severity: Medium]
Can stale entries in hv_mshv_res be read during resource marking?

If hv_parse_hyperv_resvd_new() is invoked multiple times (e.g. appended by
bootloader and supplied by user), the local index i resets to 0, overwriting
the head of hv_mshv_res without clearing the tail.

Because hv_mark_resources() iterates until hv_mshv_res[i].end == 0 rather
than using ranges_nr, it will process the remaining stale tail entries from
the previous parse.

> +		insert_resource(&iomem_resource, &hv_mshv_res[i]);
> +}

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