Re: [edk2-devel] [PATCH] FmpDevicePkg/FmpDxe: Fix uninitialized pointer dereference

["Guomin Jiang" <guomin.jiang@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 5164 bytes --]

Hi Kun,

It is clear and i have no confusion.

Reviewed-by: Guomin Jiang <guomin.jiang@intel.com>

Thanks
guomin
From: Kun Qin [mailto:Kun.Qin@microsoft.com]
Sent: Monday, March 23, 2020 3:40 PM
To: Jiang, Guomin <guomin.jiang@intel.com>; devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>; Xu, Wei6 <wei6.xu@intel.com>
Cc: Gao, Liming <liming.gao@intel.com>
Subject: RE: [edk2-devel] [PATCH] FmpDevicePkg/FmpDxe: Fix uninitialized pointer dereference

Hi Guomin,

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

If Line 582<https://github.com/tianocore/edk2/blob/4c0f6e349d32cf27a7104ddd3e729d6ebc88ea70/FmpDevicePkg/FmpDxe/Dependency.c#L582> 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<https://github.com/tianocore/edk2/blob/4c0f6e349d32cf27a7104ddd3e729d6ebc88ea70/FmpDevicePkg/FmpDxe/Dependency.c#L632>, 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<mailto:guomin.jiang@intel.com>
Sent: Monday, March 23, 2020 12:21 AM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Kinney, Michael D<mailto:michael.d.kinney@intel.com>; Xu, Wei6<mailto:wei6.xu@intel.com>
Cc: Kun Qin<mailto:Kun.Qin@microsoft.com>; Gao, Liming<mailto:liming.gao@intel.com>
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> [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<mailto:wei6.xu@intel.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Kinney, Michael
> D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>
> Cc: Kun Qin <kuqin@microsoft.com<mailto:kuqin@microsoft.com>>; Gao, Liming <liming.gao@intel.com<mailto: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<mailto:michael.d.kinney@intel.com>>
>
> > -----Original Message-----
> > From: Xu, Wei6 <wei6.xu@intel.com<mailto:wei6.xu@intel.com>>
> > Sent: Tuesday, March 17, 2020 11:12 PM
> > To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>
> > Cc: Kun Qin <kuqin@microsoft.com<mailto:kuqin@microsoft.com>>; Kinney, Michael D
> > <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; Gao, Liming <liming.gao@intel.com<mailto:liming.gao@intel.com>>
> > Subject: [edk2-devel] [PATCH] FmpDevicePkg/FmpDxe: Fix uninitialized
> > pointer dereference
> >
> > From: Kun Qin <kuqin@microsoft.com<mailto: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<mailto:michael.d.kinney@intel.com>>
> > Cc: Liming Gao <liming.gao@intel.com<mailto:liming.gao@intel.com>>
> > Signed-off-by: Wei6 Xu <wei6.xu@intel.com<mailto: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
>
>
> 


[-- Attachment #2: Type: text/html, Size: 10283 bytes --]