From: "Krzysztof Koch" <krzysztof.koch@arm.com>
To: <devel@edk2.groups.io>
Cc: <jaben.carsey@intel.com>, <ray.ni@intel.com>,
<zhichao.gao@intel.com>, <Sami.Mujawar@arm.com>,
<Matteo.Carlini@arm.com>, <nd@arm.com>
Subject: [PATCH v1 08/11] ShellPkg: acpiview: PPTT: Add error-checking in the parsing logic
Date: Fri, 12 Jul 2019 07:52:40 +0100 [thread overview]
Message-ID: <20190712065243.3812-9-krzysztof.koch@arm.com> (raw)
In-Reply-To: <20190712065243.3812-1-krzysztof.koch@arm.com>
1. Check if the global pointers (in the scope of this ACPI table parser)
have been successfully updated before they are later used to control
the parsing logic.
2. Give forward progress guarantee when parsing the PPTT table. Report
an error if a PPTT structure is too small to be valid. Without this
check, there is a possibility for the parser to enter an ifninite loop.
3. Test against buffer overruns.
4. Allow silencing ACPI table content validation errors which do not
cause table parsing to fail.
Signed-off-by: Krzysztof Koch <krzysztof.koch@arm.com>
---
Changes can be seen at: https://github.com/KrzysztofKoch1/edk2/commit/e4789351e111fa1ed6a2c55759f190166b08fc8c
Notes:
v1:
- improve the logic in the PPTT parser [Krzysztof]
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c | 95 ++++++++++++++++----
1 file changed, 76 insertions(+), 19 deletions(-)
diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c
index cec57be55e77096f9448f637ea129af2b42111ad..8d8760940b493eb94c91da3d46f9a844930c1738 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c
@@ -252,7 +252,6 @@ DumpProcessorHierarchyNodeStructure (
)
{
UINT32 Offset;
- UINT8* PrivateResourcePtr;
UINT32 Index;
CHAR16 Buffer[OUTPUT_FIELD_COLUMN_WIDTH];
@@ -265,8 +264,34 @@ DumpProcessorHierarchyNodeStructure (
PARSER_PARAMS (ProcessorHierarchyNodeStructureParser)
);
- PrivateResourcePtr = Ptr + Offset;
+ // 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 ();
+ Print (
+ L"ERROR: Invalid Number of Private Resources. " \
+ L"PrivateResourceCount = %d. RemainingBufferLength = %d. " \
+ L"Parsing of this structure aborted.\n",
+ *NumberOfPrivateResources,
+ Length - Offset
+ );
+ return;
+ }
+
Index = 0;
+
+ // Parse the specified number of private resource references or the Processor
+ // Hierarchy Node length. Whichever is minimum.
while (Index < *NumberOfPrivateResources) {
UnicodeSPrint (
Buffer,
@@ -278,10 +303,10 @@ DumpProcessorHierarchyNodeStructure (
PrintFieldName (4, Buffer);
Print (
L"0x%x\n",
- *((UINT32*) PrivateResourcePtr)
+ *((UINT32*)(Ptr + Offset))
);
- PrivateResourcePtr += sizeof(UINT32);
+ Offset += sizeof (UINT32);
Index++;
}
}
@@ -373,6 +398,7 @@ ParseAcpiPptt (
AcpiTableLength,
PARSER_PARAMS (PpttParser)
);
+
ProcessorTopologyStructurePtr = Ptr + Offset;
while (Offset < AcpiTableLength) {
@@ -382,19 +408,47 @@ ParseAcpiPptt (
0,
NULL,
ProcessorTopologyStructurePtr,
- 4, // Length of the processor topology structure header is 4 bytes
+ AcpiTableLength - Offset,
PARSER_PARAMS (ProcessorTopologyStructureHeaderParser)
);
- if ((Offset + (*ProcessorTopologyStructureLength)) > AcpiTableLength) {
+ // Check if the values used to control the parsing logic have been
+ // successfully read.
+ if ((ProcessorTopologyStructureType == NULL) ||
+ (ProcessorTopologyStructureLength == NULL)) {
IncrementErrorCount ();
Print (
- L"ERROR: Invalid processor topology structure length:"
- L" Type = %d, Length = %d\n",
- *ProcessorTopologyStructureType,
- *ProcessorTopologyStructureLength
+ L"ERROR: Insufficient remaining table buffer length to read the " \
+ L"processor topology structure header. Length = %d.\n",
+ AcpiTableLength - Offset
);
- break;
+ return;
+ }
+
+ // Make sure forward progress is made.
+ if (*ProcessorTopologyStructureLength < 2) {
+ IncrementErrorCount ();
+ Print (
+ L"ERROR: Structure length is too small: " \
+ L"ProcessorTopologyStructureLength = %d. " \
+ L"ProcessorTopologyStructureType = %d. PPTT parsing aborted.\n",
+ *ProcessorTopologyStructureLength,
+ *ProcessorTopologyStructureType
+ );
+ return;
+ }
+
+ // Make sure the PPTT structure lies inside the table
+ if ((Offset + *ProcessorTopologyStructureLength) > AcpiTableLength) {
+ IncrementErrorCount ();
+ Print (
+ L"ERROR: Invalid PPTT structure length. " \
+ L"ProcessorTopologyStructureLength = %d. " \
+ L"RemainingTableBufferLength = %d. PPTT parsing aborted.\n",
+ *ProcessorTopologyStructureLength,
+ AcpiTableLength - Offset
+ );
+ return;
}
PrintFieldName (2, L"* Structure Offset *");
@@ -420,14 +474,17 @@ ParseAcpiPptt (
);
break;
default:
- IncrementErrorCount ();
- Print (
- L"ERROR: Unknown processor topology structure:"
- L" Type = %d, Length = %d\n",
- *ProcessorTopologyStructureType,
- *ProcessorTopologyStructureLength
- );
- }
+ if (GetConsistencyChecking ()) {
+ IncrementErrorCount ();
+ Print (
+ L"ERROR: Unknown processor topology structure:"
+ L" Type = %d, Length = %d\n",
+ *ProcessorTopologyStructureType,
+ *ProcessorTopologyStructureLength
+ );
+ }
+ break;
+ } // switch
ProcessorTopologyStructurePtr += *ProcessorTopologyStructureLength;
Offset += *ProcessorTopologyStructureLength;
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
next prev parent reply other threads:[~2019-07-12 6:54 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-07-12 6:52 [PATCH v1 00/11] Add security checks in the Acpiview table parsers Krzysztof Koch
2019-07-12 6:52 ` [PATCH v1 01/11] ShellPkg: acpiview: FADT: Validate global pointers before use Krzysztof Koch
2019-07-12 14:26 ` Carsey, Jaben
2019-07-17 13:38 ` [edk2-devel] " Krzysztof Koch
2019-07-19 1:21 ` Gao, Zhichao
2019-07-12 6:52 ` [PATCH v1 02/11] ShellPkg: acpiview: SPCR: Remove redundant forward declaration Krzysztof Koch
2019-07-17 9:42 ` [edk2-devel] " Alexei Fedorov
2019-07-12 6:52 ` [PATCH v1 03/11] ShellPkg: acpiview: RSDP: Make printing table checksum optional Krzysztof Koch
2019-07-17 9:41 ` [edk2-devel] " Alexei Fedorov
2019-07-12 6:52 ` [PATCH v1 04/11] ShellPkg: acpiview: XSDT: Remove redundant ParseAcpi() call Krzysztof Koch
2019-07-17 9:41 ` [edk2-devel] " Alexei Fedorov
2019-07-12 6:52 ` [PATCH v1 05/11] ShellPkg: acpiview: SLIT: Add error-checking in the parsing logic Krzysztof Koch
2019-07-17 9:42 ` [edk2-devel] " Alexei Fedorov
2019-07-12 6:52 ` [PATCH v1 06/11] ShellPkg: acpiview: SRAT: " Krzysztof Koch
2019-07-17 9:41 ` [edk2-devel] " Alexei Fedorov
2019-07-12 6:52 ` [PATCH v1 07/11] ShellPkg: acpiview: MADT: " Krzysztof Koch
2019-07-17 9:40 ` [edk2-devel] " Alexei Fedorov
2019-07-12 6:52 ` Krzysztof Koch [this message]
2019-07-17 9:40 ` [edk2-devel] [PATCH v1 08/11] ShellPkg: acpiview: PPTT: " Alexei Fedorov
2019-07-12 6:52 ` [PATCH v1 09/11] ShellPkg: acpiview: IORT: " Krzysztof Koch
2019-07-17 9:40 ` [edk2-devel] " Alexei Fedorov
2019-07-12 6:52 ` [PATCH v1 10/11] ShellPkg: acpiview: GTDT: " Krzysztof Koch
2019-07-17 9:39 ` [edk2-devel] " Alexei Fedorov
2019-07-12 6:52 ` [PATCH v1 11/11] ShellPkg: acpiview: DBG2: " Krzysztof Koch
2019-07-17 9:39 ` [edk2-devel] " Alexei Fedorov
2019-07-19 5:39 ` Gao, Zhichao
2019-07-17 9:42 ` [edk2-devel] [PATCH v1 00/11] Add security checks in the Acpiview table parsers Alexei Fedorov
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190712065243.3812-9-krzysztof.koch@arm.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox