[PATCH 1/1] RFC: SecurityPkg: only clear HashInterface informations

[PATCH 1/1] RFC: SecurityPkg: only clear HashInterface informations

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

The ZeroMem() call goes beyond the HashInterfaceHob structure, causing
HOB list corruption. Instead, just clear the HashInterface fields, as
I suppose was originally intended.

Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Chao Zhang <chao.b.zhang@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 .../HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.c       | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.c b/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.c
index dbee0f2531bc..361a4f6508a0 100644
--- a/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.c
+++ b/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.c
@@ -424,7 +424,8 @@ 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->HashInterface, sizeof (HashInterfaceHob->HashInterface));
+    HashInterfaceHob->HashInterfaceCount = 0;
   }
 
   //
-- 
2.16.2.346.g9779355e34

Re: [PATCH 1/1] RFC: SecurityPkg: only clear HashInterface informations

[Zeng, Star]

Reviewed-by: Star Zeng <star.zeng@intel.com>

Thanks,
Star
-----Original Message-----
From: marcandre.lureau@redhat.com [mailto:marcandre.lureau@redhat.com] 
Sent: Wednesday, March 7, 2018 4:27 AM
To: edk2-devel@lists.01.org
Cc: Marc-André Lureau <marcandre.lureau@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com>; Zhang, Chao B <chao.b.zhang@intel.com>; Zeng, Star <star.zeng@intel.com>; Laszlo Ersek <lersek@redhat.com>
Subject: [PATCH 1/1] RFC: SecurityPkg: only clear HashInterface informations

From: Marc-André Lureau <marcandre.lureau@redhat.com>

The ZeroMem() call goes beyond the HashInterfaceHob structure, causing HOB list corruption. Instead, just clear the HashInterface fields, as I suppose was originally intended.

Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Chao Zhang <chao.b.zhang@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 .../HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.c       | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.c b/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.c
index dbee0f2531bc..361a4f6508a0 100644
--- a/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.c
+++ b/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRoute
+++ rPei.c
@@ -424,7 +424,8 @@ 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->HashInterface, sizeof (HashInterfaceHob->HashInterface));
+    HashInterfaceHob->HashInterfaceCount = 0;
   }
 
   //
--
2.16.2.346.g9779355e34

Re: [PATCH 1/1] RFC: SecurityPkg: only clear HashInterface informations

[Yao, Jiewen]

Excellent. Thanks to catch that.

Reviewed-by: Jiewen.yao@intel.com



> -----Original Message-----
> From: marcandre.lureau@redhat.com [mailto:marcandre.lureau@redhat.com]
> Sent: Wednesday, March 7, 2018 4:27 AM
> To: edk2-devel@lists.01.org
> Cc: Marc-André Lureau <marcandre.lureau@redhat.com>; Yao, Jiewen
> <jiewen.yao@intel.com>; Zhang, Chao B <chao.b.zhang@intel.com>; Zeng, Star
> <star.zeng@intel.com>; Laszlo Ersek <lersek@redhat.com>
> Subject: [PATCH 1/1] RFC: SecurityPkg: only clear HashInterface informations
> 
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> The ZeroMem() call goes beyond the HashInterfaceHob structure, causing
> HOB list corruption. Instead, just clear the HashInterface fields, as
> I suppose was originally intended.
> 
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Chao Zhang <chao.b.zhang@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  .../HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.c       | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git
> a/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.c
> b/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.c
> index dbee0f2531bc..361a4f6508a0 100644
> ---
> a/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.c
> +++
> b/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.c
> @@ -424,7 +424,8 @@ 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->HashInterface, sizeof
> (HashInterfaceHob->HashInterface));
> +    HashInterfaceHob->HashInterfaceCount = 0;
>    }
> 
>    //
> --
> 2.16.2.346.g9779355e34

Re: [PATCH 1/1] RFC: SecurityPkg: only clear HashInterface informations

[Zhang, Chao B]

Reviewed-by: Chao Zhang <chao.b.zhang@intel.com>

-----Original Message-----
From: marcandre.lureau@redhat.com [mailto:marcandre.lureau@redhat.com] 
Sent: Wednesday, March 7, 2018 4:27 AM
To: edk2-devel@lists.01.org
Cc: Marc-André Lureau <marcandre.lureau@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com>; Zhang, Chao B <chao.b.zhang@intel.com>; Zeng, Star <star.zeng@intel.com>; Laszlo Ersek <lersek@redhat.com>
Subject: [PATCH 1/1] RFC: SecurityPkg: only clear HashInterface informations

From: Marc-André Lureau <marcandre.lureau@redhat.com>

The ZeroMem() call goes beyond the HashInterfaceHob structure, causing HOB list corruption. Instead, just clear the HashInterface fields, as I suppose was originally intended.

Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Chao Zhang <chao.b.zhang@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 .../HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.c       | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.c b/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.c
index dbee0f2531bc..361a4f6508a0 100644
--- a/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.c
+++ b/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRoute
+++ rPei.c
@@ -424,7 +424,8 @@ 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->HashInterface, sizeof (HashInterfaceHob->HashInterface));
+    HashInterfaceHob->HashInterfaceCount = 0;
   }
 
   //
--
2.16.2.346.g9779355e34

Re: [PATCH 1/1] RFC: SecurityPkg: only clear HashInterface informations

[Laszlo Ersek]

Hi Marc-André,

On 03/06/18 21:27, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> The ZeroMem() call goes beyond the HashInterfaceHob structure, causing
> HOB list corruption. Instead, just clear the HashInterface fields, as
> I suppose was originally intended.
>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Chao Zhang <chao.b.zhang@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  .../HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.c       | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.c b/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.c
> index dbee0f2531bc..361a4f6508a0 100644
> --- a/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.c
> +++ b/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.c
> @@ -424,7 +424,8 @@ 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->HashInterface, sizeof (HashInterfaceHob->HashInterface));
> +    HashInterfaceHob->HashInterfaceCount = 0;
>    }
>
>    //
>

Excellent catch! :)

I do have a question however, for the other reviewers on this patch, in
line with your commit message saying, "clear the HashInterface fields,
as *I suppose* was originally intended".

I don't think the proposed update matches the original intent 100%.
Namely, this is the definition of the HASH_INTERFACE_HOB type (from the
same source file):

> typedef struct {
>   //
>   // If gZeroGuid, SupportedHashMask is 0 for FIRST module which consumes HashLib
>   //   or the hash algorithm bitmap of LAST module which consumes HashLib.
>   //   HashInterfaceCount and HashInterface are all 0.
>   // If gEfiCallerIdGuid, HashInterfaceCount, HashInterface and SupportedHashMask
>   //   are the hash interface information of CURRENT module which consumes HashLib.
>   //
>   EFI_GUID         Identifier;
>   UINTN            HashInterfaceCount;
>   HASH_INTERFACE   HashInterface[HASH_COUNT];
>   UINT32           SupportedHashMask;
> } HASH_INTERFACE_HOB;

Note that it says, if Identifier equals "gEfiCallerIdGuid", then *all
three* subsequent fields carry information of the current module that
consumes HashLib. Those three fields include "SupportedHashMask".

Furthermore, the comment just preceding the bug / ZeroMem() call says,

>     //
>     // In PEI phase, some modules may call RegisterForShadow and will be
>     // shadowed and executed again after memory is discovered.
>     // This is the second execution of this module, clear the hash interface
>     // information registered at its first execution.
>     //

And note that here we are just past the check

>   HashInterfaceHob = InternalGetHashInterfaceHob (&gEfiCallerIdGuid);
>   if (HashInterfaceHob != NULL) {

in other words, the "Identifier equals gEfiCallerIdGuid" branch from the
type definition applies; meaning that "SupportedHashMask" is defined
too.

Thus, I'd like the other reviewers to confirm whether we should clear
"SupportedHashMask".

>From the original (buggy) code, I think we *should* clear
SupportedHashMask as well. The "Length" argument of the original
ZeroMem() call implies precisely that:

  sizeof (*HashInterfaceHob) - sizeof (EFI_GUID)

This stands for "all fields in *HashInterfaceHob except the leading
EFI_GUID Identifier field".

So, the actual bug is in the "Buffer" argument of the ZeroMem() call: it
is a typo. Certainly the intent must have been "start clearing from the
first field right after the EFI_GUID Identifier field", namely
"HashInterfaceCount":

  &HashInterfaceHob->HashInterfaceCount
                                  ^^^^^

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.

So, long story short, I suggest the following fix instead:

> diff --git a/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.c b/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.c
> index dbee0f2531bc..84c1aaae70eb 100644
> --- a/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.c
> +++ b/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.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
> +      );
>    }
>
>    //

Thoughts?

Thanks!
Laszlo

Re: [PATCH 1/1] RFC: SecurityPkg: only clear HashInterface informations

[Zeng, Star]

Hi Laszlo,

Good analysis.

Yes, the SupportedHashMask field in HashInterfaceHob will have stale value, but that does not impact the functionality since the code have.
So the patch could fix the problem.

HashLibBaseCryptoRouterPeiConstructor():
  Status = PcdSet32S (PcdTcg2HashAlgorithmBitmap, 0);

RegisterHashInterfaceLib():
  HashInterfaceHob->SupportedHashMask = PcdGet32 (PcdTcg2HashAlgorithmBitmap) | HashMask;

HashStart()/HashUpdate()/HashCompleteAndExtend()/HashAndExtend():
  if (HashInterfaceHob->HashInterfaceCount == 0) {
    return EFI_UNSUPPORTED;
  }

RegisterHashInterfaceLib():
  if (HashInterfaceHob->HashInterfaceCount >= HASH_COUNT) {
    return EFI_OUT_OF_RESOURCES;
  }

As I know, Chao has helped push the patch.
But I am fine with keeping this patch or continue refining the code. :)

Is it working with " ZeroMem (HashInterfaceHob, sizeof (*HashInterfaceHob) - sizeof (EFI_GUID)) " if to refine the code?


Thanks,
Star
-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
Sent: Wednesday, March 7, 2018 5:06 PM
To: marcandre.lureau@redhat.com
Cc: edk2-devel@lists.01.org; Yao, Jiewen <jiewen.yao@intel.com>; Zeng, Star <star.zeng@intel.com>; Zhang, Chao B <chao.b.zhang@intel.com>
Subject: Re: [edk2] [PATCH 1/1] RFC: SecurityPkg: only clear HashInterface informations

Hi Marc-André,

On 03/06/18 21:27, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> The ZeroMem() call goes beyond the HashInterfaceHob structure, causing 
> HOB list corruption. Instead, just clear the HashInterface fields, as 
> I suppose was originally intended.
>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Chao Zhang <chao.b.zhang@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  .../HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.c       | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git 
> a/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterP
> ei.c 
> b/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterP
> ei.c index dbee0f2531bc..361a4f6508a0 100644
> --- 
> a/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterP
> ei.c
> +++ b/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRou
> +++ terPei.c
> @@ -424,7 +424,8 @@ 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->HashInterface, sizeof (HashInterfaceHob->HashInterface));
> +    HashInterfaceHob->HashInterfaceCount = 0;
>    }
>
>    //
>

Excellent catch! :)

I do have a question however, for the other reviewers on this patch, in line with your commit message saying, "clear the HashInterface fields, as *I suppose* was originally intended".

I don't think the proposed update matches the original intent 100%.
Namely, this is the definition of the HASH_INTERFACE_HOB type (from the same source file):

> typedef struct {
>   //
>   // If gZeroGuid, SupportedHashMask is 0 for FIRST module which consumes HashLib
>   //   or the hash algorithm bitmap of LAST module which consumes HashLib.
>   //   HashInterfaceCount and HashInterface are all 0.
>   // If gEfiCallerIdGuid, HashInterfaceCount, HashInterface and SupportedHashMask
>   //   are the hash interface information of CURRENT module which consumes HashLib.
>   //
>   EFI_GUID         Identifier;
>   UINTN            HashInterfaceCount;
>   HASH_INTERFACE   HashInterface[HASH_COUNT];
>   UINT32           SupportedHashMask;
> } HASH_INTERFACE_HOB;

Note that it says, if Identifier equals "gEfiCallerIdGuid", then *all
three* subsequent fields carry information of the current module that consumes HashLib. Those three fields include "SupportedHashMask".

Furthermore, the comment just preceding the bug / ZeroMem() call says,

>     //
>     // In PEI phase, some modules may call RegisterForShadow and will be
>     // shadowed and executed again after memory is discovered.
>     // This is the second execution of this module, clear the hash interface
>     // information registered at its first execution.
>     //

And note that here we are just past the check

>   HashInterfaceHob = InternalGetHashInterfaceHob (&gEfiCallerIdGuid);
>   if (HashInterfaceHob != NULL) {

in other words, the "Identifier equals gEfiCallerIdGuid" branch from the type definition applies; meaning that "SupportedHashMask" is defined too.

Thus, I'd like the other reviewers to confirm whether we should clear "SupportedHashMask".

From the original (buggy) code, I think we *should* clear SupportedHashMask as well. The "Length" argument of the original
ZeroMem() call implies precisely that:

  sizeof (*HashInterfaceHob) - sizeof (EFI_GUID)

This stands for "all fields in *HashInterfaceHob except the leading EFI_GUID Identifier field".

So, the actual bug is in the "Buffer" argument of the ZeroMem() call: it is a typo. Certainly the intent must have been "start clearing from the first field right after the EFI_GUID Identifier field", namely
"HashInterfaceCount":

  &HashInterfaceHob->HashInterfaceCount
                                  ^^^^^

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/HashLibBaseCryptoRouterP
> ei.c 
> b/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterP
> ei.c index dbee0f2531bc..a869fffae3fe 100644
> --- 
> a/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterP
> ei.c
> +++ b/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRou
> +++ terPei.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.

So, long story short, I suggest the following fix instead:

> 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
> +      );
>    }
>
>    //

Thoughts?

Thanks!
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH 1/1] RFC: SecurityPkg: only clear HashInterface informations

[Zeng, Star]

My head was stuck to say using " ZeroMem (HashInterfaceHob, sizeof (*HashInterfaceHob) - sizeof (EFI_GUID)) ", that is wrong. :(

Thanks,
Star
-----Original Message-----
From: Zeng, Star 
Sent: Wednesday, March 7, 2018 5:42 PM
To: Laszlo Ersek <lersek@redhat.com>; marcandre.lureau@redhat.com
Cc: edk2-devel@lists.01.org; Yao, Jiewen <jiewen.yao@intel.com>; Zhang, Chao B <chao.b.zhang@intel.com>; Zeng, Star <star.zeng@intel.com>
Subject: RE: [edk2] [PATCH 1/1] RFC: SecurityPkg: only clear HashInterface informations

Hi Laszlo,

Good analysis.

Yes, the SupportedHashMask field in HashInterfaceHob will have stale value, but that does not impact the functionality since the code have.
So the patch could fix the problem.

HashLibBaseCryptoRouterPeiConstructor():
  Status = PcdSet32S (PcdTcg2HashAlgorithmBitmap, 0);

RegisterHashInterfaceLib():
  HashInterfaceHob->SupportedHashMask = PcdGet32 (PcdTcg2HashAlgorithmBitmap) | HashMask;

HashStart()/HashUpdate()/HashCompleteAndExtend()/HashAndExtend():
  if (HashInterfaceHob->HashInterfaceCount == 0) {
    return EFI_UNSUPPORTED;
  }

RegisterHashInterfaceLib():
  if (HashInterfaceHob->HashInterfaceCount >= HASH_COUNT) {
    return EFI_OUT_OF_RESOURCES;
  }

As I know, Chao has helped push the patch.
But I am fine with keeping this patch or continue refining the code. :)

Is it working with " ZeroMem (HashInterfaceHob, sizeof (*HashInterfaceHob) - sizeof (EFI_GUID)) " if to refine the code?


Thanks,
Star
-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
Sent: Wednesday, March 7, 2018 5:06 PM
To: marcandre.lureau@redhat.com
Cc: edk2-devel@lists.01.org; Yao, Jiewen <jiewen.yao@intel.com>; Zeng, Star <star.zeng@intel.com>; Zhang, Chao B <chao.b.zhang@intel.com>
Subject: Re: [edk2] [PATCH 1/1] RFC: SecurityPkg: only clear HashInterface informations

Hi Marc-André,

On 03/06/18 21:27, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> The ZeroMem() call goes beyond the HashInterfaceHob structure, causing 
> HOB list corruption. Instead, just clear the HashInterface fields, as 
> I suppose was originally intended.
>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Chao Zhang <chao.b.zhang@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  .../HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.c       | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git
> a/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterP
> ei.c
> b/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterP
> ei.c index dbee0f2531bc..361a4f6508a0 100644
> ---
> a/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterP
> ei.c
> +++ b/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRou
> +++ terPei.c
> @@ -424,7 +424,8 @@ 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->HashInterface, sizeof (HashInterfaceHob->HashInterface));
> +    HashInterfaceHob->HashInterfaceCount = 0;
>    }
>
>    //
>

Excellent catch! :)

I do have a question however, for the other reviewers on this patch, in line with your commit message saying, "clear the HashInterface fields, as *I suppose* was originally intended".

I don't think the proposed update matches the original intent 100%.
Namely, this is the definition of the HASH_INTERFACE_HOB type (from the same source file):

> typedef struct {
>   //
>   // If gZeroGuid, SupportedHashMask is 0 for FIRST module which consumes HashLib
>   //   or the hash algorithm bitmap of LAST module which consumes HashLib.
>   //   HashInterfaceCount and HashInterface are all 0.
>   // If gEfiCallerIdGuid, HashInterfaceCount, HashInterface and SupportedHashMask
>   //   are the hash interface information of CURRENT module which consumes HashLib.
>   //
>   EFI_GUID         Identifier;
>   UINTN            HashInterfaceCount;
>   HASH_INTERFACE   HashInterface[HASH_COUNT];
>   UINT32           SupportedHashMask;
> } HASH_INTERFACE_HOB;

Note that it says, if Identifier equals "gEfiCallerIdGuid", then *all
three* subsequent fields carry information of the current module that consumes HashLib. Those three fields include "SupportedHashMask".

Furthermore, the comment just preceding the bug / ZeroMem() call says,

>     //
>     // In PEI phase, some modules may call RegisterForShadow and will be
>     // shadowed and executed again after memory is discovered.
>     // This is the second execution of this module, clear the hash interface
>     // information registered at its first execution.
>     //

And note that here we are just past the check

>   HashInterfaceHob = InternalGetHashInterfaceHob (&gEfiCallerIdGuid);
>   if (HashInterfaceHob != NULL) {

in other words, the "Identifier equals gEfiCallerIdGuid" branch from the type definition applies; meaning that "SupportedHashMask" is defined too.

Thus, I'd like the other reviewers to confirm whether we should clear "SupportedHashMask".

From the original (buggy) code, I think we *should* clear SupportedHashMask as well. The "Length" argument of the original
ZeroMem() call implies precisely that:

  sizeof (*HashInterfaceHob) - sizeof (EFI_GUID)

This stands for "all fields in *HashInterfaceHob except the leading EFI_GUID Identifier field".

So, the actual bug is in the "Buffer" argument of the ZeroMem() call: it is a typo. Certainly the intent must have been "start clearing from the first field right after the EFI_GUID Identifier field", namely
"HashInterfaceCount":

  &HashInterfaceHob->HashInterfaceCount
                                  ^^^^^

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/HashLibBaseCryptoRouterP
> ei.c
> b/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterP
> ei.c index dbee0f2531bc..a869fffae3fe 100644
> ---
> a/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterP
> ei.c
> +++ b/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRou
> +++ terPei.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.

So, long story short, I suggest the following fix instead:

> 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
> +      );
>    }
>
>    //

Thoughts?

Thanks!
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH 1/1] RFC: SecurityPkg: only clear HashInterface informations

[Laszlo Ersek]

On 03/07/18 10:42, Zeng, Star wrote:
> Hi Laszlo,
> 
> Good analysis.
> 
> Yes, the SupportedHashMask field in HashInterfaceHob will have stale value, but that does not impact the functionality since the code have.
> So the patch could fix the problem.
> 
> HashLibBaseCryptoRouterPeiConstructor():
>   Status = PcdSet32S (PcdTcg2HashAlgorithmBitmap, 0);
> 
> RegisterHashInterfaceLib():
>   HashInterfaceHob->SupportedHashMask = PcdGet32 (PcdTcg2HashAlgorithmBitmap) | HashMask;
> 
> HashStart()/HashUpdate()/HashCompleteAndExtend()/HashAndExtend():
>   if (HashInterfaceHob->HashInterfaceCount == 0) {
>     return EFI_UNSUPPORTED;
>   }
> 
> RegisterHashInterfaceLib():
>   if (HashInterfaceHob->HashInterfaceCount >= HASH_COUNT) {
>     return EFI_OUT_OF_RESOURCES;
>   }

Ugh, this is a bit too complex for me to digest right now, but I'll
trust you if you say the value doesn't matter :)

> As I know, Chao has helped push the patch.

Yes, that's correct, it's commit 4cc2b63bd829 ("SecurityPkg: only clear
HashInterface information", 2018-03-07).

> But I am fine with keeping this patch or continue refining the code. :)

I would slightly prefer clearing the SupportedHashMask field as well (or
adding a comment explaining why it's not necessary), because then the
uninitiated reader, like me, wouldn't have to ask themselves why the
field isn't being cleared :)

But, given your response, I don't feel strongly about it any longer;
I'll leave it up to Marc-André.

(Also, if we have to choose between extending the ZeroMem() and writing
the comment, I think the former is easier :) I could write that too, but
I couldn't write the comment.)

> Is it working with " ZeroMem (HashInterfaceHob, sizeof (*HashInterfaceHob) - sizeof (EFI_GUID)) " if to refine the code?

I'll skip this question based on your later followup.

Thanks!
Laszlo

Re: [PATCH 1/1] RFC: SecurityPkg: only clear HashInterface informations

[Marc-André Lureau]

Hi

On Wed, Mar 7, 2018 at 10:06 AM, Laszlo Ersek <lersek@redhat.com> wrote:
> Hi Marc-André,
>
> On 03/06/18 21:27, marcandre.lureau@redhat.com wrote:
>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>
>> The ZeroMem() call goes beyond the HashInterfaceHob structure, causing
>> HOB list corruption. Instead, just clear the HashInterface fields, as
>> I suppose was originally intended.
>>
>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>> Cc: Chao Zhang <chao.b.zhang@intel.com>
>> Cc: Star Zeng <star.zeng@intel.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> ---
>>  .../HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.c       | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.c b/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.c
>> index dbee0f2531bc..361a4f6508a0 100644
>> --- a/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.c
>> +++ b/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.c
>> @@ -424,7 +424,8 @@ 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->HashInterface, sizeof (HashInterfaceHob->HashInterface));
>> +    HashInterfaceHob->HashInterfaceCount = 0;
>>    }
>>
>>    //
>>
>
> Excellent catch! :)
>
> I do have a question however, for the other reviewers on this patch, in
> line with your commit message saying, "clear the HashInterface fields,
> as *I suppose* was originally intended".
>
> I don't think the proposed update matches the original intent 100%.
> Namely, this is the definition of the HASH_INTERFACE_HOB type (from the
> same source file):
>
>> typedef struct {
>>   //
>>   // If gZeroGuid, SupportedHashMask is 0 for FIRST module which consumes HashLib
>>   //   or the hash algorithm bitmap of LAST module which consumes HashLib.
>>   //   HashInterfaceCount and HashInterface are all 0.
>>   // If gEfiCallerIdGuid, HashInterfaceCount, HashInterface and SupportedHashMask
>>   //   are the hash interface information of CURRENT module which consumes HashLib.
>>   //
>>   EFI_GUID         Identifier;
>>   UINTN            HashInterfaceCount;
>>   HASH_INTERFACE   HashInterface[HASH_COUNT];
>>   UINT32           SupportedHashMask;
>> } HASH_INTERFACE_HOB;
>
> Note that it says, if Identifier equals "gEfiCallerIdGuid", then *all
> three* subsequent fields carry information of the current module that
> consumes HashLib. Those three fields include "SupportedHashMask".
>
> Furthermore, the comment just preceding the bug / ZeroMem() call says,
>
>>     //
>>     // In PEI phase, some modules may call RegisterForShadow and will be
>>     // shadowed and executed again after memory is discovered.
>>     // This is the second execution of this module, clear the hash interface
>>     // information registered at its first execution.
>>     //
>
> And note that here we are just past the check
>
>>   HashInterfaceHob = InternalGetHashInterfaceHob (&gEfiCallerIdGuid);
>>   if (HashInterfaceHob != NULL) {
>
> in other words, the "Identifier equals gEfiCallerIdGuid" branch from the
> type definition applies; meaning that "SupportedHashMask" is defined
> too.
>
> Thus, I'd like the other reviewers to confirm whether we should clear
> "SupportedHashMask".
>
> From the original (buggy) code, I think we *should* clear
> SupportedHashMask as well. The "Length" argument of the original
> ZeroMem() call implies precisely that:
>
>   sizeof (*HashInterfaceHob) - sizeof (EFI_GUID)
>
> This stands for "all fields in *HashInterfaceHob except the leading
> EFI_GUID Identifier field".
>
> So, the actual bug is in the "Buffer" argument of the ZeroMem() call: it
> is a typo. Certainly the intent must have been "start clearing from the
> first field right after the EFI_GUID Identifier field", namely
> "HashInterfaceCount":
>
>   &HashInterfaceHob->HashInterfaceCount
>                                   ^^^^^
>
> 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.
>
> So, long story short, I suggest the following fix instead:
>
>> diff --git a/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.c b/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.c
>> index dbee0f2531bc..84c1aaae70eb 100644
>> --- a/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.c
>> +++ b/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.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
>> +      );
>>    }
>>
>>    //
>
> Thoughts?

Looking at RegisterHashInterfaceLib() code, I agree it makes sense to
clear both the array and the mask. Both seem to be closely related
during registration.

I agree with your argument as well about structure packing. I'll
resend the patch with your suggested changes.

thanks