[edk2-devel] [PATCH v4 3/3] OvmfPkg/VirtNorFlashDxe: sanity-check variables

["Gerd Hoffmann" <kraxel@redhat.com>]

Extend the ValidateFvHeader function, additionally to the header checks
walk over the list of variables and sanity check them.

In case we find inconsistencies indicating variable store corruption
return EFI_NOT_FOUND so the variable store will be re-initialized.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 OvmfPkg/VirtNorFlashDxe/VirtNorFlashDxe.inf |   1 +
 OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c   | 139 +++++++++++++++++++-
 2 files changed, 135 insertions(+), 5 deletions(-)

diff --git a/OvmfPkg/VirtNorFlashDxe/VirtNorFlashDxe.inf b/OvmfPkg/VirtNorFlashDxe/VirtNorFlashDxe.inf
index 2a3d4a218e61..f549400280a1 100644
--- a/OvmfPkg/VirtNorFlashDxe/VirtNorFlashDxe.inf
+++ b/OvmfPkg/VirtNorFlashDxe/VirtNorFlashDxe.inf
@@ -34,6 +34,7 @@ [LibraryClasses]
   DxeServicesTableLib
   HobLib
   IoLib
+  SafeIntLib
   UefiBootServicesTableLib
   UefiDriverEntryPoint
   UefiLib
diff --git a/OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c b/OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c
index 9a614ae4b24d..56148e9f1f95 100644
--- a/OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c
+++ b/OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c
@@ -12,6 +12,7 @@
 #include <Library/BaseMemoryLib.h>
 #include <Library/MemoryAllocationLib.h>
 #include <Library/PcdLib.h>
+#include <Library/SafeIntLib.h>
 #include <Library/UefiLib.h>
 
 #include <Guid/NvVarStoreFormatted.h>
@@ -185,11 +186,13 @@ ValidateFvHeader (
   IN  NOR_FLASH_INSTANCE  *Instance
   )
 {
-  UINT16                      Checksum;
-  EFI_FIRMWARE_VOLUME_HEADER  *FwVolHeader;
-  VARIABLE_STORE_HEADER       *VariableStoreHeader;
-  UINTN                       VariableStoreLength;
-  UINTN                       FvLength;
+  UINT16                            Checksum;
+  CONST EFI_FIRMWARE_VOLUME_HEADER  *FwVolHeader;
+  CONST VARIABLE_STORE_HEADER       *VariableStoreHeader;
+  UINTN                             VarOffset;
+  UINTN                             VariableStoreLength;
+  UINTN                             FvLength;
+  RETURN_STATUS                     Status;
 
   FwVolHeader = (EFI_FIRMWARE_VOLUME_HEADER *)Instance->RegionBaseAddress;
 
@@ -258,6 +261,132 @@ ValidateFvHeader (
     return EFI_NOT_FOUND;
   }
 
+  //
+  // check variables
+  //
+  DEBUG ((DEBUG_INFO, "%a: checking variables\n", __func__));
+  VarOffset = sizeof (*VariableStoreHeader);
+  for ( ; ;) {
+    UINTN                                VarHeaderEnd;
+    UINTN                                VarNameEnd;
+    UINTN                                VarEnd;
+    UINTN                                VarPadding;
+    CONST AUTHENTICATED_VARIABLE_HEADER  *VarHeader;
+    CHAR16                               *VarName;
+    CONST CHAR8                          *VarState;
+
+    Status = SafeUintnAdd (VarOffset, sizeof (*VarHeader), &VarHeaderEnd);
+    if (RETURN_ERROR (Status)) {
+      DEBUG ((DEBUG_ERROR, "%a: integer overflow\n", __func__));
+      return EFI_NOT_FOUND;
+    }
+
+    if (VarHeaderEnd >= VariableStoreHeader->Size) {
+      DEBUG ((DEBUG_INFO, "%a: end of var list (no space left)\n", __func__));
+      break;
+    }
+
+    VarHeader = (VOID *)((UINTN)VariableStoreHeader + VarOffset);
+    if (VarHeader->StartId != 0x55aa) {
+      DEBUG ((DEBUG_INFO, "%a: end of var list (no startid)\n", __func__));
+      break;
+    }
+
+    VarName = NULL;
+    switch (VarHeader->State) {
+      // usage: State = VAR_HEADER_VALID_ONLY
+      case VAR_HEADER_VALID_ONLY:
+        VarState = "header-ok";
+        VarName  = L"<unknown>";
+        break;
+
+      // usage: State = VAR_ADDED
+      case VAR_ADDED:
+        VarState = "ok";
+        break;
+
+      // usage: State &= VAR_IN_DELETED_TRANSITION
+      case VAR_ADDED &VAR_IN_DELETED_TRANSITION:
+        VarState = "del-in-transition";
+        break;
+
+      // usage: State &= VAR_DELETED
+      case VAR_ADDED &VAR_DELETED:
+      case VAR_ADDED &VAR_DELETED &VAR_IN_DELETED_TRANSITION:
+        VarState = "deleted";
+        break;
+
+      default:
+        DEBUG ((
+          DEBUG_ERROR,
+          "%a: invalid variable state: 0x%x\n",
+          __func__,
+          VarHeader->State
+          ));
+        return EFI_NOT_FOUND;
+    }
+
+    Status = SafeUintnAdd (VarHeaderEnd, VarHeader->NameSize, &VarNameEnd);
+    if (RETURN_ERROR (Status)) {
+      DEBUG ((DEBUG_ERROR, "%a: integer overflow\n", __func__));
+      return EFI_NOT_FOUND;
+    }
+
+    Status = SafeUintnAdd (VarNameEnd, VarHeader->DataSize, &VarEnd);
+    if (RETURN_ERROR (Status)) {
+      DEBUG ((DEBUG_ERROR, "%a: integer overflow\n", __func__));
+      return EFI_NOT_FOUND;
+    }
+
+    if (VarEnd > VariableStoreHeader->Size) {
+      DEBUG ((
+        DEBUG_ERROR,
+        "%a: invalid variable size: 0x%Lx + 0x%Lx + 0x%x + 0x%x > 0x%x\n",
+        __func__,
+        (UINT64)VarOffset,
+        (UINT64)(sizeof (*VarHeader)),
+        VarHeader->NameSize,
+        VarHeader->DataSize,
+        VariableStoreHeader->Size
+        ));
+      return EFI_NOT_FOUND;
+    }
+
+    if ((VarHeader->NameSize & 1) ||
+        (VarHeader->NameSize < 4))
+    {
+      DEBUG ((DEBUG_ERROR, "%a: invalid name size\n", __func__));
+      return EFI_NOT_FOUND;
+    }
+
+    if (VarName == NULL) {
+      VarName = (VOID *)((UINTN)VariableStoreHeader + VarHeaderEnd);
+      if (VarName[VarHeader->NameSize / 2 - 1] != L'\0') {
+        DEBUG ((DEBUG_ERROR, "%a: name is not null terminated\n", __func__));
+        return EFI_NOT_FOUND;
+      }
+    }
+
+    DEBUG ((
+      DEBUG_VERBOSE,
+      "%a: +0x%04Lx: name=0x%x data=0x%x guid=%g '%s' (%a)\n",
+      __func__,
+      (UINT64)VarOffset,
+      VarHeader->NameSize,
+      VarHeader->DataSize,
+      &VarHeader->VendorGuid,
+      VarName,
+      VarState
+      ));
+
+    VarPadding = (4 - (VarEnd & 3)) & 3;
+    Status     = SafeUintnAdd (VarEnd, VarPadding, &VarOffset);
+    if (RETURN_ERROR (Status)) {
+      DEBUG ((DEBUG_ERROR, "%a: integer overflow\n", __func__));
+      return EFI_NOT_FOUND;
+    }
+  }
+
   return EFI_SUCCESS;
 }
 
-- 
2.43.0



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