The patch seems good to me.
Reviewed-by: Hao Wu <hao.a...@intel.com>
Best Regards,
Hao Wu
> -----Original Message-----
> From: Bi, Dandan
> Sent: Wednesday, June 27, 2018 2:16 PM
> To: edk2-devel@lists.01.org
> Cc: Gao, Liming; Wu, Hao A
> Subject: [patch] MdeModulePkg/PerformanceLib: Add NULL pointer check
>
> 1. Add NULL pointer check for the "Guid" parameter
> when handle FPDT_DUAL_GUID_STRING_EVENT_TYPE.
> 2. Make the code logic in DxeCore/SmmCore/PeiPerformanceLib
> aligned when handle FPDT_DUAL_GUID_STRING_EVENT_TYPE.
>
> Cc: Liming Gao <liming....@intel.com>
> Cc: Hao Wu <hao.a...@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Dandan Bi <dandan...@intel.com>
> ---
> .../DxeCorePerformanceLib/DxeCorePerformanceLib.c | 18 ++++++++++++-
> -----
> .../Library/PeiPerformanceLib/PeiPerformanceLib.c | 13 ++++++++-----
> .../SmmCorePerformanceLib/SmmCorePerformanceLib.c | 18
> ++++++++++++------
> 3 files changed, 32 insertions(+), 17 deletions(-)
>
> diff --git
> a/MdeModulePkg/Library/DxeCorePerformanceLib/DxeCorePerformanceLib.c
> b/MdeModulePkg/Library/DxeCorePerformanceLib/DxeCorePerformanceLib.c
> index efff5134c7b..6e0c328c635 100644
> ---
> a/MdeModulePkg/Library/DxeCorePerformanceLib/DxeCorePerformanceLib.c
> +++
> b/MdeModulePkg/Library/DxeCorePerformanceLib/DxeCorePerformanceLib.c
> @@ -1109,18 +1109,17 @@ InsertFpdtRecord (
>
> case PERF_EVENTSIGNAL_START_ID:
> case PERF_EVENTSIGNAL_END_ID:
> case PERF_CALLBACK_START_ID:
> case PERF_CALLBACK_END_ID:
> - if (String == NULL) {
> + if (String == NULL || Guid == NULL) {
> return EFI_INVALID_PARAMETER;
> }
> - //
> - // Cache the event guid in string event record when
> PcdEdkiiFpdtStringRecordEnableOnly == TRUE
> - //
> - CopyGuid (&ModuleGuid, Guid);
> StringPtr = String;
> + if (AsciiStrLen (String) == 0) {
> + StringPtr = "unknown name";
> + }
> if (!PcdGetBool (PcdEdkiiFpdtStringRecordEnableOnly)) {
> FpdtRecordPtr.DualGuidStringEvent->Header.Type =
> FPDT_DUAL_GUID_STRING_EVENT_TYPE;
> FpdtRecordPtr.DualGuidStringEvent->Header.Length = sizeof
> (FPDT_DUAL_GUID_STRING_EVENT_RECORD);
> FpdtRecordPtr.DualGuidStringEvent->Header.Revision =
> FPDT_RECORD_REVISION_1;
> FpdtRecordPtr.DualGuidStringEvent->ProgressID = PerfId;
> @@ -1194,11 +1193,18 @@ InsertFpdtRecord (
> FpdtRecordPtr.DynamicStringEvent->Header.Type =
> FPDT_DYNAMIC_STRING_EVENT_TYPE;
> FpdtRecordPtr.DynamicStringEvent->Header.Length = sizeof
> (FPDT_DYNAMIC_STRING_EVENT_RECORD);
> FpdtRecordPtr.DynamicStringEvent->Header.Revision =
> FPDT_RECORD_REVISION_1;
> FpdtRecordPtr.DynamicStringEvent->ProgressID = PerfId;
> FpdtRecordPtr.DynamicStringEvent->Timestamp = TimeStamp;
> - CopyMem (&FpdtRecordPtr.DynamicStringEvent->Guid, &ModuleGuid,
> sizeof (FpdtRecordPtr.DynamicStringEvent->Guid));
> + if (Guid != NULL) {
> + //
> + // Cache the event guid in string event record.
> + //
> + CopyMem (&FpdtRecordPtr.DynamicStringEvent->Guid, Guid, sizeof
> (FpdtRecordPtr.DynamicStringEvent->Guid));
> + } else {
> + CopyMem (&FpdtRecordPtr.DynamicStringEvent->Guid, &ModuleGuid,
> sizeof (FpdtRecordPtr.DynamicStringEvent->Guid));
> + }
> if (AsciiStrLen (StringPtr) == 0) {
> StringPtr = "unknown name";
> }
> CopyStringIntoPerfRecordAndUpdateLength
> (FpdtRecordPtr.DynamicStringEvent->String, StringPtr,
> &FpdtRecordPtr.DynamicStringEvent->Header.Length);
>
> diff --git a/MdeModulePkg/Library/PeiPerformanceLib/PeiPerformanceLib.c
> b/MdeModulePkg/Library/PeiPerformanceLib/PeiPerformanceLib.c
> index cd1b0e34ef7..3a44a0a438e 100644
> --- a/MdeModulePkg/Library/PeiPerformanceLib/PeiPerformanceLib.c
> +++ b/MdeModulePkg/Library/PeiPerformanceLib/PeiPerformanceLib.c
> @@ -75,14 +75,14 @@ GetFpdtRecordPtr (
> //
> // PEI Performance HOB was found, then return the existing one.
> //
> PeiFirmwarePerformance = (UINT8*)GET_GUID_HOB_DATA (GuidHob);
> *PeiPerformanceLogHeader = (FPDT_PEI_EXT_PERF_HEADER
> *)PeiFirmwarePerformance;
> - if (!(*PeiPerformanceLogHeader)->HobIsFull &&
> (*PeiPerformanceLogHeader)->SizeOfAllEntries + RecordSize >
> (UINTN)(PeiPerformanceLogEntries * PEI_MAX_RECORD_SIZE)) {
> + if (!(*PeiPerformanceLogHeader)->HobIsFull &&
> (*PeiPerformanceLogHeader)->SizeOfAllEntries + RecordSize >
> (PeiPerformanceLogEntries * PEI_MAX_RECORD_SIZE)) {
> (*PeiPerformanceLogHeader)->HobIsFull = TRUE;
> }
> - if (!(*PeiPerformanceLogHeader)->HobIsFull &&
> (*PeiPerformanceLogHeader)->SizeOfAllEntries + RecordSize <=
> (UINTN)(PeiPerformanceLogEntries * PEI_MAX_RECORD_SIZE)) {
> + if (!(*PeiPerformanceLogHeader)->HobIsFull &&
> (*PeiPerformanceLogHeader)->SizeOfAllEntries + RecordSize <=
> (PeiPerformanceLogEntries * PEI_MAX_RECORD_SIZE)) {
> FpdtRecordPtr->RecordHeader =
> (EFI_ACPI_5_0_FPDT_PERFORMANCE_RECORD_HEADER
> *)(PeiFirmwarePerformance + sizeof (FPDT_PEI_EXT_PERF_HEADER) +
> (*PeiPerformanceLogHeader)->SizeOfAllEntries);
> break;
> }
> //
> // Previous HOB is used, then find next one.
> @@ -421,23 +421,26 @@ InsertFpdtRecord (
>
> case PERF_EVENTSIGNAL_START_ID:
> case PERF_EVENTSIGNAL_END_ID:
> case PERF_CALLBACK_START_ID:
> case PERF_CALLBACK_END_ID:
> - if (String != NULL && AsciiStrLen (String) != 0) {
> - StringPtr = String;
> - } else {
> + if (String == NULL || Guid == NULL) {
> + return EFI_INVALID_PARAMETER;
> + }
> + StringPtr = String;
> + if (AsciiStrLen (String) == 0) {
> StringPtr = "unknown name";
> }
> if (!PcdGetBool (PcdEdkiiFpdtStringRecordEnableOnly)) {
> FpdtRecordPtr.DualGuidStringEvent->Header.Type =
> FPDT_DUAL_GUID_STRING_EVENT_TYPE;
> FpdtRecordPtr.DualGuidStringEvent->Header.Length = sizeof
> (FPDT_DUAL_GUID_STRING_EVENT_RECORD);
> FpdtRecordPtr.DualGuidStringEvent->Header.Revision =
> FPDT_RECORD_REVISION_1;
> FpdtRecordPtr.DualGuidStringEvent->ProgressID = PerfId;
> FpdtRecordPtr.DualGuidStringEvent->Timestamp = TimeStamp;
> CopyMem (&FpdtRecordPtr.DualGuidStringEvent->Guid1, ModuleGuid,
> sizeof (FpdtRecordPtr.DualGuidStringEvent->Guid1));
> CopyMem (&FpdtRecordPtr.DualGuidStringEvent->Guid2, Guid, sizeof
> (FpdtRecordPtr.DualGuidStringEvent->Guid2));
> + CopyStringIntoPerfRecordAndUpdateLength
> (FpdtRecordPtr.DualGuidStringEvent->String, StringPtr,
> &FpdtRecordPtr.DualGuidStringEvent->Header.Length);
> }
> break;
>
> case PERF_EVENT_ID:
> case PERF_FUNCTION_START_ID:
> diff --git
> a/MdeModulePkg/Library/SmmCorePerformanceLib/SmmCorePerformanceLib
> .c
> b/MdeModulePkg/Library/SmmCorePerformanceLib/SmmCorePerformanceLib
> .c
> index 0c00fb51e82..f18c3fb60df 100644
> ---
> a/MdeModulePkg/Library/SmmCorePerformanceLib/SmmCorePerformanceLib
> .c
> +++
> b/MdeModulePkg/Library/SmmCorePerformanceLib/SmmCorePerformanceLib
> .c
> @@ -647,18 +647,17 @@ InsertFpdtRecord (
>
> case PERF_EVENTSIGNAL_START_ID:
> case PERF_EVENTSIGNAL_END_ID:
> case PERF_CALLBACK_START_ID:
> case PERF_CALLBACK_END_ID:
> - if (String == NULL) {
> + if (String == NULL || Guid == NULL) {
> return EFI_INVALID_PARAMETER;
> }
> - //
> - // Cache the event guid in string event record when
> PcdEdkiiFpdtStringRecordEnableOnly == TRUE
> - //
> - CopyGuid (&ModuleGuid, Guid);
> StringPtr = String;
> + if (AsciiStrLen (String) == 0) {
> + StringPtr = "unknown name";
> + }
> if (!PcdGetBool (PcdEdkiiFpdtStringRecordEnableOnly)) {
> FpdtRecordPtr.DualGuidStringEvent->Header.Type =
> FPDT_DUAL_GUID_STRING_EVENT_TYPE;
> FpdtRecordPtr.DualGuidStringEvent->Header.Length = sizeof
> (FPDT_DUAL_GUID_STRING_EVENT_RECORD);
> FpdtRecordPtr.DualGuidStringEvent->Header.Revision =
> FPDT_RECORD_REVISION_1;
> FpdtRecordPtr.DualGuidStringEvent->ProgressID = PerfId;
> @@ -732,11 +731,18 @@ InsertFpdtRecord (
> FpdtRecordPtr.DynamicStringEvent->Header.Type =
> FPDT_DYNAMIC_STRING_EVENT_TYPE;
> FpdtRecordPtr.DynamicStringEvent->Header.Length = sizeof
> (FPDT_DYNAMIC_STRING_EVENT_RECORD);
> FpdtRecordPtr.DynamicStringEvent->Header.Revision =
> FPDT_RECORD_REVISION_1;
> FpdtRecordPtr.DynamicStringEvent->ProgressID = PerfId;
> FpdtRecordPtr.DynamicStringEvent->Timestamp = TimeStamp;
> - CopyMem (&FpdtRecordPtr.DynamicStringEvent->Guid, &ModuleGuid,
> sizeof (FpdtRecordPtr.DynamicStringEvent->Guid));
> + if (Guid != NULL) {
> + //
> + // Cache the event guid in string event record.
> + //
> + CopyMem (&FpdtRecordPtr.DynamicStringEvent->Guid, Guid, sizeof
> (FpdtRecordPtr.DynamicStringEvent->Guid));
> + } else {
> + CopyMem (&FpdtRecordPtr.DynamicStringEvent->Guid, &ModuleGuid,
> sizeof (FpdtRecordPtr.DynamicStringEvent->Guid));
> + }
> if (AsciiStrLen (StringPtr) == 0) {
> StringPtr = "unknown name";
> }
> CopyStringIntoPerfRecordAndUpdateLength
> (FpdtRecordPtr.DynamicStringEvent->String, StringPtr,
> &FpdtRecordPtr.DynamicStringEvent->Header.Length);
>
> --
> 2.14.3.windows.1
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
