From: Daniel Axtens <dja@axtens•net>
To: Russell Currey <ruscur@russell•cc>, linuxppc-dev@lists•ozlabs.org
Cc: ajd@linux•ibm.com, kernel-hardening@lists•openwall.com,
npiggin@gmail•com, joel@jms•id.au
Subject: Re: [PATCH v6 1/7] powerpc/mm: Implement set_memory() routines
Date: Wed, 11 Mar 2020 17:03:41 +1100 [thread overview]
Message-ID: <87imjbpgw2.fsf@dja-thinkpad.axtens.net> (raw)
In-Reply-To: <20200310010338.21205-2-ruscur@russell.cc>
Russell Currey <ruscur@russell•cc> writes:
> 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.
>
> These functions do nothing if STRICT_KERNEL_RWX is not enabled.
>
> Signed-off-by: Russell Currey <ruscur@russell•cc>
> Signed-off-by: Christophe Leroy <christophe.leroy@c-s•fr>
> ---
> v6: Merge patch 8/8 from v5, handling RWX not being enabled.
> Add note to change_page_attr() in case it's ever made non-static
> ---
> arch/powerpc/Kconfig | 1 +
> arch/powerpc/include/asm/set_memory.h | 32 +++++++++++
> arch/powerpc/mm/Makefile | 2 +-
> arch/powerpc/mm/pageattr.c | 79 +++++++++++++++++++++++++++
> 4 files changed, 113 insertions(+), 1 deletion(-)
> 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 497b7d0b2d7e..bd074246e34e 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -129,6 +129,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) && !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..64011ea444b4
> --- /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 0
> +#define SET_MEMORY_RW 1
> +#define SET_MEMORY_NX 2
> +#define SET_MEMORY_X 3
> +
> +int change_memory_attr(unsigned long addr, int numpages, long 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..a998fdac52f9 100644
> --- a/arch/powerpc/mm/Makefile
> +++ b/arch/powerpc/mm/Makefile
> @@ -5,7 +5,7 @@
>
> ccflags-$(CONFIG_PPC64) := $(NO_MINIMAL_TOC)
>
> -obj-y := fault.o mem.o pgtable.o mmap.o \
> +obj-y := fault.o mem.o pgtable.o mmap.o pageattr.o \
> init_$(BITS).o pgtable_$(BITS).o \
> pgtable-frag.o ioremap.o ioremap_$(BITS).o \
> init-common.o mmu_context.o drmem.o
> diff --git a/arch/powerpc/mm/pageattr.c b/arch/powerpc/mm/pageattr.c
> new file mode 100644
> index 000000000000..748fa56d9db0
> --- /dev/null
> +++ b/arch/powerpc/mm/pageattr.c
> @@ -0,0 +1,79 @@
> +// 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.
> + *
> + * NOTE: can be dangerous to call without STRICT_KERNEL_RWX
> + */
> +static int change_page_attr(pte_t *ptep, unsigned long addr, void *data)
> +{
> + long action = (long)data;
> + pte_t pte;
> +
> + spin_lock(&init_mm.page_table_lock);
> +
> + /* invalidate the PTE so it's safe to modify */
> + pte = 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 = pte_wrprotect(pte);
> + break;
> + case SET_MEMORY_RW:
> + pte = pte_mkwrite(pte);
> + break;
> + case SET_MEMORY_NX:
> + pte = pte_exprotect(pte);
> + break;
> + case SET_MEMORY_X:
> + pte = pte_mkexec(pte);
> + break;
> + default:
> + break;
Should this have a WARN_ON_ONCE to let you know you're doing something
that doesn't work? I know it's only ever called by things in this file,
but still... Anyway it's very minor and I'm not fussed either way.
> + }
> +
> + set_pte_at(&init_mm, addr, ptep, pte);
> + spin_unlock(&init_mm.page_table_lock);
Initially I thought: shouldn't you put the PTL lock/unlock in the outer
function? Then I remembered that apply_to_page_range can potentially
allocate new page table entries which would deadlock if you held the
lock.
Speaking of which - apply_to_page_range will create new pte entries if
you apply it over an address range that isn't filled in. That doesn't
really make sense here - should you use apply_to_existing_page_range
instead?
You _might_ be able to move the PTL lock if you use
apply_to_existing_page_range but I'm not completely sure if that's safe
or if the speed boost is worth it. You could check mm/memory.c if you
wanted.
> +
> + return 0;
> +}
> +
> +int change_memory_attr(unsigned long addr, int numpages, long action)
> +{
> + unsigned long start = ALIGN_DOWN(addr, PAGE_SIZE);
> + unsigned long sz = numpages * PAGE_SIZE;
> +
> + if (!IS_ENABLED(CONFIG_STRICT_KERNEL_RWX))
> + return 0;
> +
> + if (!numpages)
> + return 0;
What happens if numpages is negative? Doesn't the guard need to check
for that rather than just for zero?
With those caveats, and noting that I've been focused only on:
- lock/unlock paths
- integer arithmetic
- stuff about apply_page_range semantics
this patch is:
Reviewed-by: Daniel Axtens <dja@axtens•net>
Regards,
Daniel
> +
> + return apply_to_page_range(&init_mm, start, sz, change_page_attr, (void *)action);
> +}
> --
> 2.25.1
next prev parent reply other threads:[~2020-03-11 6:05 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-03-10 1:03 [PATCH v6 0/7] set_memory() routines and STRICT_MODULE_RWX Russell Currey
2020-03-10 1:03 ` [PATCH v6 1/7] powerpc/mm: Implement set_memory() routines Russell Currey
2020-03-11 6:03 ` Daniel Axtens [this message]
2020-03-31 1:49 ` Russell Currey
2020-03-10 1:03 ` [PATCH v6 2/7] powerpc/kprobes: Mark newly allocated probes as RO Russell Currey
2020-03-10 1:03 ` [PATCH v6 3/7] powerpc/mm/ptdump: debugfs handler for W+X checks at runtime Russell Currey
2020-03-10 1:03 ` [PATCH v6 4/7] powerpc: Set ARCH_HAS_STRICT_MODULE_RWX Russell Currey
2020-03-10 1:03 ` [PATCH v6 5/7] powerpc/configs: Enable STRICT_MODULE_RWX in skiroot_defconfig Russell Currey
2020-03-10 1:03 ` [PATCH v6 6/7] powerpc/mm: implement set_memory_attr() Russell Currey
2020-03-11 6:09 ` Daniel Axtens
2020-03-10 1:03 ` [PATCH v6 7/7] powerpc/32: use set_memory_attr() Russell Currey
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=87imjbpgw2.fsf@dja-thinkpad.axtens.net \
--to=dja@axtens$(echo .)net \
--cc=ajd@linux$(echo .)ibm.com \
--cc=joel@jms$(echo .)id.au \
--cc=kernel-hardening@lists$(echo .)openwall.com \
--cc=linuxppc-dev@lists$(echo .)ozlabs.org \
--cc=npiggin@gmail$(echo .)com \
--cc=ruscur@russell$(echo .)cc \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
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