* [edk2-devel] [PATCH v5 0/3] OvmfPkg/VirtNorFlashDxe: sanity-check variables @ 2024-01-09 11:28 Gerd Hoffmann 2024-01-09 11:29 ` [edk2-devel] [PATCH v5 1/3] OvmfPkg/RiscVVirt: use gEfiAuthenticatedVariableGuid unconditionally Gerd Hoffmann ` (2 more replies) 0 siblings, 3 replies; 7+ messages in thread From: Gerd Hoffmann @ 2024-01-09 11:28 UTC (permalink / raw) To: devel Cc: Sunil V L, Gerd Hoffmann, Jiewen Yao, oliver, Laszlo Ersek, Andrei Warkentin, Ard Biesheuvel v5: - pick up review tags - more variable check fine tuning. 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 | 151 ++++++++++++++++++-- OvmfPkg/RiscVVirt/VarStore.fdf.inc | 9 +- 3 files changed, 145 insertions(+), 16 deletions(-) -- 2.43.0 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#113444): https://edk2.groups.io/g/devel/message/113444 Mute This Topic: https://groups.io/mt/103617815/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 7+ messages in thread
* [edk2-devel] [PATCH v5 1/3] OvmfPkg/RiscVVirt: use gEfiAuthenticatedVariableGuid unconditionally 2024-01-09 11:28 [edk2-devel] [PATCH v5 0/3] OvmfPkg/VirtNorFlashDxe: sanity-check variables Gerd Hoffmann @ 2024-01-09 11:29 ` Gerd Hoffmann 2024-01-09 11:29 ` [edk2-devel] [PATCH v5 2/3] OvmfPkg/VirtNorFlashDxe: stop accepting gEfiVariableGuid Gerd Hoffmann 2024-01-09 11:29 ` [edk2-devel] [PATCH v5 3/3] OvmfPkg/VirtNorFlashDxe: sanity-check variables Gerd Hoffmann 2 siblings, 0 replies; 7+ messages in thread From: Gerd Hoffmann @ 2024-01-09 11:29 UTC (permalink / raw) To: devel Cc: Sunil V L, Gerd Hoffmann, Jiewen Yao, oliver, Laszlo Ersek, Andrei Warkentin, Ard Biesheuvel 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> Reviewed-by: Sunil V L <sunilvl@ventanamicro.com> Reviewed-by: Laszlo Ersek <lersek@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 (#113442): https://edk2.groups.io/g/devel/message/113442 Mute This Topic: https://groups.io/mt/103617813/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] 7+ messages in thread
* [edk2-devel] [PATCH v5 2/3] OvmfPkg/VirtNorFlashDxe: stop accepting gEfiVariableGuid 2024-01-09 11:28 [edk2-devel] [PATCH v5 0/3] OvmfPkg/VirtNorFlashDxe: sanity-check variables Gerd Hoffmann 2024-01-09 11:29 ` [edk2-devel] [PATCH v5 1/3] OvmfPkg/RiscVVirt: use gEfiAuthenticatedVariableGuid unconditionally Gerd Hoffmann @ 2024-01-09 11:29 ` Gerd Hoffmann 2024-01-09 11:29 ` [edk2-devel] [PATCH v5 3/3] OvmfPkg/VirtNorFlashDxe: sanity-check variables Gerd Hoffmann 2 siblings, 0 replies; 7+ messages in thread From: Gerd Hoffmann @ 2024-01-09 11:29 UTC (permalink / raw) To: devel Cc: Sunil V L, Gerd Hoffmann, Jiewen Yao, oliver, Laszlo Ersek, Andrei Warkentin, Ard Biesheuvel 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> Reviewed-by: Laszlo Ersek <lersek@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 (#113443): https://edk2.groups.io/g/devel/message/113443 Mute This Topic: https://groups.io/mt/103617814/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] 7+ messages in thread
* [edk2-devel] [PATCH v5 3/3] OvmfPkg/VirtNorFlashDxe: sanity-check variables 2024-01-09 11:28 [edk2-devel] [PATCH v5 0/3] OvmfPkg/VirtNorFlashDxe: sanity-check variables Gerd Hoffmann 2024-01-09 11:29 ` [edk2-devel] [PATCH v5 1/3] OvmfPkg/RiscVVirt: use gEfiAuthenticatedVariableGuid unconditionally Gerd Hoffmann 2024-01-09 11:29 ` [edk2-devel] [PATCH v5 2/3] OvmfPkg/VirtNorFlashDxe: stop accepting gEfiVariableGuid Gerd Hoffmann @ 2024-01-09 11:29 ` Gerd Hoffmann 2024-01-09 13:14 ` Laszlo Ersek 2 siblings, 1 reply; 7+ messages in thread From: Gerd Hoffmann @ 2024-01-09 11:29 UTC (permalink / raw) To: devel Cc: Sunil V L, Gerd Hoffmann, Jiewen Yao, oliver, Laszlo Ersek, Andrei Warkentin, Ard Biesheuvel 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 | 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; + } + + 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; } -- 2.43.0 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#113441): https://edk2.groups.io/g/devel/message/113441 Mute This Topic: https://groups.io/mt/103617812/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] 7+ messages in thread
* Re: [edk2-devel] [PATCH v5 3/3] OvmfPkg/VirtNorFlashDxe: sanity-check variables 2024-01-09 11:29 ` [edk2-devel] [PATCH v5 3/3] OvmfPkg/VirtNorFlashDxe: sanity-check variables Gerd Hoffmann @ 2024-01-09 13:14 ` Laszlo Ersek 2024-01-09 14:11 ` Gerd Hoffmann 0 siblings, 1 reply; 7+ messages in thread From: Laszlo Ersek @ 2024-01-09 13:14 UTC (permalink / raw) To: devel, kraxel Cc: Sunil V L, Jiewen Yao, oliver, Andrei Warkentin, Ard Biesheuvel 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 <kraxel@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 <lersek@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/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] 7+ messages in thread
* Re: [edk2-devel] [PATCH v5 3/3] OvmfPkg/VirtNorFlashDxe: sanity-check variables 2024-01-09 13:14 ` Laszlo Ersek @ 2024-01-09 14:11 ` Gerd Hoffmann 2024-01-09 16:36 ` Laszlo Ersek 0 siblings, 1 reply; 7+ messages in thread From: Gerd Hoffmann @ 2024-01-09 14:11 UTC (permalink / raw) To: Laszlo Ersek Cc: devel, Sunil V L, Jiewen Yao, oliver, Andrei Warkentin, Ard Biesheuvel Hi, > 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, I happily accept that service offer ;) thanks, Gerd -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#113449): https://edk2.groups.io/g/devel/message/113449 Mute This Topic: https://groups.io/mt/103617812/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [edk2-devel] [PATCH v5 3/3] OvmfPkg/VirtNorFlashDxe: sanity-check variables 2024-01-09 14:11 ` Gerd Hoffmann @ 2024-01-09 16:36 ` Laszlo Ersek 0 siblings, 0 replies; 7+ messages in thread From: Laszlo Ersek @ 2024-01-09 16:36 UTC (permalink / raw) To: devel, kraxel Cc: Sunil V L, Jiewen Yao, oliver, Andrei Warkentin, Ard Biesheuvel On 1/9/24 15:11, Gerd Hoffmann wrote: > Hi, > >> 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, > > I happily accept that service offer ;) Series merged as commit range 08a6528bac38..4a443f73fd67, via <https://github.com/tianocore/edk2/pull/5240>. (Last three commits in the PR.) Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#113484): https://edk2.groups.io/g/devel/message/113484 Mute This Topic: https://groups.io/mt/103617812/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] 7+ messages in thread
end of thread, other threads:[~2024-01-09 16:36 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-01-09 11:28 [edk2-devel] [PATCH v5 0/3] OvmfPkg/VirtNorFlashDxe: sanity-check variables Gerd Hoffmann 2024-01-09 11:29 ` [edk2-devel] [PATCH v5 1/3] OvmfPkg/RiscVVirt: use gEfiAuthenticatedVariableGuid unconditionally Gerd Hoffmann 2024-01-09 11:29 ` [edk2-devel] [PATCH v5 2/3] OvmfPkg/VirtNorFlashDxe: stop accepting gEfiVariableGuid Gerd Hoffmann 2024-01-09 11:29 ` [edk2-devel] [PATCH v5 3/3] OvmfPkg/VirtNorFlashDxe: sanity-check variables Gerd Hoffmann 2024-01-09 13:14 ` Laszlo Ersek 2024-01-09 14:11 ` Gerd Hoffmann 2024-01-09 16:36 ` Laszlo Ersek
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox