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

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.

[ ... ]

[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;

[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)

[ ... ]

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

+	}

[ ... ]

+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++) {

[ ... ]

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