* [PATCH v1 1/1] ShellPkg: acpiview: Prevent infinite loop if structure length is 0
@ 2020-02-14 13:59 Krzysztof Koch
2020-02-17 15:11 ` [edk2-devel] " Liming Gao
2020-02-17 15:35 ` Sami Mujawar
0 siblings, 2 replies; 7+ messages in thread
From: Krzysztof Koch @ 2020-02-14 13:59 UTC (permalink / raw)
To: devel; +Cc: ray.ni, zhichao.gao, Sami.Mujawar, Matteo.Carlini, nd
Extend validation of ACPI structure lengths which are read from the
ACPI table being parsed. Additionally check if the structure 'Length'
field value is positive. If not, stop parsing the faulting table.
Some ACPI tables define internal structures of variable size. The
'Length' field inside the substructure is used to update a pointer used
for table traversal. If the byte-length of the structure is equal to 0,
acpiview can enter an infinite loop. This condition can occur if, for
example, the zero-allocated ACPI table buffer is not fully populated.
This is typically a bug on the ACPI table writer side.
In short, this method helps acpiview recover gracefully from a
zero-valued ACPI structure length.
Signed-off-by: Krzysztof Koch <krzysztof.koch@arm.com>
---
Changes can be seen at: https://github.com/KrzysztofKoch1/edk2/tree/612_acpiview_prevent_inf_loops_v1
Notes:
v1:
- prevent infinite loops in acpiview parsers [Krzysztof]
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c | 15 ++++++-----
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser.c | 13 ++++-----
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser.c | 14 +++++-----
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.c | 28 ++++++--------------
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c | 15 ++++++-----
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser.c | 14 +++++-----
6 files changed, 47 insertions(+), 52 deletions(-)
diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c
index 0f730a306a94329a23fbaf54b59f1833b44616ba..9df111ecaa7d7a703a13a39c243ed78b9f12ee97 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c
@@ -1,7 +1,7 @@
/** @file
DBG2 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
@par Reference(s):
@@ -282,15 +282,16 @@ ParseAcpiDbg2 (
return;
}
- // Make sure the Debug Device Information structure lies inside the table.
- if ((Offset + *DbgDevInfoLen) > AcpiTableLength) {
+ // Validate Debug Device Information Structure length
+ if ((*DbgDevInfoLen == 0) ||
+ ((Offset + (*DbgDevInfoLen)) > AcpiTableLength)) {
IncrementErrorCount ();
Print (
- L"ERROR: Invalid Debug Device Information structure length. " \
- L"DbgDevInfoLen = %d. RemainingTableBufferLength = %d. " \
- L"DBG2 parsing aborted.\n",
+ L"ERROR: Invalid Debug Device Information Structure length. " \
+ L"Length = %d. Offset = %d. AcpiTableLength = %d.\n",
*DbgDevInfoLen,
- AcpiTableLength - Offset
+ Offset,
+ AcpiTableLength
);
return;
}
diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser.c
index 699a55b549ec3fa61bbd156898821055dc019199..bdd30ff45c61142c071ead63a27babab8998721b 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser.c
@@ -1,7 +1,7 @@
/** @file
GTDT 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
@par Reference(s):
@@ -327,15 +327,16 @@ ParseAcpiGtdt (
return;
}
- // Make sure the Platform Timer is inside the table.
- if ((Offset + *PlatformTimerLength) > AcpiTableLength) {
+ // Validate Platform Timer Structure length
+ if ((*PlatformTimerLength == 0) ||
+ ((Offset + (*PlatformTimerLength)) > AcpiTableLength)) {
IncrementErrorCount ();
Print (
L"ERROR: Invalid Platform Timer Structure length. " \
- L"PlatformTimerLength = %d. RemainingTableBufferLength = %d. " \
- L"GTDT parsing aborted.\n",
+ L"Length = %d. Offset = %d. AcpiTableLength = %d.\n",
*PlatformTimerLength,
- AcpiTableLength - Offset
+ Offset,
+ AcpiTableLength
);
return;
}
diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser.c
index 9d5d937c7b2c19945ca2ad3eba644bdfc09cc3f6..9a006a01448b897865cd7cd85651c816933acf05 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser.c
@@ -1,7 +1,7 @@
/** @file
IORT 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
@par Reference(s):
@@ -687,14 +687,16 @@ ParseAcpiIort (
return;
}
- // Make sure the IORT Node is inside the table
- if ((Offset + (*IortNodeLength)) > AcpiTableLength) {
+ // Validate IORT Node length
+ if ((*IortNodeLength == 0) ||
+ ((Offset + (*IortNodeLength)) > AcpiTableLength)) {
IncrementErrorCount ();
Print (
- L"ERROR: Invalid IORT node length. IortNodeLength = %d. " \
- L"RemainingTableBufferLength = %d. IORT parsing aborted.\n",
+ L"ERROR: Invalid IORT Node length. " \
+ L"Length = %d. Offset = %d. AcpiTableLength = %d.\n",
*IortNodeLength,
- AcpiTableLength - Offset
+ Offset,
+ AcpiTableLength
);
return;
}
diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.c
index 438905cb24f58b8b82e8fe61280e72f765d578d8..f85d2b36532cfc5db36fe7bef9830cccc64969cc 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.c
@@ -1,7 +1,7 @@
/** @file
MADT 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
@par Reference(s):
@@ -273,28 +273,16 @@ ParseAcpiMadt (
return;
}
- // Make sure forward progress is made.
- if (*MadtInterruptControllerLength < 2) {
+ // Validate Interrupt Controller Structure length
+ if ((*MadtInterruptControllerLength == 0) ||
+ ((Offset + (*MadtInterruptControllerLength)) > AcpiTableLength)) {
IncrementErrorCount ();
Print (
- L"ERROR: Structure length is too small: " \
- L"MadtInterruptControllerLength = %d. " \
- L"MadtInterruptControllerType = %d. MADT parsing aborted.\n",
+ L"ERROR: Invalid Interrupt Controller Structure length. " \
+ L"Length = %d. Offset = %d. AcpiTableLength = %d.\n",
*MadtInterruptControllerLength,
- *MadtInterruptControllerType
- );
- return;
- }
-
- // Make sure the MADT structure lies inside the table
- if ((Offset + *MadtInterruptControllerLength) > AcpiTableLength) {
- IncrementErrorCount ();
- Print (
- L"ERROR: Invalid MADT structure length. " \
- L"MadtInterruptControllerLength = %d. " \
- L"RemainingTableBufferLength = %d. MADT parsing aborted.\n",
- *MadtInterruptControllerLength,
- AcpiTableLength - Offset
+ Offset,
+ AcpiTableLength
);
return;
}
diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c
index 675ba75f02b367cd5ad9f2ac23c30ed0ab58f286..0db272c16af0ad8824c8da4c88dd409c8550112a 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c
@@ -1,7 +1,7 @@
/** @file
PPTT table parser
- Copyright (c) 2019, ARM Limited. All rights reserved.
+ Copyright (c) 2019 - 2020, ARM Limited. All rights reserved.
SPDX-License-Identifier: BSD-2-Clause-Patent
@par Reference(s):
@@ -425,15 +425,16 @@ ParseAcpiPptt (
return;
}
- // Make sure the PPTT structure lies inside the table
- if ((Offset + *ProcessorTopologyStructureLength) > AcpiTableLength) {
+ // Validate Processor Topology Structure length
+ if ((*ProcessorTopologyStructureLength == 0) ||
+ ((Offset + (*ProcessorTopologyStructureLength)) > AcpiTableLength)) {
IncrementErrorCount ();
Print (
- L"ERROR: Invalid PPTT structure length. " \
- L"ProcessorTopologyStructureLength = %d. " \
- L"RemainingTableBufferLength = %d. PPTT parsing aborted.\n",
+ L"ERROR: Invalid Processor Topology Structure length. " \
+ L"Length = %d. Offset = %d. AcpiTableLength = %d.\n",
*ProcessorTopologyStructureLength,
- AcpiTableLength - Offset
+ Offset,
+ AcpiTableLength
);
return;
}
diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser.c
index 3613900ae322483fdd3d3383de4e22ba75b2128b..6f66be68cc0bed14811a0432c61a79fd47c54890 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 - 2019, ARM Limited. All rights reserved.
+ Copyright (c) 2016 - 2020, ARM Limited. All rights reserved.
SPDX-License-Identifier: BSD-2-Clause-Patent
@par Reference(s):
@@ -412,14 +412,16 @@ ParseAcpiSrat (
return;
}
- // Make sure the SRAT structure lies inside the table
- if ((Offset + *SratRALength) > AcpiTableLength) {
+ // Validate Static Resource Allocation Structure length
+ if ((*SratRALength == 0) ||
+ ((Offset + (*SratRALength)) > AcpiTableLength)) {
IncrementErrorCount ();
Print (
- L"ERROR: Invalid SRAT structure length. SratRALength = %d. " \
- L"RemainingTableBufferLength = %d. SRAT parsing aborted.\n",
+ L"ERROR: Invalid Static Resource Allocation Structure length. " \
+ L"Length = %d. Offset = %d. AcpiTableLength = %d.\n",
*SratRALength,
- AcpiTableLength - Offset
+ Offset,
+ AcpiTableLength
);
return;
}
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [edk2-devel] [PATCH v1 1/1] ShellPkg: acpiview: Prevent infinite loop if structure length is 0
2020-02-14 13:59 [PATCH v1 1/1] ShellPkg: acpiview: Prevent infinite loop if structure length is 0 Krzysztof Koch
@ 2020-02-17 15:11 ` Liming Gao
2020-02-17 15:22 ` Krzysztof Koch
[not found] ` <15F439D65D9DE013.5373@groups.io>
2020-02-17 15:35 ` Sami Mujawar
1 sibling, 2 replies; 7+ messages in thread
From: Liming Gao @ 2020-02-17 15:11 UTC (permalink / raw)
To: devel@edk2.groups.io, krzysztof.koch@arm.com
Cc: Ni, Ray, Gao, Zhichao, Sami.Mujawar@arm.com,
Matteo.Carlini@arm.com, nd@arm.com
Krzysztof:
Is there one BZ for this issue? Does this patch catch to this edk2 stable tag 202002?
Thanks
Liming
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Krzysztof Koch
> Sent: Friday, February 14, 2020 9:59 PM
> To: devel@edk2.groups.io
> Cc: Ni, Ray <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>; Sami.Mujawar@arm.com; Matteo.Carlini@arm.com;
> nd@arm.com
> Subject: [edk2-devel] [PATCH v1 1/1] ShellPkg: acpiview: Prevent infinite loop if structure length is 0
>
> Extend validation of ACPI structure lengths which are read from the
> ACPI table being parsed. Additionally check if the structure 'Length'
> field value is positive. If not, stop parsing the faulting table.
>
> Some ACPI tables define internal structures of variable size. The
> 'Length' field inside the substructure is used to update a pointer used
> for table traversal. If the byte-length of the structure is equal to 0,
> acpiview can enter an infinite loop. This condition can occur if, for
> example, the zero-allocated ACPI table buffer is not fully populated.
> This is typically a bug on the ACPI table writer side.
>
> In short, this method helps acpiview recover gracefully from a
> zero-valued ACPI structure length.
>
> Signed-off-by: Krzysztof Koch <krzysztof.koch@arm.com>
> ---
>
> Changes can be seen at: https://github.com/KrzysztofKoch1/edk2/tree/612_acpiview_prevent_inf_loops_v1
>
> Notes:
> v1:
> - prevent infinite loops in acpiview parsers [Krzysztof]
>
> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c | 15 ++++++-----
> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser.c | 13 ++++-----
> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser.c | 14 +++++-----
> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.c | 28 ++++++--------------
> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c | 15 ++++++-----
> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser.c | 14 +++++-----
> 6 files changed, 47 insertions(+), 52 deletions(-)
>
> diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c
> index 0f730a306a94329a23fbaf54b59f1833b44616ba..9df111ecaa7d7a703a13a39c243ed78b9f12ee97 100644
> --- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c
> +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c
> @@ -1,7 +1,7 @@
> /** @file
> DBG2 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
>
> @par Reference(s):
> @@ -282,15 +282,16 @@ ParseAcpiDbg2 (
> return;
> }
>
> - // Make sure the Debug Device Information structure lies inside the table.
> - if ((Offset + *DbgDevInfoLen) > AcpiTableLength) {
> + // Validate Debug Device Information Structure length
> + if ((*DbgDevInfoLen == 0) ||
> + ((Offset + (*DbgDevInfoLen)) > AcpiTableLength)) {
> IncrementErrorCount ();
> Print (
> - L"ERROR: Invalid Debug Device Information structure length. " \
> - L"DbgDevInfoLen = %d. RemainingTableBufferLength = %d. " \
> - L"DBG2 parsing aborted.\n",
> + L"ERROR: Invalid Debug Device Information Structure length. " \
> + L"Length = %d. Offset = %d. AcpiTableLength = %d.\n",
> *DbgDevInfoLen,
> - AcpiTableLength - Offset
> + Offset,
> + AcpiTableLength
> );
> return;
> }
> diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser.c
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser.c
> index 699a55b549ec3fa61bbd156898821055dc019199..bdd30ff45c61142c071ead63a27babab8998721b 100644
> --- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser.c
> +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser.c
> @@ -1,7 +1,7 @@
> /** @file
> GTDT 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
>
> @par Reference(s):
> @@ -327,15 +327,16 @@ ParseAcpiGtdt (
> return;
> }
>
> - // Make sure the Platform Timer is inside the table.
> - if ((Offset + *PlatformTimerLength) > AcpiTableLength) {
> + // Validate Platform Timer Structure length
> + if ((*PlatformTimerLength == 0) ||
> + ((Offset + (*PlatformTimerLength)) > AcpiTableLength)) {
> IncrementErrorCount ();
> Print (
> L"ERROR: Invalid Platform Timer Structure length. " \
> - L"PlatformTimerLength = %d. RemainingTableBufferLength = %d. " \
> - L"GTDT parsing aborted.\n",
> + L"Length = %d. Offset = %d. AcpiTableLength = %d.\n",
> *PlatformTimerLength,
> - AcpiTableLength - Offset
> + Offset,
> + AcpiTableLength
> );
> return;
> }
> diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser.c
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser.c
> index 9d5d937c7b2c19945ca2ad3eba644bdfc09cc3f6..9a006a01448b897865cd7cd85651c816933acf05 100644
> --- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser.c
> +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser.c
> @@ -1,7 +1,7 @@
> /** @file
> IORT 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
>
> @par Reference(s):
> @@ -687,14 +687,16 @@ ParseAcpiIort (
> return;
> }
>
> - // Make sure the IORT Node is inside the table
> - if ((Offset + (*IortNodeLength)) > AcpiTableLength) {
> + // Validate IORT Node length
> + if ((*IortNodeLength == 0) ||
> + ((Offset + (*IortNodeLength)) > AcpiTableLength)) {
> IncrementErrorCount ();
> Print (
> - L"ERROR: Invalid IORT node length. IortNodeLength = %d. " \
> - L"RemainingTableBufferLength = %d. IORT parsing aborted.\n",
> + L"ERROR: Invalid IORT Node length. " \
> + L"Length = %d. Offset = %d. AcpiTableLength = %d.\n",
> *IortNodeLength,
> - AcpiTableLength - Offset
> + Offset,
> + AcpiTableLength
> );
> return;
> }
> diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.c
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.c
> index 438905cb24f58b8b82e8fe61280e72f765d578d8..f85d2b36532cfc5db36fe7bef9830cccc64969cc 100644
> --- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.c
> +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.c
> @@ -1,7 +1,7 @@
> /** @file
> MADT 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
>
> @par Reference(s):
> @@ -273,28 +273,16 @@ ParseAcpiMadt (
> return;
> }
>
> - // Make sure forward progress is made.
> - if (*MadtInterruptControllerLength < 2) {
> + // Validate Interrupt Controller Structure length
> + if ((*MadtInterruptControllerLength == 0) ||
> + ((Offset + (*MadtInterruptControllerLength)) > AcpiTableLength)) {
> IncrementErrorCount ();
> Print (
> - L"ERROR: Structure length is too small: " \
> - L"MadtInterruptControllerLength = %d. " \
> - L"MadtInterruptControllerType = %d. MADT parsing aborted.\n",
> + L"ERROR: Invalid Interrupt Controller Structure length. " \
> + L"Length = %d. Offset = %d. AcpiTableLength = %d.\n",
> *MadtInterruptControllerLength,
> - *MadtInterruptControllerType
> - );
> - return;
> - }
> -
> - // Make sure the MADT structure lies inside the table
> - if ((Offset + *MadtInterruptControllerLength) > AcpiTableLength) {
> - IncrementErrorCount ();
> - Print (
> - L"ERROR: Invalid MADT structure length. " \
> - L"MadtInterruptControllerLength = %d. " \
> - L"RemainingTableBufferLength = %d. MADT parsing aborted.\n",
> - *MadtInterruptControllerLength,
> - AcpiTableLength - Offset
> + Offset,
> + AcpiTableLength
> );
> return;
> }
> diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c
> index 675ba75f02b367cd5ad9f2ac23c30ed0ab58f286..0db272c16af0ad8824c8da4c88dd409c8550112a 100644
> --- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c
> +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c
> @@ -1,7 +1,7 @@
> /** @file
> PPTT table parser
>
> - Copyright (c) 2019, ARM Limited. All rights reserved.
> + Copyright (c) 2019 - 2020, ARM Limited. All rights reserved.
> SPDX-License-Identifier: BSD-2-Clause-Patent
>
> @par Reference(s):
> @@ -425,15 +425,16 @@ ParseAcpiPptt (
> return;
> }
>
> - // Make sure the PPTT structure lies inside the table
> - if ((Offset + *ProcessorTopologyStructureLength) > AcpiTableLength) {
> + // Validate Processor Topology Structure length
> + if ((*ProcessorTopologyStructureLength == 0) ||
> + ((Offset + (*ProcessorTopologyStructureLength)) > AcpiTableLength)) {
> IncrementErrorCount ();
> Print (
> - L"ERROR: Invalid PPTT structure length. " \
> - L"ProcessorTopologyStructureLength = %d. " \
> - L"RemainingTableBufferLength = %d. PPTT parsing aborted.\n",
> + L"ERROR: Invalid Processor Topology Structure length. " \
> + L"Length = %d. Offset = %d. AcpiTableLength = %d.\n",
> *ProcessorTopologyStructureLength,
> - AcpiTableLength - Offset
> + Offset,
> + AcpiTableLength
> );
> return;
> }
> diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser.c
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser.c
> index 3613900ae322483fdd3d3383de4e22ba75b2128b..6f66be68cc0bed14811a0432c61a79fd47c54890 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 - 2019, ARM Limited. All rights reserved.
> + Copyright (c) 2016 - 2020, ARM Limited. All rights reserved.
> SPDX-License-Identifier: BSD-2-Clause-Patent
>
> @par Reference(s):
> @@ -412,14 +412,16 @@ ParseAcpiSrat (
> return;
> }
>
> - // Make sure the SRAT structure lies inside the table
> - if ((Offset + *SratRALength) > AcpiTableLength) {
> + // Validate Static Resource Allocation Structure length
> + if ((*SratRALength == 0) ||
> + ((Offset + (*SratRALength)) > AcpiTableLength)) {
> IncrementErrorCount ();
> Print (
> - L"ERROR: Invalid SRAT structure length. SratRALength = %d. " \
> - L"RemainingTableBufferLength = %d. SRAT parsing aborted.\n",
> + L"ERROR: Invalid Static Resource Allocation Structure length. " \
> + L"Length = %d. Offset = %d. AcpiTableLength = %d.\n",
> *SratRALength,
> - AcpiTableLength - Offset
> + Offset,
> + AcpiTableLength
> );
> return;
> }
> --
> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
>
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [edk2-devel] [PATCH v1 1/1] ShellPkg: acpiview: Prevent infinite loop if structure length is 0
2020-02-17 15:11 ` [edk2-devel] " Liming Gao
@ 2020-02-17 15:22 ` Krzysztof Koch
2020-02-17 15:24 ` Liming Gao
[not found] ` <15F439D65D9DE013.5373@groups.io>
1 sibling, 1 reply; 7+ messages in thread
From: Krzysztof Koch @ 2020-02-17 15:22 UTC (permalink / raw)
To: devel@edk2.groups.io, liming.gao@intel.com
Cc: Ni, Ray, Gao, Zhichao, Sami Mujawar, Matteo Carlini, nd
Hi Liming,
I haven't created a BZ yet, shall I create one? It would be great if the patch makes it to the stable tag.
Over the last few months I added some security features to acpiview. They make this debug tool less sensitive to exploits from ACPI tables. This patch completes my efforts in making the tool more reliable.
Kind regards,
Krzysztof
-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Liming Gao via Groups.Io
Sent: Monday, February 17, 2020 15:11
To: devel@edk2.groups.io; Krzysztof Koch <Krzysztof.Koch@arm.com>
Cc: Ni, Ray <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>; Sami Mujawar <Sami.Mujawar@arm.com>; Matteo Carlini <Matteo.Carlini@arm.com>; nd <nd@arm.com>
Subject: Re: [edk2-devel] [PATCH v1 1/1] ShellPkg: acpiview: Prevent infinite loop if structure length is 0
Krzysztof:
Is there one BZ for this issue? Does this patch catch to this edk2 stable tag 202002?
Thanks
Liming
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> Krzysztof Koch
> Sent: Friday, February 14, 2020 9:59 PM
> To: devel@edk2.groups.io
> Cc: Ni, Ray <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>;
> Sami.Mujawar@arm.com; Matteo.Carlini@arm.com; nd@arm.com
> Subject: [edk2-devel] [PATCH v1 1/1] ShellPkg: acpiview: Prevent
> infinite loop if structure length is 0
>
> Extend validation of ACPI structure lengths which are read from the
> ACPI table being parsed. Additionally check if the structure 'Length'
> field value is positive. If not, stop parsing the faulting table.
>
> Some ACPI tables define internal structures of variable size. The
> 'Length' field inside the substructure is used to update a pointer
> used for table traversal. If the byte-length of the structure is equal
> to 0, acpiview can enter an infinite loop. This condition can occur
> if, for example, the zero-allocated ACPI table buffer is not fully populated.
> This is typically a bug on the ACPI table writer side.
>
> In short, this method helps acpiview recover gracefully from a
> zero-valued ACPI structure length.
>
> Signed-off-by: Krzysztof Koch <krzysztof.koch@arm.com>
> ---
>
> Changes can be seen at:
> https://github.com/KrzysztofKoch1/edk2/tree/612_acpiview_prevent_inf_l
> oops_v1
>
> Notes:
> v1:
> - prevent infinite loops in acpiview parsers [Krzysztof]
>
>
> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c
> | 15 ++++++-----
> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser.c
> | 13 ++++-----
> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser.c
> | 14 +++++-----
> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.c
> | 28 ++++++--------------
> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c
> | 15 ++++++-----
> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser.c
> | 14 +++++-----
> 6 files changed, 47 insertions(+), 52 deletions(-)
>
> diff --git
> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser
> .c
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser
> .c index
> 0f730a306a94329a23fbaf54b59f1833b44616ba..9df111ecaa7d7a703a13a39c243e
> d78b9f12ee97 100644
> ---
> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser
> .c
> +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Pa
> +++ rser.c
> @@ -1,7 +1,7 @@
> /** @file
> DBG2 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
>
> @par Reference(s):
> @@ -282,15 +282,16 @@ ParseAcpiDbg2 (
> return;
> }
>
> - // Make sure the Debug Device Information structure lies inside the table.
> - if ((Offset + *DbgDevInfoLen) > AcpiTableLength) {
> + // Validate Debug Device Information Structure length
> + if ((*DbgDevInfoLen == 0) ||
> + ((Offset + (*DbgDevInfoLen)) > AcpiTableLength)) {
> IncrementErrorCount ();
> Print (
> - L"ERROR: Invalid Debug Device Information structure length. " \
> - L"DbgDevInfoLen = %d. RemainingTableBufferLength = %d. " \
> - L"DBG2 parsing aborted.\n",
> + L"ERROR: Invalid Debug Device Information Structure length. " \
> + L"Length = %d. Offset = %d. AcpiTableLength = %d.\n",
> *DbgDevInfoLen,
> - AcpiTableLength - Offset
> + Offset,
> + AcpiTableLength
> );
> return;
> }
> diff --git
> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser
> .c
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser
> .c index
> 699a55b549ec3fa61bbd156898821055dc019199..bdd30ff45c61142c071ead63a27b
> abab8998721b 100644
> ---
> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser
> .c
> +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtPa
> +++ rser.c
> @@ -1,7 +1,7 @@
> /** @file
> GTDT 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
>
> @par Reference(s):
> @@ -327,15 +327,16 @@ ParseAcpiGtdt (
> return;
> }
>
> - // Make sure the Platform Timer is inside the table.
> - if ((Offset + *PlatformTimerLength) > AcpiTableLength) {
> + // Validate Platform Timer Structure length
> + if ((*PlatformTimerLength == 0) ||
> + ((Offset + (*PlatformTimerLength)) > AcpiTableLength)) {
> IncrementErrorCount ();
> Print (
> L"ERROR: Invalid Platform Timer Structure length. " \
> - L"PlatformTimerLength = %d. RemainingTableBufferLength = %d. " \
> - L"GTDT parsing aborted.\n",
> + L"Length = %d. Offset = %d. AcpiTableLength = %d.\n",
> *PlatformTimerLength,
> - AcpiTableLength - Offset
> + Offset,
> + AcpiTableLength
> );
> return;
> }
> diff --git
> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser
> .c
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser
> .c index
> 9d5d937c7b2c19945ca2ad3eba644bdfc09cc3f6..9a006a01448b897865cd7cd85651
> c816933acf05 100644
> ---
> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser
> .c
> +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortPa
> +++ rser.c
> @@ -1,7 +1,7 @@
> /** @file
> IORT 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
>
> @par Reference(s):
> @@ -687,14 +687,16 @@ ParseAcpiIort (
> return;
> }
>
> - // Make sure the IORT Node is inside the table
> - if ((Offset + (*IortNodeLength)) > AcpiTableLength) {
> + // Validate IORT Node length
> + if ((*IortNodeLength == 0) ||
> + ((Offset + (*IortNodeLength)) > AcpiTableLength)) {
> IncrementErrorCount ();
> Print (
> - L"ERROR: Invalid IORT node length. IortNodeLength = %d. " \
> - L"RemainingTableBufferLength = %d. IORT parsing aborted.\n",
> + L"ERROR: Invalid IORT Node length. " \
> + L"Length = %d. Offset = %d. AcpiTableLength = %d.\n",
> *IortNodeLength,
> - AcpiTableLength - Offset
> + Offset,
> + AcpiTableLength
> );
> return;
> }
> diff --git
> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser
> .c
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser
> .c index
> 438905cb24f58b8b82e8fe61280e72f765d578d8..f85d2b36532cfc5db36fe7bef983
> 0cccc64969cc 100644
> ---
> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser
> .c
> +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtPa
> +++ rser.c
> @@ -1,7 +1,7 @@
> /** @file
> MADT 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
>
> @par Reference(s):
> @@ -273,28 +273,16 @@ ParseAcpiMadt (
> return;
> }
>
> - // Make sure forward progress is made.
> - if (*MadtInterruptControllerLength < 2) {
> + // Validate Interrupt Controller Structure length
> + if ((*MadtInterruptControllerLength == 0) ||
> + ((Offset + (*MadtInterruptControllerLength)) >
> + AcpiTableLength)) {
> IncrementErrorCount ();
> Print (
> - L"ERROR: Structure length is too small: " \
> - L"MadtInterruptControllerLength = %d. " \
> - L"MadtInterruptControllerType = %d. MADT parsing aborted.\n",
> + L"ERROR: Invalid Interrupt Controller Structure length. " \
> + L"Length = %d. Offset = %d. AcpiTableLength = %d.\n",
> *MadtInterruptControllerLength,
> - *MadtInterruptControllerType
> - );
> - return;
> - }
> -
> - // Make sure the MADT structure lies inside the table
> - if ((Offset + *MadtInterruptControllerLength) > AcpiTableLength) {
> - IncrementErrorCount ();
> - Print (
> - L"ERROR: Invalid MADT structure length. " \
> - L"MadtInterruptControllerLength = %d. " \
> - L"RemainingTableBufferLength = %d. MADT parsing aborted.\n",
> - *MadtInterruptControllerLength,
> - AcpiTableLength - Offset
> + Offset,
> + AcpiTableLength
> );
> return;
> }
> diff --git
> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser
> .c
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser
> .c index
> 675ba75f02b367cd5ad9f2ac23c30ed0ab58f286..0db272c16af0ad8824c8da4c88dd
> 409c8550112a 100644
> ---
> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser
> .c
> +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttPa
> +++ rser.c
> @@ -1,7 +1,7 @@
> /** @file
> PPTT table parser
>
> - Copyright (c) 2019, ARM Limited. All rights reserved.
> + Copyright (c) 2019 - 2020, ARM Limited. All rights reserved.
> SPDX-License-Identifier: BSD-2-Clause-Patent
>
> @par Reference(s):
> @@ -425,15 +425,16 @@ ParseAcpiPptt (
> return;
> }
>
> - // Make sure the PPTT structure lies inside the table
> - if ((Offset + *ProcessorTopologyStructureLength) > AcpiTableLength) {
> + // Validate Processor Topology Structure length
> + if ((*ProcessorTopologyStructureLength == 0) ||
> + ((Offset + (*ProcessorTopologyStructureLength)) >
> + AcpiTableLength)) {
> IncrementErrorCount ();
> Print (
> - L"ERROR: Invalid PPTT structure length. " \
> - L"ProcessorTopologyStructureLength = %d. " \
> - L"RemainingTableBufferLength = %d. PPTT parsing aborted.\n",
> + L"ERROR: Invalid Processor Topology Structure length. " \
> + L"Length = %d. Offset = %d. AcpiTableLength = %d.\n",
> *ProcessorTopologyStructureLength,
> - AcpiTableLength - Offset
> + Offset,
> + AcpiTableLength
> );
> return;
> }
> diff --git
> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser
> .c
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser
> .c index
> 3613900ae322483fdd3d3383de4e22ba75b2128b..6f66be68cc0bed14811a0432c61a
> 79fd47c54890 100644
> ---
> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser
> .c
> +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratPa
> +++ rser.c
> @@ -1,7 +1,7 @@
> /** @file
> SRAT 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
>
> @par Reference(s):
> @@ -412,14 +412,16 @@ ParseAcpiSrat (
> return;
> }
>
> - // Make sure the SRAT structure lies inside the table
> - if ((Offset + *SratRALength) > AcpiTableLength) {
> + // Validate Static Resource Allocation Structure length
> + if ((*SratRALength == 0) ||
> + ((Offset + (*SratRALength)) > AcpiTableLength)) {
> IncrementErrorCount ();
> Print (
> - L"ERROR: Invalid SRAT structure length. SratRALength = %d. " \
> - L"RemainingTableBufferLength = %d. SRAT parsing aborted.\n",
> + L"ERROR: Invalid Static Resource Allocation Structure length. " \
> + L"Length = %d. Offset = %d. AcpiTableLength = %d.\n",
> *SratRALength,
> - AcpiTableLength - Offset
> + Offset,
> + AcpiTableLength
> );
> return;
> }
> --
> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
>
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [edk2-devel] [PATCH v1 1/1] ShellPkg: acpiview: Prevent infinite loop if structure length is 0
2020-02-17 15:22 ` Krzysztof Koch
@ 2020-02-17 15:24 ` Liming Gao
0 siblings, 0 replies; 7+ messages in thread
From: Liming Gao @ 2020-02-17 15:24 UTC (permalink / raw)
To: devel@edk2.groups.io, krzysztof.koch@arm.com
Cc: Ni, Ray, Gao, Zhichao, Sami Mujawar, Matteo Carlini, nd
Krzysztof:
Yes. Please create one BZ for this issue.
Thanks
Liming
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Krzysztof Koch
> Sent: Monday, February 17, 2020 11:23 PM
> To: devel@edk2.groups.io; Gao, Liming <liming.gao@intel.com>
> Cc: Ni, Ray <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>; Sami Mujawar <Sami.Mujawar@arm.com>; Matteo Carlini
> <Matteo.Carlini@arm.com>; nd <nd@arm.com>
> Subject: Re: [edk2-devel] [PATCH v1 1/1] ShellPkg: acpiview: Prevent infinite loop if structure length is 0
>
> Hi Liming,
>
> I haven't created a BZ yet, shall I create one? It would be great if the patch makes it to the stable tag.
>
> Over the last few months I added some security features to acpiview. They make this debug tool less sensitive to exploits from ACPI
> tables. This patch completes my efforts in making the tool more reliable.
>
> Kind regards,
> Krzysztof
>
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Liming Gao via Groups.Io
> Sent: Monday, February 17, 2020 15:11
> To: devel@edk2.groups.io; Krzysztof Koch <Krzysztof.Koch@arm.com>
> Cc: Ni, Ray <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>; Sami Mujawar <Sami.Mujawar@arm.com>; Matteo Carlini
> <Matteo.Carlini@arm.com>; nd <nd@arm.com>
> Subject: Re: [edk2-devel] [PATCH v1 1/1] ShellPkg: acpiview: Prevent infinite loop if structure length is 0
>
> Krzysztof:
> Is there one BZ for this issue? Does this patch catch to this edk2 stable tag 202002?
>
> Thanks
> Liming
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> > Krzysztof Koch
> > Sent: Friday, February 14, 2020 9:59 PM
> > To: devel@edk2.groups.io
> > Cc: Ni, Ray <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>;
> > Sami.Mujawar@arm.com; Matteo.Carlini@arm.com; nd@arm.com
> > Subject: [edk2-devel] [PATCH v1 1/1] ShellPkg: acpiview: Prevent
> > infinite loop if structure length is 0
> >
> > Extend validation of ACPI structure lengths which are read from the
> > ACPI table being parsed. Additionally check if the structure 'Length'
> > field value is positive. If not, stop parsing the faulting table.
> >
> > Some ACPI tables define internal structures of variable size. The
> > 'Length' field inside the substructure is used to update a pointer
> > used for table traversal. If the byte-length of the structure is equal
> > to 0, acpiview can enter an infinite loop. This condition can occur
> > if, for example, the zero-allocated ACPI table buffer is not fully populated.
> > This is typically a bug on the ACPI table writer side.
> >
> > In short, this method helps acpiview recover gracefully from a
> > zero-valued ACPI structure length.
> >
> > Signed-off-by: Krzysztof Koch <krzysztof.koch@arm.com>
> > ---
> >
> > Changes can be seen at:
> > https://github.com/KrzysztofKoch1/edk2/tree/612_acpiview_prevent_inf_l
> > oops_v1
> >
> > Notes:
> > v1:
> > - prevent infinite loops in acpiview parsers [Krzysztof]
> >
> >
> > ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c
> > | 15 ++++++-----
> > ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser.c
> > | 13 ++++-----
> > ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser.c
> > | 14 +++++-----
> > ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.c
> > | 28 ++++++--------------
> > ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c
> > | 15 ++++++-----
> > ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser.c
> > | 14 +++++-----
> > 6 files changed, 47 insertions(+), 52 deletions(-)
> >
> > diff --git
> > a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser
> > .c
> > b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser
> > .c index
> > 0f730a306a94329a23fbaf54b59f1833b44616ba..9df111ecaa7d7a703a13a39c243e
> > d78b9f12ee97 100644
> > ---
> > a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser
> > .c
> > +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Pa
> > +++ rser.c
> > @@ -1,7 +1,7 @@
> > /** @file
> > DBG2 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
> >
> > @par Reference(s):
> > @@ -282,15 +282,16 @@ ParseAcpiDbg2 (
> > return;
> > }
> >
> > - // Make sure the Debug Device Information structure lies inside the table.
> > - if ((Offset + *DbgDevInfoLen) > AcpiTableLength) {
> > + // Validate Debug Device Information Structure length
> > + if ((*DbgDevInfoLen == 0) ||
> > + ((Offset + (*DbgDevInfoLen)) > AcpiTableLength)) {
> > IncrementErrorCount ();
> > Print (
> > - L"ERROR: Invalid Debug Device Information structure length. " \
> > - L"DbgDevInfoLen = %d. RemainingTableBufferLength = %d. " \
> > - L"DBG2 parsing aborted.\n",
> > + L"ERROR: Invalid Debug Device Information Structure length. " \
> > + L"Length = %d. Offset = %d. AcpiTableLength = %d.\n",
> > *DbgDevInfoLen,
> > - AcpiTableLength - Offset
> > + Offset,
> > + AcpiTableLength
> > );
> > return;
> > }
> > diff --git
> > a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser
> > .c
> > b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser
> > .c index
> > 699a55b549ec3fa61bbd156898821055dc019199..bdd30ff45c61142c071ead63a27b
> > abab8998721b 100644
> > ---
> > a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser
> > .c
> > +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtPa
> > +++ rser.c
> > @@ -1,7 +1,7 @@
> > /** @file
> > GTDT 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
> >
> > @par Reference(s):
> > @@ -327,15 +327,16 @@ ParseAcpiGtdt (
> > return;
> > }
> >
> > - // Make sure the Platform Timer is inside the table.
> > - if ((Offset + *PlatformTimerLength) > AcpiTableLength) {
> > + // Validate Platform Timer Structure length
> > + if ((*PlatformTimerLength == 0) ||
> > + ((Offset + (*PlatformTimerLength)) > AcpiTableLength)) {
> > IncrementErrorCount ();
> > Print (
> > L"ERROR: Invalid Platform Timer Structure length. " \
> > - L"PlatformTimerLength = %d. RemainingTableBufferLength = %d. " \
> > - L"GTDT parsing aborted.\n",
> > + L"Length = %d. Offset = %d. AcpiTableLength = %d.\n",
> > *PlatformTimerLength,
> > - AcpiTableLength - Offset
> > + Offset,
> > + AcpiTableLength
> > );
> > return;
> > }
> > diff --git
> > a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser
> > .c
> > b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser
> > .c index
> > 9d5d937c7b2c19945ca2ad3eba644bdfc09cc3f6..9a006a01448b897865cd7cd85651
> > c816933acf05 100644
> > ---
> > a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser
> > .c
> > +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortPa
> > +++ rser.c
> > @@ -1,7 +1,7 @@
> > /** @file
> > IORT 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
> >
> > @par Reference(s):
> > @@ -687,14 +687,16 @@ ParseAcpiIort (
> > return;
> > }
> >
> > - // Make sure the IORT Node is inside the table
> > - if ((Offset + (*IortNodeLength)) > AcpiTableLength) {
> > + // Validate IORT Node length
> > + if ((*IortNodeLength == 0) ||
> > + ((Offset + (*IortNodeLength)) > AcpiTableLength)) {
> > IncrementErrorCount ();
> > Print (
> > - L"ERROR: Invalid IORT node length. IortNodeLength = %d. " \
> > - L"RemainingTableBufferLength = %d. IORT parsing aborted.\n",
> > + L"ERROR: Invalid IORT Node length. " \
> > + L"Length = %d. Offset = %d. AcpiTableLength = %d.\n",
> > *IortNodeLength,
> > - AcpiTableLength - Offset
> > + Offset,
> > + AcpiTableLength
> > );
> > return;
> > }
> > diff --git
> > a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser
> > .c
> > b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser
> > .c index
> > 438905cb24f58b8b82e8fe61280e72f765d578d8..f85d2b36532cfc5db36fe7bef983
> > 0cccc64969cc 100644
> > ---
> > a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser
> > .c
> > +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtPa
> > +++ rser.c
> > @@ -1,7 +1,7 @@
> > /** @file
> > MADT 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
> >
> > @par Reference(s):
> > @@ -273,28 +273,16 @@ ParseAcpiMadt (
> > return;
> > }
> >
> > - // Make sure forward progress is made.
> > - if (*MadtInterruptControllerLength < 2) {
> > + // Validate Interrupt Controller Structure length
> > + if ((*MadtInterruptControllerLength == 0) ||
> > + ((Offset + (*MadtInterruptControllerLength)) >
> > + AcpiTableLength)) {
> > IncrementErrorCount ();
> > Print (
> > - L"ERROR: Structure length is too small: " \
> > - L"MadtInterruptControllerLength = %d. " \
> > - L"MadtInterruptControllerType = %d. MADT parsing aborted.\n",
> > + L"ERROR: Invalid Interrupt Controller Structure length. " \
> > + L"Length = %d. Offset = %d. AcpiTableLength = %d.\n",
> > *MadtInterruptControllerLength,
> > - *MadtInterruptControllerType
> > - );
> > - return;
> > - }
> > -
> > - // Make sure the MADT structure lies inside the table
> > - if ((Offset + *MadtInterruptControllerLength) > AcpiTableLength) {
> > - IncrementErrorCount ();
> > - Print (
> > - L"ERROR: Invalid MADT structure length. " \
> > - L"MadtInterruptControllerLength = %d. " \
> > - L"RemainingTableBufferLength = %d. MADT parsing aborted.\n",
> > - *MadtInterruptControllerLength,
> > - AcpiTableLength - Offset
> > + Offset,
> > + AcpiTableLength
> > );
> > return;
> > }
> > diff --git
> > a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser
> > .c
> > b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser
> > .c index
> > 675ba75f02b367cd5ad9f2ac23c30ed0ab58f286..0db272c16af0ad8824c8da4c88dd
> > 409c8550112a 100644
> > ---
> > a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser
> > .c
> > +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttPa
> > +++ rser.c
> > @@ -1,7 +1,7 @@
> > /** @file
> > PPTT table parser
> >
> > - Copyright (c) 2019, ARM Limited. All rights reserved.
> > + Copyright (c) 2019 - 2020, ARM Limited. All rights reserved.
> > SPDX-License-Identifier: BSD-2-Clause-Patent
> >
> > @par Reference(s):
> > @@ -425,15 +425,16 @@ ParseAcpiPptt (
> > return;
> > }
> >
> > - // Make sure the PPTT structure lies inside the table
> > - if ((Offset + *ProcessorTopologyStructureLength) > AcpiTableLength) {
> > + // Validate Processor Topology Structure length
> > + if ((*ProcessorTopologyStructureLength == 0) ||
> > + ((Offset + (*ProcessorTopologyStructureLength)) >
> > + AcpiTableLength)) {
> > IncrementErrorCount ();
> > Print (
> > - L"ERROR: Invalid PPTT structure length. " \
> > - L"ProcessorTopologyStructureLength = %d. " \
> > - L"RemainingTableBufferLength = %d. PPTT parsing aborted.\n",
> > + L"ERROR: Invalid Processor Topology Structure length. " \
> > + L"Length = %d. Offset = %d. AcpiTableLength = %d.\n",
> > *ProcessorTopologyStructureLength,
> > - AcpiTableLength - Offset
> > + Offset,
> > + AcpiTableLength
> > );
> > return;
> > }
> > diff --git
> > a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser
> > .c
> > b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser
> > .c index
> > 3613900ae322483fdd3d3383de4e22ba75b2128b..6f66be68cc0bed14811a0432c61a
> > 79fd47c54890 100644
> > ---
> > a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser
> > .c
> > +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratPa
> > +++ rser.c
> > @@ -1,7 +1,7 @@
> > /** @file
> > SRAT 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
> >
> > @par Reference(s):
> > @@ -412,14 +412,16 @@ ParseAcpiSrat (
> > return;
> > }
> >
> > - // Make sure the SRAT structure lies inside the table
> > - if ((Offset + *SratRALength) > AcpiTableLength) {
> > + // Validate Static Resource Allocation Structure length
> > + if ((*SratRALength == 0) ||
> > + ((Offset + (*SratRALength)) > AcpiTableLength)) {
> > IncrementErrorCount ();
> > Print (
> > - L"ERROR: Invalid SRAT structure length. SratRALength = %d. " \
> > - L"RemainingTableBufferLength = %d. SRAT parsing aborted.\n",
> > + L"ERROR: Invalid Static Resource Allocation Structure length. " \
> > + L"Length = %d. Offset = %d. AcpiTableLength = %d.\n",
> > *SratRALength,
> > - AcpiTableLength - Offset
> > + Offset,
> > + AcpiTableLength
> > );
> > return;
> > }
> > --
> > 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
> >
> >
> >
>
>
>
>
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v1 1/1] ShellPkg: acpiview: Prevent infinite loop if structure length is 0
2020-02-14 13:59 [PATCH v1 1/1] ShellPkg: acpiview: Prevent infinite loop if structure length is 0 Krzysztof Koch
2020-02-17 15:11 ` [edk2-devel] " Liming Gao
@ 2020-02-17 15:35 ` Sami Mujawar
1 sibling, 0 replies; 7+ messages in thread
From: Sami Mujawar @ 2020-02-17 15:35 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: 14 February 2020 13:59
To: devel@edk2.groups.io
Cc: ray.ni@intel.com; zhichao.gao@intel.com; Sami Mujawar <Sami.Mujawar@arm.com>; Matteo Carlini <Matteo.Carlini@arm.com>; nd <nd@arm.com>
Subject: [PATCH v1 1/1] ShellPkg: acpiview: Prevent infinite loop if structure length is 0
Extend validation of ACPI structure lengths which are read from the ACPI table being parsed. Additionally check if the structure 'Length'
field value is positive. If not, stop parsing the faulting table.
Some ACPI tables define internal structures of variable size. The 'Length' field inside the substructure is used to update a pointer used for table traversal. If the byte-length of the structure is equal to 0, acpiview can enter an infinite loop. This condition can occur if, for example, the zero-allocated ACPI table buffer is not fully populated.
This is typically a bug on the ACPI table writer side.
In short, this method helps acpiview recover gracefully from a zero-valued ACPI structure length.
Signed-off-by: Krzysztof Koch <krzysztof.koch@arm.com>
---
Changes can be seen at: https://github.com/KrzysztofKoch1/edk2/tree/612_acpiview_prevent_inf_loops_v1
Notes:
v1:
- prevent infinite loops in acpiview parsers [Krzysztof]
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c | 15 ++++++----- ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser.c | 13 ++++----- ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser.c | 14 +++++----- ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.c | 28 ++++++-------------- ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c | 15 ++++++----- ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser.c | 14 +++++-----
6 files changed, 47 insertions(+), 52 deletions(-)
diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c
index 0f730a306a94329a23fbaf54b59f1833b44616ba..9df111ecaa7d7a703a13a39c243ed78b9f12ee97 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Pars
+++ er.c
@@ -1,7 +1,7 @@
/** @file
DBG2 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
@par Reference(s):
@@ -282,15 +282,16 @@ ParseAcpiDbg2 (
return;
}
- // Make sure the Debug Device Information structure lies inside the table.
- if ((Offset + *DbgDevInfoLen) > AcpiTableLength) {
+ // Validate Debug Device Information Structure length
+ if ((*DbgDevInfoLen == 0) ||
+ ((Offset + (*DbgDevInfoLen)) > AcpiTableLength)) {
IncrementErrorCount ();
Print (
- L"ERROR: Invalid Debug Device Information structure length. " \
- L"DbgDevInfoLen = %d. RemainingTableBufferLength = %d. " \
- L"DBG2 parsing aborted.\n",
+ L"ERROR: Invalid Debug Device Information Structure length. " \
+ L"Length = %d. Offset = %d. AcpiTableLength = %d.\n",
*DbgDevInfoLen,
- AcpiTableLength - Offset
+ Offset,
+ AcpiTableLength
);
return;
}
diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser.c
index 699a55b549ec3fa61bbd156898821055dc019199..bdd30ff45c61142c071ead63a27babab8998721b 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtPars
+++ er.c
@@ -1,7 +1,7 @@
/** @file
GTDT 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
@par Reference(s):
@@ -327,15 +327,16 @@ ParseAcpiGtdt (
return;
}
- // Make sure the Platform Timer is inside the table.
- if ((Offset + *PlatformTimerLength) > AcpiTableLength) {
+ // Validate Platform Timer Structure length
+ if ((*PlatformTimerLength == 0) ||
+ ((Offset + (*PlatformTimerLength)) > AcpiTableLength)) {
IncrementErrorCount ();
Print (
L"ERROR: Invalid Platform Timer Structure length. " \
- L"PlatformTimerLength = %d. RemainingTableBufferLength = %d. " \
- L"GTDT parsing aborted.\n",
+ L"Length = %d. Offset = %d. AcpiTableLength = %d.\n",
*PlatformTimerLength,
- AcpiTableLength - Offset
+ Offset,
+ AcpiTableLength
);
return;
}
diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser.c
index 9d5d937c7b2c19945ca2ad3eba644bdfc09cc3f6..9a006a01448b897865cd7cd85651c816933acf05 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortPars
+++ er.c
@@ -1,7 +1,7 @@
/** @file
IORT 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
@par Reference(s):
@@ -687,14 +687,16 @@ ParseAcpiIort (
return;
}
- // Make sure the IORT Node is inside the table
- if ((Offset + (*IortNodeLength)) > AcpiTableLength) {
+ // Validate IORT Node length
+ if ((*IortNodeLength == 0) ||
+ ((Offset + (*IortNodeLength)) > AcpiTableLength)) {
IncrementErrorCount ();
Print (
- L"ERROR: Invalid IORT node length. IortNodeLength = %d. " \
- L"RemainingTableBufferLength = %d. IORT parsing aborted.\n",
+ L"ERROR: Invalid IORT Node length. " \
+ L"Length = %d. Offset = %d. AcpiTableLength = %d.\n",
*IortNodeLength,
- AcpiTableLength - Offset
+ Offset,
+ AcpiTableLength
);
return;
}
diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.c
index 438905cb24f58b8b82e8fe61280e72f765d578d8..f85d2b36532cfc5db36fe7bef9830cccc64969cc 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtPars
+++ er.c
@@ -1,7 +1,7 @@
/** @file
MADT 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
@par Reference(s):
@@ -273,28 +273,16 @@ ParseAcpiMadt (
return;
}
- // Make sure forward progress is made.
- if (*MadtInterruptControllerLength < 2) {
+ // Validate Interrupt Controller Structure length
+ if ((*MadtInterruptControllerLength == 0) ||
+ ((Offset + (*MadtInterruptControllerLength)) >
+ AcpiTableLength)) {
IncrementErrorCount ();
Print (
- L"ERROR: Structure length is too small: " \
- L"MadtInterruptControllerLength = %d. " \
- L"MadtInterruptControllerType = %d. MADT parsing aborted.\n",
+ L"ERROR: Invalid Interrupt Controller Structure length. " \
+ L"Length = %d. Offset = %d. AcpiTableLength = %d.\n",
*MadtInterruptControllerLength,
- *MadtInterruptControllerType
- );
- return;
- }
-
- // Make sure the MADT structure lies inside the table
- if ((Offset + *MadtInterruptControllerLength) > AcpiTableLength) {
- IncrementErrorCount ();
- Print (
- L"ERROR: Invalid MADT structure length. " \
- L"MadtInterruptControllerLength = %d. " \
- L"RemainingTableBufferLength = %d. MADT parsing aborted.\n",
- *MadtInterruptControllerLength,
- AcpiTableLength - Offset
+ Offset,
+ AcpiTableLength
);
return;
}
diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c
index 675ba75f02b367cd5ad9f2ac23c30ed0ab58f286..0db272c16af0ad8824c8da4c88dd409c8550112a 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttPars
+++ er.c
@@ -1,7 +1,7 @@
/** @file
PPTT table parser
- Copyright (c) 2019, ARM Limited. All rights reserved.
+ Copyright (c) 2019 - 2020, ARM Limited. All rights reserved.
SPDX-License-Identifier: BSD-2-Clause-Patent
@par Reference(s):
@@ -425,15 +425,16 @@ ParseAcpiPptt (
return;
}
- // Make sure the PPTT structure lies inside the table
- if ((Offset + *ProcessorTopologyStructureLength) > AcpiTableLength) {
+ // Validate Processor Topology Structure length
+ if ((*ProcessorTopologyStructureLength == 0) ||
+ ((Offset + (*ProcessorTopologyStructureLength)) >
+ AcpiTableLength)) {
IncrementErrorCount ();
Print (
- L"ERROR: Invalid PPTT structure length. " \
- L"ProcessorTopologyStructureLength = %d. " \
- L"RemainingTableBufferLength = %d. PPTT parsing aborted.\n",
+ L"ERROR: Invalid Processor Topology Structure length. " \
+ L"Length = %d. Offset = %d. AcpiTableLength = %d.\n",
*ProcessorTopologyStructureLength,
- AcpiTableLength - Offset
+ Offset,
+ AcpiTableLength
);
return;
}
diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser.c
index 3613900ae322483fdd3d3383de4e22ba75b2128b..6f66be68cc0bed14811a0432c61a79fd47c54890 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratPars
+++ er.c
@@ -1,7 +1,7 @@
/** @file
SRAT 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
@par Reference(s):
@@ -412,14 +412,16 @@ ParseAcpiSrat (
return;
}
- // Make sure the SRAT structure lies inside the table
- if ((Offset + *SratRALength) > AcpiTableLength) {
+ // Validate Static Resource Allocation Structure length
+ if ((*SratRALength == 0) ||
+ ((Offset + (*SratRALength)) > AcpiTableLength)) {
IncrementErrorCount ();
Print (
- L"ERROR: Invalid SRAT structure length. SratRALength = %d. " \
- L"RemainingTableBufferLength = %d. SRAT parsing aborted.\n",
+ L"ERROR: Invalid Static Resource Allocation Structure length. " \
+ L"Length = %d. Offset = %d. AcpiTableLength = %d.\n",
*SratRALength,
- AcpiTableLength - Offset
+ Offset,
+ AcpiTableLength
);
return;
}
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [edk2-devel] [PATCH v1 1/1] ShellPkg: acpiview: Prevent infinite loop if structure length is 0
[not found] ` <15F439D65D9DE013.5373@groups.io>
@ 2020-02-17 15:39 ` Krzysztof Koch
2020-02-19 1:44 ` Gao, Zhichao
0 siblings, 1 reply; 7+ messages in thread
From: Krzysztof Koch @ 2020-02-17 15:39 UTC (permalink / raw)
To: devel@edk2.groups.io, Krzysztof Koch, liming.gao@intel.com
Cc: Ni, Ray, Gao, Zhichao, Sami Mujawar, Matteo Carlini, nd
Hi Liming,
The BZ is: https://bugzilla.tianocore.org/show_bug.cgi?id=2534
Please let me know if I should change something.
Kind regards,
Krzysztof
-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Krzysztof Koch via Groups.Io
Sent: Monday, February 17, 2020 15:23
To: devel@edk2.groups.io; liming.gao@intel.com
Cc: Ni, Ray <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>; Sami Mujawar <Sami.Mujawar@arm.com>; Matteo Carlini <Matteo.Carlini@arm.com>; nd <nd@arm.com>
Subject: Re: [edk2-devel] [PATCH v1 1/1] ShellPkg: acpiview: Prevent infinite loop if structure length is 0
Hi Liming,
I haven't created a BZ yet, shall I create one? It would be great if the patch makes it to the stable tag.
Over the last few months I added some security features to acpiview. They make this debug tool less sensitive to exploits from ACPI tables. This patch completes my efforts in making the tool more reliable.
Kind regards,
Krzysztof
-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Liming Gao via Groups.Io
Sent: Monday, February 17, 2020 15:11
To: devel@edk2.groups.io; Krzysztof Koch <Krzysztof.Koch@arm.com>
Cc: Ni, Ray <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>; Sami Mujawar <Sami.Mujawar@arm.com>; Matteo Carlini <Matteo.Carlini@arm.com>; nd <nd@arm.com>
Subject: Re: [edk2-devel] [PATCH v1 1/1] ShellPkg: acpiview: Prevent infinite loop if structure length is 0
Krzysztof:
Is there one BZ for this issue? Does this patch catch to this edk2 stable tag 202002?
Thanks
Liming
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> Krzysztof Koch
> Sent: Friday, February 14, 2020 9:59 PM
> To: devel@edk2.groups.io
> Cc: Ni, Ray <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>;
> Sami.Mujawar@arm.com; Matteo.Carlini@arm.com; nd@arm.com
> Subject: [edk2-devel] [PATCH v1 1/1] ShellPkg: acpiview: Prevent
> infinite loop if structure length is 0
>
> Extend validation of ACPI structure lengths which are read from the
> ACPI table being parsed. Additionally check if the structure 'Length'
> field value is positive. If not, stop parsing the faulting table.
>
> Some ACPI tables define internal structures of variable size. The
> 'Length' field inside the substructure is used to update a pointer
> used for table traversal. If the byte-length of the structure is equal
> to 0, acpiview can enter an infinite loop. This condition can occur
> if, for example, the zero-allocated ACPI table buffer is not fully populated.
> This is typically a bug on the ACPI table writer side.
>
> In short, this method helps acpiview recover gracefully from a
> zero-valued ACPI structure length.
>
> Signed-off-by: Krzysztof Koch <krzysztof.koch@arm.com>
> ---
>
> Changes can be seen at:
> https://github.com/KrzysztofKoch1/edk2/tree/612_acpiview_prevent_inf_l
> oops_v1
>
> Notes:
> v1:
> - prevent infinite loops in acpiview parsers [Krzysztof]
>
>
> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c
> | 15 ++++++-----
> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser.c
> | 13 ++++-----
> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser.c
> | 14 +++++-----
> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.c
> | 28 ++++++--------------
> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c
> | 15 ++++++-----
> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser.c
> | 14 +++++-----
> 6 files changed, 47 insertions(+), 52 deletions(-)
>
> diff --git
> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser
> .c
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser
> .c index
> 0f730a306a94329a23fbaf54b59f1833b44616ba..9df111ecaa7d7a703a13a39c243e
> d78b9f12ee97 100644
> ---
> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser
> .c
> +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Pa
> +++ rser.c
> @@ -1,7 +1,7 @@
> /** @file
> DBG2 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
>
> @par Reference(s):
> @@ -282,15 +282,16 @@ ParseAcpiDbg2 (
> return;
> }
>
> - // Make sure the Debug Device Information structure lies inside the table.
> - if ((Offset + *DbgDevInfoLen) > AcpiTableLength) {
> + // Validate Debug Device Information Structure length
> + if ((*DbgDevInfoLen == 0) ||
> + ((Offset + (*DbgDevInfoLen)) > AcpiTableLength)) {
> IncrementErrorCount ();
> Print (
> - L"ERROR: Invalid Debug Device Information structure length. " \
> - L"DbgDevInfoLen = %d. RemainingTableBufferLength = %d. " \
> - L"DBG2 parsing aborted.\n",
> + L"ERROR: Invalid Debug Device Information Structure length. " \
> + L"Length = %d. Offset = %d. AcpiTableLength = %d.\n",
> *DbgDevInfoLen,
> - AcpiTableLength - Offset
> + Offset,
> + AcpiTableLength
> );
> return;
> }
> diff --git
> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser
> .c
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser
> .c index
> 699a55b549ec3fa61bbd156898821055dc019199..bdd30ff45c61142c071ead63a27b
> abab8998721b 100644
> ---
> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser
> .c
> +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtPa
> +++ rser.c
> @@ -1,7 +1,7 @@
> /** @file
> GTDT 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
>
> @par Reference(s):
> @@ -327,15 +327,16 @@ ParseAcpiGtdt (
> return;
> }
>
> - // Make sure the Platform Timer is inside the table.
> - if ((Offset + *PlatformTimerLength) > AcpiTableLength) {
> + // Validate Platform Timer Structure length
> + if ((*PlatformTimerLength == 0) ||
> + ((Offset + (*PlatformTimerLength)) > AcpiTableLength)) {
> IncrementErrorCount ();
> Print (
> L"ERROR: Invalid Platform Timer Structure length. " \
> - L"PlatformTimerLength = %d. RemainingTableBufferLength = %d. " \
> - L"GTDT parsing aborted.\n",
> + L"Length = %d. Offset = %d. AcpiTableLength = %d.\n",
> *PlatformTimerLength,
> - AcpiTableLength - Offset
> + Offset,
> + AcpiTableLength
> );
> return;
> }
> diff --git
> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser
> .c
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser
> .c index
> 9d5d937c7b2c19945ca2ad3eba644bdfc09cc3f6..9a006a01448b897865cd7cd85651
> c816933acf05 100644
> ---
> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser
> .c
> +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortPa
> +++ rser.c
> @@ -1,7 +1,7 @@
> /** @file
> IORT 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
>
> @par Reference(s):
> @@ -687,14 +687,16 @@ ParseAcpiIort (
> return;
> }
>
> - // Make sure the IORT Node is inside the table
> - if ((Offset + (*IortNodeLength)) > AcpiTableLength) {
> + // Validate IORT Node length
> + if ((*IortNodeLength == 0) ||
> + ((Offset + (*IortNodeLength)) > AcpiTableLength)) {
> IncrementErrorCount ();
> Print (
> - L"ERROR: Invalid IORT node length. IortNodeLength = %d. " \
> - L"RemainingTableBufferLength = %d. IORT parsing aborted.\n",
> + L"ERROR: Invalid IORT Node length. " \
> + L"Length = %d. Offset = %d. AcpiTableLength = %d.\n",
> *IortNodeLength,
> - AcpiTableLength - Offset
> + Offset,
> + AcpiTableLength
> );
> return;
> }
> diff --git
> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser
> .c
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser
> .c index
> 438905cb24f58b8b82e8fe61280e72f765d578d8..f85d2b36532cfc5db36fe7bef983
> 0cccc64969cc 100644
> ---
> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser
> .c
> +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtPa
> +++ rser.c
> @@ -1,7 +1,7 @@
> /** @file
> MADT 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
>
> @par Reference(s):
> @@ -273,28 +273,16 @@ ParseAcpiMadt (
> return;
> }
>
> - // Make sure forward progress is made.
> - if (*MadtInterruptControllerLength < 2) {
> + // Validate Interrupt Controller Structure length
> + if ((*MadtInterruptControllerLength == 0) ||
> + ((Offset + (*MadtInterruptControllerLength)) >
> + AcpiTableLength)) {
> IncrementErrorCount ();
> Print (
> - L"ERROR: Structure length is too small: " \
> - L"MadtInterruptControllerLength = %d. " \
> - L"MadtInterruptControllerType = %d. MADT parsing aborted.\n",
> + L"ERROR: Invalid Interrupt Controller Structure length. " \
> + L"Length = %d. Offset = %d. AcpiTableLength = %d.\n",
> *MadtInterruptControllerLength,
> - *MadtInterruptControllerType
> - );
> - return;
> - }
> -
> - // Make sure the MADT structure lies inside the table
> - if ((Offset + *MadtInterruptControllerLength) > AcpiTableLength) {
> - IncrementErrorCount ();
> - Print (
> - L"ERROR: Invalid MADT structure length. " \
> - L"MadtInterruptControllerLength = %d. " \
> - L"RemainingTableBufferLength = %d. MADT parsing aborted.\n",
> - *MadtInterruptControllerLength,
> - AcpiTableLength - Offset
> + Offset,
> + AcpiTableLength
> );
> return;
> }
> diff --git
> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser
> .c
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser
> .c index
> 675ba75f02b367cd5ad9f2ac23c30ed0ab58f286..0db272c16af0ad8824c8da4c88dd
> 409c8550112a 100644
> ---
> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser
> .c
> +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttPa
> +++ rser.c
> @@ -1,7 +1,7 @@
> /** @file
> PPTT table parser
>
> - Copyright (c) 2019, ARM Limited. All rights reserved.
> + Copyright (c) 2019 - 2020, ARM Limited. All rights reserved.
> SPDX-License-Identifier: BSD-2-Clause-Patent
>
> @par Reference(s):
> @@ -425,15 +425,16 @@ ParseAcpiPptt (
> return;
> }
>
> - // Make sure the PPTT structure lies inside the table
> - if ((Offset + *ProcessorTopologyStructureLength) > AcpiTableLength) {
> + // Validate Processor Topology Structure length
> + if ((*ProcessorTopologyStructureLength == 0) ||
> + ((Offset + (*ProcessorTopologyStructureLength)) >
> + AcpiTableLength)) {
> IncrementErrorCount ();
> Print (
> - L"ERROR: Invalid PPTT structure length. " \
> - L"ProcessorTopologyStructureLength = %d. " \
> - L"RemainingTableBufferLength = %d. PPTT parsing aborted.\n",
> + L"ERROR: Invalid Processor Topology Structure length. " \
> + L"Length = %d. Offset = %d. AcpiTableLength = %d.\n",
> *ProcessorTopologyStructureLength,
> - AcpiTableLength - Offset
> + Offset,
> + AcpiTableLength
> );
> return;
> }
> diff --git
> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser
> .c
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser
> .c index
> 3613900ae322483fdd3d3383de4e22ba75b2128b..6f66be68cc0bed14811a0432c61a
> 79fd47c54890 100644
> ---
> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser
> .c
> +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratPa
> +++ rser.c
> @@ -1,7 +1,7 @@
> /** @file
> SRAT 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
>
> @par Reference(s):
> @@ -412,14 +412,16 @@ ParseAcpiSrat (
> return;
> }
>
> - // Make sure the SRAT structure lies inside the table
> - if ((Offset + *SratRALength) > AcpiTableLength) {
> + // Validate Static Resource Allocation Structure length
> + if ((*SratRALength == 0) ||
> + ((Offset + (*SratRALength)) > AcpiTableLength)) {
> IncrementErrorCount ();
> Print (
> - L"ERROR: Invalid SRAT structure length. SratRALength = %d. " \
> - L"RemainingTableBufferLength = %d. SRAT parsing aborted.\n",
> + L"ERROR: Invalid Static Resource Allocation Structure length. " \
> + L"Length = %d. Offset = %d. AcpiTableLength = %d.\n",
> *SratRALength,
> - AcpiTableLength - Offset
> + Offset,
> + AcpiTableLength
> );
> return;
> }
> --
> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
>
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [edk2-devel] [PATCH v1 1/1] ShellPkg: acpiview: Prevent infinite loop if structure length is 0
2020-02-17 15:39 ` Krzysztof Koch
@ 2020-02-19 1:44 ` Gao, Zhichao
0 siblings, 0 replies; 7+ messages in thread
From: Gao, Zhichao @ 2020-02-19 1:44 UTC (permalink / raw)
To: devel@edk2.groups.io, krzysztof.koch@arm.com, Gao, Liming
Cc: Ni, Ray, Sami Mujawar, Matteo Carlini, nd
Please add the the BZ link to the commit message as blow format:
--------------
Pkg/Module: message title
REF: BZ_Link
Commit message.
Signed-off-by: Name <email address>
--------------
With that done, 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: Monday, February 17, 2020 11:39 PM
> To: devel@edk2.groups.io; Krzysztof Koch <Krzysztof.Koch@arm.com>; Gao,
> Liming <liming.gao@intel.com>
> Cc: Ni, Ray <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>; Sami
> Mujawar <Sami.Mujawar@arm.com>; Matteo Carlini
> <Matteo.Carlini@arm.com>; nd <nd@arm.com>
> Subject: Re: [edk2-devel] [PATCH v1 1/1] ShellPkg: acpiview: Prevent infinite
> loop if structure length is 0
>
> Hi Liming,
>
> The BZ is: https://bugzilla.tianocore.org/show_bug.cgi?id=2534
> Please let me know if I should change something.
>
> Kind regards,
> Krzysztof
>
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Krzysztof
> Koch via Groups.Io
> Sent: Monday, February 17, 2020 15:23
> To: devel@edk2.groups.io; liming.gao@intel.com
> Cc: Ni, Ray <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>; Sami
> Mujawar <Sami.Mujawar@arm.com>; Matteo Carlini
> <Matteo.Carlini@arm.com>; nd <nd@arm.com>
> Subject: Re: [edk2-devel] [PATCH v1 1/1] ShellPkg: acpiview: Prevent infinite
> loop if structure length is 0
>
> Hi Liming,
>
> I haven't created a BZ yet, shall I create one? It would be great if the patch
> makes it to the stable tag.
>
> Over the last few months I added some security features to acpiview. They make
> this debug tool less sensitive to exploits from ACPI tables. This patch completes
> my efforts in making the tool more reliable.
>
> Kind regards,
> Krzysztof
>
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Liming Gao
> via Groups.Io
> Sent: Monday, February 17, 2020 15:11
> To: devel@edk2.groups.io; Krzysztof Koch <Krzysztof.Koch@arm.com>
> Cc: Ni, Ray <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>; Sami
> Mujawar <Sami.Mujawar@arm.com>; Matteo Carlini
> <Matteo.Carlini@arm.com>; nd <nd@arm.com>
> Subject: Re: [edk2-devel] [PATCH v1 1/1] ShellPkg: acpiview: Prevent infinite
> loop if structure length is 0
>
> Krzysztof:
> Is there one BZ for this issue? Does this patch catch to this edk2 stable tag
> 202002?
>
> Thanks
> Liming
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> > Krzysztof Koch
> > Sent: Friday, February 14, 2020 9:59 PM
> > To: devel@edk2.groups.io
> > Cc: Ni, Ray <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>;
> > Sami.Mujawar@arm.com; Matteo.Carlini@arm.com; nd@arm.com
> > Subject: [edk2-devel] [PATCH v1 1/1] ShellPkg: acpiview: Prevent
> > infinite loop if structure length is 0
> >
> > Extend validation of ACPI structure lengths which are read from the
> > ACPI table being parsed. Additionally check if the structure 'Length'
> > field value is positive. If not, stop parsing the faulting table.
> >
> > Some ACPI tables define internal structures of variable size. The
> > 'Length' field inside the substructure is used to update a pointer
> > used for table traversal. If the byte-length of the structure is equal
> > to 0, acpiview can enter an infinite loop. This condition can occur
> > if, for example, the zero-allocated ACPI table buffer is not fully populated.
> > This is typically a bug on the ACPI table writer side.
> >
> > In short, this method helps acpiview recover gracefully from a
> > zero-valued ACPI structure length.
> >
> > Signed-off-by: Krzysztof Koch <krzysztof.koch@arm.com>
> > ---
> >
> > Changes can be seen at:
> > https://github.com/KrzysztofKoch1/edk2/tree/612_acpiview_prevent_inf_l
> > oops_v1
> >
> > Notes:
> > v1:
> > - prevent infinite loops in acpiview parsers [Krzysztof]
> >
> >
> > ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c
> > | 15 ++++++-----
> > ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser.c
> > | 13 ++++-----
> > ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser.c
> > | 14 +++++-----
> > ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.c
> > | 28 ++++++--------------
> > ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c
> > | 15 ++++++-----
> > ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser.c
> > | 14 +++++-----
> > 6 files changed, 47 insertions(+), 52 deletions(-)
> >
> > diff --git
> > a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser
> > .c
> > b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser
> > .c index
> >
> 0f730a306a94329a23fbaf54b59f1833b44616ba..9df111ecaa7d7a703a13a39c24
> 3e
> > d78b9f12ee97 100644
> > ---
> > a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser
> > .c
> > +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Pa
> > +++ rser.c
> > @@ -1,7 +1,7 @@
> > /** @file
> > DBG2 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
> >
> > @par Reference(s):
> > @@ -282,15 +282,16 @@ ParseAcpiDbg2 (
> > return;
> > }
> >
> > - // Make sure the Debug Device Information structure lies inside the table.
> > - if ((Offset + *DbgDevInfoLen) > AcpiTableLength) {
> > + // Validate Debug Device Information Structure length
> > + if ((*DbgDevInfoLen == 0) ||
> > + ((Offset + (*DbgDevInfoLen)) > AcpiTableLength)) {
> > IncrementErrorCount ();
> > Print (
> > - L"ERROR: Invalid Debug Device Information structure length. " \
> > - L"DbgDevInfoLen = %d. RemainingTableBufferLength = %d. " \
> > - L"DBG2 parsing aborted.\n",
> > + L"ERROR: Invalid Debug Device Information Structure length. " \
> > + L"Length = %d. Offset = %d. AcpiTableLength = %d.\n",
> > *DbgDevInfoLen,
> > - AcpiTableLength - Offset
> > + Offset,
> > + AcpiTableLength
> > );
> > return;
> > }
> > diff --git
> > a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser
> > .c
> > b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser
> > .c index
> >
> 699a55b549ec3fa61bbd156898821055dc019199..bdd30ff45c61142c071ead63a
> 27b
> > abab8998721b 100644
> > ---
> > a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser
> > .c
> > +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtPa
> > +++ rser.c
> > @@ -1,7 +1,7 @@
> > /** @file
> > GTDT 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
> >
> > @par Reference(s):
> > @@ -327,15 +327,16 @@ ParseAcpiGtdt (
> > return;
> > }
> >
> > - // Make sure the Platform Timer is inside the table.
> > - if ((Offset + *PlatformTimerLength) > AcpiTableLength) {
> > + // Validate Platform Timer Structure length
> > + if ((*PlatformTimerLength == 0) ||
> > + ((Offset + (*PlatformTimerLength)) > AcpiTableLength)) {
> > IncrementErrorCount ();
> > Print (
> > L"ERROR: Invalid Platform Timer Structure length. " \
> > - L"PlatformTimerLength = %d. RemainingTableBufferLength = %d. " \
> > - L"GTDT parsing aborted.\n",
> > + L"Length = %d. Offset = %d. AcpiTableLength = %d.\n",
> > *PlatformTimerLength,
> > - AcpiTableLength - Offset
> > + Offset,
> > + AcpiTableLength
> > );
> > return;
> > }
> > diff --git
> > a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser
> > .c
> > b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser
> > .c index
> >
> 9d5d937c7b2c19945ca2ad3eba644bdfc09cc3f6..9a006a01448b897865cd7cd85
> 651
> > c816933acf05 100644
> > ---
> > a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser
> > .c
> > +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortPa
> > +++ rser.c
> > @@ -1,7 +1,7 @@
> > /** @file
> > IORT 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
> >
> > @par Reference(s):
> > @@ -687,14 +687,16 @@ ParseAcpiIort (
> > return;
> > }
> >
> > - // Make sure the IORT Node is inside the table
> > - if ((Offset + (*IortNodeLength)) > AcpiTableLength) {
> > + // Validate IORT Node length
> > + if ((*IortNodeLength == 0) ||
> > + ((Offset + (*IortNodeLength)) > AcpiTableLength)) {
> > IncrementErrorCount ();
> > Print (
> > - L"ERROR: Invalid IORT node length. IortNodeLength = %d. " \
> > - L"RemainingTableBufferLength = %d. IORT parsing aborted.\n",
> > + L"ERROR: Invalid IORT Node length. " \
> > + L"Length = %d. Offset = %d. AcpiTableLength = %d.\n",
> > *IortNodeLength,
> > - AcpiTableLength - Offset
> > + Offset,
> > + AcpiTableLength
> > );
> > return;
> > }
> > diff --git
> > a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser
> > .c
> > b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser
> > .c index
> >
> 438905cb24f58b8b82e8fe61280e72f765d578d8..f85d2b36532cfc5db36fe7bef9
> 83
> > 0cccc64969cc 100644
> > ---
> > a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser
> > .c
> > +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtPa
> > +++ rser.c
> > @@ -1,7 +1,7 @@
> > /** @file
> > MADT 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
> >
> > @par Reference(s):
> > @@ -273,28 +273,16 @@ ParseAcpiMadt (
> > return;
> > }
> >
> > - // Make sure forward progress is made.
> > - if (*MadtInterruptControllerLength < 2) {
> > + // Validate Interrupt Controller Structure length
> > + if ((*MadtInterruptControllerLength == 0) ||
> > + ((Offset + (*MadtInterruptControllerLength)) >
> > + AcpiTableLength)) {
> > IncrementErrorCount ();
> > Print (
> > - L"ERROR: Structure length is too small: " \
> > - L"MadtInterruptControllerLength = %d. " \
> > - L"MadtInterruptControllerType = %d. MADT parsing aborted.\n",
> > + L"ERROR: Invalid Interrupt Controller Structure length. " \
> > + L"Length = %d. Offset = %d. AcpiTableLength = %d.\n",
> > *MadtInterruptControllerLength,
> > - *MadtInterruptControllerType
> > - );
> > - return;
> > - }
> > -
> > - // Make sure the MADT structure lies inside the table
> > - if ((Offset + *MadtInterruptControllerLength) > AcpiTableLength) {
> > - IncrementErrorCount ();
> > - Print (
> > - L"ERROR: Invalid MADT structure length. " \
> > - L"MadtInterruptControllerLength = %d. " \
> > - L"RemainingTableBufferLength = %d. MADT parsing aborted.\n",
> > - *MadtInterruptControllerLength,
> > - AcpiTableLength - Offset
> > + Offset,
> > + AcpiTableLength
> > );
> > return;
> > }
> > diff --git
> > a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser
> > .c
> > b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser
> > .c index
> >
> 675ba75f02b367cd5ad9f2ac23c30ed0ab58f286..0db272c16af0ad8824c8da4c88
> dd
> > 409c8550112a 100644
> > ---
> > a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser
> > .c
> > +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttPa
> > +++ rser.c
> > @@ -1,7 +1,7 @@
> > /** @file
> > PPTT table parser
> >
> > - Copyright (c) 2019, ARM Limited. All rights reserved.
> > + Copyright (c) 2019 - 2020, ARM Limited. All rights reserved.
> > SPDX-License-Identifier: BSD-2-Clause-Patent
> >
> > @par Reference(s):
> > @@ -425,15 +425,16 @@ ParseAcpiPptt (
> > return;
> > }
> >
> > - // Make sure the PPTT structure lies inside the table
> > - if ((Offset + *ProcessorTopologyStructureLength) > AcpiTableLength) {
> > + // Validate Processor Topology Structure length
> > + if ((*ProcessorTopologyStructureLength == 0) ||
> > + ((Offset + (*ProcessorTopologyStructureLength)) >
> > + AcpiTableLength)) {
> > IncrementErrorCount ();
> > Print (
> > - L"ERROR: Invalid PPTT structure length. " \
> > - L"ProcessorTopologyStructureLength = %d. " \
> > - L"RemainingTableBufferLength = %d. PPTT parsing aborted.\n",
> > + L"ERROR: Invalid Processor Topology Structure length. " \
> > + L"Length = %d. Offset = %d. AcpiTableLength = %d.\n",
> > *ProcessorTopologyStructureLength,
> > - AcpiTableLength - Offset
> > + Offset,
> > + AcpiTableLength
> > );
> > return;
> > }
> > diff --git
> > a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser
> > .c
> > b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser
> > .c index
> >
> 3613900ae322483fdd3d3383de4e22ba75b2128b..6f66be68cc0bed14811a0432c
> 61a
> > 79fd47c54890 100644
> > ---
> > a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser
> > .c
> > +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratPa
> > +++ rser.c
> > @@ -1,7 +1,7 @@
> > /** @file
> > SRAT 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
> >
> > @par Reference(s):
> > @@ -412,14 +412,16 @@ ParseAcpiSrat (
> > return;
> > }
> >
> > - // Make sure the SRAT structure lies inside the table
> > - if ((Offset + *SratRALength) > AcpiTableLength) {
> > + // Validate Static Resource Allocation Structure length
> > + if ((*SratRALength == 0) ||
> > + ((Offset + (*SratRALength)) > AcpiTableLength)) {
> > IncrementErrorCount ();
> > Print (
> > - L"ERROR: Invalid SRAT structure length. SratRALength = %d. " \
> > - L"RemainingTableBufferLength = %d. SRAT parsing aborted.\n",
> > + L"ERROR: Invalid Static Resource Allocation Structure length. " \
> > + L"Length = %d. Offset = %d. AcpiTableLength = %d.\n",
> > *SratRALength,
> > - AcpiTableLength - Offset
> > + Offset,
> > + AcpiTableLength
> > );
> > return;
> > }
> > --
> > 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
> >
> >
> >
>
>
>
>
>
>
>
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-02-19 1:44 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-02-14 13:59 [PATCH v1 1/1] ShellPkg: acpiview: Prevent infinite loop if structure length is 0 Krzysztof Koch
2020-02-17 15:11 ` [edk2-devel] " Liming Gao
2020-02-17 15:22 ` Krzysztof Koch
2020-02-17 15:24 ` Liming Gao
[not found] ` <15F439D65D9DE013.5373@groups.io>
2020-02-17 15:39 ` Krzysztof Koch
2020-02-19 1:44 ` Gao, Zhichao
2020-02-17 15:35 ` Sami Mujawar
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox