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

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

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