Re: [PATCH v2 1/1] SecurityPkg: fix ZeroMem HashInterfaceHob

["Zhang, Chao B" <chao.b.zhang@intel.com>]

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.lureau@gmail.com>; edk2-devel@lists.01.org
Cc: Laszlo Ersek <lersek@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com>; Zhang, Chao B <chao.b.zhang@intel.com>; Zeng, Star <star.zeng@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-bounces@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 <lersek@redhat.com<mailto:lersek@redhat.com>>; Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Zhang, Chao B <chao.b.zhang@intel.com<mailto:chao.b.zhang@intel.com>>; Zeng, Star <star.zeng@intel.com<mailto:star.zeng@intel.com>>
Subject: Re: [edk2] [PATCH v2 1/1] SecurityPkg: fix ZeroMem HashInterfaceHob

Hi

On Wed, Mar 7, 2018 at 12:24 PM,  <marcandre.lureau@redhat.com<mailto:marcandre.lureau@redhat.com>> wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com<mailto:marcandre.lureau@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.yao@intel.com<mailto:jiewen.yao@intel.com>>
> Cc: Chao Zhang <chao.b.zhang@intel.com<mailto:chao.b.zhang@intel.com>>
> Cc: Star Zeng <star.zeng@intel.com<mailto:star.zeng@intel.com>>
> Cc: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com<mailto:marcandre.lureau@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