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

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

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