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

["Ard Biesheuvel" <ardb@kernel.org>]

Hi Gerd,

On Thu, 7 Dec 2023 at 10:44, 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>

This seems like a good idea to me, and the change looks generally
fine, except for one nit below:

> ---
>  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..1bfb14495abd 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:

This looks odd, so please add a comment explaining why these constants
are constructed this way.


> +        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__,
> +          VarHeader->State
> +          ));
> +        return EFI_NOT_FOUND;
> +    }
> +
> +    DEBUG ((
> +      DEBUG_VERBOSE,
> +      "%a: +0x%04x: name=0x%x data=0x%x '%s' (%a)\n",
> +      __func__,
> +      VarOffset,
> +      VarHeader->NameSize,
> +      VarHeader->DataSize,
> +      VarName,
> +      VarState
> +      ));
> +
> +    VarSize    = (VarSize + 3) & ~3; // align
> +    VarOffset += VarSize;
> +  }
> +
>    return EFI_SUCCESS;
>  }
>
> --
> 2.43.0
>


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