Reviewed-by: Dandan Bi <dandan...@intel.com>
Thanks, Dandan > -----Original Message----- > From: Laszlo Ersek [mailto:ler...@redhat.com] > Sent: Wednesday, September 4, 2019 12:38 AM > To: edk2-devel-groups-io <devel@edk2.groups.io> > Cc: Ard Biesheuvel <ard.biesheu...@linaro.org>; Bi, Dandan > <dandan...@intel.com>; Leif Lindholm <leif.lindh...@linaro.org> > Subject: [PATCH] ArmVirtPkg/PlatformBootManagerLib: unload image on > EFI_SECURITY_VIOLATION > > The LoadImage() boot service is a bit unusual in that it allocates resources > in a > particular failure case; namely, it produces a valid "ImageHandle" when it > returns EFI_SECURITY_VIOLATION. This is supposed to happen e.g. when > Secure Boot verification fails for the image, but the platform policy for the > particular image origin (such as "fixed media" or "removable media") is > DEFER_EXECUTE_ON_SECURITY_VIOLATION. The return code allows > platform logic to selectively override the verification failure, and launch > the > image nonetheless. > > ArmVirtPkg/PlatformBootManagerLib does not override > EFI_SECURITY_VIOLATION for the kernel image loaded from fw_cfg -- any > LoadImage() error is considered fatal. When we simply treat > EFI_SECURITY_VIOLATION like any other LoadImage() error, we leak the > resources associated with "KernelImageHandle". From a resource usage > perspective, EFI_SECURITY_VIOLATION must be considered "success", and > rolled back. > > Implement this rollback, without breaking the proper "nesting" of error > handling jumps and labels. > > Cc: Ard Biesheuvel <ard.biesheu...@linaro.org> > Cc: Dandan Bi <dandan...@intel.com> > Cc: Leif Lindholm <leif.lindh...@linaro.org> > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1992 > Fixes: 23d04b58e27b382bbd3f9b16ba9adb1cb203dad5 > Signed-off-by: Laszlo Ersek <ler...@redhat.com> > --- > > Notes: > Repo: https://github.com/lersek/edk2.git > Branch: ldimg_armvirt_bz1992 > > ArmVirtPkg/Library/PlatformBootManagerLib/QemuKernel.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/ArmVirtPkg/Library/PlatformBootManagerLib/QemuKernel.c > b/ArmVirtPkg/Library/PlatformBootManagerLib/QemuKernel.c > index 3cc83e3b7b95..d3851fd75fa5 100644 > --- a/ArmVirtPkg/Library/PlatformBootManagerLib/QemuKernel.c > +++ b/ArmVirtPkg/Library/PlatformBootManagerLib/QemuKernel.c > @@ -968,53 +968,60 @@ TryRunningQemuKernel ( > > // > // Create a device path for the kernel image to be loaded from that will > call > // back into our file system. > // > KernelDevicePath = FileDevicePath (FileSystemHandle, KernelBlob->Name); > if (KernelDevicePath == NULL) { > DEBUG ((EFI_D_ERROR, "%a: failed to allocate kernel device path\n", > __FUNCTION__)); > Status = EFI_OUT_OF_RESOURCES; > goto UninstallProtocols; > } > > // > // Load the image. This should call back into our file system. > // > Status = gBS->LoadImage ( > FALSE, // BootPolicy: exact match required > gImageHandle, // ParentImageHandle > KernelDevicePath, > NULL, // SourceBuffer > 0, // SourceSize > &KernelImageHandle > ); > if (EFI_ERROR (Status)) { > DEBUG ((EFI_D_ERROR, "%a: LoadImage(): %r\n", __FUNCTION__, > Status)); > - goto FreeKernelDevicePath; > + if (Status != EFI_SECURITY_VIOLATION) { > + goto FreeKernelDevicePath; > + } > + // > + // From the resource allocation perspective, EFI_SECURITY_VIOLATION > means > + // "success", so we must roll back the image loading. > + // > + goto UnloadKernelImage; > } > > // > // Construct the kernel command line. > // > Status = gBS->OpenProtocol ( > KernelImageHandle, > &gEfiLoadedImageProtocolGuid, > (VOID **)&KernelLoadedImage, > gImageHandle, // AgentHandle > NULL, // ControllerHandle > EFI_OPEN_PROTOCOL_GET_PROTOCOL > ); > ASSERT_EFI_ERROR (Status); > > if (CommandLineBlob->Data == NULL) { > KernelLoadedImage->LoadOptionsSize = 0; > } else { > // > // Verify NUL-termination of the command line. > // > if (CommandLineBlob->Data[CommandLineBlob->Size - 1] != '\0') { > DEBUG ((EFI_D_ERROR, "%a: kernel command line is not NUL- > terminated\n", > __FUNCTION__)); > Status = EFI_PROTOCOL_ERROR; > goto UnloadKernelImage; > -- > 2.19.1.3.g30247aa5d201 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#46732): https://edk2.groups.io/g/devel/message/46732 Mute This Topic: https://groups.io/mt/33128626/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
