*Thanks*, Gerd! Two comments inline.
> On 27. Feb 2023, at 13:55, Gerd Hoffmann <kra...@redhat.com> wrote: > > Call gRT->GetVariable() directly to read the SecureBoot variable. It is > one byte in size so we can easily place it on the stack instead of > having GetEfiGlobalVariable2() allocate it for us, which avoids a few > possible error cases. > > Skip secure boot checks if (and only if): > > (a) the SecureBoot variable is not present (EFI_NOT_FOUND) according to > the return value, or @Maintainers Would there be any objection to drop this and skip the SB checks only when explicitly disabled? Please explicitly respond even if not, so we don't end up with everyone silently agreeing, but forgetting about the patch after. Thanks! :) > (b) the SecureBoot variable was read successfully and is set to > SECURE_BOOT_MODE_DISABLE. > > Previously the code skipped the secure boot checks on *any* > gRT->GetVariable() error (GetEfiGlobalVariable2 sets the variable > value to NULL in that case) and also on memory allocation failures. > > Fixes: CVE-2019-14560 > Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=2167 > Signed-off-by: Gerd Hoffmann <kra...@redhat.com> > --- > .../DxeImageVerificationLib.c | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git > a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c > b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c > index 66e2f5eaa3c0..f29a27e5a053 100644 > --- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c > +++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c > @@ -1671,7 +1671,8 @@ DxeImageVerificationHandler ( > EFI_IMAGE_EXECUTION_ACTION Action; > WIN_CERTIFICATE *WinCertificate; > UINT32 Policy; > - UINT8 *SecureBoot; > + UINT8 SecureBoot; > + UINTN SecureBootSize; > PE_COFF_LOADER_IMAGE_CONTEXT ImageContext; > UINT32 NumberOfRvaAndSizes; > WIN_CERTIFICATE_EFI_PKCS *PkcsCertData; > @@ -1686,6 +1687,7 @@ DxeImageVerificationHandler ( > RETURN_STATUS PeCoffStatus; > EFI_STATUS HashStatus; > EFI_STATUS DbStatus; > + EFI_STATUS VarStatus; > BOOLEAN IsFound; > > SignatureList = NULL; > @@ -1742,24 +1744,22 @@ DxeImageVerificationHandler ( > CpuDeadLoop (); > } > > - GetEfiGlobalVariable2 (EFI_SECURE_BOOT_MODE_NAME, (VOID **)&SecureBoot, > NULL); > + SecureBootSize = sizeof (SecureBoot); > + VarStatus = gRT->GetVariable (EFI_SECURE_BOOT_MODE_NAME, > &gEfiGlobalVariableGuid, NULL, &SecureBootSize, &SecureBoot); > // > // Skip verification if SecureBoot variable doesn't exist. > // > - if (SecureBoot == NULL) { > + if (VarStatus == EFI_NOT_FOUND) { > return EFI_SUCCESS; > } > > // > // Skip verification if SecureBoot is disabled but not AuditMode > // > - if (*SecureBoot == SECURE_BOOT_MODE_DISABLE) { > - FreePool (SecureBoot); > + if ((VarStatus == EFI_SUCCESS) && (SecureBoot == > SECURE_BOOT_MODE_DISABLE)) { I would check the attributes here as well. They should be BS | RT, but explicitly not NV. This would force the SB checks in case a malicious actor somehow managed to store a persistent disable-value variable (be that a bug, physical access, or other means). Best regards, Marvin > return EFI_SUCCESS; > } > > - FreePool (SecureBoot); > - > // > // Read the Dos header. > // > -- > 2.39.2 > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#100512): https://edk2.groups.io/g/devel/message/100512 Mute This Topic: https://groups.io/mt/97265237/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
