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...@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.d...@intel.com>
>>> Sent: Monday, December 23, 2019 4:11 PM
>>> To: devel@edk2.groups.io
>>> Cc: Ni, Ray <ray...@intel.com>; Laszlo Ersek <ler...@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...@intel.com>
>>> Cc: Laszlo Ersek <ler...@redhat.com>
>>> Signed-off-by: Eric Dong <eric.d...@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
734D49CCEBEEF84792F5B80ED585239D5C3A8DD9@SHSMSX104.ccr.corp.intel.com">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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
