From: "Christopher M. Riedl" <cmr@codefail.de>
To: "Jann Horn" <jannh@google.com>
Cc: linuxppc-dev@lists.ozlabs.org,
Kernel Hardening <kernel-hardening@lists.openwall.com>
Subject: Re: [PATCH v3 4/6] powerpc: Introduce temporary mm
Date: Sun, 06 Sep 2020 19:15:02 -0500 [thread overview]
Message-ID: <C5GPCGQNRBQ2.8LRBQFQQ8QRK@geist> (raw)
In-Reply-To: <CAG48ez1W7FcDPAnqQ7TpSnKy--vaQm_f5prsZXRxcybzGg0tpg@mail.gmail.com>
On Thu Aug 27, 2020 at 11:15 AM CDT, Jann Horn wrote:
> On Thu, Aug 27, 2020 at 7:24 AM Christopher M. Riedl <cmr@codefail.de>
> wrote:
> > x86 supports the notion of a temporary mm which restricts access to
> > temporary PTEs to a single CPU. A temporary mm is useful for situations
> > where a CPU needs to perform sensitive operations (such as patching a
> > STRICT_KERNEL_RWX kernel) requiring temporary mappings without exposing
> > said mappings to other CPUs. A side benefit is that other CPU TLBs do
> > not need to be flushed when the temporary mm is torn down.
> >
> > Mappings in the temporary mm can be set in the userspace portion of the
> > address-space.
> [...]
> > diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
> [...]
> > @@ -44,6 +45,70 @@ int raw_patch_instruction(struct ppc_inst *addr, struct ppc_inst instr)
> > }
> >
> > #ifdef CONFIG_STRICT_KERNEL_RWX
> > +
> > +struct temp_mm {
> > + struct mm_struct *temp;
> > + struct mm_struct *prev;
> > + bool is_kernel_thread;
> > + struct arch_hw_breakpoint brk[HBP_NUM_MAX];
> > +};
> > +
> > +static inline void init_temp_mm(struct temp_mm *temp_mm, struct mm_struct *mm)
> > +{
> > + temp_mm->temp = mm;
> > + temp_mm->prev = NULL;
> > + temp_mm->is_kernel_thread = false;
> > + memset(&temp_mm->brk, 0, sizeof(temp_mm->brk));
> > +}
> > +
> > +static inline void use_temporary_mm(struct temp_mm *temp_mm)
> > +{
> > + lockdep_assert_irqs_disabled();
> > +
> > + temp_mm->is_kernel_thread = current->mm == NULL;
>
> (That's a somewhat misleading variable name - kernel threads can have
> a non-NULL ->mm, too.)
>
Oh I didn't know that, in that case yes this is not a good name. I am
considering some changes (based on your comments about current->mm
below) which would make this variable superfluous.
> > + if (temp_mm->is_kernel_thread)
> > + temp_mm->prev = current->active_mm;
> > + else
> > + temp_mm->prev = current->mm;
>
> Why the branch? Shouldn't current->active_mm work in both cases?
>
>
Yes you are correct.
> > + /*
> > + * Hash requires a non-NULL current->mm to allocate a userspace address
> > + * when handling a page fault. Does not appear to hurt in Radix either.
> > + */
> > + current->mm = temp_mm->temp;
>
> This looks dangerous to me. There are various places that attempt to
> find all userspace tasks that use a given mm by iterating through all
> tasks on the system and comparing each task's ->mm pointer to
> current's. Things like current_is_single_threaded() as part of various
> security checks, mm_update_next_owner(), zap_threads(), and so on. So
> if this is reachable from userspace task context (which I think it
> is?), I don't think we're allowed to switch out the ->mm pointer here.
>
>
Thanks for pointing this out! I took a step back and looked at this
again in more detail. The only reason for reassigning the ->mm pointer
is that when patching we need to hash the page and allocate an SLB
entry w/ the hash MMU. That codepath includes a check to ensure that
->mm is not NULL. Overwriting ->mm temporarily and restoring it is
pretty crappy in retrospect. I _think_ a better approach is to just call
the hashing and allocate SLB functions from `map_patch` directly - this
both removes the need to overwrite ->mm (since the functions take an mm
parameter) and it avoids taking two exceptions when doing the actual
patching.
This works fine on Power9 and a Power8 at least but needs some testing
on PPC32 before I can send a v4.
> > + switch_mm_irqs_off(NULL, temp_mm->temp, current);
>
> switch_mm_irqs_off() calls switch_mmu_context(), which in the nohash
> implementation increments next->context.active and decrements
> prev->context.active if prev is non-NULL, right? So this would
> increase temp_mm->temp->context.active...
>
> > + if (ppc_breakpoint_available()) {
> > + struct arch_hw_breakpoint null_brk = {0};
> > + int i = 0;
> > +
> > + for (; i < nr_wp_slots(); ++i) {
> > + __get_breakpoint(i, &temp_mm->brk[i]);
> > + if (temp_mm->brk[i].type != 0)
> > + __set_breakpoint(i, &null_brk);
> > + }
> > + }
> > +}
> > +
> > +static inline void unuse_temporary_mm(struct temp_mm *temp_mm)
> > +{
> > + lockdep_assert_irqs_disabled();
> > +
> > + if (temp_mm->is_kernel_thread)
> > + current->mm = NULL;
> > + else
> > + current->mm = temp_mm->prev;
> > + switch_mm_irqs_off(NULL, temp_mm->prev, current);
>
> ... whereas this would increase temp_mm->prev->context.active. As far
> as I can tell, that'll mean that both the original mm and the patching
> mm will have their .active counts permanently too high after
> use_temporary_mm()+unuse_temporary_mm()?
>
Yes you are correct. Hmm, I can't immediately recall why prev=NULL here,
and I can't find anything in the various powerpc
switch_mm_irqs_off/switch_mmu_context implementations that would break
by setting prev=actual previous mm here. I will fix this for v4. Thanks!
> > + if (ppc_breakpoint_available()) {
> > + int i = 0;
> > +
> > + for (; i < nr_wp_slots(); ++i)
> > + if (temp_mm->brk[i].type != 0)
> > + __set_breakpoint(i, &temp_mm->brk[i]);
> > + }
> > +}
next prev parent reply other threads:[~2020-09-07 1:15 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-08-27 5:26 [PATCH v3 0/6] Use per-CPU temporary mappings for patching Christopher M. Riedl
2020-08-27 5:26 ` [PATCH v3 1/6] powerpc: Add LKDTM accessor for patching addr Christopher M. Riedl
2020-08-27 5:26 ` [PATCH v3 2/6] x86: " Christopher M. Riedl
2020-08-27 5:26 ` [PATCH v3 3/6] Add LKDTM test to hijack a patch mapping (powerpc, x86_64) Christopher M. Riedl
2020-08-27 10:11 ` kernel test robot
2020-08-27 18:10 ` kernel test robot
2020-08-27 5:26 ` [PATCH v3 4/6] powerpc: Introduce temporary mm Christopher M. Riedl
2020-08-27 14:15 ` Jann Horn
2020-09-07 0:15 ` Christopher M. Riedl [this message]
2020-08-27 5:26 ` [PATCH v3 5/6] powerpc: Initialize a temporary mm for code patching Christopher M. Riedl
2020-08-27 5:26 ` [PATCH v3 6/6] powerpc: Use " Christopher M. Riedl
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=C5GPCGQNRBQ2.8LRBQFQQ8QRK@geist \
--to=cmr@codefail.de \
--cc=jannh@google.com \
--cc=kernel-hardening@lists.openwall.com \
--cc=linuxppc-dev@lists.ozlabs.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).