* [PATCH v3 01/11] ShellPkg: acpiview: Set ItemPtr to NULL for unprocessed table fields
2020-01-20 11:13 [PATCH v3 00/11] Test against invalid pointers in acpiview Krzysztof Koch
@ 2020-01-20 11:13 ` Krzysztof Koch
2020-01-20 11:13 ` [PATCH v3 02/11] ShellPkg: acpiview: RSDP: Validate global pointer before use Krzysztof Koch
` (10 subsequent siblings)
11 siblings, 0 replies; 13+ messages in thread
From: Krzysztof Koch @ 2020-01-20 11:13 UTC (permalink / raw)
To: devel; +Cc: ray.ni, zhichao.gao, Sami.Mujawar, Matteo.Carlini, nd
For fields outside the buffer length provided, reset any pointers,
which were supposed to be updated by a ParseAcpi() function call to
NULL. This way one can easily validate if a pointer was successfully
updated.
The ParseAcpi() function parses the given ACPI table buffer by a
number of bytes which is a minimum of the buffer length and the length
described by ACPI_PARSER array. If the buffer length is shorter than
the array describing how to process the ACPI structure, then it is
possible that the ItemPtr inside ACPI_PARSER may not get updated or
initialized. This can lead to an error if the value pointed to by
ItemPtr is later used to control the parsing logic.
A typical example would be a 'number of elements' field in an ACPI
structure header which defines how many substructures of a given type
are present in the structure body. If the 'number of elements' field
is not parsed, we will have a dangling pointer which could cause a
problem later.
Signed-off-by: Krzysztof Koch <krzysztof.koch@arm.com>
---
Notes:
v3:
- Rebase on latest master [Krzysztof]
v1:
- Set ItemPtr to NULL for unprocessed table fields [Krzysztof]
ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c
index 2b2ecb93cef9ee28b752e7bf2d920b059dbf7d6b..84c5f0468da55477acc96dfd0f949a5908d0f7a5 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c
@@ -543,8 +543,15 @@ ParseAcpi (
for (Index = 0; Index < ParserItems; Index++) {
if ((Offset + Parser[Index].Length) > Length) {
+
+ // For fields outside the buffer length provided, reset any pointers
+ // which were supposed to be updated by this function call
+ if (Parser[Index].ItemPtr != NULL) {
+ *Parser[Index].ItemPtr = NULL;
+ }
+
// We don't parse past the end of the max length specified
- break;
+ continue;
}
if (GetConsistencyChecking () &&
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v3 02/11] ShellPkg: acpiview: RSDP: Validate global pointer before use
2020-01-20 11:13 [PATCH v3 00/11] Test against invalid pointers in acpiview Krzysztof Koch
2020-01-20 11:13 ` [PATCH v3 01/11] ShellPkg: acpiview: Set ItemPtr to NULL for unprocessed table fields Krzysztof Koch
@ 2020-01-20 11:13 ` Krzysztof Koch
2020-01-20 11:13 ` [PATCH v3 03/11] ShellPkg: acpiview: FADT: " Krzysztof Koch
` (9 subsequent siblings)
11 siblings, 0 replies; 13+ messages in thread
From: Krzysztof Koch @ 2020-01-20 11:13 UTC (permalink / raw)
To: devel; +Cc: ray.ni, zhichao.gao, Sami.Mujawar, Matteo.Carlini, nd
Check if XsdtAddress pointer has been successfully updated before it
is used for further table parsing.
Signed-off-by: Krzysztof Koch <krzysztof.koch@arm.com>
---
Notes:
v3:
- Rebase on latest master [Krzysztof]
v1:
- Test against NULL pointers [Krzysztof]
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Rsdp/RsdpParser.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Rsdp/RsdpParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Rsdp/RsdpParser.c
index 5a5c4b50c12e6eb0aa0efb1765df7e123f614da3..f4a8732a7db7c437031f2a3d2f266b80eff17b4b 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Rsdp/RsdpParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Rsdp/RsdpParser.c
@@ -138,6 +138,18 @@ ParseAcpiRsdp (
PARSER_PARAMS (RsdpParser)
);
+ // Check if the values used to control the parsing logic have been
+ // successfully read.
+ if (XsdtAddress == NULL) {
+ IncrementErrorCount ();
+ Print (
+ L"ERROR: Insufficient table length. AcpiTableLength = %d." \
+ L"RSDP parsing aborted.\n",
+ AcpiTableLength
+ );
+ return;
+ }
+
// This code currently supports parsing of XSDT table only
// and does not parse the RSDT table. Platforms provide the
// RSDT to enable compatibility with ACPI 1.0 operating systems.
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v3 03/11] ShellPkg: acpiview: FADT: Validate global pointer before use
2020-01-20 11:13 [PATCH v3 00/11] Test against invalid pointers in acpiview Krzysztof Koch
2020-01-20 11:13 ` [PATCH v3 01/11] ShellPkg: acpiview: Set ItemPtr to NULL for unprocessed table fields Krzysztof Koch
2020-01-20 11:13 ` [PATCH v3 02/11] ShellPkg: acpiview: RSDP: Validate global pointer before use Krzysztof Koch
@ 2020-01-20 11:13 ` Krzysztof Koch
2020-01-20 11:13 ` [PATCH v3 04/11] ShellPkg: acpiview: SLIT: " Krzysztof Koch
` (8 subsequent siblings)
11 siblings, 0 replies; 13+ messages in thread
From: Krzysztof Koch @ 2020-01-20 11:13 UTC (permalink / raw)
To: devel; +Cc: ray.ni, zhichao.gao, Sami.Mujawar, Matteo.Carlini, nd
Check if global pointers have been successfully updated before they
are used for further table parsing.
Signed-off-by: Krzysztof Koch <krzysztof.koch@arm.com>
---
Notes:
v3:
- Rebase on latest master [Krzysztof]
v2:
- Do not require FadtMinorRevision and X_DsdtAddress pointers to be
valid in order to process the remaining ACPI tables [Zhichao]
v1:
- Test against NULL pointers [Krzysztof]
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c | 21 ++++++++++----------
1 file changed, 11 insertions(+), 10 deletions(-)
diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c
index 5b8cc174f16afb3d4feb6a518952e60c6564ee34..37cbd8be287944656afcd609a3dd080440d5cfef 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c
@@ -1,7 +1,7 @@
/** @file
FADT 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):
@@ -230,9 +230,11 @@ ParseAcpiFadt (
);
if (Trace) {
- Print (L"\nSummary:\n");
- PrintFieldName (2, L"FADT Version");
- Print (L"%d.%d\n", *AcpiHdrInfo.Revision, *FadtMinorRevision);
+ if (FadtMinorRevision != NULL) {
+ Print (L"\nSummary:\n");
+ PrintFieldName (2, L"FADT Version");
+ Print (L"%d.%d\n", *AcpiHdrInfo.Revision, *FadtMinorRevision);
+ }
if (*GetAcpiXsdtHeaderInfo ()->OemTableId != *AcpiHdrInfo.OemTableId) {
IncrementErrorCount ();
@@ -294,21 +296,20 @@ ParseAcpiFadt (
);
}
- // If X_DSDT is not zero then use X_DSDT and ignore DSDT,
- // else use DSDT.
- if (*X_DsdtAddress != 0) {
+ // If X_DSDT is valid then use X_DSDT and ignore DSDT, else use DSDT.
+ if ((X_DsdtAddress != NULL) && (*X_DsdtAddress != 0)) {
DsdtPtr = (UINT8*)(UINTN)(*X_DsdtAddress);
- } else if (*DsdtAddress != 0) {
+ } else if ((DsdtAddress != NULL) && (*DsdtAddress != 0)) {
DsdtPtr = (UINT8*)(UINTN)(*DsdtAddress);
} else {
- // Both DSDT and X_DSDT cannot be zero.
+ // Both DSDT and X_DSDT cannot be invalid.
#if defined (MDE_CPU_ARM) || defined (MDE_CPU_AARCH64)
if (Trace) {
// The DSDT Table is mandatory for ARM systems
// as the CPU information MUST be presented in
// the DSDT.
IncrementErrorCount ();
- Print (L"ERROR: Both X_DSDT and DSDT are NULL.\n");
+ Print (L"ERROR: Both X_DSDT and DSDT are invalid.\n");
}
#endif
return;
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v3 04/11] ShellPkg: acpiview: SLIT: Validate global pointer before use
2020-01-20 11:13 [PATCH v3 00/11] Test against invalid pointers in acpiview Krzysztof Koch
` (2 preceding siblings ...)
2020-01-20 11:13 ` [PATCH v3 03/11] ShellPkg: acpiview: FADT: " Krzysztof Koch
@ 2020-01-20 11:13 ` Krzysztof Koch
2020-01-20 11:13 ` [PATCH v3 05/11] ShellPkg: acpiview: SLIT: Validate System Locality count Krzysztof Koch
` (7 subsequent siblings)
11 siblings, 0 replies; 13+ messages in thread
From: Krzysztof Koch @ 2020-01-20 11:13 UTC (permalink / raw)
To: devel; +Cc: ray.ni, zhichao.gao, Sami.Mujawar, Matteo.Carlini, nd
Check if SlitSystemLocalityCount pointer has been successfully updated
before it is used for further table parsing.
Signed-off-by: Krzysztof Koch <krzysztof.koch@arm.com>
---
Notes:
v3:
- Rebase on latest master [Krzysztof]
v1:
- Test against NULL pointers [Krzysztof]
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Slit/SlitParser.c | 16 ++++++++++++++--
1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Slit/SlitParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Slit/SlitParser.c
index ca2808db526b1bbb79aeb21ccfc0ae6c79b2dfd8..17e2166a09d8615b714e0c51d4d93d293fcdf601 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Slit/SlitParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Slit/SlitParser.c
@@ -1,7 +1,7 @@
/** @file
SLIT table parser
- Copyright (c) 2016 - 2018, ARM Limited. All rights reserved.
+ Copyright (c) 2016 - 2019, ARM Limited. All rights reserved.
SPDX-License-Identifier: BSD-2-Clause-Patent
@par Reference(s):
@@ -75,9 +75,21 @@ ParseAcpiSlit (
AcpiTableLength,
PARSER_PARAMS (SlitParser)
);
+
+ // Check if the values used to control the parsing logic have been
+ // successfully read.
+ if (SlitSystemLocalityCount == NULL) {
+ IncrementErrorCount ();
+ Print (
+ L"ERROR: Insufficient table length. AcpiTableLength = %d.\n",
+ AcpiTableLength
+ );
+ return;
+ }
+
LocalityPtr = Ptr + Offset;
-
LocalityCount = *SlitSystemLocalityCount;
+
// We only print the Localities if the count is less than 16
// If the locality count is more than 16 then refer to the
// raw data dump.
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v3 05/11] ShellPkg: acpiview: SLIT: Validate System Locality count
2020-01-20 11:13 [PATCH v3 00/11] Test against invalid pointers in acpiview Krzysztof Koch
` (3 preceding siblings ...)
2020-01-20 11:13 ` [PATCH v3 04/11] ShellPkg: acpiview: SLIT: " Krzysztof Koch
@ 2020-01-20 11:13 ` Krzysztof Koch
2020-01-20 11:13 ` [PATCH v3 06/11] ShellPkg: acpiview: SRAT: Validate global pointers before use Krzysztof Koch
` (6 subsequent siblings)
11 siblings, 0 replies; 13+ messages in thread
From: Krzysztof Koch @ 2020-01-20 11:13 UTC (permalink / raw)
To: devel; +Cc: ray.ni, zhichao.gao, Sami.Mujawar, Matteo.Carlini, nd
1. Check if the 'Number of System Localities' provided can be
represented in the SLIT table. The table 'Length' field is a 32-bit
value while the 'Number of System Localities' field is 64-bit long.
2. Check if the SLIT matrix fits in the table buffer. If N is the SLIT
locality count, then the matrix used to represent the localities is
N*N bytes long. The ACPI table length must be big enough to fit the
matrix.
3. Remove (now) redundant 64x64 bit multiplication.
Signed-off-by: Krzysztof Koch <krzysztof.koch@arm.com>
---
Notes:
v3:
- Rebase on latest master [Krzysztof]
v1:
- Validate the 'Number of System Localities' Field [Krzysztof]
- Remove redundant 64x64 bit multiplication [Krzysztof]
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Slit/SlitParser.c | 47 +++++++++++++++++---
1 file changed, 42 insertions(+), 5 deletions(-)
diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Slit/SlitParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Slit/SlitParser.c
index 17e2166a09d8615b714e0c51d4d93d293fcdf601..e4625ee8b13907893a9b6990ecb956baf91cc3b9 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Slit/SlitParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Slit/SlitParser.c
@@ -30,7 +30,7 @@ STATIC CONST ACPI_PARSER SlitParser[] = {
/**
Macro to get the value of a System Locality
**/
-#define SLIT_ELEMENT(Ptr, i, j) *(Ptr + (MultU64x64 (i, LocalityCount)) + j)
+#define SLIT_ELEMENT(Ptr, i, j) *(Ptr + (i * LocalityCount) + j)
/**
This function parses the ACPI SLIT table.
@@ -57,9 +57,9 @@ ParseAcpiSlit (
)
{
UINT32 Offset;
- UINT64 Count;
- UINT64 Index;
- UINT64 LocalityCount;
+ UINT32 Count;
+ UINT32 Index;
+ UINT32 LocalityCount;
UINT8* LocalityPtr;
CHAR16 Buffer[80]; // Used for AsciiName param of ParseAcpi
@@ -87,8 +87,45 @@ ParseAcpiSlit (
return;
}
+ /*
+ Despite the 'Number of System Localities' being a 64-bit field in SLIT,
+ the maximum number of localities that can be represented in SLIT is limited
+ by the 'Length' field of the ACPI table.
+
+ Since the ACPI table length field is 32-bit wide. The maximum number of
+ localities that can be represented in SLIT can be calculated as:
+
+ MaxLocality = sqrt (MAX_UINT32 - sizeof (EFI_ACPI_6_3_SYSTEM_LOCALITY_DISTANCE_INFORMATION_TABLE_HEADER))
+ = 65535
+ = MAX_UINT16
+ */
+ if (*SlitSystemLocalityCount > MAX_UINT16) {
+ IncrementErrorCount ();
+ Print (
+ L"ERROR: The Number of System Localities provided can't be represented " \
+ L"in the SLIT table. SlitSystemLocalityCount = %ld. " \
+ L"MaxLocalityCountAllowed = %d.\n",
+ *SlitSystemLocalityCount,
+ MAX_UINT16
+ );
+ return;
+ }
+
+ LocalityCount = (UINT32)*SlitSystemLocalityCount;
+
+ // Make sure system localities fit in the table buffer provided
+ if (Offset + (LocalityCount * LocalityCount) > AcpiTableLength) {
+ IncrementErrorCount ();
+ Print (
+ L"ERROR: Invalid Number of System Localities. " \
+ L"SlitSystemLocalityCount = %ld. AcpiTableLength = %d.\n",
+ *SlitSystemLocalityCount,
+ AcpiTableLength
+ );
+ return;
+ }
+
LocalityPtr = Ptr + Offset;
- LocalityCount = *SlitSystemLocalityCount;
// We only print the Localities if the count is less than 16
// If the locality count is more than 16 then refer to the
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v3 06/11] ShellPkg: acpiview: SRAT: Validate global pointers before use
2020-01-20 11:13 [PATCH v3 00/11] Test against invalid pointers in acpiview Krzysztof Koch
` (4 preceding siblings ...)
2020-01-20 11:13 ` [PATCH v3 05/11] ShellPkg: acpiview: SLIT: Validate System Locality count Krzysztof Koch
@ 2020-01-20 11:13 ` Krzysztof Koch
2020-01-20 11:13 ` [PATCH v3 07/11] ShellPkg: acpiview: MADT: " Krzysztof Koch
` (5 subsequent siblings)
11 siblings, 0 replies; 13+ messages in thread
From: Krzysztof Koch @ 2020-01-20 11:13 UTC (permalink / raw)
To: devel; +Cc: ray.ni, zhichao.gao, Sami.Mujawar, Matteo.Carlini, nd
Check if SratRAType and SratRALength pointers have been successfully
updated before they are used for further table parsing.
Signed-off-by: Krzysztof Koch <krzysztof.koch@arm.com>
---
Notes:
v3:
- Rebase on latest master [Krzysztof]
v1:
- Test against NULL pointers [Krzysztof]
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser.c
index 6fe7bf681132df08133e3e03e3ee3f020d905dd2..3613900ae322483fdd3d3383de4e22ba75b2128b 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser.c
@@ -399,6 +399,19 @@ ParseAcpiSrat (
PARSER_PARAMS (SratResourceAllocationParser)
);
+ // Check if the values used to control the parsing logic have been
+ // successfully read.
+ if ((SratRAType == NULL) ||
+ (SratRALength == NULL)) {
+ IncrementErrorCount ();
+ Print (
+ L"ERROR: Insufficient remaining table buffer length to read the " \
+ L"Static Resource Allocation structure header. Length = %d.\n",
+ AcpiTableLength - Offset
+ );
+ return;
+ }
+
// Make sure the SRAT structure lies inside the table
if ((Offset + *SratRALength) > AcpiTableLength) {
IncrementErrorCount ();
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v3 07/11] ShellPkg: acpiview: MADT: Validate global pointers before use
2020-01-20 11:13 [PATCH v3 00/11] Test against invalid pointers in acpiview Krzysztof Koch
` (5 preceding siblings ...)
2020-01-20 11:13 ` [PATCH v3 06/11] ShellPkg: acpiview: SRAT: Validate global pointers before use Krzysztof Koch
@ 2020-01-20 11:13 ` Krzysztof Koch
2020-01-20 11:13 ` [PATCH v3 08/11] ShellPkg: acpiview: PPTT: " Krzysztof Koch
` (4 subsequent siblings)
11 siblings, 0 replies; 13+ messages in thread
From: Krzysztof Koch @ 2020-01-20 11:13 UTC (permalink / raw)
To: devel; +Cc: ray.ni, zhichao.gao, Sami.Mujawar, Matteo.Carlini, nd
Check if the MadtInterruptControllerType and
MadtInterruptControllerLength pointers have been successfully updated
before they are used for further table parsing.
Signed-off-by: Krzysztof Koch <krzysztof.koch@arm.com>
---
Notes:
v3:
- Rebase on latest master [Krzysztof]
v1:
- Test against NULL pointers [Krzysztof]
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.c
index 90bdafea1970db522e8ed96de7c6e986cdaca5ba..438905cb24f58b8b82e8fe61280e72f765d578d8 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.c
@@ -260,6 +260,19 @@ ParseAcpiMadt (
PARSER_PARAMS (MadtInterruptControllerHeaderParser)
);
+ // Check if the values used to control the parsing logic have been
+ // successfully read.
+ if ((MadtInterruptControllerType == NULL) ||
+ (MadtInterruptControllerLength == NULL)) {
+ IncrementErrorCount ();
+ Print (
+ L"ERROR: Insufficient remaining table buffer length to read the " \
+ L"Interrupt Controller Structure header. Length = %d.\n",
+ AcpiTableLength - Offset
+ );
+ return;
+ }
+
// Make sure forward progress is made.
if (*MadtInterruptControllerLength < 2) {
IncrementErrorCount ();
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v3 08/11] ShellPkg: acpiview: PPTT: Validate global pointers before use
2020-01-20 11:13 [PATCH v3 00/11] Test against invalid pointers in acpiview Krzysztof Koch
` (6 preceding siblings ...)
2020-01-20 11:13 ` [PATCH v3 07/11] ShellPkg: acpiview: MADT: " Krzysztof Koch
@ 2020-01-20 11:13 ` Krzysztof Koch
2020-01-20 11:13 ` [PATCH v3 09/11] ShellPkg: acpiview: IORT: " Krzysztof Koch
` (3 subsequent siblings)
11 siblings, 0 replies; 13+ messages in thread
From: Krzysztof Koch @ 2020-01-20 11:13 UTC (permalink / raw)
To: devel; +Cc: ray.ni, zhichao.gao, Sami.Mujawar, Matteo.Carlini, nd
Check if the NumberOfPrivateResources, ProcessorTopologyStructureType
and ProcessorTopologyStructureLength pointers have been successfully
updated before they are used for further table parsing.
Signed-off-by: Krzysztof Koch <krzysztof.koch@arm.com>
---
Notes:
v3:
- Rebase on latest master [Krzysztof]
v1:
- Test against NULL pointers [Krzysztof]
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c | 25 ++++++++++++++++++++
1 file changed, 25 insertions(+)
diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c
index 6254b9913fffb429fc54bb1301bf3e4b2e5bf161..675ba75f02b367cd5ad9f2ac23c30ed0ab58f286 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c
@@ -264,6 +264,17 @@ DumpProcessorHierarchyNodeStructure (
PARSER_PARAMS (ProcessorHierarchyNodeStructureParser)
);
+ // Check if the values used to control the parsing logic have been
+ // successfully read.
+ if (NumberOfPrivateResources == NULL) {
+ IncrementErrorCount ();
+ Print (
+ L"ERROR: Insufficient Processor Hierarchy Node length. Length = %d.\n",
+ Length
+ );
+ return;
+ }
+
// Make sure the Private Resource array lies inside this structure
if (Offset + (*NumberOfPrivateResources * sizeof (UINT32)) > Length) {
IncrementErrorCount ();
@@ -387,6 +398,7 @@ ParseAcpiPptt (
AcpiTableLength,
PARSER_PARAMS (PpttParser)
);
+
ProcessorTopologyStructurePtr = Ptr + Offset;
while (Offset < AcpiTableLength) {
@@ -400,6 +412,19 @@ ParseAcpiPptt (
PARSER_PARAMS (ProcessorTopologyStructureHeaderParser)
);
+ // Check if the values used to control the parsing logic have been
+ // successfully read.
+ if ((ProcessorTopologyStructureType == NULL) ||
+ (ProcessorTopologyStructureLength == NULL)) {
+ IncrementErrorCount ();
+ Print (
+ L"ERROR: Insufficient remaining table buffer length to read the " \
+ L"processor topology structure header. Length = %d.\n",
+ AcpiTableLength - Offset
+ );
+ return;
+ }
+
// Make sure the PPTT structure lies inside the table
if ((Offset + *ProcessorTopologyStructureLength) > AcpiTableLength) {
IncrementErrorCount ();
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v3 09/11] ShellPkg: acpiview: IORT: Validate global pointers before use
2020-01-20 11:13 [PATCH v3 00/11] Test against invalid pointers in acpiview Krzysztof Koch
` (7 preceding siblings ...)
2020-01-20 11:13 ` [PATCH v3 08/11] ShellPkg: acpiview: PPTT: " Krzysztof Koch
@ 2020-01-20 11:13 ` Krzysztof Koch
2020-01-20 11:13 ` [PATCH v3 10/11] ShellPkg: acpiview: GTDT: " Krzysztof Koch
` (2 subsequent siblings)
11 siblings, 0 replies; 13+ messages in thread
From: Krzysztof Koch @ 2020-01-20 11:13 UTC (permalink / raw)
To: devel; +Cc: ray.ni, zhichao.gao, Sami.Mujawar, Matteo.Carlini, nd
Check if global (in the scope of the IORT parser) pointers have been
successfully updated before they are used for further table parsing.
Signed-off-by: Krzysztof Koch <krzysztof.koch@arm.com>
---
Notes:
v3:
- Rebase on latest master [Krzysztof]
v1:
- Test against NULL pointers [Krzysztof]
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser.c | 52 ++++++++++++++++++++
1 file changed, 52 insertions(+)
diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser.c
index 72289c7680bc3cd5c444481e8d6a719803202a9b..9d5d937c7b2c19945ca2ad3eba644bdfc09cc3f6 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser.c
@@ -322,6 +322,20 @@ DumpIortNodeSmmuV1V2 (
PARSER_PARAMS (IortNodeSmmuV1V2Parser)
);
+ // Check if the values used to control the parsing logic have been
+ // successfully read.
+ if ((InterruptContextCount == NULL) ||
+ (InterruptContextOffset == NULL) ||
+ (PmuInterruptCount == NULL) ||
+ (PmuInterruptOffset == NULL)) {
+ IncrementErrorCount ();
+ Print (
+ L"ERROR: Insufficient SMMUv1/2 node length. Length = %d\n",
+ Length
+ );
+ return;
+ }
+
Offset = *InterruptContextOffset;
Index = 0;
@@ -433,6 +447,17 @@ DumpIortNodeIts (
PARSER_PARAMS (IortNodeItsParser)
);
+ // Check if the values used to control the parsing logic have been
+ // successfully read.
+ if (ItsCount == NULL) {
+ IncrementErrorCount ();
+ Print (
+ L"ERROR: Insufficient ITS group length. Length = %d.\n",
+ Length
+ );
+ return;
+ }
+
Index = 0;
while ((Index < *ItsCount) &&
@@ -617,6 +642,18 @@ ParseAcpiIort (
PARSER_PARAMS (IortParser)
);
+ // Check if the values used to control the parsing logic have been
+ // successfully read.
+ if ((IortNodeCount == NULL) ||
+ (IortNodeOffset == NULL)) {
+ IncrementErrorCount ();
+ Print (
+ L"ERROR: Insufficient table length. AcpiTableLength = %d.\n",
+ AcpiTableLength
+ );
+ return;
+ }
+
Offset = *IortNodeOffset;
NodePtr = Ptr + Offset;
Index = 0;
@@ -635,6 +672,21 @@ ParseAcpiIort (
PARSER_PARAMS (IortNodeHeaderParser)
);
+ // Check if the values used to control the parsing logic have been
+ // successfully read.
+ if ((IortNodeType == NULL) ||
+ (IortNodeLength == NULL) ||
+ (IortIdMappingCount == NULL) ||
+ (IortIdMappingOffset == NULL)) {
+ IncrementErrorCount ();
+ Print (
+ L"ERROR: Insufficient remaining table buffer length to read the " \
+ L"IORT node header. Length = %d.\n",
+ AcpiTableLength - Offset
+ );
+ return;
+ }
+
// Make sure the IORT Node is inside the table
if ((Offset + (*IortNodeLength)) > AcpiTableLength) {
IncrementErrorCount ();
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v3 10/11] ShellPkg: acpiview: GTDT: Validate global pointers before use
2020-01-20 11:13 [PATCH v3 00/11] Test against invalid pointers in acpiview Krzysztof Koch
` (8 preceding siblings ...)
2020-01-20 11:13 ` [PATCH v3 09/11] ShellPkg: acpiview: IORT: " Krzysztof Koch
@ 2020-01-20 11:13 ` Krzysztof Koch
2020-01-20 11:13 ` [PATCH v3 11/11] ShellPkg: acpiview: DBG2: " Krzysztof Koch
2020-02-03 15:36 ` [PATCH v3 00/11] Test against invalid pointers in acpiview Gao, Zhichao
11 siblings, 0 replies; 13+ messages in thread
From: Krzysztof Koch @ 2020-01-20 11:13 UTC (permalink / raw)
To: devel; +Cc: ray.ni, zhichao.gao, Sami.Mujawar, Matteo.Carlini, nd
Check if global (in the scope of the GTDT parser) pointers have been
successfully updated before they are used for further table parsing.
Signed-off-by: Krzysztof Koch <krzysztof.koch@arm.com>
---
Notes:
v3:
- Rebase on latest master [Krzysztof]
v1:
- Test against NULL pointers [Krzysztof]
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser.c | 37 ++++++++++++++++++++
1 file changed, 37 insertions(+)
diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser.c
index 57174e14c80072f12b90e1996ebe8f0002d0c404..699a55b549ec3fa61bbd156898821055dc019199 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser.c
@@ -189,6 +189,18 @@ DumpGTBlock (
PARSER_PARAMS (GtBlockParser)
);
+ // Check if the values used to control the parsing logic have been
+ // successfully read.
+ if ((GtBlockTimerCount == NULL) ||
+ (GtBlockTimerOffset == NULL)) {
+ IncrementErrorCount ();
+ Print (
+ L"ERROR: Insufficient GT Block Structure length. Length = %d.\n",
+ Length
+ );
+ return;
+ }
+
Offset = *GtBlockTimerOffset;
Index = 0;
@@ -272,6 +284,18 @@ ParseAcpiGtdt (
PARSER_PARAMS (GtdtParser)
);
+ // Check if the values used to control the parsing logic have been
+ // successfully read.
+ if ((GtdtPlatformTimerCount == NULL) ||
+ (GtdtPlatformTimerOffset == NULL)) {
+ IncrementErrorCount ();
+ Print (
+ L"ERROR: Insufficient table length. AcpiTableLength = %d.\n",
+ AcpiTableLength
+ );
+ return;
+ }
+
TimerPtr = Ptr + *GtdtPlatformTimerOffset;
Offset = *GtdtPlatformTimerOffset;
Index = 0;
@@ -290,6 +314,19 @@ ParseAcpiGtdt (
PARSER_PARAMS (GtPlatformTimerHeaderParser)
);
+ // Check if the values used to control the parsing logic have been
+ // successfully read.
+ if ((PlatformTimerType == NULL) ||
+ (PlatformTimerLength == NULL)) {
+ IncrementErrorCount ();
+ Print (
+ L"ERROR: Insufficient remaining table buffer length to read the " \
+ L"Platform Timer Structure header. Length = %d.\n",
+ AcpiTableLength - Offset
+ );
+ return;
+ }
+
// Make sure the Platform Timer is inside the table.
if ((Offset + *PlatformTimerLength) > AcpiTableLength) {
IncrementErrorCount ();
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v3 11/11] ShellPkg: acpiview: DBG2: Validate global pointers before use
2020-01-20 11:13 [PATCH v3 00/11] Test against invalid pointers in acpiview Krzysztof Koch
` (9 preceding siblings ...)
2020-01-20 11:13 ` [PATCH v3 10/11] ShellPkg: acpiview: GTDT: " Krzysztof Koch
@ 2020-01-20 11:13 ` Krzysztof Koch
2020-02-03 15:36 ` [PATCH v3 00/11] Test against invalid pointers in acpiview Gao, Zhichao
11 siblings, 0 replies; 13+ messages in thread
From: Krzysztof Koch @ 2020-01-20 11:13 UTC (permalink / raw)
To: devel; +Cc: ray.ni, zhichao.gao, Sami.Mujawar, Matteo.Carlini, nd
Check if global (in the scope of the DBG2 parser) pointers have been
successfully updated before they are used for further table parsing.
Signed-off-by: Krzysztof Koch <krzysztof.koch@arm.com>
---
Notes:
v3:
- Rebase on latest master [Krzysztof]
v1:
- Test against NULL pointers [Krzysztof]
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c | 43 ++++++++++++++++++++
1 file changed, 43 insertions(+)
diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c
index 869e700b9beda4886bf7bc5ae4ced3ab9a59efa3..0f730a306a94329a23fbaf54b59f1833b44616ba 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c
@@ -123,6 +123,24 @@ DumpDbgDeviceInfo (
PARSER_PARAMS (DbgDevInfoParser)
);
+ // Check if the values used to control the parsing logic have been
+ // successfully read.
+ if ((GasCount == NULL) ||
+ (NameSpaceStringLength == NULL) ||
+ (NameSpaceStringOffset == NULL) ||
+ (OEMDataLength == NULL) ||
+ (OEMDataOffset == NULL) ||
+ (BaseAddrRegOffset == NULL) ||
+ (AddrSizeOffset == NULL)) {
+ IncrementErrorCount ();
+ Print (
+ L"ERROR: Insufficient Debug Device Information Structure length. " \
+ L"Length = %d.\n",
+ Length
+ );
+ return;
+ }
+
// GAS
Index = 0;
Offset = *BaseAddrRegOffset;
@@ -224,6 +242,18 @@ ParseAcpiDbg2 (
PARSER_PARAMS (Dbg2Parser)
);
+ // Check if the values used to control the parsing logic have been
+ // successfully read.
+ if ((OffsetDbgDeviceInfo == NULL) ||
+ (NumberDbgDeviceInfo == NULL)) {
+ IncrementErrorCount ();
+ Print (
+ L"ERROR: Insufficient table length. AcpiTableLength = %d\n",
+ AcpiTableLength
+ );
+ return;
+ }
+
Offset = *OffsetDbgDeviceInfo;
Index = 0;
@@ -239,6 +269,19 @@ ParseAcpiDbg2 (
PARSER_PARAMS (DbgDevInfoHeaderParser)
);
+ // Check if the values used to control the parsing logic have been
+ // successfully read.
+ if (DbgDevInfoLen == NULL) {
+ IncrementErrorCount ();
+ Print (
+ L"ERROR: Insufficient remaining table buffer length to read the " \
+ L"Debug Device Information structure's 'Length' field. " \
+ L"RemainingTableBufferLength = %d.\n",
+ AcpiTableLength - Offset
+ );
+ return;
+ }
+
// Make sure the Debug Device Information structure lies inside the table.
if ((Offset + *DbgDevInfoLen) > AcpiTableLength) {
IncrementErrorCount ();
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v3 00/11] Test against invalid pointers in acpiview
2020-01-20 11:13 [PATCH v3 00/11] Test against invalid pointers in acpiview Krzysztof Koch
` (10 preceding siblings ...)
2020-01-20 11:13 ` [PATCH v3 11/11] ShellPkg: acpiview: DBG2: " Krzysztof Koch
@ 2020-02-03 15:36 ` Gao, Zhichao
11 siblings, 0 replies; 13+ messages in thread
From: Gao, Zhichao @ 2020-02-03 15:36 UTC (permalink / raw)
To: Krzysztof Koch, devel@edk2.groups.io
Cc: Ni, Ray, Sami.Mujawar@arm.com, Matteo.Carlini@arm.com, nd@arm.com
Sorry for the misunderstanding before. The patch set is good to me.
Series: Reviewed-by: Zhichao Gao <zhichao.gao@intel.com>
Thanks,
Zhichao
> -----Original Message-----
> From: Krzysztof Koch [mailto:krzysztof.koch@arm.com]
> Sent: Monday, January 20, 2020 7:14 PM
> To: devel@edk2.groups.io
> Cc: Ni, Ray <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>;
> Sami.Mujawar@arm.com; Matteo.Carlini@arm.com; nd@arm.com
> Subject: [PATCH v3 00/11] Test against invalid pointers in acpiview
>
> Prevent the use of invalid pointers when parsing ACPI tables in the UEFI shell
> acpiview tool.
>
> The parsing of ACPI tables is often controlled with the values read earlier from
> the same table. For example, the 'Offset' or 'Count' fields found in a structure
> are later used to parse the substructures. If such fields lie outside the structure's
> buffer length provided, then there is a possibility for a wild or dangling pointer.
>
> Currently, if the ParseAcpi() function terminates early because the end of the
> input table data buffer has been reached, then the pointers which were
> supposed to be updated by this function are left untouched.
> This is a security issue as the values pointed to by these pointers are later used
> for flow control.
>
> This patch series aims to solve this security issue by explicitly initializing any
> pointers lying outside the input ACPI data buffer to NULL and testing for NULL
> whenever these pointers are dereferenced.
>
> Changes can be seet at:
> https://github.com/KrzysztofKoch1/edk2/tree/612_add_pointer_validation_v3
>
> Notes:
> v3:
> - Rebase on latest master [Krzysztof]
>
> v2:
> - Do not require FadtMinorRevision and X_DsdtAddress pointers to be
> valid in FADT table parser [Zhichao]
>
> v1:
> - Validate static pointers in acpiview parsers before use [Krzysztof]
>
> Krzysztof Koch (11):
> ShellPkg: acpiview: Set ItemPtr to NULL for unprocessed table fields
> ShellPkg: acpiview: RSDP: Validate global pointer before use
> ShellPkg: acpiview: FADT: Validate global pointer before use
> ShellPkg: acpiview: SLIT: Validate global pointer before use
> ShellPkg: acpiview: SLIT: Validate System Locality count
> ShellPkg: acpiview: SRAT: Validate global pointers before use
> ShellPkg: acpiview: MADT: Validate global pointers before use
> ShellPkg: acpiview: PPTT: Validate global pointers before use
> ShellPkg: acpiview: IORT: Validate global pointers before use
> ShellPkg: acpiview: GTDT: Validate global pointers before use
> ShellPkg: acpiview: DBG2: Validate global pointers before use
>
> ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c | 9 ++-
> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c |
> 43 ++++++++++++++
> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c | 21
> +++----
> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser.c | 37
> ++++++++++++
> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser.c | 52
> +++++++++++++++++
> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.c |
> 13 +++++
> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c | 25
> ++++++++
> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Rsdp/RsdpParser.c |
> 12 ++++
> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Slit/SlitParser.c | 61
> ++++++++++++++++++--
> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser.c | 13
> +++++
> 10 files changed, 269 insertions(+), 17 deletions(-)
>
> --
> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
>
^ permalink raw reply [flat|nested] 13+ messages in thread