Star: Why do we need to add HashInterfaceHob->SupportedHashMask = 0? HashInterfaceHob is internally maintained and accessed by HashLibRouterPei. There is no impact to leave the value after module has been re-shadowed.
From: Zeng, Star Sent: Wednesday, March 7, 2018 11:10 PM To: Marc-André Lureau <marcandre.lur...@gmail.com>; edk2-devel@lists.01.org Cc: Laszlo Ersek <ler...@redhat.com>; Yao, Jiewen <jiewen....@intel.com>; Zhang, Chao B <chao.b.zh...@intel.com>; Zeng, Star <star.z...@intel.com> Subject: RE: [edk2] [PATCH v2 1/1] SecurityPkg: fix ZeroMem HashInterfaceHob Yes, since the V1 has been pushed. Just adding one line like below based on V1 should be ok. HashInterfaceHob->SupportedHashMask = 0; Thanks, Star -----Original Message----- From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Marc-André Lureau Sent: Wednesday, March 7, 2018 10:56 PM To: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> Cc: Laszlo Ersek <ler...@redhat.com<mailto:ler...@redhat.com>>; Yao, Jiewen <jiewen....@intel.com<mailto:jiewen....@intel.com>>; Zhang, Chao B <chao.b.zh...@intel.com<mailto:chao.b.zh...@intel.com>>; Zeng, Star <star.z...@intel.com<mailto:star.z...@intel.com>> Subject: Re: [edk2] [PATCH v2 1/1] SecurityPkg: fix ZeroMem HashInterfaceHob Hi On Wed, Mar 7, 2018 at 12:24 PM, <marcandre.lur...@redhat.com<mailto:marcandre.lur...@redhat.com>> wrote: > From: Marc-André Lureau > <marcandre.lur...@redhat.com<mailto:marcandre.lur...@redhat.com>> > > The ZeroMem() call goes beyond the HashInterfaceHob structure, causing > HOB list corruption. The intent was to clear all but the Identifier, > that is starting from HashInterfaceCount. > I see v1 is already applied. I'll send another patch to clear the mask in the Ovmf/TPM series. Thanks > Quoting Laszlo Ersek: > Therefore I think the *first* approach to actually fixing this bug would > be to simply add the "Count" suffix: > > > diff --git > a/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.c > b/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.c > > index dbee0f2531bc..a869fffae3fe 100644 > > --- > a/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.c > > +++ > b/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.c > > @@ -424,7 +424,10 @@ HashLibBaseCryptoRouterPeiConstructor ( > > // This is the second execution of this module, clear the hash > interface > > // information registered at its first execution. > > // > > - ZeroMem (&HashInterfaceHob->HashInterface, sizeof > (*HashInterfaceHob) - sizeof (EFI_GUID)); > > + ZeroMem ( > > + &HashInterfaceHob->HashInterfaceCount, > > + sizeof (*HashInterfaceHob) - sizeof (EFI_GUID) > > + ); > > } > > > > // > > On the other hand, I don't think such a fix would be great. First of > all, the HASH_INTERFACE_HOB struct definition is not wrapped in > > #pragma pack (1) > ... > #pragma pack () > > in other words, HASH_INTERFACE_HOB is not packed, hence there could be > an unspecified amount of padding between "Identifier" and > "HashInterfaceCount". That would lead to the same kind of buffer > overflow, because "Length" includes the padding, but the "Buffer" > argument doesn't. > > Second, even if there were no padding (and thus the byte count would be > a perfect match for the full HOB structure), in the "Buffer" argument > we take the address of the "HashInterfaceCount" field, and with the > "Length" field, we still overflow that *individual* field. Although we > wouldn't overflow the containing HOB structure, this is still (arguably) > undefined behavior -- I seem to remember that some compilers actually > blow up on this when you use the standard ISO C memcpy() or memset() > functions in such contexts. > > This patch takes Laszlo suggestion to fix the issue. > > Cc: Jiewen Yao <jiewen....@intel.com<mailto:jiewen....@intel.com>> > Cc: Chao Zhang <chao.b.zh...@intel.com<mailto:chao.b.zh...@intel.com>> > Cc: Star Zeng <star.z...@intel.com<mailto:star.z...@intel.com>> > Cc: Laszlo Ersek <ler...@redhat.com<mailto:ler...@redhat.com>> > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Marc-André Lureau > <marcandre.lur...@redhat.com<mailto:marcandre.lur...@redhat.com>> > --- > .../HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git > a/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterP > ei.c > b/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterP > ei.c index dbee0f2531bc..84c1aaae70eb 100644 > --- > a/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterP > ei.c > +++ b/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRou > +++ terPei.c > @@ -397,6 +397,7 @@ HashLibBaseCryptoRouterPeiConstructor ( { > EFI_STATUS Status; > HASH_INTERFACE_HOB *HashInterfaceHob; > + UINTN ClearStartOffset; > > HashInterfaceHob = InternalGetHashInterfaceHob (&gZeroGuid); > if (HashInterfaceHob == NULL) { > @@ -424,7 +425,11 @@ HashLibBaseCryptoRouterPeiConstructor ( > // This is the second execution of this module, clear the hash interface > // information registered at its first execution. > // > - ZeroMem (&HashInterfaceHob->HashInterface, sizeof (*HashInterfaceHob) - > sizeof (EFI_GUID)); > + ClearStartOffset = OFFSET_OF (HASH_INTERFACE_HOB, HashInterfaceCount); > + ZeroMem ( > + (UINT8 *)HashInterfaceHob + ClearStartOffset, > + sizeof (*HashInterfaceHob) - ClearStartOffset > + ); > } > > // > -- > 2.16.2.346.g9779355e34 > > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> > https://lists.01.org/mailman/listinfo/edk2-devel -- Marc-André Lureau _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
