Hi Gerd,
On Thu, 7 Dec 2023 at 10:44, Gerd Hoffmann <kra...@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 <kra...@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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
