Hi Laszlo,
Thanks comments.
>
> I have two comments:
>
>
> (1) both the pre-patch code and the post-patch code have several
> instances of the following pattern:
>
> Boolean = (Expression != 0) ? TRUE : FALSE;
>
> This is an anti-pattern. It should only be:
>
> Boolean = Expression != 0;
>
> or if you prefer the parentheses, then
>
> Boolean = (Expression != 0);
>
> I recommend cleaning up all instances of this, independently of the
> actual issue.
>
Agree, I will clean it in next version.
>
> (2) The critical information is in the last paragraph of the commit
> message ("CET feature enable & disable must be in the same function to
> avoid stack mismatch"); however, that critical information is not
> visible anywhere in the new code. People will not understand why the
> code is littered with EnableCet / DisableCet calls.
>
> In fact, I only realized the weight of the commit message after I first
> looked at the patch, and deduced that the patch did, functionally
> speaking, nothing at all!
>
> So here's what I recommend: please replace the functions
>
> - EnableReadOnlyPageWriteProtect()
> - DisableReadOnlyPageWriteProtect()
>
> with *macros*
>
> - WRITE_PROTECT_RO_PAGES()
> - WRITE_UNPROTECT_RO_PAGES()
>
ok, agree, I will refine the patch with those macros definition.
> These macros should continue taking two parameters (Wp and Cet). The WP
> manipulation can be factored out to helper functions if necessary, but
> the CET manipulation needs to be expanded inline.
>
> (I've also renamed the APIs because the current API names are awkward.
> "Enable Write Protection" is just better expressed as "Write Protect",
> and "Disable Write Protection" is just better expressed as "Write
> Unprotect", in my opinion.)
>
yes. I will rename by following the suggestion.
> And then, comments on the macro definitions should explain that these
> pieces of logic are defined as macros and not functions because "CET
> feature enable & disable must be in the same function to avoid stack
> mismatch".
>
> Thanks!
> Laszlo
>
> >
> > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> > index 654935dc76..daa843b057 100644
> > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> > @@ -1554,26 +1554,24 @@ SmmWaitForApArrival (
> >
> > /**
> > Disable Write Protect on pages marked as read-only if Cr0.Bits.WP is 1.
> >
> > @param[out] WpEnabled If Cr0.WP is enabled.
> > - @param[out] CetEnabled If CET is enabled.
> > +
> > **/
> > VOID
> > DisableReadOnlyPageWriteProtect (
> > - OUT BOOLEAN *WpEnabled,
> > - OUT BOOLEAN *CetEnabled
> > + OUT BOOLEAN *WpEnabled
> > );
> >
> > /**
> > Enable Write Protect on pages marked as read-only.
> >
> > @param[out] WpEnabled If Cr0.WP should be enabled.
> > - @param[out] CetEnabled If CET should be enabled.
> > +
> > **/
> > VOID
> > EnableReadOnlyPageWriteProtect (
> > - BOOLEAN WpEnabled,
> > - BOOLEAN CetEnabled
> > + BOOLEAN WpEnabled
> > );
> >
> > #endif
> > diff --git
> a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> > index 6f49866615..2c198a161a 100644
> > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> > @@ -42,61 +42,44 @@ BOOLEAN mIsReadOnlyPageTable = FALSE;
> >
> > /**
> > Disable Write Protect on pages marked as read-only if Cr0.Bits.WP is 1.
> >
> > @param[out] WpEnabled If Cr0.WP is enabled.
> > - @param[out] CetEnabled If CET is enabled.
> > +
> > **/
> > VOID
> > DisableReadOnlyPageWriteProtect (
> > - OUT BOOLEAN *WpEnabled,
> > - OUT BOOLEAN *CetEnabled
> > + OUT BOOLEAN *WpEnabled
> > )
> > {
> > IA32_CR0 Cr0;
> >
> > - *CetEnabled = ((AsmReadCr4 () & CR4_CET_ENABLE) != 0) ? TRUE : FALSE;
> > - Cr0.UintN = AsmReadCr0 ();
> > - *WpEnabled = (Cr0.Bits.WP != 0) ? TRUE : FALSE;
> > + Cr0.UintN = AsmReadCr0 ();
> > + *WpEnabled = (Cr0.Bits.WP != 0) ? TRUE : FALSE;
> > if (*WpEnabled) {
> > - if (*CetEnabled) {
> > - //
> > - // CET must be disabled if WP is disabled. Disable CET before
> > clearing
> CR0.WP.
> > - //
> > - DisableCet ();
> > - }
> > -
> > Cr0.Bits.WP = 0;
> > AsmWriteCr0 (Cr0.UintN);
> > }
> > }
> >
> > /**
> > Enable Write Protect on pages marked as read-only.
> >
> > @param[out] WpEnabled If Cr0.WP should be enabled.
> > - @param[out] CetEnabled If CET should be enabled.
> > +
> > **/
> > VOID
> > EnableReadOnlyPageWriteProtect (
> > - BOOLEAN WpEnabled,
> > - BOOLEAN CetEnabled
> > + BOOLEAN WpEnabled
> > )
> > {
> > IA32_CR0 Cr0;
> >
> > if (WpEnabled) {
> > Cr0.UintN = AsmReadCr0 ();
> > Cr0.Bits.WP = 1;
> > AsmWriteCr0 (Cr0.UintN);
> > -
> > - if (CetEnabled) {
> > - //
> > - // re-enable CET.
> > - //
> > - EnableCet ();
> > - }
> > }
> > }
> >
> > /**
> > Initialize a buffer pool for page table use only.
> > @@ -157,13 +140,29 @@ InitializePageTablePool (
> >
> > //
> > // If page table memory has been marked as RO, mark the new pool pages
> as read-only.
> > //
> > if (mIsReadOnlyPageTable) {
> > - DisableReadOnlyPageWriteProtect (&WpEnabled, &CetEnabled);
> > + //
> > + // CET must be disabled if WP is disabled.
> > + //
> > + CetEnabled = ((AsmReadCr4 () & CR4_CET_ENABLE) != 0) ? TRUE : FALSE;
> > + if (CetEnabled) {
> > + DisableCet ();
> > + }
> > +
> > + DisableReadOnlyPageWriteProtect (&WpEnabled);
> > +
> > SmmSetMemoryAttributes ((EFI_PHYSICAL_ADDRESS)(UINTN)Buffer,
> EFI_PAGES_TO_SIZE (PoolPages), EFI_MEMORY_RO);
> > - EnableReadOnlyPageWriteProtect (WpEnabled, CetEnabled);
> > +
> > + //
> > + // Enable the WP and restore CET to enable
> > + //
> > + EnableReadOnlyPageWriteProtect (WpEnabled);
> > + if (CetEnabled) {
> > + EnableCet ();
> > + }
> > }
> >
> > return TRUE;
> > }
> >
> > @@ -1055,11 +1054,19 @@ SetMemMapAttributes (
> > Status = PageTableParse (PageTable, mPagingMode, Map, &Count);
> > }
> >
> > ASSERT_RETURN_ERROR (Status);
> >
> > - DisableReadOnlyPageWriteProtect (&WpEnabled, &CetEnabled);
> > + //
> > + // CET must be disabled if WP is disabled.
> > + //
> > + CetEnabled = ((AsmReadCr4 () & CR4_CET_ENABLE) != 0) ? TRUE : FALSE;
> > + if (CetEnabled) {
> > + DisableCet ();
> > + }
> > +
> > + DisableReadOnlyPageWriteProtect (&WpEnabled);
> >
> > MemoryMap = MemoryMapStart;
> > for (Index = 0; Index < MemoryMapEntryCount; Index++) {
> > DEBUG ((DEBUG_VERBOSE, "SetAttribute: Memory Entry - 0x%lx,
> 0x%x\n", MemoryMap->PhysicalStart, MemoryMap->NumberOfPages));
> > if (MemoryMap->Type == EfiRuntimeServicesCode) {
> > @@ -1085,11 +1092,18 @@ SetMemMapAttributes (
> > );
> >
> > MemoryMap = NEXT_MEMORY_DESCRIPTOR (MemoryMap,
> DescriptorSize);
> > }
> >
> > - EnableReadOnlyPageWriteProtect (WpEnabled, CetEnabled);
> > + //
> > + // Enable the WP and restore CET to enable
> > + //
> > + EnableReadOnlyPageWriteProtect (WpEnabled);
> > + if (CetEnabled) {
> > + EnableCet ();
> > + }
> > +
> > FreePool (Map);
> >
> > PatchSmmSaveStateMap ();
> > PatchGdtIdtMap ();
> >
> > @@ -1399,11 +1413,19 @@ SetUefiMemMapAttributes (
> >
> > PERF_FUNCTION_BEGIN ();
> >
> > DEBUG ((DEBUG_INFO, "SetUefiMemMapAttributes\n"));
> >
> > - DisableReadOnlyPageWriteProtect (&WpEnabled, &CetEnabled);
> > + //
> > + // CET must be disabled if WP is disabled.
> > + //
> > + CetEnabled = ((AsmReadCr4 () & CR4_CET_ENABLE) != 0) ? TRUE : FALSE;
> > + if (CetEnabled) {
> > + DisableCet ();
> > + }
> > +
> > + DisableReadOnlyPageWriteProtect (&WpEnabled);
> >
> > if (mUefiMemoryMap != NULL) {
> > MemoryMapEntryCount = mUefiMemoryMapSize/mUefiDescriptorSize;
> > MemoryMap = mUefiMemoryMap;
> > for (Index = 0; Index < MemoryMapEntryCount; Index++) {
> > @@ -1479,11 +1501,17 @@ SetUefiMemMapAttributes (
> >
> > Entry = NEXT_MEMORY_DESCRIPTOR (Entry,
> mUefiMemoryAttributesTable->DescriptorSize);
> > }
> > }
> >
> > - EnableReadOnlyPageWriteProtect (WpEnabled, CetEnabled);
> > + //
> > + // Enable the WP and restore CET to enable
> > + //
> > + EnableReadOnlyPageWriteProtect (WpEnabled);
> > + if (CetEnabled) {
> > + EnableCet ();
> > + }
> >
> > //
> > // Do not free mUefiMemoryAttributesTable, it will be checked in
> IsSmmCommBufferForbiddenAddress().
> > //
> >
> > @@ -1881,23 +1909,31 @@ SetPageTableAttributes (
> >
> > PERF_FUNCTION_BEGIN ();
> > DEBUG ((DEBUG_INFO, "SetPageTableAttributes\n"));
> >
> > //
> > - // Disable write protection, because we need mark page table to be write
> protected.
> > - // We need *write* page table memory, to mark itself to be *read only*.
> > + // CET must be disabled if WP is disabled.
> > //
> > - DisableReadOnlyPageWriteProtect (&WpEnabled, &CetEnabled);
> > + CetEnabled = ((AsmReadCr4 () & CR4_CET_ENABLE) != 0) ? TRUE : FALSE;
> > + if (CetEnabled) {
> > + DisableCet ();
> > + }
> > +
> > + DisableReadOnlyPageWriteProtect (&WpEnabled);
> >
> > // Set memory used by page table as Read Only.
> > DEBUG ((DEBUG_INFO, "Start...\n"));
> > EnablePageTableProtection ();
> >
> > //
> > - // Enable write protection, after page table attribute updated.
> > + // Enable the WP and restore CET to enable
> > //
> > - EnableReadOnlyPageWriteProtect (TRUE, CetEnabled);
> > + EnableReadOnlyPageWriteProtect (WpEnabled);
> > + if (CetEnabled) {
> > + EnableCet ();
> > + }
> > +
> > mIsReadOnlyPageTable = TRUE;
> >
> > //
> > // Flush TLB after mark all page table pool as read only.
> > //
> > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c
> > index 7ac3c66f91..71fdf5f34a 100644
> > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c
> > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c
> > @@ -604,11 +604,20 @@ InitPaging (
> > Limit = BASE_4GB;
> > } else {
> > Limit = (IsRestrictedMemoryAccess ()) ? LShiftU64 (1,
> mPhysicalAddressBits) : BASE_4GB;
> > }
> >
> > - DisableReadOnlyPageWriteProtect (&WpEnabled, &CetEnabled);
> > + //
> > + // CET must be disabled if WP is disabled.
> > + //
> > + CetEnabled = ((AsmReadCr4 () & CR4_CET_ENABLE) != 0) ? TRUE : FALSE;
> > + if (CetEnabled) {
> > + DisableCet ();
> > + }
> > +
> > + DisableReadOnlyPageWriteProtect (&WpEnabled);
> > +
> > //
> > // [0, 4k] may be non-present.
> > //
> > PreviousAddress = ((PcdGet8 (PcdNullPointerDetectionPropertyMask) &
> BIT1) != 0) ? BASE_4KB : 0;
> >
> > @@ -670,11 +679,17 @@ InitPaging (
> > //
> > Status = ConvertMemoryPageAttributes (PageTable, mPagingMode,
> PreviousAddress, Limit - PreviousAddress, MemoryAttrMask, TRUE, NULL);
> > ASSERT_RETURN_ERROR (Status);
> > }
> >
> > - EnableReadOnlyPageWriteProtect (WpEnabled, CetEnabled);
> > + //
> > + // Enable the WP and restore CET to enable
> > + //
> > + EnableReadOnlyPageWriteProtect (WpEnabled);
> > + if (CetEnabled) {
> > + EnableCet ();
> > + }
> >
> > //
> > // Flush TLB
> > //
> > CpuFlushTlb ();
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110665): https://edk2.groups.io/g/devel/message/110665
Mute This Topic: https://groups.io/mt/102362300/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
