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

["Marc-André Lureau" <marcandre.lureau@gmail.com>]

Hi

On Wed, Mar 7, 2018 at 12:24 PM,  <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. 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>
> 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   | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> 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
> +      );
>    }
>
>    //
> --
> 2.16.2.346.g9779355e34
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel



-- 
Marc-André Lureau