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

[Laszlo Ersek <lersek@redhat.com>]

On 03/07/18 16:35, Zhang, Chao B wrote:
> 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.

There seems to be no functional requirement for clearing the
SupportedHashMask field, except the original (buggy) ZeroMem() call
looked like it *intended* to clear the field. So now that we have fixed
the buffer overflow, we should decide whether we want to stick with the
original intent (that would mean continuing to clear the field, one way
or another), or to depart from the original intent -- but that would
merit a comment in the code (or, it would have deserved a comment in the
commit message).

In short, it's not great to do two independent things in the same patch,
namely (a) fix the buffer overflow, (b) silently diverge from the
original intent, even if that's functionally justified.

Such changes should be split to two patches, or else explained in detail.

Thanks
Laszlo