[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <5cea9974-9bef-712c-6e7b-b3c5fa7e0702@c-s.fr>
Date: Wed, 30 Oct 2019 09:01:33 +0100
From: Christophe Leroy <christophe.leroy@....fr>
To: Russell Currey <ruscur@...sell.cc>, linuxppc-dev@...ts.ozlabs.org
Cc: joel@....id.au, mpe@...erman.id.au, ajd@...ux.ibm.com, dja@...ens.net,
npiggin@...il.com, kernel-hardening@...ts.openwall.com
Subject: Re: [PATCH v5 1/5] powerpc/mm: Implement set_memory() routines
Le 30/10/2019 à 08:31, Russell Currey a écrit :
> The set_memory_{ro/rw/nx/x}() functions are required for STRICT_MODULE_RWX,
> and are generally useful primitives to have. This implementation is
> designed to be completely generic across powerpc's many MMUs.
>
> It's possible that this could be optimised to be faster for specific
> MMUs, but the focus is on having a generic and safe implementation for
> now.
>
> This implementation does not handle cases where the caller is attempting
> to change the mapping of the page it is executing from, or if another
> CPU is concurrently using the page being altered. These cases likely
> shouldn't happen, but a more complex implementation with MMU-specific code
> could safely handle them, so that is left as a TODO for now.
>
> Signed-off-by: Russell Currey <ruscur@...sell.cc>
> ---
> arch/powerpc/Kconfig | 1 +
> arch/powerpc/include/asm/set_memory.h | 32 +++++++++++
> arch/powerpc/mm/Makefile | 1 +
> arch/powerpc/mm/pageattr.c | 77 +++++++++++++++++++++++++++
> 4 files changed, 111 insertions(+)
> create mode 100644 arch/powerpc/include/asm/set_memory.h
> create mode 100644 arch/powerpc/mm/pageattr.c
>
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index 3e56c9c2f16e..8f7005f0d097 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -133,6 +133,7 @@ config PPC
> select ARCH_HAS_PTE_SPECIAL
> select ARCH_HAS_MEMBARRIER_CALLBACKS
> select ARCH_HAS_SCALED_CPUTIME if VIRT_CPU_ACCOUNTING_NATIVE && PPC_BOOK3S_64
> + select ARCH_HAS_SET_MEMORY
> select ARCH_HAS_STRICT_KERNEL_RWX if ((PPC_BOOK3S_64 || PPC32) && !RELOCATABLE && !HIBERNATION)
> select ARCH_HAS_TICK_BROADCAST if GENERIC_CLOCKEVENTS_BROADCAST
> select ARCH_HAS_UACCESS_FLUSHCACHE
> diff --git a/arch/powerpc/include/asm/set_memory.h b/arch/powerpc/include/asm/set_memory.h
> new file mode 100644
> index 000000000000..5230ddb2fefd
> --- /dev/null
> +++ b/arch/powerpc/include/asm/set_memory.h
> @@ -0,0 +1,32 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _ASM_POWERPC_SET_MEMORY_H
> +#define _ASM_POWERPC_SET_MEMORY_H
> +
> +#define SET_MEMORY_RO 1
> +#define SET_MEMORY_RW 2
> +#define SET_MEMORY_NX 3
> +#define SET_MEMORY_X 4
> +
> +int change_memory_attr(unsigned long addr, int numpages, int action);
> +
> +static inline int set_memory_ro(unsigned long addr, int numpages)
> +{
> + return change_memory_attr(addr, numpages, SET_MEMORY_RO);
> +}
> +
> +static inline int set_memory_rw(unsigned long addr, int numpages)
> +{
> + return change_memory_attr(addr, numpages, SET_MEMORY_RW);
> +}
> +
> +static inline int set_memory_nx(unsigned long addr, int numpages)
> +{
> + return change_memory_attr(addr, numpages, SET_MEMORY_NX);
> +}
> +
> +static inline int set_memory_x(unsigned long addr, int numpages)
> +{
> + return change_memory_attr(addr, numpages, SET_MEMORY_X);
> +}
> +
> +#endif
> diff --git a/arch/powerpc/mm/Makefile b/arch/powerpc/mm/Makefile
> index 5e147986400d..d0a0bcbc9289 100644
> --- a/arch/powerpc/mm/Makefile
> +++ b/arch/powerpc/mm/Makefile
> @@ -20,3 +20,4 @@ obj-$(CONFIG_HIGHMEM) += highmem.o
> obj-$(CONFIG_PPC_COPRO_BASE) += copro_fault.o
> obj-$(CONFIG_PPC_PTDUMP) += ptdump/
> obj-$(CONFIG_KASAN) += kasan/
> +obj-$(CONFIG_ARCH_HAS_SET_MEMORY) += pageattr.o
> diff --git a/arch/powerpc/mm/pageattr.c b/arch/powerpc/mm/pageattr.c
> new file mode 100644
> index 000000000000..aedd79173a44
> --- /dev/null
> +++ b/arch/powerpc/mm/pageattr.c
> @@ -0,0 +1,77 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +/*
> + * MMU-generic set_memory implementation for powerpc
> + *
> + * Copyright 2019, IBM Corporation.
> + */
> +
> +#include <linux/mm.h>
> +#include <linux/set_memory.h>
> +
> +#include <asm/mmu.h>
> +#include <asm/page.h>
> +#include <asm/pgtable.h>
> +
> +
> +/*
> + * Updates the attributes of a page in three steps:
> + *
> + * 1. invalidate the page table entry
> + * 2. flush the TLB
> + * 3. install the new entry with the updated attributes
> + *
> + * This is unsafe if the caller is attempting to change the mapping of the
> + * page it is executing from, or if another CPU is concurrently using the
> + * page being altered.
> + *
> + * TODO make the implementation resistant to this.
> + */
> +static int change_page_attr(pte_t *ptep, unsigned long addr, void *data)
I don't like too much the way you are making this function more complex
and less readable with local var, goto, etc ...
You could just keep the v4 version of change_page_attr(), rename it
__change_page_attr(), then add:
static int change_page_attr(pte_t *ptep, unsigned long addr, void *data)
{
int ret;
spin_lock(&init_mm.page_table_lock);
ret = __change_page_attr(ptep, addr, data);
spin_unlock(&init_mm.page_table_lock);
return ret;
}
Christophe
> +{
> + int action = *((int *)data);
> + pte_t pte_val;
> + int ret = 0;
> +
> + spin_lock(&init_mm.page_table_lock);
> +
> + // invalidate the PTE so it's safe to modify
> + pte_val = ptep_get_and_clear(&init_mm, addr, ptep);
> + flush_tlb_kernel_range(addr, addr + PAGE_SIZE);
> +
> + // modify the PTE bits as desired, then apply
> + switch (action) {
> + case SET_MEMORY_RO:
> + pte_val = pte_wrprotect(pte_val);
> + break;
> + case SET_MEMORY_RW:
> + pte_val = pte_mkwrite(pte_val);
> + break;
> + case SET_MEMORY_NX:
> + pte_val = pte_exprotect(pte_val);
> + break;
> + case SET_MEMORY_X:
> + pte_val = pte_mkexec(pte_val);
> + break;
> + default:
> + WARN_ON(true);
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + set_pte_at(&init_mm, addr, ptep, pte_val);
> +out:
> + spin_unlock(&init_mm.page_table_lock);
> + return ret;
> +}
> +
> +int change_memory_attr(unsigned long addr, int numpages, int action)
> +{
> + unsigned long start = ALIGN_DOWN(addr, PAGE_SIZE);
> + unsigned long size = numpages * PAGE_SIZE;
> +
> + if (!numpages)
> + return 0;
> +
> + return apply_to_page_range(&init_mm, start, size, change_page_attr, &action);
> +}
>
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
