Skip to content

Commit

Permalink
ShellPkg: Acpiview: Remove DbgDevInfoHeader
Browse files Browse the repository at this point in the history 
Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
  • Loading branch information
@ZhichaoGao
ZhichaoGao committed Aug 6, 2019
1 parent c54700f commit 112a412
Showing 1 changed file with 36 additions and 39 deletions.
75 changes: 36 additions & 39 deletions ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,17 +64,10 @@ 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, NULL, NULL, NULL},
{L"Length", 2, 1, L"%d", NULL, (VOID**)&DbgDevInfoLen, NULL, NULL},

{L"Generic Address Registers Count", 1, 3, L"0x%x", NULL,
(VOID**)&GasCount, NULL, NULL},
Expand All @@ -100,28 +93,39 @@ STATIC CONST ACPI_PARSER DbgDevInfoParser[] = {
/**
This function parses the debug device information structure.
@param [in] Ptr Pointer to the start of the buffer.
@param [in] Length Length of the debug device information structure.
@param [in] Ptr Pointer to the start of the buffer.
@param [in] Length The remain length of the acpi table. i.e. AcpiLength - (Ptr - AcpiHeaderPtr).
@param [out] DbgDevInfoLen Return the successful parsed Debug Device Information length.
**/
STATIC
VOID
EFIAPI
DumpDbgDeviceInfo (
IN UINT8* Ptr,
IN UINT16 Length
IN UINT8* Ptr,
IN UINT32 Length,
OUT UINT16* DbgDevInfoLength
)
{
UINT16 Index;
UINT16 Offset;
UINT32 ParsedOffset;

ParsedOffset = ParseAcpi (
TRUE,
2,
"Debug Device Info",
Ptr,
Length,
PARSER_PARAMS (DbgDevInfoParser)
);

This comment has been minimized.

Sign in to view

Copy link
@KrzysztofKoch1

KrzysztofKoch1 Aug 6, 2019

The ParseAcpi() call above 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 it's 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 first read the Length of the Debug Device Information Structure, validate it against the length of the DBG2 table buffer, and then 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 cas 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 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, AddrSizeOffset. But my next patch series adds code to detect NULL pointers in all parsers.

if (DbgDevInfoLength != NULL) {
*DbgDevInfoLength = (UINT16)(ParsedOffset == 0 ? 0 : *DbgDevInfoLen);
}

ParseAcpi (
TRUE,
2,
"Debug Device Info",
Ptr,
Length,
PARSER_PARAMS (DbgDevInfoParser)
);
if (ParsedOffset == 0) {
return;
}

// GAS
Index = 0;
Expand Down Expand Up @@ -210,6 +214,7 @@ ParseAcpiDbg2 (
{
UINT32 Offset;
UINT32 Index;
UINT16 DebugDevInfoLen;

if (!Trace) {
return;
Expand All @@ -229,30 +234,22 @@ ParseAcpiDbg2 (

while (Index++ < *NumberDbgDeviceInfo) {

// Parse the Debug Device Information Structure header to obtain Length
ParseAcpi (
FALSE,
0,
NULL,
Ptr + Offset,
AcpiTableLength - Offset,
PARSER_PARAMS (DbgDevInfoHeaderParser)
);

// Make sure the Debug Device Information structure lies inside the table.
if ((Offset + *DbgDevInfoLen) > AcpiTableLength) {
if (AcpiTableLength < Offset) {
IncrementErrorCount ();
Print (
L"ERROR: Invalid Debug Device Information structure length. " \
L"DbgDevInfoLen = %d. RemainingTableBufferLength = %d. " \
L"DBG2 parsing aborted.\n",
*DbgDevInfoLen,
AcpiTableLength - Offset
L"ERROR: Invalid Offset of ACPI DBG2 table. " \
L"AcpiTableLength: %d, Offset: %d. " \
L"DEB2 parsing aborted.\n",
AcpiTableLength,
Offset
);
return;
}

DumpDbgDeviceInfo (Ptr + Offset, (*DbgDevInfoLen));
Offset += (*DbgDevInfoLen);
DumpDbgDeviceInfo (Ptr + Offset, AcpiTableLength - Offset, &DebugDevInfoLen);
if (DebugDevInfoLen == 0) {
return;
}
Offset += DebugDevInfoLen;
}
}

0 comments on commit 112a412

Please sign in to comment.