x86: fix NX bit handling in change_page_attr
PATCH 1/6 x86: fix NX bit handling in change_page_attr
This patch fixes a bug of change_page_attr/change_page_attr_addr on Intel i386/x86_64 CPUs. After changing page attribute to be executable with these functions, the page remains un-executable on Intel i386/x86_64 CPU. Because on Intel i386/x86_64 CPU, only if the "NX" bits of all three level page tables are cleared (PAE is enabled), the corresponding page is executable (refer to section 4.13.2 of Intel 64 and IA-32 Architectures Software Developer's Manual). So, the bug is fixed through clearing the "NX" bit of PMD when splitting the huge PMD.
--- arch/x86/mm/pageattr.c | 1 + 1 file changed, 1 insertion(+)
arch/x86/mm/pageattr.c +1 -0
@@ -124,6 +124,7 @@ static int split_large_page(pte_t *kpte,/* * Install the new, split up pagetable: */ +pgprot_val (ref_prot) &= ~_PAGE_NX;__set_pmd_pte (kpte, address,mk_pte (base, ref_prot)); base =NULL ;
Quoted 19 lines · Ying
This patch fixes a bug of change_page_attr/change_page_attr_addr on Intel i386/x86_64 CPUs. After changing page attribute to be executable with these functions, the page remains un-executable on Intel i386/x86_64 CPU. Because on Intel i386/x86_64 CPU, only if the "NX" bits of all three level page tables are cleared (PAE is enabled), the corresponding page is executable (refer to section 4.13.2 of Intel 64 and IA-32 Architectures Software Developer's Manual). So, the bug is fixed through clearing the "NX" bit of PMD when splitting the huge PMD.
Signed-off-by Huang Ying <[email protected]>--- arch/x86/mm/pageattr.c | 1 + 1 file changed, 1 insertion(+)
arch/x86/mm/pageattr.c static int split_large_page(pte_t *kpte, · line 128
Quoted 5 lines
arch/x86/mm/pageattr.c +1
-0@@ -124,6 +124,7 @@ static int split_large_page(pte_t *kpte,/* * Install the new, split up pagetable: */ +pgprot_val (ref_prot) &= ~_PAGE_NX;
I don't think its a good idea to treat pgprot_val() as an lvalue - it precludes it from being turned into an inline function. I know there are numerous other places which do, but we should avoid making it worse.
J
Quoted 11 lines · Jeremy Fitzhardinge
Quoted 9 lines · Ying
This patch fixes a bug of change_page_attr/change_page_attr_addr on Intel i386/x86_64 CPUs. After changing page attribute to be executable with these functions, the page remains un-executable on Intel i386/x86_64 CPU. Because on Intel i386/x86_64 CPU, only if the "NX" bits of all three level page tables are cleared (PAE is enabled), the corresponding page is executable (refer to section 4.13.2 of Intel 64 and IA-32 Architectures Software Developer's Manual). So, the bug is fixed through clearing the "NX" bit of PMD when splitting the huge PMD.
oops, nice detail!
Quoted 6 lines
Quoted 6 lines
Signed-off-by Huang Ying <[email protected]>--- arch/x86/mm/pageattr.c | 1 + 1 file changed, 1 insertion(+)
Quoted 13 lines
arch/x86/mm/pageattr.c static int split_large_page(pte_t *kpte, · line 128
Quoted 8 lines
arch/x86/mm/pageattr.c +1
-0@@ -124,6 +124,7 @@ static int split_large_page(pte_t *kpte,/* * Install the new, split up pagetable: */ +pgprot_val (ref_prot) &= ~_PAGE_NX;I don't think its a good idea to treat
pgprot_val()as an lvalue - it precludes it from being turned into an inline function. I know there are numerous other places which do, but we should avoid making it worse.
applied it with the following cleanup from Thomas:
static int split_large_page(pte_t *kpte, unsigned long address) {
- pgprot_t ref_prot = pte_pgprot(pte_clrhuge(*kpte));
+ pgprot_t ref_prot;
...
+ ref_prot = pte_pgprot(pte_mkexec(pte_clrhuge(*kpte)));
i.e. it now goes through all the proper accessors.
Ingo
Quoted 4 lines · Ingo Molnar
...
+ref_prot= pte_pgprot(pte_mkexec(pte_clrhuge(*kpte)));i.e. it now goes through all the proper accessors.
i guess it would be nicer to have the proper accessors to just clear the NX bit from ref-prot - instead of re-constructing the *kpte from scratch. But i see no other way to do it right now than to access pgprot_val() as an lvalue. Jeremy, what would be the best approach here?
Ingo
Quoted 10 lines · Ingo Molnar
Quoted 5 lines · Ingo Molnar
...
+ref_prot= pte_pgprot(pte_mkexec(pte_clrhuge(*kpte)));i.e. it now goes through all the proper accessors.
i guess it would be nicer to have the proper accessors to just clear the NX bit from ref-prot - instead of re-constructing the *kpte from scratch. But i see no other way to do it right now than to access
pgprot_val()as an lvalue. Jeremy, what would be the best approach here?
The pte_* accessors are really just helpers, so they're not mandatory. Something like this would be fine:
refprot = __pgprot(pgprot_val(refprot) & ~_PAGE_NX);
This is identical to the lvalue form, but avoids using it as an lvalue.
J