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

[Aneesh Kumar K V <aneesh.kumar@linux.ibm.com>]

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@csgroup.eu> 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@csgroup.eu>
>>> Cc: Russell Currey <ruscur@russell.cc>
>>> Cc: Kees Cook <keescook@chromium.org>
>>> ---
>>>   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?


-aneesh