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
<ray...@intel.com>" -- 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 <eric.d...@intel.com>
>> Sent: Monday, December 23, 2019 4:11 PM
>> To: devel@edk2.groups.io
>> Cc: Ni, Ray <ray...@intel.com>; Laszlo Ersek <ler...@redhat.com>
>> 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 <ray...@intel.com>
>> Cc: Laszlo Ersek <ler...@redhat.com>
>> Signed-off-by: Eric Dong <eric.d...@intel.com>
>> ---
>> 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
>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#52836): https://edk2.groups.io/g/devel/message/52836
Mute This Topic: https://groups.io/mt/69227574/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
