Re: [PATCH v2 1/1] ShellPkg: acpiview: Prevent infinite loop if structure length is 0

["Gao, Zhichao" <zhichao.gao@intel.com>]

Since there is no change for the V2 patch. It is a good behavior to add the R-B from V1 to V2. That would make the reviewer aware that there is no code change and review requirement for this patch. I would keep the R-B in V1 and help to merge the patch.

Thanks,
Zhichao

> -----Original Message-----
> From: Krzysztof Koch <krzysztof.koch@arm.com>
> Sent: Wednesday, February 19, 2020 6:24 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: [PATCH v2 1/1] ShellPkg: acpiview: Prevent infinite loop if structure
> length is 0
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2534
> 
> 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
> _v2
> 
> Notes:
>     v2:
>     - Add BZ link to the commit message [Zhichao]
> 
>     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
> 3ed78b9f12ee97 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..bdd30ff45c61142c071ead63a
> 27babab8998721b 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..9a006a01448b897865cd7cd85
> 651c816933acf05 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..f85d2b36532cfc5db36fe7bef9
> 830cccc64969cc 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..0db272c16af0ad8824c8da4c88
> dd409c8550112a 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..6f66be68cc0bed14811a0432c
> 61a79fd47c54890 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)'