From: "Christopher M. Riedl" <cmr@linux.ibm.com>
To: "Christophe Leroy" <christophe.leroy@csgroup.eu>,
<linuxppc-dev@lists.ozlabs.org>
Cc: <keescook@chromium.org>, <peterz@infradead.org>, <x86@kernel.org>,
<npiggin@gmail.com>, <linux-hardening@vger.kernel.org>,
<tglx@linutronix.de>, <dja@axtens.net>
Subject: Re: [PATCH v5 0/8] Use per-CPU temporary mappings for patching on Radix MMU
Date: Wed, 11 Aug 2021 12:49:04 -0500 [thread overview]
Message-ID: <CDGVFMNUW01K.2JJR622LILQPD@oc8246131445.ibm.com> (raw)
In-Reply-To: <0c3cb140-23ad-0eb6-7df9-633aa51a097c@csgroup.eu>
On Thu Aug 5, 2021 at 4:03 AM CDT, Christophe Leroy wrote:
>
>
> Le 13/07/2021 à 07:31, Christopher M. Riedl a écrit :
> > When compiled with CONFIG_STRICT_KERNEL_RWX, the kernel must create
> > temporary mappings when patching itself. These mappings temporarily
> > override the strict RWX text protections to permit a write. Currently,
> > powerpc allocates a per-CPU VM area for patching. Patching occurs as
> > follows:
> >
> > 1. Map page in per-CPU VM area w/ PAGE_KERNEL protection
> > 2. Patch text
> > 3. Remove the temporary mapping
> >
> > While the VM area is per-CPU, the mapping is actually inserted into the
> > kernel page tables. Presumably, this could allow another CPU to access
> > the normally write-protected text - either malicously or accidentally -
> > via this same mapping if the address of the VM area is known. Ideally,
> > the mapping should be kept local to the CPU doing the patching [0].
> >
> > x86 introduced "temporary mm" structs which allow the creation of mappings
> > local to a particular CPU [1]. This series intends to bring the notion of a
> > temporary mm to powerpc's Book3s64 Radix MMU and harden it by using such a
> > mapping for patching a kernel with strict RWX permissions.
> >
> > The first four patches implement an LKDTM test "proof-of-concept" which
> > exploits the potential vulnerability (ie. the temporary mapping during patching
> > is exposed in the kernel page tables and accessible by other CPUs) using a
> > simple brute-force approach. This test is implemented for both powerpc and
> > x86_64. The test passes on powerpc Radix with this new series, fails on
> > upstream powerpc, passes on upstream x86_64, and fails on an older (ancient)
> > x86_64 tree without the x86_64 temporary mm patches. The remaining patches add
> > support for and use a temporary mm for code patching on powerpc with the Radix
> > MMU.
>
> I think four first patches (together with last one) are quite
> independent from the heart of the
> series itself which is patches 5, 6, 7. Maybe you should split that
> series it two series ? After all
> those selftests are nice to have but are not absolutely necessary, that
> would help getting forward I
> think.
>
Hmm you're probably right. The selftest at least proves there is a
potential attack which I think is necessary for any hardening related
series/patch. I'll split the series into separate powerpc temp mm and
LKDTM series for the next spin.
> >
> > Tested boot, ftrace, and repeated LKDTM "hijack":
> > - QEMU+KVM (host: POWER9 Blackbird): Radix MMU w/ KUAP
> > - QEMU+KVM (host: POWER9 Blackbird): Hash MMU
> >
> > Tested repeated LKDTM "hijack":
> > - QEMU+KVM (host: AMD desktop): x86_64 upstream
> > - QEMU+KVM (host: AMD desktop): x86_64 w/o percpu temp mm to
> > verify the LKDTM "hijack" test fails
> >
> > Tested boot and ftrace:
> > - QEMU+TCG: ppc44x (bamboo)
> > - QEMU+TCG: g5 (mac99)
> >
> > I also tested with various extra config options enabled as suggested in
> > section 12) in Documentation/process/submit-checklist.rst.
> >
> > v5: * Only support Book3s64 Radix MMU for now. There are some issues with
> > the previous implementation on the Hash MMU as pointed out by Nick
> > Piggin. Fixing these is not trivial so we only support the Radix MMU
> > for now. I tried using realmode (no data translation) to patch with
> > Hash to at least avoid exposing the patch mapping to other CPUs but
> > this doesn't reliably work either since we cannot access vmalloc'd
> > space in realmode.
>
> So you now accept to have two different mode depending on the platform ?
By necessity yes.
> As far as I remember I commented some time ago that non SMP didn't need
> that feature and you were
> reluctant to have two different implementations. What made you change
> your mind ? (just curious).
>
The book3s64 hash mmu support is a pain ;) Supporting both the temp-mm
and vmalloc implementations turned out to be relatively simple - I
initially thought this would be messier. For now we will support both;
however, in the future I'd still like to implement the percpu temp-mm
support for the Hash MMU as well. I suppose we could re-evaluate then if
we want/need both implementations (I know you're in favor of keeping the
vmalloc-based approach for performance reasons on non-SMP).
>
> > * Use percpu variables for the patching_mm and patching_addr. This
> > avoids the need for synchronization mechanisms entirely. Thanks to
> > Peter Zijlstra for pointing out text_mutex which unfortunately didn't
> > work out without larger code changes in powerpc. Also thanks to Daniel
> > Axtens for comments about using percpu variables for the *percpu* temp
> > mm things off list.
> >
> > v4: * It's time to revisit this series again since @jpn and @mpe fixed
> > our known STRICT_*_RWX bugs on powerpc/64s.
> > * Rebase on linuxppc/next:
> > commit ee1bc694fbaec ("powerpc/kvm: Fix build error when PPC_MEM_KEYS/PPC_PSERIES=n")
> > * Completely rework how map_patch() works on book3s64 Hash MMU
> > * Split the LKDTM x86_64 and powerpc bits into separate patches
> > * Annotate commit messages with changes from v3 instead of
> > listing them here completely out-of context...
> >
> > v3: * Rebase on linuxppc/next: commit 9123e3a74ec7 ("Linux 5.9-rc1")
> > * Move temporary mm implementation into code-patching.c where it
> > belongs
> > * Implement LKDTM hijacker test on x86_64 (on IBM time oof) Do
> > * not use address zero for the patching address in the
> > temporary mm (thanks @dja for pointing this out!)
> > * Wrap the LKDTM test w/ CONFIG_SMP as suggested by Christophe
> > Leroy
> > * Comments to clarify PTE pre-allocation and patching addr
> > selection
> >
> > v2: * Rebase on linuxppc/next:
> > commit 105fb38124a4 ("powerpc/8xx: Modify ptep_get()")
> > * Always dirty pte when mapping patch
> > * Use `ppc_inst_len` instead of `sizeof` on instructions
> > * Declare LKDTM patching addr accessor in header where it belongs
> >
> > v1: * Rebase on linuxppc/next (4336b9337824)
> > * Save and restore second hw watchpoint
> > * Use new ppc_inst_* functions for patching check and in LKDTM test
> >
> > rfc-v2: * Many fixes and improvements mostly based on extensive feedback
> > and testing by Christophe Leroy (thanks!).
> > * Make patching_mm and patching_addr static and move
> > '__ro_after_init' to after the variable name (more common in
> > other parts of the kernel)
> > * Use 'asm/debug.h' header instead of 'asm/hw_breakpoint.h' to
> > fix PPC64e compile
> > * Add comment explaining why we use BUG_ON() during the init
> > call to setup for patching later
> > * Move ptep into patch_mapping to avoid walking page tables a
> > second time when unmapping the temporary mapping
> > * Use KUAP under non-radix, also manually dirty the PTE for patch
> > mapping on non-BOOK3S_64 platforms
> > * Properly return any error from __patch_instruction
> > * Do not use 'memcmp' where a simple comparison is appropriate
> > * Simplify expression for patch address by removing pointer maths
> > * Add LKDTM test
> >
> > [0]: https://github.com/linuxppc/issues/issues/224
> > [1]: https://lore.kernel.org/kernel-hardening/20190426232303.28381-1-nadav.amit@gmail.com/
> >
> > Christopher M. Riedl (8):
> > powerpc: Add LKDTM accessor for patching addr
> > lkdtm/powerpc: Add test to hijack a patch mapping
> > x86_64: Add LKDTM accessor for patching addr
> > lkdtm/x86_64: Add test to hijack a patch mapping
> > powerpc/64s: Introduce temporary mm for Radix MMU
> > powerpc: Rework and improve STRICT_KERNEL_RWX patching
> > powerpc/64s: Initialize and use a temporary mm for patching on Radix
> > lkdtm/powerpc: Fix code patching hijack test
> >
> > arch/powerpc/include/asm/code-patching.h | 4 +
> > arch/powerpc/include/asm/debug.h | 1 +
> > arch/powerpc/kernel/process.c | 5 +
> > arch/powerpc/lib/code-patching.c | 240 ++++++++++++++++++++---
> > arch/x86/include/asm/text-patching.h | 4 +
> > arch/x86/kernel/alternative.c | 7 +
> > drivers/misc/lkdtm/core.c | 1 +
> > drivers/misc/lkdtm/lkdtm.h | 1 +
> > drivers/misc/lkdtm/perms.c | 143 ++++++++++++++
> > 9 files changed, 378 insertions(+), 28 deletions(-)
> >
prev parent reply other threads:[~2021-08-11 17:49 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-07-13 5:31 [PATCH v5 0/8] Use per-CPU temporary mappings for patching on Radix MMU Christopher M. Riedl
2021-07-13 5:31 ` [PATCH v5 1/8] powerpc: Add LKDTM accessor for patching addr Christopher M. Riedl
2021-07-13 5:31 ` [PATCH v5 2/8] lkdtm/powerpc: Add test to hijack a patch mapping Christopher M. Riedl
2021-08-05 9:13 ` Christophe Leroy
2021-08-11 17:57 ` Christopher M. Riedl
2021-08-11 18:07 ` Kees Cook
2021-07-13 5:31 ` [PATCH v5 3/8] x86_64: Add LKDTM accessor for patching addr Christopher M. Riedl
2021-07-13 5:31 ` [PATCH v5 4/8] lkdtm/x86_64: Add test to hijack a patch mapping Christopher M. Riedl
2021-08-05 9:09 ` Christophe Leroy
2021-08-11 17:53 ` Christopher M. Riedl
2021-07-13 5:31 ` [PATCH v5 5/8] powerpc/64s: Introduce temporary mm for Radix MMU Christopher M. Riedl
2021-08-05 9:27 ` Christophe Leroy
2021-08-11 18:02 ` Christopher M. Riedl
2021-07-13 5:31 ` [PATCH v5 6/8] powerpc: Rework and improve STRICT_KERNEL_RWX patching Christopher M. Riedl
2021-08-05 9:34 ` Christophe Leroy
2021-08-11 18:10 ` Christopher M. Riedl
2021-07-13 5:31 ` [PATCH v5 7/8] powerpc/64s: Initialize and use a temporary mm for patching on Radix Christopher M. Riedl
2021-08-05 9:48 ` Christophe Leroy
2021-08-11 18:28 ` Christopher M. Riedl
2021-07-13 5:31 ` [PATCH v5 8/8] lkdtm/powerpc: Fix code patching hijack test Christopher M. Riedl
2021-08-05 9:18 ` Christophe Leroy
2021-08-11 17:57 ` Christopher M. Riedl
2021-08-05 9:03 ` [PATCH v5 0/8] Use per-CPU temporary mappings for patching on Radix MMU Christophe Leroy
2021-08-11 17:49 ` Christopher M. Riedl [this message]
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=CDGVFMNUW01K.2JJR622LILQPD@oc8246131445.ibm.com \
--to=cmr@linux.ibm.com \
--cc=christophe.leroy@csgroup.eu \
--cc=dja@axtens.net \
--cc=keescook@chromium.org \
--cc=linux-hardening@vger.kernel.org \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=npiggin@gmail.com \
--cc=peterz@infradead.org \
--cc=tglx@linutronix.de \
--cc=x86@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
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;
as well as URLs for NNTP newsgroup(s).