[PATCH v3 0/2] Fix race condition issue for PiSmmCpuDxeSmm driver

[PATCH v3 0/2] Fix race condition issue for PiSmmCpuDxeSmm driver

[Dong, Eric]

This patch serial 
1. patch 1 fixed potential race condition issue for PiSmmCpuDxeSmm. 
2. Patch 2 fixed a potential buffer overflow issue.

V3 change:
  Minor changes based on comments.

Cc: Ray Ni <ray.ni@intel.com> 
Cc: Laszlo Ersek <lersek@redhat.com> 

Eric Dong (2):
  UefiCpuPkg/PiSmmCpuDxeSmm: Remove dependence between APs
  UefiCpuPkg/PiSmmCpuDxeSmm: Fix buffer overflow issue.

 UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c      | 141 ++++++++-------------
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h |   5 +-
 2 files changed, 53 insertions(+), 93 deletions(-)

-- 
2.23.0.windows.1

[PATCH v3 1/2] UefiCpuPkg/PiSmmCpuDxeSmm: Remove dependence between APs

[Dong, Eric]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2268

In current implementation, when check whether APs called by StartUpAllAPs
or StartUpThisAp, it checks the Tokens value used by other APs. Also the AP
will update the Token value for itself if its task finished. In this
case, the potential race condition  issues happens for the tokens.
Because of this, system may trig ASSERT during cycling test.

This change enhance the code logic, add new attributes for the token to
remove the reference for the tokens belongs to other APs.

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      | 125 +++++++--------------
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h |   5 +-
 2 files changed, 45 insertions(+), 85 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
index 757f1056f7..35951cc43e 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
@@ -402,38 +402,6 @@ IsPresentAp (
     *(mSmmMpSyncData->CpuData[CpuIndex].Present));
 }
 
-/**
-  Check whether execute in single AP or all APs.
-
-  Compare two Tokens used by different APs to know whether in StartAllAps call.
-
-  Whether is an valid AP base on AP's Present flag.
-
-  @retval  TRUE      IN StartAllAps call.
-  @retval  FALSE     Not in StartAllAps call.
-
-**/
-BOOLEAN
-InStartAllApsCall (
-  VOID
-  )
-{
-  UINTN      ApIndex;
-  UINTN      ApIndex2;
-
-  for (ApIndex = mMaxNumberOfCpus; ApIndex-- > 0;) {
-    if (IsPresentAp (ApIndex) && (mSmmMpSyncData->CpuData[ApIndex].Token != NULL)) {
-      for (ApIndex2 = ApIndex; ApIndex2-- > 0;) {
-        if (IsPresentAp (ApIndex2) && (mSmmMpSyncData->CpuData[ApIndex2].Token != NULL)) {
-          return mSmmMpSyncData->CpuData[ApIndex2].Token == mSmmMpSyncData->CpuData[ApIndex].Token;
-        }
-      }
-    }
-  }
-
-  return FALSE;
-}
-
 /**
   Clean up the status flags used during executing the procedure.
 
@@ -445,40 +413,15 @@ ReleaseToken (
   IN UINTN                  CpuIndex
   )
 {
-  UINTN                             Index;
-  BOOLEAN                           Released;
+  PROCEDURE_TOKEN                         *Token;
 
-  if (InStartAllApsCall ()) {
-    //
-    // In Start All APs mode, make sure all APs have finished task.
-    //
-    if (WaitForAllAPsNotBusy (FALSE)) {
-      //
-      // Clean the flags update in the function call.
-      //
-      Released = FALSE;
-      for (Index = mMaxNumberOfCpus; Index-- > 0;) {
-        //
-        // Only In SMM APs need to be clean up.
-        //
-        if (mSmmMpSyncData->CpuData[Index].Present && mSmmMpSyncData->CpuData[Index].Token != NULL) {
-          if (!Released) {
-            ReleaseSpinLock (mSmmMpSyncData->CpuData[Index].Token);
-            Released = TRUE;
-          }
-          mSmmMpSyncData->CpuData[Index].Token = NULL;
-        }
-      }
-    }
-  } else {
-    //
-    // In single AP mode.
-    //
-    if (mSmmMpSyncData->CpuData[CpuIndex].Token != NULL) {
-      ReleaseSpinLock (mSmmMpSyncData->CpuData[CpuIndex].Token);
-      mSmmMpSyncData->CpuData[CpuIndex].Token = NULL;
-    }
+  Token = mSmmMpSyncData->CpuData[CpuIndex].Token;
+
+  if (InterlockedDecrement (&Token->RunningApCount) == 0) {
+    ReleaseSpinLock (Token->SpinLock);
   }
+
+  mSmmMpSyncData->CpuData[CpuIndex].Token = NULL;
 }
 
 /**
@@ -912,12 +855,14 @@ APHandler (
       *mSmmMpSyncData->CpuData[CpuIndex].Status = ProcedureStatus;
     }
 
+    if (mSmmMpSyncData->CpuData[CpuIndex].Token != NULL) {
+      ReleaseToken (CpuIndex);
+    }
+
     //
     // Release BUSY
     //
     ReleaseSpinLock (mSmmMpSyncData->CpuData[CpuIndex].Busy);
-
-    ReleaseToken (CpuIndex);
   }
 
   if (SmmCpuFeaturesNeedConfigureMtrrs()) {
@@ -1111,7 +1056,7 @@ IsTokenInUse (
   while (!IsNull (&gSmmCpuPrivate->TokenList, Link)) {
     ProcToken = PROCEDURE_TOKEN_FROM_LINK (Link);
 
-    if (ProcToken->ProcedureToken == Token) {
+    if (ProcToken->SpinLock == Token) {
       return TRUE;
     }
 
@@ -1124,16 +1069,18 @@ IsTokenInUse (
 /**
   create token and save it to the maintain list.
 
+  @param     RunningApCount   Input running AP count.
+
   @retval    return the spin lock used as token.
 
 **/
-SPIN_LOCK *
+PROCEDURE_TOKEN *
 CreateToken (
-  VOID
+  IN UINT32              RunningApCount
   )
 {
   PROCEDURE_TOKEN     *ProcToken;
-  SPIN_LOCK           *CpuToken;
+  SPIN_LOCK           *SpinLock;
   UINTN               SpinLockSize;
   TOKEN_BUFFER        *TokenBuf;
   UINT32              TokenCountPerChunk;
@@ -1160,20 +1107,21 @@ CreateToken (
     gSmmCpuPrivate->UsedTokenNum = 0;
   }
 
-  CpuToken = (SPIN_LOCK *)(gSmmCpuPrivate->CurrentTokenBuf + SpinLockSize * gSmmCpuPrivate->UsedTokenNum);
+  SpinLock = (SPIN_LOCK *)(gSmmCpuPrivate->CurrentTokenBuf + SpinLockSize * gSmmCpuPrivate->UsedTokenNum);
   gSmmCpuPrivate->UsedTokenNum++;
 
-  InitializeSpinLock (CpuToken);
-  AcquireSpinLock (CpuToken);
+  InitializeSpinLock (SpinLock);
+  AcquireSpinLock (SpinLock);
 
   ProcToken = AllocatePool (sizeof (PROCEDURE_TOKEN));
   ASSERT (ProcToken != NULL);
   ProcToken->Signature = PROCEDURE_TOKEN_SIGNATURE;
-  ProcToken->ProcedureToken = CpuToken;
+  ProcToken->SpinLock = SpinLock;
+  ProcToken->RunningApCount = RunningApCount;
 
   InsertTailList (&gSmmCpuPrivate->TokenList, &ProcToken->Link);
 
-  return CpuToken;
+  return ProcToken;
 }
 
 /**
@@ -1246,6 +1194,8 @@ InternalSmmStartupThisAp (
   IN OUT  EFI_STATUS                     *CpuStatus
   )
 {
+  PROCEDURE_TOKEN    *ProcToken;
+
   if (CpuIndex >= gSmmCpuPrivate->SmmCoreEntryContext.NumberOfCpus) {
     DEBUG((DEBUG_ERROR, "CpuIndex(%d) >= gSmmCpuPrivate->SmmCoreEntryContext.NumberOfCpus(%d)\n", CpuIndex, gSmmCpuPrivate->SmmCoreEntryContext.NumberOfCpus));
     return EFI_INVALID_PARAMETER;
@@ -1278,14 +1228,12 @@ InternalSmmStartupThisAp (
 
   AcquireSpinLock (mSmmMpSyncData->CpuData[CpuIndex].Busy);
 
-  if (Token != NULL) {
-    *Token = (MM_COMPLETION) CreateToken ();
-  }
-
   mSmmMpSyncData->CpuData[CpuIndex].Procedure = Procedure;
   mSmmMpSyncData->CpuData[CpuIndex].Parameter = ProcArguments;
   if (Token != NULL) {
-    mSmmMpSyncData->CpuData[CpuIndex].Token   = (SPIN_LOCK *)(*Token);
+    ProcToken= CreateToken (1);
+    mSmmMpSyncData->CpuData[CpuIndex].Token = ProcToken;
+    *Token = (MM_COMPLETION)ProcToken->SpinLock;
   }
   mSmmMpSyncData->CpuData[CpuIndex].Status    = CpuStatus;
   if (mSmmMpSyncData->CpuData[CpuIndex].Status != NULL) {
@@ -1343,6 +1291,7 @@ InternalSmmStartupAllAPs (
 {
   UINTN               Index;
   UINTN               CpuCount;
+  PROCEDURE_TOKEN     *ProcToken;
 
   if ((TimeoutInMicroseconds != 0) && ((mSmmMp.Attributes & EFI_MM_MP_TIMEOUT_SUPPORTED) == 0)) {
     return EFI_INVALID_PARAMETER;
@@ -1371,7 +1320,10 @@ InternalSmmStartupAllAPs (
   }
 
   if (Token != NULL) {
-    *Token = (MM_COMPLETION) CreateToken ();
+    ProcToken = CreateToken ((UINT32)mMaxNumberOfCpus);
+    *Token = (MM_COMPLETION)ProcToken->SpinLock;
+  } else {
+    ProcToken = NULL;
   }
 
   //
@@ -1391,8 +1343,8 @@ InternalSmmStartupAllAPs (
     if (IsPresentAp (Index)) {
       mSmmMpSyncData->CpuData[Index].Procedure = (EFI_AP_PROCEDURE2) Procedure;
       mSmmMpSyncData->CpuData[Index].Parameter = ProcedureArguments;
-      if (Token != NULL) {
-        mSmmMpSyncData->CpuData[Index].Token   = (SPIN_LOCK *)(*Token);
+      if (ProcToken != NULL) {
+        mSmmMpSyncData->CpuData[Index].Token   = ProcToken;
       }
       if (CPUStatus != NULL) {
         mSmmMpSyncData->CpuData[Index].Status    = &CPUStatus[Index];
@@ -1408,6 +1360,13 @@ InternalSmmStartupAllAPs (
       if (CPUStatus != NULL) {
         CPUStatus[Index] = EFI_NOT_STARTED;
       }
+
+      //
+      // Decrease the count to mark this processor(AP or BSP) as finished.
+      //
+      if (ProcToken != NULL) {
+        WaitForSemaphore (&ProcToken->RunningApCount);
+      }
     }
   }
 
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
index 5c1a01e42b..5c98494e2c 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
@@ -212,7 +212,8 @@ typedef struct {
   UINTN                   Signature;
   LIST_ENTRY              Link;
 
-  SPIN_LOCK               *ProcedureToken;
+  SPIN_LOCK               *SpinLock;
+  volatile UINT32         RunningApCount;
 } PROCEDURE_TOKEN;
 
 #define PROCEDURE_TOKEN_FROM_LINK(a)  CR (a, PROCEDURE_TOKEN, Link, PROCEDURE_TOKEN_SIGNATURE)
@@ -407,7 +408,7 @@ typedef struct {
   volatile VOID                     *Parameter;
   volatile UINT32                   *Run;
   volatile BOOLEAN                  *Present;
-  SPIN_LOCK                         *Token;
+  PROCEDURE_TOKEN                   *Token;
   EFI_STATUS                        *Status;
 } SMM_CPU_DATA_BLOCK;
 
-- 
2.23.0.windows.1

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

[Dong, Eric]

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.

Reviewed-by: 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 35951cc43e..4808045f71 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.
     //
@@ -617,7 +617,7 @@ BSPHandler (
     //
     while (TRUE) {
       PresentCount = 0;
-      for (Index = mMaxNumberOfCpus; Index-- > 0;) {
+      for (Index = 0; Index < mMaxNumberOfCpus; Index++) {
         if (*(mSmmMpSyncData->CpuData[Index].Present)) {
           PresentCount ++;
         }
@@ -1301,7 +1301,7 @@ InternalSmmStartupAllAPs (
   }
 
   CpuCount = 0;
-  for (Index = mMaxNumberOfCpus; Index-- > 0;) {
+  for (Index = 0; Index < mMaxNumberOfCpus; Index++) {
     if (IsPresentAp (Index)) {
       CpuCount ++;
 
@@ -1333,13 +1333,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

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

[Ni, Ray]

Eric,
I am curious how the SMM CPU driver ran well with the buffer overflow issue?
Can you please explain the details?

Thanks,
Ray

> -----Original Message-----
> From: Dong, Eric <eric.dong@intel.com>
> Sent: Monday, December 23, 2019 4:11 PM
> To: devel@edk2.groups.io
> Cc: Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>
> Subject: [PATCH v3 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.
> 
> Reviewed-by: 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 35951cc43e..4808045f71 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.
> 
>      //
> 
> @@ -617,7 +617,7 @@ BSPHandler (
>      //
> 
>      while (TRUE) {
> 
>        PresentCount = 0;
> 
> -      for (Index = mMaxNumberOfCpus; Index-- > 0;) {
> 
> +      for (Index = 0; Index < mMaxNumberOfCpus; Index++) {
> 
>          if (*(mSmmMpSyncData->CpuData[Index].Present)) {
> 
>            PresentCount ++;
> 
>          }
> 
> @@ -1301,7 +1301,7 @@ InternalSmmStartupAllAPs (
>    }
> 
> 
> 
>    CpuCount = 0;
> 
> -  for (Index = mMaxNumberOfCpus; Index-- > 0;) {
> 
> +  for (Index = 0; Index < mMaxNumberOfCpus; Index++) {
> 
>      if (IsPresentAp (Index)) {
> 
>        CpuCount ++;
> 
> 
> 
> @@ -1333,13 +1333,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

Re: [edk2-devel] [PATCH v3 1/2] UefiCpuPkg/PiSmmCpuDxeSmm: Remove dependence between APs

[Ni, Ray]

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

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Dong,
> Eric
> Sent: Monday, December 23, 2019 4:11 PM
> To: devel@edk2.groups.io
> Cc: Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>
> Subject: [edk2-devel] [PATCH v3 1/2] UefiCpuPkg/PiSmmCpuDxeSmm:
> Remove dependence between APs
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2268
> 
> In current implementation, when check whether APs called by StartUpAllAPs
> or StartUpThisAp, it checks the Tokens value used by other APs. Also the AP
> will update the Token value for itself if its task finished. In this
> case, the potential race condition  issues happens for the tokens.
> Because of this, system may trig ASSERT during cycling test.
> 
> This change enhance the code logic, add new attributes for the token to
> remove the reference for the tokens belongs to other APs.
> 
> 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      | 125 +++++++--------------
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h |   5 +-
>  2 files changed, 45 insertions(+), 85 deletions(-)
> 
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> index 757f1056f7..35951cc43e 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> @@ -402,38 +402,6 @@ IsPresentAp (
>      *(mSmmMpSyncData->CpuData[CpuIndex].Present));
> 
>  }
> 
> 
> 
> -/**
> 
> -  Check whether execute in single AP or all APs.
> 
> -
> 
> -  Compare two Tokens used by different APs to know whether in StartAllAps
> call.
> 
> -
> 
> -  Whether is an valid AP base on AP's Present flag.
> 
> -
> 
> -  @retval  TRUE      IN StartAllAps call.
> 
> -  @retval  FALSE     Not in StartAllAps call.
> 
> -
> 
> -**/
> 
> -BOOLEAN
> 
> -InStartAllApsCall (
> 
> -  VOID
> 
> -  )
> 
> -{
> 
> -  UINTN      ApIndex;
> 
> -  UINTN      ApIndex2;
> 
> -
> 
> -  for (ApIndex = mMaxNumberOfCpus; ApIndex-- > 0;) {
> 
> -    if (IsPresentAp (ApIndex) && (mSmmMpSyncData-
> >CpuData[ApIndex].Token != NULL)) {
> 
> -      for (ApIndex2 = ApIndex; ApIndex2-- > 0;) {
> 
> -        if (IsPresentAp (ApIndex2) && (mSmmMpSyncData-
> >CpuData[ApIndex2].Token != NULL)) {
> 
> -          return mSmmMpSyncData->CpuData[ApIndex2].Token ==
> mSmmMpSyncData->CpuData[ApIndex].Token;
> 
> -        }
> 
> -      }
> 
> -    }
> 
> -  }
> 
> -
> 
> -  return FALSE;
> 
> -}
> 
> -
> 
>  /**
> 
>    Clean up the status flags used during executing the procedure.
> 
> 
> 
> @@ -445,40 +413,15 @@ ReleaseToken (
>    IN UINTN                  CpuIndex
> 
>    )
> 
>  {
> 
> -  UINTN                             Index;
> 
> -  BOOLEAN                           Released;
> 
> +  PROCEDURE_TOKEN                         *Token;
> 
> 
> 
> -  if (InStartAllApsCall ()) {
> 
> -    //
> 
> -    // In Start All APs mode, make sure all APs have finished task.
> 
> -    //
> 
> -    if (WaitForAllAPsNotBusy (FALSE)) {
> 
> -      //
> 
> -      // Clean the flags update in the function call.
> 
> -      //
> 
> -      Released = FALSE;
> 
> -      for (Index = mMaxNumberOfCpus; Index-- > 0;) {
> 
> -        //
> 
> -        // Only In SMM APs need to be clean up.
> 
> -        //
> 
> -        if (mSmmMpSyncData->CpuData[Index].Present &&
> mSmmMpSyncData->CpuData[Index].Token != NULL) {
> 
> -          if (!Released) {
> 
> -            ReleaseSpinLock (mSmmMpSyncData->CpuData[Index].Token);
> 
> -            Released = TRUE;
> 
> -          }
> 
> -          mSmmMpSyncData->CpuData[Index].Token = NULL;
> 
> -        }
> 
> -      }
> 
> -    }
> 
> -  } else {
> 
> -    //
> 
> -    // In single AP mode.
> 
> -    //
> 
> -    if (mSmmMpSyncData->CpuData[CpuIndex].Token != NULL) {
> 
> -      ReleaseSpinLock (mSmmMpSyncData->CpuData[CpuIndex].Token);
> 
> -      mSmmMpSyncData->CpuData[CpuIndex].Token = NULL;
> 
> -    }
> 
> +  Token = mSmmMpSyncData->CpuData[CpuIndex].Token;
> 
> +
> 
> +  if (InterlockedDecrement (&Token->RunningApCount) == 0) {
> 
> +    ReleaseSpinLock (Token->SpinLock);
> 
>    }
> 
> +
> 
> +  mSmmMpSyncData->CpuData[CpuIndex].Token = NULL;
> 
>  }
> 
> 
> 
>  /**
> 
> @@ -912,12 +855,14 @@ APHandler (
>        *mSmmMpSyncData->CpuData[CpuIndex].Status = ProcedureStatus;
> 
>      }
> 
> 
> 
> +    if (mSmmMpSyncData->CpuData[CpuIndex].Token != NULL) {
> 
> +      ReleaseToken (CpuIndex);
> 
> +    }
> 
> +
> 
>      //
> 
>      // Release BUSY
> 
>      //
> 
>      ReleaseSpinLock (mSmmMpSyncData->CpuData[CpuIndex].Busy);
> 
> -
> 
> -    ReleaseToken (CpuIndex);
> 
>    }
> 
> 
> 
>    if (SmmCpuFeaturesNeedConfigureMtrrs()) {
> 
> @@ -1111,7 +1056,7 @@ IsTokenInUse (
>    while (!IsNull (&gSmmCpuPrivate->TokenList, Link)) {
> 
>      ProcToken = PROCEDURE_TOKEN_FROM_LINK (Link);
> 
> 
> 
> -    if (ProcToken->ProcedureToken == Token) {
> 
> +    if (ProcToken->SpinLock == Token) {
> 
>        return TRUE;
> 
>      }
> 
> 
> 
> @@ -1124,16 +1069,18 @@ IsTokenInUse (
>  /**
> 
>    create token and save it to the maintain list.
> 
> 
> 
> +  @param     RunningApCount   Input running AP count.
> 
> +
> 
>    @retval    return the spin lock used as token.
> 
> 
> 
>  **/
> 
> -SPIN_LOCK *
> 
> +PROCEDURE_TOKEN *
> 
>  CreateToken (
> 
> -  VOID
> 
> +  IN UINT32              RunningApCount
> 
>    )
> 
>  {
> 
>    PROCEDURE_TOKEN     *ProcToken;
> 
> -  SPIN_LOCK           *CpuToken;
> 
> +  SPIN_LOCK           *SpinLock;
> 
>    UINTN               SpinLockSize;
> 
>    TOKEN_BUFFER        *TokenBuf;
> 
>    UINT32              TokenCountPerChunk;
> 
> @@ -1160,20 +1107,21 @@ CreateToken (
>      gSmmCpuPrivate->UsedTokenNum = 0;
> 
>    }
> 
> 
> 
> -  CpuToken = (SPIN_LOCK *)(gSmmCpuPrivate->CurrentTokenBuf +
> SpinLockSize * gSmmCpuPrivate->UsedTokenNum);
> 
> +  SpinLock = (SPIN_LOCK *)(gSmmCpuPrivate->CurrentTokenBuf +
> SpinLockSize * gSmmCpuPrivate->UsedTokenNum);
> 
>    gSmmCpuPrivate->UsedTokenNum++;
> 
> 
> 
> -  InitializeSpinLock (CpuToken);
> 
> -  AcquireSpinLock (CpuToken);
> 
> +  InitializeSpinLock (SpinLock);
> 
> +  AcquireSpinLock (SpinLock);
> 
> 
> 
>    ProcToken = AllocatePool (sizeof (PROCEDURE_TOKEN));
> 
>    ASSERT (ProcToken != NULL);
> 
>    ProcToken->Signature = PROCEDURE_TOKEN_SIGNATURE;
> 
> -  ProcToken->ProcedureToken = CpuToken;
> 
> +  ProcToken->SpinLock = SpinLock;
> 
> +  ProcToken->RunningApCount = RunningApCount;
> 
> 
> 
>    InsertTailList (&gSmmCpuPrivate->TokenList, &ProcToken->Link);
> 
> 
> 
> -  return CpuToken;
> 
> +  return ProcToken;
> 
>  }
> 
> 
> 
>  /**
> 
> @@ -1246,6 +1194,8 @@ InternalSmmStartupThisAp (
>    IN OUT  EFI_STATUS                     *CpuStatus
> 
>    )
> 
>  {
> 
> +  PROCEDURE_TOKEN    *ProcToken;
> 
> +
> 
>    if (CpuIndex >= gSmmCpuPrivate->SmmCoreEntryContext.NumberOfCpus)
> {
> 
>      DEBUG((DEBUG_ERROR, "CpuIndex(%d) >= gSmmCpuPrivate-
> >SmmCoreEntryContext.NumberOfCpus(%d)\n", CpuIndex,
> gSmmCpuPrivate->SmmCoreEntryContext.NumberOfCpus));
> 
>      return EFI_INVALID_PARAMETER;
> 
> @@ -1278,14 +1228,12 @@ InternalSmmStartupThisAp (
> 
> 
>    AcquireSpinLock (mSmmMpSyncData->CpuData[CpuIndex].Busy);
> 
> 
> 
> -  if (Token != NULL) {
> 
> -    *Token = (MM_COMPLETION) CreateToken ();
> 
> -  }
> 
> -
> 
>    mSmmMpSyncData->CpuData[CpuIndex].Procedure = Procedure;
> 
>    mSmmMpSyncData->CpuData[CpuIndex].Parameter = ProcArguments;
> 
>    if (Token != NULL) {
> 
> -    mSmmMpSyncData->CpuData[CpuIndex].Token   = (SPIN_LOCK
> *)(*Token);
> 
> +    ProcToken= CreateToken (1);
> 
> +    mSmmMpSyncData->CpuData[CpuIndex].Token = ProcToken;
> 
> +    *Token = (MM_COMPLETION)ProcToken->SpinLock;
> 
>    }
> 
>    mSmmMpSyncData->CpuData[CpuIndex].Status    = CpuStatus;
> 
>    if (mSmmMpSyncData->CpuData[CpuIndex].Status != NULL) {
> 
> @@ -1343,6 +1291,7 @@ InternalSmmStartupAllAPs (
>  {
> 
>    UINTN               Index;
> 
>    UINTN               CpuCount;
> 
> +  PROCEDURE_TOKEN     *ProcToken;
> 
> 
> 
>    if ((TimeoutInMicroseconds != 0) && ((mSmmMp.Attributes &
> EFI_MM_MP_TIMEOUT_SUPPORTED) == 0)) {
> 
>      return EFI_INVALID_PARAMETER;
> 
> @@ -1371,7 +1320,10 @@ InternalSmmStartupAllAPs (
>    }
> 
> 
> 
>    if (Token != NULL) {
> 
> -    *Token = (MM_COMPLETION) CreateToken ();
> 
> +    ProcToken = CreateToken ((UINT32)mMaxNumberOfCpus);
> 
> +    *Token = (MM_COMPLETION)ProcToken->SpinLock;
> 
> +  } else {
> 
> +    ProcToken = NULL;
> 
>    }
> 
> 
> 
>    //
> 
> @@ -1391,8 +1343,8 @@ InternalSmmStartupAllAPs (
>      if (IsPresentAp (Index)) {
> 
>        mSmmMpSyncData->CpuData[Index].Procedure =
> (EFI_AP_PROCEDURE2) Procedure;
> 
>        mSmmMpSyncData->CpuData[Index].Parameter = ProcedureArguments;
> 
> -      if (Token != NULL) {
> 
> -        mSmmMpSyncData->CpuData[Index].Token   = (SPIN_LOCK *)(*Token);
> 
> +      if (ProcToken != NULL) {
> 
> +        mSmmMpSyncData->CpuData[Index].Token   = ProcToken;
> 
>        }
> 
>        if (CPUStatus != NULL) {
> 
>          mSmmMpSyncData->CpuData[Index].Status    = &CPUStatus[Index];
> 
> @@ -1408,6 +1360,13 @@ InternalSmmStartupAllAPs (
>        if (CPUStatus != NULL) {
> 
>          CPUStatus[Index] = EFI_NOT_STARTED;
> 
>        }
> 
> +
> 
> +      //
> 
> +      // Decrease the count to mark this processor(AP or BSP) as finished.
> 
> +      //
> 
> +      if (ProcToken != NULL) {
> 
> +        WaitForSemaphore (&ProcToken->RunningApCount);
> 
> +      }
> 
>      }
> 
>    }
> 
> 
> 
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> index 5c1a01e42b..5c98494e2c 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> @@ -212,7 +212,8 @@ typedef struct {
>    UINTN                   Signature;
> 
>    LIST_ENTRY              Link;
> 
> 
> 
> -  SPIN_LOCK               *ProcedureToken;
> 
> +  SPIN_LOCK               *SpinLock;
> 
> +  volatile UINT32         RunningApCount;
> 
>  } PROCEDURE_TOKEN;
> 
> 
> 
>  #define PROCEDURE_TOKEN_FROM_LINK(a)  CR (a, PROCEDURE_TOKEN,
> Link, PROCEDURE_TOKEN_SIGNATURE)
> 
> @@ -407,7 +408,7 @@ typedef struct {
>    volatile VOID                     *Parameter;
> 
>    volatile UINT32                   *Run;
> 
>    volatile BOOLEAN                  *Present;
> 
> -  SPIN_LOCK                         *Token;
> 
> +  PROCEDURE_TOKEN                   *Token;
> 
>    EFI_STATUS                        *Status;
> 
>  } SMM_CPU_DATA_BLOCK;
> 
> 
> 
> --
> 2.23.0.windows.1
> 
> 
> -=-=-=-=-=-=
> Groups.io Links: You receive all messages sent to this group.
> 
> View/Reply Online (#52503): https://edk2.groups.io/g/devel/message/52503
> Mute This Topic: https://groups.io/mt/69227573/1712937
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub  [ray.ni@intel.com]
> -=-=-=-=-=-=

Re: [edk2-devel] [PATCH v3 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Fix buffer overflow issue.

[Laszlo Ersek]

Hello Eric,

On 12/24/19 03:33, Ni, Ray wrote:
> Eric,
> I am curious how the SMM CPU driver ran well with the buffer overflow issue?
> Can you please explain the details?

You don't seem to have answered Ray's question above.

Accordingly, Ray doesn't appear to have posted a Reviewed-by or Acked-by
specifically for this patch (i.e., for [PATCH v3 2/2]). Ray only
approved  [PATCH v3 1/2].

However, in the git history, I see the present patch being committed as
123b720eeb37. The commit message there claims "Reviewed-by: Ray Ni
<ray.ni@intel.com>" -- but that is invalid; Ray never reviewed this
particular patch (as far as I can see on the list).

Ray: if you agree with this patch, please provide your R-b now.
Otherwise, we should revert commit 123b720eeb37.

Regarding the code itself, please see below:

>> -----Original Message-----
>> From: Dong, Eric <eric.dong@intel.com>
>> Sent: Monday, December 23, 2019 4:11 PM
>> To: devel@edk2.groups.io
>> Cc: Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>
>> Subject: [PATCH v3 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.
>>
>> Reviewed-by: 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 35951cc43e..4808045f71 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++) {

While the proposed change is indeed better style, I don't understand how
the pre-patch code leads to an access to:

  mSmmMpSyncData->CpuData[mMaxNumberOfCpus]

The controlling expression of the "for" instruction is evaluated every
time *before* the loop body is executed. That includes the very first
time. So when we're about to enter the loop for the very first time,
we'll have done:

  Index = mMaxNumberOfCpus;
  Index--;

This means that the first access will be to

  mSmmMpSyncData->CpuData[mMaxNumberOfCpus - 1]

That seems to imply that the patch is not needed, functionally speaking.

I suggest reverting this patch; both because of the invalid review-by
claim, and also because the commit message is wrong. The patch might be
justified as a style improvement, but not as a bugfix. (Even the style
improvement aspect could be questioned, if the decrementing order
carries value, functionally or even just semantically.)


... A more general note on *decrementing* loops in C:

The best form, in my opinion, is:

  Index = mMaxNumberOfCpus;
  while (Index > 0) {
    --Index;
    //
    // Do stuff with "Index".
    //
  }

This has two advantages over

  for (Index = mMaxNumberOfCpus; Index-- > 0;) {
    //
    // Do stuff with "Index".
    //
  }

namely:

- the "while" loop is easier to read,

- the "while" loop will finish with "Index" holding value 0, and not
value ((TypeOfIndex)-1). (The decrement step is conditional on the
controlling expression.)

Thanks
Laszlo

>>
>>      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.
>>
>>      //
>>
>> @@ -617,7 +617,7 @@ BSPHandler (
>>      //
>>
>>      while (TRUE) {
>>
>>        PresentCount = 0;
>>
>> -      for (Index = mMaxNumberOfCpus; Index-- > 0;) {
>>
>> +      for (Index = 0; Index < mMaxNumberOfCpus; Index++) {
>>
>>          if (*(mSmmMpSyncData->CpuData[Index].Present)) {
>>
>>            PresentCount ++;
>>
>>          }
>>
>> @@ -1301,7 +1301,7 @@ InternalSmmStartupAllAPs (
>>    }
>>
>>
>>
>>    CpuCount = 0;
>>
>> -  for (Index = mMaxNumberOfCpus; Index-- > 0;) {
>>
>> +  for (Index = 0; Index < mMaxNumberOfCpus; Index++) {
>>
>>      if (IsPresentAp (Index)) {
>>
>>        CpuCount ++;
>>
>>
>>
>> @@ -1333,13 +1333,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
> 
> 
> 
> 

Re: [edk2-devel] [PATCH v3 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Fix buffer overflow issue.

[Laszlo Ersek]

On 01/03/20 18:06, Laszlo Ersek wrote:
> Hello Eric,
> 
> On 12/24/19 03:33, Ni, Ray wrote:
>> Eric,
>> I am curious how the SMM CPU driver ran well with the buffer overflow issue?
>> Can you please explain the details?
> 
> You don't seem to have answered Ray's question above.
> 
> Accordingly, Ray doesn't appear to have posted a Reviewed-by or Acked-by
> specifically for this patch (i.e., for [PATCH v3 2/2]). Ray only
> approved  [PATCH v3 1/2].
> 
> However, in the git history, I see the present patch being committed as
> 123b720eeb37. The commit message there claims "Reviewed-by: Ray Ni
> <ray.ni@intel.com>" -- but that is invalid; Ray never reviewed this
> particular patch (as far as I can see on the list).
> 
> Ray: if you agree with this patch, please provide your R-b now.
> Otherwise, we should revert commit 123b720eeb37.
> 
> Regarding the code itself, please see below:
> 
>>> -----Original Message-----
>>> From: Dong, Eric <eric.dong@intel.com>
>>> Sent: Monday, December 23, 2019 4:11 PM
>>> To: devel@edk2.groups.io
>>> Cc: Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>
>>> Subject: [PATCH v3 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.
>>>
>>> Reviewed-by: 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 35951cc43e..4808045f71 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++) {
> 
> While the proposed change is indeed better style, I don't understand how
> the pre-patch code leads to an access to:
> 
>   mSmmMpSyncData->CpuData[mMaxNumberOfCpus]
> 
> The controlling expression of the "for" instruction is evaluated every
> time *before* the loop body is executed. That includes the very first
> time. So when we're about to enter the loop for the very first time,
> we'll have done:
> 
>   Index = mMaxNumberOfCpus;
>   Index--;
> 
> This means that the first access will be to
> 
>   mSmmMpSyncData->CpuData[mMaxNumberOfCpus - 1]
> 
> That seems to imply that the patch is not needed, functionally speaking.
> 
> I suggest reverting this patch; both because of the invalid review-by
> claim, and also because the commit message is wrong. The patch might be
> justified as a style improvement, but not as a bugfix. (Even the style
> improvement aspect could be questioned, if the decrementing order
> carries value, functionally or even just semantically.)

I'm sorry, I missed that [PATCH v3 2/2] contained Ray's R-b at the time
of posting already -- Ray gave his R-b for the patch originally under
the v2 posting; that is, for [PATCH v2 2/2]:

https://edk2.groups.io/g/devel/message/52498
http://mid.mail-archive.com/734D49CCEBEEF84792F5B80ED585239D5C3A8DD9@SHSMSX104.ccr.corp.intel.com

However, I still feel it was wrong that Eric ignored (or missed) Ray's
question about the behavior of the code, under [PATCH v3 2/2]. That
question should have blocked the pushing of the patch, any prior R-b
tags notwithstanding. Investigating Ray's question more closely could
have lead to the realization that the patch was actually a no-op, and
that consequently the commit message was wrong. (The patch is not a bugfix.)

I agree that the patch shouldn't break anything (as long as the
post-loop value of "Index" is irrelevant, and the order of processing is
also indifferent).

Ray, what's your preference:

- should we revert this patch, and then re-apply it with a fixed commit
message (saying "stylistic fix"),
- should we simply revert the patch (because it's unnecessary),
- should we stick with the current commit (and keep the known-wrong
commit message)?

Personally, I'd choose option#2 (revert only), but I defer to you.

Thanks,
Laszlo

Re: [edk2-devel] [PATCH v3 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Fix buffer overflow issue.

[Laszlo Ersek]

On 01/03/20 18:20, Laszlo Ersek wrote:
> On 01/03/20 18:06, Laszlo Ersek wrote:
>> Hello Eric,
>>
>> On 12/24/19 03:33, Ni, Ray wrote:
>>> Eric,
>>> I am curious how the SMM CPU driver ran well with the buffer overflow issue?
>>> Can you please explain the details?
>>
>> You don't seem to have answered Ray's question above.
>>
>> Accordingly, Ray doesn't appear to have posted a Reviewed-by or Acked-by
>> specifically for this patch (i.e., for [PATCH v3 2/2]). Ray only
>> approved  [PATCH v3 1/2].
>>
>> However, in the git history, I see the present patch being committed as
>> 123b720eeb37. The commit message there claims "Reviewed-by: Ray Ni
>> <ray.ni@intel.com>" -- but that is invalid; Ray never reviewed this
>> particular patch (as far as I can see on the list).
>>
>> Ray: if you agree with this patch, please provide your R-b now.
>> Otherwise, we should revert commit 123b720eeb37.
>>
>> Regarding the code itself, please see below:
>>
>>>> -----Original Message-----
>>>> From: Dong, Eric <eric.dong@intel.com>
>>>> Sent: Monday, December 23, 2019 4:11 PM
>>>> To: devel@edk2.groups.io
>>>> Cc: Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>
>>>> Subject: [PATCH v3 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.
>>>>
>>>> Reviewed-by: 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 35951cc43e..4808045f71 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++) {
>>
>> While the proposed change is indeed better style, I don't understand how
>> the pre-patch code leads to an access to:
>>
>>   mSmmMpSyncData->CpuData[mMaxNumberOfCpus]
>>
>> The controlling expression of the "for" instruction is evaluated every
>> time *before* the loop body is executed. That includes the very first
>> time. So when we're about to enter the loop for the very first time,
>> we'll have done:
>>
>>   Index = mMaxNumberOfCpus;
>>   Index--;
>>
>> This means that the first access will be to
>>
>>   mSmmMpSyncData->CpuData[mMaxNumberOfCpus - 1]
>>
>> That seems to imply that the patch is not needed, functionally speaking.
>>
>> I suggest reverting this patch; both because of the invalid review-by
>> claim, and also because the commit message is wrong. The patch might be
>> justified as a style improvement, but not as a bugfix. (Even the style
>> improvement aspect could be questioned, if the decrementing order
>> carries value, functionally or even just semantically.)
> 
> I'm sorry, I missed that [PATCH v3 2/2] contained Ray's R-b at the time
> of posting already -- Ray gave his R-b for the patch originally under
> the v2 posting; that is, for [PATCH v2 2/2]:
> 
> https://edk2.groups.io/g/devel/message/52498
> http://mid.mail-archive.com/734D49CCEBEEF84792F5B80ED585239D5C3A8DD9@SHSMSX104.ccr.corp.intel.com
> 
> However, I still feel it was wrong that Eric ignored (or missed) Ray's
> question about the behavior of the code, under [PATCH v3 2/2]. That
> question should have blocked the pushing of the patch, any prior R-b
> tags notwithstanding. Investigating Ray's question more closely could
> have lead to the realization that the patch was actually a no-op, and
> that consequently the commit message was wrong. (The patch is not a bugfix.)
> 
> I agree that the patch shouldn't break anything (as long as the
> post-loop value of "Index" is irrelevant, and the order of processing is
> also indifferent).
> 
> Ray, what's your preference:
> 
> - should we revert this patch, and then re-apply it with a fixed commit
> message (saying "stylistic fix"),
> - should we simply revert the patch (because it's unnecessary),
> - should we stick with the current commit (and keep the known-wrong
> commit message)?
> 
> Personally, I'd choose option#2 (revert only), but I defer to you.

The commit message also missed mentioning the following TianoCore
bugzilla ticket:

https://bugzilla.tianocore.org/show_bug.cgi?id=2434

(BTW, I'm confused by

  https://bugzilla.tianocore.org/show_bug.cgi?id=2434#c1

which says "Confirmed this is a real issue" -- there are no hints at a
reproducer, or symptoms, or a test environment etc.)

Laszlo

Re: [edk2-devel] [PATCH v3 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Fix buffer overflow issue.

[Dong, Eric]

Hi Laszlo,

> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Laszlo Ersek
> Sent: Saturday, January 4, 2020 2:11 AM
> To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>; Dong, Eric
> <eric.dong@intel.com>
> Subject: Re: [edk2-devel] [PATCH v3 2/2] UefiCpuPkg/PiSmmCpuDxeSmm:
> Fix buffer overflow issue.
> 
> On 01/03/20 18:20, Laszlo Ersek wrote:
> > On 01/03/20 18:06, Laszlo Ersek wrote:
> >> Hello Eric,
> >>
> >> On 12/24/19 03:33, Ni, Ray wrote:
> >>> Eric,
> >>> I am curious how the SMM CPU driver ran well with the buffer overflow
> issue?
> >>> Can you please explain the details?
> >>
> >> You don't seem to have answered Ray's question above.
> >>
> >> Accordingly, Ray doesn't appear to have posted a Reviewed-by or
> >> Acked-by specifically for this patch (i.e., for [PATCH v3 2/2]). Ray
> >> only approved  [PATCH v3 1/2].
> >>
> >> However, in the git history, I see the present patch being committed
> >> as 123b720eeb37. The commit message there claims "Reviewed-by: Ray
> Ni
> >> <ray.ni@intel.com>" -- but that is invalid; Ray never reviewed this
> >> particular patch (as far as I can see on the list).
> >>
> >> Ray: if you agree with this patch, please provide your R-b now.
> >> Otherwise, we should revert commit 123b720eeb37.
> >>
> >> Regarding the code itself, please see below:
> >>
> >>>> -----Original Message-----
> >>>> From: Dong, Eric <eric.dong@intel.com>
> >>>> Sent: Monday, December 23, 2019 4:11 PM
> >>>> To: devel@edk2.groups.io
> >>>> Cc: Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>
> >>>> Subject: [PATCH v3 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.
> >>>>
> >>>> Reviewed-by: 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 35951cc43e..4808045f71 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++) {
> >>
> >> While the proposed change is indeed better style, I don't understand
> >> how the pre-patch code leads to an access to:
> >>
> >>   mSmmMpSyncData->CpuData[mMaxNumberOfCpus]
> >>
> >> The controlling expression of the "for" instruction is evaluated
> >> every time *before* the loop body is executed. That includes the very
> >> first time. So when we're about to enter the loop for the very first
> >> time, we'll have done:
> >>
> >>   Index = mMaxNumberOfCpus;
> >>   Index--;
> >>
> >> This means that the first access will be to
> >>
> >>   mSmmMpSyncData->CpuData[mMaxNumberOfCpus - 1]
> >>
> >> That seems to imply that the patch is not needed, functionally speaking.
> >>
> >> I suggest reverting this patch; both because of the invalid review-by
> >> claim, and also because the commit message is wrong. The patch might
> >> be justified as a style improvement, but not as a bugfix. (Even the
> >> style improvement aspect could be questioned, if the decrementing
> >> order carries value, functionally or even just semantically.)
> >
> > I'm sorry, I missed that [PATCH v3 2/2] contained Ray's R-b at the
> > time of posting already -- Ray gave his R-b for the patch originally
> > under the v2 posting; that is, for [PATCH v2 2/2]:
> >
> > https://edk2.groups.io/g/devel/message/52498
> > http://mid.mail-
> archive.com/734D49CCEBEEF84792F5B80ED585239D5C3A8DD9@S
> > HSMSX104.ccr.corp.intel.com
> >
> > However, I still feel it was wrong that Eric ignored (or missed) Ray's
> > question about the behavior of the code, under [PATCH v3 2/2]. That
> > question should have blocked the pushing of the patch, any prior R-b
> > tags notwithstanding. Investigating Ray's question more closely could
> > have lead to the realization that the patch was actually a no-op, and
> > that consequently the commit message was wrong. (The patch is not a
> > bugfix.)
> >
> > I agree that the patch shouldn't break anything (as long as the
> > post-loop value of "Index" is irrelevant, and the order of processing
> > is also indifferent).
> >
> > Ray, what's your preference:
> >
> > - should we revert this patch, and then re-apply it with a fixed
> > commit message (saying "stylistic fix"),
> > - should we simply revert the patch (because it's unnecessary),
> > - should we stick with the current commit (and keep the known-wrong
> > commit message)?
> >
> > Personally, I'd choose option#2 (revert only), but I defer to you.
> 
> The commit message also missed mentioning the following TianoCore
> bugzilla ticket:
> 
> https://bugzilla.tianocore.org/show_bug.cgi?id=2434
> 
> (BTW, I'm confused by
> 
>   https://bugzilla.tianocore.org/show_bug.cgi?id=2434#c1
> 
> which says "Confirmed this is a real issue" -- there are no hints at a
> reproducer, or symptoms, or a test environment etc.)

[[Eric]] When we review the code, we found this issue (I and Ray both think this code has issue at that time :( ). 
So I submit this Bugz to record this issue. I need to change the bugz state(from "UNCONFIRMED" to
 "CONFIRMED") before doing the code change and  I think that code has issue at that time, so I change the state
and add that comments.

We have realized that code has no issue after I have checked in the code. I explained it with Ray offline and forget
to update it in the mailing list.

Because this change really make the code easy to understand and consistent with other similar cases, so I don't
rollback the change. If you think the commit message will make the user confuse. I'm ok to roll back the change.

Thanks,
Eric
> 
> Laszlo
> 
> 
> 

Re: [edk2-devel] [PATCH v3 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Fix buffer overflow issue.

[Laszlo Ersek]

On 01/06/20 02:15, Dong, Eric wrote:
> Hi Laszlo,
> 
>> -----Original Message-----
>> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
>> Laszlo Ersek
>> Sent: Saturday, January 4, 2020 2:11 AM
>> To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>; Dong, Eric
>> <eric.dong@intel.com>
>> Subject: Re: [edk2-devel] [PATCH v3 2/2] UefiCpuPkg/PiSmmCpuDxeSmm:
>> Fix buffer overflow issue.
>>
>> On 01/03/20 18:20, Laszlo Ersek wrote:
>>> On 01/03/20 18:06, Laszlo Ersek wrote:
>>>> Hello Eric,
>>>>
>>>> On 12/24/19 03:33, Ni, Ray wrote:
>>>>> Eric,
>>>>> I am curious how the SMM CPU driver ran well with the buffer overflow
>> issue?
>>>>> Can you please explain the details?
>>>>
>>>> You don't seem to have answered Ray's question above.
>>>>
>>>> Accordingly, Ray doesn't appear to have posted a Reviewed-by or
>>>> Acked-by specifically for this patch (i.e., for [PATCH v3 2/2]). Ray
>>>> only approved  [PATCH v3 1/2].
>>>>
>>>> However, in the git history, I see the present patch being committed
>>>> as 123b720eeb37. The commit message there claims "Reviewed-by: Ray
>> Ni
>>>> <ray.ni@intel.com>" -- but that is invalid; Ray never reviewed this
>>>> particular patch (as far as I can see on the list).
>>>>
>>>> Ray: if you agree with this patch, please provide your R-b now.
>>>> Otherwise, we should revert commit 123b720eeb37.
>>>>
>>>> Regarding the code itself, please see below:
>>>>
>>>>>> -----Original Message-----
>>>>>> From: Dong, Eric <eric.dong@intel.com>
>>>>>> Sent: Monday, December 23, 2019 4:11 PM
>>>>>> To: devel@edk2.groups.io
>>>>>> Cc: Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>
>>>>>> Subject: [PATCH v3 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.
>>>>>>
>>>>>> Reviewed-by: 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 35951cc43e..4808045f71 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++) {
>>>>
>>>> While the proposed change is indeed better style, I don't understand
>>>> how the pre-patch code leads to an access to:
>>>>
>>>>   mSmmMpSyncData->CpuData[mMaxNumberOfCpus]
>>>>
>>>> The controlling expression of the "for" instruction is evaluated
>>>> every time *before* the loop body is executed. That includes the very
>>>> first time. So when we're about to enter the loop for the very first
>>>> time, we'll have done:
>>>>
>>>>   Index = mMaxNumberOfCpus;
>>>>   Index--;
>>>>
>>>> This means that the first access will be to
>>>>
>>>>   mSmmMpSyncData->CpuData[mMaxNumberOfCpus - 1]
>>>>
>>>> That seems to imply that the patch is not needed, functionally speaking.
>>>>
>>>> I suggest reverting this patch; both because of the invalid review-by
>>>> claim, and also because the commit message is wrong. The patch might
>>>> be justified as a style improvement, but not as a bugfix. (Even the
>>>> style improvement aspect could be questioned, if the decrementing
>>>> order carries value, functionally or even just semantically.)
>>>
>>> I'm sorry, I missed that [PATCH v3 2/2] contained Ray's R-b at the
>>> time of posting already -- Ray gave his R-b for the patch originally
>>> under the v2 posting; that is, for [PATCH v2 2/2]:
>>>
>>> https://edk2.groups.io/g/devel/message/52498
>>> http://mid.mail-
>> archive.com/734D49CCEBEEF84792F5B80ED585239D5C3A8DD9@S
>>> HSMSX104.ccr.corp.intel.com
>>>
>>> However, I still feel it was wrong that Eric ignored (or missed) Ray's
>>> question about the behavior of the code, under [PATCH v3 2/2]. That
>>> question should have blocked the pushing of the patch, any prior R-b
>>> tags notwithstanding. Investigating Ray's question more closely could
>>> have lead to the realization that the patch was actually a no-op, and
>>> that consequently the commit message was wrong. (The patch is not a
>>> bugfix.)
>>>
>>> I agree that the patch shouldn't break anything (as long as the
>>> post-loop value of "Index" is irrelevant, and the order of processing
>>> is also indifferent).
>>>
>>> Ray, what's your preference:
>>>
>>> - should we revert this patch, and then re-apply it with a fixed
>>> commit message (saying "stylistic fix"),
>>> - should we simply revert the patch (because it's unnecessary),
>>> - should we stick with the current commit (and keep the known-wrong
>>> commit message)?
>>>
>>> Personally, I'd choose option#2 (revert only), but I defer to you.
>>
>> The commit message also missed mentioning the following TianoCore
>> bugzilla ticket:
>>
>> https://bugzilla.tianocore.org/show_bug.cgi?id=2434
>>
>> (BTW, I'm confused by
>>
>>   https://bugzilla.tianocore.org/show_bug.cgi?id=2434#c1
>>
>> which says "Confirmed this is a real issue" -- there are no hints at a
>> reproducer, or symptoms, or a test environment etc.)
> 
> [[Eric]] When we review the code, we found this issue (I and Ray both think this code has issue at that time :( ). 
> So I submit this Bugz to record this issue. I need to change the bugz state(from "UNCONFIRMED" to
>  "CONFIRMED") before doing the code change and  I think that code has issue at that time, so I change the state
> and add that comments.
> 
> We have realized that code has no issue after I have checked in the code. I explained it with Ray offline and forget
> to update it in the mailing list.
> 
> Because this change really make the code easy to understand and consistent with other similar cases, so I don't
> rollback the change. If you think the commit message will make the user confuse. I'm ok to roll back the change.

Thank you for the explanation. Could you please post a series with two
patches:

(1) revert 123b720eeb37 -- the commit message on the revert patch should
state that the commit message of 123b720eeb37 was incorrect, because
there had been no bug.

(2) re-apply 123b720eeb37, but with a brand new commit message:

(2a) the commit mesage should include your statement above ("make the
code easy to understand and consistent with other similar cases")

(2b) please include a reference to
<https://bugzilla.tianocore.org/show_bug.cgi?id=2434>.

The goal is that, when someone runs "git blame" on the code, a year from
now, they be led to a correct commit message.

Also, I'm going to update TianoCore#2434 now.

Thank you,
Laszlo

Re: [edk2-devel] [PATCH v3 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Fix buffer overflow issue.

[Dong, Eric]

Hi Laszlo,

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Monday, January 6, 2020 6:48 PM
> To: Dong, Eric <eric.dong@intel.com>; devel@edk2.groups.io; Ni, Ray
> <ray.ni@intel.com>
> Subject: Re: [edk2-devel] [PATCH v3 2/2] UefiCpuPkg/PiSmmCpuDxeSmm:
> Fix buffer overflow issue.
> 
> On 01/06/20 02:15, Dong, Eric wrote:
> > Hi Laszlo,
> >
> >> -----Original Message-----
> >> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> >> Laszlo Ersek
> >> Sent: Saturday, January 4, 2020 2:11 AM
> >> To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>; Dong, Eric
> >> <eric.dong@intel.com>
> >> Subject: Re: [edk2-devel] [PATCH v3 2/2] UefiCpuPkg/PiSmmCpuDxeSmm:
> >> Fix buffer overflow issue.
> >>
> >> On 01/03/20 18:20, Laszlo Ersek wrote:
> >>> On 01/03/20 18:06, Laszlo Ersek wrote:
> >>>> Hello Eric,
> >>>>
> >>>> On 12/24/19 03:33, Ni, Ray wrote:
> >>>>> Eric,
> >>>>> I am curious how the SMM CPU driver ran well with the buffer
> >>>>> overflow
> >> issue?
> >>>>> Can you please explain the details?
> >>>>
> >>>> You don't seem to have answered Ray's question above.
> >>>>
> >>>> Accordingly, Ray doesn't appear to have posted a Reviewed-by or
> >>>> Acked-by specifically for this patch (i.e., for [PATCH v3 2/2]).
> >>>> Ray only approved  [PATCH v3 1/2].
> >>>>
> >>>> However, in the git history, I see the present patch being
> >>>> committed as 123b720eeb37. The commit message there claims
> >>>> "Reviewed-by: Ray
> >> Ni
> >>>> <ray.ni@intel.com>" -- but that is invalid; Ray never reviewed this
> >>>> particular patch (as far as I can see on the list).
> >>>>
> >>>> Ray: if you agree with this patch, please provide your R-b now.
> >>>> Otherwise, we should revert commit 123b720eeb37.
> >>>>
> >>>> Regarding the code itself, please see below:
> >>>>
> >>>>>> -----Original Message-----
> >>>>>> From: Dong, Eric <eric.dong@intel.com>
> >>>>>> Sent: Monday, December 23, 2019 4:11 PM
> >>>>>> To: devel@edk2.groups.io
> >>>>>> Cc: Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>
> >>>>>> Subject: [PATCH v3 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.
> >>>>>>
> >>>>>> Reviewed-by: 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 35951cc43e..4808045f71 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++) {
> >>>>
> >>>> While the proposed change is indeed better style, I don't
> >>>> understand how the pre-patch code leads to an access to:
> >>>>
> >>>>   mSmmMpSyncData->CpuData[mMaxNumberOfCpus]
> >>>>
> >>>> The controlling expression of the "for" instruction is evaluated
> >>>> every time *before* the loop body is executed. That includes the
> >>>> very first time. So when we're about to enter the loop for the very
> >>>> first time, we'll have done:
> >>>>
> >>>>   Index = mMaxNumberOfCpus;
> >>>>   Index--;
> >>>>
> >>>> This means that the first access will be to
> >>>>
> >>>>   mSmmMpSyncData->CpuData[mMaxNumberOfCpus - 1]
> >>>>
> >>>> That seems to imply that the patch is not needed, functionally speaking.
> >>>>
> >>>> I suggest reverting this patch; both because of the invalid
> >>>> review-by claim, and also because the commit message is wrong. The
> >>>> patch might be justified as a style improvement, but not as a
> >>>> bugfix. (Even the style improvement aspect could be questioned, if
> >>>> the decrementing order carries value, functionally or even just
> >>>> semantically.)
> >>>
> >>> I'm sorry, I missed that [PATCH v3 2/2] contained Ray's R-b at the
> >>> time of posting already -- Ray gave his R-b for the patch originally
> >>> under the v2 posting; that is, for [PATCH v2 2/2]:
> >>>
> >>> https://edk2.groups.io/g/devel/message/52498
> >>> http://mid.mail-
> >> archive.com/734D49CCEBEEF84792F5B80ED585239D5C3A8DD9@S
> >>> HSMSX104.ccr.corp.intel.com
> >>>
> >>> However, I still feel it was wrong that Eric ignored (or missed)
> >>> Ray's question about the behavior of the code, under [PATCH v3 2/2].
> >>> That question should have blocked the pushing of the patch, any
> >>> prior R-b tags notwithstanding. Investigating Ray's question more
> >>> closely could have lead to the realization that the patch was
> >>> actually a no-op, and that consequently the commit message was
> >>> wrong. (The patch is not a
> >>> bugfix.)
> >>>
> >>> I agree that the patch shouldn't break anything (as long as the
> >>> post-loop value of "Index" is irrelevant, and the order of
> >>> processing is also indifferent).
> >>>
> >>> Ray, what's your preference:
> >>>
> >>> - should we revert this patch, and then re-apply it with a fixed
> >>> commit message (saying "stylistic fix"),
> >>> - should we simply revert the patch (because it's unnecessary),
> >>> - should we stick with the current commit (and keep the known-wrong
> >>> commit message)?
> >>>
> >>> Personally, I'd choose option#2 (revert only), but I defer to you.
> >>
> >> The commit message also missed mentioning the following TianoCore
> >> bugzilla ticket:
> >>
> >> https://bugzilla.tianocore.org/show_bug.cgi?id=2434
> >>
> >> (BTW, I'm confused by
> >>
> >>   https://bugzilla.tianocore.org/show_bug.cgi?id=2434#c1
> >>
> >> which says "Confirmed this is a real issue" -- there are no hints at
> >> a reproducer, or symptoms, or a test environment etc.)
> >
> > [[Eric]] When we review the code, we found this issue (I and Ray both think
> this code has issue at that time :( ).
> > So I submit this Bugz to record this issue. I need to change the bugz
> > state(from "UNCONFIRMED" to
> >  "CONFIRMED") before doing the code change and  I think that code has
> > issue at that time, so I change the state and add that comments.
> >
> > We have realized that code has no issue after I have checked in the
> > code. I explained it with Ray offline and forget to update it in the mailing list.
> >
> > Because this change really make the code easy to understand and
> > consistent with other similar cases, so I don't rollback the change. If you
> think the commit message will make the user confuse. I'm ok to roll back the
> change.
> 
> Thank you for the explanation. Could you please post a series with two
> patches:
[[Eric]] Yes, I already send the new patch serial which follow your recommendation.
Thanks for your help.

Thanks,
Eric
> 
> (1) revert 123b720eeb37 -- the commit message on the revert patch should
> state that the commit message of 123b720eeb37 was incorrect, because
> there had been no bug.
> 
> (2) re-apply 123b720eeb37, but with a brand new commit message:
> 
> (2a) the commit mesage should include your statement above ("make the
> code easy to understand and consistent with other similar cases")
> 
> (2b) please include a reference to
> <https://bugzilla.tianocore.org/show_bug.cgi?id=2434>.
> 
> The goal is that, when someone runs "git blame" on the code, a year from
> now, they be led to a correct commit message.
> 
> Also, I'm going to update TianoCore#2434 now.
> 
> Thank you,
> Laszlo