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

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

[Gerd Hoffmann]

v5:
 - pick up review tags
 - more variable check fine tuning.
v4:
 - turn into a little patch series.
 - stop using gEfiVariableGuid in risc-v.
 - stop accepting gEfiVariableGuid in VirtNorFlashDxe.
 - refine variable checking.

Gerd Hoffmann (3):
  OvmfPkg/RiscVVirt: use gEfiAuthenticatedVariableGuid unconditionally
  OvmfPkg/VirtNorFlashDxe: stop accepting gEfiVariableGuid
  OvmfPkg/VirtNorFlashDxe: sanity-check variables

 OvmfPkg/VirtNorFlashDxe/VirtNorFlashDxe.inf |   1 +
 OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c   | 151 ++++++++++++++++++--
 OvmfPkg/RiscVVirt/VarStore.fdf.inc          |   9 +-
 3 files changed, 145 insertions(+), 16 deletions(-)

-- 
2.43.0



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

[edk2-devel] [PATCH v5 1/3] OvmfPkg/RiscVVirt: use gEfiAuthenticatedVariableGuid unconditionally

[Gerd Hoffmann]

ArmVirt and OVMF are doing the same.

See commit d92eaabefbe0 ("OvmfPkg: simplify VARIABLE_STORE_HEADER
generation") for details.

Suggested-by: László Érsek <lersek@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Sunil V L <sunilvl@ventanamicro.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/RiscVVirt/VarStore.fdf.inc | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/OvmfPkg/RiscVVirt/VarStore.fdf.inc b/OvmfPkg/RiscVVirt/VarStore.fdf.inc
index aba32315cc37..6679c246b3ea 100644
--- a/OvmfPkg/RiscVVirt/VarStore.fdf.inc
+++ b/OvmfPkg/RiscVVirt/VarStore.fdf.inc
@@ -36,19 +36,12 @@
   # Blockmap[1]: End
   0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
   ## This is the VARIABLE_STORE_HEADER
-!if $(SECURE_BOOT_ENABLE) == TRUE
+  # It is compatible with SECURE_BOOT_ENABLE == FALSE as well.
   # Signature: gEfiAuthenticatedVariableGuid =
   #   { 0xaaf32c78, 0x947b, 0x439a,
   #     { 0xa1, 0x80, 0x2e, 0x14, 0x4e, 0xc3, 0x77, 0x92 }}
   0x78, 0x2c, 0xf3, 0xaa, 0x7b, 0x94, 0x9a, 0x43,
   0xa1, 0x80, 0x2e, 0x14, 0x4e, 0xc3, 0x77, 0x92,
-!else
-  # Signature: gEfiVariableGuid =
-  #   { 0xddcf3616, 0x3275, 0x4164,
-  #     { 0x98, 0xb6, 0xfe, 0x85, 0x70, 0x7f, 0xfe, 0x7d }}
-  0x16, 0x36, 0xcf, 0xdd, 0x75, 0x32, 0x64, 0x41,
-  0x98, 0xb6, 0xfe, 0x85, 0x70, 0x7f, 0xfe, 0x7d,
-!endif
   # Size: 0x40000 (gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize) -
   #         0x48 (size of EFI_FIRMWARE_VOLUME_HEADER) = 0x3FFB8
   # This can speed up the Variable Dispatch a bit.
-- 
2.43.0



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

[edk2-devel] [PATCH v5 2/3] OvmfPkg/VirtNorFlashDxe: stop accepting gEfiVariableGuid

[Gerd Hoffmann]

Only accept gEfiAuthenticatedVariableGuid when checking the variable
store header in ValidateFvHeader().

The edk2 code base has been switched to use the authenticated varstore
format unconditionally (even in case secure boot is not used or
supported) a few years ago.

Suggested-by: László Érsek <lersek@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c b/OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c
index 5ee98e9b595a..9a614ae4b24d 100644
--- a/OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c
+++ b/OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c
@@ -239,9 +239,7 @@ ValidateFvHeader (
   VariableStoreHeader = (VARIABLE_STORE_HEADER *)((UINTN)FwVolHeader + FwVolHeader->HeaderLength);
 
   // Check the Variable Store Guid
-  if (!CompareGuid (&VariableStoreHeader->Signature, &gEfiVariableGuid) &&
-      !CompareGuid (&VariableStoreHeader->Signature, &gEfiAuthenticatedVariableGuid))
-  {
+  if (!CompareGuid (&VariableStoreHeader->Signature, &gEfiAuthenticatedVariableGuid)) {
     DEBUG ((
       DEBUG_INFO,
       "%a: Variable Store Guid non-compatible\n",
-- 
2.43.0



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

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

[Gerd Hoffmann]

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   | 147 +++++++++++++++++++-
 2 files changed, 143 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..ca2e40743dfd 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,12 @@ 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;
 
   FwVolHeader = (EFI_FIRMWARE_VOLUME_HEADER *)Instance->RegionBaseAddress;
 
@@ -258,6 +260,141 @@ 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;
+    CONST CHAR16                         *VarName;
+    CONST CHAR8                          *VarState;
+    RETURN_STATUS                        Status;
+
+    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) {
+      if (VarOffset <= VariableStoreHeader->Size - sizeof (UINT16)) {
+        CONST UINT16  *StartId = (VOID *)((UINTN)VariableStoreHeader + VarOffset);
+        if (*StartId == 0x55aa) {
+          DEBUG ((DEBUG_ERROR, "%a: startid at invalid location\n", __func__));
+          return EFI_NOT_FOUND;
+        }
+      }
+
+      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) != 0) ||
+        (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 (#113441): https://edk2.groups.io/g/devel/message/113441
Mute This Topic: https://groups.io/mt/103617812/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Laszlo Ersek]

On 1/9/24 12:29, Gerd Hoffmann wrote:
> 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   | 147 +++++++++++++++++++-
>  2 files changed, 143 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..ca2e40743dfd 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,12 @@ 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;
>
>    FwVolHeader = (EFI_FIRMWARE_VOLUME_HEADER *)Instance->RegionBaseAddress;
>
> @@ -258,6 +260,141 @@ 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;
> +    CONST CHAR16                         *VarName;
> +    CONST CHAR8                          *VarState;
> +    RETURN_STATUS                        Status;
> +
> +    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) {
> +      if (VarOffset <= VariableStoreHeader->Size - sizeof (UINT16)) {
> +        CONST UINT16  *StartId = (VOID *)((UINTN)VariableStoreHeader + VarOffset);
> +        if (*StartId == 0x55aa) {
> +          DEBUG ((DEBUG_ERROR, "%a: startid at invalid location\n", __func__));
> +          return EFI_NOT_FOUND;
> +        }
> +      }
> +
> +      DEBUG ((DEBUG_INFO, "%a: end of var list (no space left)\n", __func__));
> +      break;
> +    }

I didn't think of this, but it certainly makes sense. Continues
accepting trailing garbage, unless the tail starts with 0x55aa, which
indicates that it's indeed either a truncated variable header, or one
that's not truncated, but just fits (and has no room for subsequent
name/data).

Furthermore, we consider "VariableStoreHeader->Size" trusted & valid
here, therefore the subtraction of sizeof (UINT16) is not supposed to
wrap under. (The field is documented with: "Size of entire variable
store, including size of variable store header but not including the
size of FvHeader". The varstore hdr structure is larger than 2 bytes.)

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Nit: to my knowledge, the coding style forbids initialization of "auto"
storage class variables (more commonly put, "non-static local
variables"). IOW, we should spell the above as:

| diff --git a/OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c b/OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c
| index ca2e40743dfd..8fcd999ac6df 100644
| --- a/OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c
| +++ b/OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c
| @@ -283,7 +283,9 @@ ValidateFvHeader (
|
|      if (VarHeaderEnd >= VariableStoreHeader->Size) {
|        if (VarOffset <= VariableStoreHeader->Size - sizeof (UINT16)) {
| -        CONST UINT16  *StartId = (VOID *)((UINTN)VariableStoreHeader + VarOffset);
| +        CONST UINT16  *StartId;
| +
| +        StartId = (VOID *)((UINTN)VariableStoreHeader + VarOffset);
|          if (*StartId == 0x55aa) {
|            DEBUG ((DEBUG_ERROR, "%a: startid at invalid location\n", __func__));
|            return EFI_NOT_FOUND;

Do you want me to fix up the patch upon merge for you, or do you prefer
posting v6?

(Again, I'm not sure why uncrustify accepts the code as-is... Of course
if the coding style has changed meanwhile, and the local's
initialization now counts as valid style, then I can merge this
verbatim.)

Thanks
Laszlo

> +
> +    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) != 0) ||
> +        (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;
>  }
>



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

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

[Gerd Hoffmann]

  Hi,

> Nit: to my knowledge, the coding style forbids initialization of "auto"
> storage class variables (more commonly put, "non-static local
> variables"). IOW, we should spell the above as:
> 
> | diff --git a/OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c b/OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c
> | index ca2e40743dfd..8fcd999ac6df 100644
> | --- a/OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c
> | +++ b/OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c
> | @@ -283,7 +283,9 @@ ValidateFvHeader (
> |
> |      if (VarHeaderEnd >= VariableStoreHeader->Size) {
> |        if (VarOffset <= VariableStoreHeader->Size - sizeof (UINT16)) {
> | -        CONST UINT16  *StartId = (VOID *)((UINTN)VariableStoreHeader + VarOffset);
> | +        CONST UINT16  *StartId;
> | +
> | +        StartId = (VOID *)((UINTN)VariableStoreHeader + VarOffset);
> |          if (*StartId == 0x55aa) {
> |            DEBUG ((DEBUG_ERROR, "%a: startid at invalid location\n", __func__));
> |            return EFI_NOT_FOUND;
> 
> Do you want me to fix up the patch upon merge for you,

I happily accept that service offer ;)

thanks,
  Gerd



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

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

[Laszlo Ersek]

On 1/9/24 15:11, Gerd Hoffmann wrote:
>   Hi,
> 
>> Nit: to my knowledge, the coding style forbids initialization of "auto"
>> storage class variables (more commonly put, "non-static local
>> variables"). IOW, we should spell the above as:
>>
>> | diff --git a/OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c b/OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c
>> | index ca2e40743dfd..8fcd999ac6df 100644
>> | --- a/OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c
>> | +++ b/OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c
>> | @@ -283,7 +283,9 @@ ValidateFvHeader (
>> |
>> |      if (VarHeaderEnd >= VariableStoreHeader->Size) {
>> |        if (VarOffset <= VariableStoreHeader->Size - sizeof (UINT16)) {
>> | -        CONST UINT16  *StartId = (VOID *)((UINTN)VariableStoreHeader + VarOffset);
>> | +        CONST UINT16  *StartId;
>> | +
>> | +        StartId = (VOID *)((UINTN)VariableStoreHeader + VarOffset);
>> |          if (*StartId == 0x55aa) {
>> |            DEBUG ((DEBUG_ERROR, "%a: startid at invalid location\n", __func__));
>> |            return EFI_NOT_FOUND;
>>
>> Do you want me to fix up the patch upon merge for you,
> 
> I happily accept that service offer ;)

Series merged as commit range 08a6528bac38..4a443f73fd67, via
<https://github.com/tianocore/edk2/pull/5240>. (Last three commits in
the PR.)

Laszlo



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