Re: [PATCH v2 37/37] powerpc: Support execute-only on all powerpc

[Christophe Leroy]

Le 07/11/2023 à 07:15, Aneesh Kumar K V a écrit :
> On 11/6/23 6:53 PM, Christophe Leroy wrote:
>>
>>
>> Le 02/11/2023 à 06:39, Aneesh Kumar K.V a écrit :
>>> Christophe Leroy <christophe.leroy@xxxxxxxxxx> writes:
>>>
>>>> Introduce PAGE_EXECONLY_X macro which provides exec-only rights.
>>>> The _X may be seen as redundant with the EXECONLY but it helps
>>>> keep consistancy, all macros having the EXEC right have _X.
>>>>
>>>> And put it next to PAGE_NONE as PAGE_EXECONLY_X is
>>>> somehow PAGE_NONE + EXEC just like all other SOMETHING_X are
>>>> just SOMETHING + EXEC.
>>>>
>>>> On book3s/64 PAGE_EXECONLY becomes PAGE_READONLY_X.
>>>>
>>>> On book3s/64, as PAGE_EXECONLY is only valid for Radix add
>>>> VM_READ flag in vm_get_page_prot() for non-Radix.
>>>>
>>>> And update access_error() so that a non exec fault on a VM_EXEC only
>>>> mapping is always invalid, even when the underlying layer don't
>>>> always generate a fault for that.
>>>>
>>>> For 8xx, set PAGE_EXECONLY_X as _PAGE_NA | _PAGE_EXEC.
>>>> For others, only set it as just _PAGE_EXEC
>>>>
>>>> With that change, 8xx, e500 and 44x fully honor execute-only
>>>> protection.
>>>>
>>>> On 40x that is a partial implementation of execute-only. The
>>>> implementation won't be complete because once a TLB has been loaded
>>>> via the Instruction TLB miss handler, it will be possible to read
>>>> the page. But at least it can't be read unless it is executed first.
>>>>
>>>> On 603 MMU, TLB missed are handled by SW and there are separate
>>>> DTLB and ITLB. Execute-only is therefore now supported by not loading
>>>> DTLB when read access is not permitted.
>>>>
>>>> On hash (604) MMU it is more tricky because hash table is common to
>>>> load/store and execute. Nevertheless it is still possible to check
>>>> whether _PAGE_READ is set before loading hash table for a load/store
>>>> access. At least it can't be read unless it is executed first.
>>>>
>>>> Signed-off-by: Christophe Leroy <christophe.leroy@xxxxxxxxxx>
>>>> Cc: Russell Currey <ruscur@xxxxxxxxxx>
>>>> Cc: Kees Cook <keescook@xxxxxxxxxxxx>
>>>> ---
>>>>    arch/powerpc/include/asm/book3s/32/pgtable.h |  2 +-
>>>>    arch/powerpc/include/asm/book3s/64/pgtable.h |  4 +---
>>>>    arch/powerpc/include/asm/nohash/32/pte-8xx.h |  1 +
>>>>    arch/powerpc/include/asm/nohash/pgtable.h    |  2 +-
>>>>    arch/powerpc/include/asm/nohash/pte-e500.h   |  1 +
>>>>    arch/powerpc/include/asm/pgtable-masks.h     |  2 ++
>>>>    arch/powerpc/mm/book3s64/pgtable.c           | 10 ++++------
>>>>    arch/powerpc/mm/fault.c                      |  9 +++++----
>>>>    arch/powerpc/mm/pgtable.c                    |  4 ++--
>>>>    9 files changed, 18 insertions(+), 17 deletions(-)
>>>>
>>>> diff --git a/arch/powerpc/include/asm/book3s/32/pgtable.h b/arch/powerpc/include/asm/book3s/32/pgtable.h
>>>> index 244621c88510..52971ee30717 100644
>>>> --- a/arch/powerpc/include/asm/book3s/32/pgtable.h
>>>> +++ b/arch/powerpc/include/asm/book3s/32/pgtable.h
>>>> @@ -425,7 +425,7 @@ static inline bool pte_access_permitted(pte_t pte, bool write)
>>>>    {
>>>>    	/*
>>>>    	 * A read-only access is controlled by _PAGE_READ bit.
>>>> -	 * We have _PAGE_READ set for WRITE and EXECUTE
>>>> +	 * We have _PAGE_READ set for WRITE
>>>>    	 */
>>>>    	if (!pte_present(pte) || !pte_read(pte))
>>>>    		return false;
>>>>
>>>
>>> Should this now be updated to check for EXEC bit ?
>>
>> I don't think so based on what I see in arm64:
>> https://elixir.bootlin.com/linux/v6.6/source/arch/arm64/include/asm/pgtable.h#L146
>>
> 
> But then there can be a get_user_pages() (FOLL_GET) on an exec only pte right?
> if we are going to access the page data(FOLL_PIN), then yes exec only mapping should
> fail for that. But if we using it to do struct page manipulation we need pte_access_permitted
> to return true for exec only mapping?
> 

I don't know enough the details of GUP to understand what you mean. I 
understand you think there is a problem, do you mean ARM64 did it wrong ?

The core mm only has two call sites for pte_access_permitted() which are 
gup_pte_range() and gup_hugepte(). pte_access_permitted() is not 
documented in Documentation/mm/arch_pgtable_helpers.rst

So, what do those two callers expect ?

Christophe