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

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

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