From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [205.139.110.120]) by mx.groups.io with SMTP id smtpd.web12.219.1578075081655605383 for ; Fri, 03 Jan 2020 10:11:22 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=fge9N/cB; spf=pass (domain: redhat.com, ip: 205.139.110.120, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1578075080; 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=9HGCbtq0QJUhK1vbhgT3CY23+kQOF4PlNFFriAYsnyk=; b=fge9N/cBWfQe+9/zkA53J1JFkGOBM6iTKEDn45dfkaoSiSKvKwGRSNfFzJ7SJ77Ruy3kGz aw1YYkVwGEMH+ybTcQNzq1XgzTMRwOvcBb2CoI+G6gqpKxjsw668RS3tEFPqZOYCTg9H8F zGqNgq2DhbsZmjo2WsgFLRBUcaQM11g= 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-76-22WRAYPwOY2KZ2AaEPYE6Q-1; Fri, 03 Jan 2020 13:11:15 -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 3CF2D18C35B1; Fri, 3 Jan 2020 18:11:14 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-245.ams2.redhat.com [10.36.116.245]) by smtp.corp.redhat.com (Postfix) with ESMTP id 22CD45C28C; Fri, 3 Jan 2020 18:11:12 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v3 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Fix buffer overflow issue. From: "Laszlo Ersek" To: devel@edk2.groups.io, ray.ni@intel.com, "Dong, Eric" 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> Message-ID: <3ba3ef61-29d4-45a5-2917-145f9af1d7cf@redhat.com> Date: Fri, 3 Jan 2020 19:11:12 +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: <781d27d6-86e6-ee58-abdc-cb5ecb24fa4f@redhat.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-MC-Unique: 22WRAYPwOY2KZ2AaEPYE6Q-1 X-Mimecast-Spam-Score: 0 Content-Language: en-US Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit 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@SHSMSX104.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.) Laszlo