PCIe on some ARM platforms requires address translation, not only for
legacy IO access, but also for 32bit memory BAR access as well. There
will be "Address Translation Unit" or something similar in PCI host
bridges to translation CPU address to PCI address and vice versa. So
we think it may be useful to add address translation support to the
generic PCI host bridge driver.
This RFC only contains one minor change to the definition of
PciHostBridgeLib, and there certainly will be a lot of other changes
to make it work, including:
1. Use CPU address for GCD space add and allocate operations, instead
of PCI address; also IO space will be changed to memory space if
translation exists.
2. RootBridgeIoMemRead/Write, RootBridgeIoRead/Write need to get
translation of the corresponding aperture, add the translation to the
input address, and then call CpuIo2 protocol; IO access will also be
converted to memory access if IO translation exists.
3. RootBridgeIoConfiguration needs to fill AddrTranslationOffset in
the discriptor.
If it makes sense, then I'll continue to prepare the formal patch.
Any comments?
Thanks,
Gary (Heyi Guo)
Cc: Star Zeng <star.zeng(a)intel.com>
Cc: Eric Dong <eric.dong(a)intel.com>
Cc: Ruiyu Ni <ruiyu.ni(a)intel.com>
Cc: Ard Biesheuvel <ard.biesheuvel(a)linaro.org>
Cc: Jason Zhang <jason.zhang(a)linaro.org>
---
MdeModulePkg/Include/Library/PciHostBridgeLib.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/MdeModulePkg/Include/Library/PciHostBridgeLib.h b/MdeModulePkg/Include/Library/PciHostBridgeLib.h
index d42e9ec..b9e8c0f 100644
--- a/MdeModulePkg/Include/Library/PciHostBridgeLib.h
+++ b/MdeModulePkg/Include/Library/PciHostBridgeLib.h
@@ -22,6 +22,7 @@
typedef struct {
UINT64 Base;
UINT64 Limit;
+ UINT64 Translation;
} PCI_ROOT_BRIDGE_APERTURE;
typedef struct {
--
2.7.4
When UEFI receives IPMP echo packets it will enter Ip4IcmpReplyEcho
function, and then call Ip4Output. However, if Ip4Output gets some
error and exits early, e.g. fails to find the route entry, memory
buffer of "Data" gets no chance to be freed and memory leak will be
caused. If there is such an attacker in the network, we will see UEFI
runs out of memory and system hangs.
So we explicitly free the memory when error status is returned.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Junbiao Hong <hongjunbiao(a)huawei.com>
Signed-off-by: Heyi Guo <heyi.guo(a)linaro.org>
Reviewed-by: Siyuan Fu <siyuan.fu(a)intel.com>
Reviewed-by: Jiaxin Wu <jiaxin.wu(a)intel.com>
Cc: Star Zeng <star.zeng(a)intel.com>
Cc: Eric Dong <eric.dong(a)intel.com>
Cc: Ruiyu Ni <ruiyu.ni(a)intel.com>
Cc: Siyuan Fu <siyuan.fu(a)intel.com>
Cc: Jiaxin Wu <jiaxin.wu(a)intel.com>
---
MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Icmp.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Icmp.c b/MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Icmp.c
index b4b0864..ed6bdbe 100644
--- a/MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Icmp.c
+++ b/MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Icmp.c
@@ -267,6 +267,9 @@ Ip4IcmpReplyEcho (
Ip4SysPacketSent,
NULL
);
+ if (EFI_ERROR (Status)) {
+ NetbufFree (Data);
+ }
ON_EXIT:
NetbufFree (Packet);
--
2.7.4
When UEFI receives IPMP echo packets it will enter Ip4IcmpReplyEcho
function, and then call Ip4Output. However, if Ip4Output gets some
error and exits early, e.g. fails to find the route entry, memory
buffer of "Data" gets no chance to be freed and memory leak will be
caused. If there is such an attacker in the network, we will see UEFI
runs out of memory and system hangs.
Network stack code is so complicated that this is just a RFC to fix
this issue. Please provide your comments about this.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Junbiao Hong <hongjunbiao(a)huawei.com>
Signed-off-by: Heyi Guo <heyi.guo(a)linaro.org>
Cc: Star Zeng <star.zeng(a)intel.com>
Cc: Eric Dong <eric.dong(a)intel.com>
Cc: Ruiyu Ni <ruiyu.ni(a)intel.com>
Cc: Siyuan Fu <siyuan.fu(a)intel.com>
Cc: Jiaxin Wu <jiaxin.wu(a)intel.com>
---
MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Icmp.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Icmp.c b/MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Icmp.c
index b4b0864..ed6bdbe 100644
--- a/MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Icmp.c
+++ b/MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Icmp.c
@@ -267,6 +267,9 @@ Ip4IcmpReplyEcho (
Ip4SysPacketSent,
NULL
);
+ if (EFI_ERROR (Status)) {
+ NetbufFree (Data);
+ }
ON_EXIT:
NetbufFree (Packet);
--
2.7.4
Commit f6b139b added return status handling to PciIo->Mem.Write.
However, the second status handling will override EFI_DEVICE_ERROR
returned in this branch:
//
// Check the NVMe cmd execution result
//
if (Status != EFI_TIMEOUT) {
if ((Cq->Sct == 0) && (Cq->Sc == 0)) {
Status = EFI_SUCCESS;
} else {
Status = EFI_DEVICE_ERROR;
^^^^^^^^^^^^^^^^
Since PciIo->Mem.Write will probably return SUCCESS, it causes
NvmExpressPassThru to return SUCCESS even when DEVICE_ERROR occurs.
Callers of NvmExpressPassThru will then continue executing which may
cause further unexpected results, e.g. DiscoverAllNamespaces couldn't
break out the loop.
So we save previous status before calling PciIo->Mem.Write and restore
the previous one if it already contains error.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Heyi Guo <heyi.guo(a)linaro.org>
Cc: Star Zeng <star.zeng(a)intel.com>
Cc: Eric Dong <eric.dong(a)intel.com>
Cc: Hao Wu <hao.a.wu(a)intel.com>
Cc: Ruiyu Ni <ruiyu.ni(a)intel.com>
---
MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
index c33038f..7356c1d 100644
--- a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
+++ b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
@@ -453,6 +453,7 @@ NvmExpressPassThru (
{
NVME_CONTROLLER_PRIVATE_DATA *Private;
EFI_STATUS Status;
+ EFI_STATUS PreviousStatus;
EFI_PCI_IO_PROTOCOL *PciIo;
NVME_SQ *Sq;
NVME_CQ *Cq;
@@ -831,6 +832,7 @@ NvmExpressPassThru (
}
Data = ReadUnaligned32 ((UINT32*)&Private->CqHdbl[QueueId]);
+ PreviousStatus = Status;
Status = PciIo->Mem.Write (
PciIo,
EfiPciIoWidthUint32,
@@ -839,6 +841,9 @@ NvmExpressPassThru (
1,
&Data
);
+ // The return status of PciIo->Mem.Write should not override
+ // previous status if previous status contains error.
+ Status = EFI_ERROR (PreviousStatus) ? PreviousStatus : Status;
//
// For now, the code does not support the non-blocking feature for admin queue.
--
2.7.2.windows.1
Commit f6b139b added return status handling to PciIo->Mem.Write.
However, the second status handling will override EFI_DEVICE_ERROR
returned in this branch:
//
// Check the NVMe cmd execution result
//
if (Status != EFI_TIMEOUT) {
if ((Cq->Sct == 0) && (Cq->Sc == 0)) {
Status = EFI_SUCCESS;
} else {
Status = EFI_DEVICE_ERROR;
^^^^^^^^^^^^^^^^
Since PciIo->Mem.Write will probably return SUCCESS, it causes
NvmExpressPassThru to return SUCCESS even when DEVICE_ERROR occurs.
Callers of NvmExpressPassThru will then continue executing which may
cause further unexpected results, e.g. DiscoverAllNamespaces couldn't
break out the loop.
So we add a | (bit-or) to combine the return status together.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Heyi Guo <guoheyi(a)huawei.com>
Cc: Star Zeng <star.zeng(a)intel.com>
Cc: Eric Dong <eric.dong(a)intel.com>
---
MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
index c33038f..2698b27 100644
--- a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
+++ b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
@@ -831,7 +831,7 @@ NvmExpressPassThru (
}
Data = ReadUnaligned32 ((UINT32*)&Private->CqHdbl[QueueId]);
- Status = PciIo->Mem.Write (
+ Status |= PciIo->Mem.Write (
PciIo,
EfiPciIoWidthUint32,
NVME_BAR,
--
2.7.2.windows.1
DXE performance gauge record access functions might be reentered since
we are supporting something like USB hot-plug, which is a timer event
where gBS->ConnectController might be called and then PERF will be
called in CoreConnectSingleController.
When StartGaugeEx is being reentered, not only the gauge record might
be overwritten, more serious situation will be caused if gauge data
buffer reallocation procedure is interrupted, between line 180 and 187
in DxeCorePerformanceLib.c specifically. There, mMaxGaugeRecords will
be doubled twice (denoted as 4X), but mGaugeData only points to a
buffer of size 2X, which will probably cause the following 2X memory
to be overflowed when gauge records are increased.
So we add EFI lock with with TPL_NOTIFY in
StartGaugeEx/EndGaugeEx/GetGaugeEx to avoid memory overflow and gauge
data corruption.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Heyi Guo <heyi.guo(a)linaro.org>
Cc: Star Zeng <star.zeng(a)intel.com>
Cc: Eric Dong <eric.dong(a)intel.com>
Cc: Liming Gao <liming.gao(a)intel.com>
---
.../DxeCorePerformanceLib/DxeCorePerformanceLib.c | 67 +++++++++++++++++++++-
1 file changed, 66 insertions(+), 1 deletion(-)
diff --git a/MdeModulePkg/Library/DxeCorePerformanceLib/DxeCorePerformanceLib.c b/MdeModulePkg/Library/DxeCorePerformanceLib/DxeCorePerformanceLib.c
index 51f488a..7c0e207 100644
--- a/MdeModulePkg/Library/DxeCorePerformanceLib/DxeCorePerformanceLib.c
+++ b/MdeModulePkg/Library/DxeCorePerformanceLib/DxeCorePerformanceLib.c
@@ -63,6 +63,11 @@ PERFORMANCE_EX_PROTOCOL mPerformanceExInterface = {
PERFORMANCE_PROPERTY mPerformanceProperty;
+//
+// Gauge record lock to avoid data corruption or even memory overflow
+//
+STATIC EFI_LOCK mPerfRecordLock = EFI_INITIALIZE_LOCK_VARIABLE (TPL_NOTIFY);
+
/**
Searches in the gauge array with keyword Handle, Token, Module and Identifier.
@@ -162,6 +167,12 @@ StartGaugeEx (
UINTN OldGaugeDataSize;
GAUGE_DATA_HEADER *OldGaugeData;
UINT32 Index;
+ EFI_STATUS Status;
+
+ Status = EfiAcquireLockOrFail (&mPerfRecordLock);
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
Index = mGaugeData->NumberOfEntries;
if (Index >= mMaxGaugeRecords) {
@@ -175,6 +186,7 @@ StartGaugeEx (
NewGaugeData = AllocateZeroPool (GaugeDataSize);
if (NewGaugeData == NULL) {
+ EfiReleaseLock (&mPerfRecordLock);
return EFI_OUT_OF_RESOURCES;
}
@@ -209,6 +221,8 @@ StartGaugeEx (
mGaugeData->NumberOfEntries++;
+ EfiReleaseLock (&mPerfRecordLock);
+
return EFI_SUCCESS;
}
@@ -250,6 +264,12 @@ EndGaugeEx (
{
GAUGE_DATA_ENTRY_EX *GaugeEntryExArray;
UINT32 Index;
+ EFI_STATUS Status;
+
+ Status = EfiAcquireLockOrFail (&mPerfRecordLock);
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
if (TimeStamp == 0) {
TimeStamp = GetPerformanceCounter ();
@@ -257,11 +277,13 @@ EndGaugeEx (
Index = InternalSearchForGaugeEntry (Handle, Token, Module, Identifier);
if (Index >= mGaugeData->NumberOfEntries) {
+ EfiReleaseLock (&mPerfRecordLock);
return EFI_NOT_FOUND;
}
GaugeEntryExArray = (GAUGE_DATA_ENTRY_EX *) (mGaugeData + 1);
GaugeEntryExArray[Index].EndTimeStamp = TimeStamp;
+ EfiReleaseLock (&mPerfRecordLock);
return EFI_SUCCESS;
}
@@ -274,6 +296,8 @@ EndGaugeEx (
If it stands for a valid entry, then EFI_SUCCESS is returned and
GaugeDataEntryEx stores the pointer to that entry.
+ This internal function is added to avoid releasing lock before each return statement.
+
@param LogEntryKey The key for the previous performance measurement log entry.
If 0, then the first performance measurement log entry is retrieved.
@param GaugeDataEntryEx The indirect pointer to the extended gauge data entry specified by LogEntryKey
@@ -287,7 +311,7 @@ EndGaugeEx (
**/
EFI_STATUS
EFIAPI
-GetGaugeEx (
+InternalGetGaugeEx (
IN UINTN LogEntryKey,
OUT GAUGE_DATA_ENTRY_EX **GaugeDataEntryEx
)
@@ -314,6 +338,47 @@ GetGaugeEx (
}
/**
+ Retrieves a previously logged performance measurement.
+ It can also retrieve the log created by StartGauge and EndGauge of PERFORMANCE_PROTOCOL,
+ and then assign the Identifier with 0.
+
+ Retrieves the performance log entry from the performance log specified by LogEntryKey.
+ If it stands for a valid entry, then EFI_SUCCESS is returned and
+ GaugeDataEntryEx stores the pointer to that entry.
+
+ @param LogEntryKey The key for the previous performance measurement log entry.
+ If 0, then the first performance measurement log entry is retrieved.
+ @param GaugeDataEntryEx The indirect pointer to the extended gauge data entry specified by LogEntryKey
+ if the retrieval is successful.
+
+ @retval EFI_SUCCESS The GuageDataEntryEx is successfully found based on LogEntryKey.
+ @retval EFI_NOT_FOUND The LogEntryKey is the last entry (equals to the total entry number).
+ @retval EFI_INVALIDE_PARAMETER The LogEntryKey is not a valid entry (greater than the total entry number).
+ @retval EFI_INVALIDE_PARAMETER GaugeDataEntryEx is NULL.
+
+**/
+EFI_STATUS
+EFIAPI
+GetGaugeEx (
+ IN UINTN LogEntryKey,
+ OUT GAUGE_DATA_ENTRY_EX **GaugeDataEntryEx
+ )
+{
+ EFI_STATUS Status;
+
+ Status = EfiAcquireLockOrFail (&mPerfRecordLock);
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
+ Status = InternalGetGaugeEx (LogEntryKey, GaugeDataEntryEx);
+
+ EfiReleaseLock (&mPerfRecordLock);
+
+ return Status;
+}
+
+/**
Adds a record at the end of the performance measurement log
that records the start time of a performance measurement.
--
2.7.4
