From: "Gao, Zhichao" <zhichao.gao@intel.com>
To: Krzysztof Koch <krzysztof.koch@arm.com>,
"devel@edk2.groups.io" <devel@edk2.groups.io>
Cc: "Carsey, Jaben" <jaben.carsey@intel.com>,
"Ni, Ray" <ray.ni@intel.com>,
"Sami.Mujawar@arm.com" <Sami.Mujawar@arm.com>,
"Matteo.Carlini@arm.com" <Matteo.Carlini@arm.com>,
"nd@arm.com" <nd@arm.com>
Subject: Re: [PATCH v1 1/6] ShellPkg: acpiview: DBG2: Prevent buffer overruns
Date: Mon, 5 Aug 2019 06:48:27 +0000 [thread overview]
Message-ID: <3CE959C139B4C44DBEA1810E3AA6F9000B81EA99@SHSMSX101.ccr.corp.intel.com> (raw)
In-Reply-To: <20190801084407.48712-2-krzysztof.koch@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)'
>
next prev parent reply other threads:[~2019-08-05 7:00 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
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 9:33 ` [edk2-devel] " Alexei Fedorov
2019-08-05 6:48 ` Gao, Zhichao [this message]
2019-08-05 8:21 ` Krzysztof Koch
2019-08-06 7:43 ` [edk2-devel] " Gao, Zhichao
2019-08-06 10:44 ` Krzysztof Koch
2019-08-07 1:52 ` Gao, Zhichao
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
2019-08-06 4:58 ` Gao, Zhichao
2019-08-01 8:44 ` [PATCH v1 3/6] ShellPkg: acpiview: IORT: " 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
2019-08-01 9:32 ` [edk2-devel] " Alexei Fedorov
2019-08-01 8:44 ` [PATCH v1 5/6] ShellPkg: acpiview: PPTT: " 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
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
2019-08-06 8:10 ` Gao, Zhichao
2019-08-07 8:46 ` Sami Mujawar
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=3CE959C139B4C44DBEA1810E3AA6F9000B81EA99@SHSMSX101.ccr.corp.intel.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox