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

["Laszlo Ersek" <lersek@redhat.com>]

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
> 
> 
> 
>