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

["Dandan Bi" <dandan.bi@intel.com>]

Reviewed-by: Dandan Bi <dandan.bi@intel.com>


Thanks,
Dandan
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Wednesday, September 4, 2019 12:38 AM
> To: edk2-devel-groups-io <devel@edk2.groups.io>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Bi, Dandan
> <dandan.bi@intel.com>; Leif Lindholm <leif.lindholm@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.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>
> ---
> 
> 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