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 <[email protected]> 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/[email protected]?part=4
