public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Gao, Zhichao" <zhichao.gao@intel.com>
To: "devel@edk2.groups.io" <devel@edk2.groups.io>,
	"krzysztof.koch@arm.com" <krzysztof.koch@arm.com>
Cc: "Carsey, Jaben" <jaben.carsey@intel.com>,
	"Ni, Ray" <ray.ni@intel.com>,
	"Sami.Mujawar@arm.com" <Sami.Mujawar@arm.com>,
	"Matteo.Carlini@arm.com" <Matteo.Carlini@arm.com>,
	"nd@arm.com" <nd@arm.com>
Subject: Re: [edk2-devel] [PATCH v1 2/6] ShellPkg: acpiview: GTDT: Prevent buffer overruns
Date: Mon, 5 Aug 2019 07:23:01 +0000	[thread overview]
Message-ID: <3CE959C139B4C44DBEA1810E3AA6F9000B81FAE0@SHSMSX101.ccr.corp.intel.com> (raw)
In-Reply-To: <20190801084407.48712-3-krzysztof.koch@arm.com>

One confusion below:

> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Krzysztof Koch
> Sent: Thursday, August 1, 2019 4:44 PM
> To: devel@edk2.groups.io
> Cc: Carsey, Jaben <jaben.carsey@intel.com>; 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 2/6] ShellPkg: acpiview: GTDT: Prevent
> buffer overruns
> 
> Modify the GTDT table parsing logic to prevent reading past the ACPI buffer
> lengths provided and to make it consistent with other table parsers. This
> includes converting the do-while loop in ParseAcpiGtdt() into a while loop.
> 
> Remove a check which ensures that the entire Platform GT Block Structure
> buffer has been parsed. The ACPI specification does not ban from defining
> buffers which are larger than the size indicated by the count and sizes of
> substructures which constitute it.
> 
> Change the data type of the Length parameter to the DumpGTBlock()
> function to reflect the width of the respective ACPI structure's field.
> 
> References:
> - ACPI 6.3, January 2019, Table 5-124
> 
> Signed-off-by: Krzysztof Koch <krzysztof.koch@arm.com>
> ---
> 
> Notes:
>     v1:
>     - Prevent buffer overruns in GTDT acpiview parser [Krzysztof]
> 
>  ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser.c
> | 147 ++++++++++----------
>  1 file changed, 76 insertions(+), 71 deletions(-)
> 
> diff --git
> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser
> .c
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser
> .c
> index
> 1e5b5764f50a2d29aa904c889bc89af5bdc3af5c..57174e14c80072f12b90e1996e
> be8f0002d0c404 100644
> ---
> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser
> .c
> +++
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtPars
> +++ er.c
> @@ -23,7 +23,6 @@ STATIC CONST UINT8*  PlatformTimerType;  STATIC
> CONST UINT16* PlatformTimerLength;  STATIC CONST UINT32*
> GtBlockTimerCount;  STATIC CONST UINT32* GtBlockTimerOffset; -STATIC
> CONST UINT16* GtBlockLength;  STATIC ACPI_DESCRIPTION_HEADER_INFO
> AcpiHdrInfo;
> 
>  /**
> @@ -127,7 +126,7 @@ STATIC CONST ACPI_PARSER
> GtPlatformTimerHeaderParser[] = {  **/  STATIC CONST ACPI_PARSER
> GtBlockParser[] = {
>    {L"Type", 1, 0, L"%d", NULL, NULL, NULL, NULL},
> -  {L"Length", 2, 1, L"%d", NULL, (VOID**)&GtBlockLength, NULL, NULL},
> +  {L"Length", 2, 1, L"%d", NULL, NULL, NULL, NULL},
>    {L"Reserved", 1, 3, L"%x", NULL, NULL, NULL, NULL},
>    {L"Physical address (CntCtlBase)", 8, 4, L"0x%lx", NULL, NULL, NULL, NULL},
>    {L"Timer Count", 4, 12, L"%d", NULL, (VOID**)&GtBlockTimerCount, @@ -
> 168,56 +167,43 @@ STATIC CONST ACPI_PARSER
> SBSAGenericWatchdogParser[] = {
>  /**
>    This function parses the Platform GT Block.
> 
> -  @param [in] Ptr     Pointer to the start of the GT Block data.
> -  @param [in] Length  Length of the GT Block structure.
> +  @param [in] Ptr       Pointer to the start of the GT Block data.
> +  @param [in] Length    Length of the GT Block structure.
>  **/
>  STATIC
>  VOID
>  DumpGTBlock (
>    IN UINT8* Ptr,
> -  IN UINT32 Length
> +  IN UINT16 Length
>    )
>  {
>    UINT32 Index;
>    UINT32 Offset;
> -  UINT32 GTBlockTimerLength;
> 
> -  Offset = ParseAcpi (
> -             TRUE,
> -             2,
> -             "GT Block",
> -             Ptr,
> -             Length,
> -             PARSER_PARAMS (GtBlockParser)
> -             );
> -  GTBlockTimerLength = (*GtBlockLength - Offset) / (*GtBlockTimerCount);
> -  Length -= Offset;
> +  ParseAcpi (
> +    TRUE,
> +    2,
> +    "GT Block",
> +    Ptr,
> +    Length,
> +    PARSER_PARAMS (GtBlockParser)
> +    );
> 
> -  if (*GtBlockTimerCount != 0) {
> -    Ptr += (*GtBlockTimerOffset);
> -    Index = 0;
> -    while ((Index < (*GtBlockTimerCount)) && (Length >=
> GTBlockTimerLength)) {
> -      Offset = ParseAcpi (
> -                 TRUE,
> -                 2,
> -                 "GT Block Timer",
> -                 Ptr,
> -                 GTBlockTimerLength,
> -                 PARSER_PARAMS (GtBlockTimerParser)
> -                 );
> -      // Increment by GT Block Timer structure size
> -      Ptr += Offset;
> -      Length -= Offset;
> -      Index++;
> -    }
> +  Offset = *GtBlockTimerOffset;
> +  Index = 0;
> 
> -    if (Length != 0) {
> -      IncrementErrorCount ();
> -      Print (
> -        L"ERROR:GT Block Timer length mismatch. Unparsed %d bytes.\n",
> -        Length
> -        );
> -    }
> +  // Parse the specified number of GT Block Timer Structures or the GT
> + Block  // Structure buffer length. Whichever is minimum.
> +  while ((Index++ < *GtBlockTimerCount) &&
> +         (Offset < Length)) {
> +    Offset += ParseAcpi (
> +                TRUE,
> +                2,
> +                "GT Block Timer",
> +                Ptr + Offset,
> +                Length - Offset,
> +                PARSER_PARAMS (GtBlockTimerParser)
> +                );

The above confuse me a lot. ACPI Spec 6.3 Table 5-124:
Length - "20+n*40, where n is the number of timers implemented in the GT Block"
GT Block Timer Structure[] - Byte Offset: GT Block Timer Offset - "Array of GT Block Timer Structures. See Table 5-125"

Why the 'Byte Offset' is not 20?
'Length - Offset' would be 'Length - 20' == 'n * 40'. If the 'GT Block Timer Offset' is not 20, its value should be lager 20. Then 'Length - Offset' would always less than 'n * 40'. It may miss  some info of the GtBlockTimer.
Maybe all the platforms' GT block table's 'GT Block Timer Offset' is always 20. If so, then there is no problem with this above section.

Did I misunderstand something?

Thanks,
Zhichao

>    }
>  }
> 
> @@ -270,6 +256,7 @@ ParseAcpiGtdt (
>    )
>  {
>    UINT32 Index;
> +  UINT32 Offset;
>    UINT8* TimerPtr;
> 
>    if (!Trace) {
> @@ -285,36 +272,54 @@ ParseAcpiGtdt (
>      PARSER_PARAMS (GtdtParser)
>      );
> 
> -  if (*GtdtPlatformTimerCount != 0) {
> -    TimerPtr = Ptr + (*GtdtPlatformTimerOffset);
> -    Index = 0;
> -    do {
> -      // Parse the Platform Timer Header
> -      ParseAcpi (
> -        FALSE,
> -        0,
> -        NULL,
> -        TimerPtr,
> -        4,  // GT Platform Timer structure header length.
> -        PARSER_PARAMS (GtPlatformTimerHeaderParser)
> +  TimerPtr = Ptr + *GtdtPlatformTimerOffset;  Offset =
> + *GtdtPlatformTimerOffset;  Index = 0;
> +
> +  // Parse the specified number of Platform Timer Structures or the
> + GTDT  // buffer length. Whichever is minimum.
> +  while ((Index++ < *GtdtPlatformTimerCount) &&
> +         (Offset < AcpiTableLength)) {
> +    // Parse the Platform Timer Header to obtain Length and Type
> +    ParseAcpi (
> +      FALSE,
> +      0,
> +      NULL,
> +      TimerPtr,
> +      AcpiTableLength - Offset,
> +      PARSER_PARAMS (GtPlatformTimerHeaderParser)
> +      );
> +
> +    // Make sure the Platform Timer is inside the table.
> +    if ((Offset + *PlatformTimerLength) > AcpiTableLength) {
> +      IncrementErrorCount ();
> +      Print (
> +        L"ERROR: Invalid Platform Timer Structure length. " \
> +          L"PlatformTimerLength = %d. RemainingTableBufferLength = %d. " \
> +          L"GTDT parsing aborted.\n",
> +        *PlatformTimerLength,
> +        AcpiTableLength - Offset
>          );
> -      switch (*PlatformTimerType) {
> -        case EFI_ACPI_6_2_GTDT_GT_BLOCK:
> -          DumpGTBlock (TimerPtr, *PlatformTimerLength);
> -          break;
> -        case EFI_ACPI_6_2_GTDT_SBSA_GENERIC_WATCHDOG:
> -          DumpWatchdogTimer (TimerPtr, *PlatformTimerLength);
> -          break;
> -        default:
> -          IncrementErrorCount ();
> -          Print (
> -            L"ERROR: INVALID Platform Timer Type = %d\n",
> -            *PlatformTimerType
> -            );
> -          break;
> -      } // switch
> -      TimerPtr += (*PlatformTimerLength);
> -      Index++;
> -    } while (Index < *GtdtPlatformTimerCount);
> -  }
> +      return;
> +    }
> +
> +    switch (*PlatformTimerType) {
> +      case EFI_ACPI_6_3_GTDT_GT_BLOCK:
> +        DumpGTBlock (TimerPtr, *PlatformTimerLength);
> +        break;
> +      case EFI_ACPI_6_3_GTDT_SBSA_GENERIC_WATCHDOG:
> +        DumpWatchdogTimer (TimerPtr, *PlatformTimerLength);
> +        break;
> +      default:
> +        IncrementErrorCount ();
> +        Print (
> +          L"ERROR: Invalid Platform Timer Type = %d\n",
> +          *PlatformTimerType
> +          );
> +        break;
> +    } // switch
> +
> +    TimerPtr += *PlatformTimerLength;
> +    Offset += *PlatformTimerLength;
> +  } // while
>  }
> --
> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
> 
> 
> 
> 


  parent reply	other threads:[~2019-08-05  7:23 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-01  8:44 [PATCH v1 0/6] Prevent buffer overruns in acpiview table parsers Krzysztof Koch
2019-08-01  8:44 ` [PATCH v1 1/6] ShellPkg: acpiview: DBG2: Prevent buffer overruns Krzysztof Koch
2019-08-01  9:33   ` [edk2-devel] " Alexei Fedorov
2019-08-05  6:48   ` Gao, Zhichao
2019-08-05  8:21     ` Krzysztof Koch
2019-08-06  7:43       ` [edk2-devel] " Gao, Zhichao
2019-08-06 10:44         ` Krzysztof Koch
2019-08-07  1:52           ` Gao, Zhichao
2019-08-01  8:44 ` [PATCH v1 2/6] ShellPkg: acpiview: GTDT: " Krzysztof Koch
2019-08-01  9:33   ` [edk2-devel] " Alexei Fedorov
2019-08-05  7:23   ` Gao, Zhichao [this message]
2019-08-05  9:54     ` Sami Mujawar
2019-08-06  4:58       ` Gao, Zhichao
2019-08-01  8:44 ` [PATCH v1 3/6] ShellPkg: acpiview: IORT: " Krzysztof Koch
2019-08-01  9:32   ` [edk2-devel] " Alexei Fedorov
2019-08-01  8:44 ` [PATCH v1 4/6] ShellPkg: acpiview: MADT: " Krzysztof Koch
2019-08-01  9:32   ` [edk2-devel] " Alexei Fedorov
2019-08-01  8:44 ` [PATCH v1 5/6] ShellPkg: acpiview: PPTT: " Krzysztof Koch
2019-08-01  9:31   ` [edk2-devel] " Alexei Fedorov
2019-08-01  8:44 ` [PATCH v1 6/6] ShellPkg: acpiview: SRAT: " Krzysztof Koch
2019-08-01  9:30   ` [edk2-devel] " Alexei Fedorov
2019-08-01  9:33 ` [edk2-devel] [PATCH v1 0/6] Prevent buffer overruns in acpiview table parsers Alexei Fedorov
2019-08-06  8:10 ` Gao, Zhichao
2019-08-07  8:46 ` 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=3CE959C139B4C44DBEA1810E3AA6F9000B81FAE0@SHSMSX101.ccr.corp.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