Re: [edk2-devel] [PATCH 2/2] AmdSev: Halt on failed blob allocation

["Lendacky, Thomas via groups.io" <thomas.lendacky=amd.com@groups.io>]

On 5/6/24 15:27, Tobin Feldman-Fitzthum wrote:
> A malicious host may be able to undermine the fw_cfg
> interface such that loading a blob fails.
> 
> In this case rather than continuing to the next boot
> option, the blob verifier should halt.
> 
> For non-confidential guests, the error should be non-fatal.
> 
> Signed-off-by: Tobin Feldman-Fitzthum <tobin@linux.ibm.com>
> ---
>   .../BlobVerifierSevHashes.c                     | 17 ++++++++++++++++-
>   OvmfPkg/Include/Library/BlobVerifierLib.h       | 14 ++++++++++----
>   .../BlobVerifierLibNull/BlobVerifierNull.c      | 13 ++++++++-----
>   .../QemuKernelLoaderFsDxe.c                     |  9 ++++-----
>   4 files changed, 38 insertions(+), 15 deletions(-)
> 
> diff --git a/OvmfPkg/AmdSev/BlobVerifierLibSevHashes/BlobVerifierSevHashes.c b/OvmfPkg/AmdSev/BlobVerifierLibSevHashes/BlobVerifierSevHashes.c
> index ee8bca509a..c550518d73 100644
> --- a/OvmfPkg/AmdSev/BlobVerifierLibSevHashes/BlobVerifierSevHashes.c
> +++ b/OvmfPkg/AmdSev/BlobVerifierLibSevHashes/BlobVerifierSevHashes.c
> @@ -83,6 +83,7 @@ FindBlobEntryGuid (
>     @param[in] BlobName           The name of the blob
>     @param[in] Buf                The data of the blob
>     @param[in] BufSize            The size of the blob in bytes
> +  @param[in] FetchStatus        The status of the previous blob fetch
>   
>     @retval EFI_SUCCESS           The blob was verified successfully or was not
>                                   found in the hash table.
> @@ -94,13 +95,27 @@ EFIAPI
>   VerifyBlob (
>     IN  CONST CHAR16  *BlobName,
>     IN  CONST VOID    *Buf,
> -  IN  UINT32        BufSize
> +  IN  UINT32        BufSize,
> +  IN  EFI_STATUS    FetchStatus
>     )
>   {
>     CONST GUID  *Guid;
>     INT32       Remaining;
>     HASH_TABLE  *Entry;
>   
> +  // Enter a dead loop if the fetching of this blob
> +  // failed. This prevents a malicious host from
> +  // circumventing the following checks.
> +  if (EFI_ERROR (FetchStatus)) {
> +    DEBUG ((
> +      DEBUG_ERROR,
> +      "%a: Fetching blob failed.\n",
> +      __func__
> +      ));
> +
> +    CpuDeadLoop ();
> +  }
> +
>     if ((mHashesTable == NULL) || (mHashesTableSize == 0)) {
>       DEBUG ((
>         DEBUG_WARN,
> diff --git a/OvmfPkg/Include/Library/BlobVerifierLib.h b/OvmfPkg/Include/Library/BlobVerifierLib.h
> index 7e1af27574..efe26734b1 100644
> --- a/OvmfPkg/Include/Library/BlobVerifierLib.h
> +++ b/OvmfPkg/Include/Library/BlobVerifierLib.h
> @@ -19,20 +19,26 @@
>   /**
>     Verify blob from an external source.
>   
> +  If a non-secure configuration is detected this function will enter a
> +  dead loop to prevent a boot.
> +

Probably shouldn't specify this here as the VerifyBlob() that is not in 
AmdSev will not enter a dead loop.

Thanks,
Tom

>     @param[in] BlobName           The name of the blob
>     @param[in] Buf                The data of the blob
>     @param[in] BufSize            The size of the blob in bytes
> +  @param[in] FetchStatus        The status of fetching this blob
>   
> -  @retval EFI_SUCCESS           The blob was verified successfully.
> -  @retval EFI_ACCESS_DENIED     The blob could not be verified, and therefore
> -                                should be considered non-secure.
> +  @retval EFI_SUCCESS           The blob was verified successfully or was not
> +                                found in the hash table.
> +  @retval EFI_ACCESS_DENIED     Kernel hashes not supported but the boot can
> +                                continue safely.
>   **/
>   EFI_STATUS
>   EFIAPI
>   VerifyBlob (
>     IN  CONST CHAR16  *BlobName,
>     IN  CONST VOID    *Buf,
> -  IN  UINT32        BufSize
> +  IN  UINT32        BufSize,
> +  IN  EFI_STATUS    FetchStatus
>     );
>   
>   #endif
> diff --git a/OvmfPkg/Library/BlobVerifierLibNull/BlobVerifierNull.c b/OvmfPkg/Library/BlobVerifierLibNull/BlobVerifierNull.c
> index e817c3cc95..db5320571c 100644
> --- a/OvmfPkg/Library/BlobVerifierLibNull/BlobVerifierNull.c
> +++ b/OvmfPkg/Library/BlobVerifierLibNull/BlobVerifierNull.c
> @@ -16,18 +16,21 @@
>     @param[in] BlobName           The name of the blob
>     @param[in] Buf                The data of the blob
>     @param[in] BufSize            The size of the blob in bytes
> +  @param[in] FetchStatus        The status of the fetch of this blob
>   
> -  @retval EFI_SUCCESS           The blob was verified successfully.
> -  @retval EFI_ACCESS_DENIED     The blob could not be verified, and therefore
> -                                should be considered non-secure.
> +  @retval EFI_SUCCESS           The blob was verified successfully or was not
> +                                found in the hash table.
> +  @retval EFI_ACCESS_DENIED     Kernel hashes not supported but the boot can
> +                                continue safely.
>   **/
>   EFI_STATUS
>   EFIAPI
>   VerifyBlob (
>     IN  CONST CHAR16  *BlobName,
>     IN  CONST VOID    *Buf,
> -  IN  UINT32        BufSize
> +  IN  UINT32        BufSize,
> +  IN  EFI_STATUS    FetchStatus
>     )
>   {
> -  return EFI_SUCCESS;
> +  return FetchStatus;
>   }
> diff --git a/OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c b/OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c
> index 3c12085f6c..cf58c97cd2 100644
> --- a/OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c
> +++ b/OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c
> @@ -1042,6 +1042,7 @@ QemuKernelLoaderFsDxeEntrypoint (
>     KERNEL_BLOB  *CurrentBlob;
>     KERNEL_BLOB  *KernelBlob;
>     EFI_STATUS   Status;
> +  EFI_STATUS   FetchStatus;
>     EFI_HANDLE   FileSystemHandle;
>     EFI_HANDLE   InitrdLoadFile2Handle;
>   
> @@ -1060,15 +1061,13 @@ QemuKernelLoaderFsDxeEntrypoint (
>     //
>     for (BlobType = 0; BlobType < KernelBlobTypeMax; ++BlobType) {
>       CurrentBlob = &mKernelBlob[BlobType];
> -    Status      = FetchBlob (CurrentBlob);
> -    if (EFI_ERROR (Status)) {
> -      goto FreeBlobs;
> -    }
> +    FetchStatus = FetchBlob (CurrentBlob);
>   
>       Status = VerifyBlob (
>                  CurrentBlob->Name,
>                  CurrentBlob->Data,
> -               CurrentBlob->Size
> +               CurrentBlob->Size,
> +               FetchStatus
>                  );
>       if (EFI_ERROR (Status)) {
>         goto FreeBlobs;


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119398): https://edk2.groups.io/g/devel/message/119398
Mute This Topic: https://groups.io/mt/105977015/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-