[edk2-devel] [PATCH v1 10/11] ShellPkg: acpiview: GTDT: Add error-checking in the parsing logic
Krzysztof Koch
krzysztof.koch at arm.com
Fri Jul 12 06:52:42 UTC 2019
1. Check if the global pointers (in the scope of this ACPI table parser)
have been successfully updated before they are later used to control
the parsing logic.
2. Test against buffer overruns.
3. Allow silencing ACPI table content validation errors which do not
cause table parsing to fail.
4. Remove redundant forward function declarations by repositioning
blocks of code.
5. Convert a 'do-while' loop for parsing GTDT table body into a 'while'
block for consistency with other table parsers.
Signed-off-by: Krzysztof Koch <krzysztof.koch at arm.com>
---
Changes can be seen at: https://github.com/KrzysztofKoch1/edk2/commit/8c2ed18c7f1c44620eb86e1c9117cbccee8938ce
Notes:
v1:
- improve the logic in the GTDT parser [Krzysztof]
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser.c | 294 +++++++++++---------
1 file changed, 170 insertions(+), 124 deletions(-)
diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser.c
index 3b05ff3015d4a3af62dd9fab057c32369a456267..4e8e6f3eb50596823827d20dbb72314a583d0931 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser.c
@@ -12,6 +12,10 @@
#include <Library/UefiLib.h>
#include "AcpiParser.h"
#include "AcpiTableParser.h"
+#include "AcpiView.h"
+
+// "The number of GT Block Timers must be less than or equal to 8"
+#define GT_BLOCK_TIMER_COUNT_MAX 8
// Local variables
STATIC CONST UINT32* GtdtPlatformTimerCount;
@@ -20,7 +24,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;
/**
@@ -36,7 +39,21 @@ EFIAPI
ValidateGtBlockTimerCount (
IN UINT8* Ptr,
IN VOID* Context
- );
+ )
+{
+ UINT32 BlockTimerCount;
+
+ BlockTimerCount = *(UINT32*)Ptr;
+
+ if (BlockTimerCount > GT_BLOCK_TIMER_COUNT_MAX) {
+ IncrementErrorCount ();
+ Print (
+ L"\nERROR: Timer Count = %d. Max Timer Count is %d.",
+ BlockTimerCount,
+ GT_BLOCK_TIMER_COUNT_MAX
+ );
+ }
+}
/**
This function validates the GT Frame Number.
@@ -51,7 +68,21 @@ EFIAPI
ValidateGtFrameNumber (
IN UINT8* Ptr,
IN VOID* Context
- );
+ )
+{
+ UINT8 FrameNumber;
+
+ FrameNumber = *(UINT8*)Ptr;
+
+ if (FrameNumber >= GT_BLOCK_TIMER_COUNT_MAX) {
+ IncrementErrorCount ();
+ Print (
+ L"\nERROR: GT Frame Number = %d. GT Frame Number must be in range 0-%d.",
+ FrameNumber,
+ GT_BLOCK_TIMER_COUNT_MAX - 1
+ );
+ }
+}
/**
An ACPI_PARSER array describing the ACPI GTDT Table.
@@ -96,7 +127,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,
@@ -134,115 +165,71 @@ STATIC CONST ACPI_PARSER SBSAGenericWatchdogParser[] = {
{L"Watchdog Timer Flags", 4, 24, L"0x%x", NULL, NULL, NULL, NULL}
};
-/**
- This function validates the GT Block timer count.
-
- @param [in] Ptr Pointer to the start of the field data.
- @param [in] Context Pointer to context specific information e.g. this
- could be a pointer to the ACPI table header.
-**/
-STATIC
-VOID
-EFIAPI
-ValidateGtBlockTimerCount (
- IN UINT8* Ptr,
- IN VOID* Context
- )
-{
- UINT32 BlockTimerCount;
-
- BlockTimerCount = *(UINT32*)Ptr;
-
- if (BlockTimerCount > 8) {
- IncrementErrorCount ();
- Print (
- L"\nERROR: Timer Count = %d. Max Timer Count is 8.",
- BlockTimerCount
- );
- }
-}
-
-/**
- This function validates the GT Frame Number.
-
- @param [in] Ptr Pointer to the start of the field data.
- @param [in] Context Pointer to context specific information e.g. this
- could be a pointer to the ACPI table header.
-**/
-STATIC
-VOID
-EFIAPI
-ValidateGtFrameNumber (
- IN UINT8* Ptr,
- IN VOID* Context
- )
-{
- UINT8 FrameNumber;
-
- FrameNumber = *(UINT8*)Ptr;
-
- if (FrameNumber > 7) {
- IncrementErrorCount ();
- Print (
- L"\nERROR: GT Frame Number = %d. GT Frame Number must be in range 0-7.",
- FrameNumber
- );
- }
-}
-
/**
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++;
- }
+ // Check if the values used to control the parsing logic have been
+ // successfully read.
+ if ((GtBlockTimerCount == NULL) ||
+ (GtBlockTimerOffset == NULL)) {
+ IncrementErrorCount ();
+ Print (
+ L"ERROR: Insufficient GT Block Structure length. Length = %d.\n",
+ Length
+ );
+ return;
+ }
- if (Length != 0) {
- IncrementErrorCount ();
- Print (
- L"ERROR:GT Block Timer length mismatch. Unparsed %d bytes.\n",
- Length
- );
- }
+ Offset = *GtBlockTimerOffset;
+ Index = 0;
+
+ // 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)
+ );
+ }
+
+ // Cross-check the substructure count with the length of the encapsulating
+ // buffer
+ if (GetConsistencyChecking () &&
+ (Index < *GtBlockTimerCount)) {
+ IncrementErrorCount ();
+ Print (
+ L"ERROR: Invalid GT Block Timer count. GtBlockTimerCount = %d. " \
+ L"GtBlockBufferSize = %d.\n",
+ *GtBlockTimerCount,
+ Length
+ );
}
}
@@ -295,6 +282,7 @@ ParseAcpiGtdt (
)
{
UINT32 Index;
+ UINT32 Offset;
UINT8* TimerPtr;
if (!Trace) {
@@ -310,36 +298,94 @@ 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)
+ // Check if the values used to control the parsing logic have been
+ // successfully read.
+ if ((GtdtPlatformTimerCount == NULL) ||
+ (GtdtPlatformTimerOffset == NULL)) {
+ IncrementErrorCount ();
+ Print (
+ L"ERROR: Insufficient table length. AcpiTableLength = %d.\n",
+ AcpiTableLength
+ );
+ return;
+ }
+
+ 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)
+ );
+
+ // Check if the values used to control the parsing logic have been
+ // successfully read.
+ if ((PlatformTimerType == NULL) ||
+ (PlatformTimerLength == NULL)) {
+ IncrementErrorCount ();
+ Print (
+ L"ERROR: Insufficient remaining table buffer length to read the " \
+ L"Platform Timer Structure header. Length = %d.\n",
+ 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:
+ return;
+ }
+
+ // 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
+ );
+ 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:
+ if (GetConsistencyChecking ()) {
IncrementErrorCount ();
Print (
- L"ERROR: INVALID Platform Timer Type = %d\n",
+ L"ERROR: Invalid Platform Timer Type = %d\n",
*PlatformTimerType
);
- break;
- } // switch
- TimerPtr += (*PlatformTimerLength);
- Index++;
- } while (Index < *GtdtPlatformTimerCount);
+ }
+ break;
+ } // switch
+
+ TimerPtr += *PlatformTimerLength;
+ Offset += *PlatformTimerLength;
+ } // while
+
+ // Cross-check the substructure count with the length of the encapsulating
+ // buffer
+ if (GetConsistencyChecking () &&
+ (Index < *GtdtPlatformTimerCount)) {
+ IncrementErrorCount ();
+ Print (
+ L"ERROR: Invalid Platform Timer Structure count. " \
+ L"GtdtPlatformTimerCount = %d. AcpiTableLength = %d.\n",
+ *GtdtPlatformTimerCount,
+ AcpiTableLength
+ );
}
}
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#43650): https://edk2.groups.io/g/devel/message/43650
Mute This Topic: https://groups.io/mt/32439515/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