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

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

[Gerd Hoffmann]

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   | 143 ++++++++++++++++++--
 OvmfPkg/RiscVVirt/VarStore.fdf.inc          |   9 +-
 3 files changed, 137 insertions(+), 16 deletions(-)

-- 
2.43.0



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

[edk2-devel] [PATCH v4 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>
---
 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 (#113409): https://edk2.groups.io/g/devel/message/113409
Mute This Topic: https://groups.io/mt/103605055/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v4 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>
---
 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 (#113410): https://edk2.groups.io/g/devel/message/113410
Mute This Topic: https://groups.io/mt/103605076/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v4 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   | 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]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Sunil V L]

On Mon, Jan 08, 2024 at 08:21:21PM +0100, Gerd Hoffmann wrote:
> 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>
> ---
Thanks!, Gerd. Looks good to me.

Reviewed-by: Sunil V L <sunilvl@ventanamicro.com>



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

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

[Laszlo Ersek]

On 1/8/24 20:21, Gerd Hoffmann wrote:
> 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>
> ---
>  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.

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



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113431): https://edk2.groups.io/g/devel/message/113431
Mute This Topic: https://groups.io/mt/103605055/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 v4 2/3] OvmfPkg/VirtNorFlashDxe: stop accepting gEfiVariableGuid

[Laszlo Ersek]

On 1/8/24 20:21, Gerd Hoffmann wrote:
> 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>
> ---
>  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",

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



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113432): https://edk2.groups.io/g/devel/message/113432
Mute This Topic: https://groups.io/mt/103605076/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 v4 3/3] OvmfPkg/VirtNorFlashDxe: sanity-check variables

[Laszlo Ersek]

On 1/8/24 20:21, 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   | 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;

(1) Status could have been moved in the "for" loop, AFAICT -- also
mentioned in my previous review --, but it's not a sticking point.

>  
>    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;

(2) Should have noticed in my previous review: VarName could be
CONST-ified as well.

Totally minor.

> +    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;
> +    }

(3) I *still* don't understand this.

In the v3 discussion:

- we agreed that the ">" case was clearly unacceptable,

- and you convinced me that the "=" case was also unacceptable (because
that could only potentially accommodate a VAR_HEADER_VALID_ONLY state
header without subsequent space for name & data, and we agreed that a
var header like that, residing *permanently* in the varstore, was not
acceptable).

Fine: that's reason for keeping the unified ">=" condition. But my point
in turn (which you didn't reflect upon, and seem not to have addressed
in this patch) was that this condition means that the varstore is
*bogus*, and should be reformatted. In my previous review I recommended
that we replace the "break" here -- which leads to the function
returning EFI_SUCCESS -- with "return EFI_NOT_FOUND" -- which causes the
varstore to be reformatted.

And then, as I wrote, the only successful exit from the loop would be
the subsequent "break" below:

> +
> +    VarHeader = (VOID *)((UINTN)VariableStoreHeader + VarOffset);
> +    if (VarHeader->StartId != 0x55aa) {
> +      DEBUG ((DEBUG_INFO, "%a: end of var list (no startid)\n", __func__));
> +      break;
> +    }

So: what's wrong with returning EFI_NOT_FOUND if

  VarHeaderEnd >= VariableStoreHeader->Size

evaluates to true?

> +
> +    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) ||

(4) Minor: I don't understand why uncrustify does not catch this, but
the edk2 coding style requires us to spell this as

  (VarHeader->NameSize & 1) != 0

to my understanding.

Apologies for not noticing it in my previous review.

> +        (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;
>  }
>  

- If you can explain the "break" under (3), I'm happy to R-b this patch
(and to merge this v4 series). The rest of my comments ((1), (2), (4))
doesn't justify a respin, in itself.

- If you decide to replace the "break" with "return EFI_NOT_FOUND" under
(3), then addressing the rest ((1), (2), (4)) would be nice, in v5.

Thanks!
Laszlo



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