* [edk2-devel] [PATCH v4 0/3] OvmfPkg/VirtNorFlashDxe: sanity-check variables @ 2024-01-08 19:21 Gerd Hoffmann 2024-01-08 19:21 ` [edk2-devel] [PATCH v4 1/3] OvmfPkg/RiscVVirt: use gEfiAuthenticatedVariableGuid unconditionally Gerd Hoffmann ` (2 more replies) 0 siblings, 3 replies; 8+ messages in thread From: Gerd Hoffmann @ 2024-01-08 19:21 UTC (permalink / raw) To: devel Cc: oliver, Laszlo Ersek, Jiewen Yao, Gerd Hoffmann, Ard Biesheuvel, Sunil V L, Andrei Warkentin v4: - turn into a little patch series. - stop using gEfiVariableGuid in risc-v. - stop accepting gEfiVariableGuid in VirtNorFlashDxe. - refine variable checking. Gerd Hoffmann (3): OvmfPkg/RiscVVirt: use gEfiAuthenticatedVariableGuid unconditionally OvmfPkg/VirtNorFlashDxe: stop accepting gEfiVariableGuid OvmfPkg/VirtNorFlashDxe: sanity-check variables OvmfPkg/VirtNorFlashDxe/VirtNorFlashDxe.inf | 1 + OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c | 143 ++++++++++++++++++-- OvmfPkg/RiscVVirt/VarStore.fdf.inc | 9 +- 3 files changed, 137 insertions(+), 16 deletions(-) -- 2.43.0 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#113408): https://edk2.groups.io/g/devel/message/113408 Mute This Topic: https://groups.io/mt/103605053/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 8+ messages in thread
* [edk2-devel] [PATCH v4 1/3] OvmfPkg/RiscVVirt: use gEfiAuthenticatedVariableGuid unconditionally 2024-01-08 19:21 [edk2-devel] [PATCH v4 0/3] OvmfPkg/VirtNorFlashDxe: sanity-check variables Gerd Hoffmann @ 2024-01-08 19:21 ` Gerd Hoffmann 2024-01-09 7:33 ` Sunil V L 2024-01-09 8:27 ` Laszlo Ersek 2024-01-08 19:21 ` [edk2-devel] [PATCH v4 2/3] OvmfPkg/VirtNorFlashDxe: stop accepting gEfiVariableGuid Gerd Hoffmann 2024-01-08 19:21 ` [edk2-devel] [PATCH v4 3/3] OvmfPkg/VirtNorFlashDxe: sanity-check variables Gerd Hoffmann 2 siblings, 2 replies; 8+ messages in thread From: Gerd Hoffmann @ 2024-01-08 19:21 UTC (permalink / raw) To: devel Cc: oliver, Laszlo Ersek, Jiewen Yao, Gerd Hoffmann, Ard Biesheuvel, Sunil V L, Andrei Warkentin ArmVirt and OVMF are doing the same. See commit d92eaabefbe0 ("OvmfPkg: simplify VARIABLE_STORE_HEADER generation") for details. Suggested-by: László Érsek <lersek@redhat.com> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- OvmfPkg/RiscVVirt/VarStore.fdf.inc | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/OvmfPkg/RiscVVirt/VarStore.fdf.inc b/OvmfPkg/RiscVVirt/VarStore.fdf.inc index aba32315cc37..6679c246b3ea 100644 --- a/OvmfPkg/RiscVVirt/VarStore.fdf.inc +++ b/OvmfPkg/RiscVVirt/VarStore.fdf.inc @@ -36,19 +36,12 @@ # Blockmap[1]: End 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, ## This is the VARIABLE_STORE_HEADER -!if $(SECURE_BOOT_ENABLE) == TRUE + # It is compatible with SECURE_BOOT_ENABLE == FALSE as well. # Signature: gEfiAuthenticatedVariableGuid = # { 0xaaf32c78, 0x947b, 0x439a, # { 0xa1, 0x80, 0x2e, 0x14, 0x4e, 0xc3, 0x77, 0x92 }} 0x78, 0x2c, 0xf3, 0xaa, 0x7b, 0x94, 0x9a, 0x43, 0xa1, 0x80, 0x2e, 0x14, 0x4e, 0xc3, 0x77, 0x92, -!else - # Signature: gEfiVariableGuid = - # { 0xddcf3616, 0x3275, 0x4164, - # { 0x98, 0xb6, 0xfe, 0x85, 0x70, 0x7f, 0xfe, 0x7d }} - 0x16, 0x36, 0xcf, 0xdd, 0x75, 0x32, 0x64, 0x41, - 0x98, 0xb6, 0xfe, 0x85, 0x70, 0x7f, 0xfe, 0x7d, -!endif # Size: 0x40000 (gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize) - # 0x48 (size of EFI_FIRMWARE_VOLUME_HEADER) = 0x3FFB8 # This can speed up the Variable Dispatch a bit. -- 2.43.0 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#113409): https://edk2.groups.io/g/devel/message/113409 Mute This Topic: https://groups.io/mt/103605055/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [edk2-devel] [PATCH v4 1/3] OvmfPkg/RiscVVirt: use gEfiAuthenticatedVariableGuid unconditionally 2024-01-08 19:21 ` [edk2-devel] [PATCH v4 1/3] OvmfPkg/RiscVVirt: use gEfiAuthenticatedVariableGuid unconditionally Gerd Hoffmann @ 2024-01-09 7:33 ` Sunil V L 2024-01-09 8:27 ` Laszlo Ersek 1 sibling, 0 replies; 8+ messages in thread From: Sunil V L @ 2024-01-09 7:33 UTC (permalink / raw) To: Gerd Hoffmann Cc: devel, oliver, Laszlo Ersek, Jiewen Yao, Ard Biesheuvel, Andrei Warkentin On Mon, Jan 08, 2024 at 08:21:21PM +0100, Gerd Hoffmann wrote: > ArmVirt and OVMF are doing the same. > > See commit d92eaabefbe0 ("OvmfPkg: simplify VARIABLE_STORE_HEADER > generation") for details. > > Suggested-by: László Érsek <lersek@redhat.com> > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > --- Thanks!, Gerd. Looks good to me. Reviewed-by: Sunil V L <sunilvl@ventanamicro.com> -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#113425): https://edk2.groups.io/g/devel/message/113425 Mute This Topic: https://groups.io/mt/103605055/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [edk2-devel] [PATCH v4 1/3] OvmfPkg/RiscVVirt: use gEfiAuthenticatedVariableGuid unconditionally 2024-01-08 19:21 ` [edk2-devel] [PATCH v4 1/3] OvmfPkg/RiscVVirt: use gEfiAuthenticatedVariableGuid unconditionally Gerd Hoffmann 2024-01-09 7:33 ` Sunil V L @ 2024-01-09 8:27 ` Laszlo Ersek 1 sibling, 0 replies; 8+ messages in thread From: Laszlo Ersek @ 2024-01-09 8:27 UTC (permalink / raw) To: Gerd Hoffmann, devel Cc: oliver, Jiewen Yao, Ard Biesheuvel, Sunil V L, Andrei Warkentin On 1/8/24 20:21, Gerd Hoffmann wrote: > ArmVirt and OVMF are doing the same. > > See commit d92eaabefbe0 ("OvmfPkg: simplify VARIABLE_STORE_HEADER > generation") for details. > > Suggested-by: László Érsek <lersek@redhat.com> > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > --- > OvmfPkg/RiscVVirt/VarStore.fdf.inc | 9 +-------- > 1 file changed, 1 insertion(+), 8 deletions(-) > > diff --git a/OvmfPkg/RiscVVirt/VarStore.fdf.inc b/OvmfPkg/RiscVVirt/VarStore.fdf.inc > index aba32315cc37..6679c246b3ea 100644 > --- a/OvmfPkg/RiscVVirt/VarStore.fdf.inc > +++ b/OvmfPkg/RiscVVirt/VarStore.fdf.inc > @@ -36,19 +36,12 @@ > # Blockmap[1]: End > 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > ## This is the VARIABLE_STORE_HEADER > -!if $(SECURE_BOOT_ENABLE) == TRUE > + # It is compatible with SECURE_BOOT_ENABLE == FALSE as well. > # Signature: gEfiAuthenticatedVariableGuid = > # { 0xaaf32c78, 0x947b, 0x439a, > # { 0xa1, 0x80, 0x2e, 0x14, 0x4e, 0xc3, 0x77, 0x92 }} > 0x78, 0x2c, 0xf3, 0xaa, 0x7b, 0x94, 0x9a, 0x43, > 0xa1, 0x80, 0x2e, 0x14, 0x4e, 0xc3, 0x77, 0x92, > -!else > - # Signature: gEfiVariableGuid = > - # { 0xddcf3616, 0x3275, 0x4164, > - # { 0x98, 0xb6, 0xfe, 0x85, 0x70, 0x7f, 0xfe, 0x7d }} > - 0x16, 0x36, 0xcf, 0xdd, 0x75, 0x32, 0x64, 0x41, > - 0x98, 0xb6, 0xfe, 0x85, 0x70, 0x7f, 0xfe, 0x7d, > -!endif > # Size: 0x40000 (gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize) - > # 0x48 (size of EFI_FIRMWARE_VOLUME_HEADER) = 0x3FFB8 > # This can speed up the Variable Dispatch a bit. Reviewed-by: Laszlo Ersek <lersek@redhat.com> -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#113431): https://edk2.groups.io/g/devel/message/113431 Mute This Topic: https://groups.io/mt/103605055/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 8+ messages in thread
* [edk2-devel] [PATCH v4 2/3] OvmfPkg/VirtNorFlashDxe: stop accepting gEfiVariableGuid 2024-01-08 19:21 [edk2-devel] [PATCH v4 0/3] OvmfPkg/VirtNorFlashDxe: sanity-check variables Gerd Hoffmann 2024-01-08 19:21 ` [edk2-devel] [PATCH v4 1/3] OvmfPkg/RiscVVirt: use gEfiAuthenticatedVariableGuid unconditionally Gerd Hoffmann @ 2024-01-08 19:21 ` Gerd Hoffmann 2024-01-09 8:30 ` Laszlo Ersek 2024-01-08 19:21 ` [edk2-devel] [PATCH v4 3/3] OvmfPkg/VirtNorFlashDxe: sanity-check variables Gerd Hoffmann 2 siblings, 1 reply; 8+ messages in thread From: Gerd Hoffmann @ 2024-01-08 19:21 UTC (permalink / raw) To: devel Cc: oliver, Laszlo Ersek, Jiewen Yao, Gerd Hoffmann, Ard Biesheuvel, Sunil V L, Andrei Warkentin Only accept gEfiAuthenticatedVariableGuid when checking the variable store header in ValidateFvHeader(). The edk2 code base has been switched to use the authenticated varstore format unconditionally (even in case secure boot is not used or supported) a few years ago. Suggested-by: László Érsek <lersek@redhat.com> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c b/OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c index 5ee98e9b595a..9a614ae4b24d 100644 --- a/OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c +++ b/OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c @@ -239,9 +239,7 @@ ValidateFvHeader ( VariableStoreHeader = (VARIABLE_STORE_HEADER *)((UINTN)FwVolHeader + FwVolHeader->HeaderLength); // Check the Variable Store Guid - if (!CompareGuid (&VariableStoreHeader->Signature, &gEfiVariableGuid) && - !CompareGuid (&VariableStoreHeader->Signature, &gEfiAuthenticatedVariableGuid)) - { + if (!CompareGuid (&VariableStoreHeader->Signature, &gEfiAuthenticatedVariableGuid)) { DEBUG (( DEBUG_INFO, "%a: Variable Store Guid non-compatible\n", -- 2.43.0 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#113410): https://edk2.groups.io/g/devel/message/113410 Mute This Topic: https://groups.io/mt/103605076/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [edk2-devel] [PATCH v4 2/3] OvmfPkg/VirtNorFlashDxe: stop accepting gEfiVariableGuid 2024-01-08 19:21 ` [edk2-devel] [PATCH v4 2/3] OvmfPkg/VirtNorFlashDxe: stop accepting gEfiVariableGuid Gerd Hoffmann @ 2024-01-09 8:30 ` Laszlo Ersek 0 siblings, 0 replies; 8+ messages in thread From: Laszlo Ersek @ 2024-01-09 8:30 UTC (permalink / raw) To: Gerd Hoffmann, devel Cc: oliver, Jiewen Yao, Ard Biesheuvel, Sunil V L, Andrei Warkentin On 1/8/24 20:21, Gerd Hoffmann wrote: > Only accept gEfiAuthenticatedVariableGuid when checking the variable > store header in ValidateFvHeader(). > > The edk2 code base has been switched to use the authenticated varstore > format unconditionally (even in case secure boot is not used or > supported) a few years ago. > > Suggested-by: László Érsek <lersek@redhat.com> > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > --- > OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c b/OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c > index 5ee98e9b595a..9a614ae4b24d 100644 > --- a/OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c > +++ b/OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c > @@ -239,9 +239,7 @@ ValidateFvHeader ( > VariableStoreHeader = (VARIABLE_STORE_HEADER *)((UINTN)FwVolHeader + FwVolHeader->HeaderLength); > > // Check the Variable Store Guid > - if (!CompareGuid (&VariableStoreHeader->Signature, &gEfiVariableGuid) && > - !CompareGuid (&VariableStoreHeader->Signature, &gEfiAuthenticatedVariableGuid)) > - { > + if (!CompareGuid (&VariableStoreHeader->Signature, &gEfiAuthenticatedVariableGuid)) { > DEBUG (( > DEBUG_INFO, > "%a: Variable Store Guid non-compatible\n", Reviewed-by: Laszlo Ersek <lersek@redhat.com> -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#113432): https://edk2.groups.io/g/devel/message/113432 Mute This Topic: https://groups.io/mt/103605076/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 8+ messages in thread
* [edk2-devel] [PATCH v4 3/3] OvmfPkg/VirtNorFlashDxe: sanity-check variables 2024-01-08 19:21 [edk2-devel] [PATCH v4 0/3] OvmfPkg/VirtNorFlashDxe: sanity-check variables Gerd Hoffmann 2024-01-08 19:21 ` [edk2-devel] [PATCH v4 1/3] OvmfPkg/RiscVVirt: use gEfiAuthenticatedVariableGuid unconditionally Gerd Hoffmann 2024-01-08 19:21 ` [edk2-devel] [PATCH v4 2/3] OvmfPkg/VirtNorFlashDxe: stop accepting gEfiVariableGuid Gerd Hoffmann @ 2024-01-08 19:21 ` Gerd Hoffmann 2024-01-09 9:04 ` Laszlo Ersek 2 siblings, 1 reply; 8+ messages in thread From: Gerd Hoffmann @ 2024-01-08 19:21 UTC (permalink / raw) To: devel Cc: oliver, Laszlo Ersek, Jiewen Yao, Gerd Hoffmann, Ard Biesheuvel, Sunil V L, Andrei Warkentin 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 | 139 +++++++++++++++++++- 2 files changed, 135 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..56148e9f1f95 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,13 @@ 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; + RETURN_STATUS Status; FwVolHeader = (EFI_FIRMWARE_VOLUME_HEADER *)Instance->RegionBaseAddress; @@ -258,6 +261,132 @@ 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; + CHAR16 *VarName; + CONST CHAR8 *VarState; + + 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) { + DEBUG ((DEBUG_INFO, "%a: end of var list (no space left)\n", __func__)); + break; + } + + 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) || + (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; } -- 2.43.0 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#113411): https://edk2.groups.io/g/devel/message/113411 Mute This Topic: https://groups.io/mt/103605077/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [edk2-devel] [PATCH v4 3/3] OvmfPkg/VirtNorFlashDxe: sanity-check variables 2024-01-08 19:21 ` [edk2-devel] [PATCH v4 3/3] OvmfPkg/VirtNorFlashDxe: sanity-check variables Gerd Hoffmann @ 2024-01-09 9:04 ` Laszlo Ersek 0 siblings, 0 replies; 8+ messages in thread From: Laszlo Ersek @ 2024-01-09 9:04 UTC (permalink / raw) To: Gerd Hoffmann, devel Cc: oliver, Jiewen Yao, Ard Biesheuvel, Sunil V L, Andrei Warkentin On 1/8/24 20:21, 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 | 139 +++++++++++++++++++- > 2 files changed, 135 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..56148e9f1f95 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,13 @@ 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; > + RETURN_STATUS Status; (1) Status could have been moved in the "for" loop, AFAICT -- also mentioned in my previous review --, but it's not a sticking point. > > FwVolHeader = (EFI_FIRMWARE_VOLUME_HEADER *)Instance->RegionBaseAddress; > > @@ -258,6 +261,132 @@ 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; > + CHAR16 *VarName; (2) Should have noticed in my previous review: VarName could be CONST-ified as well. Totally minor. > + CONST CHAR8 *VarState; > + > + 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) { > + DEBUG ((DEBUG_INFO, "%a: end of var list (no space left)\n", __func__)); > + break; > + } (3) I *still* don't understand this. In the v3 discussion: - we agreed that the ">" case was clearly unacceptable, - and you convinced me that the "=" case was also unacceptable (because that could only potentially accommodate a VAR_HEADER_VALID_ONLY state header without subsequent space for name & data, and we agreed that a var header like that, residing *permanently* in the varstore, was not acceptable). Fine: that's reason for keeping the unified ">=" condition. But my point in turn (which you didn't reflect upon, and seem not to have addressed in this patch) was that this condition means that the varstore is *bogus*, and should be reformatted. In my previous review I recommended that we replace the "break" here -- which leads to the function returning EFI_SUCCESS -- with "return EFI_NOT_FOUND" -- which causes the varstore to be reformatted. And then, as I wrote, the only successful exit from the loop would be the subsequent "break" below: > + > + VarHeader = (VOID *)((UINTN)VariableStoreHeader + VarOffset); > + if (VarHeader->StartId != 0x55aa) { > + DEBUG ((DEBUG_INFO, "%a: end of var list (no startid)\n", __func__)); > + break; > + } So: what's wrong with returning EFI_NOT_FOUND if VarHeaderEnd >= VariableStoreHeader->Size evaluates to true? > + > + 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) || (4) Minor: I don't understand why uncrustify does not catch this, but the edk2 coding style requires us to spell this as (VarHeader->NameSize & 1) != 0 to my understanding. Apologies for not noticing it in my previous review. > + (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; > } > - If you can explain the "break" under (3), I'm happy to R-b this patch (and to merge this v4 series). The rest of my comments ((1), (2), (4)) doesn't justify a respin, in itself. - If you decide to replace the "break" with "return EFI_NOT_FOUND" under (3), then addressing the rest ((1), (2), (4)) would be nice, in v5. Thanks! Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#113435): https://edk2.groups.io/g/devel/message/113435 Mute This Topic: https://groups.io/mt/103605077/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2024-01-09 9:04 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-01-08 19:21 [edk2-devel] [PATCH v4 0/3] OvmfPkg/VirtNorFlashDxe: sanity-check variables Gerd Hoffmann 2024-01-08 19:21 ` [edk2-devel] [PATCH v4 1/3] OvmfPkg/RiscVVirt: use gEfiAuthenticatedVariableGuid unconditionally Gerd Hoffmann 2024-01-09 7:33 ` Sunil V L 2024-01-09 8:27 ` Laszlo Ersek 2024-01-08 19:21 ` [edk2-devel] [PATCH v4 2/3] OvmfPkg/VirtNorFlashDxe: stop accepting gEfiVariableGuid Gerd Hoffmann 2024-01-09 8:30 ` Laszlo Ersek 2024-01-08 19:21 ` [edk2-devel] [PATCH v4 3/3] OvmfPkg/VirtNorFlashDxe: sanity-check variables Gerd Hoffmann 2024-01-09 9:04 ` Laszlo Ersek
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox