* [PATCH v1 0/1] Set RW and P Attributes on Split Page @ 2022-06-15 21:23 Taylor Beebe 2022-06-15 21:23 ` [PATCH v1 1/1] UefiCpuPkg: CpuDxe: Set RW and P Attributes on Split Pages Taylor Beebe 2022-06-22 22:14 ` [edk2-devel] [PATCH v1 0/1] Set RW and P Attributes on Split Page Taylor Beebe 0 siblings, 2 replies; 4+ messages in thread From: Taylor Beebe @ 2022-06-15 21:23 UTC (permalink / raw) To: devel; +Cc: Eric Dong, Ray Ni, Rahul Kumar From: Taylor Beebe <t@taylorbeebe.com> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=3869 A split page should always have the RW and P attributes set so the lowest level page frame determines the access rights as detailed in 4.10.2.2 of the Intel 64 and IA-32 Architectures Software Developer’s Manual. Cc: Eric Dong <eric.dong@intel.com> Cc: Ray Ni <ray.ni@intel.com> Cc: Rahul Kumar <rahul1.kumar@intel.com> Taylor Beebe (1): UefiCpuPkg: CpuDxe: Set RW and P Attributes on Split Pages UefiCpuPkg/CpuDxe/CpuPageTable.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) -- 2.32.0.windows.2 ^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH v1 1/1] UefiCpuPkg: CpuDxe: Set RW and P Attributes on Split Pages 2022-06-15 21:23 [PATCH v1 0/1] Set RW and P Attributes on Split Page Taylor Beebe @ 2022-06-15 21:23 ` Taylor Beebe 2022-06-23 0:23 ` Ni, Ray 2022-06-22 22:14 ` [edk2-devel] [PATCH v1 0/1] Set RW and P Attributes on Split Page Taylor Beebe 1 sibling, 1 reply; 4+ messages in thread From: Taylor Beebe @ 2022-06-15 21:23 UTC (permalink / raw) To: devel; +Cc: Eric Dong, Ray Ni, Rahul Kumar From: Taylor Beebe <t@taylorbeebe.com> A memory range can be submitted for attribute changes which is large enough to not require a page split during the attribute update. Consider the following scenario: 1. An attribute update removed the RW attribute on a range large enough to not require a page split. 2. Later, an attributes update is called to re-add the RW attribute for a subsection of that larger page which requires a split 3. The attribute update logic performs a page split, so now the parent and child pages have matching attributes 4. Then, the attribute update logic changes the child page to have the RW attribute. 5. The child page would then correctly have the RW attribute added but the parent page would still have the RW attribute removed which will cause an improper access violation. The page being split should have loose attributes to accommodate the above case. The split page should always have the attributes set so the lowest level page frame determines the access rights as detailed in 4.10.2.2 of the Intel 64 and IA-32 Architectures Software Developer Manual. Setting the User/Supervisor attribute shouldn't be necessary. Cc: Eric Dong <eric.dong@intel.com> Cc: Ray Ni <ray.ni@intel.com> Cc: Rahul Kumar <rahul1.kumar@intel.com> Signed-off-by: Taylor Beebe <t@taylorbeebe.com> --- UefiCpuPkg/CpuDxe/CpuPageTable.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c b/UefiCpuPkg/CpuDxe/CpuPageTable.c index f7a4d92e921a..288d9996f6c3 100644 --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c @@ -38,6 +38,8 @@ #define IA32_PG_NX BIT63 #define PAGE_ATTRIBUTE_BITS (IA32_PG_D | IA32_PG_A | IA32_PG_U | IA32_PG_RW | IA32_PG_P) +#define PAGE_ATTRIBUTE_BITS_POST_SPLIT (IA32_PG_RW | IA32_PG_P) + // // Bits 1, 2, 5, 6 are reserved in the IA32 PAE PDPTE // X64 PAE PDPTE does not have such restriction @@ -583,7 +585,7 @@ SplitPage ( NewPageEntry[Index] = (BaseAddress + SIZE_4KB * Index) | AddressEncMask | ((*PageEntry) & PAGE_PROGATE_BITS); } - (*PageEntry) = (UINT64)(UINTN)NewPageEntry | AddressEncMask | ((*PageEntry) & PAGE_ATTRIBUTE_BITS); + (*PageEntry) = (UINT64)(UINTN)NewPageEntry | AddressEncMask | PAGE_ATTRIBUTE_BITS_POST_SPLIT; return RETURN_SUCCESS; } else { return RETURN_UNSUPPORTED; @@ -606,7 +608,7 @@ SplitPage ( NewPageEntry[Index] = (BaseAddress + SIZE_2MB * Index) | AddressEncMask | IA32_PG_PS | ((*PageEntry) & PAGE_PROGATE_BITS); } - (*PageEntry) = (UINT64)(UINTN)NewPageEntry | AddressEncMask | ((*PageEntry) & PAGE_ATTRIBUTE_BITS); + (*PageEntry) = (UINT64)(UINTN)NewPageEntry | AddressEncMask | PAGE_ATTRIBUTE_BITS_POST_SPLIT; return RETURN_SUCCESS; } else { return RETURN_UNSUPPORTED; -- 2.32.0.windows.2 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v1 1/1] UefiCpuPkg: CpuDxe: Set RW and P Attributes on Split Pages 2022-06-15 21:23 ` [PATCH v1 1/1] UefiCpuPkg: CpuDxe: Set RW and P Attributes on Split Pages Taylor Beebe @ 2022-06-23 0:23 ` Ni, Ray 0 siblings, 0 replies; 4+ messages in thread From: Ni, Ray @ 2022-06-23 0:23 UTC (permalink / raw) To: Taylor Beebe, devel@edk2.groups.io; +Cc: Dong, Eric, Kumar, Rahul1 Reviewed-by: Ray Ni <ray.ni@intel.com> > -----Original Message----- > From: Taylor Beebe <taylor.d.beebe@gmail.com> > Sent: Thursday, June 16, 2022 5:23 AM > To: devel@edk2.groups.io > Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Kumar, Rahul1 <rahul1.kumar@intel.com> > Subject: [PATCH v1 1/1] UefiCpuPkg: CpuDxe: Set RW and P Attributes on Split Pages > > From: Taylor Beebe <t@taylorbeebe.com> > > A memory range can be submitted for attribute changes which is large > enough to not require a page split during the attribute update. Consider > the following scenario: > > 1. An attribute update removed the RW attribute on a range large enough > to not require a page split. > 2. Later, an attributes update is called to re-add the RW attribute for > a subsection of that larger page which requires a split > 3. The attribute update logic performs a page split, so now the parent > and child pages have matching attributes > 4. Then, the attribute update logic changes the child page to have the > RW attribute. > 5. The child page would then correctly have the RW attribute added but > the parent page would still have the RW attribute removed which will > cause an improper access violation. > > The page being split should have loose attributes to accommodate the > above case. The split page should always have the attributes set so > the lowest level page frame determines the access rights as detailed > in 4.10.2.2 of the Intel 64 and IA-32 Architectures Software > Developer Manual. Setting the User/Supervisor attribute shouldn't > be necessary. > > Cc: Eric Dong <eric.dong@intel.com> > Cc: Ray Ni <ray.ni@intel.com> > Cc: Rahul Kumar <rahul1.kumar@intel.com> > Signed-off-by: Taylor Beebe <t@taylorbeebe.com> > --- > UefiCpuPkg/CpuDxe/CpuPageTable.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c b/UefiCpuPkg/CpuDxe/CpuPageTable.c > index f7a4d92e921a..288d9996f6c3 100644 > --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c > +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c > @@ -38,6 +38,8 @@ > #define IA32_PG_NX BIT63 > > #define PAGE_ATTRIBUTE_BITS (IA32_PG_D | IA32_PG_A | IA32_PG_U | IA32_PG_RW | IA32_PG_P) > +#define PAGE_ATTRIBUTE_BITS_POST_SPLIT (IA32_PG_RW | IA32_PG_P) > + > // > // Bits 1, 2, 5, 6 are reserved in the IA32 PAE PDPTE > // X64 PAE PDPTE does not have such restriction > @@ -583,7 +585,7 @@ SplitPage ( > NewPageEntry[Index] = (BaseAddress + SIZE_4KB * Index) | AddressEncMask | ((*PageEntry) & > PAGE_PROGATE_BITS); > } > > - (*PageEntry) = (UINT64)(UINTN)NewPageEntry | AddressEncMask | ((*PageEntry) & PAGE_ATTRIBUTE_BITS); > + (*PageEntry) = (UINT64)(UINTN)NewPageEntry | AddressEncMask | PAGE_ATTRIBUTE_BITS_POST_SPLIT; > return RETURN_SUCCESS; > } else { > return RETURN_UNSUPPORTED; > @@ -606,7 +608,7 @@ SplitPage ( > NewPageEntry[Index] = (BaseAddress + SIZE_2MB * Index) | AddressEncMask | IA32_PG_PS | ((*PageEntry) & > PAGE_PROGATE_BITS); > } > > - (*PageEntry) = (UINT64)(UINTN)NewPageEntry | AddressEncMask | ((*PageEntry) & PAGE_ATTRIBUTE_BITS); > + (*PageEntry) = (UINT64)(UINTN)NewPageEntry | AddressEncMask | PAGE_ATTRIBUTE_BITS_POST_SPLIT; > return RETURN_SUCCESS; > } else { > return RETURN_UNSUPPORTED; > -- > 2.32.0.windows.2 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [edk2-devel] [PATCH v1 0/1] Set RW and P Attributes on Split Page 2022-06-15 21:23 [PATCH v1 0/1] Set RW and P Attributes on Split Page Taylor Beebe 2022-06-15 21:23 ` [PATCH v1 1/1] UefiCpuPkg: CpuDxe: Set RW and P Attributes on Split Pages Taylor Beebe @ 2022-06-22 22:14 ` Taylor Beebe 1 sibling, 0 replies; 4+ messages in thread From: Taylor Beebe @ 2022-06-22 22:14 UTC (permalink / raw) To: devel; +Cc: Eric Dong, Ray Ni, Rahul Kumar Reminder to review On 6/15/2022 2:23 PM, Taylor Beebe wrote: > From: Taylor Beebe <t@taylorbeebe.com> > > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=3869 > > A split page should always have the RW and P attributes set so the lowest level page frame > determines the access rights as detailed in 4.10.2.2 of the Intel 64 and IA-32 Architectures > Software Developer’s Manual. > > Cc: Eric Dong <eric.dong@intel.com> > Cc: Ray Ni <ray.ni@intel.com> > Cc: Rahul Kumar <rahul1.kumar@intel.com> > > Taylor Beebe (1): > UefiCpuPkg: CpuDxe: Set RW and P Attributes on Split Pages > > UefiCpuPkg/CpuDxe/CpuPageTable.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-06-23 0:24 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-06-15 21:23 [PATCH v1 0/1] Set RW and P Attributes on Split Page Taylor Beebe 2022-06-15 21:23 ` [PATCH v1 1/1] UefiCpuPkg: CpuDxe: Set RW and P Attributes on Split Pages Taylor Beebe 2022-06-23 0:23 ` Ni, Ray 2022-06-22 22:14 ` [edk2-devel] [PATCH v1 0/1] Set RW and P Attributes on Split Page Taylor Beebe
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox