Laszlo, thanks for catching this. This is one serious parentheses-matching issue.
The patch is good. And I also did one quick validation. Please help to check-in this fix. Reviewed-by: Qin Long <[email protected]> Best Regards & Thanks, LONG, Qin -----Original Message----- From: Laszlo Ersek [mailto:[email protected]] Sent: Friday, November 14, 2014 6:34 PM To: Long, Qin; [email protected] Subject: [PATCH] SecurityPkg: VariableServiceSetVariable(): fix dbt <-> GUID association SVN r16380 ("UEFI 2.4 X509 Certificate Hash and RFC3161 Timestamp Verification support for Secure Boot") broke the "dbt" variable's association with its expected namespace GUID. According to "MdePkg/Include/Guid/ImageAuthentication.h", *all* of the "db", "dbx", and "dbt" (== EFI_IMAGE_SECURITY_DATABASE2) variables have their special meanings in the EFI_IMAGE_SECURITY_DATABASE_GUID namespace. However, the above commit introduced the following expression in VariableServiceSetVariable(): - } else if (CompareGuid (VendorGuid, &gEfiImageSecurityDatabaseGuid) && - ((StrCmp (VariableName, EFI_IMAGE_SECURITY_DATABASE) == 0) || (StrCmp (VariableName, EFI_IMAGE_SECURITY_DATABASE1) == 0))) { + } else if (CompareGuid (VendorGuid, &gEfiImageSecurityDatabaseGuid) && + ((StrCmp (VariableName, EFI_IMAGE_SECURITY_DATABASE) == 0) || (StrCmp (VariableName, EFI_IMAGE_SECURITY_DATABASE1) == 0)) + || (StrCmp (VariableName, EFI_IMAGE_SECURITY_DATABASE2)) == + 0) { Simply replacing the individual expressions with the predicates "GuidMatch", "DbMatch", "DbxMatch", and "DbtMatch", the above transformation becomes: - } else if (GuidMatch && - ((DbMatch) || (DbxMatch))) { + } else if (GuidMatch && + ((DbMatch) || (DbxMatch)) + || DbtMatch) { In shorter form, we change GuidMatch && (DbMatch || DbxMatch) into GuidMatch && (DbMatch || DbxMatch) || DbtMatch which is incorrect, because this way "dbt" will match outside of the intended namespace / GUID. The error was caught by gcc: > SecurityPkg/VariableAuthenticated/RuntimeDxe/Variable.c: In function > 'VariableServiceSetVariable': > > SecurityPkg/VariableAuthenticated/RuntimeDxe/Variable.c:3188:71: error: > suggest parentheses around '&&' within '||' [-Werror=parentheses] > > } else if (CompareGuid (VendorGuid, &gEfiImageSecurityDatabaseGuid) && > > ^ > cc1: all warnings being treated as errors Fix the parentheses. This change may have security implications. Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Laszlo Ersek <[email protected]> --- SecurityPkg/VariableAuthenticated/RuntimeDxe/Variable.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/SecurityPkg/VariableAuthenticated/RuntimeDxe/Variable.c b/SecurityPkg/VariableAuthenticated/RuntimeDxe/Variable.c index 432531f..ac043d9 100644 --- a/SecurityPkg/VariableAuthenticated/RuntimeDxe/Variable.c +++ b/SecurityPkg/VariableAuthenticated/RuntimeDxe/Variable.c @@ -3186,8 +3186,11 @@ VariableServiceSetVariable ( } else if (CompareGuid (VendorGuid, &gEfiGlobalVariableGuid) && (StrCmp (VariableName, EFI_KEY_EXCHANGE_KEY_NAME) == 0)) { Status = ProcessVarWithPk (VariableName, VendorGuid, Data, DataSize, &Variable, Attributes, FALSE); } else if (CompareGuid (VendorGuid, &gEfiImageSecurityDatabaseGuid) && - ((StrCmp (VariableName, EFI_IMAGE_SECURITY_DATABASE) == 0) || (StrCmp (VariableName, EFI_IMAGE_SECURITY_DATABASE1) == 0)) - || (StrCmp (VariableName, EFI_IMAGE_SECURITY_DATABASE2)) == 0) { + ((StrCmp (VariableName, EFI_IMAGE_SECURITY_DATABASE) == 0) || + (StrCmp (VariableName, EFI_IMAGE_SECURITY_DATABASE1) == 0) || + (StrCmp (VariableName, EFI_IMAGE_SECURITY_DATABASE2) == 0) + ) + ) { Status = ProcessVarWithPk (VariableName, VendorGuid, Data, DataSize, &Variable, Attributes, FALSE); if (EFI_ERROR (Status)) { Status = ProcessVarWithKek (VariableName, VendorGuid, Data, DataSize, &Variable, Attributes); -- 1.8.3.1 ------------------------------------------------------------------------------ Comprehensive Server Monitoring with Site24x7. Monitor 10 servers for $9/Month. Get alerted through email, SMS, voice calls or mobile push notifications. Take corrective actions from your mobile device. http://pubads.g.doubleclick.net/gampad/clk?id=154624111&iu=/4140/ostg.clktrk _______________________________________________ edk2-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/edk2-devel
