From: "Liming Gao" <liming.gao@intel.com>
To: "devel@edk2.groups.io" <devel@edk2.groups.io>,
"krzysztof.koch@arm.com" <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
Date: Mon, 17 Feb 2020 15:24:24 +0000 [thread overview]
Message-ID: <e277732d5ccb4ae196a7b289e98453f8@intel.com> (raw)
In-Reply-To: <AM5PR0801MB177722F75EAB452CDF680B9D84160@AM5PR0801MB1777.eurprd08.prod.outlook.com>
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)'
> >
> >
> >
>
>
>
>
>
>
next prev parent reply other threads:[~2020-02-17 15:24 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
[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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=e277732d5ccb4ae196a7b289e98453f8@intel.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox