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

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

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