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

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

I got your point. How about this: https://github.com/ZhichaoGao/edk2/commit/112a41255cb775f5ebede089b8b07ba7b836ec44
I make a minor change of it. But I can't test it because I don't have a platform that implement DBG2 table.

Thanks,
Zhichao

> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Krzysztof Koch
> Sent: Monday, August 5, 2019 4:21 PM
> To: Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io
> 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 1/6] ShellPkg: acpiview: DBG2: Prevent
> buffer overruns
> 
> Hi Zhichao,
> 
> The reason why processing of the Debug Device Information Structure is split
> into:
> 1. loading the header
> 2. dumping the entire structure
> 
> Is because we want to let the users control how much of the structure is
> dumped. This is important for backward compatibility of the acpiview tool
> with the ACPI specification (and other specs).
> 
> New ACPI table fields are appended at the end of structures/tables. If, for
> example, we are asked to parse an old version of Debug Device Information
> Structure, the 'Length' field will tell us to ignore some of the newly added
> fields. These fields do not make sense in the context of an old version of the
> corresponding spec.
> 
> The following code in Dbg2Parser.c:
> 
>     // 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;
>     }
> 
> Makes sure that the user-provided structure length won't result in a buffer
> overrun with respect to the DBG2 table buffer. This way we allow users to
> specify how much of the structure they want to parse while still preventing
> buffer overruns.
> 
> In short, I'm not sure if getting rid of DbgDevInfoHeaderParser would work as
> you assume that the remaining table buffer length should be passed to
> ParseAcpi() as an argument, not the length of the Debug Device Information
> Structure. What do you think?
> 
> Kind regards,
> 
> Krzysztof
> 
> -----Original Message-----
> From: Gao, Zhichao <zhichao.gao@intel.com>
> Sent: Monday, August 5, 2019 7:48
> To: Krzysztof Koch <Krzysztof.Koch@arm.com>; devel@edk2.groups.io
> 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: [PATCH v1 1/6] ShellPkg: acpiview: DBG2: Prevent buffer
> overruns
> 
> About DbgDevInfoHeaderParser and DbgDevInfoParser.
> This patch would parse same DbgDevInfo twice, one for getting length, the
> other for dumping structure info.
> How about the following?
> Add one parameter for DumpDbgDeviceInfo
> 
> STATIC
> VOID
> EFIAPI
> DumpDbgDeviceInfo (
>   IN  UINT8*  Ptr,
>   OUT UINT32* Length
>   )
> 
> ==>
> 
> STATIC
> VOID
> EFIAPI
> DumpDbgDeviceInfo (
>   IN  UINT8*  Ptr,
>   IN  UINT32* Length			// remain length of acpi struct to
> parse to make sure all operation is in a valid scope
>   OUT UINT16* DbgDevInfoLength	// return pointer dbgdevinfo length
>   )
> 
> Then we would not need an anditional DbgDevInfoHeaderParser and the
> header would be parsed for only once.
> 
> Any better comments, please let me know.
> 
> Thanks,
> Zhichao
> 
> > -----Original Message-----
> > From: Krzysztof Koch [mailto:krzysztof.koch@arm.com]
> > 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: [PATCH v1 1/6] ShellPkg: acpiview: DBG2: Prevent buffer
> > overruns
> >
> > Modify the DBG2 table parsing logic to prevent reading past the ACPI
> > buffer lengths provided.
> >
> > Modify the signature of the DumpDbgDeviceInfo() function to make it
> > consistent with the ACPI structure processing functions in other
> > acpiview parsers. Now, the length of the Debug Device Information
> > Structure is read before the entire structure is dumped.
> >
> > This refactoring change makes it easier to stop reading beyond the
> > DBG2 table buffer if the Debug Device Information Structure Buffer
> > does not fit in the DBG2 buffer.
> >
> > For processing the first two fields of the Debug Device Information
> > Structure (to get the length) a new ACPI_PARSER array is defined.
> >
> > References:
> > - Microsoft Debug Port Table 2 (DBG2), December 10, 2015
> >
> > Signed-off-by: Krzysztof Koch <krzysztof.koch@arm.com>
> > ---
> >
> > Notes:
> >     v1:
> >     - Prevent buffer overruns in DBG2 acpiview parser [Krzysztof]
> >
> >
> >
> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c
> > | 141 +++++++++++++-------
> >  1 file changed, 92 insertions(+), 49 deletions(-)
> >
> > diff --git
> >
> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parse
> > r.c
> >
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parse
> > r.c
> > index
> >
> c6929695a1032c57761ef85002d6c51b7800ce23..869e700b9beda4886bf7bc5ae
> > 4ced3ab9a59efa3 100644
> > ---
> >
> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parse
> > r.c
> > +++
> >
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Pars
> > +++ er.c
> > @@ -64,10 +64,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},
> > @@ -93,76 +100,91 @@ STATIC CONST ACPI_PARSER DbgDevInfoParser[] =
> {
> >  /**
> >    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
> > +  // 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, GAS_LENGTH);
> > +    Offset += (UINT16)DumpGasStruct (
> > +                        Ptr + Offset,
> > +                        4,
> > +                        Length - Offset
> > +                        );
> > +  }
> > +
> > +  // 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);
> >    }
> >
> >    // 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");
> >
> >    // 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;
> >  }
> >
> >  /**
> > @@ -187,8 +209,7 @@ ParseAcpiDbg2 (
> >    )
> >  {
> >    UINT32 Offset;
> > -  UINT32 DbgDeviceInfoLength;
> > -  UINT8* DevInfoPtr;
> > +  UINT32 Index;
> >
> >    if (!Trace) {
> >      return;
> > @@ -202,14 +223,36 @@ ParseAcpiDbg2 (
> >               AcpiTableLength,
> >               PARSER_PARAMS (Dbg2Parser)
> >               );
> > -  DevInfoPtr = Ptr + Offset;
> >
> > -  while (Offset < AcpiTableLength) {
> > -    DumpDbgDeviceInfo (
> > -      DevInfoPtr,
> > -      &DbgDeviceInfoLength
> > +  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)
> >        );
> > -    Offset += DbgDeviceInfoLength;
> > -    DevInfoPtr += DbgDeviceInfoLength;
> > +
> > +    // 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);
> >    }
> >  }
> > --
> > 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
> >
> 
> 
>