Re: [PATCH v2 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Fix buffer overflow issue.

["Ni, Ray" <ray.ni@intel.com>]

Reviewed-by: Ray Ni <ray.ni@intel.com>

> -----Original Message-----
> From: Dong, Eric <eric.dong@intel.com>
> Sent: Monday, December 23, 2019 2:48 PM
> To: devel@edk2.groups.io
> Cc: Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>
> Subject: [PATCH v2 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Fix buffer overflow
> issue.
> 
> The size for the array of mSmmMpSyncData->CpuData[] is 0 ~
> mMaxNumberOfCpus -1. But current code may use
> mSmmMpSyncData->CpuData[mMaxNumberOfCpus].
> 
> This patch fixed this issue.
> 
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Eric Dong <eric.dong@intel.com>
> ---
>  UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> index 5ad12db980..e5324f8f84 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> @@ -137,7 +137,7 @@ ReleaseAllAPs (
>  {
> 
>    UINTN                             Index;
> 
> 
> 
> -  for (Index = mMaxNumberOfCpus; Index-- > 0;) {
> 
> +  for (Index = 0; Index < mMaxNumberOfCpus; Index++) {
> 
>      if (IsPresentAp (Index)) {
> 
>        ReleaseSemaphore (mSmmMpSyncData->CpuData[Index].Run);
> 
>      }
> 
> @@ -170,7 +170,7 @@ AllCpusInSmmWithExceptions (
> 
> 
>    CpuData = mSmmMpSyncData->CpuData;
> 
>    ProcessorInfo = gSmmCpuPrivate->ProcessorInfo;
> 
> -  for (Index = mMaxNumberOfCpus; Index-- > 0;) {
> 
> +  for (Index = 0; Index < mMaxNumberOfCpus; Index++) {
> 
>      if (!(*(CpuData[Index].Present)) && ProcessorInfo[Index].ProcessorId !=
> INVALID_APIC_ID) {
> 
>        if (((Exceptions & ARRIVAL_EXCEPTION_DELAYED) != 0) &&
> SmmCpuFeaturesGetSmmRegister (Index, SmmRegSmmDelayed) != 0) {
> 
>          continue;
> 
> @@ -305,7 +305,7 @@ SmmWaitForApArrival (
>      //
> 
>      // Send SMI IPIs to bring outside processors in
> 
>      //
> 
> -    for (Index = mMaxNumberOfCpus; Index-- > 0;) {
> 
> +    for (Index = 0; Index < mMaxNumberOfCpus; Index++) {
> 
>        if (!(*(mSmmMpSyncData->CpuData[Index].Present)) &&
> gSmmCpuPrivate->ProcessorInfo[Index].ProcessorId != INVALID_APIC_ID) {
> 
>          SendSmiIpi ((UINT32)gSmmCpuPrivate-
> >ProcessorInfo[Index].ProcessorId);
> 
>        }
> 
> @@ -361,7 +361,7 @@ WaitForAllAPsNotBusy (
>  {
> 
>    UINTN                             Index;
> 
> 
> 
> -  for (Index = mMaxNumberOfCpus; Index-- > 0;) {
> 
> +  for (Index = 0; Index < mMaxNumberOfCpus; Index++) {
> 
>      //
> 
>      // Ignore BSP and APs which not call in SMM.
> 
>      //
> 
> @@ -619,7 +619,7 @@ BSPHandler (
>      //
> 
>      while (TRUE) {
> 
>        PresentCount = 0;
> 
> -      for (Index = mMaxNumberOfCpus; Index-- > 0;) {
> 
> +      for (Index = 0; Index < mMaxNumberOfCpus; Index++) {
> 
>          if (*(mSmmMpSyncData->CpuData[Index].Present)) {
> 
>            PresentCount ++;
> 
>          }
> 
> @@ -1303,7 +1303,7 @@ InternalSmmStartupAllAPs (
>    }
> 
> 
> 
>    CpuCount = 0;
> 
> -  for (Index = mMaxNumberOfCpus; Index-- > 0;) {
> 
> +  for (Index = 0; Index < mMaxNumberOfCpus; Index++) {
> 
>      if (IsPresentAp (Index)) {
> 
>        CpuCount ++;
> 
> 
> 
> @@ -1335,13 +1335,13 @@ InternalSmmStartupAllAPs (
>    // Here code always use AcquireSpinLock instead of AcquireSpinLockOrFail
> for not
> 
>    // block mode.
> 
>    //
> 
> -  for (Index = mMaxNumberOfCpus; Index-- > 0;) {
> 
> +  for (Index = 0; Index < mMaxNumberOfCpus; Index++) {
> 
>      if (IsPresentAp (Index)) {
> 
>        AcquireSpinLock (mSmmMpSyncData->CpuData[Index].Busy);
> 
>      }
> 
>    }
> 
> 
> 
> -  for (Index = mMaxNumberOfCpus; Index-- > 0;) {
> 
> +  for (Index = 0; Index < mMaxNumberOfCpus; Index++) {
> 
>      if (IsPresentAp (Index)) {
> 
>        mSmmMpSyncData->CpuData[Index].Procedure =
> (EFI_AP_PROCEDURE2) Procedure;
> 
>        mSmmMpSyncData->CpuData[Index].Parameter = ProcedureArguments;
> 
> --
> 2.23.0.windows.1