* [PATCH v1 1/6] ShellPkg: acpiview: DBG2: Prevent buffer overruns
2019-08-01 8:44 [PATCH v1 0/6] Prevent buffer overruns in acpiview table parsers Krzysztof Koch
@ 2019-08-01 8:44 ` Krzysztof Koch
2019-08-01 9:33 ` [edk2-devel] " Alexei Fedorov
2019-08-05 6:48 ` Gao, Zhichao
2019-08-01 8:44 ` [PATCH v1 2/6] ShellPkg: acpiview: GTDT: " Krzysztof Koch
` (7 subsequent siblings)
8 siblings, 2 replies; 24+ messages in thread
From: Krzysztof Koch @ 2019-08-01 8:44 UTC (permalink / raw)
To: devel; +Cc: jaben.carsey, ray.ni, zhichao.gao, Sami.Mujawar, Matteo.Carlini,
nd
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/Dbg2Parser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c
index c6929695a1032c57761ef85002d6c51b7800ce23..869e700b9beda4886bf7bc5ae4ced3ab9a59efa3 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.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)'
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [edk2-devel] [PATCH v1 1/6] ShellPkg: acpiview: DBG2: Prevent buffer overruns
2019-08-01 8:44 ` [PATCH v1 1/6] ShellPkg: acpiview: DBG2: Prevent buffer overruns Krzysztof Koch
@ 2019-08-01 9:33 ` Alexei Fedorov
2019-08-05 6:48 ` Gao, Zhichao
1 sibling, 0 replies; 24+ messages in thread
From: Alexei Fedorov @ 2019-08-01 9:33 UTC (permalink / raw)
To: Krzysztof Koch, devel
[-- Attachment #1: Type: text/plain, Size: 54 bytes --]
Reviewed-by: Alexei Fedorov <Alexei.Fedorov@arm.com>
[-- Attachment #2: Type: text/html, Size: 60 bytes --]
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v1 1/6] ShellPkg: acpiview: DBG2: Prevent buffer overruns
2019-08-01 8:44 ` [PATCH v1 1/6] ShellPkg: acpiview: DBG2: Prevent buffer overruns Krzysztof Koch
2019-08-01 9:33 ` [edk2-devel] " Alexei Fedorov
@ 2019-08-05 6:48 ` Gao, Zhichao
2019-08-05 8:21 ` Krzysztof Koch
1 sibling, 1 reply; 24+ messages in thread
From: Gao, Zhichao @ 2019-08-05 6:48 UTC (permalink / raw)
To: Krzysztof Koch, devel@edk2.groups.io
Cc: Carsey, Jaben, Ni, Ray, Sami.Mujawar@arm.com,
Matteo.Carlini@arm.com, nd@arm.com
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)'
>
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v1 1/6] ShellPkg: acpiview: DBG2: Prevent buffer overruns
2019-08-05 6:48 ` Gao, Zhichao
@ 2019-08-05 8:21 ` Krzysztof Koch
2019-08-06 7:43 ` [edk2-devel] " Gao, Zhichao
0 siblings, 1 reply; 24+ messages in thread
From: Krzysztof Koch @ 2019-08-05 8:21 UTC (permalink / raw)
To: Gao, Zhichao, devel@edk2.groups.io
Cc: Carsey, Jaben, Ni, Ray, Sami Mujawar, Matteo Carlini, nd
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)'
>
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [edk2-devel] [PATCH v1 1/6] ShellPkg: acpiview: DBG2: Prevent buffer overruns
2019-08-05 8:21 ` Krzysztof Koch
@ 2019-08-06 7:43 ` Gao, Zhichao
2019-08-06 10:44 ` Krzysztof Koch
0 siblings, 1 reply; 24+ messages in thread
From: Gao, Zhichao @ 2019-08-06 7:43 UTC (permalink / raw)
To: devel@edk2.groups.io, krzysztof.koch@arm.com
Cc: Carsey, Jaben, Ni, Ray, Sami Mujawar, Matteo Carlini, nd
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)'
> >
>
>
>
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [edk2-devel] [PATCH v1 1/6] ShellPkg: acpiview: DBG2: Prevent buffer overruns
2019-08-06 7:43 ` [edk2-devel] " Gao, Zhichao
@ 2019-08-06 10:44 ` Krzysztof Koch
2019-08-07 1:52 ` Gao, Zhichao
0 siblings, 1 reply; 24+ messages in thread
From: Krzysztof Koch @ 2019-08-06 10:44 UTC (permalink / raw)
To: devel@edk2.groups.io, zhichao.gao@intel.com
Cc: Carsey, Jaben, Ni, Ray, Sami Mujawar, Matteo Carlini, nd
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/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)'
> >
>
>
>
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [edk2-devel] [PATCH v1 1/6] ShellPkg: acpiview: DBG2: Prevent buffer overruns
2019-08-06 10:44 ` Krzysztof Koch
@ 2019-08-07 1:52 ` Gao, Zhichao
0 siblings, 0 replies; 24+ messages in thread
From: Gao, Zhichao @ 2019-08-07 1:52 UTC (permalink / raw)
To: devel@edk2.groups.io, krzysztof.koch@arm.com
Cc: Carsey, Jaben, Ni, Ray, Sami Mujawar, Matteo Carlini, nd
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)'
> > >
> >
> >
> >
>
>
>
>
>
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v1 2/6] ShellPkg: acpiview: GTDT: Prevent buffer overruns
2019-08-01 8:44 [PATCH v1 0/6] Prevent buffer overruns in acpiview table parsers Krzysztof Koch
2019-08-01 8:44 ` [PATCH v1 1/6] ShellPkg: acpiview: DBG2: Prevent buffer overruns Krzysztof Koch
@ 2019-08-01 8:44 ` Krzysztof Koch
2019-08-01 9:33 ` [edk2-devel] " Alexei Fedorov
2019-08-05 7:23 ` Gao, Zhichao
2019-08-01 8:44 ` [PATCH v1 3/6] ShellPkg: acpiview: IORT: " Krzysztof Koch
` (6 subsequent siblings)
8 siblings, 2 replies; 24+ messages in thread
From: Krzysztof Koch @ 2019-08-01 8:44 UTC (permalink / raw)
To: devel; +Cc: jaben.carsey, ray.ni, zhichao.gao, Sami.Mujawar, Matteo.Carlini,
nd
Modify the GTDT table parsing logic to prevent reading past the ACPI
buffer lengths provided and to make it consistent with other table
parsers. This includes converting the do-while loop in ParseAcpiGtdt()
into a while loop.
Remove a check which ensures that the entire Platform GT Block
Structure buffer has been parsed. The ACPI specification does not ban
from defining buffers which are larger than the size indicated by the
count and sizes of substructures which constitute it.
Change the data type of the Length parameter to the DumpGTBlock()
function to reflect the width of the respective ACPI structure's
field.
References:
- ACPI 6.3, January 2019, Table 5-124
Signed-off-by: Krzysztof Koch <krzysztof.koch@arm.com>
---
Notes:
v1:
- Prevent buffer overruns in GTDT acpiview parser [Krzysztof]
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser.c | 147 ++++++++++----------
1 file changed, 76 insertions(+), 71 deletions(-)
diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser.c
index 1e5b5764f50a2d29aa904c889bc89af5bdc3af5c..57174e14c80072f12b90e1996ebe8f0002d0c404 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser.c
@@ -23,7 +23,6 @@ STATIC CONST UINT8* PlatformTimerType;
STATIC CONST UINT16* PlatformTimerLength;
STATIC CONST UINT32* GtBlockTimerCount;
STATIC CONST UINT32* GtBlockTimerOffset;
-STATIC CONST UINT16* GtBlockLength;
STATIC ACPI_DESCRIPTION_HEADER_INFO AcpiHdrInfo;
/**
@@ -127,7 +126,7 @@ STATIC CONST ACPI_PARSER GtPlatformTimerHeaderParser[] = {
**/
STATIC CONST ACPI_PARSER GtBlockParser[] = {
{L"Type", 1, 0, L"%d", NULL, NULL, NULL, NULL},
- {L"Length", 2, 1, L"%d", NULL, (VOID**)&GtBlockLength, NULL, NULL},
+ {L"Length", 2, 1, L"%d", NULL, NULL, NULL, NULL},
{L"Reserved", 1, 3, L"%x", NULL, NULL, NULL, NULL},
{L"Physical address (CntCtlBase)", 8, 4, L"0x%lx", NULL, NULL, NULL, NULL},
{L"Timer Count", 4, 12, L"%d", NULL, (VOID**)&GtBlockTimerCount,
@@ -168,56 +167,43 @@ STATIC CONST ACPI_PARSER SBSAGenericWatchdogParser[] = {
/**
This function parses the Platform GT Block.
- @param [in] Ptr Pointer to the start of the GT Block data.
- @param [in] Length Length of the GT Block structure.
+ @param [in] Ptr Pointer to the start of the GT Block data.
+ @param [in] Length Length of the GT Block structure.
**/
STATIC
VOID
DumpGTBlock (
IN UINT8* Ptr,
- IN UINT32 Length
+ IN UINT16 Length
)
{
UINT32 Index;
UINT32 Offset;
- UINT32 GTBlockTimerLength;
- Offset = ParseAcpi (
- TRUE,
- 2,
- "GT Block",
- Ptr,
- Length,
- PARSER_PARAMS (GtBlockParser)
- );
- GTBlockTimerLength = (*GtBlockLength - Offset) / (*GtBlockTimerCount);
- Length -= Offset;
+ ParseAcpi (
+ TRUE,
+ 2,
+ "GT Block",
+ Ptr,
+ Length,
+ PARSER_PARAMS (GtBlockParser)
+ );
- if (*GtBlockTimerCount != 0) {
- Ptr += (*GtBlockTimerOffset);
- Index = 0;
- while ((Index < (*GtBlockTimerCount)) && (Length >= GTBlockTimerLength)) {
- Offset = ParseAcpi (
- TRUE,
- 2,
- "GT Block Timer",
- Ptr,
- GTBlockTimerLength,
- PARSER_PARAMS (GtBlockTimerParser)
- );
- // Increment by GT Block Timer structure size
- Ptr += Offset;
- Length -= Offset;
- Index++;
- }
+ Offset = *GtBlockTimerOffset;
+ Index = 0;
- if (Length != 0) {
- IncrementErrorCount ();
- Print (
- L"ERROR:GT Block Timer length mismatch. Unparsed %d bytes.\n",
- Length
- );
- }
+ // Parse the specified number of GT Block Timer Structures or the GT Block
+ // Structure buffer length. Whichever is minimum.
+ while ((Index++ < *GtBlockTimerCount) &&
+ (Offset < Length)) {
+ Offset += ParseAcpi (
+ TRUE,
+ 2,
+ "GT Block Timer",
+ Ptr + Offset,
+ Length - Offset,
+ PARSER_PARAMS (GtBlockTimerParser)
+ );
}
}
@@ -270,6 +256,7 @@ ParseAcpiGtdt (
)
{
UINT32 Index;
+ UINT32 Offset;
UINT8* TimerPtr;
if (!Trace) {
@@ -285,36 +272,54 @@ ParseAcpiGtdt (
PARSER_PARAMS (GtdtParser)
);
- if (*GtdtPlatformTimerCount != 0) {
- TimerPtr = Ptr + (*GtdtPlatformTimerOffset);
- Index = 0;
- do {
- // Parse the Platform Timer Header
- ParseAcpi (
- FALSE,
- 0,
- NULL,
- TimerPtr,
- 4, // GT Platform Timer structure header length.
- PARSER_PARAMS (GtPlatformTimerHeaderParser)
+ TimerPtr = Ptr + *GtdtPlatformTimerOffset;
+ Offset = *GtdtPlatformTimerOffset;
+ Index = 0;
+
+ // Parse the specified number of Platform Timer Structures or the GTDT
+ // buffer length. Whichever is minimum.
+ while ((Index++ < *GtdtPlatformTimerCount) &&
+ (Offset < AcpiTableLength)) {
+ // Parse the Platform Timer Header to obtain Length and Type
+ ParseAcpi (
+ FALSE,
+ 0,
+ NULL,
+ TimerPtr,
+ AcpiTableLength - Offset,
+ PARSER_PARAMS (GtPlatformTimerHeaderParser)
+ );
+
+ // Make sure the Platform Timer is inside the table.
+ if ((Offset + *PlatformTimerLength) > AcpiTableLength) {
+ IncrementErrorCount ();
+ Print (
+ L"ERROR: Invalid Platform Timer Structure length. " \
+ L"PlatformTimerLength = %d. RemainingTableBufferLength = %d. " \
+ L"GTDT parsing aborted.\n",
+ *PlatformTimerLength,
+ AcpiTableLength - Offset
);
- switch (*PlatformTimerType) {
- case EFI_ACPI_6_2_GTDT_GT_BLOCK:
- DumpGTBlock (TimerPtr, *PlatformTimerLength);
- break;
- case EFI_ACPI_6_2_GTDT_SBSA_GENERIC_WATCHDOG:
- DumpWatchdogTimer (TimerPtr, *PlatformTimerLength);
- break;
- default:
- IncrementErrorCount ();
- Print (
- L"ERROR: INVALID Platform Timer Type = %d\n",
- *PlatformTimerType
- );
- break;
- } // switch
- TimerPtr += (*PlatformTimerLength);
- Index++;
- } while (Index < *GtdtPlatformTimerCount);
- }
+ return;
+ }
+
+ switch (*PlatformTimerType) {
+ case EFI_ACPI_6_3_GTDT_GT_BLOCK:
+ DumpGTBlock (TimerPtr, *PlatformTimerLength);
+ break;
+ case EFI_ACPI_6_3_GTDT_SBSA_GENERIC_WATCHDOG:
+ DumpWatchdogTimer (TimerPtr, *PlatformTimerLength);
+ break;
+ default:
+ IncrementErrorCount ();
+ Print (
+ L"ERROR: Invalid Platform Timer Type = %d\n",
+ *PlatformTimerType
+ );
+ break;
+ } // switch
+
+ TimerPtr += *PlatformTimerLength;
+ Offset += *PlatformTimerLength;
+ } // while
}
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [edk2-devel] [PATCH v1 2/6] ShellPkg: acpiview: GTDT: Prevent buffer overruns
2019-08-01 8:44 ` [PATCH v1 2/6] ShellPkg: acpiview: GTDT: " Krzysztof Koch
@ 2019-08-01 9:33 ` Alexei Fedorov
2019-08-05 7:23 ` Gao, Zhichao
1 sibling, 0 replies; 24+ messages in thread
From: Alexei Fedorov @ 2019-08-01 9:33 UTC (permalink / raw)
To: Krzysztof Koch, devel
[-- Attachment #1: Type: text/plain, Size: 54 bytes --]
Reviewed-by: Alexei Fedorov <Alexei.Fedorov@arm.com>
[-- Attachment #2: Type: text/html, Size: 60 bytes --]
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [edk2-devel] [PATCH v1 2/6] ShellPkg: acpiview: GTDT: Prevent buffer overruns
2019-08-01 8:44 ` [PATCH v1 2/6] ShellPkg: acpiview: GTDT: " Krzysztof Koch
2019-08-01 9:33 ` [edk2-devel] " Alexei Fedorov
@ 2019-08-05 7:23 ` Gao, Zhichao
2019-08-05 9:54 ` Sami Mujawar
1 sibling, 1 reply; 24+ messages in thread
From: Gao, Zhichao @ 2019-08-05 7:23 UTC (permalink / raw)
To: devel@edk2.groups.io, krzysztof.koch@arm.com
Cc: Carsey, Jaben, Ni, Ray, Sami.Mujawar@arm.com,
Matteo.Carlini@arm.com, nd@arm.com
One confusion below:
> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Krzysztof Koch
> 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: [edk2-devel] [PATCH v1 2/6] ShellPkg: acpiview: GTDT: Prevent
> buffer overruns
>
> Modify the GTDT table parsing logic to prevent reading past the ACPI buffer
> lengths provided and to make it consistent with other table parsers. This
> includes converting the do-while loop in ParseAcpiGtdt() into a while loop.
>
> Remove a check which ensures that the entire Platform GT Block Structure
> buffer has been parsed. The ACPI specification does not ban from defining
> buffers which are larger than the size indicated by the count and sizes of
> substructures which constitute it.
>
> Change the data type of the Length parameter to the DumpGTBlock()
> function to reflect the width of the respective ACPI structure's field.
>
> References:
> - ACPI 6.3, January 2019, Table 5-124
>
> Signed-off-by: Krzysztof Koch <krzysztof.koch@arm.com>
> ---
>
> Notes:
> v1:
> - Prevent buffer overruns in GTDT acpiview parser [Krzysztof]
>
> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser.c
> | 147 ++++++++++----------
> 1 file changed, 76 insertions(+), 71 deletions(-)
>
> diff --git
> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser
> .c
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser
> .c
> index
> 1e5b5764f50a2d29aa904c889bc89af5bdc3af5c..57174e14c80072f12b90e1996e
> be8f0002d0c404 100644
> ---
> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser
> .c
> +++
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtPars
> +++ er.c
> @@ -23,7 +23,6 @@ STATIC CONST UINT8* PlatformTimerType; STATIC
> CONST UINT16* PlatformTimerLength; STATIC CONST UINT32*
> GtBlockTimerCount; STATIC CONST UINT32* GtBlockTimerOffset; -STATIC
> CONST UINT16* GtBlockLength; STATIC ACPI_DESCRIPTION_HEADER_INFO
> AcpiHdrInfo;
>
> /**
> @@ -127,7 +126,7 @@ STATIC CONST ACPI_PARSER
> GtPlatformTimerHeaderParser[] = { **/ STATIC CONST ACPI_PARSER
> GtBlockParser[] = {
> {L"Type", 1, 0, L"%d", NULL, NULL, NULL, NULL},
> - {L"Length", 2, 1, L"%d", NULL, (VOID**)&GtBlockLength, NULL, NULL},
> + {L"Length", 2, 1, L"%d", NULL, NULL, NULL, NULL},
> {L"Reserved", 1, 3, L"%x", NULL, NULL, NULL, NULL},
> {L"Physical address (CntCtlBase)", 8, 4, L"0x%lx", NULL, NULL, NULL, NULL},
> {L"Timer Count", 4, 12, L"%d", NULL, (VOID**)&GtBlockTimerCount, @@ -
> 168,56 +167,43 @@ STATIC CONST ACPI_PARSER
> SBSAGenericWatchdogParser[] = {
> /**
> This function parses the Platform GT Block.
>
> - @param [in] Ptr Pointer to the start of the GT Block data.
> - @param [in] Length Length of the GT Block structure.
> + @param [in] Ptr Pointer to the start of the GT Block data.
> + @param [in] Length Length of the GT Block structure.
> **/
> STATIC
> VOID
> DumpGTBlock (
> IN UINT8* Ptr,
> - IN UINT32 Length
> + IN UINT16 Length
> )
> {
> UINT32 Index;
> UINT32 Offset;
> - UINT32 GTBlockTimerLength;
>
> - Offset = ParseAcpi (
> - TRUE,
> - 2,
> - "GT Block",
> - Ptr,
> - Length,
> - PARSER_PARAMS (GtBlockParser)
> - );
> - GTBlockTimerLength = (*GtBlockLength - Offset) / (*GtBlockTimerCount);
> - Length -= Offset;
> + ParseAcpi (
> + TRUE,
> + 2,
> + "GT Block",
> + Ptr,
> + Length,
> + PARSER_PARAMS (GtBlockParser)
> + );
>
> - if (*GtBlockTimerCount != 0) {
> - Ptr += (*GtBlockTimerOffset);
> - Index = 0;
> - while ((Index < (*GtBlockTimerCount)) && (Length >=
> GTBlockTimerLength)) {
> - Offset = ParseAcpi (
> - TRUE,
> - 2,
> - "GT Block Timer",
> - Ptr,
> - GTBlockTimerLength,
> - PARSER_PARAMS (GtBlockTimerParser)
> - );
> - // Increment by GT Block Timer structure size
> - Ptr += Offset;
> - Length -= Offset;
> - Index++;
> - }
> + Offset = *GtBlockTimerOffset;
> + Index = 0;
>
> - if (Length != 0) {
> - IncrementErrorCount ();
> - Print (
> - L"ERROR:GT Block Timer length mismatch. Unparsed %d bytes.\n",
> - Length
> - );
> - }
> + // Parse the specified number of GT Block Timer Structures or the GT
> + Block // Structure buffer length. Whichever is minimum.
> + while ((Index++ < *GtBlockTimerCount) &&
> + (Offset < Length)) {
> + Offset += ParseAcpi (
> + TRUE,
> + 2,
> + "GT Block Timer",
> + Ptr + Offset,
> + Length - Offset,
> + PARSER_PARAMS (GtBlockTimerParser)
> + );
The above confuse me a lot. ACPI Spec 6.3 Table 5-124:
Length - "20+n*40, where n is the number of timers implemented in the GT Block"
GT Block Timer Structure[] - Byte Offset: GT Block Timer Offset - "Array of GT Block Timer Structures. See Table 5-125"
Why the 'Byte Offset' is not 20?
'Length - Offset' would be 'Length - 20' == 'n * 40'. If the 'GT Block Timer Offset' is not 20, its value should be lager 20. Then 'Length - Offset' would always less than 'n * 40'. It may miss some info of the GtBlockTimer.
Maybe all the platforms' GT block table's 'GT Block Timer Offset' is always 20. If so, then there is no problem with this above section.
Did I misunderstand something?
Thanks,
Zhichao
> }
> }
>
> @@ -270,6 +256,7 @@ ParseAcpiGtdt (
> )
> {
> UINT32 Index;
> + UINT32 Offset;
> UINT8* TimerPtr;
>
> if (!Trace) {
> @@ -285,36 +272,54 @@ ParseAcpiGtdt (
> PARSER_PARAMS (GtdtParser)
> );
>
> - if (*GtdtPlatformTimerCount != 0) {
> - TimerPtr = Ptr + (*GtdtPlatformTimerOffset);
> - Index = 0;
> - do {
> - // Parse the Platform Timer Header
> - ParseAcpi (
> - FALSE,
> - 0,
> - NULL,
> - TimerPtr,
> - 4, // GT Platform Timer structure header length.
> - PARSER_PARAMS (GtPlatformTimerHeaderParser)
> + TimerPtr = Ptr + *GtdtPlatformTimerOffset; Offset =
> + *GtdtPlatformTimerOffset; Index = 0;
> +
> + // Parse the specified number of Platform Timer Structures or the
> + GTDT // buffer length. Whichever is minimum.
> + while ((Index++ < *GtdtPlatformTimerCount) &&
> + (Offset < AcpiTableLength)) {
> + // Parse the Platform Timer Header to obtain Length and Type
> + ParseAcpi (
> + FALSE,
> + 0,
> + NULL,
> + TimerPtr,
> + AcpiTableLength - Offset,
> + PARSER_PARAMS (GtPlatformTimerHeaderParser)
> + );
> +
> + // Make sure the Platform Timer is inside the table.
> + if ((Offset + *PlatformTimerLength) > AcpiTableLength) {
> + IncrementErrorCount ();
> + Print (
> + L"ERROR: Invalid Platform Timer Structure length. " \
> + L"PlatformTimerLength = %d. RemainingTableBufferLength = %d. " \
> + L"GTDT parsing aborted.\n",
> + *PlatformTimerLength,
> + AcpiTableLength - Offset
> );
> - switch (*PlatformTimerType) {
> - case EFI_ACPI_6_2_GTDT_GT_BLOCK:
> - DumpGTBlock (TimerPtr, *PlatformTimerLength);
> - break;
> - case EFI_ACPI_6_2_GTDT_SBSA_GENERIC_WATCHDOG:
> - DumpWatchdogTimer (TimerPtr, *PlatformTimerLength);
> - break;
> - default:
> - IncrementErrorCount ();
> - Print (
> - L"ERROR: INVALID Platform Timer Type = %d\n",
> - *PlatformTimerType
> - );
> - break;
> - } // switch
> - TimerPtr += (*PlatformTimerLength);
> - Index++;
> - } while (Index < *GtdtPlatformTimerCount);
> - }
> + return;
> + }
> +
> + switch (*PlatformTimerType) {
> + case EFI_ACPI_6_3_GTDT_GT_BLOCK:
> + DumpGTBlock (TimerPtr, *PlatformTimerLength);
> + break;
> + case EFI_ACPI_6_3_GTDT_SBSA_GENERIC_WATCHDOG:
> + DumpWatchdogTimer (TimerPtr, *PlatformTimerLength);
> + break;
> + default:
> + IncrementErrorCount ();
> + Print (
> + L"ERROR: Invalid Platform Timer Type = %d\n",
> + *PlatformTimerType
> + );
> + break;
> + } // switch
> +
> + TimerPtr += *PlatformTimerLength;
> + Offset += *PlatformTimerLength;
> + } // while
> }
> --
> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
>
>
>
>
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [edk2-devel] [PATCH v1 2/6] ShellPkg: acpiview: GTDT: Prevent buffer overruns
2019-08-05 7:23 ` Gao, Zhichao
@ 2019-08-05 9:54 ` Sami Mujawar
2019-08-06 4:58 ` Gao, Zhichao
0 siblings, 1 reply; 24+ messages in thread
From: Sami Mujawar @ 2019-08-05 9:54 UTC (permalink / raw)
To: devel@edk2.groups.io, zhichao.gao@intel.com, Krzysztof Koch
Cc: Carsey, Jaben, Ni, Ray, Matteo Carlini, nd
Hi Zhichao,
Please see my response inline.
Regards,
Sami Mujawar
-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Gao, Zhichao via Groups.Io
Sent: 05 August 2019 08:23 AM
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 2/6] ShellPkg: acpiview: GTDT: Prevent buffer overruns
One confusion below:
> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Krzysztof Koch
> 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: [edk2-devel] [PATCH v1 2/6] ShellPkg: acpiview: GTDT: Prevent
> buffer overruns
>
> Modify the GTDT table parsing logic to prevent reading past the ACPI
> buffer lengths provided and to make it consistent with other table
> parsers. This includes converting the do-while loop in ParseAcpiGtdt() into a while loop.
>
> Remove a check which ensures that the entire Platform GT Block
> Structure buffer has been parsed. The ACPI specification does not ban
> from defining buffers which are larger than the size indicated by the
> count and sizes of substructures which constitute it.
>
> Change the data type of the Length parameter to the DumpGTBlock()
> function to reflect the width of the respective ACPI structure's field.
>
> References:
> - ACPI 6.3, January 2019, Table 5-124
>
> Signed-off-by: Krzysztof Koch <krzysztof.koch@arm.com>
> ---
>
> Notes:
> v1:
> - Prevent buffer overruns in GTDT acpiview parser [Krzysztof]
>
>
> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser.c
> | 147 ++++++++++----------
> 1 file changed, 76 insertions(+), 71 deletions(-)
>
> diff --git
> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser
> .c
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser
> .c
> index
> 1e5b5764f50a2d29aa904c889bc89af5bdc3af5c..57174e14c80072f12b90e1996e
> be8f0002d0c404 100644
> ---
> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser
> .c
> +++
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtPars
> +++ er.c
> @@ -23,7 +23,6 @@ STATIC CONST UINT8* PlatformTimerType; STATIC
> CONST UINT16* PlatformTimerLength; STATIC CONST UINT32*
> GtBlockTimerCount; STATIC CONST UINT32* GtBlockTimerOffset; -STATIC
> CONST UINT16* GtBlockLength; STATIC ACPI_DESCRIPTION_HEADER_INFO
> AcpiHdrInfo;
>
> /**
> @@ -127,7 +126,7 @@ STATIC CONST ACPI_PARSER
> GtPlatformTimerHeaderParser[] = { **/ STATIC CONST ACPI_PARSER
> GtBlockParser[] = {
> {L"Type", 1, 0, L"%d", NULL, NULL, NULL, NULL},
> - {L"Length", 2, 1, L"%d", NULL, (VOID**)&GtBlockLength, NULL, NULL},
> + {L"Length", 2, 1, L"%d", NULL, NULL, NULL, NULL},
> {L"Reserved", 1, 3, L"%x", NULL, NULL, NULL, NULL},
> {L"Physical address (CntCtlBase)", 8, 4, L"0x%lx", NULL, NULL, NULL, NULL},
> {L"Timer Count", 4, 12, L"%d", NULL, (VOID**)&GtBlockTimerCount, @@
> -
> 168,56 +167,43 @@ STATIC CONST ACPI_PARSER SBSAGenericWatchdogParser[]
> = {
> /**
> This function parses the Platform GT Block.
>
> - @param [in] Ptr Pointer to the start of the GT Block data.
> - @param [in] Length Length of the GT Block structure.
> + @param [in] Ptr Pointer to the start of the GT Block data.
> + @param [in] Length Length of the GT Block structure.
> **/
> STATIC
> VOID
> DumpGTBlock (
> IN UINT8* Ptr,
> - IN UINT32 Length
> + IN UINT16 Length
> )
> {
> UINT32 Index;
> UINT32 Offset;
> - UINT32 GTBlockTimerLength;
>
> - Offset = ParseAcpi (
> - TRUE,
> - 2,
> - "GT Block",
> - Ptr,
> - Length,
> - PARSER_PARAMS (GtBlockParser)
> - );
> - GTBlockTimerLength = (*GtBlockLength - Offset) /
> (*GtBlockTimerCount);
> - Length -= Offset;
> + ParseAcpi (
> + TRUE,
> + 2,
> + "GT Block",
> + Ptr,
> + Length,
> + PARSER_PARAMS (GtBlockParser)
> + );
>
> - if (*GtBlockTimerCount != 0) {
> - Ptr += (*GtBlockTimerOffset);
> - Index = 0;
> - while ((Index < (*GtBlockTimerCount)) && (Length >=
> GTBlockTimerLength)) {
> - Offset = ParseAcpi (
> - TRUE,
> - 2,
> - "GT Block Timer",
> - Ptr,
> - GTBlockTimerLength,
> - PARSER_PARAMS (GtBlockTimerParser)
> - );
> - // Increment by GT Block Timer structure size
> - Ptr += Offset;
> - Length -= Offset;
> - Index++;
> - }
> + Offset = *GtBlockTimerOffset;
> + Index = 0;
>
> - if (Length != 0) {
> - IncrementErrorCount ();
> - Print (
> - L"ERROR:GT Block Timer length mismatch. Unparsed %d bytes.\n",
> - Length
> - );
> - }
> + // Parse the specified number of GT Block Timer Structures or the
> + GT Block // Structure buffer length. Whichever is minimum.
> + while ((Index++ < *GtBlockTimerCount) &&
> + (Offset < Length)) {
> + Offset += ParseAcpi (
> + TRUE,
> + 2,
> + "GT Block Timer",
> + Ptr + Offset,
> + Length - Offset,
> + PARSER_PARAMS (GtBlockTimerParser)
> + );
The above confuse me a lot. ACPI Spec 6.3 Table 5-124:
Length - "20+n*40, where n is the number of timers implemented in the GT Block"
GT Block Timer Structure[] - Byte Offset: GT Block Timer Offset - "Array of GT Block Timer Structures. See Table 5-125"
Why the 'Byte Offset' is not 20?
'Length - Offset' would be 'Length - 20' == 'n * 40'. If the 'GT Block Timer Offset' is not 20, its value should be lager 20. Then 'Length - Offset' would always less than 'n * 40'. It may miss some info of the GtBlockTimer.
Maybe all the platforms' GT block table's 'GT Block Timer Offset' is always 20. If so, then there is no problem with this above section.
Did I misunderstand something?
[SAMI] The '20+n*40' in the 'Description' column in the ACPI Spec 6.3 Table 5-124 is an example of how the length field may be computed.
The 'GT Block Timer Offset' field could technically allow unused bytes between the 'GT Block Timer Offset' field and the start of the first 'GT Block Timer Structure'. An OS is expected to read the 'GT Block Timer Offset' field to know where the 'GT Block Timer Structure' data starts. Conversely the firmware must encode the length field appropriately. In this case the length field would be '20+UnusedByteCount+(n*40)'.
The 'Length - Offset' is used to ensure we don't run past the buffer. This also reinforces the fact that ACPIview would be providing a view of the tables as an OS might see.
I will check if this can be made clearer in the specification.
[/SAMI]
Thanks,
Zhichao
> }
> }
>
> @@ -270,6 +256,7 @@ ParseAcpiGtdt (
> )
> {
> UINT32 Index;
> + UINT32 Offset;
> UINT8* TimerPtr;
>
> if (!Trace) {
> @@ -285,36 +272,54 @@ ParseAcpiGtdt (
> PARSER_PARAMS (GtdtParser)
> );
>
> - if (*GtdtPlatformTimerCount != 0) {
> - TimerPtr = Ptr + (*GtdtPlatformTimerOffset);
> - Index = 0;
> - do {
> - // Parse the Platform Timer Header
> - ParseAcpi (
> - FALSE,
> - 0,
> - NULL,
> - TimerPtr,
> - 4, // GT Platform Timer structure header length.
> - PARSER_PARAMS (GtPlatformTimerHeaderParser)
> + TimerPtr = Ptr + *GtdtPlatformTimerOffset; Offset =
> + *GtdtPlatformTimerOffset; Index = 0;
> +
> + // Parse the specified number of Platform Timer Structures or the
> + GTDT // buffer length. Whichever is minimum.
> + while ((Index++ < *GtdtPlatformTimerCount) &&
> + (Offset < AcpiTableLength)) {
> + // Parse the Platform Timer Header to obtain Length and Type
> + ParseAcpi (
> + FALSE,
> + 0,
> + NULL,
> + TimerPtr,
> + AcpiTableLength - Offset,
> + PARSER_PARAMS (GtPlatformTimerHeaderParser)
> + );
> +
> + // Make sure the Platform Timer is inside the table.
> + if ((Offset + *PlatformTimerLength) > AcpiTableLength) {
> + IncrementErrorCount ();
> + Print (
> + L"ERROR: Invalid Platform Timer Structure length. " \
> + L"PlatformTimerLength = %d. RemainingTableBufferLength = %d. " \
> + L"GTDT parsing aborted.\n",
> + *PlatformTimerLength,
> + AcpiTableLength - Offset
> );
> - switch (*PlatformTimerType) {
> - case EFI_ACPI_6_2_GTDT_GT_BLOCK:
> - DumpGTBlock (TimerPtr, *PlatformTimerLength);
> - break;
> - case EFI_ACPI_6_2_GTDT_SBSA_GENERIC_WATCHDOG:
> - DumpWatchdogTimer (TimerPtr, *PlatformTimerLength);
> - break;
> - default:
> - IncrementErrorCount ();
> - Print (
> - L"ERROR: INVALID Platform Timer Type = %d\n",
> - *PlatformTimerType
> - );
> - break;
> - } // switch
> - TimerPtr += (*PlatformTimerLength);
> - Index++;
> - } while (Index < *GtdtPlatformTimerCount);
> - }
> + return;
> + }
> +
> + switch (*PlatformTimerType) {
> + case EFI_ACPI_6_3_GTDT_GT_BLOCK:
> + DumpGTBlock (TimerPtr, *PlatformTimerLength);
> + break;
> + case EFI_ACPI_6_3_GTDT_SBSA_GENERIC_WATCHDOG:
> + DumpWatchdogTimer (TimerPtr, *PlatformTimerLength);
> + break;
> + default:
> + IncrementErrorCount ();
> + Print (
> + L"ERROR: Invalid Platform Timer Type = %d\n",
> + *PlatformTimerType
> + );
> + break;
> + } // switch
> +
> + TimerPtr += *PlatformTimerLength;
> + Offset += *PlatformTimerLength;
> + } // while
> }
> --
> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
>
>
>
>
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [edk2-devel] [PATCH v1 2/6] ShellPkg: acpiview: GTDT: Prevent buffer overruns
2019-08-05 9:54 ` Sami Mujawar
@ 2019-08-06 4:58 ` Gao, Zhichao
0 siblings, 0 replies; 24+ messages in thread
From: Gao, Zhichao @ 2019-08-06 4:58 UTC (permalink / raw)
To: devel@edk2.groups.io, 'sami.mujawar@arm.com',
Krzysztof Koch
Cc: Carsey, Jaben, Ni, Ray, Matteo Carlini, nd
OK, I got it. Now I have no comment of this patch. I think the description of 'Length' should be 'the whole length of the GT block include GT Block Timer Structure' or '20+("GT Block Timer Offset" - 20) + n * 40, where n ...' in the ACPI spec.
Reviewed-by: Zhichao Gao <zhichao.gao@intel.com>
Thanks,
Zhichao
> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Sami Mujawar
> Sent: Monday, August 5, 2019 5:55 PM
> To: devel@edk2.groups.io; Gao, Zhichao <zhichao.gao@intel.com>; Krzysztof
> Koch <Krzysztof.Koch@arm.com>
> Cc: Carsey, Jaben <jaben.carsey@intel.com>; Ni, Ray <ray.ni@intel.com>;
> Matteo Carlini <Matteo.Carlini@arm.com>; nd <nd@arm.com>
> Subject: Re: [edk2-devel] [PATCH v1 2/6] ShellPkg: acpiview: GTDT: Prevent
> buffer overruns
>
> Hi Zhichao,
>
> Please see my response inline.
>
> Regards,
>
> Sami Mujawar
>
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Gao,
> Zhichao via Groups.Io
> Sent: 05 August 2019 08:23 AM
> 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 2/6] ShellPkg: acpiview: GTDT: Prevent
> buffer overruns
>
> One confusion below:
>
> > -----Original Message-----
> > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> > Krzysztof Koch
> > 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: [edk2-devel] [PATCH v1 2/6] ShellPkg: acpiview: GTDT: Prevent
> > buffer overruns
> >
> > Modify the GTDT table parsing logic to prevent reading past the ACPI
> > buffer lengths provided and to make it consistent with other table
> > parsers. This includes converting the do-while loop in ParseAcpiGtdt() into
> a while loop.
> >
> > Remove a check which ensures that the entire Platform GT Block
> > Structure buffer has been parsed. The ACPI specification does not ban
> > from defining buffers which are larger than the size indicated by the
> > count and sizes of substructures which constitute it.
> >
> > Change the data type of the Length parameter to the DumpGTBlock()
> > function to reflect the width of the respective ACPI structure's field.
> >
> > References:
> > - ACPI 6.3, January 2019, Table 5-124
> >
> > Signed-off-by: Krzysztof Koch <krzysztof.koch@arm.com>
> > ---
> >
> > Notes:
> > v1:
> > - Prevent buffer overruns in GTDT acpiview parser [Krzysztof]
> >
> >
> >
> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser.c
> > | 147 ++++++++++----------
> > 1 file changed, 76 insertions(+), 71 deletions(-)
> >
> > diff --git
> >
> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser
> > .c
> >
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser
> > .c
> > index
> >
> 1e5b5764f50a2d29aa904c889bc89af5bdc3af5c..57174e14c80072f12b90e1996e
> > be8f0002d0c404 100644
> > ---
> >
> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser
> > .c
> > +++
> > b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtPars
> > +++ er.c
> > @@ -23,7 +23,6 @@ STATIC CONST UINT8* PlatformTimerType; STATIC
> > CONST UINT16* PlatformTimerLength; STATIC CONST UINT32*
> > GtBlockTimerCount; STATIC CONST UINT32* GtBlockTimerOffset; -STATIC
> > CONST UINT16* GtBlockLength; STATIC
> ACPI_DESCRIPTION_HEADER_INFO
> > AcpiHdrInfo;
> >
> > /**
> > @@ -127,7 +126,7 @@ STATIC CONST ACPI_PARSER
> > GtPlatformTimerHeaderParser[] = { **/ STATIC CONST ACPI_PARSER
> > GtBlockParser[] = {
> > {L"Type", 1, 0, L"%d", NULL, NULL, NULL, NULL},
> > - {L"Length", 2, 1, L"%d", NULL, (VOID**)&GtBlockLength, NULL, NULL},
> > + {L"Length", 2, 1, L"%d", NULL, NULL, NULL, NULL},
> > {L"Reserved", 1, 3, L"%x", NULL, NULL, NULL, NULL},
> > {L"Physical address (CntCtlBase)", 8, 4, L"0x%lx", NULL, NULL, NULL, NULL},
> > {L"Timer Count", 4, 12, L"%d", NULL, (VOID**)&GtBlockTimerCount, @@
> > -
> > 168,56 +167,43 @@ STATIC CONST ACPI_PARSER
> SBSAGenericWatchdogParser[]
> > = {
> > /**
> > This function parses the Platform GT Block.
> >
> > - @param [in] Ptr Pointer to the start of the GT Block data.
> > - @param [in] Length Length of the GT Block structure.
> > + @param [in] Ptr Pointer to the start of the GT Block data.
> > + @param [in] Length Length of the GT Block structure.
> > **/
> > STATIC
> > VOID
> > DumpGTBlock (
> > IN UINT8* Ptr,
> > - IN UINT32 Length
> > + IN UINT16 Length
> > )
> > {
> > UINT32 Index;
> > UINT32 Offset;
> > - UINT32 GTBlockTimerLength;
> >
> > - Offset = ParseAcpi (
> > - TRUE,
> > - 2,
> > - "GT Block",
> > - Ptr,
> > - Length,
> > - PARSER_PARAMS (GtBlockParser)
> > - );
> > - GTBlockTimerLength = (*GtBlockLength - Offset) /
> > (*GtBlockTimerCount);
> > - Length -= Offset;
> > + ParseAcpi (
> > + TRUE,
> > + 2,
> > + "GT Block",
> > + Ptr,
> > + Length,
> > + PARSER_PARAMS (GtBlockParser)
> > + );
> >
> > - if (*GtBlockTimerCount != 0) {
> > - Ptr += (*GtBlockTimerOffset);
> > - Index = 0;
> > - while ((Index < (*GtBlockTimerCount)) && (Length >=
> > GTBlockTimerLength)) {
> > - Offset = ParseAcpi (
> > - TRUE,
> > - 2,
> > - "GT Block Timer",
> > - Ptr,
> > - GTBlockTimerLength,
> > - PARSER_PARAMS (GtBlockTimerParser)
> > - );
> > - // Increment by GT Block Timer structure size
> > - Ptr += Offset;
> > - Length -= Offset;
> > - Index++;
> > - }
> > + Offset = *GtBlockTimerOffset;
> > + Index = 0;
> >
> > - if (Length != 0) {
> > - IncrementErrorCount ();
> > - Print (
> > - L"ERROR:GT Block Timer length mismatch. Unparsed %d bytes.\n",
> > - Length
> > - );
> > - }
> > + // Parse the specified number of GT Block Timer Structures or the
> > + GT Block // Structure buffer length. Whichever is minimum.
> > + while ((Index++ < *GtBlockTimerCount) &&
> > + (Offset < Length)) {
> > + Offset += ParseAcpi (
> > + TRUE,
> > + 2,
> > + "GT Block Timer",
> > + Ptr + Offset,
> > + Length - Offset,
> > + PARSER_PARAMS (GtBlockTimerParser)
> > + );
>
> The above confuse me a lot. ACPI Spec 6.3 Table 5-124:
> Length - "20+n*40, where n is the number of timers implemented in the GT
> Block"
> GT Block Timer Structure[] - Byte Offset: GT Block Timer Offset - "Array of GT
> Block Timer Structures. See Table 5-125"
>
> Why the 'Byte Offset' is not 20?
> 'Length - Offset' would be 'Length - 20' == 'n * 40'. If the 'GT Block Timer
> Offset' is not 20, its value should be lager 20. Then 'Length - Offset' would
> always less than 'n * 40'. It may miss some info of the GtBlockTimer.
> Maybe all the platforms' GT block table's 'GT Block Timer Offset' is always 20.
> If so, then there is no problem with this above section.
>
> Did I misunderstand something?
> [SAMI] The '20+n*40' in the 'Description' column in the ACPI Spec 6.3 Table 5-
> 124 is an example of how the length field may be computed.
> The 'GT Block Timer Offset' field could technically allow unused bytes
> between the 'GT Block Timer Offset' field and the start of the first 'GT Block
> Timer Structure'. An OS is expected to read the 'GT Block Timer Offset' field
> to know where the 'GT Block Timer Structure' data starts. Conversely the
> firmware must encode the length field appropriately. In this case the length
> field would be '20+UnusedByteCount+(n*40)'.
>
> The 'Length - Offset' is used to ensure we don't run past the buffer. This also
> reinforces the fact that ACPIview would be providing a view of the tables as
> an OS might see.
>
> I will check if this can be made clearer in the specification.
> [/SAMI]
>
> Thanks,
> Zhichao
>
> > }
> > }
> >
> > @@ -270,6 +256,7 @@ ParseAcpiGtdt (
> > )
> > {
> > UINT32 Index;
> > + UINT32 Offset;
> > UINT8* TimerPtr;
> >
> > if (!Trace) {
> > @@ -285,36 +272,54 @@ ParseAcpiGtdt (
> > PARSER_PARAMS (GtdtParser)
> > );
> >
> > - if (*GtdtPlatformTimerCount != 0) {
> > - TimerPtr = Ptr + (*GtdtPlatformTimerOffset);
> > - Index = 0;
> > - do {
> > - // Parse the Platform Timer Header
> > - ParseAcpi (
> > - FALSE,
> > - 0,
> > - NULL,
> > - TimerPtr,
> > - 4, // GT Platform Timer structure header length.
> > - PARSER_PARAMS (GtPlatformTimerHeaderParser)
> > + TimerPtr = Ptr + *GtdtPlatformTimerOffset; Offset =
> > + *GtdtPlatformTimerOffset; Index = 0;
> > +
> > + // Parse the specified number of Platform Timer Structures or the
> > + GTDT // buffer length. Whichever is minimum.
> > + while ((Index++ < *GtdtPlatformTimerCount) &&
> > + (Offset < AcpiTableLength)) {
> > + // Parse the Platform Timer Header to obtain Length and Type
> > + ParseAcpi (
> > + FALSE,
> > + 0,
> > + NULL,
> > + TimerPtr,
> > + AcpiTableLength - Offset,
> > + PARSER_PARAMS (GtPlatformTimerHeaderParser)
> > + );
> > +
> > + // Make sure the Platform Timer is inside the table.
> > + if ((Offset + *PlatformTimerLength) > AcpiTableLength) {
> > + IncrementErrorCount ();
> > + Print (
> > + L"ERROR: Invalid Platform Timer Structure length. " \
> > + L"PlatformTimerLength = %d. RemainingTableBufferLength = %d. " \
> > + L"GTDT parsing aborted.\n",
> > + *PlatformTimerLength,
> > + AcpiTableLength - Offset
> > );
> > - switch (*PlatformTimerType) {
> > - case EFI_ACPI_6_2_GTDT_GT_BLOCK:
> > - DumpGTBlock (TimerPtr, *PlatformTimerLength);
> > - break;
> > - case EFI_ACPI_6_2_GTDT_SBSA_GENERIC_WATCHDOG:
> > - DumpWatchdogTimer (TimerPtr, *PlatformTimerLength);
> > - break;
> > - default:
> > - IncrementErrorCount ();
> > - Print (
> > - L"ERROR: INVALID Platform Timer Type = %d\n",
> > - *PlatformTimerType
> > - );
> > - break;
> > - } // switch
> > - TimerPtr += (*PlatformTimerLength);
> > - Index++;
> > - } while (Index < *GtdtPlatformTimerCount);
> > - }
> > + return;
> > + }
> > +
> > + switch (*PlatformTimerType) {
> > + case EFI_ACPI_6_3_GTDT_GT_BLOCK:
> > + DumpGTBlock (TimerPtr, *PlatformTimerLength);
> > + break;
> > + case EFI_ACPI_6_3_GTDT_SBSA_GENERIC_WATCHDOG:
> > + DumpWatchdogTimer (TimerPtr, *PlatformTimerLength);
> > + break;
> > + default:
> > + IncrementErrorCount ();
> > + Print (
> > + L"ERROR: Invalid Platform Timer Type = %d\n",
> > + *PlatformTimerType
> > + );
> > + break;
> > + } // switch
> > +
> > + TimerPtr += *PlatformTimerLength;
> > + Offset += *PlatformTimerLength;
> > + } // while
> > }
> > --
> > 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
> >
> >
> >
> >
>
>
>
>
>
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v1 3/6] ShellPkg: acpiview: IORT: Prevent buffer overruns
2019-08-01 8:44 [PATCH v1 0/6] Prevent buffer overruns in acpiview table parsers Krzysztof Koch
2019-08-01 8:44 ` [PATCH v1 1/6] ShellPkg: acpiview: DBG2: Prevent buffer overruns Krzysztof Koch
2019-08-01 8:44 ` [PATCH v1 2/6] ShellPkg: acpiview: GTDT: " Krzysztof Koch
@ 2019-08-01 8:44 ` Krzysztof Koch
2019-08-01 9:32 ` [edk2-devel] " Alexei Fedorov
2019-08-01 8:44 ` [PATCH v1 4/6] ShellPkg: acpiview: MADT: " Krzysztof Koch
` (5 subsequent siblings)
8 siblings, 1 reply; 24+ messages in thread
From: Krzysztof Koch @ 2019-08-01 8:44 UTC (permalink / raw)
To: devel; +Cc: jaben.carsey, ray.ni, zhichao.gao, Sami.Mujawar, Matteo.Carlini,
nd
Modify the IORT table parsing logic to prevent reading past the buffer
lengths provided.
Change DumpIortNodeIdMappings() function's signature and implementation
to simplify buffer overrun prevention. Update all calls to this
function accordingly.
Modify the parser for each type of IORT node such that the offset from
the start of the node's buffer is tracked as the parsing function is
executed. Again, this change helps prevent buffer overruns.
Test that the IORT node buffer fits in the table buffer before the
node's buffer contents are dumped.
References:
- IO Remapping Table (Issue D), Platform Design Document, March 2018
Signed-off-by: Krzysztof Koch <krzysztof.koch@arm.com>
---
Notes:
v1:
- Prevent buffer overruns in IORT acpiview parser [Krzysztof]
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser.c | 191 +++++++++++---------
1 file changed, 105 insertions(+), 86 deletions(-)
diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser.c
index 7c850b3813d5204775e2cc247cabf42358b25769..8912d415a755c7f892b5cd2edc532aae8964a42c 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser.c
@@ -247,42 +247,41 @@ STATIC CONST ACPI_PARSER IortNodePmcgParser[] = {
/**
This function parses the IORT Node Id Mapping array.
- @param [in] Ptr Pointer to the start of the IORT Table.
+ @param [in] Ptr Pointer to the start of the ID mapping array.
+ @param [in] Length Length of the buffer.
@param [in] MappingCount The ID Mapping count.
- @param [in] MappingOffset The offset of the ID Mapping array
- from the start of the IORT table.
**/
STATIC
VOID
DumpIortNodeIdMappings (
IN UINT8* Ptr,
- IN UINT32 MappingCount,
- IN UINT32 MappingOffset
+ IN UINT32 Length,
+ IN UINT32 MappingCount
)
{
- UINT8* IdMappingPtr;
UINT32 Index;
UINT32 Offset;
CHAR8 Buffer[40]; // Used for AsciiName param of ParseAcpi
- IdMappingPtr = Ptr + MappingOffset;
Index = 0;
- while (Index < MappingCount) {
+ Offset = 0;
+
+ while ((Index < MappingCount) &&
+ (Offset < Length)) {
AsciiSPrint (
Buffer,
sizeof (Buffer),
"ID Mapping [%d]",
Index
);
- Offset = ParseAcpi (
- TRUE,
- 4,
- Buffer,
- IdMappingPtr,
- 20,
- PARSER_PARAMS (IortNodeIdMappingParser)
- );
- IdMappingPtr += Offset;
+ Offset += ParseAcpi (
+ TRUE,
+ 4,
+ Buffer,
+ Ptr + Offset,
+ Length - Offset,
+ PARSER_PARAMS (IortNodeIdMappingParser)
+ );
Index++;
}
}
@@ -309,8 +308,6 @@ DumpIortNodeSmmuV1V2 (
UINT32 Offset;
CHAR8 Buffer[50]; // Used for AsciiName param of ParseAcpi
- UINT8* ArrayPtr;
-
ParseAcpi (
TRUE,
2,
@@ -320,51 +317,55 @@ DumpIortNodeSmmuV1V2 (
PARSER_PARAMS (IortNodeSmmuV1V2Parser)
);
- ArrayPtr = Ptr + *InterruptContextOffset;
+ Offset = *InterruptContextOffset;
Index = 0;
- while (Index < *InterruptContextCount) {
+
+ while ((Index < *InterruptContextCount) &&
+ (Offset < Length)) {
AsciiSPrint (
Buffer,
sizeof (Buffer),
"Context Interrupts Array [%d]",
Index
);
- Offset = ParseAcpi (
- TRUE,
- 4,
- Buffer,
- ArrayPtr,
- 8,
- PARSER_PARAMS (InterruptArrayParser)
- );
- ArrayPtr += Offset;
+ Offset += ParseAcpi (
+ TRUE,
+ 4,
+ Buffer,
+ Ptr + Offset,
+ Length - Offset,
+ PARSER_PARAMS (InterruptArrayParser)
+ );
Index++;
}
- ArrayPtr = Ptr + *PmuInterruptOffset;
+ Offset = *PmuInterruptOffset;
Index = 0;
- while (Index < *PmuInterruptCount) {
+
+ while ((Index < *PmuInterruptCount) &&
+ (Offset < Length)) {
AsciiSPrint (
Buffer,
sizeof (Buffer),
"PMU Interrupts Array [%d]",
Index
);
- Offset = ParseAcpi (
- TRUE,
- 4,
- Buffer,
- ArrayPtr,
- 8,
- PARSER_PARAMS (InterruptArrayParser)
- );
- ArrayPtr += Offset;
+ Offset += ParseAcpi (
+ TRUE,
+ 4,
+ Buffer,
+ Ptr + Offset,
+ Length - Offset,
+ PARSER_PARAMS (InterruptArrayParser)
+ );
Index++;
}
- if (*IortIdMappingCount != 0) {
- DumpIortNodeIdMappings (Ptr, MappingCount, MappingOffset);
- }
+ DumpIortNodeIdMappings (
+ Ptr + MappingOffset,
+ Length - MappingOffset,
+ MappingCount
+ );
}
/**
@@ -394,9 +395,11 @@ DumpIortNodeSmmuV3 (
PARSER_PARAMS (IortNodeSmmuV3Parser)
);
- if (*IortIdMappingCount != 0) {
- DumpIortNodeIdMappings (Ptr, MappingCount, MappingOffset);
- }
+ DumpIortNodeIdMappings (
+ Ptr + MappingOffset,
+ Length - MappingOffset,
+ MappingCount
+ );
}
/**
@@ -414,40 +417,40 @@ DumpIortNodeIts (
{
UINT32 Offset;
UINT32 Index;
- UINT8* ItsIdPtr;
CHAR8 Buffer[80]; // Used for AsciiName param of ParseAcpi
Offset = ParseAcpi (
- TRUE,
- 2,
- "ITS Node",
- Ptr,
- Length,
- PARSER_PARAMS (IortNodeItsParser)
- );
+ TRUE,
+ 2,
+ "ITS Node",
+ Ptr,
+ Length,
+ PARSER_PARAMS (IortNodeItsParser)
+ );
- ItsIdPtr = Ptr + Offset;
Index = 0;
- while (Index < *ItsCount) {
+
+ while ((Index < *ItsCount) &&
+ (Offset < Length)) {
AsciiSPrint (
Buffer,
sizeof (Buffer),
"GIC ITS Identifier Array [%d]",
Index
);
- Offset = ParseAcpi (
- TRUE,
- 4,
- Buffer,
- ItsIdPtr,
- 4,
- PARSER_PARAMS (ItsIdParser)
- );
- ItsIdPtr += Offset;
+ Offset += ParseAcpi (
+ TRUE,
+ 4,
+ Buffer,
+ Ptr + Offset,
+ Length - Offset,
+ PARSER_PARAMS (ItsIdParser)
+ );
Index++;
}
// Note: ITS does not have the ID Mappings Array
+
}
/**
@@ -470,8 +473,6 @@ DumpIortNodeNamedComponent (
{
UINT32 Offset;
UINT32 Index;
- UINT8* DeviceNamePtr;
- UINT32 DeviceNameLength;
Offset = ParseAcpi (
TRUE,
@@ -482,19 +483,22 @@ DumpIortNodeNamedComponent (
PARSER_PARAMS (IortNodeNamedComponentParser)
);
- DeviceNamePtr = Ptr + Offset;
// Estimate the Device Name length
- DeviceNameLength = Length - Offset - (MappingCount * 20);
PrintFieldName (2, L"Device Object Name");
Index = 0;
- while ((Index < DeviceNameLength) && (DeviceNamePtr[Index] != 0)) {
- Print (L"%c", DeviceNamePtr[Index++]);
+
+ while ((*(Ptr + Offset) != 0) &&
+ (Offset < Length)) {
+ Print (L"%c", *(Ptr + Offset));
+ Offset++;
}
Print (L"\n");
- if (*IortIdMappingCount != 0) {
- DumpIortNodeIdMappings (Ptr, MappingCount, MappingOffset);
- }
+ DumpIortNodeIdMappings (
+ Ptr + MappingOffset,
+ Length - MappingOffset,
+ MappingCount
+ );
}
/**
@@ -524,9 +528,11 @@ DumpIortNodeRootComplex (
PARSER_PARAMS (IortNodeRootComplexParser)
);
- if (*IortIdMappingCount != 0) {
- DumpIortNodeIdMappings (Ptr, MappingCount, MappingOffset);
- }
+ DumpIortNodeIdMappings (
+ Ptr + MappingOffset,
+ Length - MappingOffset,
+ MappingCount
+ );
}
/**
@@ -554,11 +560,13 @@ DumpIortNodePmcg (
Ptr,
Length,
PARSER_PARAMS (IortNodePmcgParser)
- );
+ );
- if (*IortIdMappingCount != 0) {
- DumpIortNodeIdMappings (Ptr, MappingCount, MappingOffset);
- }
+ DumpIortNodeIdMappings (
+ Ptr + MappingOffset,
+ Length - MappingOffset,
+ MappingCount
+ );
}
/**
@@ -605,23 +613,34 @@ ParseAcpiIort (
AcpiTableLength,
PARSER_PARAMS (IortParser)
);
+
Offset = *IortNodeOffset;
NodePtr = Ptr + Offset;
Index = 0;
- while ((Index < *IortNodeCount) && (Offset < AcpiTableLength)) {
+ // Parse the specified number of IORT nodes or the IORT table buffer length.
+ // Whichever is minimum.
+ while ((Index++ < *IortNodeCount) &&
+ (Offset < AcpiTableLength)) {
// Parse the IORT Node Header
ParseAcpi (
FALSE,
0,
"IORT Node Header",
NodePtr,
- 16,
+ AcpiTableLength - Offset,
PARSER_PARAMS (IortNodeHeaderParser)
);
- if (*IortNodeLength == 0) {
+
+ // Make sure the IORT Node is inside the table
+ if ((Offset + (*IortNodeLength)) > AcpiTableLength) {
IncrementErrorCount ();
- Print (L"ERROR: Parser error. Invalid table data.\n");
+ Print (
+ L"ERROR: Invalid IORT node length. IortNodeLength = %d. " \
+ L"RemainingTableBufferLength = %d. IORT parsing aborted.\n",
+ *IortNodeLength,
+ AcpiTableLength - Offset
+ );
return;
}
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v1 4/6] ShellPkg: acpiview: MADT: Prevent buffer overruns
2019-08-01 8:44 [PATCH v1 0/6] Prevent buffer overruns in acpiview table parsers Krzysztof Koch
` (2 preceding siblings ...)
2019-08-01 8:44 ` [PATCH v1 3/6] ShellPkg: acpiview: IORT: " Krzysztof Koch
@ 2019-08-01 8:44 ` Krzysztof Koch
2019-08-01 9:32 ` [edk2-devel] " Alexei Fedorov
2019-08-01 8:44 ` [PATCH v1 5/6] ShellPkg: acpiview: PPTT: " Krzysztof Koch
` (4 subsequent siblings)
8 siblings, 1 reply; 24+ messages in thread
From: Krzysztof Koch @ 2019-08-01 8:44 UTC (permalink / raw)
To: devel; +Cc: jaben.carsey, ray.ni, zhichao.gao, Sami.Mujawar, Matteo.Carlini,
nd
Modify the parsing logic to prevent reading past the MADT table buffer
length provided when parsing the Interrupt Controller Structure header.
Signed-off-by: Krzysztof Koch <krzysztof.koch@arm.com>
---
Notes:
v1:
- Prevent buffer overruns in MADT acpiview parser [Krzysztof]
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.c
index d80ebd1a2bae7a4acffe687ca5ee7b4090f0e223..90bdafea1970db522e8ed96de7c6e986cdaca5ba 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.c
@@ -256,7 +256,7 @@ ParseAcpiMadt (
0,
NULL,
InterruptContollerPtr,
- 2, // Length is 1 byte at offset 1
+ AcpiTableLength - Offset,
PARSER_PARAMS (MadtInterruptControllerHeaderParser)
);
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v1 5/6] ShellPkg: acpiview: PPTT: Prevent buffer overruns
2019-08-01 8:44 [PATCH v1 0/6] Prevent buffer overruns in acpiview table parsers Krzysztof Koch
` (3 preceding siblings ...)
2019-08-01 8:44 ` [PATCH v1 4/6] ShellPkg: acpiview: MADT: " Krzysztof Koch
@ 2019-08-01 8:44 ` Krzysztof Koch
2019-08-01 9:31 ` [edk2-devel] " Alexei Fedorov
2019-08-01 8:44 ` [PATCH v1 6/6] ShellPkg: acpiview: SRAT: " Krzysztof Koch
` (3 subsequent siblings)
8 siblings, 1 reply; 24+ messages in thread
From: Krzysztof Koch @ 2019-08-01 8:44 UTC (permalink / raw)
To: devel; +Cc: jaben.carsey, ray.ni, zhichao.gao, Sami.Mujawar, Matteo.Carlini,
nd
Modify the PPTT table parsing logic to prevent reading past the ACPI
buffer lengths provided.
Check if the Number of Private Resources specified in the Processor
Hierarchy Node (Type 0) is possible given the Type 0 Structure's buffer
length.
Make sure that the processor topology structure's buffer fits in the
PPTT table buffer before its contents are dumped.
Prevent buffer overruns when reading the processor topology structure's
header.
References:
- ACPI 6.3, January 2019, Section 5.2.29
Signed-off-by: Krzysztof Koch <krzysztof.koch@arm.com>
---
Notes:
v1:
- Prevent buffer overruns in PPTT acpiview parser [Krzysztof]
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c | 38 ++++++++++++++------
1 file changed, 27 insertions(+), 11 deletions(-)
diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c
index cec57be55e77096f9448f637ea129af2b42111ad..6254b9913fffb429fc54bb1301bf3e4b2e5bf161 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c
@@ -252,7 +252,6 @@ DumpProcessorHierarchyNodeStructure (
)
{
UINT32 Offset;
- UINT8* PrivateResourcePtr;
UINT32 Index;
CHAR16 Buffer[OUTPUT_FIELD_COLUMN_WIDTH];
@@ -265,8 +264,23 @@ DumpProcessorHierarchyNodeStructure (
PARSER_PARAMS (ProcessorHierarchyNodeStructureParser)
);
- PrivateResourcePtr = Ptr + Offset;
+ // Make sure the Private Resource array lies inside this structure
+ if (Offset + (*NumberOfPrivateResources * sizeof (UINT32)) > Length) {
+ IncrementErrorCount ();
+ Print (
+ L"ERROR: Invalid Number of Private Resources. " \
+ L"PrivateResourceCount = %d. RemainingBufferLength = %d. " \
+ L"Parsing of this structure aborted.\n",
+ *NumberOfPrivateResources,
+ Length - Offset
+ );
+ return;
+ }
+
Index = 0;
+
+ // Parse the specified number of private resource references or the Processor
+ // Hierarchy Node length. Whichever is minimum.
while (Index < *NumberOfPrivateResources) {
UnicodeSPrint (
Buffer,
@@ -278,10 +292,10 @@ DumpProcessorHierarchyNodeStructure (
PrintFieldName (4, Buffer);
Print (
L"0x%x\n",
- *((UINT32*) PrivateResourcePtr)
+ *((UINT32*)(Ptr + Offset))
);
- PrivateResourcePtr += sizeof(UINT32);
+ Offset += sizeof (UINT32);
Index++;
}
}
@@ -382,19 +396,21 @@ ParseAcpiPptt (
0,
NULL,
ProcessorTopologyStructurePtr,
- 4, // Length of the processor topology structure header is 4 bytes
+ AcpiTableLength - Offset,
PARSER_PARAMS (ProcessorTopologyStructureHeaderParser)
);
- if ((Offset + (*ProcessorTopologyStructureLength)) > AcpiTableLength) {
+ // Make sure the PPTT structure lies inside the table
+ if ((Offset + *ProcessorTopologyStructureLength) > AcpiTableLength) {
IncrementErrorCount ();
Print (
- L"ERROR: Invalid processor topology structure length:"
- L" Type = %d, Length = %d\n",
- *ProcessorTopologyStructureType,
- *ProcessorTopologyStructureLength
+ L"ERROR: Invalid PPTT structure length. " \
+ L"ProcessorTopologyStructureLength = %d. " \
+ L"RemainingTableBufferLength = %d. PPTT parsing aborted.\n",
+ *ProcessorTopologyStructureLength,
+ AcpiTableLength - Offset
);
- break;
+ return;
}
PrintFieldName (2, L"* Structure Offset *");
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v1 6/6] ShellPkg: acpiview: SRAT: Prevent buffer overruns
2019-08-01 8:44 [PATCH v1 0/6] Prevent buffer overruns in acpiview table parsers Krzysztof Koch
` (4 preceding siblings ...)
2019-08-01 8:44 ` [PATCH v1 5/6] ShellPkg: acpiview: PPTT: " Krzysztof Koch
@ 2019-08-01 8:44 ` Krzysztof Koch
2019-08-01 9:30 ` [edk2-devel] " Alexei Fedorov
2019-08-01 9:33 ` [edk2-devel] [PATCH v1 0/6] Prevent buffer overruns in acpiview table parsers Alexei Fedorov
` (2 subsequent siblings)
8 siblings, 1 reply; 24+ messages in thread
From: Krzysztof Koch @ 2019-08-01 8:44 UTC (permalink / raw)
To: devel; +Cc: jaben.carsey, ray.ni, zhichao.gao, Sami.Mujawar, Matteo.Carlini,
nd
Modify the SRAT parsing logic to prevent reading past the table buffer
length provided.
Check if the Static Resource Allocation Structure's buffer fits in the
SRAT table buffer before its contents are dumped.
Prevent buffer overruns when reading the Static Resource Allocation
Structure's header.
References:
- ACPI 6.3, January 2019, Section 5.2.16
Signed-off-by: Krzysztof Koch <krzysztof.koch@arm.com>
---
Notes:
v1:
- Prevent buffer overruns in SRAT acpiview parser [Krzysztof]
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser.c
index 59c77401eaab32b73a9f83fd4d63785221b3c222..a8aa420487bb6bf29fc38221d0b221573c64b8b3 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser.c
@@ -215,10 +215,22 @@ ParseAcpiSrat (
0,
NULL,
ResourcePtr,
- 2, // The length is 1 byte at offset 1
+ AcpiTableLength - Offset,
PARSER_PARAMS (SratResourceAllocationParser)
);
+ // Make sure the SRAT structure lies inside the table
+ if ((Offset + *SratRALength) > AcpiTableLength) {
+ IncrementErrorCount ();
+ Print (
+ L"ERROR: Invalid SRAT structure length. SratRALength = %d. " \
+ L"RemainingTableBufferLength = %d. SRAT parsing aborted.\n",
+ *SratRALength,
+ AcpiTableLength - Offset
+ );
+ return;
+ }
+
switch (*SratRAType) {
case EFI_ACPI_6_2_GICC_AFFINITY:
AsciiSPrint (
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [edk2-devel] [PATCH v1 0/6] Prevent buffer overruns in acpiview table parsers
2019-08-01 8:44 [PATCH v1 0/6] Prevent buffer overruns in acpiview table parsers Krzysztof Koch
` (5 preceding siblings ...)
2019-08-01 8:44 ` [PATCH v1 6/6] ShellPkg: acpiview: SRAT: " Krzysztof Koch
@ 2019-08-01 9:33 ` Alexei Fedorov
2019-08-06 8:10 ` Gao, Zhichao
2019-08-07 8:46 ` Sami Mujawar
8 siblings, 0 replies; 24+ messages in thread
From: Alexei Fedorov @ 2019-08-01 9:33 UTC (permalink / raw)
To: Krzysztof Koch, devel
[-- Attachment #1: Type: text/plain, Size: 54 bytes --]
Reviewed-by: Alexei Fedorov <Alexei.Fedorov@arm.com>
[-- Attachment #2: Type: text/html, Size: 60 bytes --]
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [edk2-devel] [PATCH v1 0/6] Prevent buffer overruns in acpiview table parsers
2019-08-01 8:44 [PATCH v1 0/6] Prevent buffer overruns in acpiview table parsers Krzysztof Koch
` (6 preceding siblings ...)
2019-08-01 9:33 ` [edk2-devel] [PATCH v1 0/6] Prevent buffer overruns in acpiview table parsers Alexei Fedorov
@ 2019-08-06 8:10 ` Gao, Zhichao
2019-08-07 8:46 ` Sami Mujawar
8 siblings, 0 replies; 24+ messages in thread
From: Gao, Zhichao @ 2019-08-06 8:10 UTC (permalink / raw)
To: devel@edk2.groups.io, krzysztof.koch@arm.com
Cc: Carsey, Jaben, Ni, Ray, Sami.Mujawar@arm.com,
Matteo.Carlini@arm.com, nd@arm.com
For 3 - 6: Reviewed-by: Zhichao Gao <zhichao.gao@intel.com>
Thanks,
Zhichao
> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Krzysztof Koch
> 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: [edk2-devel] [PATCH v1 0/6] Prevent buffer overruns in acpiview
> table parsers
>
> This patch series makes minor modifications to a number of ACPI table
> parsers with a goal to minimize the risk of buffer overruns. Some of these
> overruns can be caused by invalid ACPI table data.
>
> Changes can be seet at:
> https://github.com/KrzysztofKoch1/edk2/tree/612_prevent_buffer_overru
> ns_v1
>
> Krzysztof Koch (6):
> ShellPkg: acpiview: DBG2: Prevent buffer overruns
> ShellPkg: acpiview: GTDT: Prevent buffer overruns
> ShellPkg: acpiview: IORT: Prevent buffer overruns
> ShellPkg: acpiview: MADT: Prevent buffer overruns
> ShellPkg: acpiview: PPTT: Prevent buffer overruns
> ShellPkg: acpiview: SRAT: Prevent buffer overruns
>
>
> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c
> | 141 ++++++++++-----
> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser.c
> | 147 +++++++--------
> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser.c |
> 191 +++++++++++---------
>
> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.
> c | 2 +-
> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c
> | 38 ++--
> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser.c |
> 14 +-
> 6 files changed, 314 insertions(+), 219 deletions(-)
>
> --
> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
>
>
>
>
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v1 0/6] Prevent buffer overruns in acpiview table parsers
2019-08-01 8:44 [PATCH v1 0/6] Prevent buffer overruns in acpiview table parsers Krzysztof Koch
` (7 preceding siblings ...)
2019-08-06 8:10 ` Gao, Zhichao
@ 2019-08-07 8:46 ` Sami Mujawar
8 siblings, 0 replies; 24+ messages in thread
From: Sami Mujawar @ 2019-08-07 8:46 UTC (permalink / raw)
To: Krzysztof Koch, devel@edk2.groups.io
Cc: jaben.carsey@intel.com, ray.ni@intel.com, zhichao.gao@intel.com,
Matteo Carlini, nd
For this patch series.
Reviewed-by: Sami Mujawar <sami.mujawar@arm.com>
Regards,
Sami Mujawar
-----Original Message-----
From: Krzysztof Koch <krzysztof.koch@arm.com>
Sent: 01 August 2019 09:44 AM
To: devel@edk2.groups.io
Cc: jaben.carsey@intel.com; ray.ni@intel.com; zhichao.gao@intel.com; Sami Mujawar <Sami.Mujawar@arm.com>; Matteo Carlini <Matteo.Carlini@arm.com>; nd <nd@arm.com>
Subject: [PATCH v1 0/6] Prevent buffer overruns in acpiview table parsers
This patch series makes minor modifications to a number of ACPI table parsers with a goal to minimize the risk of buffer overruns. Some of these overruns can be caused by invalid ACPI table data.
Changes can be seet at: https://github.com/KrzysztofKoch1/edk2/tree/612_prevent_buffer_overruns_v1
Krzysztof Koch (6):
ShellPkg: acpiview: DBG2: Prevent buffer overruns
ShellPkg: acpiview: GTDT: Prevent buffer overruns
ShellPkg: acpiview: IORT: Prevent buffer overruns
ShellPkg: acpiview: MADT: Prevent buffer overruns
ShellPkg: acpiview: PPTT: Prevent buffer overruns
ShellPkg: acpiview: SRAT: Prevent buffer overruns
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c | 141 ++++++++++----- ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser.c | 147 +++++++-------- ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser.c | 191 +++++++++++---------
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.c | 2 +-
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c | 38 ++-- ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser.c | 14 +-
6 files changed, 314 insertions(+), 219 deletions(-)
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
^ permalink raw reply [flat|nested] 24+ messages in thread