Re: [edk2-devel] [PATCH] ArmVirtPkg/PlatformBootManagerLib: unload image on EFI_SECURITY_VIOLATION

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 9/3/19 6:38 PM, Laszlo Ersek wrote:
> 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.biesheuvel@linaro.org>
> Cc: Dandan Bi <dandan.bi@intel.com>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1992
> Fixes: 23d04b58e27b382bbd3f9b16ba9adb1cb203dad5
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>

Reviewed-by: Philippe Mathieu-Daude <philmd@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;
>