Re: [PATCH 1/1] SecurityPkg/DxeImageVerificationLib: Check result of GetEfiGlobalVariable2

["Gerd Hoffmann" <kraxel@redhat.com>]

  Hi,

> > (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! :)

I hold back v2, waiting for an answer here.

> > -  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).

Like this (incremental fixup)?

Do we have macros for variable attribute checking?
Havn't seen anything while skimming variable-related headers ...

take care,
  Gerd

diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
index f29a27e5a053..79c784f77ac8 100644
--- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
+++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
@@ -1688,6 +1688,7 @@ DxeImageVerificationHandler (
   EFI_STATUS                    HashStatus;
   EFI_STATUS                    DbStatus;
   EFI_STATUS                    VarStatus;
+  UINT32                        VarAttr;
   BOOLEAN                       IsFound;
 
   SignatureList     = NULL;
@@ -1745,7 +1746,7 @@ DxeImageVerificationHandler (
   }
 
   SecureBootSize = sizeof (SecureBoot);
-  VarStatus      = gRT->GetVariable (EFI_SECURE_BOOT_MODE_NAME, &gEfiGlobalVariableGuid, NULL, &SecureBootSize, &SecureBoot);
+  VarStatus      = gRT->GetVariable (EFI_SECURE_BOOT_MODE_NAME, &gEfiGlobalVariableGuid, &VarAttr, &SecureBootSize, &SecureBoot);
   //
   // Skip verification if SecureBoot variable doesn't exist.
   //
@@ -1756,7 +1757,12 @@ DxeImageVerificationHandler (
   //
   // Skip verification if SecureBoot is disabled but not AuditMode
   //
-  if ((VarStatus == EFI_SUCCESS) && (SecureBoot == SECURE_BOOT_MODE_DISABLE)) {
+  if ((VarStatus == EFI_SUCCESS) &&
+      !(VarAttr & EFI_VARIABLE_NON_VOLATILE) &&
+      (VarAttr & EFI_VARIABLE_BOOTSERVICE_ACCESS) &&
+      (VarAttr & EFI_VARIABLE_RUNTIME_ACCESS) &&
+      (SecureBoot == SECURE_BOOT_MODE_DISABLE))
+  {
     return EFI_SUCCESS;
   }