Re: [edk2-devel] [PATCH 2/2] AmdSev: Halt on failed blob allocation
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
---
.../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] BufThe data of the blob
@param[in] BufSizeThe size of the blob in bytes
+ @param[in] FetchStatusThe 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 UINT32BufSize
+ IN UINT32BufSize,
+ IN EFI_STATUSFetchStatus
)
{
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] BufThe data of the blob
@param[in] BufSizeThe size of the blob in bytes
+ @param[in] FetchStatusThe 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 UINT32BufSize
+ IN UINT32BufSize,
+ IN EFI_STATUSFetchStatus
);
#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] BufThe data of the blob
@param[in] BufSizeThe size of the blob in bytes
+ @param[in] FetchStatusThe 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 UINT32BufSize
+ IN UINT32BufSize,
+ IN EFI_STATUSFetchStatus
)
{
- return EFI_SUCCESS;
+ return FetchStatus;
}
diff --git a/OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c
b/OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c
index
[edk2-devel] [PATCH 2/2] AmdSev: Halt on failed blob allocation
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
---
.../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] BufThe data of the blob
@param[in] BufSizeThe size of the blob in bytes
+ @param[in] FetchStatusThe 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 UINT32BufSize
+ IN UINT32BufSize,
+ IN EFI_STATUSFetchStatus
)
{
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] BufThe data of the blob
@param[in] BufSizeThe size of the blob in bytes
+ @param[in] FetchStatusThe 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 UINT32BufSize
+ IN UINT32BufSize,
+ IN EFI_STATUSFetchStatus
);
#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] BufThe data of the blob
@param[in] BufSizeThe size of the blob in bytes
+ @param[in] FetchStatusThe 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 UINT32BufSize
+ IN UINT32BufSize,
+ IN EFI_STATUSFetchStatus
)
{
- 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
