Re: [Patch v2 2/2] UefiCpuPkg/CpuS3DataDxe: Change Memory Type and address limitation.

["Ni, Ruiyu" <ruiyu.ni@intel.com>]

> (3) The "UefiCpuPkg/Include/AcpiCpuData.h" header states that
> ACPI_CPU_DATA (the structure itself) "must be allocated below 4GB from
> memory of type EfiACPIMemoryNVS".
> 
> If we have determined that this is no longer necessary, then:
> - we should first update the documentation in "AcpiCpuData.h" first,
> - the commit message should carefully explain why the change is valid.

Indeed. The header file will be updated.

> 
> In particular, my concern is not about the normal boot path. (The normal
> boot path is explained for example in the message of commit 92b87f1c8c0b,
> "OvmfPkg: build CpuS3DataDxe for -D SMM_REQUIRE",
> 2015-11-30.) I agree that copying from BootServicesData type memory into
> SMRAM is fine, during normal boot.
> 
> Instead, my concern is with the S3 resume path, when the data from the
> SMRAM buffer is *restored*. I seem to recall a case when the data was first
> restored from SMRAM to the original AcpiNVS allocation. If we make that
> area BootServicesData now, then the OS will reuse it, and then at
> S3 resume, we overwrite OS data.
> 
> ... After reviewing the GetAcpiCpuData() function in
> "UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c", I *think* this is a valid change,
> because GetAcpiCpuData() never remembers the address of the
> ACPI_CPU_DATA structure, or the addresses of its fields.

In order to making this changes, we reviewed the whole boot/S3 execution
code path and didn't see any SMRAM to normal memory restore.
We think it's safe to use BS memory. The NVS memory usage was added before
the SMRAM shadow. Now since S3 boot path only uses the SMRAM content, there
is no need to use NVS memory at all.
> 
> However, I would like Mike to review this as well (CC'ing him).
> 
> 
> (4) Because ACPI_CPU_DATA_EX contains MtrrTable, GdtrProfile and
> IdtrProfile too, not just ACPI_CPU_DATA, the above change moves all of
> those fields into BootServicesData type memory. In turn, the
> EFI_PHYSICAL_ADDRESS fields in ACPI_CPU_DATA that have the same
> names will point into BootServicesData type memory.
> 
> This conflicts with the requirements that are documented in
> "UefiCpuPkg/Include/AcpiCpuData.h" for all three fields: MtrrTable,
> GdtrProfile and IdtrProfile.
> 
> If we want to change the allocation of these objects (of type
> MTRR_SETTINGS, IA32_DESCRIPTOR and IA32_DESCRIPTOR, respectively),
> then we should audit their use during S3 resume in the PiSmmCpuDxeSmm
> driver, and document them one-by-one.
> 
> ... Based on GetAcpiCpuData() again, I *think* this is valid change,
> functionally speaking, but I'd like Mike to review as well.
> 
> 
> >    ASSERT (AcpiCpuDataEx != NULL);
> >    AcpiCpuData = &AcpiCpuDataEx->AcpiCpuData;
> >
> > @@ -210,11 +171,16 @@ CpuS3DataInitialize (
> >    AcpiCpuData->MtrrTable    =
> (EFI_PHYSICAL_ADDRESS)(UINTN)&AcpiCpuDataEx->MtrrTable;
> >
> >    //
> > -  // Allocate stack space for all CPUs
> > +  // Allocate stack space for all CPUs, use ACPI NVS memory type
> > + because it will  // not copy to smram at Smm ready to lock point.
> >    //
> > -  Stack = AllocateAcpiNvsMemoryBelow4G (NumberOfCpus *
> > AcpiCpuData->StackSize);
> > -  ASSERT (Stack != NULL);
> > -  AcpiCpuData->StackAddress = (EFI_PHYSICAL_ADDRESS)(UINTN)Stack;
> > +  Status  = gBS->AllocatePages (
> > +                   AllocateAnyPages,
> > +                   EfiACPIMemoryNVS,
> > +                   EFI_SIZE_TO_PAGES (NumberOfCpus * AcpiCpuData-
> >StackSize),
> > +                   &AcpiCpuData->StackAddress
> > +                   );
> > +  ASSERT_EFI_ERROR (Status);
> 
> (5) The leading comment should be clarified. We should state that during
> S3 resume, the stack will only be used as scratch space, i.e. we won't read
> anything from it before we write to it, in PiSmmCpuDxeSemm.
> Otherwise, it would be a security bug to keep the area in AcpiNVS and not in
> SMRAM.
> 
> 
> (6) This change requires a separate investigation from the rest of the patch,
> so it should be in a separate patch.
> 
> That implies we should first change this call site, and then remove
> AllocateAcpiNvsMemoryBelow4G() in a final patch.
> 
> 
> (7) Why is it OK to lift the 4GB limitation? ... I think it may be valid indeed,
> because PrepareApStartupVector() stores StackAddress to "mExchangeInfo-
> >StackStart" (which has type (VOID*)), and because
> "UefiCpuPkg/PiSmmCpuDxeSmm/X64/MpFuncs.nasm" reads the latter with:
> 
>         add  edi, StackStartAddressLocation
>         add  rax, qword [edi]
>         mov  rsp, rax
>         mov  qword [edi], rax
> 
> in long-mode code. But, again, this should be documented in the
> (separate) patch's commit message.
> 
> 
> (8) The change breaks the documentation on
> "ACPI_CPU_DATA.StackAddress", in "UefiCpuPkg/Include/AcpiCpuData.h".
> The documentation should be updated.
> 
> 
> >
> >    //
> >    // Get the boot processor's GDT and IDT @@ -227,7 +193,7 @@
> > CpuS3DataInitialize (
> >    //
> >    GdtSize = AcpiCpuDataEx->GdtrProfile.Limit + 1;
> >    IdtSize = AcpiCpuDataEx->IdtrProfile.Limit + 1;
> > -  Gdt = AllocateAcpiNvsMemoryBelow4G (GdtSize + IdtSize);
> > +  Gdt = AllocatePages (EFI_SIZE_TO_PAGES (GdtSize + IdtSize));
> >    ASSERT (Gdt != NULL);
> >    Idt = (VOID *)((UINTN)Gdt + GdtSize);
> >    CopyMem (Gdt, (VOID *)AcpiCpuDataEx->GdtrProfile.Base, GdtSize);
> 
> (9) This change breaks the requirements in
> "UefiCpuPkg/Include/AcpiCpuData.h" that both
> "ACPI_CPU_DATA.GdtrProfile->Base" and "ACPI_CPU_DATA.IdtrProfile-
> >Base"
> point into AcpiNVS data.
> 
> ("The buffer for GDT must also be allocated below 4GB from memory of type
> EfiACPIMemoryNVS" / "The buffer for IDT must also be allocated below 4GB
> from memory of type EfiACPIMemoryNVS".)
> 
> 
> (10) And, indeed, this is the bug that I suspected in point (3), with regard to
> the S3 resume path. Namely:
> 
> 
> Please consider the GetAcpiCpuData() function in
> "UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c". This function:
> 
> (10a) copies the IDT and GDT *descriptors* into dynamically allocated
> SMRAM, pointed-to by the "mAcpiCpuData.GdtrProfile" and
> "mAcpiCpuData.IdtrProfile" fields;
> 
> (10b) copies the IDT and GDT themselves into dynamically allocated SMRAM,
> pointed-to by the "mGdtForAp" and "mIdtForAp" global variables,
> 
> (10c) *does not* update the "Base" members of the IDT and GDT descriptors
> that are saved in SMRAM in step (10a). Those continue to point to memory
> that was allocated by CpuS3DataDxe in the
> CpuS3DataInitialize() function.
> 
> 
> Now, consider the PrepareApStartupVector() function in
> "UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c", which runs during S3 resume.
> That
> function:
> 
> (10d) copies the IDT and GDT descriptors that were saved in (10a) into
> "mExchangeInfo" -- this is just an SMRAM-to-SMRAM copy, however the
> Base members of the descriptors point to the original CpuS3DataDxe
> allocation,
> 
> (10e) the IDT and the GDT are themselves restored from step (10b), i.e.
> from the "mGdtForAp" and "mIdtForAp" SMRAM global variables, to the
> original CpuS3DataDxe allocation address. The comment says:
> 
> >   //
> >   // Copy AP's GDT, IDT and Machine Check handler from SMRAM to ACPI
> NVS memory
> >   //
> 
> In other words, while the IDT and GDT *descriptors* aren't restored in-place,
> the IDT and GDT themselves are.
> 
> This means that the above code change causes PiSmmCpuDxeSmm to
> overwrite OS memory at S3 resume.
> 
> 
> Continuing:
> 
> On 08/08/18 09:40, Eric Dong wrote:
> > @@ -243,7 +209,7 @@ CpuS3DataInitialize (
> >      // Allocate buffer for empty RegisterTable and PreSmmInitRegisterTable
> for all CPUs
> >      //
> >      TableSize = 2 * NumberOfCpus * sizeof (CPU_REGISTER_TABLE);
> > -    RegisterTable = (CPU_REGISTER_TABLE
> *)AllocateAcpiNvsMemoryBelow4G (TableSize);
> > +    RegisterTable = AllocatePages (EFI_SIZE_TO_PAGES (TableSize));
> >      ASSERT (RegisterTable != NULL);
> >
> >      for (Index = 0; Index < NumberOfCpus; Index++) {
> 
> (11) This looks valid, beyond breaking the documentation in
> "UefiCpuPkg/Include/AcpiCpuData.h", of the members
> "PreSmmInitRegisterTable" and "RegisterTable".
> 
> 
> > diff --git a/UefiCpuPkg/CpuS3DataDxe/CpuS3DataDxe.inf
> > b/UefiCpuPkg/CpuS3DataDxe/CpuS3DataDxe.inf
> > index 480c98ebcd..c16731529c 100644
> > --- a/UefiCpuPkg/CpuS3DataDxe/CpuS3DataDxe.inf
> > +++ b/UefiCpuPkg/CpuS3DataDxe/CpuS3DataDxe.inf
> > @@ -51,6 +51,7 @@
> >    DebugLib
> >    BaseLib
> >    MtrrLib
> > +  MemoryAllocationLib
> >
> >  [Guids]
> >    gEfiEndOfDxeEventGroupGuid         ## CONSUMES   ## Event
> >
> 
> I feel uncomfortable about this patch. It is very tricky to review, and if we
> make mistakes, we could corrupt OS data, or (worse) perhaps even
> compromise SMRAM.
> 
> And, all the gains that I can imagine are, "save a few 32-bit AcpiNVS pages". I
> haven't measured, but it doesn't look like an attractive trade-off to me. And,
> platforms that disable S3 via "PcdAcpiS3Enable"
> don't allocate this memory anyway.
> 
> Furthermore, the memory footprint optimization does not seem related to
> the CpuS3DataDxe / DxeRegisterCpuFeaturesLib dependency issue that
> Marvin reported (and that is tracked in bug #959).
> 
> I suggest that we drop this patch.
> 
> Thanks,
> Laszlo
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel