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....@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....@intel.com>; Krzysztof
> Koch <krzysztof.k...@arm.com>
> Cc: Carsey, Jaben <jaben.car...@intel.com>; Ni, Ray <ray...@intel.com>;
> Matteo Carlini <matteo.carl...@arm.com>; nd <n...@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.k...@arm.com>
> Cc: Carsey, Jaben <jaben.car...@intel.com>; Ni, Ray <ray...@intel.com>;
> Sami Mujawar <sami.muja...@arm.com>; Matteo Carlini
> <matteo.carl...@arm.com>; nd <n...@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.car...@intel.com>; Ni, Ray
> > <ray...@intel.com>; Gao, Zhichao <zhichao....@intel.com>;
> > sami.muja...@arm.com; matteo.carl...@arm.com; n...@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.k...@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)'
> >
> >
> >
> >
>
>
>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#44924): https://edk2.groups.io/g/devel/message/44924
Mute This Topic: https://groups.io/mt/32676830/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
