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

public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed

* [edk2-devel] [PATCH] FmpDevicePkg/FmpDxe: Fix uninitialized pointer dereference
@ 2020-03-18  6:12 Xu, Wei6
  2020-03-18 15:15 ` Michael D Kinney
  0 siblings, 1 reply; 8+ messages in thread
From: Xu, Wei6 @ 2020-03-18  6:12 UTC (permalink / raw)
  To: devel; +Cc: Kun Qin, Michael D Kinney, Liming Gao

From: Kun Qin <kuqin@microsoft.com>

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2602

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


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [edk2-devel] [PATCH] FmpDevicePkg/FmpDxe: Fix uninitialized pointer dereference
  2020-03-18  6:12 [edk2-devel] [PATCH] FmpDevicePkg/FmpDxe: Fix uninitialized pointer dereference Xu, Wei6
@ 2020-03-18 15:15 ` Michael D Kinney
  2020-03-23  7:21   ` Guomin Jiang
  0 siblings, 1 reply; 8+ messages in thread
From: Michael D Kinney @ 2020-03-18 15:15 UTC (permalink / raw)
  To: Xu, Wei6, devel@edk2.groups.io, Kinney, Michael D; +Cc: Kun Qin, Gao, Liming

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://bugzilla.tianocore.org/show_bug.cgi?id=2602
> 
> 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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [edk2-devel] [PATCH] FmpDevicePkg/FmpDxe: Fix uninitialized pointer dereference
  2020-03-18 15:15 ` Michael D Kinney
@ 2020-03-23  7:21   ` Guomin Jiang
  2020-03-23  7:40     ` Kun Qin
  2020-03-23 16:05     ` Michael D Kinney
  0 siblings, 2 replies; 8+ messages in thread
From: Guomin Jiang @ 2020-03-23  7:21 UTC (permalink / raw)
  To: devel@edk2.groups.io, Kinney, Michael D, Xu, Wei6; +Cc: Kun Qin, Gao, Liming

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://bugzilla.tianocore.org/show_bug.cgi?id=2602
> >
> > 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
> 
> 
> 


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [edk2-devel] [PATCH] FmpDevicePkg/FmpDxe: Fix uninitialized pointer dereference
  2020-03-23  7:21   ` Guomin Jiang
@ 2020-03-23  7:40     ` Kun Qin
  2020-03-23  8:12       ` Guomin Jiang
       [not found]       ` <15FEE0ADB157EE06.9780@groups.io>
  2020-03-23 16:05     ` Michael D Kinney
  1 sibling, 2 replies; 8+ messages in thread
From: Kun Qin @ 2020-03-23  7:40 UTC (permalink / raw)
  To: Jiang, Guomin, devel@edk2.groups.io, Kinney, Michael D, Xu, Wei6
  Cc: Gao, Liming

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

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] 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
>
>
> 


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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [edk2-devel] [PATCH] FmpDevicePkg/FmpDxe: Fix uninitialized pointer dereference
  2020-03-23  7:40     ` Kun Qin
@ 2020-03-23  8:12       ` Guomin Jiang
       [not found]       ` <15FEE0ADB157EE06.9780@groups.io>
  1 sibling, 0 replies; 8+ messages in thread
From: Guomin Jiang @ 2020-03-23  8:12 UTC (permalink / raw)
  To: Kun Qin, devel@edk2.groups.io, Kinney, Michael D, Xu, Wei6; +Cc: Gao, Liming

[-- 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 --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

[parent not found: <15FEE0ADB157EE06.9780@groups.io>]

* Re: [edk2-devel] [PATCH] FmpDevicePkg/FmpDxe: Fix uninitialized pointer dereference
       [not found]       ` <15FEE0ADB157EE06.9780@groups.io>
@ 2020-05-06  1:35         ` Guomin Jiang
  2020-05-06  3:17           ` Xu, Wei6
  0 siblings, 1 reply; 8+ messages in thread
From: Guomin Jiang @ 2020-05-06  1:35 UTC (permalink / raw)
  To: devel@edk2.groups.io, Jiang, Guomin, Kun Qin, Kinney, Michael D,
	Xu, Wei6
  Cc: Gao, Liming

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

I can't search the patch in master, anyone can pull the patch if it haven't been pull.

Best Regards
guomin
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Guomin Jiang
Sent: Monday, March 23, 2020 4:13 PM
To: Kun Qin <Kun.Qin@microsoft.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 Kun,

It is clear and i have no confusion.

Reviewed-by: Guomin Jiang <guomin.jiang@intel.com<mailto: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<mailto:guomin.jiang@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>>; Xu, Wei6 <wei6.xu@intel.com<mailto:wei6.xu@intel.com>>
Cc: Gao, Liming <liming.gao@intel.com<mailto: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: 12241 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [edk2-devel] [PATCH] FmpDevicePkg/FmpDxe: Fix uninitialized pointer dereference
  2020-05-06  1:35         ` Guomin Jiang
@ 2020-05-06  3:17           ` Xu, Wei6
  0 siblings, 0 replies; 8+ messages in thread
From: Xu, Wei6 @ 2020-05-06  3:17 UTC (permalink / raw)
  To: Jiang, Guomin, devel@edk2.groups.io, Kun Qin, Kinney, Michael D
  Cc: Gao, Liming

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

Hi Guomin,

The patch was just pushed. Thanks a lot.

BR,
Wei
From: Jiang, Guomin <guomin.jiang@intel.com>
Sent: Wednesday, May 6, 2020 9:36 AM
To: devel@edk2.groups.io; Jiang, Guomin <guomin.jiang@intel.com>; Kun Qin <Kun.Qin@microsoft.com>; 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

I can't search the patch in master, anyone can pull the patch if it haven't been pull.

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

Hi Kun,

It is clear and i have no confusion.

Reviewed-by: Guomin Jiang <guomin.jiang@intel.com<mailto: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<mailto:guomin.jiang@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>>; Xu, Wei6 <wei6.xu@intel.com<mailto:wei6.xu@intel.com>>
Cc: Gao, Liming <liming.gao@intel.com<mailto: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: 14824 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [edk2-devel] [PATCH] FmpDevicePkg/FmpDxe: Fix uninitialized pointer dereference
  2020-03-23  7:21   ` Guomin Jiang
  2020-03-23  7:40     ` Kun Qin
@ 2020-03-23 16:05     ` Michael D Kinney
  1 sibling, 0 replies; 8+ messages in thread
From: Michael D Kinney @ 2020-03-23 16:05 UTC (permalink / raw)
  To: Jiang, Guomin, devel@edk2.groups.io, Xu, Wei6, Sean Brogan,
	Kinney, Michael D
  Cc: Kun Qin, Gao, Liming

The BZ report indicates that an invalid point access
was observed.  Perhaps Sean can add the details in the BZ
for which line of code generated in invalid access.

https://bugzilla.tianocore.org/show_bug.cgi?id=2602

Mike

> -----Original Message-----
> From: Jiang, Guomin <guomin.jiang@intel.com>
> Sent: Monday, March 23, 2020 12:21 AM
> To: devel@edk2.groups.io; Kinney, Michael D
> <michael.d.kinney@intel.com>; Xu, Wei6
> <wei6.xu@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
> 
> 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://bugzilla.tianocore.org/show_bug.cgi?id=2602
> > >
> > > 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
> >
> >
> > 


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2020-05-06  3:17 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-03-18  6:12 [edk2-devel] [PATCH] FmpDevicePkg/FmpDxe: Fix uninitialized pointer dereference Xu, Wei6
2020-03-18 15:15 ` Michael D Kinney
2020-03-23  7:21   ` Guomin Jiang
2020-03-23  7:40     ` Kun Qin
2020-03-23  8:12       ` Guomin Jiang
     [not found]       ` <15FEE0ADB157EE06.9780@groups.io>
2020-05-06  1:35         ` Guomin Jiang
2020-05-06  3:17           ` Xu, Wei6
2020-03-23 16:05     ` Michael D Kinney

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox