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

[sashiko-bot@kernel.org]

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 <tgopinath@linux.microsoft.com>

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.

> diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
> index cef32e2c82d8f..ab3cd4fe36599 100644
> --- a/drivers/firmware/efi/libstub/x86-stub.c
> +++ b/drivers/firmware/efi/libstub/x86-stub.c

[ ... ]

> @@ -754,6 +765,47 @@ static efi_status_t exit_boot_func(struct efi_boot_memmap *map,
>  	return EFI_SUCCESS;
>  }
>  
> +#ifdef CONFIG_HYPERV_VSM
> +static void efi_set_hv_os_indications(void)
> +{
> +	efi_guid_t guid = HYPERV_PRIVATE_EFI_NAMESPACE_GUID;
> +	efi_status_t status;
> +	unsigned long size;
> +	u32 attr, val;

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

> +		return;
> +	}
> +
> +	val |= HV_OSLOADER_INDICATION_VSM;
> +	size = sizeof(val);
> +	status = set_efi_var(efi_HvPrivOsloaderIndications_name, &guid, attr, size, &val);
> +	if (status != EFI_SUCCESS)
> +		efi_err("Could not set Hyper-V OsLoaderIndications to indicate VSM support\n");
> +}
> +#endif
> +
>  static efi_status_t exit_boot(struct boot_params *boot_params, void *handle)
>  {
>  	struct setup_data *e820ext = NULL;
> @@ -768,6 +820,11 @@ static efi_status_t exit_boot(struct boot_params *boot_params, void *handle)
>  	if (status != EFI_SUCCESS)
>  		return status;
>  
> +#ifdef CONFIG_HYPERV_VSM
> +	/* Indicate to bootloader that we will be enabling VTL1 before exiting boot services */
> +	efi_set_hv_os_indications();

[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