> v6:
> Add ExecuteDisable feature check to include/exclude EFI_MEMORY_XP
> v5:
> Coding style clean-up
> v4:
> a. Remove DoUpdate and check attributes mismatch all the time to avoid
> a logic hole
> b. Add warning message if failed to update capability
> c. Add local variable to hold new attributes to make code cleaner
> v3:
> a. Add comment to explain more on updating memory capabilities
> b. Fix logic hole in updating attributes
> c. Instead of checking illegal memory space address and size, use return
> status of gDS->SetMemorySpaceCapabilities() to skip memory block which
> cannot be updated with new capabilities.
> v2
> a. Fix an issue which will cause setting capability failure if size is smaller
> than a page.
More than one entry of RT_CODE memory might cause boot problem for some
old OSs. This patch will fix this issue to keep OS compatibility as much
as possible.
More detailed information, please refer to
https://bugzilla.tianocore.org/show_bug.cgi?id=753
Cc: Eric Dong <eric.dong@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
---
UefiCpuPkg/CpuDxe/CpuPageTable.c | 94 +++++++++++++++++++++++++++++++---------
1 file changed, 73 insertions(+), 21 deletions(-)
diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c b/UefiCpuPkg/CpuDxe/CpuPageTable.c
index d312eb66f8..3297c1900b 100644
--- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
+++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
@@ -769,6 +769,20 @@ AssignMemoryPageAttributes (
return Status;
}
+/**
+ Check if Execute Disable feature is enabled or not.
+**/
+BOOLEAN
+IsExecuteDisableEnabled (
+ VOID
+ )
+{
+ MSR_CORE_IA32_EFER_REGISTER MsrEfer;
+
+ MsrEfer.Uint64 = AsmReadMsr64 (MSR_IA32_EFER);
+ return (MsrEfer.Bits.NXE == 1);
+}
+
/**
Update GCD memory space attributes according to current page table setup.
**/
@@ -790,7 +804,7 @@ RefreshGcdMemoryAttributesFromPaging (
UINT64 PageStartAddress;
UINT64 Attributes;
UINT64 Capabilities;
- BOOLEAN DoUpdate;
+ UINT64 NewAttributes;
UINTN Index;
//
@@ -802,17 +816,50 @@ RefreshGcdMemoryAttributesFromPaging (
GetCurrentPagingContext (&PagingContext);
- DoUpdate = FALSE;
- Capabilities = 0;
- Attributes = 0;
- BaseAddress = 0;
- PageLength = 0;
+ Attributes = 0;
+ NewAttributes = 0;
+ BaseAddress = 0;
+ PageLength = 0;
+
+ if (IsExecuteDisableEnabled ()) {
+ Capabilities = EFI_MEMORY_RO | EFI_MEMORY_RP | EFI_MEMORY_XP;
+ } else {
+ Capabilities = EFI_MEMORY_RO | EFI_MEMORY_RP;
+ }
for (Index = 0; Index < NumberOfDescriptors; Index++) {
if (MemorySpaceMap[Index].GcdMemoryType == EfiGcdMemoryTypeNonExistent) {
continue;
}
+ //
+ // Sync the actual paging related capabilities back to GCD service first.
+ // As a side effect (good one), this can also help to avoid unnecessary
+ // memory map entries due to the different capabilities of the same type
+ // memory, such as multiple RT_CODE and RT_DATA entries in memory map,
+ // which could cause boot failure of some old Linux distro (before v4.3).
+ //
+ Status = gDS->SetMemorySpaceCapabilities (
+ MemorySpaceMap[Index].BaseAddress,
+ MemorySpaceMap[Index].Length,
+ MemorySpaceMap[Index].Capabilities | Capabilities
+ );
+ if (EFI_ERROR (Status)) {
+ //
+ // If we cannot udpate the capabilities, we cannot update its
+ // attributes either. So just simply skip current block of memory.
+ //
+ DEBUG ((
+ DEBUG_WARN,
+ "Failed to update capability: [%lu] %016lx - %016lx (%016lx -> %016lx)\r\n",
+ (UINT64)Index, MemorySpaceMap[Index].BaseAddress,
+ MemorySpaceMap[Index].BaseAddress + MemorySpaceMap[Index].Length - 1,
+ MemorySpaceMap[Index].Capabilities,
+ MemorySpaceMap[Index].Capabilities | Capabilities
+ ));
+ continue;
+ }
+
if (MemorySpaceMap[Index].BaseAddress >= (BaseAddress + PageLength)) {
//
// Current memory space starts at a new page. Resetting PageLength will
@@ -826,7 +873,9 @@ RefreshGcdMemoryAttributesFromPaging (
PageLength -= (MemorySpaceMap[Index].BaseAddress - BaseAddress);
}
- // Sync real page attributes to GCD
+ //
+ // Sync actual page attributes to GCD
+ //
BaseAddress = MemorySpaceMap[Index].BaseAddress;
MemorySpaceLength = MemorySpaceMap[Index].Length;
while (MemorySpaceLength > 0) {
@@ -842,23 +891,26 @@ RefreshGcdMemoryAttributesFromPaging (
PageStartAddress = (*PageEntry) & (UINT64)PageAttributeToMask(PageAttribute);
PageLength = PageAttributeToLength (PageAttribute) - (BaseAddress - PageStartAddress);
Attributes = GetAttributesFromPageEntry (PageEntry);
-
- if (Attributes != (MemorySpaceMap[Index].Attributes & EFI_MEMORY_PAGETYPE_MASK)) {
- DoUpdate = TRUE;
- Attributes |= (MemorySpaceMap[Index].Attributes & ~EFI_MEMORY_PAGETYPE_MASK);
- Capabilities = Attributes | MemorySpaceMap[Index].Capabilities;
- } else {
- DoUpdate = FALSE;
- }
}
Length = MIN (PageLength, MemorySpaceLength);
- if (DoUpdate) {
- gDS->SetMemorySpaceCapabilities (BaseAddress, Length, Capabilities);
- gDS->SetMemorySpaceAttributes (BaseAddress, Length, Attributes);
- DEBUG ((DEBUG_INFO, "Update memory space attribute: [%02d] %016lx - %016lx (%08lx -> %08lx)\r\n",
- Index, BaseAddress, BaseAddress + Length - 1,
- MemorySpaceMap[Index].Attributes, Attributes));
+ if (Attributes != (MemorySpaceMap[Index].Attributes &
+ EFI_MEMORY_PAGETYPE_MASK)) {
+ NewAttributes = (MemorySpaceMap[Index].Attributes &
+ ~EFI_MEMORY_PAGETYPE_MASK) | Attributes;
+ Status = gDS->SetMemorySpaceAttributes (
+ BaseAddress,
+ Length,
+ NewAttributes
+ );
+ ASSERT_EFI_ERROR (Status);
+ DEBUG ((
+ DEBUG_INFO,
+ "Updated memory space attribute: [%lu] %016lx - %016lx (%016lx -> %016lx)\r\n",
+ (UINT64)Index, BaseAddress, BaseAddress + Length - 1,
+ MemorySpaceMap[Index].Attributes,
+ NewAttributes
+ ));
}
PageLength -= Length;
--
2.14.1.windows.1
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
On 11/16/17 08:27, Jian J Wang wrote:
>> v6:
>> Add ExecuteDisable feature check to include/exclude EFI_MEMORY_XP
Another change relative to v5 is the fixing of the first DEBUG_WARN
message -- in my v5 review I had missed that the DEBUG_WARN arguments
didn't match the preceding gDS->SetMemorySpaceCapabilities() arguments.
Yet another change that could have been (maybe) possible for this patch
is to replace the remaining occurrences of EFI_MEMORY_PAGETYPE_MASK with
"Capabilities". Namely, in v6, the attributes are enforced on a mask
that is possibly wider than supported by the hardware (i.e., in case NX
is not supported).
However, this should not be a functionality problem, because with NX
unavailable, the GetAttributesFromPageEntry() function should never
return EFI_MEMORY_XP. Thus, the "wider than needed" attribute setting
will just clear EFI_MEMORY_XP.
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
(I hope that Star and/or Jiewen will also R-b this patch.)
In addition, I will follow up with test results for the series.
Thanks
Laszlo
>> v5:
>> Coding style clean-up
>
>> v4:
>> a. Remove DoUpdate and check attributes mismatch all the time to avoid
>> a logic hole
>> b. Add warning message if failed to update capability
>> c. Add local variable to hold new attributes to make code cleaner
>
>> v3:
>> a. Add comment to explain more on updating memory capabilities
>> b. Fix logic hole in updating attributes
>> c. Instead of checking illegal memory space address and size, use return
>> status of gDS->SetMemorySpaceCapabilities() to skip memory block which
>> cannot be updated with new capabilities.
>
>> v2
>> a. Fix an issue which will cause setting capability failure if size is smaller
>> than a page.
>
> More than one entry of RT_CODE memory might cause boot problem for some
> old OSs. This patch will fix this issue to keep OS compatibility as much
> as possible.
>
> More detailed information, please refer to
> https://bugzilla.tianocore.org/show_bug.cgi?id=753
>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> ---
> UefiCpuPkg/CpuDxe/CpuPageTable.c | 94 +++++++++++++++++++++++++++++++---------
> 1 file changed, 73 insertions(+), 21 deletions(-)
>
> diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> index d312eb66f8..3297c1900b 100644
> --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> @@ -769,6 +769,20 @@ AssignMemoryPageAttributes (
> return Status;
> }
>
> +/**
> + Check if Execute Disable feature is enabled or not.
> +**/
> +BOOLEAN
> +IsExecuteDisableEnabled (
> + VOID
> + )
> +{
> + MSR_CORE_IA32_EFER_REGISTER MsrEfer;
> +
> + MsrEfer.Uint64 = AsmReadMsr64 (MSR_IA32_EFER);
> + return (MsrEfer.Bits.NXE == 1);
> +}
> +
> /**
> Update GCD memory space attributes according to current page table setup.
> **/
> @@ -790,7 +804,7 @@ RefreshGcdMemoryAttributesFromPaging (
> UINT64 PageStartAddress;
> UINT64 Attributes;
> UINT64 Capabilities;
> - BOOLEAN DoUpdate;
> + UINT64 NewAttributes;
> UINTN Index;
>
> //
> @@ -802,17 +816,50 @@ RefreshGcdMemoryAttributesFromPaging (
>
> GetCurrentPagingContext (&PagingContext);
>
> - DoUpdate = FALSE;
> - Capabilities = 0;
> - Attributes = 0;
> - BaseAddress = 0;
> - PageLength = 0;
> + Attributes = 0;
> + NewAttributes = 0;
> + BaseAddress = 0;
> + PageLength = 0;
> +
> + if (IsExecuteDisableEnabled ()) {
> + Capabilities = EFI_MEMORY_RO | EFI_MEMORY_RP | EFI_MEMORY_XP;
> + } else {
> + Capabilities = EFI_MEMORY_RO | EFI_MEMORY_RP;
> + }
>
> for (Index = 0; Index < NumberOfDescriptors; Index++) {
> if (MemorySpaceMap[Index].GcdMemoryType == EfiGcdMemoryTypeNonExistent) {
> continue;
> }
>
> + //
> + // Sync the actual paging related capabilities back to GCD service first.
> + // As a side effect (good one), this can also help to avoid unnecessary
> + // memory map entries due to the different capabilities of the same type
> + // memory, such as multiple RT_CODE and RT_DATA entries in memory map,
> + // which could cause boot failure of some old Linux distro (before v4.3).
> + //
> + Status = gDS->SetMemorySpaceCapabilities (
> + MemorySpaceMap[Index].BaseAddress,
> + MemorySpaceMap[Index].Length,
> + MemorySpaceMap[Index].Capabilities | Capabilities
> + );
> + if (EFI_ERROR (Status)) {
> + //
> + // If we cannot udpate the capabilities, we cannot update its
> + // attributes either. So just simply skip current block of memory.
> + //
> + DEBUG ((
> + DEBUG_WARN,
> + "Failed to update capability: [%lu] %016lx - %016lx (%016lx -> %016lx)\r\n",
> + (UINT64)Index, MemorySpaceMap[Index].BaseAddress,
> + MemorySpaceMap[Index].BaseAddress + MemorySpaceMap[Index].Length - 1,
> + MemorySpaceMap[Index].Capabilities,
> + MemorySpaceMap[Index].Capabilities | Capabilities
> + ));
> + continue;
> + }
> +
> if (MemorySpaceMap[Index].BaseAddress >= (BaseAddress + PageLength)) {
> //
> // Current memory space starts at a new page. Resetting PageLength will
> @@ -826,7 +873,9 @@ RefreshGcdMemoryAttributesFromPaging (
> PageLength -= (MemorySpaceMap[Index].BaseAddress - BaseAddress);
> }
>
> - // Sync real page attributes to GCD
> + //
> + // Sync actual page attributes to GCD
> + //
> BaseAddress = MemorySpaceMap[Index].BaseAddress;
> MemorySpaceLength = MemorySpaceMap[Index].Length;
> while (MemorySpaceLength > 0) {
> @@ -842,23 +891,26 @@ RefreshGcdMemoryAttributesFromPaging (
> PageStartAddress = (*PageEntry) & (UINT64)PageAttributeToMask(PageAttribute);
> PageLength = PageAttributeToLength (PageAttribute) - (BaseAddress - PageStartAddress);
> Attributes = GetAttributesFromPageEntry (PageEntry);
> -
> - if (Attributes != (MemorySpaceMap[Index].Attributes & EFI_MEMORY_PAGETYPE_MASK)) {
> - DoUpdate = TRUE;
> - Attributes |= (MemorySpaceMap[Index].Attributes & ~EFI_MEMORY_PAGETYPE_MASK);
> - Capabilities = Attributes | MemorySpaceMap[Index].Capabilities;
> - } else {
> - DoUpdate = FALSE;
> - }
> }
>
> Length = MIN (PageLength, MemorySpaceLength);
> - if (DoUpdate) {
> - gDS->SetMemorySpaceCapabilities (BaseAddress, Length, Capabilities);
> - gDS->SetMemorySpaceAttributes (BaseAddress, Length, Attributes);
> - DEBUG ((DEBUG_INFO, "Update memory space attribute: [%02d] %016lx - %016lx (%08lx -> %08lx)\r\n",
> - Index, BaseAddress, BaseAddress + Length - 1,
> - MemorySpaceMap[Index].Attributes, Attributes));
> + if (Attributes != (MemorySpaceMap[Index].Attributes &
> + EFI_MEMORY_PAGETYPE_MASK)) {
> + NewAttributes = (MemorySpaceMap[Index].Attributes &
> + ~EFI_MEMORY_PAGETYPE_MASK) | Attributes;
> + Status = gDS->SetMemorySpaceAttributes (
> + BaseAddress,
> + Length,
> + NewAttributes
> + );
> + ASSERT_EFI_ERROR (Status);
> + DEBUG ((
> + DEBUG_INFO,
> + "Updated memory space attribute: [%lu] %016lx - %016lx (%016lx -> %016lx)\r\n",
> + (UINT64)Index, BaseAddress, BaseAddress + Length - 1,
> + MemorySpaceMap[Index].Attributes,
> + NewAttributes
> + ));
> }
>
> PageLength -= Length;
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
You're right about XP/NX in this function. But from DxeCore or other drivers
perspective, they have no knowledge of current capability of NX. I think it's the
responsibility of cpu driver to add/remove it for the sake of GCD.
Actually Star has filed a bz to use GCD service instead of CPU arch protocol to
change memory attributes in DxeCore (like enforce image protection). If the
knowledge between GCD and cpu mismatch, the GCD may not do right thing
upon requests. For example, if we always add XP to capability but current cpu
doesn't support it, the DxeCore or other drivers may still try to enable image
protection which won't take into effect actually.
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Tuesday, November 21, 2017 4:32 AM
> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>;
> Zeng, Star <star.zeng@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Subject: Re: [edk2] [PATCH 2/2] UefiCpuPkg/CpuDxe: Fix multiple entries of
> RT_CODE in memory map
>
> On 11/16/17 08:27, Jian J Wang wrote:
> >> v6:
> >> Add ExecuteDisable feature check to include/exclude EFI_MEMORY_XP
>
> Another change relative to v5 is the fixing of the first DEBUG_WARN
> message -- in my v5 review I had missed that the DEBUG_WARN arguments
> didn't match the preceding gDS->SetMemorySpaceCapabilities() arguments.
>
> Yet another change that could have been (maybe) possible for this patch
> is to replace the remaining occurrences of EFI_MEMORY_PAGETYPE_MASK with
> "Capabilities". Namely, in v6, the attributes are enforced on a mask
> that is possibly wider than supported by the hardware (i.e., in case NX
> is not supported).
>
> However, this should not be a functionality problem, because with NX
> unavailable, the GetAttributesFromPageEntry() function should never
> return EFI_MEMORY_XP. Thus, the "wider than needed" attribute setting
> will just clear EFI_MEMORY_XP.
>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>
> (I hope that Star and/or Jiewen will also R-b this patch.)
>
> In addition, I will follow up with test results for the series.
>
> Thanks
> Laszlo
>
> >> v5:
> >> Coding style clean-up
> >
> >> v4:
> >> a. Remove DoUpdate and check attributes mismatch all the time to avoid
> >> a logic hole
> >> b. Add warning message if failed to update capability
> >> c. Add local variable to hold new attributes to make code cleaner
> >
> >> v3:
> >> a. Add comment to explain more on updating memory capabilities
> >> b. Fix logic hole in updating attributes
> >> c. Instead of checking illegal memory space address and size, use return
> >> status of gDS->SetMemorySpaceCapabilities() to skip memory block which
> >> cannot be updated with new capabilities.
> >
> >> v2
> >> a. Fix an issue which will cause setting capability failure if size is smaller
> >> than a page.
> >
> > More than one entry of RT_CODE memory might cause boot problem for
> some
> > old OSs. This patch will fix this issue to keep OS compatibility as much
> > as possible.
> >
> > More detailed information, please refer to
> > https://bugzilla.tianocore.org/show_bug.cgi?id=753
> >
> > Cc: Eric Dong <eric.dong@intel.com>
> > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > Cc: Star Zeng <star.zeng@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> > ---
> > UefiCpuPkg/CpuDxe/CpuPageTable.c | 94
> +++++++++++++++++++++++++++++++---------
> > 1 file changed, 73 insertions(+), 21 deletions(-)
> >
> > diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> > index d312eb66f8..3297c1900b 100644
> > --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> > +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> > @@ -769,6 +769,20 @@ AssignMemoryPageAttributes (
> > return Status;
> > }
> >
> > +/**
> > + Check if Execute Disable feature is enabled or not.
> > +**/
> > +BOOLEAN
> > +IsExecuteDisableEnabled (
> > + VOID
> > + )
> > +{
> > + MSR_CORE_IA32_EFER_REGISTER MsrEfer;
> > +
> > + MsrEfer.Uint64 = AsmReadMsr64 (MSR_IA32_EFER);
> > + return (MsrEfer.Bits.NXE == 1);
> > +}
> > +
> > /**
> > Update GCD memory space attributes according to current page table setup.
> > **/
> > @@ -790,7 +804,7 @@ RefreshGcdMemoryAttributesFromPaging (
> > UINT64 PageStartAddress;
> > UINT64 Attributes;
> > UINT64 Capabilities;
> > - BOOLEAN DoUpdate;
> > + UINT64 NewAttributes;
> > UINTN Index;
> >
> > //
> > @@ -802,17 +816,50 @@ RefreshGcdMemoryAttributesFromPaging (
> >
> > GetCurrentPagingContext (&PagingContext);
> >
> > - DoUpdate = FALSE;
> > - Capabilities = 0;
> > - Attributes = 0;
> > - BaseAddress = 0;
> > - PageLength = 0;
> > + Attributes = 0;
> > + NewAttributes = 0;
> > + BaseAddress = 0;
> > + PageLength = 0;
> > +
> > + if (IsExecuteDisableEnabled ()) {
> > + Capabilities = EFI_MEMORY_RO | EFI_MEMORY_RP | EFI_MEMORY_XP;
> > + } else {
> > + Capabilities = EFI_MEMORY_RO | EFI_MEMORY_RP;
> > + }
> >
> > for (Index = 0; Index < NumberOfDescriptors; Index++) {
> > if (MemorySpaceMap[Index].GcdMemoryType ==
> EfiGcdMemoryTypeNonExistent) {
> > continue;
> > }
> >
> > + //
> > + // Sync the actual paging related capabilities back to GCD service first.
> > + // As a side effect (good one), this can also help to avoid unnecessary
> > + // memory map entries due to the different capabilities of the same type
> > + // memory, such as multiple RT_CODE and RT_DATA entries in memory
> map,
> > + // which could cause boot failure of some old Linux distro (before v4.3).
> > + //
> > + Status = gDS->SetMemorySpaceCapabilities (
> > + MemorySpaceMap[Index].BaseAddress,
> > + MemorySpaceMap[Index].Length,
> > + MemorySpaceMap[Index].Capabilities | Capabilities
> > + );
> > + if (EFI_ERROR (Status)) {
> > + //
> > + // If we cannot udpate the capabilities, we cannot update its
> > + // attributes either. So just simply skip current block of memory.
> > + //
> > + DEBUG ((
> > + DEBUG_WARN,
> > + "Failed to update capability: [%lu] %016lx - %016lx (%016lx -
> > %016lx)\r\n",
> > + (UINT64)Index, MemorySpaceMap[Index].BaseAddress,
> > + MemorySpaceMap[Index].BaseAddress +
> MemorySpaceMap[Index].Length - 1,
> > + MemorySpaceMap[Index].Capabilities,
> > + MemorySpaceMap[Index].Capabilities | Capabilities
> > + ));
> > + continue;
> > + }
> > +
> > if (MemorySpaceMap[Index].BaseAddress >= (BaseAddress + PageLength)) {
> > //
> > // Current memory space starts at a new page. Resetting PageLength will
> > @@ -826,7 +873,9 @@ RefreshGcdMemoryAttributesFromPaging (
> > PageLength -= (MemorySpaceMap[Index].BaseAddress - BaseAddress);
> > }
> >
> > - // Sync real page attributes to GCD
> > + //
> > + // Sync actual page attributes to GCD
> > + //
> > BaseAddress = MemorySpaceMap[Index].BaseAddress;
> > MemorySpaceLength = MemorySpaceMap[Index].Length;
> > while (MemorySpaceLength > 0) {
> > @@ -842,23 +891,26 @@ RefreshGcdMemoryAttributesFromPaging (
> > PageStartAddress = (*PageEntry) &
> (UINT64)PageAttributeToMask(PageAttribute);
> > PageLength = PageAttributeToLength (PageAttribute) - (BaseAddress -
> PageStartAddress);
> > Attributes = GetAttributesFromPageEntry (PageEntry);
> > -
> > - if (Attributes != (MemorySpaceMap[Index].Attributes &
> EFI_MEMORY_PAGETYPE_MASK)) {
> > - DoUpdate = TRUE;
> > - Attributes |= (MemorySpaceMap[Index].Attributes &
> ~EFI_MEMORY_PAGETYPE_MASK);
> > - Capabilities = Attributes | MemorySpaceMap[Index].Capabilities;
> > - } else {
> > - DoUpdate = FALSE;
> > - }
> > }
> >
> > Length = MIN (PageLength, MemorySpaceLength);
> > - if (DoUpdate) {
> > - gDS->SetMemorySpaceCapabilities (BaseAddress, Length, Capabilities);
> > - gDS->SetMemorySpaceAttributes (BaseAddress, Length, Attributes);
> > - DEBUG ((DEBUG_INFO, "Update memory space attribute: [%02d] %016lx
> - %016lx (%08lx -> %08lx)\r\n",
> > - Index, BaseAddress, BaseAddress + Length - 1,
> > - MemorySpaceMap[Index].Attributes, Attributes));
> > + if (Attributes != (MemorySpaceMap[Index].Attributes &
> > + EFI_MEMORY_PAGETYPE_MASK)) {
> > + NewAttributes = (MemorySpaceMap[Index].Attributes &
> > + ~EFI_MEMORY_PAGETYPE_MASK) | Attributes;
> > + Status = gDS->SetMemorySpaceAttributes (
> > + BaseAddress,
> > + Length,
> > + NewAttributes
> > + );
> > + ASSERT_EFI_ERROR (Status);
> > + DEBUG ((
> > + DEBUG_INFO,
> > + "Updated memory space attribute: [%lu] %016lx - %016lx (%016lx -
> > %016lx)\r\n",
> > + (UINT64)Index, BaseAddress, BaseAddress + Length - 1,
> > + MemorySpaceMap[Index].Attributes,
> > + NewAttributes
> > + ));
> > }
> >
> > PageLength -= Length;
> >
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Reviewed-by: Star Zeng <star.zeng@intel.com>
-----Original Message-----
From: Wang, Jian J
Sent: Thursday, November 16, 2017 3:27 PM
To: edk2-devel@lists.01.org
Cc: Dong, Eric <eric.dong@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Zeng, Star <star.zeng@intel.com>; Laszlo Ersek <lersek@redhat.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
Subject: [PATCH 2/2] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map
> v6:
> Add ExecuteDisable feature check to include/exclude EFI_MEMORY_XP
> v5:
> Coding style clean-up
> v4:
> a. Remove DoUpdate and check attributes mismatch all the time to avoid
> a logic hole
> b. Add warning message if failed to update capability c. Add local
> variable to hold new attributes to make code cleaner
> v3:
> a. Add comment to explain more on updating memory capabilities b. Fix
> logic hole in updating attributes c. Instead of checking illegal
> memory space address and size, use return
> status of gDS->SetMemorySpaceCapabilities() to skip memory block which
> cannot be updated with new capabilities.
> v2
> a. Fix an issue which will cause setting capability failure if size is smaller
> than a page.
More than one entry of RT_CODE memory might cause boot problem for some old OSs. This patch will fix this issue to keep OS compatibility as much as possible.
More detailed information, please refer to
https://bugzilla.tianocore.org/show_bug.cgi?id=753
Cc: Eric Dong <eric.dong@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
---
UefiCpuPkg/CpuDxe/CpuPageTable.c | 94 +++++++++++++++++++++++++++++++---------
1 file changed, 73 insertions(+), 21 deletions(-)
diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c b/UefiCpuPkg/CpuDxe/CpuPageTable.c
index d312eb66f8..3297c1900b 100644
--- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
+++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
@@ -769,6 +769,20 @@ AssignMemoryPageAttributes (
return Status;
}
+/**
+ Check if Execute Disable feature is enabled or not.
+**/
+BOOLEAN
+IsExecuteDisableEnabled (
+ VOID
+ )
+{
+ MSR_CORE_IA32_EFER_REGISTER MsrEfer;
+
+ MsrEfer.Uint64 = AsmReadMsr64 (MSR_IA32_EFER);
+ return (MsrEfer.Bits.NXE == 1);
+}
+
/**
Update GCD memory space attributes according to current page table setup.
**/
@@ -790,7 +804,7 @@ RefreshGcdMemoryAttributesFromPaging (
UINT64 PageStartAddress;
UINT64 Attributes;
UINT64 Capabilities;
- BOOLEAN DoUpdate;
+ UINT64 NewAttributes;
UINTN Index;
//
@@ -802,17 +816,50 @@ RefreshGcdMemoryAttributesFromPaging (
GetCurrentPagingContext (&PagingContext);
- DoUpdate = FALSE;
- Capabilities = 0;
- Attributes = 0;
- BaseAddress = 0;
- PageLength = 0;
+ Attributes = 0;
+ NewAttributes = 0;
+ BaseAddress = 0;
+ PageLength = 0;
+
+ if (IsExecuteDisableEnabled ()) {
+ Capabilities = EFI_MEMORY_RO | EFI_MEMORY_RP | EFI_MEMORY_XP; }
+ else {
+ Capabilities = EFI_MEMORY_RO | EFI_MEMORY_RP; }
for (Index = 0; Index < NumberOfDescriptors; Index++) {
if (MemorySpaceMap[Index].GcdMemoryType == EfiGcdMemoryTypeNonExistent) {
continue;
}
+ //
+ // Sync the actual paging related capabilities back to GCD service first.
+ // As a side effect (good one), this can also help to avoid unnecessary
+ // memory map entries due to the different capabilities of the same type
+ // memory, such as multiple RT_CODE and RT_DATA entries in memory map,
+ // which could cause boot failure of some old Linux distro (before v4.3).
+ //
+ Status = gDS->SetMemorySpaceCapabilities (
+ MemorySpaceMap[Index].BaseAddress,
+ MemorySpaceMap[Index].Length,
+ MemorySpaceMap[Index].Capabilities | Capabilities
+ );
+ if (EFI_ERROR (Status)) {
+ //
+ // If we cannot udpate the capabilities, we cannot update its
+ // attributes either. So just simply skip current block of memory.
+ //
+ DEBUG ((
+ DEBUG_WARN,
+ "Failed to update capability: [%lu] %016lx - %016lx (%016lx -> %016lx)\r\n",
+ (UINT64)Index, MemorySpaceMap[Index].BaseAddress,
+ MemorySpaceMap[Index].BaseAddress + MemorySpaceMap[Index].Length - 1,
+ MemorySpaceMap[Index].Capabilities,
+ MemorySpaceMap[Index].Capabilities | Capabilities
+ ));
+ continue;
+ }
+
if (MemorySpaceMap[Index].BaseAddress >= (BaseAddress + PageLength)) {
//
// Current memory space starts at a new page. Resetting PageLength will @@ -826,7 +873,9 @@ RefreshGcdMemoryAttributesFromPaging (
PageLength -= (MemorySpaceMap[Index].BaseAddress - BaseAddress);
}
- // Sync real page attributes to GCD
+ //
+ // Sync actual page attributes to GCD
+ //
BaseAddress = MemorySpaceMap[Index].BaseAddress;
MemorySpaceLength = MemorySpaceMap[Index].Length;
while (MemorySpaceLength > 0) {
@@ -842,23 +891,26 @@ RefreshGcdMemoryAttributesFromPaging (
PageStartAddress = (*PageEntry) & (UINT64)PageAttributeToMask(PageAttribute);
PageLength = PageAttributeToLength (PageAttribute) - (BaseAddress - PageStartAddress);
Attributes = GetAttributesFromPageEntry (PageEntry);
-
- if (Attributes != (MemorySpaceMap[Index].Attributes & EFI_MEMORY_PAGETYPE_MASK)) {
- DoUpdate = TRUE;
- Attributes |= (MemorySpaceMap[Index].Attributes & ~EFI_MEMORY_PAGETYPE_MASK);
- Capabilities = Attributes | MemorySpaceMap[Index].Capabilities;
- } else {
- DoUpdate = FALSE;
- }
}
Length = MIN (PageLength, MemorySpaceLength);
- if (DoUpdate) {
- gDS->SetMemorySpaceCapabilities (BaseAddress, Length, Capabilities);
- gDS->SetMemorySpaceAttributes (BaseAddress, Length, Attributes);
- DEBUG ((DEBUG_INFO, "Update memory space attribute: [%02d] %016lx - %016lx (%08lx -> %08lx)\r\n",
- Index, BaseAddress, BaseAddress + Length - 1,
- MemorySpaceMap[Index].Attributes, Attributes));
+ if (Attributes != (MemorySpaceMap[Index].Attributes &
+ EFI_MEMORY_PAGETYPE_MASK)) {
+ NewAttributes = (MemorySpaceMap[Index].Attributes &
+ ~EFI_MEMORY_PAGETYPE_MASK) | Attributes;
+ Status = gDS->SetMemorySpaceAttributes (
+ BaseAddress,
+ Length,
+ NewAttributes
+ );
+ ASSERT_EFI_ERROR (Status);
+ DEBUG ((
+ DEBUG_INFO,
+ "Updated memory space attribute: [%lu] %016lx - %016lx (%016lx -> %016lx)\r\n",
+ (UINT64)Index, BaseAddress, BaseAddress + Length - 1,
+ MemorySpaceMap[Index].Attributes,
+ NewAttributes
+ ));
}
PageLength -= Length;
--
2.14.1.windows.1
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
© 2016 - 2025 Red Hat, Inc.