Re: [edk2-devel] [PATCH v1 2/6] ShellPkg: acpiview: GTDT: Prevent buffer overruns

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

OK, I got it. Now I have no comment of this patch. I think the description of 'Length' should be 'the whole length of the GT block include GT Block Timer Structure' or '20+("GT Block Timer Offset" - 20) + n * 40, where n ...' in the ACPI spec.
Reviewed-by: Zhichao Gao <zhichao.gao@intel.com>

Thanks,
Zhichao

> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Sami Mujawar
> Sent: Monday, August 5, 2019 5:55 PM
> To: devel@edk2.groups.io; Gao, Zhichao <zhichao.gao@intel.com>; Krzysztof
> Koch <Krzysztof.Koch@arm.com>
> Cc: Carsey, Jaben <jaben.carsey@intel.com>; Ni, Ray <ray.ni@intel.com>;
> Matteo Carlini <Matteo.Carlini@arm.com>; nd <nd@arm.com>
> Subject: Re: [edk2-devel] [PATCH v1 2/6] ShellPkg: acpiview: GTDT: Prevent
> buffer overruns
> 
> Hi Zhichao,
> 
> Please see my response inline.
> 
> Regards,
> 
> Sami Mujawar
> 
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Gao,
> Zhichao via Groups.Io
> Sent: 05 August 2019 08:23 AM
> To: devel@edk2.groups.io; Krzysztof Koch <Krzysztof.Koch@arm.com>
> Cc: Carsey, Jaben <jaben.carsey@intel.com>; Ni, Ray <ray.ni@intel.com>;
> Sami Mujawar <Sami.Mujawar@arm.com>; Matteo Carlini
> <Matteo.Carlini@arm.com>; nd <nd@arm.com>
> Subject: Re: [edk2-devel] [PATCH v1 2/6] ShellPkg: acpiview: GTDT: Prevent
> buffer overruns
> 
> 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?
> [SAMI] The '20+n*40' in the 'Description' column in the ACPI Spec 6.3 Table 5-
> 124 is an example of how the length field may be computed.
> The 'GT Block Timer Offset' field could technically allow unused bytes
> between the 'GT Block Timer Offset' field and the start of the first 'GT Block
> Timer Structure'. An OS is expected to read the 'GT Block Timer Offset' field
> to know where the 'GT Block Timer Structure' data starts. Conversely the
> firmware must encode the length field appropriately. In this case the length
> field would be '20+UnusedByteCount+(n*40)'.
> 
> The 'Length - Offset' is used to ensure we don't run past the buffer. This also
> reinforces the fact that ACPIview would be providing a view of the tables as
> an OS might see.
> 
> I will check if this can be made clearer in the specification.
> [/SAMI]
> 
> 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)'
> >
> >
> >
> >
> 
> 
> 
> 
> 
>