[PATCH v2 0/2] Fix potential race condition issue.

public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed

* [PATCH v2 0/2] Fix potential race condition issue.
@ 2019-12-23  6:48 Dong, Eric
  2019-12-23  6:48 ` [PATCH v2 1/2] UefiCpuPkg/PiSmmCpuDxeSmm: Remove dependence between APs Dong, Eric
  2019-12-23  6:48 ` [PATCH v2 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Fix buffer overflow issue Dong, Eric
  0 siblings, 2 replies; 6+ messages in thread
From: Dong, Eric @ 2019-12-23  6:48 UTC (permalink / raw)
  To: devel; +Cc: Ray Ni, Laszlo Ersek

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

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, 54 insertions(+), 92 deletions(-)

-- 
2.23.0.windows.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v2 1/2] UefiCpuPkg/PiSmmCpuDxeSmm: Remove dependence between APs
  2019-12-23  6:48 [PATCH v2 0/2] Fix potential race condition issue Dong, Eric
@ 2019-12-23  6:48 ` Dong, Eric
  2019-12-23  7:38   ` [edk2-devel] " Ni, Ray
  2019-12-23  6:48 ` [PATCH v2 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Fix buffer overflow issue Dong, Eric
  1 sibling, 1 reply; 6+ messages in thread
From: Dong, Eric @ 2019-12-23  6:48 UTC (permalink / raw)
  To: devel; +Cc: Ray Ni, Laszlo Ersek

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, 46 insertions(+), 84 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
index 757f1056f7..5ad12db980 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,17 @@ 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;
+
+  WaitForSemaphore (&Token->RunningApCount);
+
+  if (Token->RunningApCount == 0) {
+    ReleaseSpinLock (Token->SpinLock);
   }
+
+  mSmmMpSyncData->CpuData[CpuIndex].Token = NULL;
 }
 
 /**
@@ -912,12 +857,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 +1058,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 +1071,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 +1109,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 +1196,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 +1230,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 +1293,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 +1322,10 @@ InternalSmmStartupAllAPs (
   }
 
   if (Token != NULL) {
-    *Token = (MM_COMPLETION) CreateToken ();
+    ProcToken = CreateToken ((UINT32)mMaxNumberOfCpus);
+    *Token = (MM_COMPLETION)ProcToken->SpinLock;
+  } else {
+    ProcToken = NULL;
   }
 
   //
@@ -1392,7 +1346,7 @@ InternalSmmStartupAllAPs (
       mSmmMpSyncData->CpuData[Index].Procedure = (EFI_AP_PROCEDURE2) Procedure;
       mSmmMpSyncData->CpuData[Index].Parameter = ProcedureArguments;
       if (Token != NULL) {
-        mSmmMpSyncData->CpuData[Index].Token   = (SPIN_LOCK *)(*Token);
+        mSmmMpSyncData->CpuData[Index].Token   = ProcToken;
       }
       if (CPUStatus != NULL) {
         mSmmMpSyncData->CpuData[Index].Status    = &CPUStatus[Index];
@@ -1408,6 +1362,13 @@ InternalSmmStartupAllAPs (
       if (CPUStatus != NULL) {
         CPUStatus[Index] = EFI_NOT_STARTED;
       }
+
+      //
+      // Decrease the count to mark this AP as finished.
+      //
+      if (Token != 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


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [edk2-devel] [PATCH v2 1/2] UefiCpuPkg/PiSmmCpuDxeSmm: Remove dependence between APs
  2019-12-23  6:48 ` [PATCH v2 1/2] UefiCpuPkg/PiSmmCpuDxeSmm: Remove dependence between APs Dong, Eric
@ 2019-12-23  7:38   ` Ni, Ray
  2019-12-23  7:48     ` Dong, Eric
  0 siblings, 1 reply; 6+ messages in thread
From: Ni, Ray @ 2019-12-23  7:38 UTC (permalink / raw)
  To: devel@edk2.groups.io, Dong, Eric; +Cc: Laszlo Ersek

> 
> +  WaitForSemaphore (&Token->RunningApCount); 
> + 
> +  if (Token->RunningApCount == 0) {
> +    ReleaseSpinLock (Token->SpinLock); 
>    }

1. if (InterlockedDecrement (&Token->RunningApCount) == 0) {
      ReleaseSpinLock (Token->SpinLock);
    }

We should avoid checking RunningApCount directly because it's possible
that AP#1 decrease the Count to 1 and before AP#1 checks the value against 0
the Count is decreased by AP#2 to 0. So that causes AP#1 and AP#2 call
ReleaseSpinLock() on the same SpinLock.

> 
> +      // Decrease the count to mark this AP as finished.

2. BSP is also handled here. So this comment is mis-leading.

> 
> +      //
> 
> +      if (Token != NULL) { 
> +        WaitForSemaphore (&ProcToken->RunningApCount);

3. The code is written correctly but improperly IMO.
Token is checked but ProcToken is deferenced.
I suggest you check ProcToken directly.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [edk2-devel] [PATCH v2 1/2] UefiCpuPkg/PiSmmCpuDxeSmm: Remove dependence between APs
  2019-12-23  7:38   ` [edk2-devel] " Ni, Ray
@ 2019-12-23  7:48     ` Dong, Eric
  0 siblings, 0 replies; 6+ messages in thread
From: Dong, Eric @ 2019-12-23  7:48 UTC (permalink / raw)
  To: Ni, Ray, devel@edk2.groups.io; +Cc: Laszlo Ersek

Hi Ray,

> -----Original Message-----
> From: Ni, Ray
> Sent: Monday, December 23, 2019 3:38 PM
> To: devel@edk2.groups.io; Dong, Eric <eric.dong@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Subject: RE: [edk2-devel] [PATCH v2 1/2] UefiCpuPkg/PiSmmCpuDxeSmm:
> Remove dependence between APs
> 
> >
> > +  WaitForSemaphore (&Token->RunningApCount);
> > +
> > +  if (Token->RunningApCount == 0) {
> > +    ReleaseSpinLock (Token->SpinLock);
> >    }
> 
> 1. if (InterlockedDecrement (&Token->RunningApCount) == 0) {
>       ReleaseSpinLock (Token->SpinLock);
>     }
> 
> We should avoid checking RunningApCount directly because it's possible
> that AP#1 decrease the Count to 1 and before AP#1 checks the value against
> 0
> the Count is decreased by AP#2 to 0. So that causes AP#1 and AP#2 call
> ReleaseSpinLock() on the same SpinLock.
> 
[[Eric]] good comments, will update it in next version.

> >
> > +      // Decrease the count to mark this AP as finished.
> 
> 2. BSP is also handled here. So this comment is mis-leading.
[[Eric]] will enhance the comments in next version.

> 
> >
> > +      //
> >
> > +      if (Token != NULL) {
> > +        WaitForSemaphore (&ProcToken->RunningApCount);
> 
> 3. The code is written correctly but improperly IMO.
> Token is checked but ProcToken is deferenced.
> I suggest you check ProcToken directly.
[[Eric]] The other place in this function all check the Token status then update code. 
So this code consistent with other place.  I will keep this code to keep consistent.

Thanks,
Eric

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v2 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Fix buffer overflow issue.
  2019-12-23  6:48 [PATCH v2 0/2] Fix potential race condition issue Dong, Eric
  2019-12-23  6:48 ` [PATCH v2 1/2] UefiCpuPkg/PiSmmCpuDxeSmm: Remove dependence between APs Dong, Eric
@ 2019-12-23  6:48 ` Dong, Eric
  2019-12-23  7:39   ` Ni, Ray
  1 sibling, 1 reply; 6+ messages in thread
From: Dong, Eric @ 2019-12-23  6:48 UTC (permalink / raw)
  To: devel; +Cc: Ray Ni, Laszlo Ersek

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


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH v2 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Fix buffer overflow issue.
  2019-12-23  6:48 ` [PATCH v2 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Fix buffer overflow issue Dong, Eric
@ 2019-12-23  7:39   ` Ni, Ray
  0 siblings, 0 replies; 6+ messages in thread
From: Ni, Ray @ 2019-12-23  7:39 UTC (permalink / raw)
  To: Dong, Eric, devel@edk2.groups.io; +Cc: Laszlo Ersek

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2019-12-23  7:48 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-12-23  6:48 [PATCH v2 0/2] Fix potential race condition issue Dong, Eric
2019-12-23  6:48 ` [PATCH v2 1/2] UefiCpuPkg/PiSmmCpuDxeSmm: Remove dependence between APs Dong, Eric
2019-12-23  7:38   ` [edk2-devel] " Ni, Ray
2019-12-23  7:48     ` Dong, Eric
2019-12-23  6:48 ` [PATCH v2 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Fix buffer overflow issue Dong, Eric
2019-12-23  7:39   ` Ni, Ray

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox