| Message ID | 1468428286-5776-1-git-send-email-lersek@redhat.com |
|---|---|
| State | Accepted |
| Commit | 0e2c6c552990edcd6352c2395860cb0df62b158d |
| Headers | show |
On 13 July 2016 at 18:44, Laszlo Ersek <lersek@redhat.com> wrote: > (This patch ports OvmfPkg commit 2eb358986052 to ArmVirtPkg. That > functionality was not added to QemuBootOrderLib, because it was (and is) > independent from QEMU and fw_cfg.) > > Remove any boot options that point to binaries built into the firmware and > have become stale due to any of the following: > - FvMain's base address or size changed (historical -- see commit > e191a3114f4c), > - FvMain's FvNameGuid changed, > - the FILE_GUID of the pointed-to binary changed, > - the referenced binary is no longer built into the firmware. > > For example, multiple such "EFI Internal Shell" boot options can coexist. > They technically differ from each other, but may not describe any built-in > shell binary exactly. Such options can accumulate in a varstore over time, > and while they remain generally bootable (thanks to the efforts of > BmGetFileBufferByFvFilePath()), they look bad. > > Filter out any stale options. > > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> > Fixes: https://github.com/tianocore/edk2/issues/107 > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Laszlo Ersek <lersek@redhat.com> > --- > ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf | 1 + > ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c | 132 ++++++++++++++++++++ > 2 files changed, 133 insertions(+) > > diff --git a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf > index 9c95b69cc974..bec7fabb479c 100644 > --- a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf > +++ b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf > @@ -78,6 +78,7 @@ [Guids] > > [Protocols] > gEfiDevicePathProtocolGuid > + gEfiFirmwareVolume2ProtocolGuid > gEfiGraphicsOutputProtocolGuid > gEfiLoadedImageProtocolGuid > gEfiPciRootBridgeIoProtocolGuid > diff --git a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c > index eaafe7ff57ea..c11196a8a59c 100644 > --- a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c > +++ b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c > @@ -22,6 +22,7 @@ > #include <Library/QemuBootOrderLib.h> > #include <Library/UefiBootManagerLib.h> > #include <Protocol/DevicePath.h> > +#include <Protocol/FirmwareVolume2.h> > #include <Protocol/GraphicsOutput.h> > #include <Protocol/LoadedImage.h> > #include <Protocol/PciIo.h> > @@ -387,6 +388,136 @@ PlatformRegisterFvBootOption ( > } > > > +/** > + Remove all MemoryMapped(...)/FvFile(...) and Fv(...)/FvFile(...) boot options > + whose device paths do not resolve exactly to an FvFile in the system. > + > + This removes any boot options that point to binaries built into the firmware > + and have become stale due to any of the following: > + - FvMain's base address or size changed (historical), > + - FvMain's FvNameGuid changed, > + - the FILE_GUID of the pointed-to binary changed, > + - the referenced binary is no longer built into the firmware. > + > + EfiBootManagerFindLoadOption() used in PlatformRegisterFvBootOption() only > + avoids exact duplicates. > +**/ > +STATIC > +VOID > +RemoveStaleFvFileOptions ( > + VOID > + ) > +{ > + EFI_BOOT_MANAGER_LOAD_OPTION *BootOptions; > + UINTN BootOptionCount; > + UINTN Index; > + > + BootOptions = EfiBootManagerGetLoadOptions (&BootOptionCount, > + LoadOptionTypeBoot); > + > + for (Index = 0; Index < BootOptionCount; ++Index) { > + EFI_DEVICE_PATH_PROTOCOL *Node1, *Node2, *SearchNode; > + EFI_STATUS Status; > + EFI_HANDLE FvHandle; > + > + // > + // If the device path starts with neither MemoryMapped(...) nor Fv(...), > + // then keep the boot option. > + // > + Node1 = BootOptions[Index].FilePath; > + if (!(DevicePathType (Node1) == HARDWARE_DEVICE_PATH && > + DevicePathSubType (Node1) == HW_MEMMAP_DP) && > + !(DevicePathType (Node1) == MEDIA_DEVICE_PATH && > + DevicePathSubType (Node1) == MEDIA_PIWG_FW_VOL_DP)) { > + continue; > + } > + > + // > + // If the second device path node is not FvFile(...), then keep the boot > + // option. > + // > + Node2 = NextDevicePathNode (Node1); > + if (DevicePathType (Node2) != MEDIA_DEVICE_PATH || > + DevicePathSubType (Node2) != MEDIA_PIWG_FW_FILE_DP) { > + continue; > + } > + > + // > + // Locate the Firmware Volume2 protocol instance that is denoted by the > + // boot option. If this lookup fails (i.e., the boot option references a > + // firmware volume that doesn't exist), then we'll proceed to delete the > + // boot option. > + // > + SearchNode = Node1; > + Status = gBS->LocateDevicePath (&gEfiFirmwareVolume2ProtocolGuid, > + &SearchNode, &FvHandle); > + > + if (!EFI_ERROR (Status)) { > + // > + // The firmware volume was found; now let's see if it contains the FvFile > + // identified by GUID. > + // > + EFI_FIRMWARE_VOLUME2_PROTOCOL *FvProtocol; > + MEDIA_FW_VOL_FILEPATH_DEVICE_PATH *FvFileNode; > + UINTN BufferSize; > + EFI_FV_FILETYPE FoundType; > + EFI_FV_FILE_ATTRIBUTES FileAttributes; > + UINT32 AuthenticationStatus; > + > + Status = gBS->HandleProtocol (FvHandle, &gEfiFirmwareVolume2ProtocolGuid, > + (VOID **)&FvProtocol); > + ASSERT_EFI_ERROR (Status); > + > + FvFileNode = (MEDIA_FW_VOL_FILEPATH_DEVICE_PATH *)Node2; > + // > + // Buffer==NULL means we request metadata only: BufferSize, FoundType, > + // FileAttributes. > + // > + Status = FvProtocol->ReadFile ( > + FvProtocol, > + &FvFileNode->FvFileName, // NameGuid > + NULL, // Buffer > + &BufferSize, > + &FoundType, > + &FileAttributes, > + &AuthenticationStatus > + ); > + if (!EFI_ERROR (Status)) { > + // > + // The FvFile was found. Keep the boot option. > + // > + continue; > + } > + } > + > + // > + // Delete the boot option. > + // > + Status = EfiBootManagerDeleteLoadOptionVariable ( > + BootOptions[Index].OptionNumber, LoadOptionTypeBoot); So I suppose deleting by index does not reshuffle the list? > + DEBUG_CODE ( > + CHAR16 *DevicePathString; > + > + DevicePathString = ConvertDevicePathToText(BootOptions[Index].FilePath, > + FALSE, FALSE); > + DEBUG (( > + EFI_ERROR (Status) ? EFI_D_WARN : EFI_D_VERBOSE, > + "%a: removing stale Boot#%04x %s: %r\n", > + __FUNCTION__, > + (UINT32)BootOptions[Index].OptionNumber, > + DevicePathString == NULL ? L"<unavailable>" : DevicePathString, > + Status > + )); > + if (DevicePathString != NULL) { > + FreePool (DevicePathString); > + } > + ); Indentation? > + } > + > + EfiBootManagerFreeLoadOptions (BootOptions, BootOptionCount); > +} > + > + > STATIC > VOID > PlatformRegisterOptionsAndKeys ( > @@ -560,6 +691,7 @@ PlatformBootManagerAfterConsole ( > PcdGetPtr (PcdShellFile), L"EFI Internal Shell", LOAD_OPTION_ACTIVE > ); > > + RemoveStaleFvFileOptions (); > SetBootOrderFromQemu (); > } > > -- > 1.8.3.1 > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 07/13/16 22:03, Ard Biesheuvel wrote: > On 13 July 2016 at 18:44, Laszlo Ersek <lersek@redhat.com> wrote: >> (This patch ports OvmfPkg commit 2eb358986052 to ArmVirtPkg. That >> functionality was not added to QemuBootOrderLib, because it was (and is) >> independent from QEMU and fw_cfg.) >> >> Remove any boot options that point to binaries built into the firmware and >> have become stale due to any of the following: >> - FvMain's base address or size changed (historical -- see commit >> e191a3114f4c), >> - FvMain's FvNameGuid changed, >> - the FILE_GUID of the pointed-to binary changed, >> - the referenced binary is no longer built into the firmware. >> >> For example, multiple such "EFI Internal Shell" boot options can coexist. >> They technically differ from each other, but may not describe any built-in >> shell binary exactly. Such options can accumulate in a varstore over time, >> and while they remain generally bootable (thanks to the efforts of >> BmGetFileBufferByFvFilePath()), they look bad. >> >> Filter out any stale options. >> >> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> Fixes: https://github.com/tianocore/edk2/issues/107 >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Laszlo Ersek <lersek@redhat.com> >> --- >> ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf | 1 + >> ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c | 132 ++++++++++++++++++++ >> 2 files changed, 133 insertions(+) >> >> diff --git a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf >> index 9c95b69cc974..bec7fabb479c 100644 >> --- a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf >> +++ b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf >> @@ -78,6 +78,7 @@ [Guids] >> >> [Protocols] >> gEfiDevicePathProtocolGuid >> + gEfiFirmwareVolume2ProtocolGuid >> gEfiGraphicsOutputProtocolGuid >> gEfiLoadedImageProtocolGuid >> gEfiPciRootBridgeIoProtocolGuid >> diff --git a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c >> index eaafe7ff57ea..c11196a8a59c 100644 >> --- a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c >> +++ b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c >> @@ -22,6 +22,7 @@ >> #include <Library/QemuBootOrderLib.h> >> #include <Library/UefiBootManagerLib.h> >> #include <Protocol/DevicePath.h> >> +#include <Protocol/FirmwareVolume2.h> >> #include <Protocol/GraphicsOutput.h> >> #include <Protocol/LoadedImage.h> >> #include <Protocol/PciIo.h> >> @@ -387,6 +388,136 @@ PlatformRegisterFvBootOption ( >> } >> >> >> +/** >> + Remove all MemoryMapped(...)/FvFile(...) and Fv(...)/FvFile(...) boot options >> + whose device paths do not resolve exactly to an FvFile in the system. >> + >> + This removes any boot options that point to binaries built into the firmware >> + and have become stale due to any of the following: >> + - FvMain's base address or size changed (historical), >> + - FvMain's FvNameGuid changed, >> + - the FILE_GUID of the pointed-to binary changed, >> + - the referenced binary is no longer built into the firmware. >> + >> + EfiBootManagerFindLoadOption() used in PlatformRegisterFvBootOption() only >> + avoids exact duplicates. >> +**/ >> +STATIC >> +VOID >> +RemoveStaleFvFileOptions ( >> + VOID >> + ) >> +{ >> + EFI_BOOT_MANAGER_LOAD_OPTION *BootOptions; >> + UINTN BootOptionCount; >> + UINTN Index; >> + >> + BootOptions = EfiBootManagerGetLoadOptions (&BootOptionCount, >> + LoadOptionTypeBoot); >> + >> + for (Index = 0; Index < BootOptionCount; ++Index) { >> + EFI_DEVICE_PATH_PROTOCOL *Node1, *Node2, *SearchNode; >> + EFI_STATUS Status; >> + EFI_HANDLE FvHandle; >> + >> + // >> + // If the device path starts with neither MemoryMapped(...) nor Fv(...), >> + // then keep the boot option. >> + // >> + Node1 = BootOptions[Index].FilePath; >> + if (!(DevicePathType (Node1) == HARDWARE_DEVICE_PATH && >> + DevicePathSubType (Node1) == HW_MEMMAP_DP) && >> + !(DevicePathType (Node1) == MEDIA_DEVICE_PATH && >> + DevicePathSubType (Node1) == MEDIA_PIWG_FW_VOL_DP)) { >> + continue; >> + } >> + >> + // >> + // If the second device path node is not FvFile(...), then keep the boot >> + // option. >> + // >> + Node2 = NextDevicePathNode (Node1); >> + if (DevicePathType (Node2) != MEDIA_DEVICE_PATH || >> + DevicePathSubType (Node2) != MEDIA_PIWG_FW_FILE_DP) { >> + continue; >> + } >> + >> + // >> + // Locate the Firmware Volume2 protocol instance that is denoted by the >> + // boot option. If this lookup fails (i.e., the boot option references a >> + // firmware volume that doesn't exist), then we'll proceed to delete the >> + // boot option. >> + // >> + SearchNode = Node1; >> + Status = gBS->LocateDevicePath (&gEfiFirmwareVolume2ProtocolGuid, >> + &SearchNode, &FvHandle); >> + >> + if (!EFI_ERROR (Status)) { >> + // >> + // The firmware volume was found; now let's see if it contains the FvFile >> + // identified by GUID. >> + // >> + EFI_FIRMWARE_VOLUME2_PROTOCOL *FvProtocol; >> + MEDIA_FW_VOL_FILEPATH_DEVICE_PATH *FvFileNode; >> + UINTN BufferSize; >> + EFI_FV_FILETYPE FoundType; >> + EFI_FV_FILE_ATTRIBUTES FileAttributes; >> + UINT32 AuthenticationStatus; >> + >> + Status = gBS->HandleProtocol (FvHandle, &gEfiFirmwareVolume2ProtocolGuid, >> + (VOID **)&FvProtocol); >> + ASSERT_EFI_ERROR (Status); >> + >> + FvFileNode = (MEDIA_FW_VOL_FILEPATH_DEVICE_PATH *)Node2; >> + // >> + // Buffer==NULL means we request metadata only: BufferSize, FoundType, >> + // FileAttributes. >> + // >> + Status = FvProtocol->ReadFile ( >> + FvProtocol, >> + &FvFileNode->FvFileName, // NameGuid >> + NULL, // Buffer >> + &BufferSize, >> + &FoundType, >> + &FileAttributes, >> + &AuthenticationStatus >> + ); >> + if (!EFI_ERROR (Status)) { >> + // >> + // The FvFile was found. Keep the boot option. >> + // >> + continue; >> + } >> + } >> + >> + // >> + // Delete the boot option. >> + // >> + Status = EfiBootManagerDeleteLoadOptionVariable ( >> + BootOptions[Index].OptionNumber, LoadOptionTypeBoot); > > So I suppose deleting by index does not reshuffle the list? That's right. The OptionNumber field is the "Boot####" variable's number, and EfiBootManagerDeleteLoadOptionVariable() works on the "BootOrder" and "Boot####" variables themselves. The BootOptions array that we have in memory at this time is independent. The "BootOrder" variable is shrunken, but the next call will again supply a "Boot####" variable number, which is looked up from scratch in "BootOrder". > >> + DEBUG_CODE ( >> + CHAR16 *DevicePathString; >> + >> + DevicePathString = ConvertDevicePathToText(BootOptions[Index].FilePath, >> + FALSE, FALSE); >> + DEBUG (( >> + EFI_ERROR (Status) ? EFI_D_WARN : EFI_D_VERBOSE, >> + "%a: removing stale Boot#%04x %s: %r\n", >> + __FUNCTION__, >> + (UINT32)BootOptions[Index].OptionNumber, >> + DevicePathString == NULL ? L"<unavailable>" : DevicePathString, >> + Status >> + )); >> + if (DevicePathString != NULL) { >> + FreePool (DevicePathString); >> + } >> + ); > > Indentation? This was intentional on my part; the canonical edk2 style requires BlahBlah ( Foo, Bar, Quux ); Are you suggesting we shouldn't follow it in this case, with DEBUG_CODE? If so I hope you'd be okay with me fixing up that one trailing ")" before pushing the patch :) Thanks! Laszlo > >> + } >> + >> + EfiBootManagerFreeLoadOptions (BootOptions, BootOptionCount); >> +} >> + >> + >> STATIC >> VOID >> PlatformRegisterOptionsAndKeys ( >> @@ -560,6 +691,7 @@ PlatformBootManagerAfterConsole ( >> PcdGetPtr (PcdShellFile), L"EFI Internal Shell", LOAD_OPTION_ACTIVE >> ); >> >> + RemoveStaleFvFileOptions (); >> SetBootOrderFromQemu (); >> } >> >> -- >> 1.8.3.1 >> _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 13 July 2016 at 22:17, Laszlo Ersek <lersek@redhat.com> wrote: > On 07/13/16 22:03, Ard Biesheuvel wrote: >> On 13 July 2016 at 18:44, Laszlo Ersek <lersek@redhat.com> wrote: >>> (This patch ports OvmfPkg commit 2eb358986052 to ArmVirtPkg. That >>> functionality was not added to QemuBootOrderLib, because it was (and is) >>> independent from QEMU and fw_cfg.) >>> >>> Remove any boot options that point to binaries built into the firmware and >>> have become stale due to any of the following: >>> - FvMain's base address or size changed (historical -- see commit >>> e191a3114f4c), >>> - FvMain's FvNameGuid changed, >>> - the FILE_GUID of the pointed-to binary changed, >>> - the referenced binary is no longer built into the firmware. >>> >>> For example, multiple such "EFI Internal Shell" boot options can coexist. >>> They technically differ from each other, but may not describe any built-in >>> shell binary exactly. Such options can accumulate in a varstore over time, >>> and while they remain generally bootable (thanks to the efforts of >>> BmGetFileBufferByFvFilePath()), they look bad. >>> >>> Filter out any stale options. >>> >>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> >>> Fixes: https://github.com/tianocore/edk2/issues/107 >>> Contributed-under: TianoCore Contribution Agreement 1.0 >>> Signed-off-by: Laszlo Ersek <lersek@redhat.com> >>> --- >>> ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf | 1 + >>> ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c | 132 ++++++++++++++++++++ >>> 2 files changed, 133 insertions(+) >>> >>> diff --git a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf >>> index 9c95b69cc974..bec7fabb479c 100644 >>> --- a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf >>> +++ b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf >>> @@ -78,6 +78,7 @@ [Guids] >>> >>> [Protocols] >>> gEfiDevicePathProtocolGuid >>> + gEfiFirmwareVolume2ProtocolGuid >>> gEfiGraphicsOutputProtocolGuid >>> gEfiLoadedImageProtocolGuid >>> gEfiPciRootBridgeIoProtocolGuid >>> diff --git a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c >>> index eaafe7ff57ea..c11196a8a59c 100644 >>> --- a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c >>> +++ b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c >>> @@ -22,6 +22,7 @@ >>> #include <Library/QemuBootOrderLib.h> >>> #include <Library/UefiBootManagerLib.h> >>> #include <Protocol/DevicePath.h> >>> +#include <Protocol/FirmwareVolume2.h> >>> #include <Protocol/GraphicsOutput.h> >>> #include <Protocol/LoadedImage.h> >>> #include <Protocol/PciIo.h> >>> @@ -387,6 +388,136 @@ PlatformRegisterFvBootOption ( >>> } >>> >>> >>> +/** >>> + Remove all MemoryMapped(...)/FvFile(...) and Fv(...)/FvFile(...) boot options >>> + whose device paths do not resolve exactly to an FvFile in the system. >>> + >>> + This removes any boot options that point to binaries built into the firmware >>> + and have become stale due to any of the following: >>> + - FvMain's base address or size changed (historical), >>> + - FvMain's FvNameGuid changed, >>> + - the FILE_GUID of the pointed-to binary changed, >>> + - the referenced binary is no longer built into the firmware. >>> + >>> + EfiBootManagerFindLoadOption() used in PlatformRegisterFvBootOption() only >>> + avoids exact duplicates. >>> +**/ >>> +STATIC >>> +VOID >>> +RemoveStaleFvFileOptions ( >>> + VOID >>> + ) >>> +{ >>> + EFI_BOOT_MANAGER_LOAD_OPTION *BootOptions; >>> + UINTN BootOptionCount; >>> + UINTN Index; >>> + >>> + BootOptions = EfiBootManagerGetLoadOptions (&BootOptionCount, >>> + LoadOptionTypeBoot); >>> + >>> + for (Index = 0; Index < BootOptionCount; ++Index) { >>> + EFI_DEVICE_PATH_PROTOCOL *Node1, *Node2, *SearchNode; >>> + EFI_STATUS Status; >>> + EFI_HANDLE FvHandle; >>> + >>> + // >>> + // If the device path starts with neither MemoryMapped(...) nor Fv(...), >>> + // then keep the boot option. >>> + // >>> + Node1 = BootOptions[Index].FilePath; >>> + if (!(DevicePathType (Node1) == HARDWARE_DEVICE_PATH && >>> + DevicePathSubType (Node1) == HW_MEMMAP_DP) && >>> + !(DevicePathType (Node1) == MEDIA_DEVICE_PATH && >>> + DevicePathSubType (Node1) == MEDIA_PIWG_FW_VOL_DP)) { >>> + continue; >>> + } >>> + >>> + // >>> + // If the second device path node is not FvFile(...), then keep the boot >>> + // option. >>> + // >>> + Node2 = NextDevicePathNode (Node1); >>> + if (DevicePathType (Node2) != MEDIA_DEVICE_PATH || >>> + DevicePathSubType (Node2) != MEDIA_PIWG_FW_FILE_DP) { >>> + continue; >>> + } >>> + >>> + // >>> + // Locate the Firmware Volume2 protocol instance that is denoted by the >>> + // boot option. If this lookup fails (i.e., the boot option references a >>> + // firmware volume that doesn't exist), then we'll proceed to delete the >>> + // boot option. >>> + // >>> + SearchNode = Node1; >>> + Status = gBS->LocateDevicePath (&gEfiFirmwareVolume2ProtocolGuid, >>> + &SearchNode, &FvHandle); >>> + >>> + if (!EFI_ERROR (Status)) { >>> + // >>> + // The firmware volume was found; now let's see if it contains the FvFile >>> + // identified by GUID. >>> + // >>> + EFI_FIRMWARE_VOLUME2_PROTOCOL *FvProtocol; >>> + MEDIA_FW_VOL_FILEPATH_DEVICE_PATH *FvFileNode; >>> + UINTN BufferSize; >>> + EFI_FV_FILETYPE FoundType; >>> + EFI_FV_FILE_ATTRIBUTES FileAttributes; >>> + UINT32 AuthenticationStatus; >>> + >>> + Status = gBS->HandleProtocol (FvHandle, &gEfiFirmwareVolume2ProtocolGuid, >>> + (VOID **)&FvProtocol); >>> + ASSERT_EFI_ERROR (Status); >>> + >>> + FvFileNode = (MEDIA_FW_VOL_FILEPATH_DEVICE_PATH *)Node2; >>> + // >>> + // Buffer==NULL means we request metadata only: BufferSize, FoundType, >>> + // FileAttributes. >>> + // >>> + Status = FvProtocol->ReadFile ( >>> + FvProtocol, >>> + &FvFileNode->FvFileName, // NameGuid >>> + NULL, // Buffer >>> + &BufferSize, >>> + &FoundType, >>> + &FileAttributes, >>> + &AuthenticationStatus >>> + ); >>> + if (!EFI_ERROR (Status)) { >>> + // >>> + // The FvFile was found. Keep the boot option. >>> + // >>> + continue; >>> + } >>> + } >>> + >>> + // >>> + // Delete the boot option. >>> + // >>> + Status = EfiBootManagerDeleteLoadOptionVariable ( >>> + BootOptions[Index].OptionNumber, LoadOptionTypeBoot); >> >> So I suppose deleting by index does not reshuffle the list? > > That's right. The OptionNumber field is the "Boot####" variable's > number, and EfiBootManagerDeleteLoadOptionVariable() works on the > "BootOrder" and "Boot####" variables themselves. > > The BootOptions array that we have in memory at this time is > independent. The "BootOrder" variable is shrunken, but the next call > will again supply a "Boot####" variable number, which is looked up from > scratch in "BootOrder". > OK, thanks for clarifying. >> >>> + DEBUG_CODE ( >>> + CHAR16 *DevicePathString; >>> + >>> + DevicePathString = ConvertDevicePathToText(BootOptions[Index].FilePath, >>> + FALSE, FALSE); >>> + DEBUG (( >>> + EFI_ERROR (Status) ? EFI_D_WARN : EFI_D_VERBOSE, >>> + "%a: removing stale Boot#%04x %s: %r\n", >>> + __FUNCTION__, >>> + (UINT32)BootOptions[Index].OptionNumber, >>> + DevicePathString == NULL ? L"<unavailable>" : DevicePathString, >>> + Status >>> + )); >>> + if (DevicePathString != NULL) { >>> + FreePool (DevicePathString); >>> + } >>> + ); >> >> Indentation? > > This was intentional on my part; the canonical edk2 style requires > > BlahBlah ( > Foo, > Bar, > Quux > ); > > Are you suggesting we shouldn't follow it in this case, with DEBUG_CODE? > If so I hope you'd be okay with me fixing up that one trailing ")" > before pushing the patch :) > I don't care too deeply, tbh, it is just something that stands out when looking at the patch. So leave it, or change it, as you prefer. Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 07/13/16 22:31, Ard Biesheuvel wrote: > On 13 July 2016 at 22:17, Laszlo Ersek <lersek@redhat.com> wrote: >> On 07/13/16 22:03, Ard Biesheuvel wrote: >>> On 13 July 2016 at 18:44, Laszlo Ersek <lersek@redhat.com> wrote: >>>> + DEBUG_CODE ( >>>> + CHAR16 *DevicePathString; >>>> + >>>> + DevicePathString = ConvertDevicePathToText(BootOptions[Index].FilePath, >>>> + FALSE, FALSE); >>>> + DEBUG (( >>>> + EFI_ERROR (Status) ? EFI_D_WARN : EFI_D_VERBOSE, >>>> + "%a: removing stale Boot#%04x %s: %r\n", >>>> + __FUNCTION__, >>>> + (UINT32)BootOptions[Index].OptionNumber, >>>> + DevicePathString == NULL ? L"<unavailable>" : DevicePathString, >>>> + Status >>>> + )); >>>> + if (DevicePathString != NULL) { >>>> + FreePool (DevicePathString); >>>> + } >>>> + ); >>> >>> Indentation? >> >> This was intentional on my part; the canonical edk2 style requires >> >> BlahBlah ( >> Foo, >> Bar, >> Quux >> ); >> >> Are you suggesting we shouldn't follow it in this case, with DEBUG_CODE? >> If so I hope you'd be okay with me fixing up that one trailing ")" >> before pushing the patch :) >> > > I don't care too deeply, tbh, it is just something that stands out > when looking at the patch. > > So leave it, or change it, as you prefer. I left it alone, in order to stay close to the original OvmfPkg patch. > Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> Thank you! Pushed as 0e2c6c552990. Cheers Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
diff --git a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf index 9c95b69cc974..bec7fabb479c 100644 --- a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf +++ b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf @@ -78,6 +78,7 @@ [Guids] [Protocols] gEfiDevicePathProtocolGuid + gEfiFirmwareVolume2ProtocolGuid gEfiGraphicsOutputProtocolGuid gEfiLoadedImageProtocolGuid gEfiPciRootBridgeIoProtocolGuid diff --git a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c index eaafe7ff57ea..c11196a8a59c 100644 --- a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c +++ b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c @@ -22,6 +22,7 @@ #include <Library/QemuBootOrderLib.h> #include <Library/UefiBootManagerLib.h> #include <Protocol/DevicePath.h> +#include <Protocol/FirmwareVolume2.h> #include <Protocol/GraphicsOutput.h> #include <Protocol/LoadedImage.h> #include <Protocol/PciIo.h> @@ -387,6 +388,136 @@ PlatformRegisterFvBootOption ( } +/** + Remove all MemoryMapped(...)/FvFile(...) and Fv(...)/FvFile(...) boot options + whose device paths do not resolve exactly to an FvFile in the system. + + This removes any boot options that point to binaries built into the firmware + and have become stale due to any of the following: + - FvMain's base address or size changed (historical), + - FvMain's FvNameGuid changed, + - the FILE_GUID of the pointed-to binary changed, + - the referenced binary is no longer built into the firmware. + + EfiBootManagerFindLoadOption() used in PlatformRegisterFvBootOption() only + avoids exact duplicates. +**/ +STATIC +VOID +RemoveStaleFvFileOptions ( + VOID + ) +{ + EFI_BOOT_MANAGER_LOAD_OPTION *BootOptions; + UINTN BootOptionCount; + UINTN Index; + + BootOptions = EfiBootManagerGetLoadOptions (&BootOptionCount, + LoadOptionTypeBoot); + + for (Index = 0; Index < BootOptionCount; ++Index) { + EFI_DEVICE_PATH_PROTOCOL *Node1, *Node2, *SearchNode; + EFI_STATUS Status; + EFI_HANDLE FvHandle; + + // + // If the device path starts with neither MemoryMapped(...) nor Fv(...), + // then keep the boot option. + // + Node1 = BootOptions[Index].FilePath; + if (!(DevicePathType (Node1) == HARDWARE_DEVICE_PATH && + DevicePathSubType (Node1) == HW_MEMMAP_DP) && + !(DevicePathType (Node1) == MEDIA_DEVICE_PATH && + DevicePathSubType (Node1) == MEDIA_PIWG_FW_VOL_DP)) { + continue; + } + + // + // If the second device path node is not FvFile(...), then keep the boot + // option. + // + Node2 = NextDevicePathNode (Node1); + if (DevicePathType (Node2) != MEDIA_DEVICE_PATH || + DevicePathSubType (Node2) != MEDIA_PIWG_FW_FILE_DP) { + continue; + } + + // + // Locate the Firmware Volume2 protocol instance that is denoted by the + // boot option. If this lookup fails (i.e., the boot option references a + // firmware volume that doesn't exist), then we'll proceed to delete the + // boot option. + // + SearchNode = Node1; + Status = gBS->LocateDevicePath (&gEfiFirmwareVolume2ProtocolGuid, + &SearchNode, &FvHandle); + + if (!EFI_ERROR (Status)) { + // + // The firmware volume was found; now let's see if it contains the FvFile + // identified by GUID. + // + EFI_FIRMWARE_VOLUME2_PROTOCOL *FvProtocol; + MEDIA_FW_VOL_FILEPATH_DEVICE_PATH *FvFileNode; + UINTN BufferSize; + EFI_FV_FILETYPE FoundType; + EFI_FV_FILE_ATTRIBUTES FileAttributes; + UINT32 AuthenticationStatus; + + Status = gBS->HandleProtocol (FvHandle, &gEfiFirmwareVolume2ProtocolGuid, + (VOID **)&FvProtocol); + ASSERT_EFI_ERROR (Status); + + FvFileNode = (MEDIA_FW_VOL_FILEPATH_DEVICE_PATH *)Node2; + // + // Buffer==NULL means we request metadata only: BufferSize, FoundType, + // FileAttributes. + // + Status = FvProtocol->ReadFile ( + FvProtocol, + &FvFileNode->FvFileName, // NameGuid + NULL, // Buffer + &BufferSize, + &FoundType, + &FileAttributes, + &AuthenticationStatus + ); + if (!EFI_ERROR (Status)) { + // + // The FvFile was found. Keep the boot option. + // + continue; + } + } + + // + // Delete the boot option. + // + Status = EfiBootManagerDeleteLoadOptionVariable ( + BootOptions[Index].OptionNumber, LoadOptionTypeBoot); + DEBUG_CODE ( + CHAR16 *DevicePathString; + + DevicePathString = ConvertDevicePathToText(BootOptions[Index].FilePath, + FALSE, FALSE); + DEBUG (( + EFI_ERROR (Status) ? EFI_D_WARN : EFI_D_VERBOSE, + "%a: removing stale Boot#%04x %s: %r\n", + __FUNCTION__, + (UINT32)BootOptions[Index].OptionNumber, + DevicePathString == NULL ? L"<unavailable>" : DevicePathString, + Status + )); + if (DevicePathString != NULL) { + FreePool (DevicePathString); + } + ); + } + + EfiBootManagerFreeLoadOptions (BootOptions, BootOptionCount); +} + + STATIC VOID PlatformRegisterOptionsAndKeys ( @@ -560,6 +691,7 @@ PlatformBootManagerAfterConsole ( PcdGetPtr (PcdShellFile), L"EFI Internal Shell", LOAD_OPTION_ACTIVE ); + RemoveStaleFvFileOptions (); SetBootOrderFromQemu (); }
(This patch ports OvmfPkg commit 2eb358986052 to ArmVirtPkg. That functionality was not added to QemuBootOrderLib, because it was (and is) independent from QEMU and fw_cfg.) Remove any boot options that point to binaries built into the firmware and have become stale due to any of the following: - FvMain's base address or size changed (historical -- see commit e191a3114f4c), - FvMain's FvNameGuid changed, - the FILE_GUID of the pointed-to binary changed, - the referenced binary is no longer built into the firmware. For example, multiple such "EFI Internal Shell" boot options can coexist. They technically differ from each other, but may not describe any built-in shell binary exactly. Such options can accumulate in a varstore over time, and while they remain generally bootable (thanks to the efforts of BmGetFileBufferByFvFilePath()), they look bad. Filter out any stale options. Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> Fixes: https://github.com/tianocore/edk2/issues/107 Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf | 1 + ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c | 132 ++++++++++++++++++++ 2 files changed, 133 insertions(+) -- 1.8.3.1 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel