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

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

OK. I got your point. 
1. The length of debug device info structure indicate the correct length should be parsed. I'd prefer to see this as a backward compatibility as you mentioned. See below:
The ACPI table may be updated to extend some of the table. For example, if DBG2->Debug Device Info structure is enlarged (add new field after AddressSizeOffset), we would update the DbgDevInforParser and that would cause an incorrect parsing of old version Debug device info structure. So it is required to get the length of the structure first. I am not sure if it is required for the acpi table header.
2. As you mentioned, using length to parse a structure may cause some variable not updated. I didn't consider that before. That's good point. If it is not updated,  it would use its default value or the previous value that initialize thru the previous ParseAcpi. So I think it is required to get parsed value (return value of ParseAcpi) to judge if the variable is valid.

For this patch, I have no comments. Reviewed-by: Zhichao Gao <zhichao.gao@inte.com>

Thanks,
Zhichao

> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Krzysztof Koch
> Sent: Tuesday, August 6, 2019 6:45 PM
> To: devel@edk2.groups.io; Gao, Zhichao <zhichao.gao@intel.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 1/6] ShellPkg: acpiview: DBG2: Prevent
> buffer overruns
> 
> Hi Zhichao,
> 
> Thanks for the feedback. I had a look at your code and I have put comments
> inline (in GitHub) to describe why I think it does not achieve the same
> functionality as the patch I've submitted.
> 
> I copied my comments below as well, so that they're easier to access:
> 
> """
> The ParseAcpi() call (Line 113) will parse either the entire DbgDevInfoParser[]
> array or as much data as there is left in the ACPI table buffer. I agree this
> prevents buffer overruns with respect to the ACPI table buffer. However,
> the parser now ignores the length of the Debug Device Information Structure
> (loaded into the *DbgDevInfoLen variable) when dumping its contents.
> 
> Here is an example:
> If the DBG2 table buffer is 100-byte long, and the Debug Device Information
> Structure is (let's say) located at offset 20 with a byte-size (as described in
> the 'Length' field) of only 10 bytes, then we have a problem.
> 
> The DbgDevInfoParser[] array says that 22 bytes should be parsed, however,
> the user-provided structure length is 10. I believe that only 10 bytes should
> be parsed to reflect what an OS would do in this situation.
> 
> This is why I created a new ACPI_PARSER array in my submitted patch to:
> 1. Read the Length of the Debug Device Information Structure 2. Validate the
> Length against the length of the DBG2 table buffer 3. Use the Length to
> control how many statements from DbgDevInfoParser[] should be executed.
> 
> If we print only as much data as the ACPI table writer has specified then any
> errors in the 'Length' field are easier to detect. You can easily see that some
> data is missing and this is due to the 'Length' field having wrong value.
> 
> Reading the 'Length' field before the whole structure is dumped is also
> important for our acpiview implementation for the sake of backward
> compatibility. As ACPI tables usually get updated by appending new fields to
> existing structures. If someone provides us with a Length that matches the
> old DBG2 version then we won't print the fields that got recently added to
> DbgDevInfoParser[] due to a spec update.
> 
> I understand there is still an issue of some variables not getting updated
> correctly because we haven't parsed enough of the DbgDevInfoParser[], for
> example, the AddrSizeOffset variable. But my next patch series adds code to
> detect NULL pointers in all parsers.
> """
> 
> Please let me know what you think.
> 
> Kind regards,
> 
> Krzysztof
> 
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Gao,
> Zhichao via Groups.Io
> Sent: Tuesday, August 6, 2019 8:43
> 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 1/6] ShellPkg: acpiview: DBG2: Prevent
> buffer overruns
> 
> I got your point. How about this:
> https://github.com/ZhichaoGao/edk2/commit/112a41255cb775f5ebede089b
> 8b07ba7b836ec44
> 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)'
> > >
> >
> >
> >
> 
> 
> 
> 
> 
>