* [PATCH 0/3] Fix a bug that prevents PMEM access
@ 2018-09-21 7:25 Ruiyu Ni
2018-09-21 7:25 ` [PATCH 1/3] MdeModulePkg/PciHostBridge: Enhance boundary check in Io/Mem.Read/Write Ruiyu Ni
` (2 more replies)
0 siblings, 3 replies; 20+ messages in thread
From: Ruiyu Ni @ 2018-09-21 7:25 UTC (permalink / raw)
To: edk2-devel
The patch sets fix a bug in PciHostBridge driver that prevents PMEM access
sometimes. Additionally, it enhances the boundary check and add a new macro
to simplify the code.
Ruiyu Ni (3):
MdeModulePkg/PciHostBridge: Enhance boundary check in
Io/Mem.Read/Write
MdeModulePkg/PciHostBridge: Fix a bug that prevents PMEM access
MdeModulePkg/PciHostBridge: Add RESOURCE_VALID() to simplify code
.../Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c | 64 ++++++++++++++--------
1 file changed, 42 insertions(+), 22 deletions(-)
--
2.16.1.windows.1
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 1/3] MdeModulePkg/PciHostBridge: Enhance boundary check in Io/Mem.Read/Write
2018-09-21 7:25 [PATCH 0/3] Fix a bug that prevents PMEM access Ruiyu Ni
@ 2018-09-21 7:25 ` Ruiyu Ni
2018-09-21 10:53 ` Laszlo Ersek
` (2 more replies)
2018-09-21 7:25 ` [PATCH 2/3] MdeModulePkg/PciHostBridge: Fix a bug that prevents PMEM access Ruiyu Ni
2018-09-21 7:25 ` [PATCH 3/3] MdeModulePkg/PciHostBridge: Add RESOURCE_VALID() to simplify code Ruiyu Ni
2 siblings, 3 replies; 20+ messages in thread
From: Ruiyu Ni @ 2018-09-21 7:25 UTC (permalink / raw)
To: edk2-devel; +Cc: Star Zeng
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
---
.../Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c | 26 +++++++++++++++++-----
1 file changed, 21 insertions(+), 5 deletions(-)
diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
index f8a1239ceb..0b6b56f846 100644
--- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
+++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
@@ -321,6 +321,7 @@ RootBridgeIoCheckParameter (
UINT64 Base;
UINT64 Limit;
UINT32 Size;
+ UINT64 Length;
//
// Check to see if Buffer is NULL
@@ -337,7 +338,7 @@ RootBridgeIoCheckParameter (
}
//
- // For FIFO type, the target address won't increase during the access,
+ // For FIFO type, the device address won't increase during the access,
// so treat Count as 1
//
if (Width >= EfiPciWidthFifoUint8 && Width <= EfiPciWidthFifoUint64) {
@@ -347,6 +348,13 @@ RootBridgeIoCheckParameter (
Width = (EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_WIDTH) (Width & 0x03);
Size = 1 << Width;
+ //
+ // Make sure (Count * Size) doesn't exceed MAX_UINT64
+ //
+ if (Count > DivU64x32 (MAX_UINT64, Size)) {
+ return EFI_INVALID_PARAMETER;
+ }
+
//
// Check to see if Address is aligned
//
@@ -354,6 +362,14 @@ RootBridgeIoCheckParameter (
return EFI_UNSUPPORTED;
}
+ //
+ // Make sure (Address + Count * Size) doesn't exceed MAX_UINT64
+ //
+ Length = MultU64x32 (Count, Size);
+ if (Address > MAX_UINT64 - Length) {
+ return EFI_INVALID_PARAMETER;
+ }
+
RootBridge = ROOT_BRIDGE_FROM_THIS (This);
//
@@ -372,7 +388,7 @@ RootBridgeIoCheckParameter (
//
// Allow Legacy IO access
//
- if (Address + MultU64x32 (Count, Size) <= 0x1000) {
+ if (Address + Length <= 0x1000) {
if ((RootBridge->Attributes & (
EFI_PCI_ATTRIBUTE_ISA_IO | EFI_PCI_ATTRIBUTE_VGA_PALETTE_IO | EFI_PCI_ATTRIBUTE_VGA_IO |
EFI_PCI_ATTRIBUTE_IDE_PRIMARY_IO | EFI_PCI_ATTRIBUTE_IDE_SECONDARY_IO |
@@ -386,7 +402,7 @@ RootBridgeIoCheckParameter (
//
// Allow Legacy MMIO access
//
- if ((Address >= 0xA0000) && (Address + MultU64x32 (Count, Size)) <= 0xC0000) {
+ if ((Address >= 0xA0000) && (Address + Length) <= 0xC0000) {
if ((RootBridge->Attributes & EFI_PCI_ATTRIBUTE_VGA_MEMORY) != 0) {
return EFI_SUCCESS;
}
@@ -395,7 +411,7 @@ RootBridgeIoCheckParameter (
// By comparing the Address against Limit we know which range to be used
// for checking
//
- if (Address + MultU64x32 (Count, Size) <= RootBridge->Mem.Limit + 1) {
+ if (Address + Length <= RootBridge->Mem.Limit + 1) {
Base = RootBridge->Mem.Base;
Limit = RootBridge->Mem.Limit;
} else {
@@ -427,7 +443,7 @@ RootBridgeIoCheckParameter (
return EFI_INVALID_PARAMETER;
}
- if (Address + MultU64x32 (Count, Size) > Limit + 1) {
+ if (Address + Length > Limit + 1) {
return EFI_INVALID_PARAMETER;
}
--
2.16.1.windows.1
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 2/3] MdeModulePkg/PciHostBridge: Fix a bug that prevents PMEM access
2018-09-21 7:25 [PATCH 0/3] Fix a bug that prevents PMEM access Ruiyu Ni
2018-09-21 7:25 ` [PATCH 1/3] MdeModulePkg/PciHostBridge: Enhance boundary check in Io/Mem.Read/Write Ruiyu Ni
@ 2018-09-21 7:25 ` Ruiyu Ni
2018-09-21 11:06 ` Laszlo Ersek
` (2 more replies)
2018-09-21 7:25 ` [PATCH 3/3] MdeModulePkg/PciHostBridge: Add RESOURCE_VALID() to simplify code Ruiyu Ni
2 siblings, 3 replies; 20+ messages in thread
From: Ruiyu Ni @ 2018-09-21 7:25 UTC (permalink / raw)
To: edk2-devel; +Cc: Star Zeng, Kirkendall, Garrett
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1196
RootBridgeIoCheckParameter() verifies that the requested MMIO access
can fit in any of the MEM/PMEM 32/64 ranges. But today's logic
somehow only checks the requested access against MEM 32/64 ranges.
It should also check the requested access against PMEM 32/64 ranges.
The patch fixes this issue.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Kirkendall, Garrett <Garrett.Kirkendall@amd.com>
---
MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
index 0b6b56f846..f6234b5d11 100644
--- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
+++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
@@ -411,12 +411,18 @@ RootBridgeIoCheckParameter (
// By comparing the Address against Limit we know which range to be used
// for checking
//
- if (Address + Length <= RootBridge->Mem.Limit + 1) {
- Base = RootBridge->Mem.Base;
+ if ((Address >= RootBridge->Mem.Base) && (Address + Length <= RootBridge->Mem.Limit + 1)) {
+ Base = RootBridge->Mem.Base;
Limit = RootBridge->Mem.Limit;
- } else {
- Base = RootBridge->MemAbove4G.Base;
+ } else if ((Address >= RootBridge->PMem.Base) && (Address + Length <= RootBridge->PMem.Limit + 1)) {
+ Base = RootBridge->PMem.Base;
+ Limit = RootBridge->PMem.Limit;
+ } else if ((Address >= RootBridge->MemAbove4G.Base) && (Address + Length <= RootBridge->MemAbove4G.Limit + 1)) {
+ Base = RootBridge->MemAbove4G.Base;
Limit = RootBridge->MemAbove4G.Limit;
+ } else {
+ Base = RootBridge->PMemAbove4G.Base;
+ Limit = RootBridge->PMemAbove4G.Limit;
}
} else {
PciRbAddr = (EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_PCI_ADDRESS*) &Address;
--
2.16.1.windows.1
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 3/3] MdeModulePkg/PciHostBridge: Add RESOURCE_VALID() to simplify code
2018-09-21 7:25 [PATCH 0/3] Fix a bug that prevents PMEM access Ruiyu Ni
2018-09-21 7:25 ` [PATCH 1/3] MdeModulePkg/PciHostBridge: Enhance boundary check in Io/Mem.Read/Write Ruiyu Ni
2018-09-21 7:25 ` [PATCH 2/3] MdeModulePkg/PciHostBridge: Fix a bug that prevents PMEM access Ruiyu Ni
@ 2018-09-21 7:25 ` Ruiyu Ni
2018-09-21 11:12 ` Laszlo Ersek
2018-09-24 13:20 ` Kirkendall, Garrett
2 siblings, 2 replies; 20+ messages in thread
From: Ruiyu Ni @ 2018-09-21 7:25 UTC (permalink / raw)
To: edk2-devel; +Cc: Star Zeng
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
---
.../Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c | 26 ++++++++++------------
1 file changed, 12 insertions(+), 14 deletions(-)
diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
index f6234b5d11..916709e276 100644
--- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
+++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
@@ -21,6 +21,8 @@ extern EDKII_IOMMU_PROTOCOL *mIoMmuProtocol;
#define NO_MAPPING (VOID *) (UINTN) -1
+#define RESOURCE_VALID(R) ((R).Base <= (R).Limit)
+
//
// Lookup table for increment values based on transfer widths
//
@@ -122,25 +124,25 @@ CreateRootBridge (
//
// Make sure Mem and MemAbove4G apertures are valid
//
- if (Bridge->Mem.Base <= Bridge->Mem.Limit) {
+ if (RESOURCE_VALID (Bridge->Mem)) {
ASSERT (Bridge->Mem.Limit < SIZE_4GB);
if (Bridge->Mem.Limit >= SIZE_4GB) {
return NULL;
}
}
- if (Bridge->MemAbove4G.Base <= Bridge->MemAbove4G.Limit) {
+ if (RESOURCE_VALID (Bridge->MemAbove4G)) {
ASSERT (Bridge->MemAbove4G.Base >= SIZE_4GB);
if (Bridge->MemAbove4G.Base < SIZE_4GB) {
return NULL;
}
}
- if (Bridge->PMem.Base <= Bridge->PMem.Limit) {
+ if (RESOURCE_VALID (Bridge->PMem)) {
ASSERT (Bridge->PMem.Limit < SIZE_4GB);
if (Bridge->PMem.Limit >= SIZE_4GB) {
return NULL;
}
}
- if (Bridge->PMemAbove4G.Base <= Bridge->PMemAbove4G.Limit) {
+ if (RESOURCE_VALID (Bridge->PMemAbove4G)) {
ASSERT (Bridge->PMemAbove4G.Base >= SIZE_4GB);
if (Bridge->PMemAbove4G.Base < SIZE_4GB) {
return NULL;
@@ -157,11 +159,9 @@ CreateRootBridge (
// support separate windows for Non-prefetchable and Prefetchable
// memory.
//
- ASSERT (Bridge->PMem.Base > Bridge->PMem.Limit);
- ASSERT (Bridge->PMemAbove4G.Base > Bridge->PMemAbove4G.Limit);
- if ((Bridge->PMem.Base <= Bridge->PMem.Limit) ||
- (Bridge->PMemAbove4G.Base <= Bridge->PMemAbove4G.Limit)
- ) {
+ ASSERT (!RESOURCE_VALID (Bridge->PMem));
+ ASSERT (!RESOURCE_VALID (Bridge->PMemAbove4G));
+ if (RESOURCE_VALID (Bridge->PMem) || RESOURCE_VALID (Bridge->PMemAbove4G)) {
return NULL;
}
}
@@ -171,11 +171,9 @@ CreateRootBridge (
// If this bit is not set, then the PCI Root Bridge does not support
// 64 bit memory windows.
//
- ASSERT (Bridge->MemAbove4G.Base > Bridge->MemAbove4G.Limit);
- ASSERT (Bridge->PMemAbove4G.Base > Bridge->PMemAbove4G.Limit);
- if ((Bridge->MemAbove4G.Base <= Bridge->MemAbove4G.Limit) ||
- (Bridge->PMemAbove4G.Base <= Bridge->PMemAbove4G.Limit)
- ) {
+ ASSERT (!RESOURCE_VALID (Bridge->MemAbove4G));
+ ASSERT (!RESOURCE_VALID (Bridge->PMemAbove4G));
+ if (RESOURCE_VALID (Bridge->MemAbove4G) || RESOURCE_VALID (Bridge->PMemAbove4G)) {
return NULL;
}
}
--
2.16.1.windows.1
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 1/3] MdeModulePkg/PciHostBridge: Enhance boundary check in Io/Mem.Read/Write
2018-09-21 7:25 ` [PATCH 1/3] MdeModulePkg/PciHostBridge: Enhance boundary check in Io/Mem.Read/Write Ruiyu Ni
@ 2018-09-21 10:53 ` Laszlo Ersek
2018-09-24 13:18 ` Kirkendall, Garrett
2018-09-25 2:14 ` Zeng, Star
2 siblings, 0 replies; 20+ messages in thread
From: Laszlo Ersek @ 2018-09-21 10:53 UTC (permalink / raw)
To: Ruiyu Ni, edk2-devel; +Cc: Star Zeng
On 09/21/18 09:25, Ruiyu Ni wrote:
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> ---
> .../Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c | 26 +++++++++++++++++-----
> 1 file changed, 21 insertions(+), 5 deletions(-)
>
> diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> index f8a1239ceb..0b6b56f846 100644
> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> @@ -321,6 +321,7 @@ RootBridgeIoCheckParameter (
> UINT64 Base;
> UINT64 Limit;
> UINT32 Size;
> + UINT64 Length;
>
> //
> // Check to see if Buffer is NULL
> @@ -337,7 +338,7 @@ RootBridgeIoCheckParameter (
> }
>
> //
> - // For FIFO type, the target address won't increase during the access,
> + // For FIFO type, the device address won't increase during the access,
> // so treat Count as 1
> //
> if (Width >= EfiPciWidthFifoUint8 && Width <= EfiPciWidthFifoUint64) {
> @@ -347,6 +348,13 @@ RootBridgeIoCheckParameter (
> Width = (EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_WIDTH) (Width & 0x03);
> Size = 1 << Width;
>
> + //
> + // Make sure (Count * Size) doesn't exceed MAX_UINT64
> + //
> + if (Count > DivU64x32 (MAX_UINT64, Size)) {
> + return EFI_INVALID_PARAMETER;
> + }
> +
> //
> // Check to see if Address is aligned
> //
> @@ -354,6 +362,14 @@ RootBridgeIoCheckParameter (
> return EFI_UNSUPPORTED;
> }
>
> + //
> + // Make sure (Address + Count * Size) doesn't exceed MAX_UINT64
> + //
> + Length = MultU64x32 (Count, Size);
> + if (Address > MAX_UINT64 - Length) {
> + return EFI_INVALID_PARAMETER;
> + }
> +
> RootBridge = ROOT_BRIDGE_FROM_THIS (This);
>
> //
> @@ -372,7 +388,7 @@ RootBridgeIoCheckParameter (
> //
> // Allow Legacy IO access
> //
> - if (Address + MultU64x32 (Count, Size) <= 0x1000) {
> + if (Address + Length <= 0x1000) {
> if ((RootBridge->Attributes & (
> EFI_PCI_ATTRIBUTE_ISA_IO | EFI_PCI_ATTRIBUTE_VGA_PALETTE_IO | EFI_PCI_ATTRIBUTE_VGA_IO |
> EFI_PCI_ATTRIBUTE_IDE_PRIMARY_IO | EFI_PCI_ATTRIBUTE_IDE_SECONDARY_IO |
> @@ -386,7 +402,7 @@ RootBridgeIoCheckParameter (
> //
> // Allow Legacy MMIO access
> //
> - if ((Address >= 0xA0000) && (Address + MultU64x32 (Count, Size)) <= 0xC0000) {
> + if ((Address >= 0xA0000) && (Address + Length) <= 0xC0000) {
> if ((RootBridge->Attributes & EFI_PCI_ATTRIBUTE_VGA_MEMORY) != 0) {
> return EFI_SUCCESS;
> }
> @@ -395,7 +411,7 @@ RootBridgeIoCheckParameter (
> // By comparing the Address against Limit we know which range to be used
> // for checking
> //
> - if (Address + MultU64x32 (Count, Size) <= RootBridge->Mem.Limit + 1) {
> + if (Address + Length <= RootBridge->Mem.Limit + 1) {
> Base = RootBridge->Mem.Base;
> Limit = RootBridge->Mem.Limit;
> } else {
> @@ -427,7 +443,7 @@ RootBridgeIoCheckParameter (
> return EFI_INVALID_PARAMETER;
> }
>
> - if (Address + MultU64x32 (Count, Size) > Limit + 1) {
> + if (Address + Length > Limit + 1) {
> return EFI_INVALID_PARAMETER;
> }
>
>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/3] MdeModulePkg/PciHostBridge: Fix a bug that prevents PMEM access
2018-09-21 7:25 ` [PATCH 2/3] MdeModulePkg/PciHostBridge: Fix a bug that prevents PMEM access Ruiyu Ni
@ 2018-09-21 11:06 ` Laszlo Ersek
2018-09-25 2:11 ` Ni, Ruiyu
2018-09-24 13:19 ` Kirkendall, Garrett
2018-09-25 2:15 ` Zeng, Star
2 siblings, 1 reply; 20+ messages in thread
From: Laszlo Ersek @ 2018-09-21 11:06 UTC (permalink / raw)
To: Ruiyu Ni, edk2-devel; +Cc: Garrett, Star Zeng, Kirkendall
On 09/21/18 09:25, Ruiyu Ni wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1196
>
> RootBridgeIoCheckParameter() verifies that the requested MMIO access
> can fit in any of the MEM/PMEM 32/64 ranges. But today's logic
> somehow only checks the requested access against MEM 32/64 ranges.
>
> It should also check the requested access against PMEM 32/64 ranges.
>
> The patch fixes this issue.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Kirkendall, Garrett <Garrett.Kirkendall@amd.com>
> ---
> MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c | 14 ++++++++++----
> 1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> index 0b6b56f846..f6234b5d11 100644
> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> @@ -411,12 +411,18 @@ RootBridgeIoCheckParameter (
> // By comparing the Address against Limit we know which range to be used
> // for checking
> //
> - if (Address + Length <= RootBridge->Mem.Limit + 1) {
> - Base = RootBridge->Mem.Base;
> + if ((Address >= RootBridge->Mem.Base) && (Address + Length <= RootBridge->Mem.Limit + 1)) {
> + Base = RootBridge->Mem.Base;
> Limit = RootBridge->Mem.Limit;
> - } else {
> - Base = RootBridge->MemAbove4G.Base;
> + } else if ((Address >= RootBridge->PMem.Base) && (Address + Length <= RootBridge->PMem.Limit + 1)) {
> + Base = RootBridge->PMem.Base;
> + Limit = RootBridge->PMem.Limit;
> + } else if ((Address >= RootBridge->MemAbove4G.Base) && (Address + Length <= RootBridge->MemAbove4G.Limit + 1)) {
> + Base = RootBridge->MemAbove4G.Base;
> Limit = RootBridge->MemAbove4G.Limit;
> + } else {
> + Base = RootBridge->PMemAbove4G.Base;
> + Limit = RootBridge->PMemAbove4G.Limit;
> }
> } else {
> PciRbAddr = (EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_PCI_ADDRESS*) &Address;
>
The interesting thing about this patch is that, if any one of the first
three branches is taken, then the final checks will automatically pass.
That's because, on the first three branches, we select the base & the
limit *because* the access falls between them. Therefore, in the end,
when we check whether the access falls between base and end, they
miraculously happen to do so. :)
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Thanks
Laszlo
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 3/3] MdeModulePkg/PciHostBridge: Add RESOURCE_VALID() to simplify code
2018-09-21 7:25 ` [PATCH 3/3] MdeModulePkg/PciHostBridge: Add RESOURCE_VALID() to simplify code Ruiyu Ni
@ 2018-09-21 11:12 ` Laszlo Ersek
2018-09-25 2:25 ` Ni, Ruiyu
2018-09-25 2:35 ` Zeng, Star
2018-09-24 13:20 ` Kirkendall, Garrett
1 sibling, 2 replies; 20+ messages in thread
From: Laszlo Ersek @ 2018-09-21 11:12 UTC (permalink / raw)
To: Ruiyu Ni, edk2-devel; +Cc: Star Zeng
On 09/21/18 09:25, Ruiyu Ni wrote:
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> ---
> .../Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c | 26 ++++++++++------------
> 1 file changed, 12 insertions(+), 14 deletions(-)
>
> diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> index f6234b5d11..916709e276 100644
> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> @@ -21,6 +21,8 @@ extern EDKII_IOMMU_PROTOCOL *mIoMmuProtocol;
>
> #define NO_MAPPING (VOID *) (UINTN) -1
>
> +#define RESOURCE_VALID(R) ((R).Base <= (R).Limit)
> +
> //
> // Lookup table for increment values based on transfer widths
> //
> @@ -122,25 +124,25 @@ CreateRootBridge (
> //
> // Make sure Mem and MemAbove4G apertures are valid
> //
> - if (Bridge->Mem.Base <= Bridge->Mem.Limit) {
> + if (RESOURCE_VALID (Bridge->Mem)) {
> ASSERT (Bridge->Mem.Limit < SIZE_4GB);
> if (Bridge->Mem.Limit >= SIZE_4GB) {
> return NULL;
> }
> }
> - if (Bridge->MemAbove4G.Base <= Bridge->MemAbove4G.Limit) {
> + if (RESOURCE_VALID (Bridge->MemAbove4G)) {
> ASSERT (Bridge->MemAbove4G.Base >= SIZE_4GB);
> if (Bridge->MemAbove4G.Base < SIZE_4GB) {
> return NULL;
> }
> }
> - if (Bridge->PMem.Base <= Bridge->PMem.Limit) {
> + if (RESOURCE_VALID (Bridge->PMem)) {
> ASSERT (Bridge->PMem.Limit < SIZE_4GB);
> if (Bridge->PMem.Limit >= SIZE_4GB) {
> return NULL;
> }
> }
> - if (Bridge->PMemAbove4G.Base <= Bridge->PMemAbove4G.Limit) {
> + if (RESOURCE_VALID (Bridge->PMemAbove4G)) {
> ASSERT (Bridge->PMemAbove4G.Base >= SIZE_4GB);
> if (Bridge->PMemAbove4G.Base < SIZE_4GB) {
> return NULL;
> @@ -157,11 +159,9 @@ CreateRootBridge (
> // support separate windows for Non-prefetchable and Prefetchable
> // memory.
> //
> - ASSERT (Bridge->PMem.Base > Bridge->PMem.Limit);
> - ASSERT (Bridge->PMemAbove4G.Base > Bridge->PMemAbove4G.Limit);
> - if ((Bridge->PMem.Base <= Bridge->PMem.Limit) ||
> - (Bridge->PMemAbove4G.Base <= Bridge->PMemAbove4G.Limit)
> - ) {
> + ASSERT (!RESOURCE_VALID (Bridge->PMem));
> + ASSERT (!RESOURCE_VALID (Bridge->PMemAbove4G));
> + if (RESOURCE_VALID (Bridge->PMem) || RESOURCE_VALID (Bridge->PMemAbove4G)) {
> return NULL;
> }
> }
> @@ -171,11 +171,9 @@ CreateRootBridge (
> // If this bit is not set, then the PCI Root Bridge does not support
> // 64 bit memory windows.
> //
> - ASSERT (Bridge->MemAbove4G.Base > Bridge->MemAbove4G.Limit);
> - ASSERT (Bridge->PMemAbove4G.Base > Bridge->PMemAbove4G.Limit);
> - if ((Bridge->MemAbove4G.Base <= Bridge->MemAbove4G.Limit) ||
> - (Bridge->PMemAbove4G.Base <= Bridge->PMemAbove4G.Limit)
> - ) {
> + ASSERT (!RESOURCE_VALID (Bridge->MemAbove4G));
> + ASSERT (!RESOURCE_VALID (Bridge->PMemAbove4G));
> + if (RESOURCE_VALID (Bridge->MemAbove4G) || RESOURCE_VALID (Bridge->PMemAbove4G)) {
> return NULL;
> }
> }
>
Two superficial comments:
- edk2 prefers long parameter names, so I suggest replacing "R" in the
macro definition with "Resource"
- taking the parameter as a pointer is frequently considered more flexible.
#define RESOURCE_VALID(Resource) ((Resource)->Base <= (Resource)->Limit)
...
if (RESOURCE_VALID (&Bridge->Mem)) {
...
Up to you -- if you like these, feel free to update the patch before
pushing it (from my side anyway; you do need MdeModulePkg maintainer
review as well).
With or without changes:
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Thanks
Laszlo
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/3] MdeModulePkg/PciHostBridge: Enhance boundary check in Io/Mem.Read/Write
2018-09-21 7:25 ` [PATCH 1/3] MdeModulePkg/PciHostBridge: Enhance boundary check in Io/Mem.Read/Write Ruiyu Ni
2018-09-21 10:53 ` Laszlo Ersek
@ 2018-09-24 13:18 ` Kirkendall, Garrett
2018-09-25 2:14 ` Zeng, Star
2 siblings, 0 replies; 20+ messages in thread
From: Kirkendall, Garrett @ 2018-09-24 13:18 UTC (permalink / raw)
To: Ruiyu Ni, edk2-devel@lists.01.org; +Cc: Star Zeng
GARRETT KIRKENDALL
SMTS Firmware Engineer | CTE
7171 Southwest Parkway, Austin, TX 78735 USA
AMD facebook | amd.com
-----Original Message-----
From: edk2-devel <edk2-devel-bounces@lists.01.org> On Behalf Of Ruiyu Ni
Sent: Friday, September 21, 2018 2:26 AM
To: edk2-devel@lists.01.org
Cc: Star Zeng <star.zeng@intel.com>
Subject: [edk2] [PATCH 1/3] MdeModulePkg/PciHostBridge: Enhance boundary check in Io/Mem.Read/Write
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
---
.../Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c | 26 +++++++++++++++++-----
1 file changed, 21 insertions(+), 5 deletions(-)
diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
index f8a1239ceb..0b6b56f846 100644
--- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
+++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
@@ -321,6 +321,7 @@ RootBridgeIoCheckParameter (
UINT64 Base;
UINT64 Limit;
UINT32 Size;
+ UINT64 Length;
//
// Check to see if Buffer is NULL
@@ -337,7 +338,7 @@ RootBridgeIoCheckParameter (
}
//
- // For FIFO type, the target address won't increase during the access,
+ // For FIFO type, the device address won't increase during the
+ access,
// so treat Count as 1
//
if (Width >= EfiPciWidthFifoUint8 && Width <= EfiPciWidthFifoUint64) { @@ -347,6 +348,13 @@ RootBridgeIoCheckParameter (
Width = (EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_WIDTH) (Width & 0x03);
Size = 1 << Width;
+ //
+ // Make sure (Count * Size) doesn't exceed MAX_UINT64 // if (Count
+ > DivU64x32 (MAX_UINT64, Size)) {
+ return EFI_INVALID_PARAMETER;
+ }
+
//
// Check to see if Address is aligned
//
@@ -354,6 +362,14 @@ RootBridgeIoCheckParameter (
return EFI_UNSUPPORTED;
}
+ //
+ // Make sure (Address + Count * Size) doesn't exceed MAX_UINT64 //
+ Length = MultU64x32 (Count, Size); if (Address > MAX_UINT64 - Length)
+ {
+ return EFI_INVALID_PARAMETER;
+ }
+
RootBridge = ROOT_BRIDGE_FROM_THIS (This);
//
@@ -372,7 +388,7 @@ RootBridgeIoCheckParameter (
//
// Allow Legacy IO access
//
- if (Address + MultU64x32 (Count, Size) <= 0x1000) {
+ if (Address + Length <= 0x1000) {
if ((RootBridge->Attributes & (
EFI_PCI_ATTRIBUTE_ISA_IO | EFI_PCI_ATTRIBUTE_VGA_PALETTE_IO | EFI_PCI_ATTRIBUTE_VGA_IO |
EFI_PCI_ATTRIBUTE_IDE_PRIMARY_IO | EFI_PCI_ATTRIBUTE_IDE_SECONDARY_IO | @@ -386,7 +402,7 @@ RootBridgeIoCheckParameter (
//
// Allow Legacy MMIO access
//
- if ((Address >= 0xA0000) && (Address + MultU64x32 (Count, Size)) <= 0xC0000) {
+ if ((Address >= 0xA0000) && (Address + Length) <= 0xC0000) {
if ((RootBridge->Attributes & EFI_PCI_ATTRIBUTE_VGA_MEMORY) != 0) {
return EFI_SUCCESS;
}
@@ -395,7 +411,7 @@ RootBridgeIoCheckParameter (
// By comparing the Address against Limit we know which range to be used
// for checking
//
- if (Address + MultU64x32 (Count, Size) <= RootBridge->Mem.Limit + 1) {
+ if (Address + Length <= RootBridge->Mem.Limit + 1) {
Base = RootBridge->Mem.Base;
Limit = RootBridge->Mem.Limit;
} else {
@@ -427,7 +443,7 @@ RootBridgeIoCheckParameter (
return EFI_INVALID_PARAMETER;
}
- if (Address + MultU64x32 (Count, Size) > Limit + 1) {
+ if (Address + Length > Limit + 1) {
return EFI_INVALID_PARAMETER;
}
--
2.16.1.windows.1
Reviewed-by: Garrett Kirkendall <garrett.kirkendall@amd.com>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 2/3] MdeModulePkg/PciHostBridge: Fix a bug that prevents PMEM access
2018-09-21 7:25 ` [PATCH 2/3] MdeModulePkg/PciHostBridge: Fix a bug that prevents PMEM access Ruiyu Ni
2018-09-21 11:06 ` Laszlo Ersek
@ 2018-09-24 13:19 ` Kirkendall, Garrett
2018-09-25 2:15 ` Zeng, Star
2 siblings, 0 replies; 20+ messages in thread
From: Kirkendall, Garrett @ 2018-09-24 13:19 UTC (permalink / raw)
To: Ruiyu Ni, edk2-devel@lists.01.org; +Cc: Star Zeng
GARRETT KIRKENDALL
SMTS Firmware Engineer | CTE
7171 Southwest Parkway, Austin, TX 78735 USA
AMD facebook | amd.com
-----Original Message-----
From: Ruiyu Ni <ruiyu.ni@intel.com>
Sent: Friday, September 21, 2018 2:26 AM
To: edk2-devel@lists.01.org
Cc: Star Zeng <star.zeng@intel.com>; Kirkendall; Kirkendall, Garrett <Garrett.Kirkendall@amd.com>
Subject: [PATCH 2/3] MdeModulePkg/PciHostBridge: Fix a bug that prevents PMEM access
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1196
RootBridgeIoCheckParameter() verifies that the requested MMIO access can fit in any of the MEM/PMEM 32/64 ranges. But today's logic somehow only checks the requested access against MEM 32/64 ranges.
It should also check the requested access against PMEM 32/64 ranges.
The patch fixes this issue.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Kirkendall, Garrett <Garrett.Kirkendall@amd.com>
---
MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
index 0b6b56f846..f6234b5d11 100644
--- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
+++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
@@ -411,12 +411,18 @@ RootBridgeIoCheckParameter (
// By comparing the Address against Limit we know which range to be used
// for checking
//
- if (Address + Length <= RootBridge->Mem.Limit + 1) {
- Base = RootBridge->Mem.Base;
+ if ((Address >= RootBridge->Mem.Base) && (Address + Length <= RootBridge->Mem.Limit + 1)) {
+ Base = RootBridge->Mem.Base;
Limit = RootBridge->Mem.Limit;
- } else {
- Base = RootBridge->MemAbove4G.Base;
+ } else if ((Address >= RootBridge->PMem.Base) && (Address + Length <= RootBridge->PMem.Limit + 1)) {
+ Base = RootBridge->PMem.Base;
+ Limit = RootBridge->PMem.Limit;
+ } else if ((Address >= RootBridge->MemAbove4G.Base) && (Address + Length <= RootBridge->MemAbove4G.Limit + 1)) {
+ Base = RootBridge->MemAbove4G.Base;
Limit = RootBridge->MemAbove4G.Limit;
+ } else {
+ Base = RootBridge->PMemAbove4G.Base;
+ Limit = RootBridge->PMemAbove4G.Limit;
}
} else {
PciRbAddr = (EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_PCI_ADDRESS*) &Address;
--
2.16.1.windows.1
Reviewed-by: Garrett Kirkendall <garrett.kirkendall@amd.com>
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 3/3] MdeModulePkg/PciHostBridge: Add RESOURCE_VALID() to simplify code
2018-09-21 7:25 ` [PATCH 3/3] MdeModulePkg/PciHostBridge: Add RESOURCE_VALID() to simplify code Ruiyu Ni
2018-09-21 11:12 ` Laszlo Ersek
@ 2018-09-24 13:20 ` Kirkendall, Garrett
1 sibling, 0 replies; 20+ messages in thread
From: Kirkendall, Garrett @ 2018-09-24 13:20 UTC (permalink / raw)
To: Ruiyu Ni, edk2-devel@lists.01.org; +Cc: Star Zeng
GARRETT KIRKENDALL
SMTS Firmware Engineer | CTE
7171 Southwest Parkway, Austin, TX 78735 USA
AMD facebook | amd.com
-----Original Message-----
From: edk2-devel <edk2-devel-bounces@lists.01.org> On Behalf Of Ruiyu Ni
Sent: Friday, September 21, 2018 2:26 AM
To: edk2-devel@lists.01.org
Cc: Star Zeng <star.zeng@intel.com>
Subject: [edk2] [PATCH 3/3] MdeModulePkg/PciHostBridge: Add RESOURCE_VALID() to simplify code
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
---
.../Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c | 26 ++++++++++------------
1 file changed, 12 insertions(+), 14 deletions(-)
diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
index f6234b5d11..916709e276 100644
--- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
+++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
@@ -21,6 +21,8 @@ extern EDKII_IOMMU_PROTOCOL *mIoMmuProtocol;
#define NO_MAPPING (VOID *) (UINTN) -1
+#define RESOURCE_VALID(R) ((R).Base <= (R).Limit)
+
//
// Lookup table for increment values based on transfer widths // @@ -122,25 +124,25 @@ CreateRootBridge (
//
// Make sure Mem and MemAbove4G apertures are valid
//
- if (Bridge->Mem.Base <= Bridge->Mem.Limit) {
+ if (RESOURCE_VALID (Bridge->Mem)) {
ASSERT (Bridge->Mem.Limit < SIZE_4GB);
if (Bridge->Mem.Limit >= SIZE_4GB) {
return NULL;
}
}
- if (Bridge->MemAbove4G.Base <= Bridge->MemAbove4G.Limit) {
+ if (RESOURCE_VALID (Bridge->MemAbove4G)) {
ASSERT (Bridge->MemAbove4G.Base >= SIZE_4GB);
if (Bridge->MemAbove4G.Base < SIZE_4GB) {
return NULL;
}
}
- if (Bridge->PMem.Base <= Bridge->PMem.Limit) {
+ if (RESOURCE_VALID (Bridge->PMem)) {
ASSERT (Bridge->PMem.Limit < SIZE_4GB);
if (Bridge->PMem.Limit >= SIZE_4GB) {
return NULL;
}
}
- if (Bridge->PMemAbove4G.Base <= Bridge->PMemAbove4G.Limit) {
+ if (RESOURCE_VALID (Bridge->PMemAbove4G)) {
ASSERT (Bridge->PMemAbove4G.Base >= SIZE_4GB);
if (Bridge->PMemAbove4G.Base < SIZE_4GB) {
return NULL;
@@ -157,11 +159,9 @@ CreateRootBridge (
// support separate windows for Non-prefetchable and Prefetchable
// memory.
//
- ASSERT (Bridge->PMem.Base > Bridge->PMem.Limit);
- ASSERT (Bridge->PMemAbove4G.Base > Bridge->PMemAbove4G.Limit);
- if ((Bridge->PMem.Base <= Bridge->PMem.Limit) ||
- (Bridge->PMemAbove4G.Base <= Bridge->PMemAbove4G.Limit)
- ) {
+ ASSERT (!RESOURCE_VALID (Bridge->PMem));
+ ASSERT (!RESOURCE_VALID (Bridge->PMemAbove4G));
+ if (RESOURCE_VALID (Bridge->PMem) || RESOURCE_VALID
+ (Bridge->PMemAbove4G)) {
return NULL;
}
}
@@ -171,11 +171,9 @@ CreateRootBridge (
// If this bit is not set, then the PCI Root Bridge does not support
// 64 bit memory windows.
//
- ASSERT (Bridge->MemAbove4G.Base > Bridge->MemAbove4G.Limit);
- ASSERT (Bridge->PMemAbove4G.Base > Bridge->PMemAbove4G.Limit);
- if ((Bridge->MemAbove4G.Base <= Bridge->MemAbove4G.Limit) ||
- (Bridge->PMemAbove4G.Base <= Bridge->PMemAbove4G.Limit)
- ) {
+ ASSERT (!RESOURCE_VALID (Bridge->MemAbove4G));
+ ASSERT (!RESOURCE_VALID (Bridge->PMemAbove4G));
+ if (RESOURCE_VALID (Bridge->MemAbove4G) || RESOURCE_VALID
+ (Bridge->PMemAbove4G)) {
return NULL;
}
}
--
2.16.1.windows.1
With Laszlo's comments
Reviewed-by: Garrett Kirkendall <garrett.kirkendall@amd.com>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 2/3] MdeModulePkg/PciHostBridge: Fix a bug that prevents PMEM access
2018-09-21 11:06 ` Laszlo Ersek
@ 2018-09-25 2:11 ` Ni, Ruiyu
0 siblings, 0 replies; 20+ messages in thread
From: Ni, Ruiyu @ 2018-09-25 2:11 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel; +Cc: Garrett, Star Zeng, Kirkendall
On 9/21/2018 7:06 PM, Laszlo Ersek wrote:
> On 09/21/18 09:25, Ruiyu Ni wrote:
>
> The interesting thing about this patch is that, if any one of the first
> three branches is taken, then the final checks will automatically pass.
> That's because, on the first three branches, we select the base & the
> limit *because* the access falls between them. Therefore, in the end,
> when we check whether the access falls between base and end, they
> miraculously happen to do so. :)
The code was written like this to maximally share the final check code:)
>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>
> Thanks
> Laszlo
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
>
--
Thanks,
Ray
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/3] MdeModulePkg/PciHostBridge: Enhance boundary check in Io/Mem.Read/Write
2018-09-21 7:25 ` [PATCH 1/3] MdeModulePkg/PciHostBridge: Enhance boundary check in Io/Mem.Read/Write Ruiyu Ni
2018-09-21 10:53 ` Laszlo Ersek
2018-09-24 13:18 ` Kirkendall, Garrett
@ 2018-09-25 2:14 ` Zeng, Star
2018-09-25 2:43 ` Ni, Ruiyu
2 siblings, 1 reply; 20+ messages in thread
From: Zeng, Star @ 2018-09-25 2:14 UTC (permalink / raw)
To: Ruiyu Ni, edk2-devel; +Cc: star.zeng
Two very small comments are added below.
On 2018/9/21 15:25, Ruiyu Ni wrote:
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> ---
> .../Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c | 26 +++++++++++++++++-----
> 1 file changed, 21 insertions(+), 5 deletions(-)
>
> diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> index f8a1239ceb..0b6b56f846 100644
> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> @@ -321,6 +321,7 @@ RootBridgeIoCheckParameter (
> UINT64 Base;
> UINT64 Limit;
> UINT32 Size;
> + UINT64 Length;
>
> //
> // Check to see if Buffer is NULL
> @@ -337,7 +338,7 @@ RootBridgeIoCheckParameter (
> }
>
> //
> - // For FIFO type, the target address won't increase during the access,
> + // For FIFO type, the device address won't increase during the access,
> // so treat Count as 1
> //
> if (Width >= EfiPciWidthFifoUint8 && Width <= EfiPciWidthFifoUint64) {
> @@ -347,6 +348,13 @@ RootBridgeIoCheckParameter (
> Width = (EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_WIDTH) (Width & 0x03);
> Size = 1 << Width;
>
> + //
> + // Make sure (Count * Size) doesn't exceed MAX_UINT64
> + //
> + if (Count > DivU64x32 (MAX_UINT64, Size)) {
> + return EFI_INVALID_PARAMETER;
> + }
> +
Mark as "Code Block 1".
> //
> // Check to see if Address is aligned
> //
> @@ -354,6 +362,14 @@ RootBridgeIoCheckParameter (
> return EFI_UNSUPPORTED;
> }
>
> + //
> + // Make sure (Address + Count * Size) doesn't exceed MAX_UINT64
> + //
> + Length = MultU64x32 (Count, Size);
> + if (Address > MAX_UINT64 - Length) {
> + return EFI_INVALID_PARAMETER;
> + }
> +
Is there some reason this code block is not put together with the "Code
Block 1"? Both are checking integer overflow.
How about also enhancing the function description a little to add one
line for describing the overflow invalid parameter cases?
@retval EFI_INVALID_PARAMETER XXX.
or just updating the line below?
@retval EFI_UNSUPPORTED The address range specified by
Address, Width,
and Count is not valid for this PI system.
Thanks,
Star
> RootBridge = ROOT_BRIDGE_FROM_THIS (This);
>
> //
> @@ -372,7 +388,7 @@ RootBridgeIoCheckParameter (
> //
> // Allow Legacy IO access
> //
> - if (Address + MultU64x32 (Count, Size) <= 0x1000) {
> + if (Address + Length <= 0x1000) {
> if ((RootBridge->Attributes & (
> EFI_PCI_ATTRIBUTE_ISA_IO | EFI_PCI_ATTRIBUTE_VGA_PALETTE_IO | EFI_PCI_ATTRIBUTE_VGA_IO |
> EFI_PCI_ATTRIBUTE_IDE_PRIMARY_IO | EFI_PCI_ATTRIBUTE_IDE_SECONDARY_IO |
> @@ -386,7 +402,7 @@ RootBridgeIoCheckParameter (
> //
> // Allow Legacy MMIO access
> //
> - if ((Address >= 0xA0000) && (Address + MultU64x32 (Count, Size)) <= 0xC0000) {
> + if ((Address >= 0xA0000) && (Address + Length) <= 0xC0000) {
> if ((RootBridge->Attributes & EFI_PCI_ATTRIBUTE_VGA_MEMORY) != 0) {
> return EFI_SUCCESS;
> }
> @@ -395,7 +411,7 @@ RootBridgeIoCheckParameter (
> // By comparing the Address against Limit we know which range to be used
> // for checking
> //
> - if (Address + MultU64x32 (Count, Size) <= RootBridge->Mem.Limit + 1) {
> + if (Address + Length <= RootBridge->Mem.Limit + 1) {
> Base = RootBridge->Mem.Base;
> Limit = RootBridge->Mem.Limit;
> } else {
> @@ -427,7 +443,7 @@ RootBridgeIoCheckParameter (
> return EFI_INVALID_PARAMETER;
> }
>
> - if (Address + MultU64x32 (Count, Size) > Limit + 1) {
> + if (Address + Length > Limit + 1) {
> return EFI_INVALID_PARAMETER;
> }
>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/3] MdeModulePkg/PciHostBridge: Fix a bug that prevents PMEM access
2018-09-21 7:25 ` [PATCH 2/3] MdeModulePkg/PciHostBridge: Fix a bug that prevents PMEM access Ruiyu Ni
2018-09-21 11:06 ` Laszlo Ersek
2018-09-24 13:19 ` Kirkendall, Garrett
@ 2018-09-25 2:15 ` Zeng, Star
2 siblings, 0 replies; 20+ messages in thread
From: Zeng, Star @ 2018-09-25 2:15 UTC (permalink / raw)
To: Ruiyu Ni, edk2-devel; +Cc: Garrett, Kirkendall, star.zeng
On 2018/9/21 15:25, Ruiyu Ni wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1196
>
> RootBridgeIoCheckParameter() verifies that the requested MMIO access
> can fit in any of the MEM/PMEM 32/64 ranges. But today's logic
> somehow only checks the requested access against MEM 32/64 ranges.
>
> It should also check the requested access against PMEM 32/64 ranges.
>
> The patch fixes this issue.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Kirkendall, Garrett <Garrett.Kirkendall@amd.com>
Reviewed-by: Star Zeng <star.zeng@intel.com>
Thanks,
Star
> ---
> MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c | 14 ++++++++++----
> 1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> index 0b6b56f846..f6234b5d11 100644
> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> @@ -411,12 +411,18 @@ RootBridgeIoCheckParameter (
> // By comparing the Address against Limit we know which range to be used
> // for checking
> //
> - if (Address + Length <= RootBridge->Mem.Limit + 1) {
> - Base = RootBridge->Mem.Base;
> + if ((Address >= RootBridge->Mem.Base) && (Address + Length <= RootBridge->Mem.Limit + 1)) {
> + Base = RootBridge->Mem.Base;
> Limit = RootBridge->Mem.Limit;
> - } else {
> - Base = RootBridge->MemAbove4G.Base;
> + } else if ((Address >= RootBridge->PMem.Base) && (Address + Length <= RootBridge->PMem.Limit + 1)) {
> + Base = RootBridge->PMem.Base;
> + Limit = RootBridge->PMem.Limit;
> + } else if ((Address >= RootBridge->MemAbove4G.Base) && (Address + Length <= RootBridge->MemAbove4G.Limit + 1)) {
> + Base = RootBridge->MemAbove4G.Base;
> Limit = RootBridge->MemAbove4G.Limit;
> + } else {
> + Base = RootBridge->PMemAbove4G.Base;
> + Limit = RootBridge->PMemAbove4G.Limit;
> }
> } else {
> PciRbAddr = (EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_PCI_ADDRESS*) &Address;
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 3/3] MdeModulePkg/PciHostBridge: Add RESOURCE_VALID() to simplify code
2018-09-21 11:12 ` Laszlo Ersek
@ 2018-09-25 2:25 ` Ni, Ruiyu
2018-09-25 2:35 ` Zeng, Star
1 sibling, 0 replies; 20+ messages in thread
From: Ni, Ruiyu @ 2018-09-25 2:25 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel; +Cc: Star Zeng
On 9/21/2018 7:12 PM, Laszlo Ersek wrote:
> On 09/21/18 09:25, Ruiyu Ni wrote:
>
> Two superficial comments:
>
> - edk2 prefers long parameter names, so I suggest replacing "R" in the
> macro definition with "Resource"
>
> - taking the parameter as a pointer is frequently considered more flexible.
>
> #define RESOURCE_VALID(Resource) ((Resource)->Base <= (Resource)->Limit)
> ...
> if (RESOURCE_VALID (&Bridge->Mem)) {
> ...
>
> Up to you -- if you like these, feel free to update the patch before
> pushing it (from my side anyway; you do need MdeModulePkg maintainer
> review as well).
Good comments. I will update the patch then commit.
>
> With or without changes:
>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>
> Thanks
> Laszlo
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
>
--
Thanks,
Ray
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 3/3] MdeModulePkg/PciHostBridge: Add RESOURCE_VALID() to simplify code
2018-09-21 11:12 ` Laszlo Ersek
2018-09-25 2:25 ` Ni, Ruiyu
@ 2018-09-25 2:35 ` Zeng, Star
2018-09-25 2:47 ` Ni, Ruiyu
1 sibling, 1 reply; 20+ messages in thread
From: Zeng, Star @ 2018-09-25 2:35 UTC (permalink / raw)
To: Laszlo Ersek, Ruiyu Ni, edk2-devel; +Cc: star.zeng
On 2018/9/21 19:12, Laszlo Ersek wrote:
> On 09/21/18 09:25, Ruiyu Ni wrote:
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
>> Cc: Star Zeng <star.zeng@intel.com>
>> ---
>> .../Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c | 26 ++++++++++------------
>> 1 file changed, 12 insertions(+), 14 deletions(-)
>>
>> diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
>> index f6234b5d11..916709e276 100644
>> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
>> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
>> @@ -21,6 +21,8 @@ extern EDKII_IOMMU_PROTOCOL *mIoMmuProtocol;
>>
>> #define NO_MAPPING (VOID *) (UINTN) -1
>>
>> +#define RESOURCE_VALID(R) ((R).Base <= (R).Limit)
>> +
>> //
>> // Lookup table for increment values based on transfer widths
>> //
>> @@ -122,25 +124,25 @@ CreateRootBridge (
>> //
>> // Make sure Mem and MemAbove4G apertures are valid
>> //
>> - if (Bridge->Mem.Base <= Bridge->Mem.Limit) {
>> + if (RESOURCE_VALID (Bridge->Mem)) {
>> ASSERT (Bridge->Mem.Limit < SIZE_4GB);
>> if (Bridge->Mem.Limit >= SIZE_4GB) {
>> return NULL;
>> }
>> }
>> - if (Bridge->MemAbove4G.Base <= Bridge->MemAbove4G.Limit) {
>> + if (RESOURCE_VALID (Bridge->MemAbove4G)) {
>> ASSERT (Bridge->MemAbove4G.Base >= SIZE_4GB);
>> if (Bridge->MemAbove4G.Base < SIZE_4GB) {
>> return NULL;
>> }
>> }
>> - if (Bridge->PMem.Base <= Bridge->PMem.Limit) {
>> + if (RESOURCE_VALID (Bridge->PMem)) {
>> ASSERT (Bridge->PMem.Limit < SIZE_4GB);
>> if (Bridge->PMem.Limit >= SIZE_4GB) {
>> return NULL;
>> }
>> }
>> - if (Bridge->PMemAbove4G.Base <= Bridge->PMemAbove4G.Limit) {
>> + if (RESOURCE_VALID (Bridge->PMemAbove4G)) {
>> ASSERT (Bridge->PMemAbove4G.Base >= SIZE_4GB);
>> if (Bridge->PMemAbove4G.Base < SIZE_4GB) {
>> return NULL;
>> @@ -157,11 +159,9 @@ CreateRootBridge (
>> // support separate windows for Non-prefetchable and Prefetchable
>> // memory.
>> //
>> - ASSERT (Bridge->PMem.Base > Bridge->PMem.Limit);
>> - ASSERT (Bridge->PMemAbove4G.Base > Bridge->PMemAbove4G.Limit);
>> - if ((Bridge->PMem.Base <= Bridge->PMem.Limit) ||
>> - (Bridge->PMemAbove4G.Base <= Bridge->PMemAbove4G.Limit)
>> - ) {
>> + ASSERT (!RESOURCE_VALID (Bridge->PMem));
>> + ASSERT (!RESOURCE_VALID (Bridge->PMemAbove4G));
>> + if (RESOURCE_VALID (Bridge->PMem) || RESOURCE_VALID (Bridge->PMemAbove4G)) {
>> return NULL;
>> }
>> }
>> @@ -171,11 +171,9 @@ CreateRootBridge (
>> // If this bit is not set, then the PCI Root Bridge does not support
>> // 64 bit memory windows.
>> //
>> - ASSERT (Bridge->MemAbove4G.Base > Bridge->MemAbove4G.Limit);
>> - ASSERT (Bridge->PMemAbove4G.Base > Bridge->PMemAbove4G.Limit);
>> - if ((Bridge->MemAbove4G.Base <= Bridge->MemAbove4G.Limit) ||
>> - (Bridge->PMemAbove4G.Base <= Bridge->PMemAbove4G.Limit)
>> - ) {
>> + ASSERT (!RESOURCE_VALID (Bridge->MemAbove4G));
>> + ASSERT (!RESOURCE_VALID (Bridge->PMemAbove4G));
>> + if (RESOURCE_VALID (Bridge->MemAbove4G) || RESOURCE_VALID (Bridge->PMemAbove4G)) {
>> return NULL;
>> }
>> }
>>
>
> Two superficial comments:
>
> - edk2 prefers long parameter names, so I suggest replacing "R" in the
> macro definition with "Resource"
>
> - taking the parameter as a pointer is frequently considered more flexible.
>
> #define RESOURCE_VALID(Resource) ((Resource)->Base <= (Resource)->Limit)
> ....
> if (RESOURCE_VALID (&Bridge->Mem)) {
> ....
>
> Up to you -- if you like these, feel free to update the patch before
> pushing it (from my side anyway; you do need MdeModulePkg maintainer
> review as well).
I have no strong preference here. Let Ray to make the choice.
I have another very small comment.
Is it better to add "#define RESOURCE_VALID(R) ((R).Base <= (R).Limit)"
in PciRootBridge.h?
Also move "#define NO_MAPPING (VOID *) (UINTN) -1" into PciRootBridge.h?
And also move "extern EDKII_IOMMU_PROTOCOL *mIoMmuProtocol;" into
PciHostBridge.h?
Thanks,
Star
>
> With or without changes:
>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>
> Thanks
> Laszlo
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/3] MdeModulePkg/PciHostBridge: Enhance boundary check in Io/Mem.Read/Write
2018-09-25 2:14 ` Zeng, Star
@ 2018-09-25 2:43 ` Ni, Ruiyu
2018-09-25 3:02 ` Zeng, Star
0 siblings, 1 reply; 20+ messages in thread
From: Ni, Ruiyu @ 2018-09-25 2:43 UTC (permalink / raw)
To: Zeng, Star, edk2-devel
On 9/25/2018 10:14 AM, Zeng, Star wrote:
> Two very small comments are added below.
>
> On 2018/9/21 15:25, Ruiyu Ni wrote:
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
>> Cc: Star Zeng <star.zeng@intel.com>
>> ---
>> .../Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c | 26
>> +++++++++++++++++-----
>> 1 file changed, 21 insertions(+), 5 deletions(-)
>>
>> diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
>> b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
>> index f8a1239ceb..0b6b56f846 100644
>> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
>> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
>> @@ -321,6 +321,7 @@ RootBridgeIoCheckParameter (
>> UINT64 Base;
>> UINT64 Limit;
>> UINT32 Size;
>> + UINT64 Length;
>> //
>> // Check to see if Buffer is NULL
>> @@ -337,7 +338,7 @@ RootBridgeIoCheckParameter (
>> }
>> //
>> - // For FIFO type, the target address won't increase during the access,
>> + // For FIFO type, the device address won't increase during the access,
>> // so treat Count as 1
>> //
>> if (Width >= EfiPciWidthFifoUint8 && Width <=
>> EfiPciWidthFifoUint64) {
>> @@ -347,6 +348,13 @@ RootBridgeIoCheckParameter (
>> Width = (EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_WIDTH) (Width & 0x03);
>> Size = 1 << Width;
>> + //
>> + // Make sure (Count * Size) doesn't exceed MAX_UINT64
>> + //
>> + if (Count > DivU64x32 (MAX_UINT64, Size)) {
>> + return EFI_INVALID_PARAMETER;
>> + }
>> +
>
> Mark as "Code Block 1".
>
>> //
>> // Check to see if Address is aligned
>> //
>> @@ -354,6 +362,14 @@ RootBridgeIoCheckParameter (
>> return EFI_UNSUPPORTED;
>> }
>> + //
>> + // Make sure (Address + Count * Size) doesn't exceed MAX_UINT64
>> + //
>> + Length = MultU64x32 (Count, Size);
>> + if (Address > MAX_UINT64 - Length) {
>> + return EFI_INVALID_PARAMETER;
>> + }
>> +
>
> Is there some reason this code block is not put together with the "Code
> Block 1"? Both are checking integer overflow.
This code block is to check whether the Address is valid.
I group the code by the parameter. If you check the original code, you
will see the checks performed on parameters: Buffer, Width, Count, Address.
>
> How about also enhancing the function description a little to add one
> line for describing the overflow invalid parameter cases?
>
> @retval EFI_INVALID_PARAMETER XXX.
Sure, I will send V2 with the updated function description.
>
> or just updating the line below?
>
> @retval EFI_UNSUPPORTED The address range specified by
> Address, Width,
> and Count is not valid for this PI
> system.
>
>
> Thanks,
> Star
>
>> RootBridge = ROOT_BRIDGE_FROM_THIS (This);
>> //
>> @@ -372,7 +388,7 @@ RootBridgeIoCheckParameter (
>> //
>> // Allow Legacy IO access
>> //
>> - if (Address + MultU64x32 (Count, Size) <= 0x1000) {
>> + if (Address + Length <= 0x1000) {
>> if ((RootBridge->Attributes & (
>> EFI_PCI_ATTRIBUTE_ISA_IO |
>> EFI_PCI_ATTRIBUTE_VGA_PALETTE_IO | EFI_PCI_ATTRIBUTE_VGA_IO |
>> EFI_PCI_ATTRIBUTE_IDE_PRIMARY_IO |
>> EFI_PCI_ATTRIBUTE_IDE_SECONDARY_IO |
>> @@ -386,7 +402,7 @@ RootBridgeIoCheckParameter (
>> //
>> // Allow Legacy MMIO access
>> //
>> - if ((Address >= 0xA0000) && (Address + MultU64x32 (Count, Size))
>> <= 0xC0000) {
>> + if ((Address >= 0xA0000) && (Address + Length) <= 0xC0000) {
>> if ((RootBridge->Attributes & EFI_PCI_ATTRIBUTE_VGA_MEMORY) !=
>> 0) {
>> return EFI_SUCCESS;
>> }
>> @@ -395,7 +411,7 @@ RootBridgeIoCheckParameter (
>> // By comparing the Address against Limit we know which range to
>> be used
>> // for checking
>> //
>> - if (Address + MultU64x32 (Count, Size) <= RootBridge->Mem.Limit +
>> 1) {
>> + if (Address + Length <= RootBridge->Mem.Limit + 1) {
>> Base = RootBridge->Mem.Base;
>> Limit = RootBridge->Mem.Limit;
>> } else {
>> @@ -427,7 +443,7 @@ RootBridgeIoCheckParameter (
>> return EFI_INVALID_PARAMETER;
>> }
>> - if (Address + MultU64x32 (Count, Size) > Limit + 1) {
>> + if (Address + Length > Limit + 1) {
>> return EFI_INVALID_PARAMETER;
>> }
>>
>
--
Thanks,
Ray
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 3/3] MdeModulePkg/PciHostBridge: Add RESOURCE_VALID() to simplify code
2018-09-25 2:35 ` Zeng, Star
@ 2018-09-25 2:47 ` Ni, Ruiyu
2018-09-25 3:13 ` Zeng, Star
0 siblings, 1 reply; 20+ messages in thread
From: Ni, Ruiyu @ 2018-09-25 2:47 UTC (permalink / raw)
To: Zeng, Star, Laszlo Ersek, edk2-devel
On 9/25/2018 10:35 AM, Zeng, Star wrote:
> On 2018/9/21 19:12, Laszlo Ersek wrote:
>> On 09/21/18 09:25, Ruiyu Ni wrote:
>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>> Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
>>> Cc: Star Zeng <star.zeng@intel.com>
>>> ---
>>> .../Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c | 26
>>> ++++++++++------------
>>> 1 file changed, 12 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
>>> b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
>>> index f6234b5d11..916709e276 100644
>>> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
>>> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
>>> @@ -21,6 +21,8 @@ extern EDKII_IOMMU_PROTOCOL *mIoMmuProtocol;
>>> #define NO_MAPPING (VOID *) (UINTN) -1
>>> +#define RESOURCE_VALID(R) ((R).Base <= (R).Limit)
>>> +
>>> //
>>> // Lookup table for increment values based on transfer widths
>>> //
>>> @@ -122,25 +124,25 @@ CreateRootBridge (
>>> //
>>> // Make sure Mem and MemAbove4G apertures are valid
>>> //
>>> - if (Bridge->Mem.Base <= Bridge->Mem.Limit) {
>>> + if (RESOURCE_VALID (Bridge->Mem)) {
>>> ASSERT (Bridge->Mem.Limit < SIZE_4GB);
>>> if (Bridge->Mem.Limit >= SIZE_4GB) {
>>> return NULL;
>>> }
>>> }
>>> - if (Bridge->MemAbove4G.Base <= Bridge->MemAbove4G.Limit) {
>>> + if (RESOURCE_VALID (Bridge->MemAbove4G)) {
>>> ASSERT (Bridge->MemAbove4G.Base >= SIZE_4GB);
>>> if (Bridge->MemAbove4G.Base < SIZE_4GB) {
>>> return NULL;
>>> }
>>> }
>>> - if (Bridge->PMem.Base <= Bridge->PMem.Limit) {
>>> + if (RESOURCE_VALID (Bridge->PMem)) {
>>> ASSERT (Bridge->PMem.Limit < SIZE_4GB);
>>> if (Bridge->PMem.Limit >= SIZE_4GB) {
>>> return NULL;
>>> }
>>> }
>>> - if (Bridge->PMemAbove4G.Base <= Bridge->PMemAbove4G.Limit) {
>>> + if (RESOURCE_VALID (Bridge->PMemAbove4G)) {
>>> ASSERT (Bridge->PMemAbove4G.Base >= SIZE_4GB);
>>> if (Bridge->PMemAbove4G.Base < SIZE_4GB) {
>>> return NULL;
>>> @@ -157,11 +159,9 @@ CreateRootBridge (
>>> // support separate windows for Non-prefetchable and
>>> Prefetchable
>>> // memory.
>>> //
>>> - ASSERT (Bridge->PMem.Base > Bridge->PMem.Limit);
>>> - ASSERT (Bridge->PMemAbove4G.Base > Bridge->PMemAbove4G.Limit);
>>> - if ((Bridge->PMem.Base <= Bridge->PMem.Limit) ||
>>> - (Bridge->PMemAbove4G.Base <= Bridge->PMemAbove4G.Limit)
>>> - ) {
>>> + ASSERT (!RESOURCE_VALID (Bridge->PMem));
>>> + ASSERT (!RESOURCE_VALID (Bridge->PMemAbove4G));
>>> + if (RESOURCE_VALID (Bridge->PMem) || RESOURCE_VALID
>>> (Bridge->PMemAbove4G)) {
>>> return NULL;
>>> }
>>> }
>>> @@ -171,11 +171,9 @@ CreateRootBridge (
>>> // If this bit is not set, then the PCI Root Bridge does not
>>> support
>>> // 64 bit memory windows.
>>> //
>>> - ASSERT (Bridge->MemAbove4G.Base > Bridge->MemAbove4G.Limit);
>>> - ASSERT (Bridge->PMemAbove4G.Base > Bridge->PMemAbove4G.Limit);
>>> - if ((Bridge->MemAbove4G.Base <= Bridge->MemAbove4G.Limit) ||
>>> - (Bridge->PMemAbove4G.Base <= Bridge->PMemAbove4G.Limit)
>>> - ) {
>>> + ASSERT (!RESOURCE_VALID (Bridge->MemAbove4G));
>>> + ASSERT (!RESOURCE_VALID (Bridge->PMemAbove4G));
>>> + if (RESOURCE_VALID (Bridge->MemAbove4G) || RESOURCE_VALID
>>> (Bridge->PMemAbove4G)) {
>>> return NULL;
>>> }
>>> }
>>>
>>
>> Two superficial comments:
>>
>> - edk2 prefers long parameter names, so I suggest replacing "R" in the
>> macro definition with "Resource"
>>
>> - taking the parameter as a pointer is frequently considered more
>> flexible.
>>
>> #define RESOURCE_VALID(Resource) ((Resource)->Base <= (Resource)->Limit)
>> ....
>> if (RESOURCE_VALID (&Bridge->Mem)) {
>> ....
>>
>> Up to you -- if you like these, feel free to update the patch before
>> pushing it (from my side anyway; you do need MdeModulePkg maintainer
>> review as well).
>
> I have no strong preference here. Let Ray to make the choice.
> I have another very small comment.
> Is it better to add "#define RESOURCE_VALID(R) ((R).Base <= (R).Limit)"
> in PciRootBridge.h?
> Also move "#define NO_MAPPING (VOID *) (UINTN) -1" into PciRootBridge.h?
> And also move "extern EDKII_IOMMU_PROTOCOL *mIoMmuProtocol;" into
> PciHostBridge.h?
The NO_MAPPING, RESOURCE_VALID macros are only used in this C file.
I prefer to put them in PciRootBridge.h only when another C file needs
to reference these macros.
I agree moving mIoMmuProtocol to PciHostBridge.h.
I am happy to do that in a separate patch in V2.
Agree?
>
> Thanks,
> Star
>
>>
>> With or without changes:
>>
>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>>
>> Thanks
>> Laszlo
>>
>
--
Thanks,
Ray
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/3] MdeModulePkg/PciHostBridge: Enhance boundary check in Io/Mem.Read/Write
2018-09-25 2:43 ` Ni, Ruiyu
@ 2018-09-25 3:02 ` Zeng, Star
0 siblings, 0 replies; 20+ messages in thread
From: Zeng, Star @ 2018-09-25 3:02 UTC (permalink / raw)
To: Ni, Ruiyu, edk2-devel; +Cc: star.zeng
On 2018/9/25 10:43, Ni, Ruiyu wrote:
> On 9/25/2018 10:14 AM, Zeng, Star wrote:
>> Two very small comments are added below.
>>
>> On 2018/9/21 15:25, Ruiyu Ni wrote:
>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>> Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
>>> Cc: Star Zeng <star.zeng@intel.com>
>>> ---
>>> .../Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c | 26
>>> +++++++++++++++++-----
>>> 1 file changed, 21 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
>>> b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
>>> index f8a1239ceb..0b6b56f846 100644
>>> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
>>> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
>>> @@ -321,6 +321,7 @@ RootBridgeIoCheckParameter (
>>> UINT64 Base;
>>> UINT64 Limit;
>>> UINT32 Size;
>>> + UINT64 Length;
>>> //
>>> // Check to see if Buffer is NULL
>>> @@ -337,7 +338,7 @@ RootBridgeIoCheckParameter (
>>> }
>>> //
>>> - // For FIFO type, the target address won't increase during the
>>> access,
>>> + // For FIFO type, the device address won't increase during the
>>> access,
>>> // so treat Count as 1
>>> //
>>> if (Width >= EfiPciWidthFifoUint8 && Width <=
>>> EfiPciWidthFifoUint64) {
>>> @@ -347,6 +348,13 @@ RootBridgeIoCheckParameter (
>>> Width = (EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_WIDTH) (Width & 0x03);
>>> Size = 1 << Width;
>>> + //
>>> + // Make sure (Count * Size) doesn't exceed MAX_UINT64
>>> + //
>>> + if (Count > DivU64x32 (MAX_UINT64, Size)) {
>>> + return EFI_INVALID_PARAMETER;
>>> + }
>>> +
>>
>> Mark as "Code Block 1".
>>
>>> //
>>> // Check to see if Address is aligned
>>> //
>>> @@ -354,6 +362,14 @@ RootBridgeIoCheckParameter (
>>> return EFI_UNSUPPORTED;
>>> }
>>> + //
>>> + // Make sure (Address + Count * Size) doesn't exceed MAX_UINT64
>>> + //
>>> + Length = MultU64x32 (Count, Size);
>>> + if (Address > MAX_UINT64 - Length) {
>>> + return EFI_INVALID_PARAMETER;
>>> + }
>>> +
>>
>> Is there some reason this code block is not put together with the
>> "Code Block 1"? Both are checking integer overflow.
>
> This code block is to check whether the Address is valid.
> I group the code by the parameter. If you check the original code, you
> will see the checks performed on parameters: Buffer, Width, Count, Address.
Got it about the checking sequence.
But even this code block is moved to before "Check to see if Address is
aligned" and after "Make sure (Count * Size) doesn't exceed MAX_UINT64",
the checking sequence is kept. Only difference is first checking
unsupported or first checking invalid for Address parameter.
Anyway, I have no strong opinion for that. You can decide. :)
>
>
>>
>> How about also enhancing the function description a little to add one
>> line for describing the overflow invalid parameter cases?
>>
>> @retval EFI_INVALID_PARAMETER XXX.
>
> Sure, I will send V2 with the updated function description.
Thanks. You may consider just updating the below EFI_UNSUPPORTED to
EFI_INVALID_PARAMETER.
@retval EFI_UNSUPPORTED The address range specified by
Address, Width,
and Count is not valid for this PI system.
Star
>
>>
>> or just updating the line below?
>>
>> @retval EFI_UNSUPPORTED The address range specified by
>> Address, Width,
>> and Count is not valid for this PI
>> system.
>>
>>
>> Thanks,
>> Star
>>
>>> RootBridge = ROOT_BRIDGE_FROM_THIS (This);
>>> //
>>> @@ -372,7 +388,7 @@ RootBridgeIoCheckParameter (
>>> //
>>> // Allow Legacy IO access
>>> //
>>> - if (Address + MultU64x32 (Count, Size) <= 0x1000) {
>>> + if (Address + Length <= 0x1000) {
>>> if ((RootBridge->Attributes & (
>>> EFI_PCI_ATTRIBUTE_ISA_IO |
>>> EFI_PCI_ATTRIBUTE_VGA_PALETTE_IO | EFI_PCI_ATTRIBUTE_VGA_IO |
>>> EFI_PCI_ATTRIBUTE_IDE_PRIMARY_IO |
>>> EFI_PCI_ATTRIBUTE_IDE_SECONDARY_IO |
>>> @@ -386,7 +402,7 @@ RootBridgeIoCheckParameter (
>>> //
>>> // Allow Legacy MMIO access
>>> //
>>> - if ((Address >= 0xA0000) && (Address + MultU64x32 (Count, Size))
>>> <= 0xC0000) {
>>> + if ((Address >= 0xA0000) && (Address + Length) <= 0xC0000) {
>>> if ((RootBridge->Attributes & EFI_PCI_ATTRIBUTE_VGA_MEMORY)
>>> != 0) {
>>> return EFI_SUCCESS;
>>> }
>>> @@ -395,7 +411,7 @@ RootBridgeIoCheckParameter (
>>> // By comparing the Address against Limit we know which range
>>> to be used
>>> // for checking
>>> //
>>> - if (Address + MultU64x32 (Count, Size) <= RootBridge->Mem.Limit
>>> + 1) {
>>> + if (Address + Length <= RootBridge->Mem.Limit + 1) {
>>> Base = RootBridge->Mem.Base;
>>> Limit = RootBridge->Mem.Limit;
>>> } else {
>>> @@ -427,7 +443,7 @@ RootBridgeIoCheckParameter (
>>> return EFI_INVALID_PARAMETER;
>>> }
>>> - if (Address + MultU64x32 (Count, Size) > Limit + 1) {
>>> + if (Address + Length > Limit + 1) {
>>> return EFI_INVALID_PARAMETER;
>>> }
>>>
>>
>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 3/3] MdeModulePkg/PciHostBridge: Add RESOURCE_VALID() to simplify code
2018-09-25 2:47 ` Ni, Ruiyu
@ 2018-09-25 3:13 ` Zeng, Star
2018-09-25 5:03 ` Ni, Ruiyu
0 siblings, 1 reply; 20+ messages in thread
From: Zeng, Star @ 2018-09-25 3:13 UTC (permalink / raw)
To: Ni, Ruiyu, Laszlo Ersek, edk2-devel; +Cc: star.zeng
On 2018/9/25 10:47, Ni, Ruiyu wrote:
> On 9/25/2018 10:35 AM, Zeng, Star wrote:
>> On 2018/9/21 19:12, Laszlo Ersek wrote:
>>> On 09/21/18 09:25, Ruiyu Ni wrote:
>>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>>> Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
>>>> Cc: Star Zeng <star.zeng@intel.com>
>>>> ---
>>>> .../Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c | 26
>>>> ++++++++++------------
>>>> 1 file changed, 12 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
>>>> b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
>>>> index f6234b5d11..916709e276 100644
>>>> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
>>>> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
>>>> @@ -21,6 +21,8 @@ extern EDKII_IOMMU_PROTOCOL *mIoMmuProtocol;
>>>> #define NO_MAPPING (VOID *) (UINTN) -1
>>>> +#define RESOURCE_VALID(R) ((R).Base <= (R).Limit)
>>>> +
>>>> //
>>>> // Lookup table for increment values based on transfer widths
>>>> //
>>>> @@ -122,25 +124,25 @@ CreateRootBridge (
>>>> //
>>>> // Make sure Mem and MemAbove4G apertures are valid
>>>> //
>>>> - if (Bridge->Mem.Base <= Bridge->Mem.Limit) {
>>>> + if (RESOURCE_VALID (Bridge->Mem)) {
>>>> ASSERT (Bridge->Mem.Limit < SIZE_4GB);
>>>> if (Bridge->Mem.Limit >= SIZE_4GB) {
>>>> return NULL;
>>>> }
>>>> }
>>>> - if (Bridge->MemAbove4G.Base <= Bridge->MemAbove4G.Limit) {
>>>> + if (RESOURCE_VALID (Bridge->MemAbove4G)) {
>>>> ASSERT (Bridge->MemAbove4G.Base >= SIZE_4GB);
>>>> if (Bridge->MemAbove4G.Base < SIZE_4GB) {
>>>> return NULL;
>>>> }
>>>> }
>>>> - if (Bridge->PMem.Base <= Bridge->PMem.Limit) {
>>>> + if (RESOURCE_VALID (Bridge->PMem)) {
>>>> ASSERT (Bridge->PMem.Limit < SIZE_4GB);
>>>> if (Bridge->PMem.Limit >= SIZE_4GB) {
>>>> return NULL;
>>>> }
>>>> }
>>>> - if (Bridge->PMemAbove4G.Base <= Bridge->PMemAbove4G.Limit) {
>>>> + if (RESOURCE_VALID (Bridge->PMemAbove4G)) {
>>>> ASSERT (Bridge->PMemAbove4G.Base >= SIZE_4GB);
>>>> if (Bridge->PMemAbove4G.Base < SIZE_4GB) {
>>>> return NULL;
>>>> @@ -157,11 +159,9 @@ CreateRootBridge (
>>>> // support separate windows for Non-prefetchable and
>>>> Prefetchable
>>>> // memory.
>>>> //
>>>> - ASSERT (Bridge->PMem.Base > Bridge->PMem.Limit);
>>>> - ASSERT (Bridge->PMemAbove4G.Base > Bridge->PMemAbove4G.Limit);
>>>> - if ((Bridge->PMem.Base <= Bridge->PMem.Limit) ||
>>>> - (Bridge->PMemAbove4G.Base <= Bridge->PMemAbove4G.Limit)
>>>> - ) {
>>>> + ASSERT (!RESOURCE_VALID (Bridge->PMem));
>>>> + ASSERT (!RESOURCE_VALID (Bridge->PMemAbove4G));
>>>> + if (RESOURCE_VALID (Bridge->PMem) || RESOURCE_VALID
>>>> (Bridge->PMemAbove4G)) {
>>>> return NULL;
>>>> }
>>>> }
>>>> @@ -171,11 +171,9 @@ CreateRootBridge (
>>>> // If this bit is not set, then the PCI Root Bridge does not
>>>> support
>>>> // 64 bit memory windows.
>>>> //
>>>> - ASSERT (Bridge->MemAbove4G.Base > Bridge->MemAbove4G.Limit);
>>>> - ASSERT (Bridge->PMemAbove4G.Base > Bridge->PMemAbove4G.Limit);
>>>> - if ((Bridge->MemAbove4G.Base <= Bridge->MemAbove4G.Limit) ||
>>>> - (Bridge->PMemAbove4G.Base <= Bridge->PMemAbove4G.Limit)
>>>> - ) {
>>>> + ASSERT (!RESOURCE_VALID (Bridge->MemAbove4G));
>>>> + ASSERT (!RESOURCE_VALID (Bridge->PMemAbove4G));
>>>> + if (RESOURCE_VALID (Bridge->MemAbove4G) || RESOURCE_VALID
>>>> (Bridge->PMemAbove4G)) {
>>>> return NULL;
>>>> }
>>>> }
>>>>
>>>
>>> Two superficial comments:
>>>
>>> - edk2 prefers long parameter names, so I suggest replacing "R" in the
>>> macro definition with "Resource"
>>>
>>> - taking the parameter as a pointer is frequently considered more
>>> flexible.
>>>
>>> #define RESOURCE_VALID(Resource) ((Resource)->Base <= (Resource)->Limit)
>>> ....
>>> if (RESOURCE_VALID (&Bridge->Mem)) {
>>> ....
>>>
>>> Up to you -- if you like these, feel free to update the patch before
>>> pushing it (from my side anyway; you do need MdeModulePkg maintainer
>>> review as well).
>>
>> I have no strong preference here. Let Ray to make the choice.
>> I have another very small comment.
>> Is it better to add "#define RESOURCE_VALID(R) ((R).Base <=
>> (R).Limit)" in PciRootBridge.h?
>> Also move "#define NO_MAPPING (VOID *) (UINTN) -1" into PciRootBridge.h?
>> And also move "extern EDKII_IOMMU_PROTOCOL *mIoMmuProtocol;"
>> into PciHostBridge.h?
>
> The NO_MAPPING, RESOURCE_VALID macros are only used in this C file.
> I prefer to put them in PciRootBridge.h only when another C file needs
> to reference these macros.
But then there will be a little inconsistent, for example OPERATION_TYPE
is only used by PciRootBridgeIo.c.
>
> I agree moving mIoMmuProtocol to PciHostBridge.h.
> I am happy to do that in a separate patch in V2.
Thanks. If we will only move mIoMmuProtocol, it can be in a separated patch.
Star
>
> Agree?
>
>>
>> Thanks,
>> Star
>>
>>>
>>> With or without changes:
>>>
>>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>>>
>>> Thanks
>>> Laszlo
>>>
>>
>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 3/3] MdeModulePkg/PciHostBridge: Add RESOURCE_VALID() to simplify code
2018-09-25 3:13 ` Zeng, Star
@ 2018-09-25 5:03 ` Ni, Ruiyu
0 siblings, 0 replies; 20+ messages in thread
From: Ni, Ruiyu @ 2018-09-25 5:03 UTC (permalink / raw)
To: Zeng, Star, Laszlo Ersek, edk2-devel
On 9/25/2018 11:13 AM, Zeng, Star wrote:
> On 2018/9/25 10:47, Ni, Ruiyu wrote:
>
> But then there will be a little inconsistent, for example OPERATION_TYPE
> is only used by PciRootBridgeIo.c.
Since coding style document doesn't define clear rule for this (I also
don't like a coding style document with so many detailed restrictions.
This removes the fun from coding.), I agree there is inconsistency.
>
>
>>
>> I agree moving mIoMmuProtocol to PciHostBridge.h.
>> I am happy to do that in a separate patch in V2.
>
> Thanks. If we will only move mIoMmuProtocol, it can be in a separated
> patch.
>
>
> Star
>
>>
>> Agree?
>>
>>>
>>> Thanks,
>>> Star
>>>
>>>>
>>>> With or without changes:
>>>>
>>>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>>>>
>>>> Thanks
>>>> Laszlo
>>>>
>>>
>>
>>
>
--
Thanks,
Ray
^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2018-09-25 5:03 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-09-21 7:25 [PATCH 0/3] Fix a bug that prevents PMEM access Ruiyu Ni
2018-09-21 7:25 ` [PATCH 1/3] MdeModulePkg/PciHostBridge: Enhance boundary check in Io/Mem.Read/Write Ruiyu Ni
2018-09-21 10:53 ` Laszlo Ersek
2018-09-24 13:18 ` Kirkendall, Garrett
2018-09-25 2:14 ` Zeng, Star
2018-09-25 2:43 ` Ni, Ruiyu
2018-09-25 3:02 ` Zeng, Star
2018-09-21 7:25 ` [PATCH 2/3] MdeModulePkg/PciHostBridge: Fix a bug that prevents PMEM access Ruiyu Ni
2018-09-21 11:06 ` Laszlo Ersek
2018-09-25 2:11 ` Ni, Ruiyu
2018-09-24 13:19 ` Kirkendall, Garrett
2018-09-25 2:15 ` Zeng, Star
2018-09-21 7:25 ` [PATCH 3/3] MdeModulePkg/PciHostBridge: Add RESOURCE_VALID() to simplify code Ruiyu Ni
2018-09-21 11:12 ` Laszlo Ersek
2018-09-25 2:25 ` Ni, Ruiyu
2018-09-25 2:35 ` Zeng, Star
2018-09-25 2:47 ` Ni, Ruiyu
2018-09-25 3:13 ` Zeng, Star
2018-09-25 5:03 ` Ni, Ruiyu
2018-09-24 13:20 ` Kirkendall, Garrett
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox