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 [off-list ref]

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.

[ ... ]

+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?

[ ... ]

+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?

[ ... ]

+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.

[ ... ]

+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?

[ ... ]

+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/20260810124527.1409634-1-weh@linux.microsoft.com?part=12