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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#52504): https://edk2.groups.io/g/devel/message/52504
Mute This Topic: https://groups.io/mt/69227574/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#52516): https://edk2.groups.io/g/devel/message/52516
Mute This Topic: https://groups.io/mt/69227574/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#52836): https://edk2.groups.io/g/devel/message/52836
Mute This Topic: https://groups.io/mt/69227574/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#52839): https://edk2.groups.io/g/devel/message/52839
Mute This Topic: https://groups.io/mt/69227574/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#52842): https://edk2.groups.io/g/devel/message/52842
Mute This Topic: https://groups.io/mt/69227574/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#52853): https://edk2.groups.io/g/devel/message/52853
Mute This Topic: https://groups.io/mt/69227574/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#52892): https://edk2.groups.io/g/devel/message/52892
Mute This Topic: https://groups.io/mt/69227574/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#52951): https://edk2.groups.io/g/devel/message/52951
Mute This Topic: https://groups.io/mt/69227574/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-