From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by mx.groups.io with SMTP id smtpd.web12.5383.1578273320658518556 for ; Sun, 05 Jan 2020 17:15:21 -0800 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 134.134.136.24, mailfrom: eric.dong@intel.com) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by orsmga102.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 05 Jan 2020 17:15:20 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.69,400,1571727600"; d="scan'208";a="253190153" Received: from fmsmsx108.amr.corp.intel.com ([10.18.124.206]) by fmsmga002.fm.intel.com with ESMTP; 05 Jan 2020 17:15:19 -0800 Received: from fmsmsx151.amr.corp.intel.com (10.18.125.4) by FMSMSX108.amr.corp.intel.com (10.18.124.206) with Microsoft SMTP Server (TLS) id 14.3.439.0; Sun, 5 Jan 2020 17:15:19 -0800 Received: from shsmsx108.ccr.corp.intel.com (10.239.4.97) by FMSMSX151.amr.corp.intel.com (10.18.125.4) with Microsoft SMTP Server (TLS) id 14.3.439.0; Sun, 5 Jan 2020 17:15:19 -0800 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.202]) by SHSMSX108.ccr.corp.intel.com ([169.254.8.39]) with mapi id 14.03.0439.000; Mon, 6 Jan 2020 09:15:17 +0800 From: "Dong, Eric" To: "devel@edk2.groups.io" , "lersek@redhat.com" , "Ni, Ray" Subject: Re: [edk2-devel] [PATCH v3 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Fix buffer overflow issue. Thread-Topic: [edk2-devel] [PATCH v3 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Fix buffer overflow issue. Thread-Index: AQHVuWh9PZo7cPr6p0SiGGfCe+GRwKfIkVnAgBAl64CAAAQdAIAADg4AgAQcf4A= Date: Mon, 6 Jan 2020 01:15:16 +0000 Message-ID: References: <20191223081037.1565-1-eric.dong@intel.com> <20191223081037.1565-3-eric.dong@intel.com> <734D49CCEBEEF84792F5B80ED585239D5C3A9D46@SHSMSX104.ccr.corp.intel.com> <781d27d6-86e6-ee58-abdc-cb5ecb24fa4f@redhat.com> <3ba3ef61-29d4-45a5-2917-145f9af1d7cf@redhat.com> In-Reply-To: <3ba3ef61-29d4-45a5-2917-145f9af1d7cf@redhat.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Return-Path: eric.dong@intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable 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 ; Dong, Eric > > Subject: Re: [edk2-devel] [PATCH v3 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: > Fix buffer overflow issue. >=20 > 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 overflo= w > 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 > >> " -- 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 > >>>> Sent: Monday, December 23, 2019 4:11 PM > >>>> To: devel@edk2.groups.io > >>>> Cc: Ni, Ray ; Laszlo Ersek > >>>> 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 > >>>> Cc: Laszlo Ersek > >>>> Signed-off-by: Eric Dong > >>>> --- > >>>> 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 =3D mMaxNumberOfCpus; Index-- > 0;) { > >>>> > >>>> + for (Index =3D 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 =3D 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 speaki= ng. > >> > >> 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. >=20 > The commit message also missed mentioning the following TianoCore > bugzilla ticket: >=20 > https://bugzilla.tianocore.org/show_bug.cgi?id=3D2434 >=20 > (BTW, I'm confused by >=20 > https://bugzilla.tianocore.org/show_bug.cgi?id=3D2434#c1 >=20 > 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 thin= k this code has issue at that time :( ).=20 So I submit this Bugz to record this issue. I need to change the bugz stat= e(from "UNCONFIRMED" to "CONFIRMED") before doing the code change and I think that code has issu= e 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 co= nfuse. I'm ok to roll back the change. Thanks, Eric >=20 > Laszlo >=20 >=20 >=20