[edk2-devel] [PATCH v1 11/11] ShellPkg: acpiview: DBG2: Add error-checking in the parsing logic
Gao, Zhichao
zhichao.gao at intel.com
Fri Jul 19 05:39:18 UTC 2019
Two comments with this patch.
> -----Original Message-----
> From: Krzysztof Koch [mailto:krzysztof.koch at arm.com]
> Sent: Friday, July 12, 2019 2:53 PM
> To: devel at edk2.groups.io
> Cc: Carsey, Jaben <jaben.carsey at intel.com>; Ni, Ray <ray.ni at intel.com>;
> Gao, Zhichao <zhichao.gao at intel.com>; Sami.Mujawar at arm.com;
> Matteo.Carlini at arm.com; nd at arm.com
> Subject: [PATCH v1 11/11] ShellPkg: acpiview: DBG2: Add error-checking in
> the parsing logic
>
> 1. Check if the global pointers (in the scope of this ACPI table parser) have
> been successfully updated before they are later used to control the parsing
> logic.
>
> 2. Remove redundant forward function declarations by repositioning blocks
> of code.
>
> 3. Test against buffer overruns.
>
> 4. Introduce a ACPI_PARSER array for parsing the header of the debug device
> information structure. This way, the length of the buffer storing a debug
> device information structure instance can be passed to
> DumpDbgDeviceInfo(). Consequently, the parsing logic becomes consistent
> with other ACPI table parsers and tests against buffer overrruns are simpler
> to implement.
>
> 5. Modify the signature of DumpGasStruct() function inside AcpiParser.c to
> facilitate protection against buffer overruns in the DBG2 parser.
>
> Signed-off-by: Krzysztof Koch <krzysztof.koch at arm.com>
> ---
>
> Changes can be seen at:
> https://github.com/KrzysztofKoch1/edk2/commit/530b059a9fe4aa9f1df36b4
> 07f97d76acaab8b74
>
> Notes:
> v1:
> - improve the logic in the DBG2 parser [Krzysztof]
>
> ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c | 26 +-
> ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h | 8 +-
>
> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c
> | 298 ++++++++++++++------
> 3 files changed, 225 insertions(+), 107 deletions(-)
>
> diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c
> index
> 8b3153516d2b7d9b920ab2de0344c17798ac572c..2d6ff80e299eebe7853061d3
> db89332197c0dc0e 100644
> --- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c
> +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c
> @@ -589,23 +589,27 @@ STATIC CONST ACPI_PARSER GasParser[] = {
>
> @param [in] Ptr Pointer to the start of the buffer.
> @param [in] Indent Number of spaces to indent the output.
> + @param [in] Length Length of the GAS structure buffer.
> +
> + @retval Number of bytes parsed.
> **/
> -VOID
> +UINT32
> EFIAPI
> DumpGasStruct (
> IN UINT8* Ptr,
> - IN UINT32 Indent
> + IN UINT32 Indent,
> + IN UINT32 Length
> )
> {
> Print (L"\n");
> - ParseAcpi (
> - TRUE,
> - Indent,
> - NULL,
> - Ptr,
> - GAS_LENGTH,
> - PARSER_PARAMS (GasParser)
> - );
> + return ParseAcpi (
> + TRUE,
> + Indent,
> + NULL,
> + Ptr,
> + Length,
> + PARSER_PARAMS (GasParser)
> + );
> }
>
> /**
> @@ -621,7 +625,7 @@ DumpGas (
> IN UINT8* Ptr
> )
> {
> - DumpGasStruct (Ptr, 2);
> + DumpGasStruct (Ptr, 2, GAS_LENGTH);
> }
>
> /**
> diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h
> index
> 7657892d9fd2e2e14c6578611ff0cf1b6f6cd750..20ca358bddfa5953bfb1d1beba
> ebbf3079eaba01 100644
> --- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h
> +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h
> @@ -405,12 +405,16 @@ ParseAcpi (
>
> @param [in] Ptr Pointer to the start of the buffer.
> @param [in] Indent Number of spaces to indent the output.
> + @param [in] Length Length of the GAS structure buffer.
> +
> + @retval Number of bytes parsed.
> **/
> -VOID
> +UINT32
> EFIAPI
> DumpGasStruct (
> IN UINT8* Ptr,
> - IN UINT32 Indent
> + IN UINT32 Indent,
> + IN UINT32 Length
> );
>
> /**
> diff --git
> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parse
> r.c
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parse
> r.c
> index
> 8de5ebf74775bab8e765849cba6ef4eb6f659a5a..2bbd622ffb7cec0a340de3e10
> bdcd01ba4d330df 100644
> ---
> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parse
> r.c
> +++
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Pars
> +++ er.c
> @@ -12,6 +12,7 @@
> #include <Library/UefiLib.h>
> #include "AcpiParser.h"
> #include "AcpiTableParser.h"
> +#include "AcpiView.h"
>
> // Local variables pointing to the table fields STATIC CONST UINT32*
> OffsetDbgDeviceInfo; @@ -27,7 +28,7 @@ STATIC CONST UINT16*
> AddrSizeOffset; STATIC ACPI_DESCRIPTION_HEADER_INFO AcpiHdrInfo;
>
> /**
> - This function Validates the NameSpace string length.
> + This function validates the NameSpace string length.
>
> @param [in] Ptr Pointer to the start of the buffer.
> @param [in] Context Pointer to context specific information e.g. this @@ -
> 37,24 +38,23 @@ STATIC VOID EFIAPI ValidateNameSpaceStrLen (
> - IN UINT8* Ptr,
> - IN VOID* Context
> - );
> + IN UINT8* Ptr,
> + IN VOID* Context
> + )
> +{
> + UINT16 NameSpaceStrLen;
>
> -/**
> - This function parses the debug device information structure.
> + NameSpaceStrLen = *(UINT16*)Ptr;
>
> - @param [in] Ptr Pointer to the start of the buffer.
> - @param [out] Length Pointer in which the length of the debug
> - device information is returned.
> -**/
> -STATIC
> -VOID
> -EFIAPI
> -DumpDbgDeviceInfo (
> - IN UINT8* Ptr,
> - OUT UINT32* Length
> - );
> + if (NameSpaceStrLen < 2) {
> + IncrementErrorCount ();
> + Print (
> + L"\nERROR: NamespaceString Length = %d. If no Namespace device
> exists, " \
> + L"NamespaceString[] must contain a period '.'",
> + NameSpaceStrLen
> + );
> + }
> +}
>
> /// An ACPI_PARSER array describing the ACPI DBG2 table.
> STATIC CONST ACPI_PARSER Dbg2Parser[] = { @@ -65,10 +65,17 @@ STATIC
> CONST ACPI_PARSER Dbg2Parser[] = {
> (VOID**)&NumberDbgDeviceInfo, NULL, NULL} };
>
> +/// An ACPI_PARSER array describing the debug device information
> +structure /// header.
> +STATIC CONST ACPI_PARSER DbgDevInfoHeaderParser[] = {
> + {L"Revision", 1, 0, L"0x%x", NULL, NULL, NULL, NULL},
> + {L"Length", 2, 1, L"%d", NULL, (VOID**)&DbgDevInfoLen, NULL, NULL} };
> +
> /// An ACPI_PARSER array describing the debug device information.
> STATIC CONST ACPI_PARSER DbgDevInfoParser[] = {
> {L"Revision", 1, 0, L"0x%x", NULL, NULL, NULL, NULL},
> - {L"Length", 2, 1, L"%d", NULL, (VOID**)&DbgDevInfoLen, NULL, NULL},
> + {L"Length", 2, 1, L"%d", NULL, NULL, NULL, NULL},
>
> {L"Generic Address Registers Count", 1, 3, L"0x%x", NULL,
> (VOID**)&GasCount, NULL, NULL},
> @@ -91,108 +98,152 @@ STATIC CONST ACPI_PARSER DbgDevInfoParser[] =
> {
> (VOID**)&AddrSizeOffset, NULL, NULL} };
>
> -/**
> - This function validates the NameSpace string length.
> -
> - @param [in] Ptr Pointer to the start of the buffer.
> - @param [in] Context Pointer to context specific information e.g. this
> - could be a pointer to the ACPI table header.
> -**/
> -STATIC
> -VOID
> -EFIAPI
> -ValidateNameSpaceStrLen (
> - IN UINT8* Ptr,
> - IN VOID* Context
> - )
> -{
> - UINT16 NameSpaceStrLen;
> -
> - NameSpaceStrLen = *(UINT16*)Ptr;
> -
> - if (NameSpaceStrLen < 2) {
> - IncrementErrorCount ();
> - Print (
> - L"\nERROR: NamespaceString Length = %d. If no Namespace device
> exists,\n"
> - L" then NamespaceString[] must contain a period '.'",
> - NameSpaceStrLen
> - );
> - }
> -}
> -
> /**
> This function parses the debug device information structure.
>
> - @param [in] Ptr Pointer to the start of the buffer.
> - @param [out] Length Pointer in which the length of the debug
> - device information is returned.
> + @param [in] Ptr Pointer to the start of the buffer.
> + @param [in] Length Length of the debug device information structure.
> **/
> STATIC
> VOID
> EFIAPI
> DumpDbgDeviceInfo (
> - IN UINT8* Ptr,
> - OUT UINT32* Length
> + IN UINT8* Ptr,
> + IN UINT16 Length
> )
> {
> UINT16 Index;
> - UINT8* DataPtr;
> - UINT32* AddrSize;
> -
> - // Parse the debug device info to get the Length
> - ParseAcpi (
> - FALSE,
> - 0,
> - "Debug Device Info",
> - Ptr,
> - 3, // Length is 2 bytes starting at offset 1
> - PARSER_PARAMS (DbgDevInfoParser)
> - );
> + UINT16 Offset;
>
> ParseAcpi (
> TRUE,
> 2,
> "Debug Device Info",
> Ptr,
> - *DbgDevInfoLen,
> + Length,
> PARSER_PARAMS (DbgDevInfoParser)
> );
>
> - // GAS and Address Size
> + // Check if the values used to control the parsing logic have been
> + // successfully read.
> + if ((GasCount == NULL) ||
> + (NameSpaceStringLength == NULL) ||
> + (NameSpaceStringOffset == NULL) ||
> + (OEMDataLength == NULL) ||
> + (OEMDataOffset == NULL) ||
> + (BaseAddrRegOffset == NULL) ||
> + (AddrSizeOffset == NULL)) {
> + IncrementErrorCount ();
> + Print (
> + L"ERROR: Insufficient Debug Device Information Structure length. " \
> + L"Length = %d.\n",
> + Length
> + );
> + return;
> + }
> +
> + // GAS
> Index = 0;
> - DataPtr = Ptr + (*BaseAddrRegOffset);
> - AddrSize = (UINT32*)(Ptr + (*AddrSizeOffset));
> - while (Index < (*GasCount)) {
> + Offset = *BaseAddrRegOffset;
> + while ((Index++ < *GasCount) &&
> + (Offset < Length)) {
> PrintFieldName (4, L"BaseAddressRegister");
> - DumpGasStruct (DataPtr, 4);
> + Offset += (UINT16)DumpGasStruct (
> + Ptr + Offset,
> + 4,
> + Length - Offset
> + );
> + }
> +
> + // Cross-check the substructure count with the length of the
> + encapsulating // buffer if (GetConsistencyChecking () &&
> + (Index < *GasCount)) {
> + IncrementErrorCount ();
> + Print (
> + L"ERROR: Invalid GAS count. GasCount = %d. DbgDevInfoLen = %d.\n",
> + *GasCount,
> + Length
> + );
> + return;
> + }
> +
> + // Make sure the array of address sizes corresponding to each GAS fit
> + in the // Debug Device Information structure if ((*AddrSizeOffset +
> + (*GasCount * sizeof (UINT32))) > Length) {
> + IncrementErrorCount ();
> + Print (
> + L"ERROR: Invalid GAS count. GasCount = %d. RemainingBufferLength
> = %d. " \
> + L"Parsing of the Debug Device Information structure aborted.\n",
> + *GasCount,
> + Length - *AddrSizeOffset
> + );
> + return;
> + }
> +
> + // Address Size
> + Index = 0;
> + Offset = *AddrSizeOffset;
> + while ((Index++ < *GasCount) &&
> + (Offset < Length)) {
> PrintFieldName (4, L"Address Size");
> - Print (L"0x%x\n", AddrSize[Index]);
> - DataPtr += GAS_LENGTH;
> - Index++;
> + Print (L"0x%x\n", *((UINT32*)(Ptr + Offset)));
> + Offset += sizeof (UINT32);
1. Why use sizof (UINT32) to replace GAS_LENGTH? The MACRO make a good view and meaningful.
> }
>
> // NameSpace String
> Index = 0;
> - DataPtr = Ptr + (*NameSpaceStringOffset);
> + Offset = *NameSpaceStringOffset;
> PrintFieldName (4, L"NameSpace String");
> - while (Index < (*NameSpaceStringLength)) {
> - Print (L"%c", DataPtr[Index++]);
> + while ((Index++ < *NameSpaceStringLength) &&
> + (Offset < Length)) {
> + Print (L"%c", *(Ptr + Offset));
> + Offset++;
> }
> Print (L"\n");
>
> + // Cross-check the string length with the size of the encapsulating
> + // buffer if (GetConsistencyChecking () &&
> + (Index < *NameSpaceStringLength)) {
> + IncrementErrorCount ();
> + Print (
> + L"ERROR: Invalid NameSpaceString length. NameSpaceStringLength = %d.
> " \
> + L"DbgDevInfoLen = %d.\n",
> + *NameSpaceStringLength,
> + Length
> + );
> + return;
> + }
> +
> // OEM Data
> - Index = 0;
> - DataPtr = Ptr + (*OEMDataOffset);
> - PrintFieldName (4, L"OEM Data");
> - while (Index < (*OEMDataLength)) {
> - Print (L"%x ", DataPtr[Index++]);
> - if ((Index & 7) == 0) {
> - Print (L"\n%-*s ", OUTPUT_FIELD_COLUMN_WIDTH, L"");
> + if (*OEMDataOffset != 0) {
> + Index = 0;
> + Offset = *OEMDataOffset;
> + PrintFieldName (4, L"OEM Data");
> + while ((Index++ < *OEMDataLength) &&
> + (Offset < Length)) {
> + Print (L"%x ", *(Ptr + Offset));
> + if ((Index & 7) == 0) {
> + Print (L"\n%-*s ", OUTPUT_FIELD_COLUMN_WIDTH, L"");
> + }
> + Offset++;
> }
> + Print (L"\n");
> }
> - Print (L"\n");
>
> - *Length = *DbgDevInfoLen;
> + // Cross-check the OEM data length with the size of the encapsulating
> + // buffer if (GetConsistencyChecking () &&
> + (Index < *OEMDataLength)) {
> + IncrementErrorCount ();
> + Print (
> + L"ERROR: Invalid OEM Data size. OEMDataLength = %d. " \
> + L"DbgDevInfoLen = %d.\n",
> + *OEMDataLength,
> + Length
> + );
> + }
> }
>
> /**
> @@ -217,8 +268,7 @@ ParseAcpiDbg2 (
> )
> {
> UINT32 Offset;
> - UINT32 DbgDeviceInfoLength;
> - UINT8* DevInfoPtr;
> + UINT32 Index;
>
> if (!Trace) {
> return;
> @@ -232,14 +282,74 @@ ParseAcpiDbg2 (
> AcpiTableLength,
> PARSER_PARAMS (Dbg2Parser)
> );
> - DevInfoPtr = Ptr + Offset;
>
> - while (Offset < AcpiTableLength) {
> - DumpDbgDeviceInfo (
> - DevInfoPtr,
> - &DbgDeviceInfoLength
> + // Check if the values used to control the parsing logic have been
> + // successfully read.
> + if ((OffsetDbgDeviceInfo == NULL) ||
> + (NumberDbgDeviceInfo == NULL)) {
> + IncrementErrorCount ();
> + Print (
> + L"ERROR: Insufficient table length. AcpiTableLength = %d\n",
> + AcpiTableLength
> + );
> + return;
> + }
> +
> + Offset = *OffsetDbgDeviceInfo;
> + Index = 0;
> +
> + while (Index++ < *NumberDbgDeviceInfo) {
> +
> + // Parse the Debug Device Information Structure header to obtain Length
> + ParseAcpi (
> + FALSE,
> + 0,
> + NULL,
> + Ptr + Offset,
> + AcpiTableLength - Offset,
> + PARSER_PARAMS (DbgDevInfoHeaderParser)
> + );
2. Here adding the DbgDevInfoHeaderParser to get the DbgDevInfoLen. This may be conflict with 4/10. 4/10 removes the get header section. I think it is fine to get the whole devinfo if the Length is valid. And the DbgDevInfoHeaderParser isn't required to add.
Thanks,
Zhichao
> +
> + // Check if the values used to control the parsing logic have been
> + // successfully read.
> + if (DbgDevInfoLen == NULL) {
> + IncrementErrorCount ();
> + Print (
> + L"ERROR: Insufficient remaining table buffer length to read the " \
> + L"Debug Device Information structure's 'Length' field. " \
> + L"RemainingTableBufferLength = %d.\n",
> + AcpiTableLength - Offset
> + );
> + return;
> + }
> +
> + // Make sure the Debug Device Information structure lies inside the table.
> + if ((Offset + *DbgDevInfoLen) > AcpiTableLength) {
> + IncrementErrorCount ();
> + Print (
> + L"ERROR: Invalid Debug Device Information structure length. " \
> + L"DbgDevInfoLen = %d. RemainingTableBufferLength = %d. " \
> + L"DBG2 parsing aborted.\n",
> + *DbgDevInfoLen,
> + AcpiTableLength - Offset
> + );
> + return;
> + }
> +
> + DumpDbgDeviceInfo (Ptr + Offset, (*DbgDevInfoLen));
> + Offset += (*DbgDevInfoLen);
> + }
> +
> + // Cross-check the substructure count with the length of the
> + encapsulating // buffer if (GetConsistencyChecking () &&
> + (Index < *NumberDbgDeviceInfo)) {
> + IncrementErrorCount ();
> + Print (
> + L"ERROR: Invalid Debug Device Information structure count. " \
> + L"NumberDbgDeviceInfo = %d. AcpiTableLength = %d.\n",
> + *NumberDbgDeviceInfo,
> + AcpiTableLength
> );
> - Offset += DbgDeviceInfoLength;
> - DevInfoPtr += DbgDeviceInfoLength;
> }
> }
> --
> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#44004): https://edk2.groups.io/g/devel/message/44004
Mute This Topic: https://groups.io/mt/32439516/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list