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

["Dong, Eric" <eric.dong@intel.com>]

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