Date   

Re: [PATCH] MdeModulePkg/SetupBrowserDxe: Fix IsZeroGuid() ASSERT.

Liming Gao
 

Nickle:
Is this a real issue found in production?

Thanks
Liming

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Dandan Bi
Sent: Thursday, February 20, 2020 4:24 PM
To: devel@edk2.groups.io; nickle.wang@...
Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/SetupBrowserDxe: Fix IsZeroGuid() ASSERT.

Thanks Nickle for the fix.
One minor comment is that please pay attention to the format of commit message.
Refer to https://github.com/tianocore/tianocore.github.io/wiki/Commit-Message-Format, Line length of commit message should be less
than 76 characters when possible.
Please address it when submit the patch.
Reviewed-by: Dandan Bi <dandan.bi@...>


Thanks,
Dandan

-----Original Message-----
From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
Nickle Wang
Sent: Wednesday, February 19, 2020 10:23 PM
To: devel@edk2.groups.io; nickle.wang@...
Cc: Bi, Dandan <dandan.bi@...>
Subject: [edk2-devel] [PATCH] MdeModulePkg/SetupBrowserDxe: Fix
IsZeroGuid() ASSERT.

From the function description of GetIfrBinaryData(), FormSetGuid can be
NULL. However, FormSetGuid is passed to IsZeroGuid(). This causes
exception when FormSetGuid is NULL.

Signed-off-by: Nickle Wang <nickle.wang@...>
---
MdeModulePkg/Universal/SetupBrowserDxe/Setup.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Universal/SetupBrowserDxe/Setup.c
b/MdeModulePkg/Universal/SetupBrowserDxe/Setup.c
index 288f1c3197..82067b541c 100644
--- a/MdeModulePkg/Universal/SetupBrowserDxe/Setup.c
+++ b/MdeModulePkg/Universal/SetupBrowserDxe/Setup.c
@@ -2,6 +2,7 @@
Entry and initialization module for the browser.

Copyright (c) 2007 - 2018, Intel Corporation. All rights reserved.<BR>
+(C) Copyright 2020 Hewlett Packard Enterprise Development LP<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent

**/
@@ -5844,7 +5845,7 @@ GetIfrBinaryData (
//
// Try to compare against formset GUID
//
- if (IsZeroGuid (FormSetGuid) ||
+ if (IsZeroGuid (ComparingGuid) ||
CompareGuid (ComparingGuid, (EFI_GUID *)(OpCodeData + sizeof
(EFI_IFR_OP_HEADER)))) {
break;
}
--
2.20.1.windows.1




Re: [PATCHv2 0/4] MdeModulePkg/SdMmcPciHcDxe: Refactor command processing

Albecki, Mateusz
 

Hi,

Github with test code: https://github.com/malbecki/edk2/tree/test_code_for_async

This test code was used as is on Platform2 and it worked if I have only scheduled one request. On platform 1 I had to rebuild it a couple of times to make it read instead of writing when testing after reboot.

Regarding the push - I am fine with this change making it to master after the stable tag.

Thanks,
Mateusz

-----Original Message-----
From: Wu, Hao A <hao.a.wu@...>
Sent: Thursday, February 20, 2020 1:40 AM
To: Albecki, Mateusz <mateusz.albecki@...>; devel@edk2.groups.io
Cc: Marcin Wojtas <mw@...>; Gao, Zhichao
<zhichao.gao@...>; Gao, Liming <liming.gao@...>
Subject: RE: [PATCHv2 0/4] MdeModulePkg/SdMmcPciHcDxe: Refactor
command processing

-----Original Message-----
From: Albecki, Mateusz
Sent: Thursday, February 20, 2020 12:05 AM
To: devel@edk2.groups.io
Cc: Albecki, Mateusz; Wu, Hao A; Marcin Wojtas; Gao, Zhichao; Gao,
Liming
Subject: [PATCHv2 0/4] MdeModulePkg/SdMmcPciHcDxe: Refactor
command
processing

This patch series aims to refactor command processing to achieve
following

- Trace the failing TRB packets to see what commands are failing and
for what reasons
- Get the response data even if data transfer timed out to allow
easier debugging
- Fix the PIO mode which is currently completely broken.

Changes in v2:
- Moved verbose packet prints after the command is finished to capture
the successfull command response
- Fixed the debug prints
- PIO data will be moved with width matching the alignment of the block
size.
For majority of transfers that means UINT32 width.

Tests performed:
- Each patch in the series has passed boot from eMMC with ADMAv3 data
transfer mode
- SDMA based boot has been tested with the full patch series
- PIO based boot has been tested with the full patch series
- PIO based data transfer has been additionally tested by creating and
modyfing a file in EFI shell
- Tested async PIO transfer - results below

Async test results:
I've tested a simple async write and then readback from the eMMMC
device using block IO v2. I have observed that while the requests are
processed correctly by the eMMC driver as soon as they finish platform
will sometimes mibehave. I've tested async without my changes and the
strange behavior reproduces. Right now I am suspecting there is some
problem either in the EDK2 core that performs async or in the platform
specific code. It is also possible that I coded the Async BlockIo
incorrectly although the test code was rather simple. Additionally I
was able to observe that on many eMMC controllers PIO based data
transfer is broken.
I was only able to find one platform
that supported PIO. Detailed observation below

Platform 1 (PIO working).
- Test code was able to perform async write to eMMC LBA 0
- As soon as the callback is called CPU exception happens
- After reboot test code was able to perform async read
- As soon as the callback is called CPU exception happens
- After reboot sync read was able to confirm that data matches what
was written by async write

Platform2 (PIO is returning trash data - all 0xAF)
- Test code was able to perform async write(although it didn't realyl
came through to the device)
- After write finished test code was able to perform async read(again
all data was 0xAF but the logic in the driver works)

Platform2 (again PIO is returning trash data)
- Test code scheduled 5 async writes. 2 writes finished and after that
CPU exception was signaled.

Platform3(also trash data from PIO)
- Test code scheduled one async write as soon as it was done platform
rebooted

I didn't want to spend any more time debugging this issue as I think
it would turn into the platform debug and what I observed gave me some
confidence that PIO in async is generally working.

Hello Mateusz,

Could you help to share your async test codes?
I can help to double confirm whether the issue you observed is related with
them or not.

Also, since edk2 repo is under soft freeze period for the next stable tag, I
would prefer for the series to get into the code base after the formal
announce of the stable tag (2020-02-28). I will still give the 'Reviewed-by'
after my review of the series though.

Do you have concern for this?

Best Regards,
Hao Wu



All tests were performed with eMMC in HS400 @200MHz clock frequency.

For easier review & integration patch has been pushed here:
Whole series:
https://github.com/malbecki/edk2/tree/emmc_transfer_refactor
Whole series + SDMA force code(test 3):
https://github.com/malbecki/edk2/tree/emmc_transfer_refactor_force_sd
ma
Whole series + PIO force code(test 4):
https://github.com/malbecki/edk2/tree/emmc_transfer_refactor_force_pio

Cc: Hao A Wu <hao.a.wu@...>
Cc: Marcin Wojtas <mw@...>
Cc: Zhichao Gao <zhichao.gao@...>
Cc: Liming Gao <liming.gao@...>

Mateusz Albecki (4):
MdeModulePkg/SdMmcPciHcDxe: Enhance driver traces
MdeModulePkg/SdMmcPciHcDxe: Read response on command
completion
MdeModulePkg/SdMmcPciHcDxe: Refactor data transfer completion
MdeModulePkg/SdMmcPciHcDxe: Fix PIO transfer mode

MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h | 4 +
MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c | 521
++++++++++++++++-----
2 files changed, 417 insertions(+), 108 deletions(-)

--
2.14.1.windows.1
--------------------------------------------------------------------

Intel Technology Poland sp. z o.o.
ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.

Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek
przegladanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by
others is strictly prohibited.


[PATCH edk2-platforms v2 1/1] Platform: Add SerialUartClockLib for all platforms

Pankaj Bansal <pankaj.bansal@...>
 

From: Pankaj Bansal <pankaj.bansal@...>

BaseSerialPortLib16550 has been modified to use SerialUartClockLib.
Therefore, add the default implementation of SerialUartClockLib for
all the platforms using BaseSerialPortLib16550.

Signed-off-by: Pankaj Bansal <pankaj.bansal@...>
---

Notes:
V2:
- removed RaspberryPi3 and RaspberryPi4 platforms from patch
as these platforms no longer use BaseSerialPortLib16550.inf

Platform/Intel/AdvancedFeaturePkg/AdvancedFeaturePkg.dsc | 1 +
Platform/Intel/MinPlatformPkg/Include/Dsc/CoreCommonLib.dsc | 1 +
Platform/Intel/QuarkPlatformPkg/Quark.dsc | 1 +
Platform/Intel/QuarkPlatformPkg/QuarkMin.dsc | 1 +
Platform/Intel/Vlv2TbltDevicePkg/PlatformPkgIA32.dsc | 6 ++++++
Platform/Intel/Vlv2TbltDevicePkg/PlatformPkgX64.dsc | 6 ++++++
Platform/Socionext/DeveloperBox/DeveloperBox.dsc | 1 +
Platform/Socionext/DeveloperBox/DeveloperBoxMm.dsc | 1 +
Silicon/Intel/QuarkSocPkg/QuarkSocPkg.dsc | 1 +
Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc | 1 +
10 files changed, 20 insertions(+)

diff --git a/Platform/Intel/AdvancedFeaturePkg/AdvancedFeaturePkg.dsc b/Platform/Intel/AdvancedFeaturePkg/AdvancedFeaturePkg.dsc
index b83e72b48c47..90cd72aff0d5 100644
--- a/Platform/Intel/AdvancedFeaturePkg/AdvancedFeaturePkg.dsc
+++ b/Platform/Intel/AdvancedFeaturePkg/AdvancedFeaturePkg.dsc
@@ -74,6 +74,7 @@ [LibraryClasses.common]
PeiServicesTablePointerLib|MdePkg/Library/PeiServicesTablePointerLibIdt/PeiServicesTablePointerLibIdt.inf
PrintLib|MdePkg/Library/BasePrintLib/BasePrintLib.inf
ReportStatusCodeLib|MdePkg/Library/BaseReportStatusCodeLibNull/BaseReportStatusCodeLibNull.inf
+ SerialUartClockLib|MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.inf
SerialPortLib|MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf
SortLib|MdeModulePkg/Library/BaseSortLib/BaseSortLib.inf
TimerLib|MdePkg/Library/BaseTimerLibNullTemplate/BaseTimerLibNullTemplate.inf
diff --git a/Platform/Intel/MinPlatformPkg/Include/Dsc/CoreCommonLib.dsc b/Platform/Intel/MinPlatformPkg/Include/Dsc/CoreCommonLib.dsc
index 23ab53dbfe12..172ba6822419 100644
--- a/Platform/Intel/MinPlatformPkg/Include/Dsc/CoreCommonLib.dsc
+++ b/Platform/Intel/MinPlatformPkg/Include/Dsc/CoreCommonLib.dsc
@@ -20,6 +20,7 @@ [LibraryClasses.common]
DxeCoreEntryPoint|MdePkg/Library/DxeCoreEntryPoint/DxeCoreEntryPoint.inf
UefiDriverEntryPoint|MdePkg/Library/UefiDriverEntryPoint/UefiDriverEntryPoint.inf
UefiApplicationEntryPoint|MdePkg/Library/UefiApplicationEntryPoint/UefiApplicationEntryPoint.inf
+ SerialUartClockLib|MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.inf
SerialPortLib|MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf
PeCoffExtraActionLib|MdePkg/Library/BasePeCoffExtraActionLibNull/BasePeCoffExtraActionLibNull.inf

diff --git a/Platform/Intel/QuarkPlatformPkg/Quark.dsc b/Platform/Intel/QuarkPlatformPkg/Quark.dsc
index a02adb64e695..b275298a5ddb 100644
--- a/Platform/Intel/QuarkPlatformPkg/Quark.dsc
+++ b/Platform/Intel/QuarkPlatformPkg/Quark.dsc
@@ -213,6 +213,7 @@ [LibraryClasses]
#
IohLib|QuarkSocPkg/QuarkSouthCluster/Library/IohLib/IohLib.inf
I2cLib|QuarkSocPkg/QuarkSouthCluster/Library/I2cLib/I2cLib.inf
+ SerialUartClockLib|MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.inf
SerialPortLib|MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf
PlatformHookLib|MdeModulePkg/Library/BasePlatformHookLibNull/BasePlatformHookLibNull.inf

diff --git a/Platform/Intel/QuarkPlatformPkg/QuarkMin.dsc b/Platform/Intel/QuarkPlatformPkg/QuarkMin.dsc
index 3dbf616c664d..cffd777a6721 100644
--- a/Platform/Intel/QuarkPlatformPkg/QuarkMin.dsc
+++ b/Platform/Intel/QuarkPlatformPkg/QuarkMin.dsc
@@ -185,6 +185,7 @@ [LibraryClasses]
#
IohLib|QuarkSocPkg/QuarkSouthCluster/Library/IohLib/IohLib.inf
I2cLib|QuarkSocPkg/QuarkSouthCluster/Library/I2cLib/I2cLib.inf
+ SerialUartClockLib|MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.inf
SerialPortLib|MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf
PlatformHookLib|MdeModulePkg/Library/BasePlatformHookLibNull/BasePlatformHookLibNull.inf

diff --git a/Platform/Intel/Vlv2TbltDevicePkg/PlatformPkgIA32.dsc b/Platform/Intel/Vlv2TbltDevicePkg/PlatformPkgIA32.dsc
index 463b952e6582..91b2754ea509 100644
--- a/Platform/Intel/Vlv2TbltDevicePkg/PlatformPkgIA32.dsc
+++ b/Platform/Intel/Vlv2TbltDevicePkg/PlatformPkgIA32.dsc
@@ -184,6 +184,7 @@ [LibraryClasses.common]
SerialPortLib|MdePkg/Library/BaseSerialPortLibNull/BaseSerialPortLibNull.inf
!else
DebugLib|MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf
+ SerialUartClockLib|MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.inf
SerialPortLib|MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf
!endif

@@ -200,6 +201,7 @@ [LibraryClasses.common]
!if $(SOURCE_DEBUG_ENABLE) == TRUE
PeCoffExtraActionLib|SourceLevelDebugPkg/Library/PeCoffExtraActionLibDebug/PeCoffExtraActionLibDebug.inf
DebugCommunicationLib|SourceLevelDebugPkg/Library/DebugCommunicationLibSerialPort/DebugCommunicationLibSerialPort.inf
+ SerialUartClockLib|MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.inf
SerialPortLib|MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf
!else
PeCoffExtraActionLib|MdePkg/Library/BasePeCoffExtraActionLibNull/BasePeCoffExtraActionLibNull.inf
@@ -280,6 +282,7 @@ [LibraryClasses.IA32.PEIM, LibraryClasses.IA32.PEI_CORE, LibraryClasses.IA32.SEC
SerialPortLib|MdePkg/Library/BaseSerialPortLibNull/BaseSerialPortLibNull.inf
!else
DebugLib|MdeModulePkg/Library/PeiDxeDebugLibReportStatusCode/PeiDxeDebugLibReportStatusCode.inf
+ SerialUartClockLib|MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.inf
SerialPortLib|MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf
!endif

@@ -779,6 +782,7 @@ [Components.IA32]
PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
DebugAgentLib|SourceLevelDebugPkg/Library/DebugAgent/SecPeiDebugAgentLib.inf
PlatformHookLib|MdeModulePkg/Library/BasePlatformHookLibNull/BasePlatformHookLibNull.inf
+ SerialUartClockLib|MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.inf
SerialPortLib|MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf
}
!endif
@@ -933,6 +937,7 @@ [Components.IA32]
MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf {
<LibraryClasses>
NULL|MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLib.inf
+ SerialUartClockLib|MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.inf
SerialPortLib|MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf
}
Vlv2TbltDevicePkg/FvbRuntimeDxe/FvbSmm.inf
@@ -1223,6 +1228,7 @@ [Components.IA32]
<LibraryClasses>
DebugLib|MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf
PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
+ SerialUartClockLib|MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.inf
SerialPortLib|MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf
}
!endif
diff --git a/Platform/Intel/Vlv2TbltDevicePkg/PlatformPkgX64.dsc b/Platform/Intel/Vlv2TbltDevicePkg/PlatformPkgX64.dsc
index ee18b45c9712..749a2dd4948b 100644
--- a/Platform/Intel/Vlv2TbltDevicePkg/PlatformPkgX64.dsc
+++ b/Platform/Intel/Vlv2TbltDevicePkg/PlatformPkgX64.dsc
@@ -186,6 +186,7 @@ [LibraryClasses.common]
SerialPortLib|MdePkg/Library/BaseSerialPortLibNull/BaseSerialPortLibNull.inf
!else
DebugLib|MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf
+ SerialUartClockLib|MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.inf
SerialPortLib|MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf
!endif

@@ -202,6 +203,7 @@ [LibraryClasses.common]
!if $(SOURCE_DEBUG_ENABLE) == TRUE
PeCoffExtraActionLib|SourceLevelDebugPkg/Library/PeCoffExtraActionLibDebug/PeCoffExtraActionLibDebug.inf
DebugCommunicationLib|SourceLevelDebugPkg/Library/DebugCommunicationLibSerialPort/DebugCommunicationLibSerialPort.inf
+ SerialUartClockLib|MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.inf
SerialPortLib|MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf
!else
PeCoffExtraActionLib|MdePkg/Library/BasePeCoffExtraActionLibNull/BasePeCoffExtraActionLibNull.inf
@@ -282,6 +284,7 @@ [LibraryClasses.IA32.PEIM, LibraryClasses.IA32.PEI_CORE, LibraryClasses.IA32.SEC
SerialPortLib|MdePkg/Library/BaseSerialPortLibNull/BaseSerialPortLibNull.inf
!else
DebugLib|MdeModulePkg/Library/PeiDxeDebugLibReportStatusCode/PeiDxeDebugLibReportStatusCode.inf
+ SerialUartClockLib|MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.inf
SerialPortLib|MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf
!endif

@@ -781,6 +784,7 @@ [Components.IA32]
PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
DebugAgentLib|SourceLevelDebugPkg/Library/DebugAgent/SecPeiDebugAgentLib.inf
PlatformHookLib|MdeModulePkg/Library/BasePlatformHookLibNull/BasePlatformHookLibNull.inf
+ SerialUartClockLib|MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.inf
SerialPortLib|MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf
}
!endif
@@ -948,6 +952,7 @@ [Components.X64]
MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf {
<LibraryClasses>
NULL|MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLib.inf
+ SerialUartClockLib|MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.inf
SerialPortLib|MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf
}
Vlv2TbltDevicePkg/FvbRuntimeDxe/FvbSmm.inf
@@ -1238,6 +1243,7 @@ [Components.X64]
<LibraryClasses>
DebugLib|MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf
PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
+ SerialUartClockLib|MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.inf
SerialPortLib|MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf
}
!endif
diff --git a/Platform/Socionext/DeveloperBox/DeveloperBox.dsc b/Platform/Socionext/DeveloperBox/DeveloperBox.dsc
index 9f8cb68cdd26..281de3197d0c 100644
--- a/Platform/Socionext/DeveloperBox/DeveloperBox.dsc
+++ b/Platform/Socionext/DeveloperBox/DeveloperBox.dsc
@@ -32,6 +32,7 @@ [LibraryClasses]
!if $(DEBUG_ON_UART1) == FALSE
SerialPortLib|ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortLib.inf
!else
+ SerialUartClockLib|MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.inf
SerialPortLib|MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf
PlatformHookLib|MdeModulePkg/Library/BasePlatformHookLibNull/BasePlatformHookLibNull.inf
PciLib|MdePkg/Library/BasePciLibPciExpress/BasePciLibPciExpress.inf
diff --git a/Platform/Socionext/DeveloperBox/DeveloperBoxMm.dsc b/Platform/Socionext/DeveloperBox/DeveloperBoxMm.dsc
index 00f6d4496377..ef3ed0f20911 100644
--- a/Platform/Socionext/DeveloperBox/DeveloperBoxMm.dsc
+++ b/Platform/Socionext/DeveloperBox/DeveloperBoxMm.dsc
@@ -35,6 +35,7 @@ [LibraryClasses]
StandaloneMmDriverEntryPoint|MdePkg/Library/StandaloneMmDriverEntryPoint/StandaloneMmDriverEntryPoint.inf
StandaloneMmMmuLib|ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.inf

+ SerialUartClockLib|MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.inf
SerialPortLib|MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf
PlatformHookLib|MdeModulePkg/Library/BasePlatformHookLibNull/BasePlatformHookLibNull.inf
PciLib|MdePkg/Library/BasePciLibPciExpress/BasePciLibPciExpress.inf
diff --git a/Silicon/Intel/QuarkSocPkg/QuarkSocPkg.dsc b/Silicon/Intel/QuarkSocPkg/QuarkSocPkg.dsc
index e743a5e272e4..2e4bf8c1259c 100644
--- a/Silicon/Intel/QuarkSocPkg/QuarkSocPkg.dsc
+++ b/Silicon/Intel/QuarkSocPkg/QuarkSocPkg.dsc
@@ -105,6 +105,7 @@ [LibraryClasses]
# Quark South Cluster
#
IohLib|QuarkSocPkg/QuarkSouthCluster/Library/IohLib/IohLib.inf
+ SerialUartClockLib|MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.inf
SerialPortLib|MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf
PlatformHookLib|MdeModulePkg/Library/BasePlatformHookLibNull/BasePlatformHookLibNull.inf
#
diff --git a/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc b/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc
index 42f8bbba926c..4cf6429d7476 100644
--- a/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc
+++ b/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc
@@ -87,6 +87,7 @@ [LibraryClasses.common]
UefiScsiLib|MdePkg/Library/UefiScsiLib/UefiScsiLib.inf

# Serial port libraries
+ SerialUartClockLib|MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.inf
SerialPortLib|MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf
PlatformHookLib|MdeModulePkg/Library/BasePlatformHookLibNull/BasePlatformHookLibNull.inf

--
2.17.1


Re: [EXT] RE: [PATCH 1/1] MdeModulePkg/Pci: Fixed Asserts in SCT PCIIO Protocol Test.

Gaurav Jain
 

-----Original Message-----
From: Wu, Hao A <hao.a.wu@...>
Sent: Wednesday, February 19, 2020 12:04 PM
To: Gaurav Jain <gaurav.jain@...>; devel@edk2.groups.io; Ard Biesheuvel
<ard.biesheuvel@...>
Cc: Wang, Jian J <jian.j.wang@...>; Ni, Ray <ray.ni@...>; Pankaj
Bansal <pankaj.bansal@...>
Subject: [EXT] RE: [PATCH 1/1] MdeModulePkg/Pci: Fixed Asserts in SCT PCIIO
Protocol Test.

Caution: EXT Email

-----Original Message-----
From: Gaurav Jain [mailto:gaurav.jain@...]
Sent: Thursday, January 30, 2020 4:18 PM
To: devel@edk2.groups.io
Cc: Wang, Jian J; Wu, Hao A; Ni, Ray; Pankaj Bansal; Gaurav Jain
Subject: [PATCH 1/1] MdeModulePkg/Pci: Fixed Asserts in SCT PCIIO
Protocol Test.

ASSERT in CopyMem_Conf, PollMem_Conf, SetBarAttributes_Conf
Conformance Test.
SCT Test expect return as Invalid Parameter.
So removed ASSERT().

Include Ard (as the driver contributor) to see if he has additional comments.

A couple of general level comments:
1. I think the ASSERT can still be added after the checks you add. The ASSERT
may serve as a notification in the DEBUG image that the service is not
implemented.

2. I found that for functions:
PciIoPollIo()
PciIoIoRead()
PciIoIoWrite()
They also do not have checks that perfectly match with the UEFI spec. Even
though they are not exposed by the SCT tests, could you help to address them
as well?

Some other inline comments below.
Done the suggested changes and sent v2.
Please review.



Signed-off-by: Gaurav Jain <gaurav.jain@...>
---
.../NonDiscoverablePciDeviceIo.c | 20 ++++++++++++++++---
1 file changed, 17 insertions(+), 3 deletions(-)

diff --git
a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverableP
ciDeviceIo.c
b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverableP
ciDeviceIo.c
index 2d55c9699322..76cb000602fc 100644
---
a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverableP
ciDeviceIo.c
+++
b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverableP
ciDeviceIo.c
@@ -93,7 +93,15 @@ PciIoPollMem (
OUT UINT64 *Result
)
{
- ASSERT (FALSE);
+ if ((UINT32)Width >= EfiPciIoWidthMaximum ||

Looks to me that the 1st part of the 'if' check is redundant.
Could you help to double confirm?


+ Width > EfiPciIoWidthUint64) {
+ return EFI_INVALID_PARAMETER;
+ }
+
+ if (Result == NULL) {
+ return EFI_INVALID_PARAMETER;
+ }
+
return EFI_UNSUPPORTED;
}

@@ -556,7 +564,10 @@ PciIoCopyMem (
IN UINTN Count
)
{
- ASSERT (FALSE);
+ if ((UINT32)Width >= EfiPciIoWidthMaximum ||

Looks to me that the 1st part of the 'if' check is redundant.
Could you help to double confirm?

Best Regards,
Hao Wu


+ Width > EfiPciIoWidthUint64) {
+ return EFI_INVALID_PARAMETER;
+ }
return EFI_UNSUPPORTED;
}

@@ -1414,7 +1425,10 @@ PciIoSetBarAttributes (
IN OUT UINT64 *Length
)
{
- ASSERT (FALSE);
+ if (Offset == NULL || Length == NULL) {
+ return EFI_INVALID_PARAMETER;
+ }
+
return EFI_UNSUPPORTED;
}

--
2.17.1


[PATCH v2 1/1] MdeModulePkg/Pci: Fixed Asserts in SCT PCIIO Protocol Test.

Gaurav Jain
 

ASSERT in PollMem_Conf, CopyMem_Conf, SetBarAttributes_Conf
Conformance Test.
SCT Test expect return as Invalid Parameter or Unsupported.
Added Checks for Function Parameters.
return Invalid or Unsupported if Check fails.

Added Checks in PciIoPollIo(), PciIoIoRead()
PciIoIoWrite()

Signed-off-by: Gaurav Jain <gaurav.jain@...>
---

Notes:
v2
- Reverted ASSERT(FALSE) code.
- Added Checks for Width, BarIndex, Buffer,
Address range in PciIoIoRead, PciIoIoWrite.
- Added Checks for Width, BarIndex, Result,
Address range in PciIoPollIo, PciIoPollMem,
PciIoCopyMem.
- Added Checks for Attributes, BarIndex,
Address range in PciIoSetBarAttributes.

.../NonDiscoverablePciDeviceIo.c | 180 ++++++++++++++++++
1 file changed, 180 insertions(+)

diff --git a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceIo.c b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceIo.c
index 2d55c9699322..4dd804356021 100644
--- a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceIo.c
+++ b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceIo.c
@@ -93,6 +93,35 @@ PciIoPollMem (
OUT UINT64 *Result
)
{
+ NON_DISCOVERABLE_PCI_DEVICE *Dev;
+ EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *Desc;
+ UINTN Count;
+ EFI_STATUS Status;
+
+ if ((UINT32)Width > EfiPciIoWidthUint64) {
+ return EFI_INVALID_PARAMETER;
+ }
+
+ if (BarIndex >= PCI_MAX_BAR) {
+ return EFI_UNSUPPORTED;
+ }
+
+ if (Result == NULL) {
+ return EFI_INVALID_PARAMETER;
+ }
+
+ Dev = NON_DISCOVERABLE_PCI_DEVICE_FROM_PCI_IO(This);
+ Count = 1;
+
+ Status = GetBarResource (Dev, BarIndex, &Desc);
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
+ if (Offset + (Count << (Width & 0x3)) > Desc->AddrLen) {
+ return EFI_UNSUPPORTED;
+ }
+
ASSERT (FALSE);
return EFI_UNSUPPORTED;
}
@@ -126,6 +155,35 @@ PciIoPollIo (
OUT UINT64 *Result
)
{
+ NON_DISCOVERABLE_PCI_DEVICE *Dev;
+ EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *Desc;
+ UINTN Count;
+ EFI_STATUS Status;
+
+ if ((UINT32)Width > EfiPciIoWidthUint64) {
+ return EFI_INVALID_PARAMETER;
+ }
+
+ if (BarIndex >= PCI_MAX_BAR) {
+ return EFI_UNSUPPORTED;
+ }
+
+ if (Result == NULL) {
+ return EFI_INVALID_PARAMETER;
+ }
+
+ Dev = NON_DISCOVERABLE_PCI_DEVICE_FROM_PCI_IO(This);
+ Count = 1;
+
+ Status = GetBarResource (Dev, BarIndex, &Desc);
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
+ if (Offset + (Count << (Width & 0x3)) > Desc->AddrLen) {
+ return EFI_UNSUPPORTED;
+ }
+
ASSERT (FALSE);
return EFI_UNSUPPORTED;
}
@@ -396,6 +454,33 @@ PciIoIoRead (
IN OUT VOID *Buffer
)
{
+ NON_DISCOVERABLE_PCI_DEVICE *Dev;
+ EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *Desc;
+ EFI_STATUS Status;
+
+ if ((UINT32)Width > EfiPciIoWidthUint64) {
+ return EFI_INVALID_PARAMETER;
+ }
+
+ if (BarIndex >= PCI_MAX_BAR) {
+ return EFI_UNSUPPORTED;
+ }
+
+ if (Buffer == NULL) {
+ return EFI_INVALID_PARAMETER;
+ }
+
+ Dev = NON_DISCOVERABLE_PCI_DEVICE_FROM_PCI_IO(This);
+
+ Status = GetBarResource (Dev, BarIndex, &Desc);
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
+ if (Offset + (Count << (Width & 0x3)) > Desc->AddrLen) {
+ return EFI_UNSUPPORTED;
+ }
+
ASSERT (FALSE);
return EFI_UNSUPPORTED;
}
@@ -425,6 +510,33 @@ PciIoIoWrite (
IN OUT VOID *Buffer
)
{
+ NON_DISCOVERABLE_PCI_DEVICE *Dev;
+ EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *Desc;
+ EFI_STATUS Status;
+
+ if ((UINT32)Width > EfiPciIoWidthUint64) {
+ return EFI_INVALID_PARAMETER;
+ }
+
+ if (BarIndex >= PCI_MAX_BAR) {
+ return EFI_UNSUPPORTED;
+ }
+
+ if (Buffer == NULL) {
+ return EFI_INVALID_PARAMETER;
+ }
+
+ Dev = NON_DISCOVERABLE_PCI_DEVICE_FROM_PCI_IO(This);
+
+ Status = GetBarResource (Dev, BarIndex, &Desc);
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
+ if (Offset + (Count << (Width & 0x3)) > Desc->AddrLen) {
+ return EFI_UNSUPPORTED;
+ }
+
ASSERT (FALSE);
return EFI_UNSUPPORTED;
}
@@ -556,6 +668,40 @@ PciIoCopyMem (
IN UINTN Count
)
{
+ NON_DISCOVERABLE_PCI_DEVICE *Dev;
+ EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *DestDesc;
+ EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *SrcDesc;
+ EFI_STATUS Status;
+
+ if ((UINT32)Width > EfiPciIoWidthUint64) {
+ return EFI_INVALID_PARAMETER;
+ }
+
+ if (DestBarIndex >= PCI_MAX_BAR ||
+ SrcBarIndex >= PCI_MAX_BAR) {
+ return EFI_UNSUPPORTED;
+ }
+
+ Dev = NON_DISCOVERABLE_PCI_DEVICE_FROM_PCI_IO(This);
+
+ Status = GetBarResource (Dev, DestBarIndex, &DestDesc);
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
+ if (DestOffset + (Count << (Width & 0x3)) > DestDesc->AddrLen) {
+ return EFI_UNSUPPORTED;
+ }
+
+ Status = GetBarResource (Dev, SrcBarIndex, &SrcDesc);
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
+ if (SrcOffset + (Count << (Width & 0x3)) > SrcDesc->AddrLen) {
+ return EFI_UNSUPPORTED;
+ }
+
ASSERT (FALSE);
return EFI_UNSUPPORTED;
}
@@ -1414,6 +1560,40 @@ PciIoSetBarAttributes (
IN OUT UINT64 *Length
)
{
+ NON_DISCOVERABLE_PCI_DEVICE *Dev;
+ EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *Desc;
+ EFI_PCI_IO_PROTOCOL_WIDTH Width;
+ UINTN Count;
+ EFI_STATUS Status;
+
+ #define DEV_SUPPORTED_ATTRIBUTES \
+ (EFI_PCI_DEVICE_ENABLE | EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE)
+
+ if ((Attributes & (~DEV_SUPPORTED_ATTRIBUTES)) != 0) {
+ return EFI_UNSUPPORTED;
+ }
+
+ if (BarIndex >= PCI_MAX_BAR) {
+ return EFI_UNSUPPORTED;
+ }
+
+ if (Offset == NULL || Length == NULL) {
+ return EFI_INVALID_PARAMETER;
+ }
+
+ Dev = NON_DISCOVERABLE_PCI_DEVICE_FROM_PCI_IO(This);
+ Width = EfiPciIoWidthUint8;
+ Count = (UINT32) *Length;
+
+ Status = GetBarResource(Dev, BarIndex, &Desc);
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
+ if (*Offset + (Count << (Width & 0x3)) > Desc->AddrLen) {
+ return EFI_UNSUPPORTED;
+ }
+
ASSERT (FALSE);
return EFI_UNSUPPORTED;
}
--
2.17.1


Re: [PATCH v3 0/1] Add PCD to disable safe string constraint assertions

Marvin Häuser <mhaeuser@...>
 

Hey Andrew,

Thanks once again for your comments, mine are inline.

Best regards,
Marvin

Am 20.02.2020 um 00:55 schrieb Andrew Fish:


On Feb 17, 2020, at 12:26 AM, Marvin Häuser <mhaeuser@...
<mailto:mhaeuser@...>> wrote:

Good day Andrew,

First of all, thank you very much for putting this amount of thought
into the situation. I definitely agree with the problem you see, but I
could also live with Vitaly's proposal. However, I think you are
overcomplicating the situation a little. So, we do agree the caller
needs control of the behaviour, however your proposal needs "support"
from both the caller (choose the handler) and the callee (call the
handler) and is neither thread-safe nor event-safe as Vitaly already
pointed out. Fixing these problems would only worsen the complexity
situation in my opinion.
Well EFI does not have Threads but it does have Events. I agree if an
Sorry, we were refering to the UEFI Multi-Processor services, so threads
in a physical sense. I think very similar concerns apply regarding these
services as with software multithreading in this context, please correct
me if I'm wrong.

Event handler changes the constraint handler it would need to restore it
so my proposal is missing an API.

CONSTRAINT_HANDLER *
GetConstraintHandler (
  VOID
  )
{
  return gActiveConstraintHandler;
}

You an always use the standard EFI

It is important to remember that all the EFI images
(drivers/applications) are statically linked and they carry a unique
instance of the Debug (or new Constraint) lib. So in your driver or App
is very self contained. Also a lot of the events are phase related
callbacks, and since there are no threads your drivers main thread is
only really running when your driver, or app, dispatches. A lot of the
callbacks are via protocols your driver publishes, but they have strict
TPL rules so you can prevent reentrancy in general. So there is a lot
more determinism in EFI vs. generic threaded coding.
This is true, but yet all edge-cases should be covered (event
"interruption", actual (processor) interrupts, Multi-Processor
execution), so developers do not run into unexpected (seemingly)
undeterministic behaviour. However, I agree it is easier to ensure for
something like UEFI than for a full-fledged OS of course.



Diverging from both of your concepts entirely and keeping it very
simple, what keeps us from simply ASSERTing based on return value? We
could introduce a CONSTRAINT_ASSERT or similar, but it would be
completely different from Vitaly's proposal. It would be a caller macro
to ASSERT based on a pre-defined list of return statuses.
To achieve this, we would order all statuses by types (classes). At the
moment, I can only think of two: "environmental" and "constraint". For
example, "Out Of Resources" would be environmental and "Invalid
Parameter" would be constraint. We could define environment statuses
explicitly (as it is less likely new environment statuses are
introduced) and define constraint statuses as "not environmental" (to
silently support new constraint statuses, however of course it is a
little error-prone with forwards-compatibility). As a bonus, it forces
developers to use truly appropiate error codes and correctly propagate
them from callees for this to work. :)
This solution would be backwards-compatible in a compilation sense as it
can simply be a new macro, however for its possible complexity (switch
cases) I might actually prefer a function. But it would not be
backwards-compatible with the current functionality, which none of the
"caller-based" concepts can be anyway (the caller needs explicit code).
It is an interesting idea to add granularity, but at the end of the
knowledge of the correct behavior of the lower level library code really
lives in code that calls this. I will admit I can see some value to
making RETURN_INVALID_PARAMETER different than RETURN_BUFFER_TOO_SMALL.
So far, I prefer this idea as the only requirement is that function
contracts are well-designed (i.e. reflect environmental vs constraint
errors by using sensical return values).


I kind of went down the C11 path (thanks for mentioning the event issue)
but there are other ways to empower the caller.
It's definitely always a good idea to stick to standards and
conventions. It appears my proposal is not conventional at all. However,
I also believe there sometimes is a more pragmatic approach to things
without imposing significant disadvantages, especially by limiting the
scope.

To be honest, I'm now interested for which cases the C11 path is
advantagous over my approach, except that setting the constraint handler
is a one-time thing if it remains consistent throughout execution. I'd
imagine for some more complex handling logic I cannot think of an
example of (all we really want to do is ASSERT conditionally afterall),
and I hope this is not something edk2 would ever run into - keep it
simple in my opinion.


We could let the caller decide the behavior. That implies passing
CHECK_DEFAULT (PCD), CHECK_ASSERT, or CHECK_NULL.

Then keep backward compatibility.
#define StrCpyS(Destination,DestMax,Source)StrCpuSC(Destination,
DestMax, Source, CHECK_DEFAULT)

But that seems like a lot of thrash to the BaseLib and a lot of new
magic to teach developers.
Fully agreed with the last sentence. That was one of the approaches I
mentioned before my proposal, but I included it only for completeness'
sake so we have a full overview. This requires prototype adaption for
literally every function that may be used in such a way (which scopes
beyond just BaseLib), ugly macros and more. Please don't do that. :)


I would guess that the most common usage of my library would be to turn
Constraint Checking off in the entire driver or app and then handle the
errors manually.
I'm afraid that's what most platforms will end up with. Our main concern
was we may not allow such constraint violations to ASSERT, that is our
main objective. However, if it can be made advantageous to future coding
practice, it would be nice to have a decent solution for the caller to
have some freedom. More about that below.


On driver/app entry do:

SetConstraintHandler (IgnoreConstraintHandler);

Then handle the errors you care about.

Status = StrCpyS (Destination,DestMax,Source);
if (EFI_ERROR (Status)) {
ASSERT_EFI_ERROR (Status);
return Status;
}

or

Status = StrCpyS (Destination,DestMax,Source);
if (Status == RETURN_INVALID_PARAMETER) {
ASSERT_EFI_ERROR (Status);
return Status;
}

At the end of the day I've code my driver and I know the rules I coded
to and I want predictable behavior.
Yes, agreed. We want predictable behaviour of "not ASSERTing" for
untrusted input, because we *know* it is untrusted and the function
*can* (and should be able to) handle it - it is simply not a precondition.


I can see a Si vendor developing with ASSERT Constraint off and then
when the customer turns it on stuff starts randomly breaking.
A lot of Si and core (DxeCore, PeiCore, related libraries) code seems to
ASSERT (frequently *only* ASSERT with no actual handling, especially for
AllocatePool() == NULL) when there are valid error situations possible,
so I hope this approach would be a handy tool to satisfy their needs
without this absolutely terrible practice. :)



I think I understood all your comment individually, but I'm afraid I'm
not sure what your suggested solution is right now. You commented
semi-positively on my proposal, you did not revoke your C11 path, and
you furthermore introduced CHECK_* - this is brainstorming, and I think
it's a great thing, but I'm simply not sure what your exact goal or
prefered route is right now.

My proposal is a bit unconventional and I agree it sounds hacky, but so
far I'm not convinced it would be bad practice at all. It boils down to
sensefully using the information a function returns. This requires
nothing but the function to return sensical information, which every
function should anyway.

Your proposal is close to the C standard, and that is a good thing first
of all. However, with the rest of edk2 not being very compliant, I don't
think it's an instant win as in "we just comply to standards" because
this choice does not suddenly make porting existing code significantly
easier compared to the total lack of standard library support for
anything but Shell apps.
Pragmatically, I think it solves the same problem equally well, but at a
higher cost (ensuring the implementation is safe and, strictly speaking,
an ever-so-slight runtime overhead).


However, CONSTRAINT_ASSERT might actually be too specific. Maybe we want
something like "ASSERT_RETURN" and have the classes "ASSERT_CONSTRAINT"
and "ASSERT_ENVIRONMENT" (bitfield, like with DEBUG). The reason for
this is embedded systems where the environment is trusted/known and the
firmware is supposed to be well-matched. Think of a SoC with soldered
RAM - the firmware can very well make assumption about memory capacity
(considering usage across all modules in the worst case) and might
actually want ASSERTs on something like "Out Of Resources" because it's
supposed to be impossible within the bounds of this specific design.
It's possible this would need a new PCD's involvement, for example to
tell DxeCore whether to ASSERT on environmental aspects too. There could
be an argument-less macro that uses PCD config as base, and an
argument-based macro that uses only the parameters. Of course this
cannot cover everyone's very specific preferences as it's a per-module
switch, but it's the best I can think of right now.

Something a bit unrelated now, but it would make this easier. You
mentioned PeCoffLib as example, and I think it's a good one, however it
has its own return status type within the context[1] which I will most
definitely be getting rid of as part of my upcoming (in the far-ish
future :) ) RFC. Could we expand RETURN_STATUS to have (very high?)
reserved values for caller-defined error codes to have all RETURN_STATUS
macros apply to those special return values too? We'd need to define
them categorically as "constraint status", but I don't see how a library
would declare a new environment status anyway.

Regarding MdePkgCompatability.dsc.inc, I think one can override library
classes from the inc by declaring them after the !include statement,
please correct me if I'm wrong. If so, I strongly agree and think it
should be the case for all packages, so one only overrides the defaults
when something specific (debugging, performance-optimized versions, ...)
is required - easier to read, easier to maintain. The content is
needed/there anyway as the libraries are declared in the package's own
DSC.
Yes it the new .inc DSC file could be overridden by the platform DSC
that includes it.

I think this is a more general idea than something for this given patch.
It is something we could make sure we update every stable tag or so, but
I guess someone has to go 1st :). It would make it easier on platforms
when they update the edk2 version.
Agreed.





It would be nice if you had comments regarding every aspect I just
mentioned, it was just something coming to my mind this morning. Thanks
for the input so far, it's nice to see some movement now!
Sorry for the delay
Sure, thanks for taking time to respond!


Thanks,

Andrew Fish

Best regards,
Marvin

[1]
https://github.com/tianocore/edk2/blob/f1d78c489a39971b5aac5d2fc8a39bfa925c3c5d/MdePkg/Include/Library/PeCoffLib.h#L20-L31
https://github.com/tianocore/edk2/blob/f1d78c489a39971b5aac5d2fc8a39bfa925c3c5d/MdePkg/Include/Library/PeCoffLib.h#L159

Am 16.02.2020 um 22:25 schrieb Andrew Fish via Groups.Io:
Mike,

Sorry I don't think I totally understood what felt wrong to me, so I did
a bad job explaining my concerns. Also I don't think I was thinking
enough in the context of the C11 StdLib.

I think my concern is still the global scope of the constraint, even if
we tune it per module. For example the DXE Core can interact with
PE/COFF images and other data that could have indirectly come from the
disk. So conceptually you may want to ASSERT on some constraints and not
on others. I think that is why I ratholed on expanding the error
handling macros as that was more in the vein of having the caller deal
with it, so that is kind of like what you would do pre C11. Also the
DebugLib is probably one of the MdePkg Libs that has the most instances
floating around, so I think we should change it in a non backwards way
very carefully.

So after reading up on the C11 implementation of Constraints I think my
alternate proposal is for us to add a ConstraintLib modeled after C11
vs. updating the DebugLib. This would solve the 2 things that made me
uncomfortable: 1) Extending the DebugLib API; 2) Giving the caller
control of the ASSERT behavior. It would still have the down side of
breaking builds as the BaseLib would get a new dependency, so we could
talk about adding these functions to the DebugLib as the cost of
replicating code.

C11 defines constraint_handler_t and set_constraint_handler_s as a way
for the caller to configure the behavior for bounds checked functions. I
think that is the behavior I prefer. So if we are going to make a change
that impacts DebugLib compatibility I just want to make sure we have a
conversation about all the options. My primary goal is we have the
conversation, and if folks don't agree with me that is fine at least we
talked about it.

What I'm thinking about is as simply exposing an API to control the
Constraint handler like C11. This could be done via an ConstrainLib or
extending the DebugLib.

The basic implementation of the lib would look like:

typedef
VOID
(EFIAPI *CONSTRAINT_HANDLER) (
IN CONST CHAR8  *FileName,
IN UINTN                 LineNumber,
IN CONST CHAR8  *Description,
IN EFI_STATUS      Status
);


// Default to AssertConstraintHandler to make it easier to implement
Base and XIP libs.
// We could have a PCD that also sets the default handler in a Lib
Constructor. The default handler is implementation defined in C11.
CONSTRAINT_HANDLER gDefaultConstraintHandler = AssertConstraintHandler;
CONSTRAINT_HANDLER gActiveConstraintHandler = gDefaultConstraintHandler;

BOOLEAN
EFIAPI
ConstraintAssertEnabled (
VOID
)
{
  return (BOOLEAN) ((PcdGet8(PcdDebugPropertyMask) &
DEBUG_PROPERTY_DEBUG_CONSTRAINT_ENABLED) != 0);
}

EFI_STATUS
EFIAPI
SetConstraintHandler (
IN CONSTRAINT_HANDLER Handler
)
{
if (Handler == NULL) {
gActiveConstraintHandler = gDefaultConstraintHandler;
} else {
gActiveConstraintHandler = Handler;
}
}

VOID
AssertConstraintHandler (
IN CONST CHAR8  *FileName,
IN UINTN                 LineNumber,
IN CONST CHAR8  *Description,
IN EFI_STATUS      Status
)
{
  if (ConstraintAssertEnabled ()) {
     DEBUG ((EFI_D_ERROR, "\Constraint ASSERT (Status = %r): ", Status));
     DebugAssert (FileName, LineNumber, Description)
}

 return;
}

VOID
IgnoreConstraintHandler (
IN CONST CHAR8  *FileName,
IN UINTN                 LineNumber,
IN CONST CHAR8  *Description,
IN EFI_STATUS      Status
)
{
return;
}

We could add macros for the code in the lib to call:

#define CONSTRAINT_CHECK(Expression, Status)  \
do { \
if (!(Expression)) { \
gActiveConstraintHandler (__FILE__, __LINE__, Expression, Status); \
return Status; \
} \
} while (FALSE)

#define CONSTRAINT_REQUIRE(Expression, Status, Label)  \
do { \
if (!(Expression)) { \
gActiveConstraintHandler (__FILE__, __LINE__, Expression, Status); \
goto Label; \
} \
} while (FALSE)


As a caller we have now have control:
EFI_STATUS Status;
CHAR16        Dst[2];

SetConstraintHandler (IgnoreConstraintHandler);
Status = StrCpyS (Dst, sizeof (Dst), L"Too Long");
Print (L"Dst =%s (%r)\n",  Dst, Status);

SetConstraintHandler (AssertConstraintHandler);
StrCpyS (Dst, sizeof (Dst), L"Too Long");

Thanks,

Andrew Fish

PS Since I'm on a crazy idea roll another idea would be to add a
MdePkgCompatability.dsc.inc file that could be used to future proof
adding dependent libs to existing MdePkg libs. So a platform could
include this .DSC and that would give them the default library mapping
to keep code compiling. It will only work after other platforms start
including it, but after that it would give default mappings for
dependent libs.

In our above example we could have added this and then existing builds
that included MdePkgCompatability.dsc.inc would keep compiling.

 [LibraryClasses]

DebugConstraintLib|MdePkg/Library/DebugConstraintLib/DebugConstraintLib.inf



On Feb 15, 2020, at 11:38 AM, Michael D Kinney
<michael.d.kinney@...
<mailto:michael.d.kinney@...><mailto:michael.d.kinney@...>>
wrote:

Andrew,
I do not think of this as a globally scoped PCD.It can be set in
global scope in DSC.But is can also be set to values based on module
type or for specific modules.In the case of the safe string functions,
I think there is a desire to disable the constraint asserts when
building a UEFI App or UEFI Driver and implement those modules to
handle the error return values.Enabling the constraint asserts for PEI
Core, DXE Core, SMM/MM Core, PEIM, DXE, SMM/MM modules makes sense to
find incorrect input to these functions from modules that can
guarantee the inputs would never return an error and catch these as
part of dev/debug/validation builds.
I would not expect disabling on a module by module basis to be common.
I think the rule for API implementations is to only use
CONSTRAINT_ASSERT() for conditions that are also checked and return an
error or fail with predicable behavior that allows the system to
continue to function.ASSERT() is for conditions that the systemcan
notcontinue.
Best regards,
Mike
*From:*afish@...
<mailto:afish@...><mailto:afish@...><afish@...
<mailto:afish@...>
<mailto:afish@...>>
*Sent:*Friday, February 14, 2020 10:27 PM
*To:*vit9696 <vit9696@...
<mailto:vit9696@...><mailto:vit9696@...>>
*Cc:*devel@edk2.groups.io
<mailto:devel@edk2.groups.io><mailto:devel@edk2.groups.io>; Kinney,
Michael D <michael.d.kinney@...
<mailto:michael.d.kinney@...>
<mailto:michael.d.kinney@...>>; Gao, Liming
<liming.gao@...
<mailto:liming.gao@...><mailto:liming.gao@...>>; Gao,
Zhichao
<zhichao.gao@...
<mailto:zhichao.gao@...><mailto:zhichao.gao@...>>;
Marvin Häuser
<marvin.haeuser@...
<mailto:marvin.haeuser@...><mailto:marvin.haeuser@...>>;
Laszlo Ersek <lersek@...
<mailto:lersek@...><mailto:lersek@...>>
*Subject:*Re: [edk2-devel] [PATCH v3 0/1] Add PCD to disable safe
string constraint assertions
Vitaly,
Sorry after I sent the mail I realized it may come  across as me
asking you to do work and that was not my intent.
I will point out though that a non backward compatible change to
something as fundamental as the DebugLib is a very big deal. I've got
a few different custom implementations that would break with this
change as Mike proposed. Given breaking every one's debug lib is such
a big deal maybe it is something that we should do as a long term plan
vs. some incremental fix. So my intent was to start a conversation
about what else we might want to change if we are going to break the
world. The only think worse than breaking the world is breaking the
world frequently.
I'm also a little worried that we are taking things that are today
locally scoped like SAFE_STRING_CONSTRAINT_CHECK and
SAFE_PRINT_CONSTRAINT_CHECK and making them global constructs. I think
the way others have dealt with things like this is to make them be
DEBUG prints vs. ASSERTs. Also even something as simple as
SAFE_STRING_CONSTRAINT_CHECK could be called from code that wants
ASSERT and CONSTRAINT_ASSERT behavior. It is not clear to me that the
low level code knows the right thing to do in a global sense even if
there is a PCD.  It almost seems like we should have wrappers for the
Safe string functions that implement the behavior you want as a
caller. I'm not sure about that, but it seems like it is worth talking
about?
Thanks,
Andrew Fish


   On Feb 14, 2020, at 7:31 PM, vit9696 <vit9696@...
<mailto:vit9696@...>
   <mailto:vit9696@...>> wrote:
   Hi Andrew,
   While your suggestions look interesting, I am afraid they are not
   particularly what we want to cover with this discussion at the
moment.
   Making assertions go through DEBUG printing functions/macros is
   what we have to strongly disagree about. Assertions and debug
   prints are separate things configurable by separate PCDs. We
   should not mix them. Introducing constraint assertions is a
   logical step forward in understanding that different software
   works in different environments.

     * There are normal, or, as I call them, invariant
       assertions (e.g. preconditions), for places where the function
       cannot work unless the assertion is satisfied. This is where
       we ASSERT.
     * There are constraint assertions, which signalise that bad data
       came through the function, even though the function was called
       from a trusted source. This is where we call CONSTRAINT_ASSERT.

   The thing we need is to have the latter separable
   and configurable, because not every piece of software works in a
   trusted environment. Other than that, constraint assertions, when
   enabled, are not anyhow different from normal assertions in the
   sense of action taken. Assertions have configurable breakpoints
   and deadloops, and DEBUG prints go through a different route in
   DebugLib that may cause entirely different effects. For example,
   we halt execution upon printing to DEBUG_ERROR in our DebugLib
   even in release builds.

   =To make it clear, currently I plan to add the following interface:
   #define CONSTRAINT_ASSERT(Expression) \
   do { \
   if (DebugConstraintAssertEnabled ()) { \
   if (!(Expression)) { \
   _ASSERT (Expression); \
   ANALYZER_UNREACHABLE (); \
   } \
   } \
   } while (FALSE)
   with DebugConstraintAssertEnabled implemented as
   (BOOLEAN) ((PcdGet8(PcdDebugPropertyMask) &
   DEBUG_PROPERTY_DEBUG_ASSERT_ENABLED |
   DEBUG_PROPERTY_CONTRAINT_ASSERT_DISABLED) ==
   DEBUG_PROPERTY_DEBUG_ASSERT_ENABLED)
   Your suggestion with require macros looks interesting indeed, but
   I believe it is in fact parallel to this discussion. The change we
   discuss introduces a new assertion primitive — constraint
   assertions, while REQUIRE macros are mostly about advanced syntax
   sugar and higher level assertion primitives on top of existing
   ones. Perhaps we can have this and make a good use of it,
   especially given that it brought some practical benefit in Apple,
   but I would rather discuss this later once constraint assertions
   are merged into EDK II tree.
   Best wishes,
   Vitaly
   On Sat, Feb 15, 2020 at 03:02, Andrew Fish <afish@...
<mailto:afish@...>
   <mailto:afish@...>> wrote:



           On Feb 14, 2020, at 2:50 PM, Michael D Kinney
           <michael.d.kinney@...
<mailto:michael.d.kinney@...>
           <mailto:michael.d.kinney@...>> wrote:
           Hi Vitaly,
           I agree that this proposal makes a lot of sense. We
           recently added a new assert type called STATIC_ASSERT()
           for assert conditions that can be tested at build time.
           A new assert type for checks that can be removed and the
           API still guarantees that it fails gracefully with a
           proper return code is a good idea. Given we have
           STATIC_ASSERT(), how about naming the new macro
           CONSTRAINT_ASSERT()?
           We also want the default to be enabled. The current use of
           bit 0x40 inPcdDebugPropertyMask is always clear, so we
           want the asserts enabled when 0x40 is clear. We can change
           the name of the define bit to
           DEBUG_PROPERTY_CONTRAINT_ASSERT_DISABLED so bit 0x40 needs
           to be set inPcdDebugPropertyMaskto disable these types of
           asserts.
           This approach does require all theDebugLibimplementations
           to be updated with the newDebugConstraintAssertDisabled()
API.

       Mike,
       If you wanted to be backward compatible you could just
       use DebugAssertEnabled () but in place of _ASSERT() use
       _CONSTRAINT_ASSERT
       #define _ASSERT(Expression)  DebugAssert (__FILE__, __LINE__,
       #Expression)
       #define _CONSTRAINT_ASSERT(Expression)  DebugPrint
       (DEBUG_ERROR,  "ASSERT %a(%d): %a\n",, __FILE__, __LINE__,
       #Expression)
       Not as elegant as the non backward compatible change, but I
       thought I'd throw it out there.
       There are some ancient Apple C ASSERT macros [AssertMacros.h]
        that also have the concept of require. Require includes an
       exception label (goto label). It is like a CONSTRAINT_ASSERT()
       but with the label. On release builds the DEBUG prints are
       skipped.
       So we could do something like:
         EFI_STATUS Status =  EFI_INVALID_PARAMETER;
         REQUIRE(Arg1 != NULL, ErrorExit);
         REQUIRE(Arg2 != NULL, ErrorExit);
         REQUIRE(Arg3 != NULL, ErrorExit);
       ErrorExit:
         return Status;
       There is another form that allows an ACTION (a statement to
       execute. So you could have:
         EFI_STATUS Status;
         REQUIRE_ACTION(Arg1 != NULL, ErrorExit, Status =
       EFI_INVALID_PARAMETER);
         REQUIRE_ACTION(Arg2 != NULL, ErrorExit, Status =
       EFI_INVALID_PARAMETER);
         REQUIRE_ACTION(Arg3 != NULL, ErrorExit, Status =
       EFI_INVALID_PARAMETER);
       ErrorExit:
         return Status;
       If you use CONSTRAINT_ASSERT();
         if (Arg1 == NULL || Arg2 == NULL || Arg3 == NULL) {
          CONSTRAINT_ASSERT (Arg1 != NULL);
          CONSTRAINT_ASSERT (Arg2 != NULL);
          CONSTRAINT_ASSERT (Arg3 != NULL);
          return EFI_INVALID_PARAMETER;
        }
       I'd note error processing args on entry is the simplest case.
        In a more complex case when cleanup is required the goto
       label is more useful.
       I guess we could argue for less typing and more symmetry and
       say use ASSERT, CONSTRAINT, and REQUIRE. I guess you could add
       an ASSERT_ACTION too.
       The AssertMacros.h versions also support _quiet (skip the
       print) and _string (add a string to the print) so you end up
with:
       REQUIRE
       REQUIRE_STRING
       REQUIRE_QUIET
       REQUIRE_ACTION
       REQUIRE_ACTION_STRING
       REQUIRE_ACTION_QUIET
       We could also end up with
       CONSTRAINT
       CONSTRAINT_STRING
       CONSTRAINT_QUIET
       I think the main idea behind _QUIET is you can silence things
       that are too noisy, and you can easily make noise things show
       up by removing the _QUIET to debug.
       I'd thought I throw out the other forms for folks to think
       about. I'm probably biased as I used to looking at code and
       seeing things like require_action_string(Arg1 != NULL,
       ErrorExit, Status = EFI_INVALID_PARAMETER, "1st Arg1 check");
       Thanks,
       Andrew Fish
       PS The old debug macros had 2 versions of CONSTRAINT check and
       verify. The check version was compiled out on a release build,
       the verify version always does the check and just skips the
       DEBUG print.

           Mike
           *From:*vit9696 <vit9696@...
<mailto:vit9696@...>
           <mailto:vit9696@...>>
           *Sent:*Friday, February 14, 2020 9:38 AM
           *To:*Kinney, Michael D <michael.d.kinney@...
<mailto:michael.d.kinney@...>
           <mailto:michael.d.kinney@...>>
           *Cc:*devel@edk2.groups.io
<mailto:devel@edk2.groups.io><mailto:devel@edk2.groups.io>;
           Gao, Liming <liming.gao@...
<mailto:liming.gao@...>
           <mailto:liming.gao@...>>; Gao, Zhichao
           <zhichao.gao@...
<mailto:zhichao.gao@...><mailto:zhichao.gao@...>>;
           Marvin Häuser <marvin.haeuser@...
<mailto:marvin.haeuser@...>
           <mailto:marvin.haeuser@...>>; Laszlo Ersek
           <lersek@...
<mailto:lersek@...><mailto:lersek@...>>
           *Subject:*Re: [edk2-devel] [PATCH v3 0/1] Add PCD to
           disable safe string constraint assertions
           Michael,
           Generalising the approach makes good sense to me, but we
           need to make an obvious distinguishable difference between:
           - precondition and invariant assertions (i.e. assertions,
           where function will NOT work if they are violated)
           - constraint asserts (i.e. assertions, which allow us to
           spot unintentional behaviour when parsing untrusted data,
           but which do not break function behaviour).
           As we want to use this outside of SafeString,  I suggest
           the following:
           - Introduce DEBUG_PROPERTY_ASSERT_CONSTRAINT_ENABLED 0x40
           bit for PcdDebugPropertyMask instead
           of PcdAssertOnSafeStringConstraints.
           - Introduce DebugAssertConstraintEnabled DebugLib function
           to check for DEBUG_PROPERTY_ASSERT_CONSTRAINT_ENABLED.
           - Introduce ASSERT_CONSTRAINT macro, that will assert only
           if DebugConstraintAssertEnabled returns true.
           - Change SafeString ASSERTS
           in SAFE_STRING_CONSTRAINT_CHECK to ASSERT_CONSTRAINTs.
           - Use ASSERT_CONSTRAINT in other places where necessary.

           I believe this way lines best with EDK II design. If there
           are no objections, I can submit the patch in the beginning
           of next week.

           Best wishes,
           Vitaly

               14 февр. 2020 г., в 20:00, Kinney, Michael D
               <michael.d.kinney@...
<mailto:michael.d.kinney@...>
               <mailto:michael.d.kinney@...>> написал(а):
               Vitaly,
               I want to make sure a feature PCD can be used to
               disable ASSERT() behavior in more than just safe
               string functions inBaseLib.
               Can we consider changing the name and description
               ofPcdAssertOnSafeStringConstraintsto be more generic,
               so if we find other lib APIs, the name will make sense?
               Maybe something like:PcdEnableLibraryAssertChecks?
               Default is TRUE. Can set to FALSE in DSC file to
               disable ASSERT() checks.
               Thanks,
               Mike
               *From:*devel@edk2.groups.io <mailto:devel@edk2.groups.io>
               <mailto:devel@edk2.groups.io><devel@edk2.groups.io
<mailto:devel@edk2.groups.io>
               <mailto:devel@edk2.groups.io>>*On Behalf Of*Vitaly
               Cheptsov via Groups.Io
               *Sent:*Friday, February 14, 2020 3:55 AM
               *To:*Kinney, Michael D <michael.d.kinney@...
<mailto:michael.d.kinney@...>
               <mailto:michael.d.kinney@...>>; Gao, Liming
               <liming.gao@...
<mailto:liming.gao@...><mailto:liming.gao@...>>;
               Gao, Zhichao <zhichao.gao@...
<mailto:zhichao.gao@...>
               <mailto:zhichao.gao@...>>;devel@edk2.groups.io
<mailto:devel@edk2.groups.io>
               <mailto:devel@edk2.groups.io>
               *Cc:*Marvin Häuser <marvin.haeuser@...
<mailto:marvin.haeuser@...>
               <mailto:marvin.haeuser@...>>; Laszlo Ersek
               <lersek@...
<mailto:lersek@...><mailto:lersek@...>>
               *Subject:*Re: [edk2-devel] [PATCH v3 0/1] Add PCD to
               disable safe string constraint assertions
               Replying as per Liming's request for this to be merged
               into edk2-stable202002.
               On Mon, Feb 10, 2020 at 14:12, vit9696
               <vit9696@... <mailto:vit9696@...>
               <mailto:vit9696@...>> wrote:

                   Hello,

                   It has been quite some time since we submitted the
                   patch with so far no negative response. As I
                   mentioned previously, my team will strongly
                   benefit from its landing in EDK II mainline. Since
                   it does not add any regressions and can be viewed
                   as a feature implementation for the rest of EDK II
                   users, I request this to be merged upstream in
                   edk2-stable202002.

                   Best wishes,
                   Vitaly

27 янв. 2020 г., в 12:47, vit9696
                   <vit9696@...
<mailto:vit9696@...>
                   <mailto:vit9696@...>> написал(а):


Hi Mike,

Any progress with this? We would really benefit
                   from this landing in the next stable release.

Best,
Vitaly

8 янв. 2020 г., в 19:35, Kinney, Michael D
                   <michael.d.kinney@...
<mailto:michael.d.kinney@...>
                   <mailto:michael.d.kinney@...>> написал(а):


Hi Vitaly,

Thanks for the additional background. I would like
a couple extra day to review the PCD name and
                   the places
the PCD might potentially be used.

If we find other APIs where ASSERT() behavior
                   is only
valuable during dev/debug to quickly identify
                   misuse
with trusted data and the API provides predicable
return behavior when ASSERT() is disabled, then
                   I would
like to have a pattern we can potentially apply
                   to all
these APIs across all packages.

Thanks,

Mike

-----Original Message-----
From:devel@edk2.groups.io <mailto:devel@edk2.groups.io>
                   <mailto:devel@edk2.groups.io><devel@edk2.groups.io <mailto:devel@edk2.groups.io>
                   <mailto:devel@edk2.groups.io>> On
Behalf Of Vitaly Cheptsov via Groups.Io
Sent: Monday, January 6, 2020 10:44 AM
To: Kinney, Michael D
                   <michael.d.kinney@...
<mailto:michael.d.kinney@...>
                   <mailto:michael.d.kinney@...>>
Cc:devel@edk2.groups.io <mailto:devel@edk2.groups.io>
                   <mailto:devel@edk2.groups.io>
Subject: Re: [edk2-devel] [PATCH v3 0/1] Add
                   PCD to
disable safe string constraint assertions

Hi Mike,

Yes, the primary use case is for UEFI
                   Applications. We
do not want to disable ASSERT’s completely, as
assertions that make sense, i.e. the ones
                   signalising
about interface misuse, are helpful for debugging.

I have already explained in the BZ that
                   basically all
safe string constraint assertions make no
                   sense for
handling untrusted data. We find this use case
                   very
logical, as these functions behave properly with
assertions disabled and cover all these error
conditions by the return statuses. In such
                   situation is
not useful for these functions to assert, as
                   we end up
inefficiently reimplementing the logic. I
                   would have
liked the approach of discussing the interfaces
individually, but I struggle to find any that
                   makes
sense from this point of view.

AsciiStrToGuid will ASSERT when the length of the
passed string is odd. Functions that cannot, ahem,
parse, for us are pretty much useless.
AsciiStrCatS will ASSERT when the appended
                   string does
not fit the buffer. For us this logic makes this
function pretty much equivalent to deprecated
                   and thus
unavailable AsciiStrCat, except it is also slower.

My original suggestion was to remove the
                   assertions
entirely, but several people here said that
                   they use
them to verify usage errors when handling
                   trusted data.
This makes good sense to me, so we suggest to
                   support
both cases by introducing a PCD in this patch.

Best wishes,
Vitaly

6 янв. 2020 г., в 21:28, Kinney, Michael D
<michael.d.kinney@... <mailto:michael.d.kinney@...>
                   <mailto:michael.d.kinney@...>> написал(а):


Hi Vitaly,

Is the use case for UEFI Applications?

There is a different mechanism to disable all
ASSERT()
statements within a UEFI Application.

If a component is consuming data from an
                   untrusted
source,
then that component is required to verify the
untrusted
data before passing it to a function that clearly
documents
is input requirements. If this approach is
                   followed,
then
the BaseLib functions can be used "as is" as
                   long as
the
ASSERT() conditions are verified before calling.

If there are some APIs that currently
                   document their
ASSERT()
behavior and we think that ASSERT() behavior is
incorrect and
should be handled by an existing error return
                   value,
then we
should discuss each of those APIs individually.

Mike


-----Original Message-----
From:devel@edk2.groups.io <mailto:devel@edk2.groups.io>
                   <mailto:devel@edk2.groups.io><devel@edk2.groups.io <mailto:devel@edk2.groups.io>
                   <mailto:devel@edk2.groups.io>> On
Behalf Of Vitaly Cheptsov via Groups.Io
Sent: Friday, January 3, 2020 9:13 AM
To:devel@edk2.groups.io <mailto:devel@edk2.groups.io>
                   <mailto:devel@edk2.groups.io>
Subject: [edk2-devel] [PATCH v3 0/1] Add PCD to
disable
safe string constraint assertions

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

Requesting for merge in edk2-stable202002.

Changes since V1:
- Enable assertions by default to preserve the
original
behaviour
- Fix bugzilla reference link
- Update documentation in BaseLib.h

Vitaly Cheptsov (1):
MdePkg: Add PCD to disable safe string
                   constraint
assertions

MdePkg/MdePkg.dec | 6 ++
MdePkg/Library/BaseLib/BaseLib.inf | 11 +--
MdePkg/Include/Library/BaseLib.h | 74
+++++++++++++-------
MdePkg/Library/BaseLib/SafeString.c | 4 +-
MdePkg/MdePkg.uni | 6 ++
5 files changed, 71 insertions(+), 30
                   deletions(-)

--
2.21.0 (Apple Git-122.2)


-=-=-=-=-=-=
Groups.io <http://groups.io/><http://groups.io/>Links: You
                   receive all messages sent to
this
group.

View/Reply Online (#52837):
https://edk2.groups.io/g/devel/message/52837
Mute This Topic:
https://groups.io/mt/69401948/1643496
Group Owner:devel+owner@edk2.groups.io
<mailto:devel+owner@edk2.groups.io>
                   <mailto:devel+owner@edk2.groups.io>
Unsubscribe:https://edk2.groups.io/g/devel/unsub
[michael.d.kinney@... <mailto:michael.d.kinney@...>
                   <mailto:michael.d.kinney@...>]
-=-=-=-=-=-=


Re: [edk2-platforms] [PATCH V2 2/2] Enable build for CometlakeOpenBoardPkg

Chaganty, Rangasai V
 

Reviewed-by: Sai Chaganty <rangasai.v.chaganty@...>

-----Original Message-----
From: Esakkithevar, Kathappan <kathappan.esakkithevar@...>
Sent: Wednesday, February 19, 2020 3:56 AM
To: devel@edk2.groups.io
Cc: Chaganty, Rangasai V <rangasai.v.chaganty@...>; Chiu, Chasel <chasel.chiu@...>; Desimone, Nathaniel L <nathaniel.l.desimone@...>; Kethi Reddy, Deepika <deepika.kethi.reddy@...>; Agyeman, Prince <prince.agyeman@...>
Subject: [edk2-platforms] [PATCH V2 2/2] Enable build for CometlakeOpenBoardPkg

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

This change adds the configuration to enable build for CometlakeURvp.
Also it updates Cometlake U Rvp details to the Readme.md.

Signed-off-by: Kathappan Esakkithevar <kathappan.esakkithevar@...>
Cc: Sai Chaganty <rangasai.v.chaganty@...>
Cc: Chasel Chiu <chasel.chiu@...>
Cc: Nate DeSimone <nathaniel.l.desimone@...>
Cc: Deepika Kethi Reddy <deepika.kethi.reddy@...>
Cc: Prince Agyeman <prince.agyeman@...>
---
Platform/Intel/Readme.md | 25 ++++++++++++++++++------- Platform/Intel/build.cfg | 3 ++-
2 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/Platform/Intel/Readme.md b/Platform/Intel/Readme.md index 02d9517d19..2e66b4ce72 100644
--- a/Platform/Intel/Readme.md
+++ b/Platform/Intel/Readme.md
@@ -19,7 +19,7 @@ package.
## Board Naming Convention
The board packages supported by Intel follow the naming convention \<xxx\>OpenBoardPkg where xxx refers to the encompassing platform name for a particular platform generation. For example, the [`KabylakeOpenBoardPkg`](https://github.com/tianocore/edk2-platforms/tree/master/Platform/Intel/KabylakeOpenBoardPkg) contains the -board code for Intel Kaby Lake reference systems. Intel uses the moniker "OpenBoardPkg" to indicate that this package
+board code for Intel KabyLake reference systems. Intel uses the moniker
+"OpenBoardPkg" to indicate that this package
is the open source board code. A closed source counterpart may exist which simply uses "BoardPkg". Both directly use the MinPlatformPkg from edk2-platforms.

@@ -53,9 +53,10 @@ A UEFI firmware implementation using MinPlatformPkg is constructed using the fol


## Board Support
-* The `KabylakeOpenBoardPkg` contains board implementations for Kaby Lake systems.
+* The `KabylakeOpenBoardPkg` contains board implementations for KabyLake systems.
* The `SimicsOpenBoardPkg` contains board implementations for the Simics hardware simulator.
-* The `WhiskeylakeOpenBoardPkg` contains board implementations for Whiskey Lake systems.
+* The `WhiskeylakeOpenBoardPkg` contains board implementations for WhiskeyLake systems.
+* The `CometlakeOpenBoardPkg` contains board implementations for CometLake systems.

### **Supported Hardware**

@@ -65,8 +66,9 @@ A UEFI firmware implementation using MinPlatformPkg is constructed using the fol

| Machine Name | Supported Chipsets | BoardPkg | Board Name |
----------------------------------------|--------------------------------------------|------------------------------|--------------------|
-| RVP 3 | Sky Lake, Kaby Lake, Kaby Lake Refresh | KabylakeOpenBoardPkg | KabylakeRvp3 |
-| WHL-U DDR4 RVP | Whiskey Lake | WhiskeylakeOpenBoardPkg | WhiskeylakeURvp |
+| RVP 3 | SkyLake, KabyLake, KabyLake Refresh | KabylakeOpenBoardPkg | KabylakeRvp3 |
+| WHL-U DDR4 RVP | WhiskeyLake | WhiskeylakeOpenBoardPkg | WhiskeylakeURvp |
+| CML-U LPDDR3 RVP | CometLake V1 | CometlakeOpenBoardPkg | CometlakeURvp |

*Note: RVP = Reference and Validation Platform*

@@ -82,8 +84,8 @@ A UEFI firmware implementation using MinPlatformPkg is constructed using the fol

| Machine Name | Supported Chipsets | BoardPkg | Board Name |
----------------------------------------|--------------------------------------------|------------------------------|--------------------|
-| galp2 | Kaby Lake | KabylakeOpenBoardPkg | GalagoPro3 |
-| galp3 & galp3-b | Kaby Lake Refresh | KabylakeOpenBoardPkg | GalagoPro3 |
+| galp2 | KabyLake | KabylakeOpenBoardPkg | GalagoPro3 |
+| galp3 & galp3-b | KabyLake Refresh | KabylakeOpenBoardPkg | GalagoPro3 |

## Board Package Organization
The board package follows the standard EDK II package structure with the following additional elements and guidelines:
@@ -237,6 +239,11 @@ return back to the minimum platform caller.
| | | |---build_config.cfg: WhiskeylakeURvp specific build
| | | settings environment variables.
| | |
+ | | |------CometlakeOpenBoardPkg
+ | | | |------CometlakeURvp
+ | | | |---build_config.cfg: CometlakeURvp specific build
+ | | | settings environment variables.
+ | | |
|------FSP
</pre>

@@ -257,6 +264,10 @@ return back to the minimum platform caller.
1. This firmware project has only been tested booting to Microsoft Windows 10 x64 with AHCI mode and Integrated Graphic
Device.

+**CometlakeOpenBoardPkg**
+1. This firmware project has been tested booting to Microsoft Windows 10 x64 with AHCI mode and External Graphic Device.
+2. This firmware project has been also tested booting to Ubuntu 17.10 with AHCI mode and Integrated Graphic Device.
+
### **Package Builds**

In some cases, such as BoardModulePkg, a package may provide a set of functionality that is included in other diff --git a/Platform/Intel/build.cfg b/Platform/Intel/build.cfg index 86a9115021..5bc1dea43c 100644
--- a/Platform/Intel/build.cfg
+++ b/Platform/Intel/build.cfg
@@ -1,7 +1,7 @@
# @ build.cfg
# This is the main/default build configuration file # -# Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
+# Copyright (c) 2019 - 2020 Intel Corporation. All rights reserved.<BR>
# SPDX-License-Identifier: BSD-2-Clause-Patent #

@@ -58,3 +58,4 @@ BoardX58Ich10 = SimicsOpenBoardPkg/BoardX58Ich10/build_config.cfg
GalagoPro3 = KabylakeOpenBoardPkg/GalagoPro3/build_config.cfg
KabylakeRvp3 = KabylakeOpenBoardPkg/KabylakeRvp3/build_config.cfg
WhiskeylakeURvp = WhiskeylakeOpenBoardPkg/WhiskeylakeURvp/build_config.cfg
+CometlakeURvp = CometlakeOpenBoardPkg/CometlakeURvp/build_config.cfg
--
2.16.2.windows.1


Re: [edk2-platforms] [PATCH V2 2/2] Enable build for CometlakeOpenBoardPkg

Chiu, Chasel
 

Reviewed-by: Chasel Chiu <chasel.chiu@...>

-----Original Message-----
From: Esakkithevar, Kathappan <kathappan.esakkithevar@...>
Sent: Wednesday, February 19, 2020 7:56 PM
To: devel@edk2.groups.io
Cc: Chaganty, Rangasai V <rangasai.v.chaganty@...>; Chiu, Chasel
<chasel.chiu@...>; Desimone, Nathaniel L
<nathaniel.l.desimone@...>; Kethi Reddy, Deepika
<deepika.kethi.reddy@...>; Agyeman, Prince
<prince.agyeman@...>
Subject: [edk2-platforms] [PATCH V2 2/2] Enable build for
CometlakeOpenBoardPkg

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

This change adds the configuration to enable build for CometlakeURvp.
Also it updates Cometlake U Rvp details to the Readme.md.

Signed-off-by: Kathappan Esakkithevar <kathappan.esakkithevar@...>
Cc: Sai Chaganty <rangasai.v.chaganty@...>
Cc: Chasel Chiu <chasel.chiu@...>
Cc: Nate DeSimone <nathaniel.l.desimone@...>
Cc: Deepika Kethi Reddy <deepika.kethi.reddy@...>
Cc: Prince Agyeman <prince.agyeman@...>
---
Platform/Intel/Readme.md | 25 ++++++++++++++++++-------
Platform/Intel/build.cfg | 3 ++-
2 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/Platform/Intel/Readme.md b/Platform/Intel/Readme.md index
02d9517d19..2e66b4ce72 100644
--- a/Platform/Intel/Readme.md
+++ b/Platform/Intel/Readme.md
@@ -19,7 +19,7 @@ package.
## Board Naming Convention
The board packages supported by Intel follow the naming convention
\<xxx\>OpenBoardPkg where xxx refers to the encompassing platform
name for a particular platform generation. For example, the
[`KabylakeOpenBoardPkg`](https://github.com/tianocore/edk2-platforms/tree
/master/Platform/Intel/KabylakeOpenBoardPkg) contains the -board code for
Intel Kaby Lake reference systems. Intel uses the moniker "OpenBoardPkg" to
indicate that this package
+board code for Intel KabyLake reference systems. Intel uses the moniker
+"OpenBoardPkg" to indicate that this package
is the open source board code. A closed source counterpart may exist which
simply uses "BoardPkg". Both directly use the MinPlatformPkg from
edk2-platforms.

@@ -53,9 +53,10 @@ A UEFI firmware implementation using MinPlatformPkg
is constructed using the fol


## Board Support
-* The `KabylakeOpenBoardPkg` contains board implementations for Kaby
Lake systems.
+* The `KabylakeOpenBoardPkg` contains board implementations for
KabyLake systems.
* The `SimicsOpenBoardPkg` contains board implementations for the Simics
hardware simulator.
-* The `WhiskeylakeOpenBoardPkg` contains board implementations for
Whiskey Lake systems.
+* The `WhiskeylakeOpenBoardPkg` contains board implementations for
WhiskeyLake systems.
+* The `CometlakeOpenBoardPkg` contains board implementations for
CometLake systems.

### **Supported Hardware**

@@ -65,8 +66,9 @@ A UEFI firmware implementation using MinPlatformPkg
is constructed using the fol

| Machine Name | Supported Chipsets
| BoardPkg | Board Name |

----------------------------------------|--------------------------------------------|-------------
-----------------|--------------------|
-| RVP 3 | Sky Lake, Kaby Lake, Kaby
Lake Refresh | KabylakeOpenBoardPkg | KabylakeRvp3
|
-| WHL-U DDR4 RVP | Whiskey Lake
| WhiskeylakeOpenBoardPkg | WhiskeylakeURvp |
+| RVP 3 | SkyLake, KabyLake,
KabyLake Refresh | KabylakeOpenBoardPkg |
KabylakeRvp3 |
+| WHL-U DDR4 RVP | WhiskeyLake
| WhiskeylakeOpenBoardPkg | WhiskeylakeURvp |
+| CML-U LPDDR3 RVP | CometLake V1
| CometlakeOpenBoardPkg | CometlakeURvp |

*Note: RVP = Reference and Validation Platform*

@@ -82,8 +84,8 @@ A UEFI firmware implementation using MinPlatformPkg
is constructed using the fol

| Machine Name | Supported Chipsets
| BoardPkg | Board Name |

----------------------------------------|--------------------------------------------|-------------
-----------------|--------------------|
-| galp2 | Kaby Lake
| KabylakeOpenBoardPkg | GalagoPro3 |
-| galp3 & galp3-b | Kaby Lake Refresh
| KabylakeOpenBoardPkg | GalagoPro3 |
+| galp2 | KabyLake
| KabylakeOpenBoardPkg | GalagoPro3 |
+| galp3 & galp3-b | KabyLake Refresh
| KabylakeOpenBoardPkg | GalagoPro3 |

## Board Package Organization
The board package follows the standard EDK II package structure with the
following additional elements and guidelines:
@@ -237,6 +239,11 @@ return back to the minimum platform caller.
| | | |---build_config.cfg:
WhiskeylakeURvp specific build
| | |
settings environment variables.
| | |
+ | | |------CometlakeOpenBoardPkg
+ | | | |------CometlakeURvp
+ | | | |---build_config.cfg:
CometlakeURvp specific build
+ | | |
settings environment variables.
+ | | |
|------FSP
</pre>

@@ -257,6 +264,10 @@ return back to the minimum platform caller.
1. This firmware project has only been tested booting to Microsoft Windows
10 x64 with AHCI mode and Integrated Graphic
Device.

+**CometlakeOpenBoardPkg**
+1. This firmware project has been tested booting to Microsoft Windows 10
x64 with AHCI mode and External Graphic Device.
+2. This firmware project has been also tested booting to Ubuntu 17.10 with
AHCI mode and Integrated Graphic Device.
+
### **Package Builds**

In some cases, such as BoardModulePkg, a package may provide a set of
functionality that is included in other diff --git a/Platform/Intel/build.cfg
b/Platform/Intel/build.cfg index 86a9115021..5bc1dea43c 100644
--- a/Platform/Intel/build.cfg
+++ b/Platform/Intel/build.cfg
@@ -1,7 +1,7 @@
# @ build.cfg
# This is the main/default build configuration file # -# Copyright (c) 2019,
Intel Corporation. All rights reserved.<BR>
+# Copyright (c) 2019 - 2020 Intel Corporation. All rights reserved.<BR>
# SPDX-License-Identifier: BSD-2-Clause-Patent #

@@ -58,3 +58,4 @@ BoardX58Ich10 =
SimicsOpenBoardPkg/BoardX58Ich10/build_config.cfg
GalagoPro3 = KabylakeOpenBoardPkg/GalagoPro3/build_config.cfg
KabylakeRvp3 = KabylakeOpenBoardPkg/KabylakeRvp3/build_config.cfg
WhiskeylakeURvp =
WhiskeylakeOpenBoardPkg/WhiskeylakeURvp/build_config.cfg
+CometlakeURvp =
CometlakeOpenBoardPkg/CometlakeURvp/build_config.cfg
--
2.16.2.windows.1


Re: [PATCH] MdeModulePkg/SetupBrowserDxe: Fix IsZeroGuid() ASSERT.

Dandan Bi
 

Thanks Nickle for the fix.
One minor comment is that please pay attention to the format of commit message.
Refer to https://github.com/tianocore/tianocore.github.io/wiki/Commit-Message-Format, Line length of commit message should be less than 76 characters when possible.
Please address it when submit the patch.
Reviewed-by: Dandan Bi <dandan.bi@...>


Thanks,
Dandan

-----Original Message-----
From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
Nickle Wang
Sent: Wednesday, February 19, 2020 10:23 PM
To: devel@edk2.groups.io; nickle.wang@...
Cc: Bi, Dandan <dandan.bi@...>
Subject: [edk2-devel] [PATCH] MdeModulePkg/SetupBrowserDxe: Fix
IsZeroGuid() ASSERT.

From the function description of GetIfrBinaryData(), FormSetGuid can be
NULL. However, FormSetGuid is passed to IsZeroGuid(). This causes
exception when FormSetGuid is NULL.

Signed-off-by: Nickle Wang <nickle.wang@...>
---
MdeModulePkg/Universal/SetupBrowserDxe/Setup.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Universal/SetupBrowserDxe/Setup.c
b/MdeModulePkg/Universal/SetupBrowserDxe/Setup.c
index 288f1c3197..82067b541c 100644
--- a/MdeModulePkg/Universal/SetupBrowserDxe/Setup.c
+++ b/MdeModulePkg/Universal/SetupBrowserDxe/Setup.c
@@ -2,6 +2,7 @@
Entry and initialization module for the browser.

Copyright (c) 2007 - 2018, Intel Corporation. All rights reserved.<BR>
+(C) Copyright 2020 Hewlett Packard Enterprise Development LP<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent

**/
@@ -5844,7 +5845,7 @@ GetIfrBinaryData (
//
// Try to compare against formset GUID
//
- if (IsZeroGuid (FormSetGuid) ||
+ if (IsZeroGuid (ComparingGuid) ||
CompareGuid (ComparingGuid, (EFI_GUID *)(OpCodeData + sizeof
(EFI_IFR_OP_HEADER)))) {
break;
}
--
2.20.1.windows.1



Re: Patch List for 202002 stable tag

Vitaly Cheptsov
 

Liming, no problem from our side. The patch is now reviewed and I believe I provided all the necessarily material regarding its status.

In case Ray would rather postpone it, I give no objection to this without prior notice. There is no problem from our side if EDK II team wants to prioritise other issues, we can always merge it right after the stable tag lands.

Best wishes,
Vitaly

В чт, февр. 20, 2020 в 09:58, Gao, Liming <liming.gao@...> пишет:

Ray, Zhichao and Vitaly:

  Thanks. BZ is the good place to catch all discussion. Because we are close to edk2 stable tag 202002, can you make the agreement soon for BZ 2510?

 

Thanks

Liming

From: Ni, Ray <ray.ni@...>
Sent: Thursday, February 20, 2020 11:13 AM
To: Gao, Liming <liming.gao@...>; vit9696 <vit9696@...>
Cc: Kinney, Michael D <michael.d.kinney@...>; Laszlo Ersek <lersek@...>; Guptha, Soumya K <soumya.k.guptha@...>; leif@...; afish@...; devel@edk2.groups.io; Marvin Häuser <marvin.haeuser@...>; Gao, Zhichao <zhichao.gao@...>
Subject: RE: Patch List for 202002 stable tag

 

Liming,

I provided my comments in the BZ.

 

From: Gao, Liming <liming.gao@...>
Sent: Thursday, February 20, 2020 9:17 AM
To: vit9696 <vit9696@...>
Cc: Kinney, Michael D <michael.d.kinney@...>; Laszlo Ersek <lersek@...>; Guptha, Soumya K <soumya.k.guptha@...>; leif@...; afish@...; devel@edk2.groups.io; Marvin Häuser <marvin.haeuser@...>; Ni, Ray <ray.ni@...>; Gao, Zhichao <zhichao.gao@...>
Subject: RE: Patch List for 202002 stable tag

 

Vitaly:

  I add my comments.

 

Zhichao and Ray:

   Can you give your opinion for BZ https://bugzilla.tianocore.org/show_bug.cgi?id=2510? Is it a bug fix or feature enhancement?

 

Thanks

Liming

From: vit9696 <vit9696@...>
Sent: Thursday, February 20, 2020 2:09 AM
To: Gao, Liming <liming.gao@...>
Cc: Kinney, Michael D <michael.d.kinney@...>; Laszlo Ersek <lersek@...>; Guptha, Soumya K <soumya.k.guptha@...>; leif@...; afish@...; devel@edk2.groups.io; Marvin Häuser <marvin.haeuser@...>
Subject: Re: Patch List for 202002 stable tag

 

Liming,

 

Thanks for pinging me about this!

 

With the PCD[1][2] I fully agree. The fact that it did not manage to land is mainly due to a sudden discussion that arose after complete silence for almost half a year, which was sort of unexpected. I will use this message as a suggestion to include this change as one of the primary goals for 202005 and kindly ask others to help to agree on the actual implementation. This bug strongly concerns us and we believe the fact that it does not (yet) cause issues to everyone is mainly coincidence.

 

[Liming] You can also present the topic in Tiano Design meeting to collect the feedback. Ni, Ray is the meeting host. You can send the topic to him.

 

With the Shell patch, the fact that I cannot enter upper case letters or use hotkeys in the editor sounds like a bug to me. The way the actual commit message is written reflects the change of the internal logic in the codebase (it adds support of specific behaviour handling on the target). In my opinion, it should not necessarily include the word «Fix» to be qualified as a bugfix, this is what bugzilla is for.

 

[Liming] If this fix is the bug, I agree it follows the process to catch this stable tag. I include ShellPkg maintainers (Ray Ni and Zhichao Gao) to give the opinion for the bug or not.

 

I am personally ok with deferring it to a next stable tag, but if the reasoning for this is «Feature planning freeze» dates, they do not strictly apply due to the reasons I stated above. So far the patch received only one review comment, which in fact was due to a minor misinterpretation. We also did some fairly extensive testing on our side before the submission (that’s why it actually took us a few more days). Unless the team has a lot of important work for the release, we can postpone the merge, otherwise I think it should be safe to merge this.

 

Best wishes,

Vitaly

 

 

 

19 февр. 2020 г., в 18:39, Gao, Liming <liming.gao@...> написал(а):

 


Mike and Laszlo:
 Thanks for your comments.

Vitaly:
 You request below two patches to catch 202002 stable tag. I agree with Mike and Laszlo comments. They are not ready to catch this stable tag. The first one is under discussion. The second one is like the enhancement or feature instead of the bug fix. It is submitted after Feb 7th Feature Planning Freeze. So, I suggest to defer them to next stable tag 202005.

https://edk2.groups.io/g/devel/topic/patch_v3_0_1_add_pcd_to/69401948 [PATCH v3 0/1] Add PCD to disable safe string constraint assertions (solution under discussion)
https://edk2.groups.io/g/devel/message/54122 [PATCH 1/1] ShellPkg: Add support for input with separately reported modifiers (under review, is this a feature or bug in the discussion)

Thanks
Liming

-----Original Message-----
From: Kinney, Michael D <michael.d.kinney@...>
Sent: Wednesday, February 19, 2020 4:43 AM
To: Laszlo Ersek <lersek@...>; Gao, Liming <liming.gao@...>; Guptha, Soumya K <soumya.k.guptha@...>;
leif@...; afish@...; Kinney, Michael D <michael.d.kinney@...>
Cc: devel@edk2.groups.io
Subject: RE: Patch List for 202002 stable tag

Hi Laszlo,

I agree with your assessments.

One comment below.

Mike

-----Original Message-----
From: Laszlo Ersek <lersek@...>
Sent: Tuesday, February 18, 2020 12:04 PM
To: Gao, Liming <liming.gao@...>; Guptha, Soumya
K <soumya.k.guptha@...>; Kinney, Michael D
<michael.d.kinney@...>; leif@...;
afish@...
Cc: devel@edk2.groups.io
Subject: Re: Patch List for 202002 stable tag

On 02/18/20 15:08, Gao, Liming wrote:

Hi Stewards and all:
 I collect current patch lists in devel mail list.

Those patch

 contributors request to add them for 201902 stable

tag. Because we

 have enter into Soft Feature Freeze, I want to

collect your feedback

 for them. If any patches are missing, please reply

this mail to add

 them.

Feature List (under review):


According to
<https://github.com/tianocore/tianocore.github.io/wiki/
SoftFeatureFreeze
>,
features can be merged during the SFF if their review
completed before
the SFF.

The SFF date is 2020-02-14 00:00:00 UTC-8, per
<https://github.com/tianocore/tianocore.github.io/wiki/
EDK-II-Release-Planning
>.
For me (in CET = UTC+1), that makes the deadline 2020-
02-14 09:00:00
CET.

 

https://edk2.groups.io/g/devel/topic/patch_v3_0_1_add_p
cd_to/69401948

[PATCH v3 0/1] Add PCD to disable safe string

constraint assertions

(solution under discussion)


Posted on 2020-01-03. Review doesn't appear complete.
Technically
speaking, it has missed edk2-stable202002.

There were two large gaps in the review process, namely
between these
messages:

- https://edk2.groups.io/g/devel/message/53026 [2020-
01-08]
- https://edk2.groups.io/g/devel/message/53485 [2020-
01-27]
- https://edk2.groups.io/g/devel/message/54133 [2020-
02-10]

If review seems stuck, it's advisable to ping once per
week, or a bit
more frequently. Two weeks ore more between pings is
way too long.

https://edk2.groups.io/g/devel/message/54122 [PATCH

1/1] ShellPkg: Add

support for input with separately reported modifiers

(under review, is

this a feature or bug in the disucssion)


The subject starts with "Add support for...", so it's a
new feature, or
at least a feature-enablement.

Posted on 2020-02-10. Has not been reviewed yet,
AFAICT. Same situation
as above. (Missed edk2-stable202002, technically
speaking.)

Note: I don't have a personal preference either way.
I'm just pointing
out what the SFF definition formally dictates, in my
interpretation.

If we want to extend the freeze dates, I won't object.

Bug List (reviewed):
https://edk2.groups.io/g/devel/message/54416 [PATCH

v2 00/10] Fix

false negative issue in

DxeImageVerificationHandler(CVE-2019-14575)

Clearly a bug fix; it could go in even during the HFF
<https://github.com/tianocore/tianocore.github.io/wiki/
HardFeatureFreeze
>.

https://edk2.groups.io/g/devel/message/54523 [PATCH
v1][edk2-stable202002] MdeModulePkg/SdMmcPciHcDxe:

Fix double PciIo

Unmap in TRB creation (CVE-2019-14587)


Ditto.

https://edk2.groups.io/g/devel/message/54510 [PATCH

v6 0/2]

Enhancement and Fixes to BaseHashApiLib


Hm. I feel like I need some convincing that patch#1 --
"CryptoPkg/BaseHashApiLib: Align BaseHashApiLib with
TPM 2.0
Implementation" -- is *also* a bugfix (like patch#2).

That question matters because the reviews:

- https://edk2.groups.io/g/devel/message/54513
- https://edk2.groups.io/g/devel/message/54567

were not posted before the SFF.

... I guess it's OK.


The description of the bug does not emphasis that
this really is a bug fix.  There were additional
review comments from the CryptoPkg reviewers after
the initial review/commit of this feature.  These
changes address that feedback.  The alignment with
TPM 2.0 is to use an existing set of defines for
the hash algorithms instead of define yet another
set of defines.  Details in this thread:

https://edk2.groups.io/g/devel/topic/70960524#53733

 

https://edk2.groups.io/g/devel/message/53703 [PATCH

V2] UefiCpuPkg

RegisterCpuFeaturesLib: Match data type and format

specifier

Even if this were a feature, it could go in; the review
was posted in
time:
- https://edk2.groups.io/g/devel/message/53803

In fact I don't understand why it hasn't been merged
for more than a
week now!

https://edk2.groups.io/g/devel/message/53577 [PATCH

v1 1/1] ShellPkg:

acpiview: Remove duplicate ACPI structure size

definitions

Approved in time, regardless of bugfix vs. feature.
Should go in.

https://edk2.groups.io/g/devel/message/54192 [PATCH

v2 1/1] ShellPkg:

acpiview: Validate ACPI table 'Length' field


The review was posted past the SFF, but I agree this
looks like a
bugfix, so should be OK. (Supplying missing input
sanitization is
arguably a fix.)


Bug List (under review)
https://edk2.groups.io/g/devel/message/54361 [PATCH

1/1]

NetworkPkg/ArpDxe: Recycle invalid ARP packets(CVE-

2019-14559)

https://edk2.groups.io/g/devel/message/54569 [PATCH

v3]

NetworkPkg/Ip4Dxe: Check the received package length

(CVE-2019-14559)

CVE fixes can clearly go in during the HFF too.

https://edk2.groups.io/g/devel/message/54448 [PATCH

v1 1/1] ShellPkg:

acpiview: Prevent infinite loop if structure length

is 0

Similar to "ShellPkg: acpiview: Validate ACPI table
'Length' field";
should be OK.


Just my opinion, of course.

Thanks
Laszlo

 

 




[PATCH] MdeModulePkg/Application: Overflowed Array Index

GuoMinJ
 

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2272
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2289
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2290
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2287
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2288

Index should be off-by one than size of array, so when check
array, the max index should less than size of array.

Signed-off-by: GuoMinJ <newexplorerj@...>
---
.../SmiHandlerProfileInfo/SmiHandlerProfileInfo.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/MdeModulePkg/Application/SmiHandlerProfileInfo/SmiHandlerProfileInfo.c b/MdeModulePkg/Application/SmiHandlerProfileInfo/SmiHandlerProfileInfo.c
index 0f7163160b..4f195b16ce 100644
--- a/MdeModulePkg/Application/SmiHandlerProfileInfo/SmiHandlerProfileInfo.c
+++ b/MdeModulePkg/Application/SmiHandlerProfileInfo/SmiHandlerProfileInfo.c
@@ -382,7 +382,7 @@ SxTypeToString (
IN EFI_SLEEP_TYPE Type
)
{
- if (Type >= 0 && Type <= ARRAY_SIZE(mSxTypeString)) {
+ if (Type >= 0 && Type < ARRAY_SIZE(mSxTypeString)) {
return mSxTypeString[Type];
} else {
AsciiSPrint (mNameString, sizeof(mNameString), "0x%x", Type);
@@ -407,7 +407,7 @@ SxPhaseToString (
IN EFI_SLEEP_PHASE Phase
)
{
- if (Phase >= 0 && Phase <= ARRAY_SIZE(mSxPhaseString)) {
+ if (Phase >= 0 && Phase < ARRAY_SIZE(mSxPhaseString)) {
return mSxPhaseString[Phase];
} else {
AsciiSPrint (mNameString, sizeof(mNameString), "0x%x", Phase);
@@ -457,7 +457,7 @@ StandbyButtonPhaseToString (
IN EFI_STANDBY_BUTTON_PHASE Phase
)
{
- if (Phase >= 0 && Phase <= ARRAY_SIZE(mStandbyButtonPhaseString)) {
+ if (Phase >= 0 && Phase < ARRAY_SIZE(mStandbyButtonPhaseString)) {
return mStandbyButtonPhaseString[Phase];
} else {
AsciiSPrint (mNameString, sizeof(mNameString), "0x%x", Phase);
@@ -483,7 +483,7 @@ IoTrapTypeToString (
IN EFI_SMM_IO_TRAP_DISPATCH_TYPE Type
)
{
- if (Type >= 0 && Type <= ARRAY_SIZE(mIoTrapTypeString)) {
+ if (Type >= 0 && Type < ARRAY_SIZE(mIoTrapTypeString)) {
return mIoTrapTypeString[Type];
} else {
AsciiSPrint (mNameString, sizeof(mNameString), "0x%x", Type);
@@ -508,7 +508,7 @@ UsbTypeToString (
IN EFI_USB_SMI_TYPE Type
)
{
- if (Type >= 0 && Type <= ARRAY_SIZE(mUsbTypeString)) {
+ if (Type >= 0 && Type < ARRAY_SIZE(mUsbTypeString)) {
return mUsbTypeString[Type];
} else {
AsciiSPrint (mNameString, sizeof(mNameString), "0x%x", Type);
--
2.17.1


Re: Patch List for 202002 stable tag

Liming Gao
 

Ray, Zhichao and Vitaly:

  Thanks. BZ is the good place to catch all discussion. Because we are close to edk2 stable tag 202002, can you make the agreement soon for BZ 2510?

 

Thanks

Liming

From: Ni, Ray <ray.ni@...>
Sent: Thursday, February 20, 2020 11:13 AM
To: Gao, Liming <liming.gao@...>; vit9696 <vit9696@...>
Cc: Kinney, Michael D <michael.d.kinney@...>; Laszlo Ersek <lersek@...>; Guptha, Soumya K <soumya.k.guptha@...>; leif@...; afish@...; devel@edk2.groups.io; Marvin Häuser <marvin.haeuser@...>; Gao, Zhichao <zhichao.gao@...>
Subject: RE: Patch List for 202002 stable tag

 

Liming,

I provided my comments in the BZ.

 

From: Gao, Liming <liming.gao@...>
Sent: Thursday, February 20, 2020 9:17 AM
To: vit9696 <vit9696@...>
Cc: Kinney, Michael D <michael.d.kinney@...>; Laszlo Ersek <lersek@...>; Guptha, Soumya K <soumya.k.guptha@...>; leif@...; afish@...; devel@edk2.groups.io; Marvin Häuser <marvin.haeuser@...>; Ni, Ray <ray.ni@...>; Gao, Zhichao <zhichao.gao@...>
Subject: RE: Patch List for 202002 stable tag

 

Vitaly:

  I add my comments.

 

Zhichao and Ray:

   Can you give your opinion for BZ https://bugzilla.tianocore.org/show_bug.cgi?id=2510? Is it a bug fix or feature enhancement?

 

Thanks

Liming

From: vit9696 <vit9696@...>
Sent: Thursday, February 20, 2020 2:09 AM
To: Gao, Liming <liming.gao@...>
Cc: Kinney, Michael D <michael.d.kinney@...>; Laszlo Ersek <lersek@...>; Guptha, Soumya K <soumya.k.guptha@...>; leif@...; afish@...; devel@edk2.groups.io; Marvin Häuser <marvin.haeuser@...>
Subject: Re: Patch List for 202002 stable tag

 

Liming,

 

Thanks for pinging me about this!

 

With the PCD[1][2] I fully agree. The fact that it did not manage to land is mainly due to a sudden discussion that arose after complete silence for almost half a year, which was sort of unexpected. I will use this message as a suggestion to include this change as one of the primary goals for 202005 and kindly ask others to help to agree on the actual implementation. This bug strongly concerns us and we believe the fact that it does not (yet) cause issues to everyone is mainly coincidence.

 

[Liming] You can also present the topic in Tiano Design meeting to collect the feedback. Ni, Ray is the meeting host. You can send the topic to him.

 

With the Shell patch, the fact that I cannot enter upper case letters or use hotkeys in the editor sounds like a bug to me. The way the actual commit message is written reflects the change of the internal logic in the codebase (it adds support of specific behaviour handling on the target). In my opinion, it should not necessarily include the word «Fix» to be qualified as a bugfix, this is what bugzilla is for.

 

[Liming] If this fix is the bug, I agree it follows the process to catch this stable tag. I include ShellPkg maintainers (Ray Ni and Zhichao Gao) to give the opinion for the bug or not.

 

I am personally ok with deferring it to a next stable tag, but if the reasoning for this is «Feature planning freeze» dates, they do not strictly apply due to the reasons I stated above. So far the patch received only one review comment, which in fact was due to a minor misinterpretation. We also did some fairly extensive testing on our side before the submission (that’s why it actually took us a few more days). Unless the team has a lot of important work for the release, we can postpone the merge, otherwise I think it should be safe to merge this.

 

Best wishes,

Vitaly

 

 

 

19 февр. 2020 г., в 18:39, Gao, Liming <liming.gao@...> написал(а):

 


Mike and Laszlo:
 Thanks for your comments.

Vitaly:
 You request below two patches to catch 202002 stable tag. I agree with Mike and Laszlo comments. They are not ready to catch this stable tag. The first one is under discussion. The second one is like the enhancement or feature instead of the bug fix. It is submitted after Feb 7th Feature Planning Freeze. So, I suggest to defer them to next stable tag 202005.

https://edk2.groups.io/g/devel/topic/patch_v3_0_1_add_pcd_to/69401948 [PATCH v3 0/1] Add PCD to disable safe string constraint assertions (solution under discussion)
https://edk2.groups.io/g/devel/message/54122 [PATCH 1/1] ShellPkg: Add support for input with separately reported modifiers (under review, is this a feature or bug in the discussion)

Thanks
Liming

-----Original Message-----
From: Kinney, Michael D <michael.d.kinney@...>
Sent: Wednesday, February 19, 2020 4:43 AM
To: Laszlo Ersek <lersek@...>; Gao, Liming <liming.gao@...>; Guptha, Soumya K <soumya.k.guptha@...>;
leif@...; afish@...; Kinney, Michael D <michael.d.kinney@...>
Cc: devel@edk2.groups.io
Subject: RE: Patch List for 202002 stable tag

Hi Laszlo,

I agree with your assessments.

One comment below.

Mike

-----Original Message-----
From: Laszlo Ersek <lersek@...>
Sent: Tuesday, February 18, 2020 12:04 PM
To: Gao, Liming <liming.gao@...>; Guptha, Soumya
K <soumya.k.guptha@...>; Kinney, Michael D
<michael.d.kinney@...>; leif@...;
afish@...
Cc: devel@edk2.groups.io
Subject: Re: Patch List for 202002 stable tag

On 02/18/20 15:08, Gao, Liming wrote:

Hi Stewards and all:
 I collect current patch lists in devel mail list.

Those patch

 contributors request to add them for 201902 stable

tag. Because we

 have enter into Soft Feature Freeze, I want to

collect your feedback

 for them. If any patches are missing, please reply

this mail to add

 them.

Feature List (under review):


According to
<https://github.com/tianocore/tianocore.github.io/wiki/
SoftFeatureFreeze
>,
features can be merged during the SFF if their review
completed before
the SFF.

The SFF date is 2020-02-14 00:00:00 UTC-8, per
<https://github.com/tianocore/tianocore.github.io/wiki/
EDK-II-Release-Planning
>.
For me (in CET = UTC+1), that makes the deadline 2020-
02-14 09:00:00
CET.

 

https://edk2.groups.io/g/devel/topic/patch_v3_0_1_add_p
cd_to/69401948

[PATCH v3 0/1] Add PCD to disable safe string

constraint assertions

(solution under discussion)


Posted on 2020-01-03. Review doesn't appear complete.
Technically
speaking, it has missed edk2-stable202002.

There were two large gaps in the review process, namely
between these
messages:

- https://edk2.groups.io/g/devel/message/53026 [2020-
01-08]
- https://edk2.groups.io/g/devel/message/53485 [2020-
01-27]
- https://edk2.groups.io/g/devel/message/54133 [2020-
02-10]

If review seems stuck, it's advisable to ping once per
week, or a bit
more frequently. Two weeks ore more between pings is
way too long.

https://edk2.groups.io/g/devel/message/54122 [PATCH

1/1] ShellPkg: Add

support for input with separately reported modifiers

(under review, is

this a feature or bug in the disucssion)


The subject starts with "Add support for...", so it's a
new feature, or
at least a feature-enablement.

Posted on 2020-02-10. Has not been reviewed yet,
AFAICT. Same situation
as above. (Missed edk2-stable202002, technically
speaking.)

Note: I don't have a personal preference either way.
I'm just pointing
out what the SFF definition formally dictates, in my
interpretation.

If we want to extend the freeze dates, I won't object.

Bug List (reviewed):
https://edk2.groups.io/g/devel/message/54416 [PATCH

v2 00/10] Fix

false negative issue in

DxeImageVerificationHandler(CVE-2019-14575)

Clearly a bug fix; it could go in even during the HFF
<https://github.com/tianocore/tianocore.github.io/wiki/
HardFeatureFreeze
>.

https://edk2.groups.io/g/devel/message/54523 [PATCH
v1][edk2-stable202002] MdeModulePkg/SdMmcPciHcDxe:

Fix double PciIo

Unmap in TRB creation (CVE-2019-14587)


Ditto.

https://edk2.groups.io/g/devel/message/54510 [PATCH

v6 0/2]

Enhancement and Fixes to BaseHashApiLib


Hm. I feel like I need some convincing that patch#1 --
"CryptoPkg/BaseHashApiLib: Align BaseHashApiLib with
TPM 2.0
Implementation" -- is *also* a bugfix (like patch#2).

That question matters because the reviews:

- https://edk2.groups.io/g/devel/message/54513
- https://edk2.groups.io/g/devel/message/54567

were not posted before the SFF.

... I guess it's OK.


The description of the bug does not emphasis that
this really is a bug fix.  There were additional
review comments from the CryptoPkg reviewers after
the initial review/commit of this feature.  These
changes address that feedback.  The alignment with
TPM 2.0 is to use an existing set of defines for
the hash algorithms instead of define yet another
set of defines.  Details in this thread:

https://edk2.groups.io/g/devel/topic/70960524#53733

 

https://edk2.groups.io/g/devel/message/53703 [PATCH

V2] UefiCpuPkg

RegisterCpuFeaturesLib: Match data type and format

specifier

Even if this were a feature, it could go in; the review
was posted in
time:
- https://edk2.groups.io/g/devel/message/53803

In fact I don't understand why it hasn't been merged
for more than a
week now!

https://edk2.groups.io/g/devel/message/53577 [PATCH

v1 1/1] ShellPkg:

acpiview: Remove duplicate ACPI structure size

definitions

Approved in time, regardless of bugfix vs. feature.
Should go in.

https://edk2.groups.io/g/devel/message/54192 [PATCH

v2 1/1] ShellPkg:

acpiview: Validate ACPI table 'Length' field


The review was posted past the SFF, but I agree this
looks like a
bugfix, so should be OK. (Supplying missing input
sanitization is
arguably a fix.)


Bug List (under review)
https://edk2.groups.io/g/devel/message/54361 [PATCH

1/1]

NetworkPkg/ArpDxe: Recycle invalid ARP packets(CVE-

2019-14559)

https://edk2.groups.io/g/devel/message/54569 [PATCH

v3]

NetworkPkg/Ip4Dxe: Check the received package length

(CVE-2019-14559)

CVE fixes can clearly go in during the HFF too.

https://edk2.groups.io/g/devel/message/54448 [PATCH

v1 1/1] ShellPkg:

acpiview: Prevent infinite loop if structure length

is 0

Similar to "ShellPkg: acpiview: Validate ACPI table
'Length' field";
should be OK.


Just my opinion, of course.

Thanks
Laszlo

 

 


[PATCH] MdeModulePkg/Application: Overflowed Array Index

GuoMinJ
 

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

Index should be off-by one than size of array, so when check
mUsbTypeString, the max index should less than size of array.

Signed-off-by: GuoMinJ <newexplorerj@...>
---
.../Application/SmiHandlerProfileInfo/SmiHandlerProfileInfo.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/MdeModulePkg/Application/SmiHandlerProfileInfo/SmiHandlerProfileInfo.c b/MdeModulePkg/Application/SmiHandlerProfileInfo/SmiHandlerProfileInfo.c
index 0f7163160b..f8afcd6f96 100644
--- a/MdeModulePkg/Application/SmiHandlerProfileInfo/SmiHandlerProfileInfo.c
+++ b/MdeModulePkg/Application/SmiHandlerProfileInfo/SmiHandlerProfileInfo.c
@@ -508,7 +508,7 @@ UsbTypeToString (
IN EFI_USB_SMI_TYPE Type
)
{
- if (Type >= 0 && Type <= ARRAY_SIZE(mUsbTypeString)) {
+ if (Type >= 0 && Type < ARRAY_SIZE(mUsbTypeString)) {
return mUsbTypeString[Type];
} else {
AsciiSPrint (mNameString, sizeof(mNameString), "0x%x", Type);
--
2.17.1


Re: [PATCH v4 1/1] NetworkPkg/ArpDxe: Recycle invalid ARP packets (CVE-2019-14559)

Liming Gao
 

This patch is for BZ https://bugzilla.tianocore.org/show_bug.cgi?id=2031

So, please add this line into the commit message. This can be done when submit this patch. Don't need to send the patch again.

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

Thanks
Liming

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Siyuan, Fu
Sent: Thursday, February 20, 2020 8:31 AM
To: Armour, Nicholas <nicholas.armour@...>; devel@edk2.groups.io
Cc: Wu, Jiaxin <jiaxin.wu@...>; Maciej Rabeda <maciej.rabeda@...>
Subject: Re: [edk2-devel] [PATCH v4 1/1] NetworkPkg/ArpDxe: Recycle invalid ARP packets (CVE-2019-14559)

Reviewed-by: Siyuan Fu <siyuan.fu@...>

-----Original Message-----
From: Armour, Nicholas <nicholas.armour@...>
Sent: 2020年2月20日 8:23
To: devel@edk2.groups.io
Cc: Armour, Nicholas <nicholas.armour@...>; Wu, Jiaxin
<jiaxin.wu@...>; Maciej Rabeda <maciej.rabeda@...>; Fu,
Siyuan <siyuan.fu@...>
Subject: [PATCH v4 1/1] NetworkPkg/ArpDxe: Recycle invalid ARP packets
(CVE-2019-14559)

Update copyright

Cc: Jiaxin Wu <jiaxin.wu@...>
Cc: Maciej Rabeda <maciej.rabeda@...>
Cc: Siyuan Fu <siyuan.fu@...>
Signed-off-by: Nicholas Armour <nicholas.armour@...>
---
NetworkPkg/ArpDxe/ArpImpl.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/NetworkPkg/ArpDxe/ArpImpl.c b/NetworkPkg/ArpDxe/ArpImpl.c
index 9cdb33f2bd66..ed2d756d3e17 100644
--- a/NetworkPkg/ArpDxe/ArpImpl.c
+++ b/NetworkPkg/ArpDxe/ArpImpl.c
@@ -1,7 +1,7 @@
/** @file
The implementation of the ARP protocol.

-Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2006 - 2020, Intel Corporation. All rights reserved.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent

**/
@@ -113,7 +113,7 @@ ArpOnFrameRcvdDpc (
//
// Restart the receiving if packet size is not correct.
//
- goto RESTART_RECEIVE;
+ goto RECYCLE_RXDATA;
}

//
@@ -125,7 +125,7 @@ ArpOnFrameRcvdDpc (
Head->OpCode = NTOHS (Head->OpCode);

if (RxData->DataLength < (sizeof (ARP_HEAD) + 2 * Head->HwAddrLen + 2 *
Head->ProtoAddrLen)) {
- goto RESTART_RECEIVE;
+ goto RECYCLE_RXDATA;
}

if ((Head->HwType != ArpService->SnpMode.IfType) ||
--
2.16.2.windows.1


Re: [PATCH v2 1/1] ShellPkg: acpiview: Prevent infinite loop if structure length is 0

Gao, Zhichao
 

Since there is no change for the V2 patch. It is a good behavior to add the R-B from V1 to V2. That would make the reviewer aware that there is no code change and review requirement for this patch. I would keep the R-B in V1 and help to merge the patch.

Thanks,
Zhichao

-----Original Message-----
From: Krzysztof Koch <krzysztof.koch@...>
Sent: Wednesday, February 19, 2020 6:24 PM
To: devel@edk2.groups.io
Cc: Ni, Ray <ray.ni@...>; Gao, Zhichao <zhichao.gao@...>;
Sami.Mujawar@...; Matteo.Carlini@...; nd@...
Subject: [PATCH v2 1/1] ShellPkg: acpiview: Prevent infinite loop if structure
length is 0

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

Extend validation of ACPI structure lengths which are read from the ACPI table
being parsed. Additionally check if the structure 'Length'
field value is positive. If not, stop parsing the faulting table.

Some ACPI tables define internal structures of variable size. The 'Length' field
inside the substructure is used to update a pointer used for table traversal. If the
byte-length of the structure is equal to 0, acpiview can enter an infinite loop.
This condition can occur if, for example, the zero-allocated ACPI table buffer is
not fully populated.
This is typically a bug on the ACPI table writer side.

In short, this method helps acpiview recover gracefully from a zero-valued ACPI
structure length.

Signed-off-by: Krzysztof Koch <krzysztof.koch@...>
---

Changes can be seen at:
https://github.com/KrzysztofKoch1/edk2/tree/612_acpiview_prevent_inf_loops
_v2

Notes:
v2:
- Add BZ link to the commit message [Zhichao]

v1:
- prevent infinite loops in acpiview parsers [Krzysztof]

ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c |
15 ++++++-----
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser.c | 13
++++-----
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser.c | 14
+++++-----
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.c |
28 ++++++--------------
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c | 15
++++++-----
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser.c | 14
+++++-----
6 files changed, 47 insertions(+), 52 deletions(-)

diff --git
a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c
b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c
index
0f730a306a94329a23fbaf54b59f1833b44616ba..9df111ecaa7d7a703a13a39c24
3ed78b9f12ee97 100644
---
a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Pars
+++ er.c
@@ -1,7 +1,7 @@
/** @file
DBG2 table parser

- Copyright (c) 2016 - 2019, ARM Limited. All rights reserved.
+ Copyright (c) 2016 - 2020, ARM Limited. All rights reserved.
SPDX-License-Identifier: BSD-2-Clause-Patent

@par Reference(s):
@@ -282,15 +282,16 @@ ParseAcpiDbg2 (
return;
}

- // Make sure the Debug Device Information structure lies inside the table.
- if ((Offset + *DbgDevInfoLen) > AcpiTableLength) {
+ // Validate Debug Device Information Structure length
+ if ((*DbgDevInfoLen == 0) ||
+ ((Offset + (*DbgDevInfoLen)) > AcpiTableLength)) {
IncrementErrorCount ();
Print (
- L"ERROR: Invalid Debug Device Information structure length. " \
- L"DbgDevInfoLen = %d. RemainingTableBufferLength = %d. " \
- L"DBG2 parsing aborted.\n",
+ L"ERROR: Invalid Debug Device Information Structure length. " \
+ L"Length = %d. Offset = %d. AcpiTableLength = %d.\n",
*DbgDevInfoLen,
- AcpiTableLength - Offset
+ Offset,
+ AcpiTableLength
);
return;
}
diff --git
a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser.c
b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser.c
index
699a55b549ec3fa61bbd156898821055dc019199..bdd30ff45c61142c071ead63a
27babab8998721b 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtPars
+++ er.c
@@ -1,7 +1,7 @@
/** @file
GTDT table parser

- Copyright (c) 2016 - 2019, ARM Limited. All rights reserved.
+ Copyright (c) 2016 - 2020, ARM Limited. All rights reserved.
SPDX-License-Identifier: BSD-2-Clause-Patent

@par Reference(s):
@@ -327,15 +327,16 @@ ParseAcpiGtdt (
return;
}

- // Make sure the Platform Timer is inside the table.
- if ((Offset + *PlatformTimerLength) > AcpiTableLength) {
+ // Validate Platform Timer Structure length
+ if ((*PlatformTimerLength == 0) ||
+ ((Offset + (*PlatformTimerLength)) > AcpiTableLength)) {
IncrementErrorCount ();
Print (
L"ERROR: Invalid Platform Timer Structure length. " \
- L"PlatformTimerLength = %d. RemainingTableBufferLength = %d. " \
- L"GTDT parsing aborted.\n",
+ L"Length = %d. Offset = %d. AcpiTableLength = %d.\n",
*PlatformTimerLength,
- AcpiTableLength - Offset
+ Offset,
+ AcpiTableLength
);
return;
}
diff --git
a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser.c
b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser.c
index
9d5d937c7b2c19945ca2ad3eba644bdfc09cc3f6..9a006a01448b897865cd7cd85
651c816933acf05 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortPars
+++ er.c
@@ -1,7 +1,7 @@
/** @file
IORT table parser

- Copyright (c) 2016 - 2019, ARM Limited. All rights reserved.
+ Copyright (c) 2016 - 2020, ARM Limited. All rights reserved.
SPDX-License-Identifier: BSD-2-Clause-Patent

@par Reference(s):
@@ -687,14 +687,16 @@ ParseAcpiIort (
return;
}

- // Make sure the IORT Node is inside the table
- if ((Offset + (*IortNodeLength)) > AcpiTableLength) {
+ // Validate IORT Node length
+ if ((*IortNodeLength == 0) ||
+ ((Offset + (*IortNodeLength)) > AcpiTableLength)) {
IncrementErrorCount ();
Print (
- L"ERROR: Invalid IORT node length. IortNodeLength = %d. " \
- L"RemainingTableBufferLength = %d. IORT parsing aborted.\n",
+ L"ERROR: Invalid IORT Node length. " \
+ L"Length = %d. Offset = %d. AcpiTableLength = %d.\n",
*IortNodeLength,
- AcpiTableLength - Offset
+ Offset,
+ AcpiTableLength
);
return;
}
diff --git
a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.c
b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.c
index
438905cb24f58b8b82e8fe61280e72f765d578d8..f85d2b36532cfc5db36fe7bef9
830cccc64969cc 100644
---
a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtPars
+++ er.c
@@ -1,7 +1,7 @@
/** @file
MADT table parser

- Copyright (c) 2016 - 2019, ARM Limited. All rights reserved.
+ Copyright (c) 2016 - 2020, ARM Limited. All rights reserved.
SPDX-License-Identifier: BSD-2-Clause-Patent

@par Reference(s):
@@ -273,28 +273,16 @@ ParseAcpiMadt (
return;
}

- // Make sure forward progress is made.
- if (*MadtInterruptControllerLength < 2) {
+ // Validate Interrupt Controller Structure length
+ if ((*MadtInterruptControllerLength == 0) ||
+ ((Offset + (*MadtInterruptControllerLength)) >
+ AcpiTableLength)) {
IncrementErrorCount ();
Print (
- L"ERROR: Structure length is too small: " \
- L"MadtInterruptControllerLength = %d. " \
- L"MadtInterruptControllerType = %d. MADT parsing aborted.\n",
+ L"ERROR: Invalid Interrupt Controller Structure length. " \
+ L"Length = %d. Offset = %d. AcpiTableLength = %d.\n",
*MadtInterruptControllerLength,
- *MadtInterruptControllerType
- );
- return;
- }
-
- // Make sure the MADT structure lies inside the table
- if ((Offset + *MadtInterruptControllerLength) > AcpiTableLength) {
- IncrementErrorCount ();
- Print (
- L"ERROR: Invalid MADT structure length. " \
- L"MadtInterruptControllerLength = %d. " \
- L"RemainingTableBufferLength = %d. MADT parsing aborted.\n",
- *MadtInterruptControllerLength,
- AcpiTableLength - Offset
+ Offset,
+ AcpiTableLength
);
return;
}
diff --git
a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c
b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c
index
675ba75f02b367cd5ad9f2ac23c30ed0ab58f286..0db272c16af0ad8824c8da4c88
dd409c8550112a 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttPars
+++ er.c
@@ -1,7 +1,7 @@
/** @file
PPTT table parser

- Copyright (c) 2019, ARM Limited. All rights reserved.
+ Copyright (c) 2019 - 2020, ARM Limited. All rights reserved.
SPDX-License-Identifier: BSD-2-Clause-Patent

@par Reference(s):
@@ -425,15 +425,16 @@ ParseAcpiPptt (
return;
}

- // Make sure the PPTT structure lies inside the table
- if ((Offset + *ProcessorTopologyStructureLength) > AcpiTableLength) {
+ // Validate Processor Topology Structure length
+ if ((*ProcessorTopologyStructureLength == 0) ||
+ ((Offset + (*ProcessorTopologyStructureLength)) >
+ AcpiTableLength)) {
IncrementErrorCount ();
Print (
- L"ERROR: Invalid PPTT structure length. " \
- L"ProcessorTopologyStructureLength = %d. " \
- L"RemainingTableBufferLength = %d. PPTT parsing aborted.\n",
+ L"ERROR: Invalid Processor Topology Structure length. " \
+ L"Length = %d. Offset = %d. AcpiTableLength = %d.\n",
*ProcessorTopologyStructureLength,
- AcpiTableLength - Offset
+ Offset,
+ AcpiTableLength
);
return;
}
diff --git
a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser.c
b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser.c
index
3613900ae322483fdd3d3383de4e22ba75b2128b..6f66be68cc0bed14811a0432c
61a79fd47c54890 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratPars
+++ er.c
@@ -1,7 +1,7 @@
/** @file
SRAT table parser

- Copyright (c) 2016 - 2019, ARM Limited. All rights reserved.
+ Copyright (c) 2016 - 2020, ARM Limited. All rights reserved.
SPDX-License-Identifier: BSD-2-Clause-Patent

@par Reference(s):
@@ -412,14 +412,16 @@ ParseAcpiSrat (
return;
}

- // Make sure the SRAT structure lies inside the table
- if ((Offset + *SratRALength) > AcpiTableLength) {
+ // Validate Static Resource Allocation Structure length
+ if ((*SratRALength == 0) ||
+ ((Offset + (*SratRALength)) > AcpiTableLength)) {
IncrementErrorCount ();
Print (
- L"ERROR: Invalid SRAT structure length. SratRALength = %d. " \
- L"RemainingTableBufferLength = %d. SRAT parsing aborted.\n",
+ L"ERROR: Invalid Static Resource Allocation Structure length. " \
+ L"Length = %d. Offset = %d. AcpiTableLength = %d.\n",
*SratRALength,
- AcpiTableLength - Offset
+ Offset,
+ AcpiTableLength
);
return;
}
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'


Re: [EXT] RE: [edk2-devel] [PATCH v2 1/1] EmbeddedPkg: Fixed Asserts in SCT Runtime Services test.

Liming Gao
 

Gaurav:
Got it.

Lefi and Ard:
Can you review this patch for stable tag 202002?

Thanks
Liming

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Gaurav Jain
Sent: Thursday, February 20, 2020 1:19 PM
To: Gao, Liming <liming.gao@...>; devel@edk2.groups.io
Cc: Leif Lindholm <leif@...>; Ard Biesheuvel <ard.biesheuvel@...>; Pankaj Bansal <pankaj.bansal@...>
Subject: Re: [EXT] RE: [edk2-devel] [PATCH v2 1/1] EmbeddedPkg: Fixed Asserts in SCT Runtime Services test.

Gaurav:
Does this patch catch to edk2 stable tag 202002?
Yes

Thanks
Gaurav

-----Original Message-----
From: Gao, Liming <liming.gao@...>
Sent: Wednesday, February 19, 2020 2:36 PM
To: devel@edk2.groups.io; Gaurav Jain <gaurav.jain@...>
Cc: Leif Lindholm <leif@...>; Ard Biesheuvel
<ard.biesheuvel@...>; Pankaj Bansal <pankaj.bansal@...>
Subject: [EXT] RE: [edk2-devel] [PATCH v2 1/1] EmbeddedPkg: Fixed Asserts in
SCT Runtime Services test.

Caution: EXT Email

Gaurav:
Does this patch catch to edk2 stable tag 202002?

Thanks
Liming
-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Gaurav
Jain
Sent: Tuesday, February 18, 2020 8:24 PM
To: devel@edk2.groups.io
Cc: Leif Lindholm <leif@...>; Ard Biesheuvel
<ard.biesheuvel@...>; Pankaj Bansal <pankaj.bansal@...>;
Gaurav Jain <gaurav.jain@...>
Subject: [edk2-devel] [PATCH v2 1/1] EmbeddedPkg: Fixed Asserts in SCT
Runtime Services test.

ASSERT in SetTime_Conf and SetWakeupTime_Conf Consistency Test.
SCT Test expect return as Invalid Parameter.
So removed ASSERT().

Added Time Validity Checks in SetWakeupTime.

Signed-off-by: Gaurav Jain <gaurav.jain@...>
---
Changes in v2:
- reverted changes related to valid range of years.
---
EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClock.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClock.c
b/EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClock.c
index 08fb9b0100b6..70a0d78125b9 100644
--- a/EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClock.c
+++ b/EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClock.c
@@ -85,10 +85,6 @@ IsDayValid (
IN EFI_TIME *Time
)
{
- ASSERT (Time->Day >= 1);
- ASSERT (Time->Day <= mDayOfMonth[Time->Month - 1]);
- ASSERT (Time->Month != 2 || IsLeapYear (Time) || Time->Day <= 28);
-
if (Time->Day < 1 ||
Time->Day > mDayOfMonth[Time->Month - 1] ||
(Time->Month == 2 && !IsLeapYear (Time) && Time->Day > 28)) {
@@ -113,6 +109,7 @@ IsTimeValid(
Time->Hour > 23 ||
Time->Minute > 59 ||
Time->Second > 59 ||
+ Time->Nanosecond > 999999999 ||
!IsValidTimeZone (Time->TimeZone) ||
!IsValidDaylight (Time->Daylight)) {
return FALSE;
@@ -254,6 +251,9 @@ SetWakeupTime (
OUT EFI_TIME *Time
)
{
+ if (Time == NULL || !IsTimeValid (Time)) {
+ return EFI_INVALID_PARAMETER;
+ }
return LibSetWakeupTime (Enabled, Time); }

--
2.17.1




Re: [EXT] RE: [edk2-devel] [PATCH v2 1/1] EmbeddedPkg: Fixed Asserts in SCT Runtime Services test.

Gaurav Jain
 

Gaurav:
Does this patch catch to edk2 stable tag 202002?
Yes

Thanks
Gaurav

-----Original Message-----
From: Gao, Liming <liming.gao@...>
Sent: Wednesday, February 19, 2020 2:36 PM
To: devel@edk2.groups.io; Gaurav Jain <gaurav.jain@...>
Cc: Leif Lindholm <leif@...>; Ard Biesheuvel
<ard.biesheuvel@...>; Pankaj Bansal <pankaj.bansal@...>
Subject: [EXT] RE: [edk2-devel] [PATCH v2 1/1] EmbeddedPkg: Fixed Asserts in
SCT Runtime Services test.

Caution: EXT Email

Gaurav:
Does this patch catch to edk2 stable tag 202002?

Thanks
Liming
-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Gaurav
Jain
Sent: Tuesday, February 18, 2020 8:24 PM
To: devel@edk2.groups.io
Cc: Leif Lindholm <leif@...>; Ard Biesheuvel
<ard.biesheuvel@...>; Pankaj Bansal <pankaj.bansal@...>;
Gaurav Jain <gaurav.jain@...>
Subject: [edk2-devel] [PATCH v2 1/1] EmbeddedPkg: Fixed Asserts in SCT
Runtime Services test.

ASSERT in SetTime_Conf and SetWakeupTime_Conf Consistency Test.
SCT Test expect return as Invalid Parameter.
So removed ASSERT().

Added Time Validity Checks in SetWakeupTime.

Signed-off-by: Gaurav Jain <gaurav.jain@...>
---
Changes in v2:
- reverted changes related to valid range of years.
---
EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClock.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClock.c
b/EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClock.c
index 08fb9b0100b6..70a0d78125b9 100644
--- a/EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClock.c
+++ b/EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClock.c
@@ -85,10 +85,6 @@ IsDayValid (
IN EFI_TIME *Time
)
{
- ASSERT (Time->Day >= 1);
- ASSERT (Time->Day <= mDayOfMonth[Time->Month - 1]);
- ASSERT (Time->Month != 2 || IsLeapYear (Time) || Time->Day <= 28);
-
if (Time->Day < 1 ||
Time->Day > mDayOfMonth[Time->Month - 1] ||
(Time->Month == 2 && !IsLeapYear (Time) && Time->Day > 28)) {
@@ -113,6 +109,7 @@ IsTimeValid(
Time->Hour > 23 ||
Time->Minute > 59 ||
Time->Second > 59 ||
+ Time->Nanosecond > 999999999 ||
!IsValidTimeZone (Time->TimeZone) ||
!IsValidDaylight (Time->Daylight)) {
return FALSE;
@@ -254,6 +251,9 @@ SetWakeupTime (
OUT EFI_TIME *Time
)
{
+ if (Time == NULL || !IsTimeValid (Time)) {
+ return EFI_INVALID_PARAMETER;
+ }
return LibSetWakeupTime (Enabled, Time); }

--
2.17.1



Re: Patch List for 202002 stable tag

Ni, Ray
 

Liming,

I provided my comments in the BZ.

 

From: Gao, Liming <liming.gao@...>
Sent: Thursday, February 20, 2020 9:17 AM
To: vit9696 <vit9696@...>
Cc: Kinney, Michael D <michael.d.kinney@...>; Laszlo Ersek <lersek@...>; Guptha, Soumya K <soumya.k.guptha@...>; leif@...; afish@...; devel@edk2.groups.io; Marvin Häuser <marvin.haeuser@...>; Ni, Ray <ray.ni@...>; Gao, Zhichao <zhichao.gao@...>
Subject: RE: Patch List for 202002 stable tag

 

Vitaly:

  I add my comments.

 

Zhichao and Ray:

   Can you give your opinion for BZ https://bugzilla.tianocore.org/show_bug.cgi?id=2510? Is it a bug fix or feature enhancement?

 

Thanks

Liming

From: vit9696 <vit9696@...>
Sent: Thursday, February 20, 2020 2:09 AM
To: Gao, Liming <liming.gao@...>
Cc: Kinney, Michael D <michael.d.kinney@...>; Laszlo Ersek <lersek@...>; Guptha, Soumya K <soumya.k.guptha@...>; leif@...; afish@...; devel@edk2.groups.io; Marvin Häuser <marvin.haeuser@...>
Subject: Re: Patch List for 202002 stable tag

 

Liming,

 

Thanks for pinging me about this!

 

With the PCD[1][2] I fully agree. The fact that it did not manage to land is mainly due to a sudden discussion that arose after complete silence for almost half a year, which was sort of unexpected. I will use this message as a suggestion to include this change as one of the primary goals for 202005 and kindly ask others to help to agree on the actual implementation. This bug strongly concerns us and we believe the fact that it does not (yet) cause issues to everyone is mainly coincidence.

 

[Liming] You can also present the topic in Tiano Design meeting to collect the feedback. Ni, Ray is the meeting host. You can send the topic to him.

 

With the Shell patch, the fact that I cannot enter upper case letters or use hotkeys in the editor sounds like a bug to me. The way the actual commit message is written reflects the change of the internal logic in the codebase (it adds support of specific behaviour handling on the target). In my opinion, it should not necessarily include the word «Fix» to be qualified as a bugfix, this is what bugzilla is for.

 

[Liming] If this fix is the bug, I agree it follows the process to catch this stable tag. I include ShellPkg maintainers (Ray Ni and Zhichao Gao) to give the opinion for the bug or not.

 

I am personally ok with deferring it to a next stable tag, but if the reasoning for this is «Feature planning freeze» dates, they do not strictly apply due to the reasons I stated above. So far the patch received only one review comment, which in fact was due to a minor misinterpretation. We also did some fairly extensive testing on our side before the submission (that’s why it actually took us a few more days). Unless the team has a lot of important work for the release, we can postpone the merge, otherwise I think it should be safe to merge this.

 

Best wishes,

Vitaly

 

 

 

19 февр. 2020 г., в 18:39, Gao, Liming <liming.gao@...> написал(а):

 


Mike and Laszlo:
 Thanks for your comments.

Vitaly:
 You request below two patches to catch 202002 stable tag. I agree with Mike and Laszlo comments. They are not ready to catch this stable tag. The first one is under discussion. The second one is like the enhancement or feature instead of the bug fix. It is submitted after Feb 7th Feature Planning Freeze. So, I suggest to defer them to next stable tag 202005.

https://edk2.groups.io/g/devel/topic/patch_v3_0_1_add_pcd_to/69401948 [PATCH v3 0/1] Add PCD to disable safe string constraint assertions (solution under discussion)
https://edk2.groups.io/g/devel/message/54122 [PATCH 1/1] ShellPkg: Add support for input with separately reported modifiers (under review, is this a feature or bug in the discussion)

Thanks
Liming

-----Original Message-----
From: Kinney, Michael D <michael.d.kinney@...>
Sent: Wednesday, February 19, 2020 4:43 AM
To: Laszlo Ersek <lersek@...>; Gao, Liming <liming.gao@...>; Guptha, Soumya K <soumya.k.guptha@...>;
leif@...; afish@...; Kinney, Michael D <michael.d.kinney@...>
Cc: devel@edk2.groups.io
Subject: RE: Patch List for 202002 stable tag

Hi Laszlo,

I agree with your assessments.

One comment below.

Mike

-----Original Message-----
From: Laszlo Ersek <lersek@...>
Sent: Tuesday, February 18, 2020 12:04 PM
To: Gao, Liming <liming.gao@...>; Guptha, Soumya
K <soumya.k.guptha@...>; Kinney, Michael D
<michael.d.kinney@...>; leif@...;
afish@...
Cc: devel@edk2.groups.io
Subject: Re: Patch List for 202002 stable tag

On 02/18/20 15:08, Gao, Liming wrote:

Hi Stewards and all:
 I collect current patch lists in devel mail list.

Those patch

 contributors request to add them for 201902 stable

tag. Because we

 have enter into Soft Feature Freeze, I want to

collect your feedback

 for them. If any patches are missing, please reply

this mail to add

 them.

Feature List (under review):


According to
<https://github.com/tianocore/tianocore.github.io/wiki/
SoftFeatureFreeze
>,
features can be merged during the SFF if their review
completed before
the SFF.

The SFF date is 2020-02-14 00:00:00 UTC-8, per
<https://github.com/tianocore/tianocore.github.io/wiki/
EDK-II-Release-Planning
>.
For me (in CET = UTC+1), that makes the deadline 2020-
02-14 09:00:00
CET.

 

https://edk2.groups.io/g/devel/topic/patch_v3_0_1_add_p
cd_to/69401948

[PATCH v3 0/1] Add PCD to disable safe string

constraint assertions

(solution under discussion)


Posted on 2020-01-03. Review doesn't appear complete.
Technically
speaking, it has missed edk2-stable202002.

There were two large gaps in the review process, namely
between these
messages:

- https://edk2.groups.io/g/devel/message/53026 [2020-
01-08]
- https://edk2.groups.io/g/devel/message/53485 [2020-
01-27]
- https://edk2.groups.io/g/devel/message/54133 [2020-
02-10]

If review seems stuck, it's advisable to ping once per
week, or a bit
more frequently. Two weeks ore more between pings is
way too long.

https://edk2.groups.io/g/devel/message/54122 [PATCH

1/1] ShellPkg: Add

support for input with separately reported modifiers

(under review, is

this a feature or bug in the disucssion)


The subject starts with "Add support for...", so it's a
new feature, or
at least a feature-enablement.

Posted on 2020-02-10. Has not been reviewed yet,
AFAICT. Same situation
as above. (Missed edk2-stable202002, technically
speaking.)

Note: I don't have a personal preference either way.
I'm just pointing
out what the SFF definition formally dictates, in my
interpretation.

If we want to extend the freeze dates, I won't object.

Bug List (reviewed):
https://edk2.groups.io/g/devel/message/54416 [PATCH

v2 00/10] Fix

false negative issue in

DxeImageVerificationHandler(CVE-2019-14575)

Clearly a bug fix; it could go in even during the HFF
<https://github.com/tianocore/tianocore.github.io/wiki/
HardFeatureFreeze
>.

https://edk2.groups.io/g/devel/message/54523 [PATCH
v1][edk2-stable202002] MdeModulePkg/SdMmcPciHcDxe:

Fix double PciIo

Unmap in TRB creation (CVE-2019-14587)


Ditto.

https://edk2.groups.io/g/devel/message/54510 [PATCH

v6 0/2]

Enhancement and Fixes to BaseHashApiLib


Hm. I feel like I need some convincing that patch#1 --
"CryptoPkg/BaseHashApiLib: Align BaseHashApiLib with
TPM 2.0
Implementation" -- is *also* a bugfix (like patch#2).

That question matters because the reviews:

- https://edk2.groups.io/g/devel/message/54513
- https://edk2.groups.io/g/devel/message/54567

were not posted before the SFF.

... I guess it's OK.


The description of the bug does not emphasis that
this really is a bug fix.  There were additional
review comments from the CryptoPkg reviewers after
the initial review/commit of this feature.  These
changes address that feedback.  The alignment with
TPM 2.0 is to use an existing set of defines for
the hash algorithms instead of define yet another
set of defines.  Details in this thread:

https://edk2.groups.io/g/devel/topic/70960524#53733


 

https://edk2.groups.io/g/devel/message/53703 [PATCH

V2] UefiCpuPkg

RegisterCpuFeaturesLib: Match data type and format

specifier

Even if this were a feature, it could go in; the review
was posted in
time:
- https://edk2.groups.io/g/devel/message/53803

In fact I don't understand why it hasn't been merged
for more than a
week now!

https://edk2.groups.io/g/devel/message/53577 [PATCH

v1 1/1] ShellPkg:

acpiview: Remove duplicate ACPI structure size

definitions

Approved in time, regardless of bugfix vs. feature.
Should go in.

https://edk2.groups.io/g/devel/message/54192 [PATCH

v2 1/1] ShellPkg:

acpiview: Validate ACPI table 'Length' field


The review was posted past the SFF, but I agree this
looks like a
bugfix, so should be OK. (Supplying missing input
sanitization is
arguably a fix.)


Bug List (under review)
https://edk2.groups.io/g/devel/message/54361 [PATCH

1/1]

NetworkPkg/ArpDxe: Recycle invalid ARP packets(CVE-

2019-14559)

https://edk2.groups.io/g/devel/message/54569 [PATCH

v3]

NetworkPkg/Ip4Dxe: Check the received package length

(CVE-2019-14559)

CVE fixes can clearly go in during the HFF too.

https://edk2.groups.io/g/devel/message/54448 [PATCH

v1 1/1] ShellPkg:

acpiview: Prevent infinite loop if structure length

is 0

Similar to "ShellPkg: acpiview: Validate ACPI table
'Length' field";
should be OK.


Just my opinion, of course.

Thanks
Laszlo

 

 


Re: [EXTERNAL] [edk2-devel] [PATCH] UnitTestFrameworkPkg: Suspicious check for pointer Suite

Bret Barkelew
 

Reviewed-by: Bret Barkelew <bret.barkelew@...>

 

- Bret

 


From: devel@edk2.groups.io <devel@edk2.groups.io> on behalf of GuoMinJ via Groups.Io <newexplorerj@...>
Sent: Tuesday, February 18, 2020 6:34:49 PM
To: devel@edk2.groups.io <devel@edk2.groups.io>
Cc: GuoMinJ <newexplorerj@...>
Subject: [EXTERNAL] [edk2-devel] [PATCH] UnitTestFrameworkPkg: Suspicious check for pointer Suite
 
REF: https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D2530&amp;data=02%7C01%7Cbret.barkelew%40microsoft.com%7Cc3d0da979cc14c97e3ab08d7b4e4b9d0%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637176766775311376&amp;sdata=eRhhSwb6DcYF9%2B%2BTnCe0wzBPvRHTgl0Ue%2BRZ7LCYtxU%3D&amp;reserved=0

The Suite pointer is used before check if it is valid,
correct it to check the validation before use.

Signed-off-by: GuoMinJ <newexplorerj@...>
---
 UnitTestFrameworkPkg/Library/UnitTestLib/RunTests.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/UnitTestFrameworkPkg/Library/UnitTestLib/RunTests.c b/UnitTestFrameworkPkg/Library/UnitTestLib/RunTests.c
index fb247c59e7..b053e04959 100644
--- a/UnitTestFrameworkPkg/Library/UnitTestLib/RunTests.c
+++ b/UnitTestFrameworkPkg/Library/UnitTestLib/RunTests.c
@@ -33,13 +33,13 @@ RunTestSuite (
   UNIT_TEST             *Test;
   UNIT_TEST_FRAMEWORK   *ParentFramework;
 
-  TestEntry       = NULL;
-  ParentFramework = (UNIT_TEST_FRAMEWORK *)Suite->ParentFramework;
-
   if (Suite == NULL) {
     return EFI_INVALID_PARAMETER;
   }
 
+  TestEntry       = NULL;
+  ParentFramework = (UNIT_TEST_FRAMEWORK *)Suite->ParentFramework;
+
   DEBUG ((DEBUG_VERBOSE, "---------------------------------------------------------\n"));
   DEBUG ((DEBUG_VERBOSE, "RUNNING TEST SUITE: %a\n", Suite->Title));
   DEBUG ((DEBUG_VERBOSE, "---------------------------------------------------------\n"));
--
2.17.1





Re: [EXTERNAL] Re: [edk2-devel] [Patch 1/2] UnitTestFrameworkPkg: Disable EBC in DSC file

Bret Barkelew
 

Reviewed-by: Bret Barkelew <bret.barkelew@...>

 

- Bret

 


From: Laszlo Ersek <lersek@...>
Sent: Tuesday, February 11, 2020 1:02:17 AM
To: devel@edk2.groups.io <devel@edk2.groups.io>; Kinney, Michael D <michael.d.kinney@...>
Cc: Sean Brogan <sean.brogan@...>; Bret Barkelew <Bret.Barkelew@...>
Subject: [EXTERNAL] Re: [edk2-devel] [Patch 1/2] UnitTestFrameworkPkg: Disable EBC in DSC file
 
On 02/10/20 21:24, Michael D Kinney wrote:
> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D2514&amp;data=02%7C01%7Cbret.barkelew%40microsoft.com%7Cabce6dc4e8984e69faae08d7aed12330%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637170085571112419&amp;sdata=J40%2BxMF1CTlCdsZMK4%2Fjtp2jKkSdunF1b07tU34BIk4%3D&amp;reserved=0
>
> Remove EBC as one of the supported architectures
> in the UnitTestFrameworkPkg DSC file.  The EBC
> compiler does not support cararg macros and the

small typo: s/cararg/vararg/

Thanks
Laszlo

> UnitTestLib class uses this feature.
>
> Cc: Sean Brogan <sean.brogan@...>
> Cc: Bret Barkelew <Bret.Barkelew@...>
> Signed-off-by: Michael D Kinney <michael.d.kinney@...>
> ---
>  UnitTestFrameworkPkg/UnitTestFrameworkPkg.dsc | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/UnitTestFrameworkPkg/UnitTestFrameworkPkg.dsc b/UnitTestFrameworkPkg/UnitTestFrameworkPkg.dsc
> index 0ad4482273..53d8f52754 100644
> --- a/UnitTestFrameworkPkg/UnitTestFrameworkPkg.dsc
> +++ b/UnitTestFrameworkPkg/UnitTestFrameworkPkg.dsc
> @@ -13,7 +13,7 @@ [Defines]
>    PLATFORM_VERSION        = 1.00
>    DSC_SPECIFICATION       = 0x00010005
>    OUTPUT_DIRECTORY        = Build/UnitTestFrameworkPkg
> -  SUPPORTED_ARCHITECTURES = IA32|X64|EBC|ARM|AARCH64
> +  SUPPORTED_ARCHITECTURES = IA32|X64|ARM|AARCH64
>    BUILD_TARGETS           = DEBUG|RELEASE|NOOPT
>    SKUID_IDENTIFIER        = DEFAULT

>