public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
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 05/11] ShellPkg: acpiview: SLIT: Add error-checking in the parsing logic
Date: Fri, 12 Jul 2019 07:52:37 +0100	[thread overview]
Message-ID: <20190712065243.3812-6-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. Test against buffer overruns.

3. Allow silencing ACPI table content validation errors which do not
cause table parsing to fail.

4. Check if the 'Number of System Localities' provided can be
represented in a SLIT table. The table 'Length' field is a 32-bit value
while the 'Number of System Localities' is a 64-bit value.

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

Changes can be seen at: https://github.com/KrzysztofKoch1/edk2/commit/2f1ffd1a74d06c23c01971be965d856f0a0e3ac4

Notes:
    v1:
    - improve the logic in the SLIT parser [Krzysztof]

 ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Slit/SlitParser.c | 115 ++++++++++++++------
 1 file changed, 83 insertions(+), 32 deletions(-)

diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Slit/SlitParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Slit/SlitParser.c
index 1f9dac66eea5b0f4366a7a9584ac6702a74beaac..dd5f039b67326acc710ee703a6b132a1e280dcaa 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):
@@ -13,6 +13,7 @@
 #include <Library/UefiLib.h>
 #include "AcpiParser.h"
 #include "AcpiTableParser.h"
+#include "AcpiView.h"
 
 // Local Variables
 STATIC CONST UINT64* SlitSystemLocalityCount;
@@ -59,7 +60,7 @@ ParseAcpiSlit (
   UINT32 Offset;
   UINT64 Count;
   UINT64 Index;
-  UINT64 LocalityCount;
+  UINT32 LocalityCount;
   UINT8* LocalityPtr;
   CHAR16 Buffer[80];  // Used for AsciiName param of ParseAcpi
 
@@ -77,7 +78,55 @@ ParseAcpiSlit (
              );
   LocalityPtr = Ptr + Offset;
 
-  LocalityCount = *SlitSystemLocalityCount;
+  // 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;
+  }
+
+  /*
+    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;
+  }
+
   // 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.
@@ -96,7 +145,7 @@ ParseAcpiSlit (
       Print (L" (%3d) ", Index);
     }
     Print (L"\n");
-    for (Count = 0; Count< LocalityCount; Count++) {
+    for (Count = 0; Count < LocalityCount; Count++) {
       Print (L" (%3d) ", Count);
       for (Index = 0; Index < LocalityCount; Index++) {
         Print (L"  %3d  ", SLIT_ELEMENT (LocalityPtr, Count, Index));
@@ -106,34 +155,36 @@ ParseAcpiSlit (
   }
 
   // Validate
-  for (Count = 0; Count < LocalityCount; Count++) {
-    for (Index = 0; Index < LocalityCount; Index++) {
-      // Element[x][x] must be equal to 10
-      if ((Count == Index) && (SLIT_ELEMENT (LocalityPtr, Count,Index) != 10)) {
-        IncrementErrorCount ();
-        Print (
-          L"ERROR: Diagonal Element[0x%lx][0x%lx] (%3d)."
-            L" Normalized Value is not 10\n",
-          Count,
-          Index,
-          SLIT_ELEMENT (LocalityPtr, Count, Index)
-          );
-      }
-      // Element[i][j] must be equal to Element[j][i]
-      if (SLIT_ELEMENT (LocalityPtr, Count, Index) !=
-          SLIT_ELEMENT (LocalityPtr, Index, Count)) {
-        IncrementErrorCount ();
-        Print (
-          L"ERROR: Relative distances for Element[0x%lx][0x%lx] (%3d) and \n"
-           L"Element[0x%lx][0x%lx] (%3d) do not match.\n",
-          Count,
-          Index,
-          SLIT_ELEMENT (LocalityPtr, Count, Index),
-          Index,
-          Count,
-          SLIT_ELEMENT (LocalityPtr, Index, Count)
-          );
+  if (GetConsistencyChecking ()) {
+    for (Count = 0; Count < LocalityCount; Count++) {
+      for (Index = 0; Index < LocalityCount; Index++) {
+        // Element[x][x] must be equal to 10
+        if ((Count == Index) && (SLIT_ELEMENT (LocalityPtr, Count,Index) != 10)) {
+          IncrementErrorCount ();
+          Print (
+            L"ERROR: Diagonal Element[0x%lx][0x%lx] (%3d)."
+              L" Normalized Value is not 10\n",
+            Count,
+            Index,
+            SLIT_ELEMENT (LocalityPtr, Count, Index)
+            );
+        }
+        // Element[i][j] must be equal to Element[j][i]
+        if (SLIT_ELEMENT (LocalityPtr, Count, Index) !=
+            SLIT_ELEMENT (LocalityPtr, Index, Count)) {
+          IncrementErrorCount ();
+          Print (
+            L"ERROR: Relative distances for Element[0x%lx][0x%lx] (%3d) and \n"
+            L"Element[0x%lx][0x%lx] (%3d) do not match.\n",
+            Count,
+            Index,
+            SLIT_ELEMENT (LocalityPtr, Count, Index),
+            Index,
+            Count,
+            SLIT_ELEMENT (LocalityPtr, Index, Count)
+            );
+        }
       }
     }
-  }
+  } // if (GetConsistencyChecking ())
 }
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'



  parent reply	other threads:[~2019-07-12  6:53 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 ` Krzysztof Koch [this message]
2019-07-17  9:42   ` [edk2-devel] [PATCH v1 05/11] ShellPkg: acpiview: SLIT: Add error-checking in the parsing logic 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 ` [PATCH v1 08/11] ShellPkg: acpiview: PPTT: " Krzysztof Koch
2019-07-17  9:40   ` [edk2-devel] " 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-6-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