[PATCH 3/3] powerpc/mm/ Add proper pte access check helper

[Christophe LEROY]

Le 04/12/2017 à 03:19, Aneesh Kumar K.V a écrit :
> pte_access_premitted get called in get_user_pages_fast path. If we have marked
> the pte PROT_NONE, we should not allow a read access on the address. With
> the current implementation we are not checking the READ and only check for
> WRITE. This is needed on archs like ppc64 that implement PROT_NONE using
> _PAGE_USER access instead of _PAGE_PRESENT. Also add pte_user check just to make sure
> we are not accessing kernel mapping.
> 
> Even though there is code duplication, keeping the low level pte accessors
> different for different platforms helps in code readability.
> 
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar at linux.vnet.ibm.com>
> ---
>   arch/powerpc/include/asm/book3s/32/pgtable.h | 23 +++++++++++++++++++++++
>   arch/powerpc/include/asm/nohash/pgtable.h    | 23 +++++++++++++++++++++++
>   2 files changed, 46 insertions(+)
> 
> diff --git a/arch/powerpc/include/asm/book3s/32/pgtable.h b/arch/powerpc/include/asm/book3s/32/pgtable.h
> index 016579ef16d3..30a155c0a6b0 100644
> --- a/arch/powerpc/include/asm/book3s/32/pgtable.h
> +++ b/arch/powerpc/include/asm/book3s/32/pgtable.h
> @@ -311,6 +311,29 @@ static inline int pte_present(pte_t pte)
>   	return pte_val(pte) & _PAGE_PRESENT;
>   }
>   
> +/*
> + * We only find page table entry in the last level
> + * Hence no need for other accessors
> + */
> +#define pte_access_permitted pte_access_permitted
> +static inline bool pte_access_permitted(pte_t pte, bool write)
> +{
> +	unsigned long pteval = pte_val(pte);
> +	/*
> +	 * A read-only access is controlled by _PAGE_USER bit.
> +	 * We have _PAGE_READ set for WRITE and EXECUTE
> +	 */
> +	unsigned long need_pte_bits = _PAGE_PRESENT | _PAGE_USER;
> +
> +	if (write)
> +		need_pte_bits |= _PAGE_WRITE;
> +
> +	if ((pteval & need_pte_bits) != need_pte_bits)
> +		return false;
> +
> +	return true;
> +}
> +
>   /* Conversion functions: convert a page and protection to a page entry,
>    * and a page entry and page directory to the page they refer to.
>    *
> diff --git a/arch/powerpc/include/asm/nohash/pgtable.h b/arch/powerpc/include/asm/nohash/pgtable.h
> index 5c68f4a59f75..fc4376c8d444 100644
> --- a/arch/powerpc/include/asm/nohash/pgtable.h
> +++ b/arch/powerpc/include/asm/nohash/pgtable.h
> @@ -45,6 +45,29 @@ static inline int pte_present(pte_t pte)
>   	return pte_val(pte) & _PAGE_PRESENT;
>   }
>   
> +/*
> + * We only find page table entry in the last level
> + * Hence no need for other accessors
> + */
> +#define pte_access_permitted pte_access_permitted
> +static inline bool pte_access_permitted(pte_t pte, bool write)
> +{
> +	unsigned long pteval = pte_val(pte);
> +	/*
> +	 * A read-only access is controlled by _PAGE_USER bit.
> +	 * We have _PAGE_READ set for WRITE and EXECUTE
> +	 */

Not fully right. asm/pte-common.h defines:

#define PAGE_NONE	__pgprot(_PAGE_BASE | _PAGE_NA)
#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_COPY	__pgprot(_PAGE_BASE | _PAGE_USER | _PAGE_RO)
#define PAGE_COPY_X	__pgprot(_PAGE_BASE | _PAGE_USER | _PAGE_RO | \
				 _PAGE_EXEC)
#define PAGE_READONLY	__pgprot(_PAGE_BASE | _PAGE_USER | _PAGE_RO)
#define PAGE_READONLY_X	__pgprot(_PAGE_BASE | _PAGE_USER | _PAGE_RO | \
				 _PAGE_EXEC)

On the 8xx, _PAGE_USER = 0

> +	unsigned long need_pte_bits = _PAGE_PRESENT | _PAGE_USER;
> +
> +	if (write)
> +		need_pte_bits |= _PAGE_WRITE;
> +
> +	if ((pteval & need_pte_bits) != need_pte_bits)
> +		return false;

This test is not fully correct:
- To check access(read) permission, you also have to check that _PAGE_NA 
is not set.
- To check write permission, you also have to check that neither 
_PAGE_NA nor _PAGE_RO are set.

On the 8xx, you have:
_PAGE_RW = _PAGE_WRITE = 0
_PAGE_NA = 0x0200
_PAGE_RO = 0x0600

Christophe

> +
> +	return true;
> +}
> +
>   /* Conversion functions: convert a page and protection to a page entry,
>    * and a page entry and page directory to the page they refer to.
>    *
>