linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Christophe Leroy <christophe.leroy@csgroup.eu>
To: Benjamin Herrenschmidt <benh@kernel.crashing.org>,
	Paul Mackerras <paulus@samba.org>,
	Michael Ellerman <mpe@ellerman.id.au>
Cc: linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 02/10] powerpc/book3e: Fix set_memory_x() and set_memory_nx()
Date: Tue, 26 Oct 2021 09:03:41 +0200	[thread overview]
Message-ID: <40347194-fcc6-307b-0ddd-c8a221424dd4@csgroup.eu> (raw)
In-Reply-To: <5794f254-0523-7f2f-f9e7-ff64a7fe400d@csgroup.eu>



Le 25/10/2021 à 23:53, Christophe Leroy a écrit :
> 
> 
> On 23/10/2021 13:47, Christophe Leroy wrote:
>> set_memory_x() calls pte_mkexec() which sets _PAGE_EXEC.
>> set_memory_nx() calls pte_exprotec() which clears _PAGE_EXEC.
>>
>> Book3e has 2 bits, UX and SX, which defines the exec rights
>> resp. for user (PR=1) and for kernel (PR=0).
>>
>> _PAGE_EXEC is defined as UX only.
>>
>> An executable kernel page is set with either _PAGE_KERNEL_RWX
>> or _PAGE_KERNEL_ROX, which both have SX set and UX cleared.
>>
>> So set_memory_nx() call for an executable kernel page does
>> nothing because UX is already cleared.
>>
>> And set_memory_x() on a non-executable kernel page makes it
>> executable for the user and keeps it non-executable for kernel.
>>
>> Also, pte_exec() always returns 'false' on kernel pages, because
>> it checks _PAGE_EXEC which doesn't include SX, so for instance
>> the W+X check doesn't work.
>>
>> To fix this:
>> - change tlb_low_64e.S to use _PAGE_BAP_UX instead of _PAGE_USER
>> - sets both UX and SX in _PAGE_EXEC so that pte_user() returns
>> true whenever one of the two bits is set and pte_exprotect()
>> clears both bits.
>> - Define a book3e specific version of pte_mkexec() which sets
>> either SX or UX based on UR.
>>
>> Fixes: 1f9ad21c3b38 ("powerpc/mm: Implement set_memory() routines")
>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>> ---
>> v2: New
> 
> pte_mkexec() in nohash/64/pgtable.h conflicts with the one in 
> nohash/pte_book3e.h
> 
> Should guard it with  #ifndef pte_mkexec(), but as pte_book3e is the 
> only user in 64 bits, then just remove it from there.
> 
> Send v3 with only that change compared to v2.

Series v1 was merged into next so I submitted followup series with the 
three fixes.

Christophe

> 
> Christophe
> 
>> ---
>>   arch/powerpc/include/asm/nohash/32/pgtable.h |  2 ++
>>   arch/powerpc/include/asm/nohash/pte-book3e.h | 18 ++++++++++++++----
>>   arch/powerpc/mm/nohash/tlb_low_64e.S         |  8 ++++----
>>   3 files changed, 20 insertions(+), 8 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/nohash/32/pgtable.h 
>> b/arch/powerpc/include/asm/nohash/32/pgtable.h
>> index ac0a5ff48c3a..d6ba821a56ce 100644
>> --- a/arch/powerpc/include/asm/nohash/32/pgtable.h
>> +++ b/arch/powerpc/include/asm/nohash/32/pgtable.h
>> @@ -193,10 +193,12 @@ static inline pte_t pte_wrprotect(pte_t pte)
>>   }
>>   #endif
>> +#ifndef pte_mkexec
>>   static inline pte_t pte_mkexec(pte_t pte)
>>   {
>>       return __pte(pte_val(pte) | _PAGE_EXEC);
>>   }
>> +#endif
>>   #define pmd_none(pmd)        (!pmd_val(pmd))
>>   #define    pmd_bad(pmd)        (pmd_val(pmd) & _PMD_BAD)
>> diff --git a/arch/powerpc/include/asm/nohash/pte-book3e.h 
>> b/arch/powerpc/include/asm/nohash/pte-book3e.h
>> index 813918f40765..f798640422c2 100644
>> --- a/arch/powerpc/include/asm/nohash/pte-book3e.h
>> +++ b/arch/powerpc/include/asm/nohash/pte-book3e.h
>> @@ -48,7 +48,7 @@
>>   #define _PAGE_WRITETHRU    0x800000 /* W: cache write-through */
>>   /* "Higher level" linux bit combinations */
>> -#define _PAGE_EXEC        _PAGE_BAP_UX /* .. and was cache cleaned */
>> +#define _PAGE_EXEC        (_PAGE_BAP_SX | _PAGE_BAP_UX) /* .. and was 
>> cache cleaned */
>>   #define _PAGE_RW        (_PAGE_BAP_SW | _PAGE_BAP_UW) /* User write 
>> permission */
>>   #define _PAGE_KERNEL_RW        (_PAGE_BAP_SW | _PAGE_BAP_SR | 
>> _PAGE_DIRTY)
>>   #define _PAGE_KERNEL_RO        (_PAGE_BAP_SR)
>> @@ -93,11 +93,11 @@
>>   /* Permission masks used to generate the __P and __S table */
>>   #define PAGE_NONE    __pgprot(_PAGE_BASE)
>>   #define PAGE_SHARED    __pgprot(_PAGE_BASE | _PAGE_USER | _PAGE_RW)
>> -#define PAGE_SHARED_X    __pgprot(_PAGE_BASE | _PAGE_USER | _PAGE_RW 
>> | _PAGE_EXEC)
>> +#define PAGE_SHARED_X    __pgprot(_PAGE_BASE | _PAGE_USER | _PAGE_RW 
>> | _PAGE_BAP_UX)
>>   #define PAGE_COPY    __pgprot(_PAGE_BASE | _PAGE_USER)
>> -#define PAGE_COPY_X    __pgprot(_PAGE_BASE | _PAGE_USER | _PAGE_EXEC)
>> +#define PAGE_COPY_X    __pgprot(_PAGE_BASE | _PAGE_USER | _PAGE_BAP_UX)
>>   #define PAGE_READONLY    __pgprot(_PAGE_BASE | _PAGE_USER)
>> -#define PAGE_READONLY_X    __pgprot(_PAGE_BASE | _PAGE_USER | 
>> _PAGE_EXEC)
>> +#define PAGE_READONLY_X    __pgprot(_PAGE_BASE | _PAGE_USER | 
>> _PAGE_BAP_UX)
>>   #ifndef __ASSEMBLY__
>>   static inline pte_t pte_mkprivileged(pte_t pte)
>> @@ -113,6 +113,16 @@ static inline pte_t pte_mkuser(pte_t pte)
>>   }
>>   #define pte_mkuser pte_mkuser
>> +
>> +static inline pte_t pte_mkexec(pte_t pte)
>> +{
>> +    if (pte_val(pte) & _PAGE_BAP_UR)
>> +        return __pte((pte_val(pte) & ~_PAGE_BAP_SX) | _PAGE_BAP_UX);
>> +    else
>> +        return __pte((pte_val(pte) & ~_PAGE_BAP_UX) | _PAGE_BAP_SX);
>> +}
>> +#define pte_mkexec pte_mkexec
>> +
>>   #endif /* __ASSEMBLY__ */
>>   #endif /* __KERNEL__ */
>> diff --git a/arch/powerpc/mm/nohash/tlb_low_64e.S 
>> b/arch/powerpc/mm/nohash/tlb_low_64e.S
>> index bf24451f3e71..9235e720e357 100644
>> --- a/arch/powerpc/mm/nohash/tlb_low_64e.S
>> +++ b/arch/powerpc/mm/nohash/tlb_low_64e.S
>> @@ -222,7 +222,7 @@ tlb_miss_kernel_bolted:
>>   tlb_miss_fault_bolted:
>>       /* We need to check if it was an instruction miss */
>> -    andi.    r10,r11,_PAGE_EXEC|_PAGE_BAP_SX
>> +    andi.    r10,r11,_PAGE_BAP_UX|_PAGE_BAP_SX
>>       bne    itlb_miss_fault_bolted
>>   dtlb_miss_fault_bolted:
>>       tlb_epilog_bolted
>> @@ -239,7 +239,7 @@ itlb_miss_fault_bolted:
>>       srdi    r15,r16,60        /* get region */
>>       bne-    itlb_miss_fault_bolted
>> -    li    r11,_PAGE_PRESENT|_PAGE_EXEC    /* Base perm */
>> +    li    r11,_PAGE_PRESENT|_PAGE_BAP_UX    /* Base perm */
>>       /* We do the user/kernel test for the PID here along with the RW 
>> test
>>        */
>> @@ -614,7 +614,7 @@ itlb_miss_fault_e6500:
>>       /* We do the user/kernel test for the PID here along with the RW 
>> test
>>        */
>> -    li    r11,_PAGE_PRESENT|_PAGE_EXEC    /* Base perm */
>> +    li    r11,_PAGE_PRESENT|_PAGE_BAP_UX    /* Base perm */
>>       oris    r11,r11,_PAGE_ACCESSED@h
>>       cmpldi    cr0,r15,0            /* Check for user region */
>> @@ -734,7 +734,7 @@ normal_tlb_miss_done:
>>   normal_tlb_miss_access_fault:
>>       /* We need to check if it was an instruction miss */
>> -    andi.    r10,r11,_PAGE_EXEC
>> +    andi.    r10,r11,_PAGE_BAP_UX
>>       bne    1f
>>       ld    r14,EX_TLB_DEAR(r12)
>>       ld    r15,EX_TLB_ESR(r12)
>>

  reply	other threads:[~2021-10-26  7:03 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-10-23 11:47 [PATCH v2 01/10] powerpc/nohash: Fix __ptep_set_access_flags() and ptep_set_wrprotect() Christophe Leroy
2021-10-23 11:47 ` [PATCH v2 02/10] powerpc/book3e: Fix set_memory_x() and set_memory_nx() Christophe Leroy
2021-10-25 21:53   ` Christophe Leroy
2021-10-26  7:03     ` Christophe Leroy [this message]
2021-10-23 11:47 ` [PATCH v2 03/10] powerpc/booke: Disable STRICT_KERNEL_RWX, DEBUG_PAGEALLOC and KFENCE Christophe Leroy
2021-10-23 11:47 ` [PATCH v2 04/10] powerpc/fsl_booke: Rename fsl_booke.c to fsl_book3e.c Christophe Leroy
2021-10-23 11:47 ` [PATCH v2 05/10] powerpc/fsl_booke: Take exec flag into account when setting TLBCAMs Christophe Leroy
2021-10-23 11:47 ` [PATCH v2 06/10] powerpc/fsl_booke: Enable reloading of TLBCAM without switching to AS1 Christophe Leroy
2021-10-23 11:47 ` [PATCH v2 07/10] powerpc/fsl_booke: Tell map_mem_in_cams() if init is done Christophe Leroy
2021-10-23 11:47 ` [PATCH v2 08/10] powerpc/fsl_booke: Allocate separate TLBCAMs for readonly memory Christophe Leroy
2021-10-23 11:47 ` [PATCH v2 09/10] powerpc/fsl_booke: Update of TLBCAMs after init Christophe Leroy
2021-10-23 11:47 ` [PATCH v2 10/10] powerpc/fsl_booke: Enable STRICT_KERNEL_RWX Christophe Leroy

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=40347194-fcc6-307b-0ddd-c8a221424dd4@csgroup.eu \
    --to=christophe.leroy@csgroup.eu \
    --cc=benh@kernel.crashing.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linuxppc-dev@lists.ozlabs.org \
    --cc=mpe@ellerman.id.au \
    --cc=paulus@samba.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).