HI Hao:

   I think the patch is to complete the security check both in info and code logic to ValidateCapsuleNameCapsuleIntegrity . 

It is OK to keep it in one patch.

From: Wu, Hao A
Sent: Friday, June 28, 2019 8:54 AM
To: Xu, Wei6 <wei6.xu@intel.com>; devel@edk2.groups.io
Cc: Wang, Jian J <jian.j.wang@intel.com>; Zhang, Chao B <chao.b.zhang@intel.com>
Subject: RE: [edk2-devel][Patch] MdeModulePkg/DxeCapsuleLibFmp: Add missing NULL pointer check.

> -----Original Message-----
> From: Xu, Wei6
> Sent: Friday, June 28, 2019 12:26 AM
> To: devel@edk2.groups.io
> Cc: Wang, Jian J; Wu, Hao A; Zhang, Chao B
> Subject: [edk2-devel][Patch] MdeModulePkg/DxeCapsuleLibFmp: Add
> missing NULL pointer check.
> 
> Add missing NULL pointer check for CapsuleNameBufStart.
> Also add comments to notice that capsule name integrity check assumes
> the capsule has been validated by IsValidCapsuleHeader().


The patch is doing 2 things. Please help to split it into 2 commits.

With this handled,
Reviewed-by: Hao A Wu <hao.a.wu@intel.com>

Best Regards,
Hao Wu


> 
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Chao B Zhang <chao.b.zhang@intel.com>
> Signed-off-by: Wei6 Xu <wei6.xu@intel.com>
> ---
>  MdeModulePkg/Library/DxeCapsuleLibFmp/CapsuleOnDisk.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/MdeModulePkg/Library/DxeCapsuleLibFmp/CapsuleOnDisk.c
> b/MdeModulePkg/Library/DxeCapsuleLibFmp/CapsuleOnDisk.c
> index 66c9be8e1f..3193ca8f4d 100644
> --- a/MdeModulePkg/Library/DxeCapsuleLibFmp/CapsuleOnDisk.c
> +++ b/MdeModulePkg/Library/DxeCapsuleLibFmp/CapsuleOnDisk.c
> @@ -23,10 +23,13 @@ IsCapsuleNameCapsule (
> 
>  /**
>    Check the integrity of the capsule name capsule.
>    If the capsule is vaild, return the physical address of each capsule name
> string.
> 
> +  This routine assumes the capsule has been validated by
> IsValidCapsuleHeader(), so
> +  capsule memory overflow is not going to happen in this routine.
> +
>    @param[in]  CapsuleHeader   Pointer to the capsule header of a capsule
> name capsule.
>    @param[out] CapsuleNameNum  Number of capsule name.
> 
>    @retval NULL                Capsule name capsule is not valid.
>    @retval CapsuleNameBuf      Array of capsule name physical address.
> @@ -63,10 +66,13 @@ ValidateCapsuleNameCapsuleIntegrity (
>    //
>    // If strings are not aligned on a 16-bit boundary, reallocate memory for it.
>    //
>    if (((UINTN) CapsuleNameBufStart & BIT0) != 0) {
>      CapsuleNameBufStart = AllocateCopyPool (CapsuleHeader-
> >CapsuleImageSize - CapsuleHeader->HeaderSize, CapsuleNameBufStart);
> +    if (CapsuleNameBufStart == NULL) {
> +      return NULL;
> +    }
>    }
> 
>    CapsuleNameBufEnd = CapsuleNameBufStart + CapsuleHeader-
> >CapsuleImageSize - CapsuleHeader->HeaderSize;
> 
>    CapsuleNamePtr = CapsuleNameBufStart;
> --
> 2.16.2.windows.1