public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [PATCH v1 1/1] ShellPkg: acpiview: Validate ACPI table 'Length' field
@ 2020-01-30 16:19 Krzysztof Koch
  2020-01-30 16:36 ` Sami Mujawar
  2020-02-11  3:03 ` [edk2-devel] " Gao, Zhichao
  0 siblings, 2 replies; 3+ messages in thread
From: Krzysztof Koch @ 2020-01-30 16:19 UTC (permalink / raw)
  To: devel; +Cc: ray.ni, zhichao.gao, Matteo.Carlini, sami.mujawar, nd

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_v1

Notes:
    v1:
    - Validate ACPI table length [Krzysztof]

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

diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiTableParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiTableParser.c
index d5500bcb2b4a55c7a69f45444aa49d36d2c1694f..0c93bca4fc0f7d2f105a7654258e00f714fc1519 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,25 @@ 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)'



^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH v1 1/1] ShellPkg: acpiview: Validate ACPI table 'Length' field
  2020-01-30 16:19 [PATCH v1 1/1] ShellPkg: acpiview: Validate ACPI table 'Length' field Krzysztof Koch
@ 2020-01-30 16:36 ` Sami Mujawar
  2020-02-11  3:03 ` [edk2-devel] " Gao, Zhichao
  1 sibling, 0 replies; 3+ messages in thread
From: Sami Mujawar @ 2020-01-30 16:36 UTC (permalink / raw)
  To: Krzysztof Koch, devel@edk2.groups.io
  Cc: ray.ni@intel.com, zhichao.gao@intel.com, Matteo Carlini, nd,
	Laura Moretta

Reviewed-by: Sami Mujawar <sami.mujawar@arm.com>

Regards,

Sami Mujawar

-----Original Message-----
From: Krzysztof Koch <krzysztof.koch@arm.com> 
Sent: 30 January 2020 16:20
To: devel@edk2.groups.io
Cc: ray.ni@intel.com; zhichao.gao@intel.com; Matteo Carlini <Matteo.Carlini@arm.com>; Sami Mujawar <Sami.Mujawar@arm.com>; nd <nd@arm.com>
Subject: [PATCH v1 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_acpi_table_v1

Notes:
    v1:
    - Validate ACPI table length [Krzysztof]

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

diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiTableParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiTableParser.c
index d5500bcb2b4a55c7a69f45444aa49d36d2c1694f..0c93bca4fc0f7d2f105a7654258e00f714fc1519 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,25 @@ 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)'



^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [edk2-devel] [PATCH v1 1/1] ShellPkg: acpiview: Validate ACPI table 'Length' field
  2020-01-30 16:19 [PATCH v1 1/1] ShellPkg: acpiview: Validate ACPI table 'Length' field Krzysztof Koch
  2020-01-30 16:36 ` Sami Mujawar
@ 2020-02-11  3:03 ` Gao, Zhichao
  1 sibling, 0 replies; 3+ messages in thread
From: Gao, Zhichao @ 2020-02-11  3:03 UTC (permalink / raw)
  To: devel@edk2.groups.io, krzysztof.koch@arm.com
  Cc: Ni, Ray, Matteo.Carlini@arm.com, sami.mujawar@arm.com, nd@arm.com

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Krzysztof
> Koch
> Sent: Friday, January 31, 2020 12:20 AM
> 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 v1 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_v1
> 
> Notes:
>     v1:
>     - Validate ACPI table length [Krzysztof]
> 
>  ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiTableParser.c | 22
> +++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiTableParser.c
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiTableParser.c
> index
> d5500bcb2b4a55c7a69f45444aa49d36d2c1694f..0c93bca4fc0f7d2f105a765425
> 8e00f714fc1519 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,25 @@ 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.
> +    */

The internal comment is suggested to use C++ style, refer to CSS 2.1 section 6.5.2.1:
For internal code comments, use C++ style (//) comment lines.

Thanks,
Zhichao

> +    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)'
> 
> 
> 
> 


^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2020-02-11  3:03 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-01-30 16:19 [PATCH v1 1/1] ShellPkg: acpiview: Validate ACPI table 'Length' field Krzysztof Koch
2020-01-30 16:36 ` Sami Mujawar
2020-02-11  3:03 ` [edk2-devel] " Gao, Zhichao

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox