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 06/11] ShellPkg: acpiview: SRAT: Add error-checking in the parsing logic
Date: Fri, 12 Jul 2019 07:52:38 +0100	[thread overview]
Message-ID: <20190712065243.3812-7-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 SRAT table. Report
an error if a SRAT 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. Remove redundant forward function declarations by repositioning
blocks of code.

5. 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/d46d682d28654b1c6263be2f4fd961c35e80e5cb

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

 ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser.c | 113 +++++++++++---------
 1 file changed, 63 insertions(+), 50 deletions(-)

diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser.c
index 075ff2a141a82b522e8aaedb7ad79249aaf5eaac..a12aceb70d273a628387b72437819dc05ad7301e 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser.c
@@ -1,7 +1,7 @@
 /** @file
   SRAT 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 UINT8* SratRAType;
@@ -32,7 +33,13 @@ EFIAPI
 ValidateSratReserved (
   IN UINT8* Ptr,
   IN VOID*  Context
-  );
+  )
+{
+  if (*(UINT32*)Ptr != 1) {
+    IncrementErrorCount ();
+    Print (L"\nERROR: Reserved should be 1 for backward compatibility.\n");
+  }
+}
 
 /**
   This function traces the APIC Proximity Domain field.
@@ -44,9 +51,16 @@ STATIC
 VOID
 EFIAPI
 DumpSratApicProximity (
-  IN  CONST CHAR16*  Format,
-  IN  UINT8*         Ptr
-  );
+ IN CONST CHAR16* Format,
+ IN UINT8*        Ptr
+ )
+{
+  UINT32 ProximityDomain;
+
+  ProximityDomain = Ptr[0] | (Ptr[1] << 8) | (Ptr[2] << 16);
+
+  Print (Format, ProximityDomain);
+}
 
 /**
   An ACPI_PARSER array describing the SRAT Table.
@@ -139,47 +153,6 @@ STATIC CONST ACPI_PARSER SratX2ApciAffinityParser[] = {
   {L"Reserved", 4, 20, L"0x%x", NULL, NULL, NULL, NULL}
 };
 
-/** This function validates the Reserved field in the SRAT table header.
-
-  @param [in] Ptr     Pointer to the start of the field data.
-  @param [in] Context Pointer to context specific information e.g. this
-                      could be a pointer to the ACPI table header.
-**/
-STATIC
-VOID
-EFIAPI
-ValidateSratReserved (
-  IN UINT8* Ptr,
-  IN VOID*  Context
-  )
-{
-  if (*(UINT32*)Ptr != 1) {
-    IncrementErrorCount ();
-    Print (L"\nERROR: Reserved should be 1 for backward compatibility.\n");
-  }
-}
-
-/**
-  This function traces the APIC Proximity Domain field.
-
-  @param [in] Format  Format string for tracing the data.
-  @param [in] Ptr     Pointer to the start of the buffer.
-**/
-STATIC
-VOID
-EFIAPI
-DumpSratApicProximity (
- IN CONST CHAR16* Format,
- IN UINT8*        Ptr
- )
-{
-  UINT32 ProximityDomain;
-
-  ProximityDomain = Ptr[0] | (Ptr[1] << 8) | (Ptr[2] << 16);
-
-  Print (Format, ProximityDomain);
-}
-
 /**
   This function parses the ACPI SRAT table.
   When trace is enabled this function parses the SRAT table and
@@ -234,6 +207,7 @@ ParseAcpiSrat (
              AcpiTableLength,
              PARSER_PARAMS (SratParser)
              );
+
   ResourcePtr = Ptr + Offset;
 
   while (Offset < AcpiTableLength) {
@@ -242,10 +216,47 @@ ParseAcpiSrat (
       0,
       NULL,
       ResourcePtr,
-      2,  // The length is 1 byte at offset 1
+      AcpiTableLength - Offset,
       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 forward progress is made.
+    if (*SratRALength < 2) {
+      IncrementErrorCount ();
+      Print (
+        L"ERROR: Structure length is too small: SratRALength = %d. " \
+          L"SratRAType = %d. SRAT parsing aborted.\n",
+        *SratRALength,
+        *SratRAType
+        );
+      return;
+    }
+
+    // Make sure the SRAT structure lies inside the table
+    if ((Offset + *SratRALength) > AcpiTableLength) {
+      IncrementErrorCount ();
+      Print (
+        L"ERROR: Invalid SRAT structure length. SratRALength = %d. " \
+          L"RemainingTableBufferLength = %d. SRAT parsing aborted.\n",
+        *SratRALength,
+        AcpiTableLength - Offset
+        );
+      return;
+    }
+
     switch (*SratRAType) {
       case EFI_ACPI_6_2_GICC_AFFINITY:
         AsciiSPrint (
@@ -278,7 +289,7 @@ ParseAcpiSrat (
           ResourcePtr,
           *SratRALength,
           PARSER_PARAMS (SratGicITSAffinityParser)
-        );
+          );
         break;
 
       case EFI_ACPI_6_2_MEMORY_AFFINITY:
@@ -333,8 +344,10 @@ ParseAcpiSrat (
         break;
 
       default:
-        IncrementErrorCount ();
-        Print (L"ERROR: Unknown SRAT Affinity type = 0x%x\n", *SratRAType);
+        if (GetConsistencyChecking ()) {
+          IncrementErrorCount ();
+          Print (L"ERROR: Unknown SRAT Affinity type = 0x%x\n", *SratRAType);
+        }
         break;
     }
 
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'



  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 ` Krzysztof Koch [this message]
2019-07-17  9:41   ` [edk2-devel] [PATCH v1 06/11] ShellPkg: acpiview: SRAT: " 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-7-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