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.web11.6540.1578071177110344607 for ; Fri, 03 Jan 2020 09:06:18 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=hJRDErve; 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=1578071176; 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=Z0aorTzMqy+FhfM7x0yXj55fsOKD8jcn66ZQ0O3rg20=; b=hJRDErveWLPC6qBXsOq+EnPN7r9/dg6cAO8FN+qnaxzRhaXzyj5GGtEw4dvjvknoknWF13 r3oMd4uGd9KZdKK/+O6+dZ+YuUw6qNkGwFIrExyKq8id8CJebx9V3brcZz+1J3CbDbgo8o nSs38aZjSFKhDVSkpFEetfTIOFZLBxM= 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-245-yQZWpvy3NTe-u1d1dyk-Xw-1; Fri, 03 Jan 2020 12:06:14 -0500 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 74299477; Fri, 3 Jan 2020 17:06:13 +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 6C7686F432; Fri, 3 Jan 2020 17:06:12 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v3 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Fix buffer overflow issue. 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> From: "Laszlo Ersek" Message-ID: Date: Fri, 3 Jan 2020 18:06:11 +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: <734D49CCEBEEF84792F5B80ED585239D5C3A9D46@SHSMSX104.ccr.corp.intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-MC-Unique: yQZWpvy3NTe-u1d1dyk-Xw-1 X-Mimecast-Spam-Score: 0 Content-Language: en-US Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit 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.) ... A more general note on *decrementing* loops in C: The best form, in my opinion, is: Index = mMaxNumberOfCpus; while (Index > 0) { --Index; // // Do stuff with "Index". // } This has two advantages over for (Index = mMaxNumberOfCpus; Index-- > 0;) { // // Do stuff with "Index". // } namely: - the "while" loop is easier to read, - the "while" loop will finish with "Index" holding value 0, and not value ((TypeOfIndex)-1). (The decrement step is conditional on the controlling expression.) Thanks Laszlo >> >> if (IsPresentAp (Index)) { >> >> ReleaseSemaphore (mSmmMpSyncData->CpuData[Index].Run); >> >> } >> >> @@ -170,7 +170,7 @@ AllCpusInSmmWithExceptions ( >> >> >> CpuData = mSmmMpSyncData->CpuData; >> >> ProcessorInfo = gSmmCpuPrivate->ProcessorInfo; >> >> - for (Index = mMaxNumberOfCpus; Index-- > 0;) { >> >> + for (Index = 0; Index < mMaxNumberOfCpus; Index++) { >> >> if (!(*(CpuData[Index].Present)) && ProcessorInfo[Index].ProcessorId != >> INVALID_APIC_ID) { >> >> if (((Exceptions & ARRIVAL_EXCEPTION_DELAYED) != 0) && >> SmmCpuFeaturesGetSmmRegister (Index, SmmRegSmmDelayed) != 0) { >> >> continue; >> >> @@ -305,7 +305,7 @@ SmmWaitForApArrival ( >> // >> >> // Send SMI IPIs to bring outside processors in >> >> // >> >> - for (Index = mMaxNumberOfCpus; Index-- > 0;) { >> >> + for (Index = 0; Index < mMaxNumberOfCpus; Index++) { >> >> if (!(*(mSmmMpSyncData->CpuData[Index].Present)) && >> gSmmCpuPrivate->ProcessorInfo[Index].ProcessorId != INVALID_APIC_ID) { >> >> SendSmiIpi ((UINT32)gSmmCpuPrivate- >>> ProcessorInfo[Index].ProcessorId); >> >> } >> >> @@ -361,7 +361,7 @@ WaitForAllAPsNotBusy ( >> { >> >> UINTN Index; >> >> >> >> - for (Index = mMaxNumberOfCpus; Index-- > 0;) { >> >> + for (Index = 0; Index < mMaxNumberOfCpus; Index++) { >> >> // >> >> // Ignore BSP and APs which not call in SMM. >> >> // >> >> @@ -617,7 +617,7 @@ BSPHandler ( >> // >> >> while (TRUE) { >> >> PresentCount = 0; >> >> - for (Index = mMaxNumberOfCpus; Index-- > 0;) { >> >> + for (Index = 0; Index < mMaxNumberOfCpus; Index++) { >> >> if (*(mSmmMpSyncData->CpuData[Index].Present)) { >> >> PresentCount ++; >> >> } >> >> @@ -1301,7 +1301,7 @@ InternalSmmStartupAllAPs ( >> } >> >> >> >> CpuCount = 0; >> >> - for (Index = mMaxNumberOfCpus; Index-- > 0;) { >> >> + for (Index = 0; Index < mMaxNumberOfCpus; Index++) { >> >> if (IsPresentAp (Index)) { >> >> CpuCount ++; >> >> >> >> @@ -1333,13 +1333,13 @@ InternalSmmStartupAllAPs ( >> // Here code always use AcquireSpinLock instead of AcquireSpinLockOrFail >> for not >> >> // block mode. >> >> // >> >> - for (Index = mMaxNumberOfCpus; Index-- > 0;) { >> >> + for (Index = 0; Index < mMaxNumberOfCpus; Index++) { >> >> if (IsPresentAp (Index)) { >> >> AcquireSpinLock (mSmmMpSyncData->CpuData[Index].Busy); >> >> } >> >> } >> >> >> >> - for (Index = mMaxNumberOfCpus; Index-- > 0;) { >> >> + for (Index = 0; Index < mMaxNumberOfCpus; Index++) { >> >> if (IsPresentAp (Index)) { >> >> mSmmMpSyncData->CpuData[Index].Procedure = >> (EFI_AP_PROCEDURE2) Procedure; >> >> mSmmMpSyncData->CpuData[Index].Parameter = ProcedureArguments; >> >> -- >> 2.23.0.windows.1 > > > >