I forget to mention: I agree with you on #2 - (2) The comparison is not appropriate here (after the patch); it is too strict.
BASE_4GB is an exclusive limit, hence the pre-patch status is good. But MAX_ADDRESS is an inclusive limit, so the condition should be CpuStart + PhysicalSize - 1 <= MAX_ADDRESS Because CpuStart and PhysicalSize are UINT64, it may cause problem when the source is CpuStart+PhysicalSize==0x10000000000000000, theoretically. So I think we should update DxeCore and PeiCore as well - to use Start+Size - 1 <= MAX_ADDRESS. Thank you Yao Jiewen From: Yao, Jiewen Sent: Wednesday, June 29, 2016 9:02 PM To: Laszlo Ersek <[email protected]>; [email protected] Cc: Kinney, Michael D <[email protected]>; Fan, Jeff <[email protected]>; Yao, Jiewen <[email protected]> Subject: RE: [edk2] [PATCH] MdeModulePkg/PiSmmCore: Replace BASE_4GB with MAX_ADDRESS check. HI Laszlo I appreciate your careful review. I would like to clarify why we did not consider adding check for integer overflow. The EDKII security model is that: 1) We trust any internal data communication before EndOfDxe. We have to, because the SMM core has to trust the component runs before SMM, and construct SMM. 2) We do not trust any external data communication after EndOfDxe. We should not, external data might be from an untrusted source. That is why we need lots of check in PE/COFF library, SMM communication buffer, UEFI authenticated variable, etc. Based upon above assumption, SmramRanges is from a trusted source. There is *no need* to add security check, we just need add function check. You can refer to how DxeCore parse resource description hob: if (PhitHob->EfiFreeMemoryTop > (ResourceHob->PhysicalStart + ResourceHob->ResourceLength)) { continue; } if ((ResourceHob->PhysicalStart + ResourceHob->ResourceLength) > (EFI_PHYSICAL_ADDRESS)MAX_ADDRESS) { continue; } You can refer to how PeiCore parse the resource description hob: if (ResourceHob->ResourceType != EFI_RESOURCE_SYSTEM_MEMORY || ResourceHob->PhysicalStart + ResourceHob->ResourceLength > MAX_ADDRESS) { continue; } if (NextResourceHob->ResourceType != EFI_RESOURCE_SYSTEM_MEMORY || NextResourceHob->PhysicalStart + NextResourceHob->ResourceLength > MAX_ADDRESS) { continue; } if (ResourceHob->ResourceType == EFI_RESOURCE_SYSTEM_MEMORY && ResourceHob->PhysicalStart + ResourceHob->ResourceLength <= MAX_ADDRESS && ResourceHob->ResourceLength > TotalReservedMemorySize && PeiLoadFixAddressIsMemoryRangeAvailable(PrivateData, ResourceHob)) { Go back to your suggestion. I have no problem to adopt you suggestion on using below format. It is not harmful. if (PhysicalSize <= MAX_ADDRESS && CpuStart - 1 <= MAX_ADDRESS - PhysicalSize) { But one thing I have to mention is that: If we do not trust CpuStart and PhysicalSize, we will have big problem, because CPU driver need use these data to set SMRR, which is the basic of SMM security. We have assume CpuStart and PhysicalSize is set correctly. Thank you Yao Jiewen From: edk2-devel [mailto:[email protected]] On Behalf Of Laszlo Ersek Sent: Wednesday, June 29, 2016 8:30 PM To: Yao, Jiewen <[email protected]<mailto:[email protected]>>; [email protected]<mailto:[email protected]> Cc: Kinney, Michael D <[email protected]<mailto:[email protected]>>; Fan, Jeff <[email protected]<mailto:[email protected]>> Subject: Re: [edk2] [PATCH] MdeModulePkg/PiSmmCore: Replace BASE_4GB with MAX_ADDRESS check. On 06/29/16 04:29, Jiewen Yao wrote: > PI specification Vol 4 - SMM does not have any limitation of BASE_4GB for SMM. > So we should replace BASE_4GB check with MAX_ADDRESS check to make sure > the SMM memory is accessible by SMM Core. > > Cc: Jeff Fan <[email protected]<mailto:[email protected]>> > Cc: Michael D Kinney > <[email protected]<mailto:[email protected]>> > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Jiewen Yao <[email protected]<mailto:[email protected]>> > --- > MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c | 2 +- > MdeModulePkg/Core/PiSmmCore/Pool.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c > b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c > index acfcc83..3788bd5 100644 > --- a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c > +++ b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c > @@ -1556,7 +1556,7 @@ SmmIplEntry ( > } > > if (gSmmCorePrivate->SmramRanges[Index].CpuStart >= BASE_1MB) { > - if ((gSmmCorePrivate->SmramRanges[Index].CpuStart + > gSmmCorePrivate->SmramRanges[Index].PhysicalSize) <= BASE_4GB) { > + if ((gSmmCorePrivate->SmramRanges[Index].CpuStart + > gSmmCorePrivate->SmramRanges[Index].PhysicalSize) <= MAX_ADDRESS) { > if (gSmmCorePrivate->SmramRanges[Index].PhysicalSize >= MaxSize) { > MaxSize = gSmmCorePrivate->SmramRanges[Index].PhysicalSize; > mCurrentSmramRange = &gSmmCorePrivate->SmramRanges[Index]; > diff --git a/MdeModulePkg/Core/PiSmmCore/Pool.c > b/MdeModulePkg/Core/PiSmmCore/Pool.c > index 761988e..e493fd2 100644 > --- a/MdeModulePkg/Core/PiSmmCore/Pool.c > +++ b/MdeModulePkg/Core/PiSmmCore/Pool.c > @@ -67,7 +67,7 @@ SmmInitializeMemoryServices ( > } > > if (SmramRanges[Index].CpuStart >= BASE_1MB) { > - if ((SmramRanges[Index].CpuStart + SmramRanges[Index].PhysicalSize) > <= BASE_4GB) { > + if ((SmramRanges[Index].CpuStart + SmramRanges[Index].PhysicalSize) > <= MAX_ADDRESS) { > if (SmramRanges[Index].PhysicalSize >= MaxSize) { > MaxSize = SmramRanges[Index].PhysicalSize; > CurrentSmramRangesIndex = Index; > I have one question and two suggestions: (1) These checks refer to the SMRAM ranges that are reported by EFI_SMM_ACCESS2_PROTOCOL.GetCapabilities() member function, correct? So whatever worked before should continue to work. (2) The comparison is not appropriate here (after the patch); it is too strict. BASE_4GB is an exclusive limit, hence the pre-patch status is good. But MAX_ADDRESS is an inclusive limit, so the condition should be CpuStart + PhysicalSize - 1 <= MAX_ADDRESS (3) The addition in (2) can easily overflow (wrap around) before the comparison. Therefore I recommend the following steps instead: * First, reorder the LHS of the formula in (2): (CpuStart - 1) + PhysicalSize <= MAX_ADDRESS The subtraction is safe, because we have an outermost condition that ensures (CpuStart >= BASE_1MB). * Second, subtract PhysicalSize from both sides: CpuStart - 1 <= MAX_ADDRESS - PhysicalSize Considering X64 only, this could be the final form: MAX_ADDRESS is MAX_UINT64, so the subtraction on the right hand side is safe too. * However, that is not the case on Ia32. There we must compare MAX_ADDRESS vs. PhysicalSize explicitly. Which leaves us with: if (PhysicalSize <= MAX_ADDRESS && CpuStart - 1 <= MAX_ADDRESS - PhysicalSize) { /* ... */ } This is the final form that I'm proposing for the patch. The first condition is always true on X64, but it has "teeth" on Ia32. Then, once the first condition is true, the RHS of the second condition will not underflow, and we can safely do the comparison. (The LHS of the second condition will not underflow due to the argument about CpuStart above.) Thanks Laszlo _______________________________________________ edk2-devel mailing list [email protected]<mailto:[email protected]> https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
