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

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

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