Re: [PATCH v1 11/11] ShellPkg: acpiview: DBG2: Add error-checking in the parsing logic

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

Two comments with this patch.

> -----Original Message-----
> From: Krzysztof Koch [mailto:krzysztof.koch@arm.com]
> Sent: Friday, July 12, 2019 2:53 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: [PATCH v1 11/11] ShellPkg: acpiview: DBG2: Add error-checking in
> the parsing logic
> 
> 1. Check if the global pointers (in the scope of this ACPI table parser) have
> been successfully updated before they are later used to control the parsing
> logic.
> 
> 2. Remove redundant forward function declarations by repositioning blocks
> of code.
> 
> 3. Test against buffer overruns.
> 
> 4. Introduce a ACPI_PARSER array for parsing the header of the debug device
> information structure. This way, the length of the buffer storing a debug
> device information structure instance can be passed to
> DumpDbgDeviceInfo(). Consequently, the parsing logic becomes consistent
> with other ACPI table parsers and tests against buffer overrruns are simpler
> to implement.
> 
> 5. Modify the signature of DumpGasStruct() function inside AcpiParser.c to
> facilitate protection against buffer overruns in the DBG2 parser.
> 
> Signed-off-by: Krzysztof Koch <krzysztof.koch@arm.com>
> ---
> 
> Changes can be seen at:
> https://github.com/KrzysztofKoch1/edk2/commit/530b059a9fe4aa9f1df36b4
> 07f97d76acaab8b74
> 
> Notes:
>     v1:
>     - improve the logic in the DBG2 parser [Krzysztof]
> 
>  ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c              |  26 +-
>  ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h              |   8 +-
> 
> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c
> | 298 ++++++++++++++------
>  3 files changed, 225 insertions(+), 107 deletions(-)
> 
> diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c
> index
> 8b3153516d2b7d9b920ab2de0344c17798ac572c..2d6ff80e299eebe7853061d3
> db89332197c0dc0e 100644
> --- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c
> +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c
> @@ -589,23 +589,27 @@ STATIC CONST ACPI_PARSER GasParser[] = {
> 
>    @param [in] Ptr     Pointer to the start of the buffer.
>    @param [in] Indent  Number of spaces to indent the output.
> +  @param [in] Length  Length of the GAS structure buffer.
> +
> +  @retval Number of bytes parsed.
>  **/
> -VOID
> +UINT32
>  EFIAPI
>  DumpGasStruct (
>    IN UINT8*        Ptr,
> -  IN UINT32        Indent
> +  IN UINT32        Indent,
> +  IN UINT32        Length
>    )
>  {
>    Print (L"\n");
> -  ParseAcpi (
> -    TRUE,
> -    Indent,
> -    NULL,
> -    Ptr,
> -    GAS_LENGTH,
> -    PARSER_PARAMS (GasParser)
> -    );
> +  return ParseAcpi (
> +           TRUE,
> +           Indent,
> +           NULL,
> +           Ptr,
> +           Length,
> +           PARSER_PARAMS (GasParser)
> +           );
>  }
> 
>  /**
> @@ -621,7 +625,7 @@ DumpGas (
>    IN UINT8*        Ptr
>    )
>  {
> -  DumpGasStruct (Ptr, 2);
> +  DumpGasStruct (Ptr, 2, GAS_LENGTH);
>  }
> 
>  /**
> diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h
> index
> 7657892d9fd2e2e14c6578611ff0cf1b6f6cd750..20ca358bddfa5953bfb1d1beba
> ebbf3079eaba01 100644
> --- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h
> +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h
> @@ -405,12 +405,16 @@ ParseAcpi (
> 
>    @param [in] Ptr     Pointer to the start of the buffer.
>    @param [in] Indent  Number of spaces to indent the output.
> +  @param [in] Length  Length of the GAS structure buffer.
> +
> +  @retval Number of bytes parsed.
>  **/
> -VOID
> +UINT32
>  EFIAPI
>  DumpGasStruct (
>    IN UINT8*        Ptr,
> -  IN UINT32        Indent
> +  IN UINT32        Indent,
> +  IN UINT32        Length
>    );
> 
>  /**
> diff --git
> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parse
> r.c
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parse
> r.c
> index
> 8de5ebf74775bab8e765849cba6ef4eb6f659a5a..2bbd622ffb7cec0a340de3e10
> bdcd01ba4d330df 100644
> ---
> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parse
> r.c
> +++
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Pars
> +++ er.c
> @@ -12,6 +12,7 @@
>  #include <Library/UefiLib.h>
>  #include "AcpiParser.h"
>  #include "AcpiTableParser.h"
> +#include "AcpiView.h"
> 
>  // Local variables pointing to the table fields  STATIC CONST UINT32*
> OffsetDbgDeviceInfo; @@ -27,7 +28,7 @@ STATIC CONST UINT16*
> AddrSizeOffset;  STATIC ACPI_DESCRIPTION_HEADER_INFO AcpiHdrInfo;
> 
>  /**
> -  This function Validates the NameSpace string length.
> +  This function validates the NameSpace string length.
> 
>    @param [in] Ptr     Pointer to the start of the buffer.
>    @param [in] Context Pointer to context specific information e.g. this @@ -
> 37,24 +38,23 @@ STATIC  VOID  EFIAPI  ValidateNameSpaceStrLen (
> -  IN  UINT8* Ptr,
> -  IN  VOID*  Context
> -  );
> +  IN UINT8* Ptr,
> +  IN VOID*  Context
> +  )
> +{
> +  UINT16 NameSpaceStrLen;
> 
> -/**
> -  This function parses the debug device information structure.
> +  NameSpaceStrLen = *(UINT16*)Ptr;
> 
> -  @param [in]  Ptr     Pointer to the start of the buffer.
> -  @param [out] Length  Pointer in which the length of the debug
> -                       device information is returned.
> -**/
> -STATIC
> -VOID
> -EFIAPI
> -DumpDbgDeviceInfo (
> -  IN  UINT8*  Ptr,
> -  OUT UINT32* Length
> -  );
> +  if (NameSpaceStrLen < 2) {
> +    IncrementErrorCount ();
> +    Print (
> +      L"\nERROR: NamespaceString Length = %d. If no Namespace device
> exists, " \
> +        L"NamespaceString[] must contain a period '.'",
> +      NameSpaceStrLen
> +      );
> +  }
> +}
> 
>  /// An ACPI_PARSER array describing the ACPI DBG2 table.
>  STATIC CONST ACPI_PARSER Dbg2Parser[] = { @@ -65,10 +65,17 @@ STATIC
> CONST ACPI_PARSER Dbg2Parser[] = {
>     (VOID**)&NumberDbgDeviceInfo, NULL, NULL}  };
> 
> +/// An ACPI_PARSER array describing the debug device information
> +structure /// header.
> +STATIC CONST ACPI_PARSER DbgDevInfoHeaderParser[] = {
> +  {L"Revision", 1, 0, L"0x%x", NULL, NULL, NULL, NULL},
> +  {L"Length", 2, 1, L"%d", NULL, (VOID**)&DbgDevInfoLen, NULL, NULL} };
> +
>  /// An ACPI_PARSER array describing the debug device information.
>  STATIC CONST ACPI_PARSER DbgDevInfoParser[] = {
>    {L"Revision", 1, 0, L"0x%x", NULL, NULL, NULL, NULL},
> -  {L"Length", 2, 1, L"%d", NULL, (VOID**)&DbgDevInfoLen, NULL, NULL},
> +  {L"Length", 2, 1, L"%d", NULL, NULL, NULL, NULL},
> 
>    {L"Generic Address Registers Count", 1, 3, L"0x%x", NULL,
>     (VOID**)&GasCount, NULL, NULL},
> @@ -91,108 +98,152 @@ STATIC CONST ACPI_PARSER DbgDevInfoParser[] =
> {
>     (VOID**)&AddrSizeOffset, NULL, NULL}  };
> 
> -/**
> -  This function validates the NameSpace string length.
> -
> -  @param [in] Ptr     Pointer to the start of the buffer.
> -  @param [in] Context Pointer to context specific information e.g. this
> -                      could be a pointer to the ACPI table header.
> -**/
> -STATIC
> -VOID
> -EFIAPI
> -ValidateNameSpaceStrLen (
> -  IN UINT8* Ptr,
> -  IN VOID*  Context
> -  )
> -{
> -  UINT16 NameSpaceStrLen;
> -
> -  NameSpaceStrLen = *(UINT16*)Ptr;
> -
> -  if (NameSpaceStrLen < 2) {
> -    IncrementErrorCount ();
> -    Print (
> -      L"\nERROR: NamespaceString Length = %d. If no Namespace device
> exists,\n"
> -       L"    then NamespaceString[] must contain a period '.'",
> -      NameSpaceStrLen
> -      );
> -  }
> -}
> -
>  /**
>    This function parses the debug device information structure.
> 
> -  @param [in]  Ptr     Pointer to the start of the buffer.
> -  @param [out] Length  Pointer in which the length of the debug
> -                       device information is returned.
> +  @param [in] Ptr     Pointer to the start of the buffer.
> +  @param [in] Length  Length of the debug device information structure.
>  **/
>  STATIC
>  VOID
>  EFIAPI
>  DumpDbgDeviceInfo (
> -  IN  UINT8*  Ptr,
> -  OUT UINT32* Length
> +  IN UINT8* Ptr,
> +  IN UINT16 Length
>    )
>  {
>    UINT16  Index;
> -  UINT8*  DataPtr;
> -  UINT32* AddrSize;
> -
> -  // Parse the debug device info to get the Length
> -  ParseAcpi (
> -    FALSE,
> -    0,
> -    "Debug Device Info",
> -    Ptr,
> -    3,  // Length is 2 bytes starting at offset 1
> -    PARSER_PARAMS (DbgDevInfoParser)
> -    );
> +  UINT16  Offset;
> 
>    ParseAcpi (
>      TRUE,
>      2,
>      "Debug Device Info",
>      Ptr,
> -    *DbgDevInfoLen,
> +    Length,
>      PARSER_PARAMS (DbgDevInfoParser)
>      );
> 
> -  // GAS and Address Size
> +  // Check if the values used to control the parsing logic have been
> + // successfully read.
> +  if ((GasCount == NULL)              ||
> +      (NameSpaceStringLength == NULL) ||
> +      (NameSpaceStringOffset == NULL) ||
> +      (OEMDataLength == NULL)         ||
> +      (OEMDataOffset == NULL)         ||
> +      (BaseAddrRegOffset == NULL)     ||
> +      (AddrSizeOffset == NULL)) {
> +    IncrementErrorCount ();
> +    Print (
> +      L"ERROR: Insufficient Debug Device Information Structure length. " \
> +        L"Length = %d.\n",
> +      Length
> +      );
> +    return;
> +  }
> +
> +  // GAS
>    Index = 0;
> -  DataPtr = Ptr + (*BaseAddrRegOffset);
> -  AddrSize = (UINT32*)(Ptr + (*AddrSizeOffset));
> -  while (Index < (*GasCount)) {
> +  Offset = *BaseAddrRegOffset;
> +  while ((Index++ < *GasCount) &&
> +         (Offset < Length)) {
>      PrintFieldName (4, L"BaseAddressRegister");
> -    DumpGasStruct (DataPtr, 4);
> +    Offset += (UINT16)DumpGasStruct (
> +                        Ptr + Offset,
> +                        4,
> +                        Length - Offset
> +                        );
> +  }
> +
> +  // Cross-check the substructure count with the length of the
> + encapsulating  // buffer  if (GetConsistencyChecking () &&
> +      (Index < *GasCount)) {
> +    IncrementErrorCount ();
> +    Print (
> +      L"ERROR: Invalid GAS count. GasCount = %d. DbgDevInfoLen = %d.\n",
> +      *GasCount,
> +      Length
> +      );
> +    return;
> +  }
> +
> +  // Make sure the array of address sizes corresponding to each GAS fit
> + in the  // Debug Device Information structure  if ((*AddrSizeOffset +
> + (*GasCount * sizeof (UINT32))) > Length) {
> +    IncrementErrorCount ();
> +    Print (
> +      L"ERROR: Invalid GAS count. GasCount = %d. RemainingBufferLength
> = %d. " \
> +        L"Parsing of the Debug Device Information structure aborted.\n",
> +      *GasCount,
> +      Length - *AddrSizeOffset
> +      );
> +    return;
> +  }
> +
> +  // Address Size
> +  Index = 0;
> +  Offset = *AddrSizeOffset;
> +  while ((Index++ < *GasCount) &&
> +         (Offset < Length)) {
>      PrintFieldName (4, L"Address Size");
> -    Print (L"0x%x\n", AddrSize[Index]);
> -    DataPtr += GAS_LENGTH;
> -    Index++;
> +    Print (L"0x%x\n", *((UINT32*)(Ptr + Offset)));
> +    Offset += sizeof (UINT32);

1. Why use sizof (UINT32) to replace GAS_LENGTH? The MACRO make a good view and meaningful.

>    }
> 
>    // NameSpace String
>    Index = 0;
> -  DataPtr = Ptr + (*NameSpaceStringOffset);
> +  Offset = *NameSpaceStringOffset;
>    PrintFieldName (4, L"NameSpace String");
> -  while (Index < (*NameSpaceStringLength)) {
> -    Print (L"%c", DataPtr[Index++]);
> +  while ((Index++ < *NameSpaceStringLength) &&
> +         (Offset < Length)) {
> +    Print (L"%c", *(Ptr + Offset));
> +    Offset++;
>    }
>    Print (L"\n");
> 
> +  // Cross-check the string length with the size of the encapsulating
> + // buffer  if (GetConsistencyChecking () &&
> +      (Index < *NameSpaceStringLength)) {
> +    IncrementErrorCount ();
> +    Print (
> +      L"ERROR: Invalid NameSpaceString length. NameSpaceStringLength = %d.
> " \
> +        L"DbgDevInfoLen = %d.\n",
> +      *NameSpaceStringLength,
> +      Length
> +      );
> +    return;
> +  }
> +
>    // OEM Data
> -  Index = 0;
> -  DataPtr = Ptr + (*OEMDataOffset);
> -  PrintFieldName (4, L"OEM Data");
> -  while (Index < (*OEMDataLength)) {
> -    Print (L"%x ", DataPtr[Index++]);
> -    if ((Index & 7) == 0) {
> -      Print (L"\n%-*s   ", OUTPUT_FIELD_COLUMN_WIDTH, L"");
> +  if (*OEMDataOffset != 0) {
> +    Index = 0;
> +    Offset = *OEMDataOffset;
> +    PrintFieldName (4, L"OEM Data");
> +    while ((Index++ < *OEMDataLength) &&
> +           (Offset < Length)) {
> +      Print (L"%x ", *(Ptr + Offset));
> +      if ((Index & 7) == 0) {
> +        Print (L"\n%-*s   ", OUTPUT_FIELD_COLUMN_WIDTH, L"");
> +      }
> +      Offset++;
>      }
> +    Print (L"\n");
>    }
> -  Print (L"\n");
> 
> -  *Length = *DbgDevInfoLen;
> +  // Cross-check the OEM data length with the size of the encapsulating
> + // buffer  if (GetConsistencyChecking () &&
> +      (Index < *OEMDataLength)) {
> +    IncrementErrorCount ();
> +    Print (
> +      L"ERROR: Invalid OEM Data size. OEMDataLength = %d. " \
> +        L"DbgDevInfoLen = %d.\n",
> +      *OEMDataLength,
> +      Length
> +      );
> +  }
>  }
> 
>  /**
> @@ -217,8 +268,7 @@ ParseAcpiDbg2 (
>    )
>  {
>    UINT32 Offset;
> -  UINT32 DbgDeviceInfoLength;
> -  UINT8* DevInfoPtr;
> +  UINT32 Index;
> 
>    if (!Trace) {
>      return;
> @@ -232,14 +282,74 @@ ParseAcpiDbg2 (
>               AcpiTableLength,
>               PARSER_PARAMS (Dbg2Parser)
>               );
> -  DevInfoPtr = Ptr + Offset;
> 
> -  while (Offset < AcpiTableLength) {
> -    DumpDbgDeviceInfo (
> -      DevInfoPtr,
> -      &DbgDeviceInfoLength
> +  // Check if the values used to control the parsing logic have been
> + // successfully read.
> +  if ((OffsetDbgDeviceInfo == NULL) ||
> +      (NumberDbgDeviceInfo == NULL)) {
> +    IncrementErrorCount ();
> +    Print (
> +      L"ERROR: Insufficient table length. AcpiTableLength = %d\n",
> +      AcpiTableLength
> +      );
> +    return;
> +  }
> +
> +  Offset = *OffsetDbgDeviceInfo;
> +  Index = 0;
> +
> +  while (Index++ < *NumberDbgDeviceInfo) {
> +
> +    // Parse the Debug Device Information Structure header to obtain Length
> +    ParseAcpi (
> +      FALSE,
> +      0,
> +      NULL,
> +      Ptr + Offset,
> +      AcpiTableLength - Offset,
> +      PARSER_PARAMS (DbgDevInfoHeaderParser)
> +      );

2. Here adding the DbgDevInfoHeaderParser to get the DbgDevInfoLen. This may be conflict with 4/10. 4/10 removes the get header section. I think it is fine to get the whole devinfo if the Length is valid. And the DbgDevInfoHeaderParser isn't required to add.

Thanks,
Zhichao

> +
> +    // Check if the values used to control the parsing logic have been
> +    // successfully read.
> +    if (DbgDevInfoLen == NULL) {
> +      IncrementErrorCount ();
> +      Print (
> +        L"ERROR: Insufficient remaining table buffer length to read the " \
> +          L"Debug Device Information structure's 'Length' field. " \
> +          L"RemainingTableBufferLength = %d.\n",
> +        AcpiTableLength - Offset
> +        );
> +      return;
> +    }
> +
> +    // Make sure the Debug Device Information structure lies inside the table.
> +    if ((Offset + *DbgDevInfoLen) > AcpiTableLength) {
> +      IncrementErrorCount ();
> +      Print (
> +        L"ERROR: Invalid Debug Device Information structure length. " \
> +          L"DbgDevInfoLen = %d. RemainingTableBufferLength = %d. " \
> +          L"DBG2 parsing aborted.\n",
> +        *DbgDevInfoLen,
> +        AcpiTableLength - Offset
> +        );
> +      return;
> +    }
> +
> +    DumpDbgDeviceInfo (Ptr + Offset, (*DbgDevInfoLen));
> +    Offset += (*DbgDevInfoLen);
> +  }
> +
> +  // Cross-check the substructure count with the length of the
> + encapsulating  // buffer  if (GetConsistencyChecking () &&
> +      (Index < *NumberDbgDeviceInfo)) {
> +    IncrementErrorCount ();
> +    Print (
> +      L"ERROR: Invalid Debug Device Information structure count. " \
> +        L"NumberDbgDeviceInfo = %d. AcpiTableLength = %d.\n",
> +      *NumberDbgDeviceInfo,
> +      AcpiTableLength
>        );
> -    Offset += DbgDeviceInfoLength;
> -    DevInfoPtr += DbgDeviceInfoLength;
>    }
>  }
> --
> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
>