Re: [edk2-devel] [PATCH 1/2] AmdSev: Rework Blob Verifier

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

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