Hi Guomin,

Thanks for reaching out. I did encounter a GP fault because of this issue:

If Line 582 is triggered when the first Fmp->GetImageInfo failed, this specific mFmpImageInfoBuf[Index] will remain to be uninitialized value (0xFAFAFAFAFAF in my case). Later on when it comes to line 632, it will pass the null pointer check and try to dereference it, which leads to GP fault. Please let me know if you need further clarification.

Thanks,

Kun

From: Jiang, Guomin
Sent: Monday, March 23, 2020 12:21 AM
To: devel@edk2.groups.io; Kinney, Michael D; Xu, Wei6
Cc: Kun Qin; Gao, Liming
Subject: [EXTERNAL] RE: [edk2-devel] [PATCH] FmpDevicePkg/FmpDxe: Fix uninitialized pointer dereference

Hi Xuwei, QinKun,

Have you indeed encounter this issue or just think it is potential issue.

I think  below code will always initialize the mFmpImageInfoBuf[] and make sure it is valid.
Line 585 - mFmpImageInfoBuf[Index] = AllocateZeroPool (ImageInfoSize);

If the second GetImageInfo() is runned, I think it will always have correct mfmpImageInfoBuf[] address.

Of course, it is ok to use AllocateZeroPool to ensure zero buffer is allocated.

Thanks

> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Michael D Kinney
> Sent: Wednesday, March 18, 2020 11:15 PM
> To: Xu, Wei6 <wei6.xu@intel.com>; devel@edk2.groups.io; Kinney, Michael
> D <michael.d.kinney@intel.com>
> Cc: Kun Qin <kuqin@microsoft.com>; Gao, Liming <liming.gao@intel.com>
> Subject: Re: [edk2-devel] [PATCH] FmpDevicePkg/FmpDxe: Fix uninitialized
> pointer dereference
>
> Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>
>
> > -----Original Message-----
> > From: Xu, Wei6 <wei6.xu@intel.com>
> > Sent: Tuesday, March 17, 2020 11:12 PM
> > To: devel@edk2.groups.io
> > Cc: Kun Qin <kuqin@microsoft.com>; Kinney, Michael D
> > <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>
> > Subject: [edk2-devel] [PATCH] FmpDevicePkg/FmpDxe: Fix uninitialized
> > pointer dereference
> >
> > From: Kun Qin <kuqin@microsoft.com>
> >
> > REF:
> > https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D2602&amp;data=02%7C01%7CKun.Qin%40microsoft.com%7C3c1042cd095b42a51b9d08d7cefad022%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637205448946602054&amp;sdata=95z6fDC0uceCCs2MuoeCR4MXgRhAI3dVssWeddsWT5s%3D&amp;reserved=0
> >
> > Zero the allocated buffer in case GetImageInfo `continue` in the
> > middle of a loop. This will cause unexpected GetImageInfo failure not
> > clearing the corresponding entry and lead to GP faults when
> > dereferencing this entry.
> >
> > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > Cc: Liming Gao <liming.gao@intel.com>
> > Signed-off-by: Wei6 Xu <wei6.xu@intel.com>
> > ---
> >  FmpDevicePkg/FmpDxe/Dependency.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/FmpDevicePkg/FmpDxe/Dependency.c
> > b/FmpDevicePkg/FmpDxe/Dependency.c
> > index 8f97c42916..65c23989c6 100644
> > --- a/FmpDevicePkg/FmpDxe/Dependency.c
> > +++ b/FmpDevicePkg/FmpDxe/Dependency.c
> > @@ -550,11 +550,11 @@ EvaluateImageDependencies (
> >                  );
> >    if (EFI_ERROR (Status)) {
> >      return EFI_ABORTED;
> >    }
> >
> > -  mFmpImageInfoBuf = AllocatePool
> > (sizeof(EFI_FIRMWARE_IMAGE_DESCRIPTOR *) *
> mNumberOfFmpInstance);
> > +  mFmpImageInfoBuf = AllocateZeroPool
> > (sizeof(EFI_FIRMWARE_IMAGE_DESCRIPTOR *) *
> mNumberOfFmpInstance);
> >    if (mFmpImageInfoBuf == NULL) {
> >      return EFI_OUT_OF_RESOURCES;
> >    }
> >
> >    for (Index = 0; Index < mNumberOfFmpInstance; Index
> > ++) {
> > --
> > 2.16.2.windows.1
>
>
>