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

["Mike Maslenkin" <mike.maslenkin@gmail.com>]

Hi Gerd,

On Tue, Dec 5, 2023 at 4:51 PM Gerd Hoffmann <kraxel@redhat.com> 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/VirtNorFlashFvb.c | 82 +++++++++++++++++++++--
>  1 file changed, 77 insertions(+), 5 deletions(-)
>
> diff --git a/OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c b/OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c
> index 5ee98e9b595a..72a197e5aa20 100644
> --- a/OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c
> +++ b/OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c
> @@ -185,11 +185,16 @@ 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;
> +  AUTHENTICATED_VARIABLE_HEADER  *VarHeader;
> +  UINTN                          VarSize;
> +  CHAR16                         *VarName;
> +  CHAR8                          *VarState;
> +  UINTN                          VariableStoreLength;
> +  UINTN                          FvLength;
>
>    FwVolHeader = (EFI_FIRMWARE_VOLUME_HEADER *)Instance->RegionBaseAddress;
>
> @@ -260,6 +265,73 @@ ValidateFvHeader (
>      return EFI_NOT_FOUND;
>    }
>
> +  // check variables
> +  DEBUG ((DEBUG_INFO, "%a: checking variables\n", __func__));
> +  VarOffset = sizeof (*VariableStoreHeader);
> +  while (VarOffset + sizeof (*VarHeader) < VariableStoreHeader->Size) {
> +    VarHeader = (VOID *)((UINTN)VariableStoreHeader + VarOffset);
> +    if (VarHeader->StartId != 0x55aa) {
> +      DEBUG ((DEBUG_INFO, "%a: end of var list\n", __func__));
> +      break;
> +    }
> +
> +    VarSize = sizeof (*VarHeader) + VarHeader->NameSize + VarHeader->DataSize;
> +    if (VarOffset + VarSize > VariableStoreHeader->Size) {
> +      DEBUG ((
> +        DEBUG_ERROR,
> +        "%a: invalid variable size: 0x%x + 0x%x + 0x%x + 0x%x > 0x%x\n",
> +        __func__,
> +        VarOffset,
> +        sizeof (*VarHeader),
> +        VarHeader->NameSize,
> +        VarHeader->DataSize,
> +        VariableStoreHeader->Size
> +        ));
> +      return EFI_NOT_FOUND;
> +    }
> +
> +    VarName = (VOID *)((UINTN)VariableStoreHeader + VarOffset
> +                       + sizeof (*VarHeader));
> +    switch (VarHeader->State) {
> +      case VAR_HEADER_VALID_ONLY:
> +        VarState = "header-ok";
> +        VarName  = L"<unknown>";
> +        break;
> +      case VAR_ADDED:
> +        VarState = "ok";
> +        break;
> +      case VAR_ADDED &VAR_IN_DELETED_TRANSITION:
> +        VarState = "del-in-transition";
> +        break;
> +      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__,
> +          VarState
> +          ));

Did you want to print VarHeader->State?

Regards,
Mike.


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