From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=134.134.136.65; helo=mga03.intel.com; envelope-from=hao.a.wu@intel.com; receiver=edk2-devel@lists.01.org Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 1B77321A02937 for ; Wed, 27 Jun 2018 00:22:35 -0700 (PDT) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga004.jf.intel.com ([10.7.209.38]) by orsmga103.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 27 Jun 2018 00:22:35 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.51,278,1526367600"; d="scan'208";a="211516786" Received: from fmsmsx108.amr.corp.intel.com ([10.18.124.206]) by orsmga004.jf.intel.com with ESMTP; 27 Jun 2018 00:22:35 -0700 Received: from fmsmsx155.amr.corp.intel.com (10.18.116.71) by FMSMSX108.amr.corp.intel.com (10.18.124.206) with Microsoft SMTP Server (TLS) id 14.3.319.2; Wed, 27 Jun 2018 00:22:35 -0700 Received: from shsmsx101.ccr.corp.intel.com (10.239.4.153) by FMSMSX155.amr.corp.intel.com (10.18.116.71) with Microsoft SMTP Server (TLS) id 14.3.319.2; Wed, 27 Jun 2018 00:22:34 -0700 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.87]) by SHSMSX101.ccr.corp.intel.com ([169.254.1.82]) with mapi id 14.03.0319.002; Wed, 27 Jun 2018 15:22:32 +0800 From: "Wu, Hao A" To: "Bi, Dandan" , "edk2-devel@lists.01.org" CC: "Gao, Liming" Thread-Topic: [patch] MdeModulePkg/PerformanceLib: Add NULL pointer check Thread-Index: AQHUDd6xspgx0WUYfUKh5WVzUFWDQqRzsfoA Date: Wed, 27 Jun 2018 07:22:31 +0000 Message-ID: References: <20180627061623.169516-1-dandan.bi@intel.com> <20180627061623.169516-2-dandan.bi@intel.com> In-Reply-To: <20180627061623.169516-2-dandan.bi@intel.com> Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: [patch] MdeModulePkg/PerformanceLib: Add NULL pointer check X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.26 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 27 Jun 2018 07:22:36 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable The patch seems good to me. Reviewed-by: Hao Wu 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 >=20 > 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. >=20 > Cc: Liming Gao > Cc: Hao Wu > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Dandan Bi > --- > .../DxeCorePerformanceLib/DxeCorePerformanceLib.c | 18 ++++++++++++= - > ----- > .../Library/PeiPerformanceLib/PeiPerformanceLib.c | 13 ++++++++----= - > .../SmmCorePerformanceLib/SmmCorePerformanceLib.c | 18 > ++++++++++++------ > 3 files changed, 32 insertions(+), 17 deletions(-) >=20 > 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 ( >=20 > case PERF_EVENTSIGNAL_START_ID: > case PERF_EVENTSIGNAL_END_ID: > case PERF_CALLBACK_START_ID: > case PERF_CALLBACK_END_ID: > - if (String =3D=3D NULL) { > + if (String =3D=3D NULL || Guid =3D=3D NULL) { > return EFI_INVALID_PARAMETER; > } > - // > - // Cache the event guid in string event record when > PcdEdkiiFpdtStringRecordEnableOnly =3D=3D TRUE > - // > - CopyGuid (&ModuleGuid, Guid); > StringPtr =3D String; > + if (AsciiStrLen (String) =3D=3D 0) { > + StringPtr =3D "unknown name"; > + } > if (!PcdGetBool (PcdEdkiiFpdtStringRecordEnableOnly)) { > FpdtRecordPtr.DualGuidStringEvent->Header.Type =3D > FPDT_DUAL_GUID_STRING_EVENT_TYPE; > FpdtRecordPtr.DualGuidStringEvent->Header.Length =3D sizeof > (FPDT_DUAL_GUID_STRING_EVENT_RECORD); > FpdtRecordPtr.DualGuidStringEvent->Header.Revision =3D > FPDT_RECORD_REVISION_1; > FpdtRecordPtr.DualGuidStringEvent->ProgressID =3D PerfId; > @@ -1194,11 +1193,18 @@ InsertFpdtRecord ( > FpdtRecordPtr.DynamicStringEvent->Header.Type =3D > FPDT_DYNAMIC_STRING_EVENT_TYPE; > FpdtRecordPtr.DynamicStringEvent->Header.Length =3D sizeof > (FPDT_DYNAMIC_STRING_EVENT_RECORD); > FpdtRecordPtr.DynamicStringEvent->Header.Revision =3D > FPDT_RECORD_REVISION_1; > FpdtRecordPtr.DynamicStringEvent->ProgressID =3D PerfId; > FpdtRecordPtr.DynamicStringEvent->Timestamp =3D TimeStamp; > - CopyMem (&FpdtRecordPtr.DynamicStringEvent->Guid, &ModuleGuid, > sizeof (FpdtRecordPtr.DynamicStringEvent->Guid)); > + if (Guid !=3D 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) =3D=3D 0) { > StringPtr =3D "unknown name"; > } > CopyStringIntoPerfRecordAndUpdateLength > (FpdtRecordPtr.DynamicStringEvent->String, StringPtr, > &FpdtRecordPtr.DynamicStringEvent->Header.Length); >=20 > 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 =3D (UINT8*)GET_GUID_HOB_DATA (GuidHob); > *PeiPerformanceLogHeader =3D (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 =3D TRUE; > } > - if (!(*PeiPerformanceLogHeader)->HobIsFull && > (*PeiPerformanceLogHeader)->SizeOfAllEntries + RecordSize <=3D > (UINTN)(PeiPerformanceLogEntries * PEI_MAX_RECORD_SIZE)) { > + if (!(*PeiPerformanceLogHeader)->HobIsFull && > (*PeiPerformanceLogHeader)->SizeOfAllEntries + RecordSize <=3D > (PeiPerformanceLogEntries * PEI_MAX_RECORD_SIZE)) { > FpdtRecordPtr->RecordHeader =3D > (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 ( >=20 > case PERF_EVENTSIGNAL_START_ID: > case PERF_EVENTSIGNAL_END_ID: > case PERF_CALLBACK_START_ID: > case PERF_CALLBACK_END_ID: > - if (String !=3D NULL && AsciiStrLen (String) !=3D 0) { > - StringPtr =3D String; > - } else { > + if (String =3D=3D NULL || Guid =3D=3D NULL) { > + return EFI_INVALID_PARAMETER; > + } > + StringPtr =3D String; > + if (AsciiStrLen (String) =3D=3D 0) { > StringPtr =3D "unknown name"; > } > if (!PcdGetBool (PcdEdkiiFpdtStringRecordEnableOnly)) { > FpdtRecordPtr.DualGuidStringEvent->Header.Type =3D > FPDT_DUAL_GUID_STRING_EVENT_TYPE; > FpdtRecordPtr.DualGuidStringEvent->Header.Length =3D sizeof > (FPDT_DUAL_GUID_STRING_EVENT_RECORD); > FpdtRecordPtr.DualGuidStringEvent->Header.Revision =3D > FPDT_RECORD_REVISION_1; > FpdtRecordPtr.DualGuidStringEvent->ProgressID =3D PerfId; > FpdtRecordPtr.DualGuidStringEvent->Timestamp =3D 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; >=20 > 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 ( >=20 > case PERF_EVENTSIGNAL_START_ID: > case PERF_EVENTSIGNAL_END_ID: > case PERF_CALLBACK_START_ID: > case PERF_CALLBACK_END_ID: > - if (String =3D=3D NULL) { > + if (String =3D=3D NULL || Guid =3D=3D NULL) { > return EFI_INVALID_PARAMETER; > } > - // > - // Cache the event guid in string event record when > PcdEdkiiFpdtStringRecordEnableOnly =3D=3D TRUE > - // > - CopyGuid (&ModuleGuid, Guid); > StringPtr =3D String; > + if (AsciiStrLen (String) =3D=3D 0) { > + StringPtr =3D "unknown name"; > + } > if (!PcdGetBool (PcdEdkiiFpdtStringRecordEnableOnly)) { > FpdtRecordPtr.DualGuidStringEvent->Header.Type =3D > FPDT_DUAL_GUID_STRING_EVENT_TYPE; > FpdtRecordPtr.DualGuidStringEvent->Header.Length =3D sizeof > (FPDT_DUAL_GUID_STRING_EVENT_RECORD); > FpdtRecordPtr.DualGuidStringEvent->Header.Revision =3D > FPDT_RECORD_REVISION_1; > FpdtRecordPtr.DualGuidStringEvent->ProgressID =3D PerfId; > @@ -732,11 +731,18 @@ InsertFpdtRecord ( > FpdtRecordPtr.DynamicStringEvent->Header.Type =3D > FPDT_DYNAMIC_STRING_EVENT_TYPE; > FpdtRecordPtr.DynamicStringEvent->Header.Length =3D sizeof > (FPDT_DYNAMIC_STRING_EVENT_RECORD); > FpdtRecordPtr.DynamicStringEvent->Header.Revision =3D > FPDT_RECORD_REVISION_1; > FpdtRecordPtr.DynamicStringEvent->ProgressID =3D PerfId; > FpdtRecordPtr.DynamicStringEvent->Timestamp =3D TimeStamp; > - CopyMem (&FpdtRecordPtr.DynamicStringEvent->Guid, &ModuleGuid, > sizeof (FpdtRecordPtr.DynamicStringEvent->Guid)); > + if (Guid !=3D 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) =3D=3D 0) { > StringPtr =3D "unknown name"; > } > CopyStringIntoPerfRecordAndUpdateLength > (FpdtRecordPtr.DynamicStringEvent->String, StringPtr, > &FpdtRecordPtr.DynamicStringEvent->Header.Length); >=20 > -- > 2.14.3.windows.1