* [PATCH v3 0/2] Fix race condition issue for PiSmmCpuDxeSmm driver @ 2019-12-23 8:10 Dong, Eric 2019-12-23 8:10 ` [PATCH v3 1/2] UefiCpuPkg/PiSmmCpuDxeSmm: Remove dependence between APs Dong, Eric 2019-12-23 8:10 ` [PATCH v3 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Fix buffer overflow issue Dong, Eric 0 siblings, 2 replies; 11+ messages in thread From: Dong, Eric @ 2019-12-23 8:10 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. 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v3 1/2] UefiCpuPkg/PiSmmCpuDxeSmm: Remove dependence between APs 2019-12-23 8:10 [PATCH v3 0/2] Fix race condition issue for PiSmmCpuDxeSmm driver Dong, Eric @ 2019-12-23 8:10 ` Dong, Eric 2019-12-24 2:44 ` [edk2-devel] " Ni, Ray 2019-12-23 8:10 ` [PATCH v3 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Fix buffer overflow issue Dong, Eric 1 sibling, 1 reply; 11+ messages in thread From: Dong, Eric @ 2019-12-23 8:10 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, 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [edk2-devel] [PATCH v3 1/2] UefiCpuPkg/PiSmmCpuDxeSmm: Remove dependence between APs 2019-12-23 8:10 ` [PATCH v3 1/2] UefiCpuPkg/PiSmmCpuDxeSmm: Remove dependence between APs Dong, Eric @ 2019-12-24 2:44 ` Ni, Ray 0 siblings, 0 replies; 11+ messages in thread From: Ni, Ray @ 2019-12-24 2:44 UTC (permalink / raw) To: devel@edk2.groups.io, Dong, Eric; +Cc: Laszlo Ersek 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] > -=-=-=-=-=-= ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v3 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Fix buffer overflow issue. 2019-12-23 8:10 [PATCH v3 0/2] Fix race condition issue for PiSmmCpuDxeSmm driver Dong, Eric 2019-12-23 8:10 ` [PATCH v3 1/2] UefiCpuPkg/PiSmmCpuDxeSmm: Remove dependence between APs Dong, Eric @ 2019-12-23 8:10 ` Dong, Eric 2019-12-24 2:33 ` Ni, Ray 1 sibling, 1 reply; 11+ messages in thread From: Dong, Eric @ 2019-12-23 8:10 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. 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v3 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Fix buffer overflow issue. 2019-12-23 8:10 ` [PATCH v3 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Fix buffer overflow issue Dong, Eric @ 2019-12-24 2:33 ` Ni, Ray 2020-01-03 17:06 ` [edk2-devel] " Laszlo Ersek 0 siblings, 1 reply; 11+ messages in thread From: Ni, Ray @ 2019-12-24 2:33 UTC (permalink / raw) To: Dong, Eric, devel@edk2.groups.io; +Cc: Laszlo Ersek 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [edk2-devel] [PATCH v3 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Fix buffer overflow issue. 2019-12-24 2:33 ` Ni, Ray @ 2020-01-03 17:06 ` Laszlo Ersek 2020-01-03 17:20 ` Laszlo Ersek 0 siblings, 1 reply; 11+ messages in thread From: Laszlo Ersek @ 2020-01-03 17:06 UTC (permalink / raw) To: devel, ray.ni, Dong, Eric 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 > > > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [edk2-devel] [PATCH v3 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Fix buffer overflow issue. 2020-01-03 17:06 ` [edk2-devel] " Laszlo Ersek @ 2020-01-03 17:20 ` Laszlo Ersek 2020-01-03 18:11 ` Laszlo Ersek 0 siblings, 1 reply; 11+ messages in thread From: Laszlo Ersek @ 2020-01-03 17:20 UTC (permalink / raw) To: devel, ray.ni, Dong, Eric 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [edk2-devel] [PATCH v3 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Fix buffer overflow issue. 2020-01-03 17:20 ` Laszlo Ersek @ 2020-01-03 18:11 ` Laszlo Ersek 2020-01-06 1:15 ` Dong, Eric 0 siblings, 1 reply; 11+ messages in thread From: Laszlo Ersek @ 2020-01-03 18:11 UTC (permalink / raw) To: devel, ray.ni, Dong, Eric 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [edk2-devel] [PATCH v3 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Fix buffer overflow issue. 2020-01-03 18:11 ` Laszlo Ersek @ 2020-01-06 1:15 ` Dong, Eric 2020-01-06 10:48 ` Laszlo Ersek 0 siblings, 1 reply; 11+ messages in thread From: Dong, Eric @ 2020-01-06 1:15 UTC (permalink / raw) To: devel@edk2.groups.io, lersek@redhat.com, Ni, Ray 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 > > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [edk2-devel] [PATCH v3 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Fix buffer overflow issue. 2020-01-06 1:15 ` Dong, Eric @ 2020-01-06 10:48 ` Laszlo Ersek 2020-01-07 2:47 ` Dong, Eric 0 siblings, 1 reply; 11+ messages in thread From: Laszlo Ersek @ 2020-01-06 10:48 UTC (permalink / raw) To: Dong, Eric, devel@edk2.groups.io, Ni, Ray 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [edk2-devel] [PATCH v3 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Fix buffer overflow issue. 2020-01-06 10:48 ` Laszlo Ersek @ 2020-01-07 2:47 ` Dong, Eric 0 siblings, 0 replies; 11+ messages in thread From: Dong, Eric @ 2020-01-07 2:47 UTC (permalink / raw) To: Laszlo Ersek, devel@edk2.groups.io, Ni, Ray 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2020-01-07 2:47 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-12-23 8:10 [PATCH v3 0/2] Fix race condition issue for PiSmmCpuDxeSmm driver Dong, Eric 2019-12-23 8:10 ` [PATCH v3 1/2] UefiCpuPkg/PiSmmCpuDxeSmm: Remove dependence between APs Dong, Eric 2019-12-24 2:44 ` [edk2-devel] " Ni, Ray 2019-12-23 8:10 ` [PATCH v3 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Fix buffer overflow issue Dong, Eric 2019-12-24 2:33 ` Ni, Ray 2020-01-03 17:06 ` [edk2-devel] " Laszlo Ersek 2020-01-03 17:20 ` Laszlo Ersek 2020-01-03 18:11 ` Laszlo Ersek 2020-01-06 1:15 ` Dong, Eric 2020-01-06 10:48 ` Laszlo Ersek 2020-01-07 2:47 ` Dong, Eric
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox