From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [207.211.31.120]) by mx.groups.io with SMTP id smtpd.web09.2112.1578307697909247416 for ; Mon, 06 Jan 2020 02:48:18 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=Uy70QuOY; spf=pass (domain: redhat.com, ip: 207.211.31.120, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1578307697; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=ltmvsdt/Gn9rEC4K1hyGiUQTCpmwWSnpJcVOyEjsMn0=; b=Uy70QuOYli03HpphDWOpBycGDAEbjHxbwto8wTcsy03SEsY9PzEH9wmKFEUJP/Ec27Blz/ fLuOot6LBx8vjS417JgdF+g6VRu0cq5djOQpNXZNwtKZH9eNP4n9c/jK+Y2cdZTWuJIdsq nPBNJiVF/21iDdE9fpMT0fcKH0A9jTc= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-9-MLyEJx9lPj6UAjcMBWP-Dg-1; Mon, 06 Jan 2020 05:48:12 -0500 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id C9E7DDBA3; Mon, 6 Jan 2020 10:48:10 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-117-24.ams2.redhat.com [10.36.117.24]) by smtp.corp.redhat.com (Postfix) with ESMTP id C830D5C578; Mon, 6 Jan 2020 10:48:09 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v3 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Fix buffer overflow issue. To: "Dong, Eric" , "devel@edk2.groups.io" , "Ni, Ray" 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> From: "Laszlo Ersek" Message-ID: <118367d0-c1a9-e57b-d63b-05eea6752ad6@redhat.com> Date: Mon, 6 Jan 2020 11:48:08 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-MC-Unique: MLyEJx9lPj6UAjcMBWP-Dg-1 X-Mimecast-Spam-Score: 0 Content-Language: en-US Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit 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 = 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: (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 . 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