[PATCH v2 1/1] ShellPkg: acpiview: Validate ACPI table 'Length' field

[PATCH v2 1/1] ShellPkg: acpiview: Validate ACPI table 'Length' field

[Krzysztof Koch]

Check if the ACPI table length, as reported in the ACPI table header, is
big enough to fit at least the header itself.

If not, report an error to the user and stop parsing the table in order
to prevent buffer overruns.

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

Changes can be seet at: https://github.com/KrzysztofKoch1/edk2/pull/new/650_add_checks_process_acpi_table_v2

Notes:
    v2:
    - Change code comment style [Zhichao]

    v1:
    - Validate ACPI table length [Krzysztof]

 ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiTableParser.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiTableParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiTableParser.c
index d5500bcb2b4a55c7a69f45444aa49d36d2c1694f..501967c4dde680809c56e5d79ed744a1013a69e1 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiTableParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiTableParser.c
@@ -1,7 +1,7 @@
 /** @file
   ACPI 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
 **/
 
@@ -176,6 +176,7 @@ ProcessAcpiTable (
   CONST UINT32* AcpiTableSignature;
   CONST UINT32* AcpiTableLength;
   CONST UINT8*  AcpiTableRevision;
+  CONST UINT8*  SignaturePtr;
   PARSE_ACPI_TABLE_PROC ParserProc;
 
   ParseAcpiHeader (
@@ -193,6 +194,23 @@ ProcessAcpiTable (
 
   if (Trace) {
     DumpRaw (Ptr, *AcpiTableLength);
+
+    // Do not process the ACPI table any further if the table length read
+    // is invalid. The ACPI table should at least contain the table header.
+    if (*AcpiTableLength < sizeof (EFI_ACPI_DESCRIPTION_HEADER)) {
+      SignaturePtr = (CONST UINT8*)AcpiTableSignature;
+      IncrementErrorCount ();
+      Print (
+        L"ERROR: Invalid %c%c%c%c table length. Length = %d\n",
+        SignaturePtr[0],
+        SignaturePtr[1],
+        SignaturePtr[2],
+        SignaturePtr[3],
+        *AcpiTableLength
+        );
+      return;
+    }
+
     if (GetConsistencyChecking ()) {
       VerifyChecksum (TRUE, Ptr, *AcpiTableLength);
     }
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'

Re: [edk2-devel] [PATCH v2 1/1] ShellPkg: acpiview: Validate ACPI table 'Length' field

[Gao, Zhichao]

Reviewed-by: Zhichao Gao <zhichao.gao@intel.com>

Thanks,
Zhichao

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Krzysztof
> Koch
> Sent: Tuesday, February 11, 2020 6:01 PM
> To: devel@edk2.groups.io
> Cc: Ni, Ray <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>;
> Matteo.Carlini@arm.com; sami.mujawar@arm.com; nd@arm.com
> Subject: [edk2-devel] [PATCH v2 1/1] ShellPkg: acpiview: Validate ACPI table
> 'Length' field
> 
> Check if the ACPI table length, as reported in the ACPI table header, is big
> enough to fit at least the header itself.
> 
> If not, report an error to the user and stop parsing the table in order to prevent
> buffer overruns.
> 
> Signed-off-by: Krzysztof Koch <krzysztof.koch@arm.com>
> ---
> 
> Changes can be seet at:
> https://github.com/KrzysztofKoch1/edk2/pull/new/650_add_checks_process_a
> cpi_table_v2
> 
> Notes:
>     v2:
>     - Change code comment style [Zhichao]
> 
>     v1:
>     - Validate ACPI table length [Krzysztof]
> 
>  ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiTableParser.c | 20
> +++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiTableParser.c
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiTableParser.c
> index
> d5500bcb2b4a55c7a69f45444aa49d36d2c1694f..501967c4dde680809c56e5d79
> ed744a1013a69e1 100644
> --- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiTableParser.c
> +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiTableParser.c
> @@ -1,7 +1,7 @@
>  /** @file
>    ACPI 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  **/
> 
> @@ -176,6 +176,7 @@ ProcessAcpiTable (
>    CONST UINT32* AcpiTableSignature;
>    CONST UINT32* AcpiTableLength;
>    CONST UINT8*  AcpiTableRevision;
> +  CONST UINT8*  SignaturePtr;
>    PARSE_ACPI_TABLE_PROC ParserProc;
> 
>    ParseAcpiHeader (
> @@ -193,6 +194,23 @@ ProcessAcpiTable (
> 
>    if (Trace) {
>      DumpRaw (Ptr, *AcpiTableLength);
> +
> +    // Do not process the ACPI table any further if the table length read
> +    // is invalid. The ACPI table should at least contain the table header.
> +    if (*AcpiTableLength < sizeof (EFI_ACPI_DESCRIPTION_HEADER)) {
> +      SignaturePtr = (CONST UINT8*)AcpiTableSignature;
> +      IncrementErrorCount ();
> +      Print (
> +        L"ERROR: Invalid %c%c%c%c table length. Length = %d\n",
> +        SignaturePtr[0],
> +        SignaturePtr[1],
> +        SignaturePtr[2],
> +        SignaturePtr[3],
> +        *AcpiTableLength
> +        );
> +      return;
> +    }
> +
>      if (GetConsistencyChecking ()) {
>        VerifyChecksum (TRUE, Ptr, *AcpiTableLength);
>      }
> --
> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
> 
> 
>