On 11/14/14 12:51, Long, Qin wrote: > 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 <qin.l...@intel.com>
Thank you. Committed as SVN r16389. Cheers Laszlo > > > Best Regards & Thanks, > LONG, Qin > > > -----Original Message----- > From: Laszlo Ersek [mailto:ler...@redhat.com] > Sent: Friday, November 14, 2014 6:34 PM > To: Long, Qin; edk2-devel@lists.sourceforge.net > 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 <ler...@redhat.com> > --- > 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 edk2-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/edk2-devel
