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

["Laszlo Ersek" <lersek@redhat.com>]

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]
-=-=-=-=-=-=-=-=-=-=-=-