Re: [RFC PATCH 04/12] firmware: efi: libstub: x86-stub: Enable VSM awareness in efi os indications variable

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Uninitialized stack memory leak to NVRAM and unintended EFI variable size modification.
- [Medium] Unconditional execution of `efi_set_hv_os_indications()` causes an EFI error message on all non-Hyper-V systems.
- [Medium] `efi_set_hv_os_indications()` fails to enable VSM if `OsLoaderIndications` does not already exist.
- [Low] Missing newline character in `efi_info()` output.
--

commit 3d285ae76c7e0c1fdff6a68f110b5b5a021e7b38
Author: Thara Gopinath [off-list ref]

firmware: efi: libstub: x86-stub: Enable VSM awareness in efi os indications variable

This commit sets bit 0 of the Hyper-V private OsLoaderIndications EFI variable
during exit_boot() to let the bootloader or firmware know that the OS intends
to enable VTL1. The variable is written only when supported and when the bit
is not already set.

[ ... ]

[Severity: High]
Since val is declared without zero-initialization, is it possible for
get_efi_var() to copy fewer than 4 bytes if the actual size of the variable
in NVRAM is smaller?

If so, the upper bytes of val might remain as uninitialized stack memory.
When set_efi_var() is subsequently called with size = sizeof(val), could
this leak up to 3 bytes of uninitialized kernel stack memory into persistent
NVRAM? 

Also, could this force the EFI variable to be exactly 4 bytes, potentially
corrupting it if the firmware expects a different size?

+
+	size = sizeof(val);
+	status = get_efi_var(efi_HvPrivOsloaderIndicationsSupported_name,
+			     &guid, &attr, &size, &val);
+	if (status != EFI_SUCCESS) {
+		efi_err("Could not read Hyper-V OsloaderIndicationsSupported\n");
+		return;
+	}
+
+	if (!(val & HV_OSLOADER_INDICATION_VSM)) {
+		efi_info("Hyper-V does not support VSM in OsloaderIndicationsSupported\n");
+		return;
+	}
+
+	size = sizeof(val);
+	status = get_efi_var(efi_HvPrivOsloaderIndications_name, &guid, &attr, &size, &val);
+	if (status != EFI_SUCCESS) {
+		efi_err("Could not read Hyper-V OsLoaderIndications\n");

[Severity: Medium]
If the OsLoaderIndications variable does not exist (for example, on a fresh
OS installation or if not pre-populated by firmware), get_efi_var() will
return EFI_NOT_FOUND. 

Does treating this as a fatal error prevent VTL1 from being brought up?

Would it be better to handle EFI_NOT_FOUND by assuming val is 0 and
proceeding to create the variable?

+		return;
+	}
+
+	if (val & HV_OSLOADER_INDICATION_VSM) {
+		efi_info("VSM is already supported in OsLoaderIndications.");

[Severity: Low]
Does this efi_info() string need a trailing newline character?

Because efi_info() maps to efi_printk() which does not append newlines
automatically, this might cause subsequent EFI console output to be
concatenated onto the same line.

[Severity: Medium]
Since this function is called unconditionally on all x86 EFI boots when
CONFIG_HYPERV_VSM is enabled, will this cause get_efi_var() to return
EFI_NOT_FOUND on systems without Hyper-V?

It seems this could result in spurious error messages in the boot log for
all non-Hyper-V users because efi_err() is called unconditionally inside
efi_set_hv_os_indications().

Should there be a check for Hyper-V presence before making this call?

+#endif
+
 	/* Might as well exit boot services now */
 	status = efi_exit_boot_services(handle, &priv, exit_boot_func);
 	if (status != EFI_SUCCESS)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260901165647.3160413-1-tgopinath@linux.microsoft.com?part=4