From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) by mx.groups.io with SMTP id smtpd.web09.830.1578365276925457205 for ; Mon, 06 Jan 2020 18:47:57 -0800 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 192.55.52.136, mailfrom: eric.dong@intel.com) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga001.jf.intel.com ([10.7.209.18]) by fmsmga106.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 06 Jan 2020 18:47:56 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.69,404,1571727600"; d="scan'208";a="303057223" Received: from fmsmsx106.amr.corp.intel.com ([10.18.124.204]) by orsmga001.jf.intel.com with ESMTP; 06 Jan 2020 18:47:56 -0800 Received: from fmsmsx163.amr.corp.intel.com (10.18.125.72) by FMSMSX106.amr.corp.intel.com (10.18.124.204) with Microsoft SMTP Server (TLS) id 14.3.439.0; Mon, 6 Jan 2020 18:47:55 -0800 Received: from shsmsx101.ccr.corp.intel.com (10.239.4.153) by fmsmsx163.amr.corp.intel.com (10.18.125.72) with Microsoft SMTP Server (TLS) id 14.3.439.0; Mon, 6 Jan 2020 18:47:55 -0800 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.202]) by SHSMSX101.ccr.corp.intel.com ([169.254.1.30]) with mapi id 14.03.0439.000; Tue, 7 Jan 2020 10:47:50 +0800 From: "Dong, Eric" To: Laszlo Ersek , "devel@edk2.groups.io" , "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+GRwKfIkVnAgBAl64CAAAQdAIAADg4AgAQcf4CAAB61AIABkerQ Date: Tue, 7 Jan 2020 02:47:50 +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> <118367d0-c1a9-e57b-d63b-05eea6752ad6@redhat.com> In-Reply-To: <118367d0-c1a9-e57b-d63b-05eea6752ad6@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: Laszlo Ersek [mailto:lersek@redhat.com] > Sent: Monday, January 6, 2020 6:48 PM > To: Dong, Eric ; devel@edk2.groups.io; Ni, Ray > > Subject: Re: [edk2-devel] [PATCH v3 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: > Fix buffer overflow issue. >=20 > 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 ; Dong, Eric > >> > >> 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 > >>>> " -- 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 speak= ing. > >>>> > >>>> 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=3D2434 > >> > >> (BTW, I'm confused by > >> > >> https://bugzilla.tianocore.org/show_bug.cgi?id=3D2434#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 t= hink > 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 ma= iling 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. >=20 > 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 recomme= ndation. Thanks for your help. Thanks, Eric >=20 > (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. >=20 > (2) re-apply 123b720eeb37, but with a brand new commit message: >=20 > (2a) the commit mesage should include your statement above ("make the > code easy to understand and consistent with other similar cases") >=20 > (2b) please include a reference to > . >=20 > The goal is that, when someone runs "git blame" on the code, a year from > now, they be led to a correct commit message. >=20 > Also, I'm going to update TianoCore#2434 now. >=20 > Thank you, > Laszlo