From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by mx.groups.io with SMTP id smtpd.web12.48091.1584951155033466182 for ; Mon, 23 Mar 2020 01:12:35 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 192.55.52.115, mailfrom: guomin.jiang@intel.com) IronPort-SDR: 8rxMhFVON6bUrHPLljJj1UvovoAP4LpSw47tYvbF5vAniaiBNXQPV2mvs+wCiibsE0QxzYspsW zXXTLhKhyOXg== X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga007.jf.intel.com ([10.7.209.58]) by fmsmga103.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 23 Mar 2020 01:12:34 -0700 IronPort-SDR: 9qCYFQhBVVZ7PQyFxwx5BxMEapIWskFOBLEWA5z5CJHT5ZDcywcLEjWrOZScSvQg9PlGJvSUlF yQgRHWgZ0x2g== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.72,295,1580803200"; d="scan'208,217";a="235149085" Received: from fmsmsx106.amr.corp.intel.com ([10.18.124.204]) by orsmga007.jf.intel.com with ESMTP; 23 Mar 2020 01:12:33 -0700 Received: from fmsmsx152.amr.corp.intel.com (10.18.125.5) by FMSMSX106.amr.corp.intel.com (10.18.124.204) with Microsoft SMTP Server (TLS) id 14.3.439.0; Mon, 23 Mar 2020 01:12:33 -0700 Received: from shsmsx107.ccr.corp.intel.com (10.239.4.96) by FMSMSX152.amr.corp.intel.com (10.18.125.5) with Microsoft SMTP Server (TLS) id 14.3.439.0; Mon, 23 Mar 2020 01:12:32 -0700 Received: from shsmsx101.ccr.corp.intel.com ([169.254.1.43]) by SHSMSX107.ccr.corp.intel.com ([169.254.9.232]) with mapi id 14.03.0439.000; Mon, 23 Mar 2020 16:12:30 +0800 From: "Guomin Jiang" To: Kun Qin , "devel@edk2.groups.io" , "Kinney, Michael D" , "Xu, Wei6" CC: "Gao, Liming" Subject: Re: [edk2-devel] [PATCH] FmpDevicePkg/FmpDxe: Fix uninitialized pointer dereference Thread-Topic: [edk2-devel] [PATCH] FmpDevicePkg/FmpDxe: Fix uninitialized pointer dereference Thread-Index: AQHV/Ow90WEWUrmAW06w7cZabl8GS6hN8BUAgAfYilD//4PyAIAAjjLw Date: Mon, 23 Mar 2020 08:12:30 +0000 Message-ID: References: <20200318061227.12480-1-wei6.xu@intel.com> , In-Reply-To: Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Return-Path: guomin.jiang@intel.com Content-Language: en-US Content-Type: multipart/alternative; boundary="_000_B1F5B0856690F44595CF70FED755C93954953FSHSMSX101ccrcorpi_" --_000_B1F5B0856690F44595CF70FED755C93954953FSHSMSX101ccrcorpi_ Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Hi Kun, It is clear and i have no confusion. Reviewed-by: Guomin Jiang Thanks guomin From: Kun Qin [mailto:Kun.Qin@microsoft.com] Sent: Monday, March 23, 2020 3:40 PM To: Jiang, Guomin ; devel@edk2.groups.io; Kinney, = Michael D ; Xu, Wei6 Cc: Gao, Liming Subject: RE: [edk2-devel] [PATCH] FmpDevicePkg/FmpDxe: Fix uninitialized p= ointer dereference 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] w= ill remain to be uninitialized value (0xFAFAFAFAFAF in my case). Later on w= hen it comes to line 632, it wi= ll 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 unin= itialized 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] =3D AllocateZeroPool (ImageInfoSize); If the second GetImageInfo() is runned, I think it will always have correc= t mfmpImageInfoBuf[] address. Of course, it is ok to use AllocateZeroPool to ensure zero buffer is alloc= ated. Thanks > -----Original Message----- > From: devel@edk2.groups.io [mailto:devel@ed= k2.groups.io] On Behalf Of > Michael D Kinney > Sent: Wednesday, March 18, 2020 11:15 PM > To: Xu, Wei6 >; devel@edk2.g= roups.io; Kinney, Michael > D > > Cc: Kun Qin >; Gao, Limi= ng > > Subject: Re: [edk2-devel] [PATCH] FmpDevicePkg/FmpDxe: Fix uninitialized > pointer dereference > > Reviewed-by: Michael D Kinney > > > > -----Original Message----- > > From: Xu, Wei6 > > > Sent: Tuesday, March 17, 2020 11:12 PM > > To: devel@edk2.groups.io > > Cc: Kun Qin >; Kinney,= Michael D > > >; Gao, = Liming > > > Subject: [edk2-devel] [PATCH] FmpDevicePkg/FmpDxe: Fix uninitialized > > pointer dereference > > > > From: Kun Qin > > > > > REF: > > https://nam06.safelinks.protection.outlook.com/?url=3Dhttps%3A%2F%2Fbu= gzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D2602&data=3D02%7C01%7CKun.Qi= n%40microsoft.com%7C3c1042cd095b42a51b9d08d7cefad022%7C72f988bf86f141af91ab= 2d7cd011db47%7C1%7C0%7C637205448946602054&sdata=3D95z6fDC0uceCCs2MuoeCR= 4MXgRhAI3dVssWeddsWT5s%3D&reserved=3D0 > > > > 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 > > > Cc: Liming Gao > > > Signed-off-by: Wei6 Xu > > > --- > > 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 =3D AllocatePool > > (sizeof(EFI_FIRMWARE_IMAGE_DESCRIPTOR *) * > mNumberOfFmpInstance); > > + mFmpImageInfoBuf =3D AllocateZeroPool > > (sizeof(EFI_FIRMWARE_IMAGE_DESCRIPTOR *) * > mNumberOfFmpInstance); > > if (mFmpImageInfoBuf =3D=3D NULL) { > > return EFI_OUT_OF_RESOURCES; > > } > > > > for (Index =3D 0; Index < mNumberOfFmpInstance; Index > > ++) { > > -- > > 2.16.2.windows.1 > > >=20 --_000_B1F5B0856690F44595CF70FED755C93954953FSHSMSX101ccrcorpi_ Content-Type: text/html; charset="us-ascii" Content-Transfer-Encoding: quoted-printable

Hi Kun,

 

It is clear and i hav= e no confusion.

 

Reviewed-by: Guomin J= iang <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 <wei= 6.xu@intel.com>
Cc: Gao, Liming <liming.gao@intel.com>
Subject: RE: [edk2-devel] [PATCH] FmpDevicePkg/FmpDxe: Fix uninitia= lized pointer dereference

 

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 (0xFA= FAFAFAFAF in my case). Later on when it comes to line 632, it will pass the null pointer check and try to dereference i= t, which leads to GP fault. Please let me know if you need further clarific= ation.

 

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: F= ix uninitialized pointer dereference

 

Hi Xuwei, QinKun,
Have you indeed encounter this issue or just think it is potential issue.<= br>
I think  below code will always initialize the mFmpImageInfoBuf[] and= make sure it is valid.
Line 585 - mFmpImageInfoBuf[Index] =3D AllocateZeroPool (ImageInfoSize);
If the second GetImageInfo() is runned, I think it will always have correc= t mfmpImageInfoBuf[] address.

Of course, it is ok to use AllocateZeroPool to ensure zero buffer is alloc= ated.

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.c= om>; devel@edk2.groups.io; Kinney, Michael
> D <michael.d.kinney@= intel.com>
> Cc: Kun Qin <kuqin@microsof= t.com>; Gao, Liming <limi= ng.gao@intel.com>
> Subject: Re: [edk2-devel] [PATCH] FmpDevicePkg/FmpDxe: Fix uninitiali= zed
> 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@mic= rosoft.com>; Kinney, Michael D
> > <michael.d.kinn= ey@intel.com>; Gao, Liming <liming.gao@intel.com>
> > Subject: [edk2-devel] [PATCH] FmpDevicePkg/FmpDxe: Fix uninitial= ized
> > pointer dereference
> >
> > From: Kun Qin <kuqin@m= icrosoft.com>
> >
> > REF:
> > https://nam06.safelinks.protection.outlook.com/?url=3Dhttps%3A%2F%2Fbugzil= la.tianocore.org%2Fshow_bug.cgi%3Fid%3D2602&amp;data=3D02%7C01%7CKun.Qi= n%40microsoft.com%7C3c1042cd095b42a51b9d08d7cefad022%7C72f988bf86f141af91ab= 2d7cd011db47%7C1%7C0%7C637205448946602054&amp;sdata=3D95z6fDC0uceCCs2Mu= oeCR4MXgRhAI3dVssWeddsWT5s%3D&amp;reserved=3D0
> >
> > Zero the allocated buffer in case GetImageInfo `continue` in the=
> > middle of a loop. This will cause unexpected GetImageInfo failur= e 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 <limin= g.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 =3D AllocatePool
> > (sizeof(EFI_FIRMWARE_IMAGE_DESCRIPTOR *) *
> mNumberOfFmpInstance);
> > +  mFmpImageInfoBuf =3D AllocateZeroPool
> > (sizeof(EFI_FIRMWARE_IMAGE_DESCRIPTOR *) *
> mNumberOfFmpInstance);
> >    if (mFmpImageInfoBuf =3D=3D NULL) {
> >      return EFI_OUT_OF_RESOURCES;
> >    }
> >
> >    for (Index =3D 0; Index < mNumberOfFmpInsta= nce; Index
> > ++) {
> > --
> > 2.16.2.windows.1
>
>
>

 

--_000_B1F5B0856690F44595CF70FED755C93954953FSHSMSX101ccrcorpi_--