[PATCH 35/35] UefiPayloadPkg/BlSupportDxe: fix ReserveResourceInGcd() calls


Laszlo Ersek
 

The last parameter of ReserveResourceInGcd() is "ImageHandle", forwarded
in turn to gDS->AllocateMemorySpace() or gDS->AllocateIoSpace() as "owner=
"
image handle.

But BlDxeEntryPoint() passes "SystemTable" as "ImageHandle".

Compilers have not flagged it because EFI_HANDLE (the type of
"ImageHandle") is unfortunately specified as (VOID*), and
(EFI_SYSTEM_TABLE*) converts to (VOID*) silently.

Hand the entry point function's "ImageHandle" parameter to
ReserveResourceInGcd(). This fixes an actual bug.

Cc: Benjamin You <benjamin.you@intel.com>
Cc: Guo Dong <guo.dong@intel.com>
Cc: Maurice Ma <maurice.ma@intel.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---

Notes:
build-tested only

UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c b/UefiPayloadPkg/=
BlSupportDxe/BlSupportDxe.c
index bcee4cd9bc41..28dfc8fc5545 100644
--- a/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c
+++ b/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c
@@ -106,10 +106,10 @@ BlDxeEntryPoint (
//
// Report MMIO/IO Resources
//
- Status =3D ReserveResourceInGcd (TRUE, EfiGcdMemoryTypeMemoryMappedIo,=
0xFEC00000, SIZE_4KB, 0, SystemTable); // IOAPIC
+ Status =3D ReserveResourceInGcd (TRUE, EfiGcdMemoryTypeMemoryMappedIo,=
0xFEC00000, SIZE_4KB, 0, ImageHandle); // IOAPIC
ASSERT_EFI_ERROR (Status);
=20
- Status =3D ReserveResourceInGcd (TRUE, EfiGcdMemoryTypeMemoryMappedIo,=
0xFED00000, SIZE_1KB, 0, SystemTable); // HPET
+ Status =3D ReserveResourceInGcd (TRUE, EfiGcdMemoryTypeMemoryMappedIo,=
0xFED00000, SIZE_1KB, 0, ImageHandle); // HPET
ASSERT_EFI_ERROR (Status);
=20
//
--=20
2.19.1.3.g30247aa5d201


Guo Dong
 

Thanks for the fix.

Reviewed-by: Guo Dong <guo.dong@intel.com>

-----Original Message-----
From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
Laszlo Ersek
Sent: Tuesday, September 17, 2019 12:50 PM
To: edk2-devel-groups-io <devel@edk2.groups.io>
Cc: You, Benjamin <benjamin.you@intel.com>; Dong, Guo
<guo.dong@intel.com>; Ma, Maurice <maurice.ma@intel.com>
Subject: [edk2-devel] [PATCH 35/35] UefiPayloadPkg/BlSupportDxe: fix
ReserveResourceInGcd() calls

The last parameter of ReserveResourceInGcd() is "ImageHandle", forwarded
in turn to gDS->AllocateMemorySpace() or gDS->AllocateIoSpace() as
"owner"
image handle.

But BlDxeEntryPoint() passes "SystemTable" as "ImageHandle".

Compilers have not flagged it because EFI_HANDLE (the type of
"ImageHandle") is unfortunately specified as (VOID*), and
(EFI_SYSTEM_TABLE*) converts to (VOID*) silently.

Hand the entry point function's "ImageHandle" parameter to
ReserveResourceInGcd(). This fixes an actual bug.

Cc: Benjamin You <benjamin.you@intel.com>
Cc: Guo Dong <guo.dong@intel.com>
Cc: Maurice Ma <maurice.ma@intel.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---

Notes:
build-tested only

UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c
b/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c
index bcee4cd9bc41..28dfc8fc5545 100644
--- a/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c
+++ b/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c
@@ -106,10 +106,10 @@ BlDxeEntryPoint (
//
// Report MMIO/IO Resources
//
- Status = ReserveResourceInGcd (TRUE,
EfiGcdMemoryTypeMemoryMappedIo, 0xFEC00000, SIZE_4KB, 0,
SystemTable); // IOAPIC
+ Status = ReserveResourceInGcd (TRUE,
EfiGcdMemoryTypeMemoryMappedIo,
+ 0xFEC00000, SIZE_4KB, 0, ImageHandle); // IOAPIC
ASSERT_EFI_ERROR (Status);

- Status = ReserveResourceInGcd (TRUE,
EfiGcdMemoryTypeMemoryMappedIo, 0xFED00000, SIZE_1KB, 0,
SystemTable); // HPET
+ Status = ReserveResourceInGcd (TRUE,
EfiGcdMemoryTypeMemoryMappedIo,
+ 0xFED00000, SIZE_1KB, 0, ImageHandle); // HPET
ASSERT_EFI_ERROR (Status);

//
--
2.19.1.3.g30247aa5d201


-=-=-=-=-=-=
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#47422): https://edk2.groups.io/g/devel/message/47422
Mute This Topic: https://groups.io/mt/34180240/1781375
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [guo.dong@intel.com]
-=-=-=-=-=-=


Philippe Mathieu-Daudé
 

On 9/17/19 9:49 PM, Laszlo Ersek wrote:
The last parameter of ReserveResourceInGcd() is "ImageHandle", forwarded
in turn to gDS->AllocateMemorySpace() or gDS->AllocateIoSpace() as "owner"
image handle.

But BlDxeEntryPoint() passes "SystemTable" as "ImageHandle".

Compilers have not flagged it because EFI_HANDLE (the type of
"ImageHandle") is unfortunately specified as (VOID*), and
(EFI_SYSTEM_TABLE*) converts to (VOID*) silently.

Hand the entry point function's "ImageHandle" parameter to
ReserveResourceInGcd(). This fixes an actual bug.
Wow very buggy, so I assume this is mostly dead code, right?

Cc: Benjamin You <benjamin.you@intel.com>
Cc: Guo Dong <guo.dong@intel.com>
Cc: Maurice Ma <maurice.ma@intel.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---

Notes:
build-tested only

UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c b/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c
index bcee4cd9bc41..28dfc8fc5545 100644
--- a/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c
+++ b/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c
@@ -106,10 +106,10 @@ BlDxeEntryPoint (
//
// Report MMIO/IO Resources
//
- Status = ReserveResourceInGcd (TRUE, EfiGcdMemoryTypeMemoryMappedIo, 0xFEC00000, SIZE_4KB, 0, SystemTable); // IOAPIC
+ Status = ReserveResourceInGcd (TRUE, EfiGcdMemoryTypeMemoryMappedIo, 0xFEC00000, SIZE_4KB, 0, ImageHandle); // IOAPIC
ASSERT_EFI_ERROR (Status);

- Status = ReserveResourceInGcd (TRUE, EfiGcdMemoryTypeMemoryMappedIo, 0xFED00000, SIZE_1KB, 0, SystemTable); // HPET
+ Status = ReserveResourceInGcd (TRUE, EfiGcdMemoryTypeMemoryMappedIo, 0xFED00000, SIZE_1KB, 0, ImageHandle); // HPET
ASSERT_EFI_ERROR (Status);

//
Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>


Guo Dong
 

This is not dead code.
This actual bug didn't cause issues since BlSupportDxe just allocate resources reported from bootloaders.
Anyway, this is a great enhancement from spec to capture such bugs.

Thanks,
Guo

-----Original Message-----
From: Philippe Mathieu-Daudé [mailto:philmd@redhat.com]
Sent: Monday, September 23, 2019 8:08 AM
To: devel@edk2.groups.io; lersek@redhat.com
Cc: You, Benjamin <benjamin.you@intel.com>; Dong, Guo
<guo.dong@intel.com>; Ma, Maurice <maurice.ma@intel.com>
Subject: Re: [edk2-devel] [PATCH 35/35] UefiPayloadPkg/BlSupportDxe: fix
ReserveResourceInGcd() calls

On 9/17/19 9:49 PM, Laszlo Ersek wrote:
The last parameter of ReserveResourceInGcd() is "ImageHandle",
forwarded in turn to gDS->AllocateMemorySpace() or gDS-
AllocateIoSpace() as "owner"
image handle.

But BlDxeEntryPoint() passes "SystemTable" as "ImageHandle".

Compilers have not flagged it because EFI_HANDLE (the type of
"ImageHandle") is unfortunately specified as (VOID*), and
(EFI_SYSTEM_TABLE*) converts to (VOID*) silently.

Hand the entry point function's "ImageHandle" parameter to
ReserveResourceInGcd(). This fixes an actual bug.
Wow very buggy, so I assume this is mostly dead code, right?

Cc: Benjamin You <benjamin.you@intel.com>
Cc: Guo Dong <guo.dong@intel.com>
Cc: Maurice Ma <maurice.ma@intel.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---

Notes:
build-tested only

UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c
b/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c
index bcee4cd9bc41..28dfc8fc5545 100644
--- a/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c
+++ b/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c
@@ -106,10 +106,10 @@ BlDxeEntryPoint (
//
// Report MMIO/IO Resources
//
- Status = ReserveResourceInGcd (TRUE,
EfiGcdMemoryTypeMemoryMappedIo, 0xFEC00000, SIZE_4KB, 0,
SystemTable);
// IOAPIC
+ Status = ReserveResourceInGcd (TRUE,
+ EfiGcdMemoryTypeMemoryMappedIo, 0xFEC00000, SIZE_4KB, 0,
+ ImageHandle); // IOAPIC
ASSERT_EFI_ERROR (Status);

- Status = ReserveResourceInGcd (TRUE,
EfiGcdMemoryTypeMemoryMappedIo, 0xFED00000, SIZE_1KB, 0,
SystemTable);
// HPET
+ Status = ReserveResourceInGcd (TRUE,
+ EfiGcdMemoryTypeMemoryMappedIo, 0xFED00000, SIZE_1KB, 0,
+ ImageHandle); // HPET
ASSERT_EFI_ERROR (Status);

//
Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>


Philippe Mathieu-Daudé
 

On 9/23/19 6:02 PM, Dong, Guo wrote:

This is not dead code.
This actual bug didn't cause issues since BlSupportDxe just allocate resources reported from bootloaders.
Ah OK, thanks Guo.

Anyway, this is a great enhancement from spec to capture such bugs.

Thanks,
Guo

-----Original Message-----
From: Philippe Mathieu-Daudé [mailto:philmd@redhat.com]
Sent: Monday, September 23, 2019 8:08 AM
To: devel@edk2.groups.io; lersek@redhat.com
Cc: You, Benjamin <benjamin.you@intel.com>; Dong, Guo
<guo.dong@intel.com>; Ma, Maurice <maurice.ma@intel.com>
Subject: Re: [edk2-devel] [PATCH 35/35] UefiPayloadPkg/BlSupportDxe: fix
ReserveResourceInGcd() calls

On 9/17/19 9:49 PM, Laszlo Ersek wrote:
The last parameter of ReserveResourceInGcd() is "ImageHandle",
forwarded in turn to gDS->AllocateMemorySpace() or gDS-
AllocateIoSpace() as "owner"
image handle.

But BlDxeEntryPoint() passes "SystemTable" as "ImageHandle".

Compilers have not flagged it because EFI_HANDLE (the type of
"ImageHandle") is unfortunately specified as (VOID*), and
(EFI_SYSTEM_TABLE*) converts to (VOID*) silently.

Hand the entry point function's "ImageHandle" parameter to
ReserveResourceInGcd(). This fixes an actual bug.
Wow very buggy, so I assume this is mostly dead code, right?

Cc: Benjamin You <benjamin.you@intel.com>
Cc: Guo Dong <guo.dong@intel.com>
Cc: Maurice Ma <maurice.ma@intel.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---

Notes:
build-tested only

UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c
b/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c
index bcee4cd9bc41..28dfc8fc5545 100644
--- a/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c
+++ b/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c
@@ -106,10 +106,10 @@ BlDxeEntryPoint (
//
// Report MMIO/IO Resources
//
- Status = ReserveResourceInGcd (TRUE,
EfiGcdMemoryTypeMemoryMappedIo, 0xFEC00000, SIZE_4KB, 0,
SystemTable);
// IOAPIC
+ Status = ReserveResourceInGcd (TRUE,
+ EfiGcdMemoryTypeMemoryMappedIo, 0xFEC00000, SIZE_4KB, 0,
+ ImageHandle); // IOAPIC
ASSERT_EFI_ERROR (Status);

- Status = ReserveResourceInGcd (TRUE,
EfiGcdMemoryTypeMemoryMappedIo, 0xFED00000, SIZE_1KB, 0,
SystemTable);
// HPET
+ Status = ReserveResourceInGcd (TRUE,
+ EfiGcdMemoryTypeMemoryMappedIo, 0xFED00000, SIZE_1KB, 0,
+ ImageHandle); // HPET
ASSERT_EFI_ERROR (Status);

//
Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>


Laszlo Ersek
 

On 09/23/19 17:08, Philippe Mathieu-Daudé wrote:
On 9/17/19 9:49 PM, Laszlo Ersek wrote:
The last parameter of ReserveResourceInGcd() is "ImageHandle", forwarded
in turn to gDS->AllocateMemorySpace() or gDS->AllocateIoSpace() as "owner"
image handle.

But BlDxeEntryPoint() passes "SystemTable" as "ImageHandle".

Compilers have not flagged it because EFI_HANDLE (the type of
"ImageHandle") is unfortunately specified as (VOID*), and
(EFI_SYSTEM_TABLE*) converts to (VOID*) silently.

Hand the entry point function's "ImageHandle" parameter to
ReserveResourceInGcd(). This fixes an actual bug.
Wow very buggy, so I assume this is mostly dead code, right?
Not necessarily; as long as noone tries to use the "owner" image handle
in practice, the issue may remain dormant.

Thanks
Laszlo

Cc: Benjamin You <benjamin.you@intel.com>
Cc: Guo Dong <guo.dong@intel.com>
Cc: Maurice Ma <maurice.ma@intel.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---

Notes:
build-tested only

UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c b/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c
index bcee4cd9bc41..28dfc8fc5545 100644
--- a/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c
+++ b/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c
@@ -106,10 +106,10 @@ BlDxeEntryPoint (
//
// Report MMIO/IO Resources
//
- Status = ReserveResourceInGcd (TRUE, EfiGcdMemoryTypeMemoryMappedIo, 0xFEC00000, SIZE_4KB, 0, SystemTable); // IOAPIC
+ Status = ReserveResourceInGcd (TRUE, EfiGcdMemoryTypeMemoryMappedIo, 0xFEC00000, SIZE_4KB, 0, ImageHandle); // IOAPIC
ASSERT_EFI_ERROR (Status);

- Status = ReserveResourceInGcd (TRUE, EfiGcdMemoryTypeMemoryMappedIo, 0xFED00000, SIZE_1KB, 0, SystemTable); // HPET
+ Status = ReserveResourceInGcd (TRUE, EfiGcdMemoryTypeMemoryMappedIo, 0xFED00000, SIZE_1KB, 0, ImageHandle); // HPET
ASSERT_EFI_ERROR (Status);

//
Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>