[edk2-devel] [PATCH v3 1/1] OvmfPkg/VirtNorFlashDxe: sanity-check variables

[edk2-devel] [PATCH v3 1/1] 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   | 125 +++++++++++++++++++-
 2 files changed, 121 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 5ee98e9b595a..d69bf8783d77 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,19 @@ ValidateFvHeader (
   IN  NOR_FLASH_INSTANCE  *Instance
   )
 {
-  UINT16                      Checksum;
-  EFI_FIRMWARE_VOLUME_HEADER  *FwVolHeader;
-  VARIABLE_STORE_HEADER       *VariableStoreHeader;
-  UINTN                       VariableStoreLength;
-  UINTN                       FvLength;
+  UINT16                         Checksum;
+  EFI_FIRMWARE_VOLUME_HEADER     *FwVolHeader;
+  VARIABLE_STORE_HEADER          *VariableStoreHeader;
+  UINTN                          VarOffset;
+  UINTN                          VarHeaderEnd;
+  UINTN                          VarNameEnd;
+  UINTN                          VarEnd;
+  AUTHENTICATED_VARIABLE_HEADER  *VarHeader;
+  CHAR16                         *VarName;
+  CONST CHAR8                    *VarState;
+  UINTN                          VariableStoreLength;
+  UINTN                          FvLength;
+  RETURN_STATUS                  Status;
 
   FwVolHeader = (EFI_FIRMWARE_VOLUME_HEADER *)Instance->RegionBaseAddress;
 
@@ -260,6 +269,112 @@ ValidateFvHeader (
     return EFI_NOT_FOUND;
   }
 
+  //
+  // check variables
+  //
+  DEBUG ((DEBUG_INFO, "%a: checking variables\n", __func__));
+  VarOffset = sizeof (*VariableStoreHeader);
+  for ( ; ;) {
+    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) {
+      DEBUG ((DEBUG_ERROR, "%a: invalid name size\n", __func__));
+      return EFI_NOT_FOUND;
+    }
+
+    if (VarName == NULL) {
+      VarName = (VOID *)((UINTN)VariableStoreHeader + VarHeaderEnd);
+    }
+
+    DEBUG ((
+      DEBUG_VERBOSE,
+      "%a: +0x%04Lx: name=0x%x data=0x%x '%s' (%a)\n",
+      __func__,
+      (UINT64)VarOffset,
+      VarHeader->NameSize,
+      VarHeader->DataSize,
+      VarName,
+      VarState
+      ));
+
+    VarOffset = ALIGN_VALUE (VarEnd, 4);
+  }
+
   return EFI_SUCCESS;
 }
 
-- 
2.43.0



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

Re: [edk2-devel] [PATCH v3 1/1] OvmfPkg/VirtNorFlashDxe: sanity-check variables

[Laszlo Ersek]

On 12/14/23 16:31, 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   | 125 +++++++++++++++++++-
>  2 files changed, 121 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 5ee98e9b595a..d69bf8783d77 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,19 @@ ValidateFvHeader (
>    IN  NOR_FLASH_INSTANCE  *Instance
>    )
>  {
> -  UINT16                      Checksum;
> -  EFI_FIRMWARE_VOLUME_HEADER  *FwVolHeader;
> -  VARIABLE_STORE_HEADER       *VariableStoreHeader;
> -  UINTN                       VariableStoreLength;
> -  UINTN                       FvLength;
> +  UINT16                         Checksum;
> +  EFI_FIRMWARE_VOLUME_HEADER     *FwVolHeader;
> +  VARIABLE_STORE_HEADER          *VariableStoreHeader;
> +  UINTN                          VarOffset;
> +  UINTN                          VarHeaderEnd;
> +  UINTN                          VarNameEnd;
> +  UINTN                          VarEnd;
> +  AUTHENTICATED_VARIABLE_HEADER  *VarHeader;

(1) The fact that we try to traverse the variables using
AUTHENTICATED_VARIABLE_HEADER creates a *very slight* inconsistency in
the code.

More precisely, the inconsistency is already there, but this patch makes
it "stronger".

Or even more precisely... this patch in fact introduces a bug, as far as
I can tell, for the RISC-V virt platform firmware.

Here's the details:

In ancient times, authenticated and non-authenticated variable stores
were handled by separate variable drivers. Over time they got unified --
in commit fa0737a839d0 ("MdeModulePkg Variable: Merge from Auth Variable
driver in SecurityPkg", 2015-07-01). The unified driver would then
manage both kinds of varstores.

This got reflected under OvmfPkg in commit d92eaabefbe0 ("OvmfPkg:
simplify VARIABLE_STORE_HEADER generation", 2016-02-15). I recommend
reviewing *that* commit in detail.

ArmVirtPkg's varstore generation was implemented in commit bf57a42a0e2c
("ArmVirtPkg: add FDF definition for empty varstore", 2016-06-23),
duplicating the (simplified, auth-only) varstore header signature from
OvmfPkg.

The upshot was that the OVMF and ArmVirt build processes would only
generate varstores with "gEfiAuthenticatedVariableGuid" in the varstore
header signature.

At the time of commit bf57a42a0e2c ("ArmVirtPkg: add FDF definition for
empty varstore", 2016-06-23), we didn't have VirtNorFlashDxe yet. The
various platform flash drivers back then tolerated (recognized) the
"gEfiVariableGuid" varstore header signature as well. So it's pretty
difficult to say now whether the exclusive generation of the
"gEfiAuthenticatedVariableGuid" signature in OVMF and ArmVirt *should
have been followed* of a restriction of the ValidateFvHeader() functions
in all those drivers -- i.e., whether we should have updated all those
drivers too, to reject "gEfiVariableGuid". After all, the unified
variable driver would just deal with "gEfiVariableGuid" fine, so the
"tolerance" of "gEfiVariableGuid"  in the flash drivers' validation code
was harmless.

But that's not the case anymore, with this patch:

For the variable traversal, we now expect each variable to come with an
AUTHENTICATED_VARIABLE_HEADER.

First, that is inconsistent with our tolerance of "gEfiVariableGuid" in
the varstore header signature. If one part of the code says "OK, this
can validly be 'gEfiVariableGuid'", then the other part of the code
should read the variable headers *as* VARIABLE_HEADER. Or else, in the
opposite direction: if we're only willing to parse the variable list via
AUTHENTICATED_VARIABLE_HEADER, then we should now reject the
"gEfiVariableGuid" header signature *upfront*.

Second (and worse): the bug. In "OvmfPkg/RiscVVirt/VarStore.fdf.inc", it
turns out that we *still* generate the gEfiVariableGuid varstore header
signature, in case SECURE_BOOT_ENABLE is FALSE. For some reason, commit
92b27c2e6ada ("OvmfPkg/RiscVVirt: Add build files for Qemu Virt
platform", 2023-02-16) did not consider commit d92eaabefbe0 ("OvmfPkg:
simplify VARIABLE_STORE_HEADER generation", 2016-02-15), and
*resurrected* the non-unified varstore generation for RiscVVirt.
Furthermore, RiscVVirt uses "VirtNorFlashDxe" as its platform flash
driver. As a result, if you now build RiscVVirt with this patch applied,
and with SECURE_BOOT_ENABLE=FALSE, I expect the ValidateFvHeader()
function to always fail, becase it will try to validate the contents of
the varstore through AUTHENTICATED_VARIABLE_HEADER entries, despite the
varstore containing (arguably valid) VARIABLE_HEADER entries.

So here's what I propose:

- keep this patch, but *prepend* two other patches:

- first, reflect commit d92eaabefbe0 to
"OvmfPkg/RiscVVirt/VarStore.fdf.inc" -- only generate the authenticated
signature GUID, regardless of SECURE_BOOT_ENABLE,

- second, in this function, stop accepting the "gEfiVariableGuid"
varstore header signature.


(2) Minor: consider CONST-ifying FwVolHeader, VariableStoreHeader,
VarHeader. (Feel free to ignore if it causes many problems.)


(3) Minor: consider declaring all of the new variables except VarOffset
(i.e., the variables VarHeaderEnd, VarNameEnd, VarEnd, VarHeader,
VarName, VarState, Status) at the top of the body of the "for" loop
below. That restricts their scopes as narrowly as possible, which helps
us understand the data flow.

> +  CHAR16                         *VarName;
> +  CONST CHAR8                    *VarState;
> +  UINTN                          VariableStoreLength;
> +  UINTN                          FvLength;
> +  RETURN_STATUS                  Status;
>
>    FwVolHeader = (EFI_FIRMWARE_VOLUME_HEADER *)Instance->RegionBaseAddress;
>
> @@ -260,6 +269,112 @@ ValidateFvHeader (
>      return EFI_NOT_FOUND;
>    }
>
> +  //
> +  // check variables
> +  //
> +  DEBUG ((DEBUG_INFO, "%a: checking variables\n", __func__));
> +  VarOffset = sizeof (*VariableStoreHeader);
> +  for ( ; ;) {
> +    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;
> +    }

(4) In case of inequality, the variable header is truncated. Accepting
it as "success" looks doubtful.


(5) In case of equality, the variable header fits, but it is followed by
no payload at all. I think there are sub-cases to distinguish there:

- if the StartId differs from 0x55aa, then we may consider the variable
list to be terminated, and break out of the loop (returning success from
the function)

- if the StartId is 0x55aa, then we need to look further, beause we
can't decide yet. For example, if State is VAR_HEADER_VALID_ONLY (0x7f),
then it might be fine for the variable header (at the very end of the
varstore) *not* to be followed by payload bytes (name, data).

I find this code hard to review because I don't know (and the Intel
whitepaper doesn't seem to tell) precisely how a valid variable list is
supposed to be terminated.

> +
> +    VarHeader = (VOID *)((UINTN)VariableStoreHeader + VarOffset);

This addition is unchecked, but it's safe. We're operating under the
assumption that the complete variable store (starting at
VariableStoreHeader, running for VariableStoreHeader->Size bytes) has
been mapped into guest-physical address space. Furthermore, at the very
end of the address space, the "code" flash device is mapped (with the
reset vector at the tail). Therefore, as long as our relative offsets
are smaller than, or equal to, VariableStoreHeader->Size, we can safely
evaluate the *absolute addresses* (possibly pointing just one past the
varstore, but still not overflowing, in case of equality), and if the
relative offset is strictly smaller than VariableStoreHeader->Size, then
we can even dereference those absolute addresses.

> +    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) {
> +      DEBUG ((DEBUG_ERROR, "%a: invalid name size\n", __func__));
> +      return EFI_NOT_FOUND;
> +    }
> +
> +    if (VarName == NULL) {
> +      VarName = (VOID *)((UINTN)VariableStoreHeader + VarHeaderEnd);

From the perspective of "VarName" being well-aligned, I think this is
fine. Our assumption thus far is that VarHeader is well-aligned, and
given the sizeof AUTHENTICATED_VARIABLE_HEADER, VarName should be
well-aligned as well.

(6) I suggest two further checks (within the braces here):

- enforce

  VarHeader->NameSize > 0

- enforce

  VarName[VarHeader->NameSize / 2 - 1] == L'\0'

because the "AUTHENTICATED_VARIABLE_HEADER.NameSize" documentation in
"MdeModulePkg/Include/Guid/VariableFormat.h" specifies NUL-termination.

(This is also important for the immediately subsequent code: we print
the name!)

> +    }
> +
> +    DEBUG ((
> +      DEBUG_VERBOSE,
> +      "%a: +0x%04Lx: name=0x%x data=0x%x '%s' (%a)\n",
> +      __func__,
> +      (UINT64)VarOffset,
> +      VarHeader->NameSize,
> +      VarHeader->DataSize,
> +      VarName,
> +      VarState
> +      ));

(7) Not really important, I'm just throwing it out: how about logging
"VarHeader->VendorGuid" too?

It would require something like this:

  CONST EFI_GUID  *VarGuid;

  ...

  VarGuid = &gZeroGuid;
  if (VarName == NULL) {
    ...
    VarGuid = &VarHeader->VendorGuid;
    ...
  }

and then format VarGuid with "%g" in the DEBUG macro invocation.

This would be useful for the non-standard namespace GUIDs especially.

> +
> +    VarOffset = ALIGN_VALUE (VarEnd, 4);

(8) Apologies if it was me who suggested ALIGN_VALUE() previously, but
this is, in effect, an unchecked addition. I can't off-hand see evidence
that it can never overflow (the previous checks don't seem to prevent an
overflow here), so I suggest:

  //
  // the next variable header starts aligned at 4 bytes
  //
  Status = SafeUintnAdd (VarEnd, (4 - (VarEnd & 4)) & 4, &VarOffset);
  if RETURN_ERROR (Status) {
    DEBUG ((DEBUG_ERROR, "%a: integer overflow\n", __func__));
    return EFI_NOT_FOUND;
  }

> +  }
> +
>    return EFI_SUCCESS;
>  }
>

Thanks
Laszlo



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

[Laszlo Ersek]

On 1/3/24 13:56, Laszlo Ersek wrote:

> (8) Apologies if it was me who suggested ALIGN_VALUE() previously, but
> this is, in effect, an unchecked addition. I can't off-hand see evidence
> that it can never overflow (the previous checks don't seem to prevent an
> overflow here), so I suggest:
> 
>   //
>   // the next variable header starts aligned at 4 bytes
>   //
>   Status = SafeUintnAdd (VarEnd, (4 - (VarEnd & 4)) & 4, &VarOffset);
>   if RETURN_ERROR (Status) {
>     DEBUG ((DEBUG_ERROR, "%a: integer overflow\n", __func__));
>     return EFI_NOT_FOUND;
>   }

Heh, what I wrote is bogus. Man, binary is hard. :) So, let me try again:

  Status = SafeUintnAdd (VarEnd, (4 - (VarEnd & 3)) & 3, &VarOffset);

Ideally, we'd have a SafeIntLib set of APIs for aligning up...

Sorry :)
Laszlo



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

[Laszlo Ersek]

On 1/3/24 14:09, Laszlo Ersek wrote:
> On 1/3/24 13:56, Laszlo Ersek wrote:
> 
>> (8) Apologies if it was me who suggested ALIGN_VALUE() previously, but
>> this is, in effect, an unchecked addition. I can't off-hand see evidence
>> that it can never overflow (the previous checks don't seem to prevent an
>> overflow here), so I suggest:
>>
>>   //
>>   // the next variable header starts aligned at 4 bytes
>>   //
>>   Status = SafeUintnAdd (VarEnd, (4 - (VarEnd & 4)) & 4, &VarOffset);
>>   if RETURN_ERROR (Status) {
>>     DEBUG ((DEBUG_ERROR, "%a: integer overflow\n", __func__));
>>     return EFI_NOT_FOUND;
>>   }
> 
> Heh, what I wrote is bogus. Man, binary is hard. :) So, let me try again:
> 
>   Status = SafeUintnAdd (VarEnd, (4 - (VarEnd & 3)) & 3, &VarOffset);
> 
> Ideally, we'd have a SafeIntLib set of APIs for aligning up...

BTW the reason I messed it up was that I'm much more attracted to the
"%" operator, so initially I wrote it as

  Status = SafeUintnAdd (VarEnd, (4 - (VarEnd % 4)) % 4, &VarOffset);

which was correct, but then I thought, from your code, that you probably
liked "&" more (and that "&" might be faster for the CPU...), and then I
"replaced" the "%" operator with "&", but forgot to replace the divisor
4 with the bitmask 3...

Laszlo



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

[Gerd Hoffmann]

  Hi,

> Second (and worse): the bug. In "OvmfPkg/RiscVVirt/VarStore.fdf.inc", it
> turns out that we *still* generate the gEfiVariableGuid varstore header
> signature, in case SECURE_BOOT_ENABLE is FALSE. For some reason, commit
> 92b27c2e6ada ("OvmfPkg/RiscVVirt: Add build files for Qemu Virt
> platform", 2023-02-16) did not consider commit d92eaabefbe0 ("OvmfPkg:
> simplify VARIABLE_STORE_HEADER generation", 2016-02-15), and
> *resurrected* the non-unified varstore generation for RiscVVirt.
> Furthermore, RiscVVirt uses "VirtNorFlashDxe" as its platform flash
> driver. As a result, if you now build RiscVVirt with this patch applied,
> and with SECURE_BOOT_ENABLE=FALSE, I expect the ValidateFvHeader()
> function to always fail, becase it will try to validate the contents of
> the varstore through AUTHENTICATED_VARIABLE_HEADER entries, despite the
> varstore containing (arguably valid) VARIABLE_HEADER entries.

I expect it will fail only once.  In case the checks don't pass
VirtNorFlashDxe will re-initialize the flash varstore with
gEfiAuthenticatedVariableGuid, so on next boot everything is fine.

> So here's what I propose:
> 
> - keep this patch, but *prepend* two other patches:
> 
> - first, reflect commit d92eaabefbe0 to
> "OvmfPkg/RiscVVirt/VarStore.fdf.inc" -- only generate the authenticated
> signature GUID, regardless of SECURE_BOOT_ENABLE,
> 
> - second, in this function, stop accepting the "gEfiVariableGuid"
> varstore header signature.

Makes sense.

> > +    if (VarHeaderEnd >= VariableStoreHeader->Size) {
> > +      DEBUG ((DEBUG_INFO, "%a: end of var list (no space left)\n", __func__));
> > +      break;
> > +    }
> 
> (4) In case of inequality, the variable header is truncated. Accepting
> it as "success" looks doubtful.

We don't know whenever it is supposed to be a valid header, we didn't
check the StartId yet.

Reversing the check ordering looks wrong too (looking at header fields
before we know the header is inside the store).

> (5) In case of equality, the variable header fits, but it is followed by
> no payload at all. I think there are sub-cases to distinguish there:
> 
> - if the StartId differs from 0x55aa, then we may consider the variable
> list to be terminated, and break out of the loop (returning success from
> the function)
> 
> - if the StartId is 0x55aa, then we need to look further, beause we
> can't decide yet. For example, if State is VAR_HEADER_VALID_ONLY (0x7f),
> then it might be fine for the variable header (at the very end of the
> varstore) *not* to be followed by payload bytes (name, data).

Not sure this makes sense.  VAR_HEADER_VALID_ONLY is a temporary state,
while the variable driver writes name and data just after the header,
to be updated to VAR_ADDED when the write completed successfully.  So
I'd expect to never find a header without space for name + data.

> I find this code hard to review because I don't know (and the Intel
> whitepaper doesn't seem to tell) precisely how a valid variable list is
> supposed to be terminated.

Which is why the code logs the condition why it considers the list to be
terminated ...

> (6) I suggest two further checks (within the braces here):
> 
> - enforce
> 
>   VarHeader->NameSize > 0

NameSize >= 4 ?  (room for one char and the terminating null)

> - enforce
> 
>   VarName[VarHeader->NameSize / 2 - 1] == L'\0'

ok

> (This is also important for the immediately subsequent code: we print
> the name!)

Indeed.

> (7) Not really important, I'm just throwing it out: how about logging
> "VarHeader->VendorGuid" too?
> 
> It would require something like this:
> 
>   CONST EFI_GUID  *VarGuid;
> 
>   ...
> 
>   VarGuid = &gZeroGuid;
>   if (VarName == NULL) {
>     ...
>     VarGuid = &VarHeader->VendorGuid;
>     ...
>   }

I think we can just use VarHeader->VendorGuid directly, given that the
guid is part of the fixed header it should be valid even in case the
state is VAR_HEADER_VALID_ONLY.

take care,
  Gerd



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

Re: [edk2-devel] [PATCH v3 1/1] OvmfPkg/VirtNorFlashDxe: sanity-check variables

[Laszlo Ersek]

On 1/3/24 16:11, Gerd Hoffmann wrote:
>   Hi,
> 
>> Second (and worse): the bug. In "OvmfPkg/RiscVVirt/VarStore.fdf.inc", it
>> turns out that we *still* generate the gEfiVariableGuid varstore header
>> signature, in case SECURE_BOOT_ENABLE is FALSE. For some reason, commit
>> 92b27c2e6ada ("OvmfPkg/RiscVVirt: Add build files for Qemu Virt
>> platform", 2023-02-16) did not consider commit d92eaabefbe0 ("OvmfPkg:
>> simplify VARIABLE_STORE_HEADER generation", 2016-02-15), and
>> *resurrected* the non-unified varstore generation for RiscVVirt.
>> Furthermore, RiscVVirt uses "VirtNorFlashDxe" as its platform flash
>> driver. As a result, if you now build RiscVVirt with this patch applied,
>> and with SECURE_BOOT_ENABLE=FALSE, I expect the ValidateFvHeader()
>> function to always fail, becase it will try to validate the contents of
>> the varstore through AUTHENTICATED_VARIABLE_HEADER entries, despite the
>> varstore containing (arguably valid) VARIABLE_HEADER entries.
> 
> I expect it will fail only once.  In case the checks don't pass
> VirtNorFlashDxe will re-initialize the flash varstore with
> gEfiAuthenticatedVariableGuid, so on next boot everything is fine.

Good point about reinit, but it might still needlessly cause the loss of
preexistent variables, so if we can avoid it easily, we should.

> 
>> So here's what I propose:
>>
>> - keep this patch, but *prepend* two other patches:
>>
>> - first, reflect commit d92eaabefbe0 to
>> "OvmfPkg/RiscVVirt/VarStore.fdf.inc" -- only generate the authenticated
>> signature GUID, regardless of SECURE_BOOT_ENABLE,
>>
>> - second, in this function, stop accepting the "gEfiVariableGuid"
>> varstore header signature.
> 
> Makes sense.
> 
>>> +    if (VarHeaderEnd >= VariableStoreHeader->Size) {
>>> +      DEBUG ((DEBUG_INFO, "%a: end of var list (no space left)\n", __func__));
>>> +      break;
>>> +    }
>>
>> (4) In case of inequality, the variable header is truncated. Accepting
>> it as "success" looks doubtful.
> 
> We don't know whenever it is supposed to be a valid header, we didn't
> check the StartId yet.
> 
> Reversing the check ordering looks wrong too (looking at header fields
> before we know the header is inside the store).

Oh I certainly don't imply that we should reverse the order of the
checks; I meant to say (a) we should separate

  VarHeaderEnd > VariableStoreHeader->Size

from

  VarHeaderEnd == VariableStoreHeader->Size

and (b) we should perhaps consider the former condition (i.e.,
inequality) as a hard failure (and not as success), i.e., cause for
reformatting the varstore.

> 
>> (5) In case of equality, the variable header fits, but it is followed by
>> no payload at all. I think there are sub-cases to distinguish there:
>>
>> - if the StartId differs from 0x55aa, then we may consider the variable
>> list to be terminated, and break out of the loop (returning success from
>> the function)
>>
>> - if the StartId is 0x55aa, then we need to look further, beause we
>> can't decide yet. For example, if State is VAR_HEADER_VALID_ONLY (0x7f),
>> then it might be fine for the variable header (at the very end of the
>> varstore) *not* to be followed by payload bytes (name, data).
> 
> Not sure this makes sense.  VAR_HEADER_VALID_ONLY is a temporary state,
> while the variable driver writes name and data just after the header,
> to be updated to VAR_ADDED when the write completed successfully.  So
> I'd expect to never find a header without space for name + data.

I have two comments here:

- if you are right, then I agree it's a good argument for keeping the
two conditions

  VarHeaderEnd > VariableStoreHeader->Size
  VarHeaderEnd == VariableStoreHeader->Size

unified as

  VarHeaderEnd >= VariableStoreHeader->Size

*but* then it only strengthens my argument that the *handling* for this
case should not be a "break" statement, but "return EFI_NOT_FOUND"! (And
then the only successful exit from the loop would be for (StartId !=
0x55aa).)

- Do we know for sure that VAR_HEADER_VALID_ONLY is never expected to be
seen? What if the variable update design defines VAR_HEADER_VALID_ONLY
specifically so that the variable driver can recover from a power loss
"in the middle"? In that case, we should not consider
VAR_HEADER_VALID_ONLY reason for reformatting the whole varstore -- in
fact, the swith statement at the end of the patch tolaretes
VAR_HEADER_VALID_ONLY. So I figure, if we accept VAR_HEADER_VALID_ONLY
in that logic, then we should also accept VAR_HEADER_VALID_ONLY if it's
at the very end of the varstore.

> 
>> I find this code hard to review because I don't know (and the Intel
>> whitepaper doesn't seem to tell) precisely how a valid variable list is
>> supposed to be terminated.
> 
> Which is why the code logs the condition why it considers the list to be
> terminated ...

OK!

> 
>> (6) I suggest two further checks (within the braces here):
>>
>> - enforce
>>
>>   VarHeader->NameSize > 0
> 
> NameSize >= 4 ?  (room for one char and the terminating null)

Sure, that works too.

> 
>> - enforce
>>
>>   VarName[VarHeader->NameSize / 2 - 1] == L'\0'
> 
> ok
> 
>> (This is also important for the immediately subsequent code: we print
>> the name!)
> 
> Indeed.
> 
>> (7) Not really important, I'm just throwing it out: how about logging
>> "VarHeader->VendorGuid" too?
>>
>> It would require something like this:
>>
>>   CONST EFI_GUID  *VarGuid;
>>
>>   ...
>>
>>   VarGuid = &gZeroGuid;
>>   if (VarName == NULL) {
>>     ...
>>     VarGuid = &VarHeader->VendorGuid;
>>     ...
>>   }
> 
> I think we can just use VarHeader->VendorGuid directly, given that the
> guid is part of the fixed header it should be valid even in case the
> state is VAR_HEADER_VALID_ONLY.

Good point -- I think I briefly considered it, but ruled it out because
<guid>:"<unknown>" in the log didn't look useful to me at once. But now
you're making me reconsider -- it simplifies the code, and it doesn't
"hurt" in the log either.

Thanks!
Laszlo



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

[Gerd Hoffmann]

  Hi,

> >> - if the StartId is 0x55aa, then we need to look further, beause we
> >> can't decide yet. For example, if State is VAR_HEADER_VALID_ONLY (0x7f),
> >> then it might be fine for the variable header (at the very end of the
> >> varstore) *not* to be followed by payload bytes (name, data).
> > 
> > Not sure this makes sense.  VAR_HEADER_VALID_ONLY is a temporary state,
> > while the variable driver writes name and data just after the header,
> > to be updated to VAR_ADDED when the write completed successfully.  So
> > I'd expect to never find a header without space for name + data.
> 
> - Do we know for sure that VAR_HEADER_VALID_ONLY is never expected to be
> seen?

Writing goes like this:

  (1) find free space
  (2) write header, with VAR_HEADER_VALID_ONLY.
  (3) write name + data
  (4) update header, set state = VAR_ADDED.

> What if the variable update design defines VAR_HEADER_VALID_ONLY
> specifically so that the variable driver can recover from a power loss
> "in the middle"?

Power loss in step (3) can surely lead to variables in
VAR_HEADER_VALID_ONLY state, and I'd expect the variable driver can
actually recover from that.

[ side note:  The (2) write should be small enough that it fits into the
              flash block write buffer (128 bytes).  Which could be
              important to maintain variable store consistency. ]

Nevertheless we should never find a header at the end of the variable
store, without space allocated for name + date.  Minimal space for the
name is 4 bytes (one char16 + '\0'), for the data 1 byte, alignment
rounds the latter to 4 bytes too, so this should be true:

VarOffset + sizeof(*VarHeader) + 8 <= VariableStoreHeader->Size

> So I figure, if we accept VAR_HEADER_VALID_ONLY in that logic, then we
> should also accept VAR_HEADER_VALID_ONLY if it's at the very end of
> the varstore.

Disagree, see above.  Storing the header at a place which leaves no room
for name + data doesn't make sense to me.

We could go the extra mile and look at the next StartId location, verify
StartId != 0x55aa, in the no-space-left-for-header case.

take care,
  Gerd



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

Re: [edk2-devel] [PATCH v3 1/1] OvmfPkg/VirtNorFlashDxe: sanity-check variables

[Laszlo Ersek]

On 1/4/24 16:06, Gerd Hoffmann wrote:
>   Hi,
> 
>>>> - if the StartId is 0x55aa, then we need to look further, beause we
>>>> can't decide yet. For example, if State is VAR_HEADER_VALID_ONLY (0x7f),
>>>> then it might be fine for the variable header (at the very end of the
>>>> varstore) *not* to be followed by payload bytes (name, data).
>>>
>>> Not sure this makes sense.  VAR_HEADER_VALID_ONLY is a temporary state,
>>> while the variable driver writes name and data just after the header,
>>> to be updated to VAR_ADDED when the write completed successfully.  So
>>> I'd expect to never find a header without space for name + data.
>>
>> - Do we know for sure that VAR_HEADER_VALID_ONLY is never expected to be
>> seen?
> 
> Writing goes like this:
> 
>   (1) find free space
>   (2) write header, with VAR_HEADER_VALID_ONLY.
>   (3) write name + data
>   (4) update header, set state = VAR_ADDED.
> 
>> What if the variable update design defines VAR_HEADER_VALID_ONLY
>> specifically so that the variable driver can recover from a power loss
>> "in the middle"?
> 
> Power loss in step (3) can surely lead to variables in
> VAR_HEADER_VALID_ONLY state, and I'd expect the variable driver can
> actually recover from that.
> 
> [ side note:  The (2) write should be small enough that it fits into the
>               flash block write buffer (128 bytes).  Which could be
>               important to maintain variable store consistency. ]
> 
> Nevertheless we should never find a header at the end of the variable
> store, without space allocated for name + date.  Minimal space for the
> name is 4 bytes (one char16 + '\0'), for the data 1 byte, alignment
> rounds the latter to 4 bytes too, so this should be true:
> 
> VarOffset + sizeof(*VarHeader) + 8 <= VariableStoreHeader->Size
> 
>> So I figure, if we accept VAR_HEADER_VALID_ONLY in that logic, then we
>> should also accept VAR_HEADER_VALID_ONLY if it's at the very end of
>> the varstore.
> 
> Disagree, see above.  Storing the header at a place which leaves no room
> for name + data doesn't make sense to me.

OK, that sounds convincing, thanks!
Laszlo

> We could go the extra mile and look at the next StartId location, verify
> StartId != 0x55aa, in the no-space-left-for-header case.
> 
> take care,
>   Gerd
> 



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