[edk2-devel] [PATCH v1 11/11] ShellPkg: acpiview: DBG2: Add error-checking in the parsing logic
Krzysztof Koch
krzysztof.koch at arm.com
Fri Jul 12 06:52:43 UTC 2019
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 at arm.com>
---
Changes can be seen at: https://github.com/KrzysztofKoch1/edk2/commit/530b059a9fe4aa9f1df36b407f97d76acaab8b74
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..2d6ff80e299eebe7853061d3db89332197c0dc0e 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..20ca358bddfa5953bfb1d1bebaebbf3079eaba01 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/Dbg2Parser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c
index 8de5ebf74775bab8e765849cba6ef4eb6f659a5a..2bbd622ffb7cec0a340de3e10bdcd01ba4d330df 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.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);
}
// 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)
+ );
+
+ // 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)'
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#43651): https://edk2.groups.io/g/devel/message/43651
Mute This Topic: https://groups.io/mt/32439516/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list