[PATCH 1/6] x86: fix NX bit handling in change_page_attr

[PATCH 1/6] x86: fix NX bit handling in change_page_attr

[Huang, 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 <ying.huang@intel.com>

---
 arch/x86/mm/pageattr.c |    1 +
 1 file changed, 1 insertion(+)

--- a/arch/x86/mm/pageattr.c
+++ b/arch/x86/mm/pageattr.c
@@ -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;
 

Re: [PATCH 1/6] x86: fix NX bit handling in change_page_attr

[Jeremy Fitzhardinge]

Huang, Ying wrote:
> 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 <ying.huang@intel.com>
>
> ---
>  arch/x86/mm/pageattr.c |    1 +
>  1 file changed, 1 insertion(+)
>
> --- a/arch/x86/mm/pageattr.c
> +++ b/arch/x86/mm/pageattr.c
> @@ -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

Re: [PATCH 1/6] x86: fix NX bit handling in change_page_attr

[Ingo Molnar]

* Jeremy Fitzhardinge <jeremy@goop.org> wrote:

> Huang, Ying wrote:

>> 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!

>> Signed-off-by: Huang Ying <ying.huang@intel.com>
>>
>> ---
>>  arch/x86/mm/pageattr.c |    1 +
>>  1 file changed, 1 insertion(+)
>>
>> --- a/arch/x86/mm/pageattr.c
>> +++ b/arch/x86/mm/pageattr.c
>> @@ -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

Re: [PATCH 1/6] x86: fix NX bit handling in change_page_attr

[Ingo Molnar]

* Ingo Molnar <mingo@elte.hu> wrote:

> ...
> +       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

Re: [PATCH 1/6] x86: fix NX bit handling in change_page_attr

[Jeremy Fitzhardinge]

Ingo Molnar wrote:
> * Ingo Molnar <mingo@elte.hu> wrote:
>
>   
>> ...
>> +       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