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 <kra...@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 <ler...@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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe:
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
