public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [edk2-devel] [PATCH 0/2] AmdSev: Harden SEV Kernel hashes verifier
@ 2024-05-06 20:27 Tobin Feldman-Fitzthum
  2024-05-06 20:27 ` [edk2-devel] [PATCH 1/2] AmdSev: Rework Blob Verifier Tobin Feldman-Fitzthum
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Tobin Feldman-Fitzthum @ 2024-05-06 20:27 UTC (permalink / raw)
  To: devel
  Cc: dov.murik, james.bottomley, thomas.lendacky, tobin,
	Tobin Feldman-Fitzthum

The AmdSev package has a so-called BlobVerifier, which
is meant to extend the TCB of a confidential guest
(SEV or SNP) to include components provided via fw_cfg
such as initrd, kernel, kernel params.

This series fixes a few implementation errors in the
blob verifier. One common theme is that the verifier
currently fails to halt the boot when an invalid blob
is detected. This can lead to a confidential guest
having a launch measurement that does not reflect the
guest TCB.

This series could also help us move towards consolidating
the AmdSev package back into the OvmfPkg although more
discussion will be needed on this.

Thank you for Ryan Savino at AMD for pointing out
some of these issues.

Tobin Feldman-Fitzthum (2):
  AmdSev: Rework Blob Verifier
  AmdSev: Halt on failed blob allocation

 .../BlobVerifierSevHashes.c                   | 56 ++++++++++++++++---
 OvmfPkg/Include/Library/BlobVerifierLib.h     | 14 +++--
 .../BlobVerifierLibNull/BlobVerifierNull.c    | 13 +++--
 .../QemuKernelLoaderFsDxe.c                   |  9 ++-
 4 files changed, 69 insertions(+), 23 deletions(-)

-- 
2.34.1



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



^ permalink raw reply	[flat|nested] 9+ messages in thread

* [edk2-devel] [PATCH 1/2] AmdSev: Rework Blob Verifier
  2024-05-06 20:27 [edk2-devel] [PATCH 0/2] AmdSev: Harden SEV Kernel hashes verifier Tobin Feldman-Fitzthum
@ 2024-05-06 20:27 ` Tobin Feldman-Fitzthum
  2024-05-30 15:46   ` Lendacky, Thomas via groups.io
  2024-05-06 20:27 ` [edk2-devel] [PATCH 2/2] AmdSev: Halt on failed blob allocation Tobin Feldman-Fitzthum
  2024-06-26  8:08 ` [edk2-devel] [PATCH 0/2] AmdSev: Harden SEV Kernel hashes verifier Aithal, Srikanth via groups.io
  2 siblings, 1 reply; 9+ messages in thread
From: Tobin Feldman-Fitzthum @ 2024-05-06 20:27 UTC (permalink / raw)
  To: devel
  Cc: dov.murik, james.bottomley, thomas.lendacky, tobin,
	Tobin Feldman-Fitzthum

The Blob Verifier checks boot artifacts against a hash table
injected by the hypervisor and measured by hardware.

Update the Blob Verifier to enter a dead loop if the artifacts
do not match.

Signed-off-by: Tobin Feldman-Fitzthum <tobin@linux.ibm.com>
---
 .../BlobVerifierSevHashes.c                   | 39 +++++++++++++++----
 1 file changed, 31 insertions(+), 8 deletions(-)

diff --git a/OvmfPkg/AmdSev/BlobVerifierLibSevHashes/BlobVerifierSevHashes.c b/OvmfPkg/AmdSev/BlobVerifierLibSevHashes/BlobVerifierSevHashes.c
index 2e58794c3c..ee8bca509a 100644
--- a/OvmfPkg/AmdSev/BlobVerifierLibSevHashes/BlobVerifierSevHashes.c
+++ b/OvmfPkg/AmdSev/BlobVerifierLibSevHashes/BlobVerifierSevHashes.c
@@ -77,13 +77,17 @@ FindBlobEntryGuid (
 /**
   Verify blob from an external source.
 
+  If a non-secure configuration is detected this function will enter a
+  dead loop to prevent a boot.
+
   @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
 
-  @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
@@ -99,8 +103,8 @@ VerifyBlob (
 
   if ((mHashesTable == NULL) || (mHashesTableSize == 0)) {
     DEBUG ((
-      DEBUG_ERROR,
-      "%a: Verifier called but no hashes table discoverd in MEMFD\n",
+      DEBUG_WARN,
+      "%a: No hashes table discovered in MEMFD\n",
       __func__
       ));
     return EFI_ACCESS_DENIED;
@@ -114,7 +118,8 @@ VerifyBlob (
       __func__,
       BlobName
       ));
-    return EFI_ACCESS_DENIED;
+
+    CpuDeadLoop ();
   }
 
   //
@@ -136,10 +141,22 @@ VerifyBlob (
 
     DEBUG ((DEBUG_INFO, "%a: Found GUID %g in table\n", __func__, Guid));
 
+    if (BufSize == 0) {
+      DEBUG ((
+        DEBUG_ERROR,
+        "%a: Blob Specified in Hash Table was not Provided",
+        __func__,
+        EntrySize,
+        SHA256_DIGEST_SIZE
+        ));
+
+      CpuDeadLoop ();
+    }
+
     EntrySize = Entry->Len - sizeof Entry->Guid - sizeof Entry->Len;
     if (EntrySize != SHA256_DIGEST_SIZE) {
       DEBUG ((
-        DEBUG_ERROR,
+        DEBUG_WARN,
         "%a: Hash has the wrong size %d != %d\n",
         __func__,
         EntrySize,
@@ -170,18 +187,24 @@ VerifyBlob (
         __func__,
         BlobName
         ));
+
+      CpuDeadLoop ();
     }
 
     return Status;
   }
 
+  //
+  // If the GUID is not in the hash table, execution can still continue.
+  // This blob will not be measured, but at least one blob must be.
+  //
   DEBUG ((
     DEBUG_ERROR,
     "%a: Hash GUID %g not found in table\n",
     __func__,
     Guid
     ));
-  return EFI_ACCESS_DENIED;
+  return EFI_SUCCESS;
 }
 
 /**
-- 
2.34.1



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



^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [edk2-devel] [PATCH 2/2] AmdSev: Halt on failed blob allocation
  2024-05-06 20:27 [edk2-devel] [PATCH 0/2] AmdSev: Harden SEV Kernel hashes verifier Tobin Feldman-Fitzthum
  2024-05-06 20:27 ` [edk2-devel] [PATCH 1/2] AmdSev: Rework Blob Verifier Tobin Feldman-Fitzthum
@ 2024-05-06 20:27 ` Tobin Feldman-Fitzthum
  2024-05-30 15:51   ` Lendacky, Thomas via groups.io
  2024-06-26  8:08 ` [edk2-devel] [PATCH 0/2] AmdSev: Harden SEV Kernel hashes verifier Aithal, Srikanth via groups.io
  2 siblings, 1 reply; 9+ messages in thread
From: Tobin Feldman-Fitzthum @ 2024-05-06 20:27 UTC (permalink / raw)
  To: devel
  Cc: dov.murik, james.bottomley, thomas.lendacky, tobin,
	Tobin Feldman-Fitzthum

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.
+
   @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;
-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#118663): https://edk2.groups.io/g/devel/message/118663
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]
-=-=-=-=-=-=-=-=-=-=-=-



^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [edk2-devel] [PATCH 1/2] AmdSev: Rework Blob Verifier
  2024-05-06 20:27 ` [edk2-devel] [PATCH 1/2] AmdSev: Rework Blob Verifier Tobin Feldman-Fitzthum
@ 2024-05-30 15:46   ` Lendacky, Thomas via groups.io
  0 siblings, 0 replies; 9+ messages in thread
From: Lendacky, Thomas via groups.io @ 2024-05-30 15:46 UTC (permalink / raw)
  To: Tobin Feldman-Fitzthum, devel, Gerd Hoffmann, Ard Biesheuvel
  Cc: dov.murik, james.bottomley, tobin

On 5/6/24 15:27, Tobin Feldman-Fitzthum wrote:
> The Blob Verifier checks boot artifacts against a hash table
> injected by the hypervisor and measured by hardware.
> 
> Update the Blob Verifier to enter a dead loop if the artifacts
> do not match.

There are some changes to messages from ERROR to WARN and the return is 
kept as EFI_ACCESS_DENIED, so it would be best to describe in the commit 
message what situations result in EFI_ACCES_DENIED vs dead loop.

> 
> Signed-off-by: Tobin Feldman-Fitzthum <tobin@linux.ibm.com>
> ---
>   .../BlobVerifierSevHashes.c                   | 39 +++++++++++++++----
>   1 file changed, 31 insertions(+), 8 deletions(-)
> 
> diff --git a/OvmfPkg/AmdSev/BlobVerifierLibSevHashes/BlobVerifierSevHashes.c b/OvmfPkg/AmdSev/BlobVerifierLibSevHashes/BlobVerifierSevHashes.c
> index 2e58794c3c..ee8bca509a 100644
> --- a/OvmfPkg/AmdSev/BlobVerifierLibSevHashes/BlobVerifierSevHashes.c
> +++ b/OvmfPkg/AmdSev/BlobVerifierLibSevHashes/BlobVerifierSevHashes.c
> @@ -77,13 +77,17 @@ FindBlobEntryGuid (
>   /**
>     Verify blob from an external source.
>   
> +  If a non-secure configuration is detected this function will enter a
> +  dead loop to prevent a boot.
> +
>     @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
>   
> -  @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
> @@ -99,8 +103,8 @@ VerifyBlob (
>   
>     if ((mHashesTable == NULL) || (mHashesTableSize == 0)) {
>       DEBUG ((
> -      DEBUG_ERROR,
> -      "%a: Verifier called but no hashes table discoverd in MEMFD\n",
> +      DEBUG_WARN,
> +      "%a: No hashes table discovered in MEMFD\n",

Technically, this should really just change the ERROR to WARN without 
changing the message text.

>         __func__
>         ));
>       return EFI_ACCESS_DENIED;
> @@ -114,7 +118,8 @@ VerifyBlob (
>         __func__,
>         BlobName
>         ));
> -    return EFI_ACCESS_DENIED;
> +
> +    CpuDeadLoop ();
>     }
>   
>     //
> @@ -136,10 +141,22 @@ VerifyBlob (
>   
>       DEBUG ((DEBUG_INFO, "%a: Found GUID %g in table\n", __func__, Guid));
>   
> +    if (BufSize == 0) {
> +      DEBUG ((
> +        DEBUG_ERROR,
> +        "%a: Blob Specified in Hash Table was not Provided",
> +        __func__,
> +        EntrySize,
> +        SHA256_DIGEST_SIZE

Looks like there's no substitution spots in the message for these last 
two parameters.

Thanks,
Tom

> +        ));
> +
> +      CpuDeadLoop ();
> +    }
> +
>       EntrySize = Entry->Len - sizeof Entry->Guid - sizeof Entry->Len;
>       if (EntrySize != SHA256_DIGEST_SIZE) {
>         DEBUG ((
> -        DEBUG_ERROR,
> +        DEBUG_WARN,
>           "%a: Hash has the wrong size %d != %d\n",
>           __func__,
>           EntrySize,
> @@ -170,18 +187,24 @@ VerifyBlob (
>           __func__,
>           BlobName
>           ));
> +
> +      CpuDeadLoop ();
>       }
>   
>       return Status;
>     }
>   
> +  //
> +  // If the GUID is not in the hash table, execution can still continue.
> +  // This blob will not be measured, but at least one blob must be.
> +  //
>     DEBUG ((
>       DEBUG_ERROR,
>       "%a: Hash GUID %g not found in table\n",
>       __func__,
>       Guid
>       ));
> -  return EFI_ACCESS_DENIED;
> +  return EFI_SUCCESS;
>   }
>   
>   /**


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



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [edk2-devel] [PATCH 2/2] AmdSev: Halt on failed blob allocation
  2024-05-06 20:27 ` [edk2-devel] [PATCH 2/2] AmdSev: Halt on failed blob allocation Tobin Feldman-Fitzthum
@ 2024-05-30 15:51   ` Lendacky, Thomas via groups.io
  0 siblings, 0 replies; 9+ messages in thread
From: Lendacky, Thomas via groups.io @ 2024-05-30 15:51 UTC (permalink / raw)
  To: Tobin Feldman-Fitzthum, devel, Gerd Hoffmann, Ard Biesheuvel
  Cc: dov.murik, james.bottomley, tobin

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]
-=-=-=-=-=-=-=-=-=-=-=-



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [edk2-devel] [PATCH 0/2] AmdSev: Harden SEV Kernel hashes verifier
  2024-05-06 20:27 [edk2-devel] [PATCH 0/2] AmdSev: Harden SEV Kernel hashes verifier Tobin Feldman-Fitzthum
  2024-05-06 20:27 ` [edk2-devel] [PATCH 1/2] AmdSev: Rework Blob Verifier Tobin Feldman-Fitzthum
  2024-05-06 20:27 ` [edk2-devel] [PATCH 2/2] AmdSev: Halt on failed blob allocation Tobin Feldman-Fitzthum
@ 2024-06-26  8:08 ` Aithal, Srikanth via groups.io
  2024-06-26 13:58   ` Tobin Feldman-Fitzthum
  2 siblings, 1 reply; 9+ messages in thread
From: Aithal, Srikanth via groups.io @ 2024-06-26  8:08 UTC (permalink / raw)
  To: devel, tobin; +Cc: dov.murik, james.bottomley, thomas.lendacky, tobin

[-- Attachment #1: Type: text/plain, Size: 3219 bytes --]

Hello,

SEV/SEVES guest boot fails with AMDSEV OVMF package built using upstream 
edk2 master [commit head: 2fbaaa96d11ad61a9133df1728e3fe965d1457a5].

SEV/SEVES guest boot with AMDSEV package gets stuck at below point:

Plain Text

|2024-06-26 04:38:02: FetchBlob: loading 14332416 bytes for "kernel" 
2024-06-26 04:38:02: Select Item: 0x18 2024-06-26 04:38:02: Select Item: 
0x11 2024-06-26 04:38:02: VerifyBlob: Found GUID 
4DE79437-ABD2-427F-B835-D5B172D2045B in table 2024-06-26 04:38:02: 
VerifyBlob: Hash comparison succeeded for "kernel" 2024-06-26 04:38:02: 
Select Item: 0xB 2024-06-26 04:38:02: VerifyBlob: Found GUID 
44BAF731-3A2F-4BD7-9AF1-41E29169781D in table 2024-06-26 04:38:02: 
VerifyBlob: Blob Specified in Hash Table was not Provided --> Hung here |

This was working until yesterday [commit head: be38c01], where we can 
see boot was proceeding

Plain Text

|2024-06-25 03:13:23: VerifyBlob: Found GUID 
4DE79437-ABD2-427F-B835-D5B172D2045B in table 2024-06-25 03:13:23: 
VerifyBlob: Hash comparison succeeded for "kernel" 2024-06-25 03:13:23: 
Select Item: 0xB 2024-06-25 03:13:23: VerifyBlob: Found GUID 
44BAF731-3A2F-4BD7-9AF1-41E29169781D in table 2024-06-25 03:13:23: 
VerifyBlob: Hash comparison succeeded for "initrd" 2024-06-25 03:13:23: 
Select Item: 0x14 2024-06-25 03:13:23: FetchBlob: loading 120 bytes for 
"cmdline" 2024-06-25 03:13:23: Select Item: 0x15 2024-06-25 03:13:23: 
VerifyBlob: Found GUID 97D02DD8-BD20-4C94-AA78-E7714D36AB2A in table 
2024-06-25 03:13:23: VerifyBlob: Hash comparison succeeded for "cmdline"|


After this patch got merged the regression is seen.

Thanks,

Srikanth Aithal


On 5/7/2024 1:57 AM, Tobin Feldman-Fitzthum via groups.io wrote:
> The AmdSev package has a so-called BlobVerifier, which
> is meant to extend the TCB of a confidential guest
> (SEV or SNP) to include components provided via fw_cfg
> such as initrd, kernel, kernel params.
>
> This series fixes a few implementation errors in the
> blob verifier. One common theme is that the verifier
> currently fails to halt the boot when an invalid blob
> is detected. This can lead to a confidential guest
> having a launch measurement that does not reflect the
> guest TCB.
>
> This series could also help us move towards consolidating
> the AmdSev package back into the OvmfPkg although more
> discussion will be needed on this.
>
> Thank you for Ryan Savino at AMD for pointing out
> some of these issues.
>
> Tobin Feldman-Fitzthum (2):
>    AmdSev: Rework Blob Verifier
>    AmdSev: Halt on failed blob allocation
>
>   .../BlobVerifierSevHashes.c                   | 56 ++++++++++++++++---
>   OvmfPkg/Include/Library/BlobVerifierLib.h     | 14 +++--
>   .../BlobVerifierLibNull/BlobVerifierNull.c    | 13 +++--
>   .../QemuKernelLoaderFsDxe.c                   |  9 ++-
>   4 files changed, 69 insertions(+), 23 deletions(-)
>


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



[-- Attachment #2: Type: text/html, Size: 6450 bytes --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [edk2-devel] [PATCH 0/2] AmdSev: Harden SEV Kernel hashes verifier
  2024-06-26  8:08 ` [edk2-devel] [PATCH 0/2] AmdSev: Harden SEV Kernel hashes verifier Aithal, Srikanth via groups.io
@ 2024-06-26 13:58   ` Tobin Feldman-Fitzthum
  2024-06-26 14:33     ` Aithal, Srikanth via groups.io
  0 siblings, 1 reply; 9+ messages in thread
From: Tobin Feldman-Fitzthum @ 2024-06-26 13:58 UTC (permalink / raw)
  To: Aithal, Srikanth, devel
  Cc: dov.murik, james.bottomley, thomas.lendacky, tobin



On 6/26/24 4:08 AM, Aithal, Srikanth wrote:
> Hello, SEV/SEVES guest boot fails with AMDSEV OVMF package built using 
> upstream edk2 master [commit head: 
> 2fbaaa96d11ad61a9133df1728e3fe965d1457a5]. SEV/SEVES guest boot with 
> AMDSEV package gets stuck at below point: Plain Text 2024-06-26 04: 38: 
>  02: 
> 
> 
> Hello,
> 
> SEV/SEVES guest boot fails with AMDSEV OVMF package built using upstream 
> edk2 master [commit head: 2fbaaa96d11ad61a9133df1728e3fe965d1457a5].
> 
> SEV/SEVES guest boot with AMDSEV package gets stuck at below point:
> 
> Plain Text
> 
> |2024-06-26 04:38:02: FetchBlob: loading 14332416 bytes for "kernel" 
> 2024-06-26 04:38:02: Select Item: 0x18 2024-06-26 04:38:02: Select Item: 
> 0x11 2024-06-26 04:38:02: VerifyBlob: Found GUID 
> 4DE79437-ABD2-427F-B835-D5B172D2045B in table 2024-06-26 04:38:02: 
> VerifyBlob: Hash comparison succeeded for "kernel" 2024-06-26 04:38:02: 
> Select Item: 0xB 2024-06-26 04:38:02: VerifyBlob: Found GUID 
> 44BAF731-3A2F-4BD7-9AF1-41E29169781D in table 2024-06-26 04:38:02: 
> VerifyBlob: Blob Specified in Hash Table was not Provided --> Hung here |
> 
Can you send qemu command that produces this case?
It looks like what is happening is that the initrd is specified in the 
hash table, but is not provided to the guest.
You might try with and without the -initrd option and compare.
It's possible that when -initrd is not provided, QEMU will still put 
something in the table, which this patch might not account for.

-Tobin
> This was working until yesterday [commit head: be38c01], where we can 
> see boot was proceeding
> 
> Plain Text
> 
> |2024-06-25 03:13:23: VerifyBlob: Found GUID 
> 4DE79437-ABD2-427F-B835-D5B172D2045B in table 2024-06-25 03:13:23: 
> VerifyBlob: Hash comparison succeeded for "kernel" 2024-06-25 03:13:23: 
> Select Item: 0xB 2024-06-25 03:13:23: VerifyBlob: Found GUID 
> 44BAF731-3A2F-4BD7-9AF1-41E29169781D in table 2024-06-25 03:13:23: 
> VerifyBlob: Hash comparison succeeded for "initrd" 2024-06-25 03:13:23: 
> Select Item: 0x14 2024-06-25 03:13:23: FetchBlob: loading 120 bytes for 
> "cmdline" 2024-06-25 03:13:23: Select Item: 0x15 2024-06-25 03:13:23: 
> VerifyBlob: Found GUID 97D02DD8-BD20-4C94-AA78-E7714D36AB2A in table 
> 2024-06-25 03:13:23: VerifyBlob: Hash comparison succeeded for "cmdline"|
> 
> 
> After this patch got merged the regression is seen.
> 
> Thanks,
> 
> Srikanth Aithal
> 
> 
> On 5/7/2024 1:57 AM, Tobin Feldman-Fitzthum via groups.io wrote:
>> The AmdSev package has a so-called BlobVerifier, which
>> is meant to extend the TCB of a confidential guest
>> (SEV or SNP) to include components provided via fw_cfg
>> such as initrd, kernel, kernel params.
>>
>> This series fixes a few implementation errors in the
>> blob verifier. One common theme is that the verifier
>> currently fails to halt the boot when an invalid blob
>> is detected. This can lead to a confidential guest
>> having a launch measurement that does not reflect the
>> guest TCB.
>>
>> This series could also help us move towards consolidating
>> the AmdSev package back into the OvmfPkg although more
>> discussion will be needed on this.
>>
>> Thank you for Ryan Savino at AMD for pointing out
>> some of these issues.
>>
>> Tobin Feldman-Fitzthum (2):
>>    AmdSev: Rework Blob Verifier
>>    AmdSev: Halt on failed blob allocation
>>
>>   .../BlobVerifierSevHashes.c                   | 56 ++++++++++++++++---
>>   OvmfPkg/Include/Library/BlobVerifierLib.h     | 14 +++--
>>   .../BlobVerifierLibNull/BlobVerifierNull.c    | 13 +++--
>>   .../QemuKernelLoaderFsDxe.c                   |  9 ++-
>>   4 files changed, 69 insertions(+), 23 deletions(-)
>>


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



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [edk2-devel] [PATCH 0/2] AmdSev: Harden SEV Kernel hashes verifier
  2024-06-26 13:58   ` Tobin Feldman-Fitzthum
@ 2024-06-26 14:33     ` Aithal, Srikanth via groups.io
  2024-06-26 17:14       ` Tobin Feldman-Fitzthum
  0 siblings, 1 reply; 9+ messages in thread
From: Aithal, Srikanth via groups.io @ 2024-06-26 14:33 UTC (permalink / raw)
  To: Tobin Feldman-Fitzthum, devel
  Cc: dov.murik, james.bottomley, thomas.lendacky, tobin


On 6/26/2024 7:28 PM, Tobin Feldman-Fitzthum wrote:
>
>
> On 6/26/24 4:08 AM, Aithal, Srikanth wrote:
>> Hello, SEV/SEVES guest boot fails with AMDSEV OVMF package built 
>> using upstream edk2 master [commit head: 
>> 2fbaaa96d11ad61a9133df1728e3fe965d1457a5]. SEV/SEVES guest boot with 
>> AMDSEV package gets stuck at below point: Plain Text 2024-06-26 
>> 04: 38:  02: 
>>
>>
>> Hello,
>>
>> SEV/SEVES guest boot fails with AMDSEV OVMF package built using 
>> upstream edk2 master [commit 
>> head: 2fbaaa96d11ad61a9133df1728e3fe965d1457a5].
>>
>> SEV/SEVES guest boot with AMDSEV package gets stuck at below point:
>>
>> Plain Text
>>
>> |2024-06-26 04:38:02: FetchBlob: loading 14332416 bytes for "kernel" 
>> 2024-06-26 04:38:02: Select Item: 0x18 2024-06-26 04:38:02: Select 
>> Item: 0x11 2024-06-26 04:38:02: VerifyBlob: Found GUID 
>> 4DE79437-ABD2-427F-B835-D5B172D2045B in table 2024-06-26 04:38:02: 
>> VerifyBlob: Hash comparison succeeded for "kernel" 2024-06-26 
>> 04:38:02: Select Item: 0xB 2024-06-26 04:38:02: VerifyBlob: Found 
>> GUID 44BAF731-3A2F-4BD7-9AF1-41E29169781D in table 2024-06-26 
>> 04:38:02: VerifyBlob: Blob Specified in Hash Table was not Provided 
>> --> Hung here |
>>
> Can you send qemu command that produces this case?
> It looks like what is happening is that the initrd is specified in the 
> hash table, but is not provided to the guest.
> You might try with and without the -initrd option and compare.
> It's possible that when -initrd is not provided, QEMU will still put 
> something in the table, which this patch might not account for.
>
> -Tobin
my qemu command line to recreate the issue:

qemu-system-x86_64 \
-machine q35,confidential-guest-support=sev0,vmport=off \
-object sev-guest,id=sev0,cbitpos=51,reduced-phys-bits=1,kernel-hashes=on \
-name guest=vm,debug-threads=on \
-drive 
if=pflash,format=raw,unit=0,file=/VT_BUILD/OVMF_AmdSev/OVMF.fd,readonly \
-cpu host \
-m 4096 \
-kernel '/VT_BUILD/usr/local/bzImage' \
-append 'root=/dev/sda rw console=ttyS0,115200n8 
earlyprintk=ttyS0,115200 net.ifnames=0 biosdevname=0 movable_node 
swiotlb=65536' \
-smp 1,cores=1,threads=1,dies=1,sockets=1 \
-drive 
file=/VT_BUILD/images/22.04-server.qcow2,index=0,media=disk,format=qcow2 \
--enable-kvm \
--nographic


Yes, if I pass -initrd explicitly the Hash verify step passes properly

FetchBlob: loading 14332416 bytes for "kernel"
Select Item: 0x18
Select Item: 0x11
VerifyBlob: Found GUID 4DE79437-ABD2-427F-B835-D5B172D2045B in table
VerifyBlob: Hash comparison succeeded for "kernel"
Select Item: 0xB
FetchBlob: loading 75379943 bytes for "initrd"
Select Item: 0x12
VerifyBlob: Found GUID 44BAF731-3A2F-4BD7-9AF1-41E29169781D in table
VerifyBlob: Hash comparison succeeded for "initrd"
Select Item: 0x14
FetchBlob: loading 120 bytes for "cmdline"
Select Item: 0x15
VerifyBlob: Found GUID 97D02DD8-BD20-4C94-AA78-E7714D36AB2A in table
VerifyBlob: Hash comparison succeeded for "cmdline"

>> This was working until yesterday [commit head: be38c01], where we can 
>> see boot was proceeding
>>
>> Plain Text
>>
>> |2024-06-25 03:13:23: VerifyBlob: Found GUID 
>> 4DE79437-ABD2-427F-B835-D5B172D2045B in table 2024-06-25 03:13:23: 
>> VerifyBlob: Hash comparison succeeded for "kernel" 2024-06-25 
>> 03:13:23: Select Item: 0xB 2024-06-25 03:13:23: VerifyBlob: Found 
>> GUID 44BAF731-3A2F-4BD7-9AF1-41E29169781D in table 2024-06-25 
>> 03:13:23: VerifyBlob: Hash comparison succeeded for "initrd" 
>> 2024-06-25 03:13:23: Select Item: 0x14 2024-06-25 03:13:23: 
>> FetchBlob: loading 120 bytes for "cmdline" 2024-06-25 03:13:23: 
>> Select Item: 0x15 2024-06-25 03:13:23: VerifyBlob: Found GUID 
>> 97D02DD8-BD20-4C94-AA78-E7714D36AB2A in table 2024-06-25 03:13:23: 
>> VerifyBlob: Hash comparison succeeded for "cmdline"|
>>
>>
>> After this patch got merged the regression is seen.
>>
>> Thanks,
>>
>> Srikanth Aithal
>>
>>
>> On 5/7/2024 1:57 AM, Tobin Feldman-Fitzthum via groups.io wrote:
>>> The AmdSev package has a so-called BlobVerifier, which
>>> is meant to extend the TCB of a confidential guest
>>> (SEV or SNP) to include components provided via fw_cfg
>>> such as initrd, kernel, kernel params.
>>>
>>> This series fixes a few implementation errors in the
>>> blob verifier. One common theme is that the verifier
>>> currently fails to halt the boot when an invalid blob
>>> is detected. This can lead to a confidential guest
>>> having a launch measurement that does not reflect the
>>> guest TCB.
>>>
>>> This series could also help us move towards consolidating
>>> the AmdSev package back into the OvmfPkg although more
>>> discussion will be needed on this.
>>>
>>> Thank you for Ryan Savino at AMD for pointing out
>>> some of these issues.
>>>
>>> Tobin Feldman-Fitzthum (2):
>>>    AmdSev: Rework Blob Verifier
>>>    AmdSev: Halt on failed blob allocation
>>>
>>>   .../BlobVerifierSevHashes.c                   | 56 
>>> ++++++++++++++++---
>>>   OvmfPkg/Include/Library/BlobVerifierLib.h     | 14 +++--
>>>   .../BlobVerifierLibNull/BlobVerifierNull.c    | 13 +++--
>>>   .../QemuKernelLoaderFsDxe.c                   |  9 ++-
>>>   4 files changed, 69 insertions(+), 23 deletions(-)
>>>


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



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [edk2-devel] [PATCH 0/2] AmdSev: Harden SEV Kernel hashes verifier
  2024-06-26 14:33     ` Aithal, Srikanth via groups.io
@ 2024-06-26 17:14       ` Tobin Feldman-Fitzthum
  0 siblings, 0 replies; 9+ messages in thread
From: Tobin Feldman-Fitzthum @ 2024-06-26 17:14 UTC (permalink / raw)
  To: Aithal, Srikanth, devel
  Cc: dov.murik, james.bottomley, thomas.lendacky, tobin



On 6/26/24 10:33 AM, Aithal, Srikanth wrote:
> 
> On 6/26/2024 7:28 PM, Tobin Feldman-Fitzthum wrote:
>>
>>
>> On 6/26/24 4:08 AM, Aithal, Srikanth wrote:
>>> Hello, SEV/SEVES guest boot fails with AMDSEV OVMF package built 
>>> using upstream edk2 master [commit head: 
>>> 2fbaaa96d11ad61a9133df1728e3fe965d1457a5]. SEV/SEVES guest boot with 
>>> AMDSEV package gets stuck at below point: Plain Text 2024-06-26 04: 
>>>  38:  02: 
>>>
>>>
>>> Hello,
>>>
>>> SEV/SEVES guest boot fails with AMDSEV OVMF package built using 
>>> upstream edk2 master [commit 
>>> head: 2fbaaa96d11ad61a9133df1728e3fe965d1457a5].
>>>
>>> SEV/SEVES guest boot with AMDSEV package gets stuck at below point:
>>>
>>> Plain Text
>>>
>>> |2024-06-26 04:38:02: FetchBlob: loading 14332416 bytes for "kernel" 
>>> 2024-06-26 04:38:02: Select Item: 0x18 2024-06-26 04:38:02: Select 
>>> Item: 0x11 2024-06-26 04:38:02: VerifyBlob: Found GUID 
>>> 4DE79437-ABD2-427F-B835-D5B172D2045B in table 2024-06-26 04:38:02: 
>>> VerifyBlob: Hash comparison succeeded for "kernel" 2024-06-26 
>>> 04:38:02: Select Item: 0xB 2024-06-26 04:38:02: VerifyBlob: Found 
>>> GUID 44BAF731-3A2F-4BD7-9AF1-41E29169781D in table 2024-06-26 
>>> 04:38:02: VerifyBlob: Blob Specified in Hash Table was not Provided 
>>> --> Hung here |
>>>
>> Can you send qemu command that produces this case?
>> It looks like what is happening is that the initrd is specified in the 
>> hash table, but is not provided to the guest.
>> You might try with and without the -initrd option and compare.
>> It's possible that when -initrd is not provided, QEMU will still put 
>> something in the table, which this patch might not account for.
>>
>> -Tobin
> my qemu command line to recreate the issue:
> 
> qemu-system-x86_64 \
> -machine q35,confidential-guest-support=sev0,vmport=off \
> -object sev-guest,id=sev0,cbitpos=51,reduced-phys-bits=1,kernel-hashes=on \
> -name guest=vm,debug-threads=on \
> -drive 
> if=pflash,format=raw,unit=0,file=/VT_BUILD/OVMF_AmdSev/OVMF.fd,readonly \
> -cpu host \
> -m 4096 \
> -kernel '/VT_BUILD/usr/local/bzImage' \
> -append 'root=/dev/sda rw console=ttyS0,115200n8 
> earlyprintk=ttyS0,115200 net.ifnames=0 biosdevname=0 movable_node 
> swiotlb=65536' \
> -smp 1,cores=1,threads=1,dies=1,sockets=1 \
> -drive 
> file=/VT_BUILD/images/22.04-server.qcow2,index=0,media=disk,format=qcow2 \
> --enable-kvm \
> --nographic
> 
> 
> Yes, if I pass -initrd explicitly the Hash verify step passes properly
> 
> FetchBlob: loading 14332416 bytes for "kernel"
> Select Item: 0x18
> Select Item: 0x11
> VerifyBlob: Found GUID 4DE79437-ABD2-427F-B835-D5B172D2045B in table
> VerifyBlob: Hash comparison succeeded for "kernel"
> Select Item: 0xB
> FetchBlob: loading 75379943 bytes for "initrd"
> Select Item: 0x12
> VerifyBlob: Found GUID 44BAF731-3A2F-4BD7-9AF1-41E29169781D in table
> VerifyBlob: Hash comparison succeeded for "initrd"
> Select Item: 0x14
> FetchBlob: loading 120 bytes for "cmdline"
> Select Item: 0x15
> VerifyBlob: Found GUID 97D02DD8-BD20-4C94-AA78-E7714D36AB2A in table
> VerifyBlob: Hash comparison succeeded for "cmdline"
> 
Ok, I'll submit a little tweak this afternoon that should fix your case.

-Tobin
>>> This was working until yesterday [commit head: be38c01], where we can 
>>> see boot was proceeding
>>>
>>> Plain Text
>>>
>>> |2024-06-25 03:13:23: VerifyBlob: Found GUID 
>>> 4DE79437-ABD2-427F-B835-D5B172D2045B in table 2024-06-25 03:13:23: 
>>> VerifyBlob: Hash comparison succeeded for "kernel" 2024-06-25 
>>> 03:13:23: Select Item: 0xB 2024-06-25 03:13:23: VerifyBlob: Found 
>>> GUID 44BAF731-3A2F-4BD7-9AF1-41E29169781D in table 2024-06-25 
>>> 03:13:23: VerifyBlob: Hash comparison succeeded for "initrd" 
>>> 2024-06-25 03:13:23: Select Item: 0x14 2024-06-25 03:13:23: 
>>> FetchBlob: loading 120 bytes for "cmdline" 2024-06-25 03:13:23: 
>>> Select Item: 0x15 2024-06-25 03:13:23: VerifyBlob: Found GUID 
>>> 97D02DD8-BD20-4C94-AA78-E7714D36AB2A in table 2024-06-25 03:13:23: 
>>> VerifyBlob: Hash comparison succeeded for "cmdline"|
>>>
>>>
>>> After this patch got merged the regression is seen.
>>>
>>> Thanks,
>>>
>>> Srikanth Aithal
>>>
>>>
>>> On 5/7/2024 1:57 AM, Tobin Feldman-Fitzthum via groups.io wrote:
>>>> The AmdSev package has a so-called BlobVerifier, which
>>>> is meant to extend the TCB of a confidential guest
>>>> (SEV or SNP) to include components provided via fw_cfg
>>>> such as initrd, kernel, kernel params.
>>>>
>>>> This series fixes a few implementation errors in the
>>>> blob verifier. One common theme is that the verifier
>>>> currently fails to halt the boot when an invalid blob
>>>> is detected. This can lead to a confidential guest
>>>> having a launch measurement that does not reflect the
>>>> guest TCB.
>>>>
>>>> This series could also help us move towards consolidating
>>>> the AmdSev package back into the OvmfPkg although more
>>>> discussion will be needed on this.
>>>>
>>>> Thank you for Ryan Savino at AMD for pointing out
>>>> some of these issues.
>>>>
>>>> Tobin Feldman-Fitzthum (2):
>>>>    AmdSev: Rework Blob Verifier
>>>>    AmdSev: Halt on failed blob allocation
>>>>
>>>>   .../BlobVerifierSevHashes.c                   | 56 
>>>> ++++++++++++++++---
>>>>   OvmfPkg/Include/Library/BlobVerifierLib.h     | 14 +++--
>>>>   .../BlobVerifierLibNull/BlobVerifierNull.c    | 13 +++--
>>>>   .../QemuKernelLoaderFsDxe.c                   |  9 ++-
>>>>   4 files changed, 69 insertions(+), 23 deletions(-)
>>>>


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



^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2024-06-27 22:42 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-05-06 20:27 [edk2-devel] [PATCH 0/2] AmdSev: Harden SEV Kernel hashes verifier Tobin Feldman-Fitzthum
2024-05-06 20:27 ` [edk2-devel] [PATCH 1/2] AmdSev: Rework Blob Verifier Tobin Feldman-Fitzthum
2024-05-30 15:46   ` Lendacky, Thomas via groups.io
2024-05-06 20:27 ` [edk2-devel] [PATCH 2/2] AmdSev: Halt on failed blob allocation Tobin Feldman-Fitzthum
2024-05-30 15:51   ` Lendacky, Thomas via groups.io
2024-06-26  8:08 ` [edk2-devel] [PATCH 0/2] AmdSev: Harden SEV Kernel hashes verifier Aithal, Srikanth via groups.io
2024-06-26 13:58   ` Tobin Feldman-Fitzthum
2024-06-26 14:33     ` Aithal, Srikanth via groups.io
2024-06-26 17:14       ` Tobin Feldman-Fitzthum

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox