From: "Wu, Hao A" <hao.a.wu@intel.com>
To: "Bi, Dandan" <dandan.bi@intel.com>,
"edk2-devel@lists.01.org" <edk2-devel@lists.01.org>
Cc: "Gao, Liming" <liming.gao@intel.com>
Subject: Re: [patch] MdeModulePkg/PerformanceLib: Add NULL pointer check
Date: Wed, 27 Jun 2018 07:22:31 +0000 [thread overview]
Message-ID: <B80AF82E9BFB8E4FBD8C89DA810C6A0931E0B77E@SHSMSX104.ccr.corp.intel.com> (raw)
In-Reply-To: <20180627061623.169516-2-dandan.bi@intel.com>
The patch seems good to me.
Reviewed-by: Hao Wu <hao.a.wu@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.gao@intel.com>
> Cc: Hao Wu <hao.a.wu@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Dandan Bi <dandan.bi@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
next prev parent reply other threads:[~2018-06-27 7:22 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-06-27 6:16 [patch] MdeModulePkg/PiSmmCore: Remove SMM Perf entry Dandan Bi
2018-06-27 6:16 ` [patch] MdeModulePkg/PerformanceLib: Add NULL pointer check Dandan Bi
2018-06-27 7:22 ` Wu, Hao A [this message]
2018-06-27 6:59 ` [patch] MdeModulePkg/PiSmmCore: Remove SMM Perf entry Gao, Liming
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=B80AF82E9BFB8E4FBD8C89DA810C6A0931E0B77E@SHSMSX104.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