From: Daniel Axtens <dja@axtens•net>
To: "Christopher M. Riedl" <cmr@codefail•de>, linuxppc-dev@lists•ozlabs.org
Subject: Re: [PATCH v5 03/10] powerpc/signal64: Move non-inline functions out of setup_sigcontext()
Date: Thu, 11 Feb 2021 08:06:00 +1100 [thread overview]
Message-ID: <87a6sbeipz.fsf@dja-thinkpad.axtens.net> (raw)
In-Reply-To: <C95KM1QVX9G3.3HP1E2NXRPNSG@oc8246131445.ibm.com>
"Christopher M. Riedl" <cmr@codefail•de> writes:
> On Sun Feb 7, 2021 at 10:44 PM CST, Daniel Axtens wrote:
>> Hi Chris,
>>
>> These two paragraphs are a little confusing and they seem slightly
>> repetitive. But I get the general idea. Two specific comments below:
>
> Umm... yeah only one of those was supposed to be sent. I will reword
> this for the next spin and address the comment below about how it is
> not entirely clear that the inline functions are being moved out.
>
>>
>> > There are non-inline functions which get called in setup_sigcontext() to
>> > save register state to the thread struct. Move these functions into a
>> > separate prepare_setup_sigcontext() function so that
>> > setup_sigcontext() can be refactored later into an "unsafe" version
>> > which assumes an open uaccess window. Non-inline functions should be
>> > avoided when uaccess is open.
>>
>> Why do we want to avoid non-inline functions? We came up with:
>>
>> - we want KUAP protection for as much of the kernel as possible: each
>> extra bit of code run with the window open is another piece of attack
>> surface.
>>
>> - non-inline functions default to traceable, which means we could end
>> up ftracing while uaccess is enabled. That's a pretty big hole in the
>> defences that KUAP provides.
>>
>> I think we've also had problems with the window being opened or closed
>> unexpectedly by various bits of code? So the less code runs in uaccess
>> context the less likely that is to occur.
>
> That is my understanding as well.
>
>>
>> > The majority of setup_sigcontext() can be refactored to execute in an
>> > "unsafe" context (uaccess window is opened) except for some non-inline
>> > functions. Move these out into a separate prepare_setup_sigcontext()
>> > function which must be called first and before opening up a uaccess
>> > window. A follow-up commit converts setup_sigcontext() to be "unsafe".
>>
>> This was a bit confusing until we realise that you're moving the _calls_
>> to the non-inline functions out, not the non-inline functions
>> themselves.
>>
>> > Signed-off-by: Christopher M. Riedl <cmr@codefail•de>
>> > ---
>> > arch/powerpc/kernel/signal_64.c | 32 +++++++++++++++++++++-----------
>> > 1 file changed, 21 insertions(+), 11 deletions(-)
>> >
>> > diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
>> > index f9e4a1ac440f..b211a8ea4f6e 100644
>> > --- a/arch/powerpc/kernel/signal_64.c
>> > +++ b/arch/powerpc/kernel/signal_64.c
>> > @@ -79,6 +79,24 @@ static elf_vrreg_t __user *sigcontext_vmx_regs(struct sigcontext __user *sc)
>> > }
>> > #endif
>> >
>> > +static void prepare_setup_sigcontext(struct task_struct *tsk, int ctx_has_vsx_region)
>>
>> ctx_has_vsx_region should probably be a bool? Although setup_sigcontext
>> also has it as an int so I guess that's arguable, and maybe it's better
>> to stick with this for constency.
>
> I've been told not to introduce unrelated changes in my patches before
> so chose to keep this as an int for consistency.
Seems reasonable.
>
>>
>> > +{
>> > +#ifdef CONFIG_ALTIVEC
>> > + /* save altivec registers */
>> > + if (tsk->thread.used_vr)
>> > + flush_altivec_to_thread(tsk);
>> > + if (cpu_has_feature(CPU_FTR_ALTIVEC))
>> > + tsk->thread.vrsave = mfspr(SPRN_VRSAVE);
>> > +#endif /* CONFIG_ALTIVEC */
>> > +
>> > + flush_fp_to_thread(tsk);
>> > +
>> > +#ifdef CONFIG_VSX
>> > + if (tsk->thread.used_vsr && ctx_has_vsx_region)
>> > + flush_vsx_to_thread(tsk);
>> > +#endif /* CONFIG_VSX */
>>
>> Alternatively, given that this is the only use of ctx_has_vsx_region,
>> mpe suggested that perhaps we could drop it entirely and always
>> flush_vsx if used_vsr. The function is only ever called with either
>> `current` or wth ctx_has_vsx_region set to 1, so in either case I think
>> that's safe? I'm not sure if it would have performance implications.
>
> I think that could work as long as we can guarantee that the context
> passed to swapcontext will always be sufficiently sized if used_vsr,
> which I think *has* to be the case?
I think you're always guaranteed that you'll have a big enough one
in your kernel thread, which is what you end up writing to, iiuc?
>>
>> Should we move this and the altivec ifdef to IS_ENABLED(CONFIG_VSX) etc?
>> I'm not sure if that runs into any problems with things like 'used_vsr'
>> only being defined if CONFIG_VSX is set, but I thought I'd ask.
>
> That's why I didn't use IS_ENABLED(CONFIG_...) here - all of these
> field (used_vr, vrsave, used_vsr) declarations are guarded by #ifdefs :/
Dang. Oh well.
>
>>
>>
>> > +}
>> > +
>> > /*
>> > * Set up the sigcontext for the signal frame.
>> > */
>> > @@ -97,7 +115,6 @@ static long setup_sigcontext(struct sigcontext __user *sc,
>> > */
>> > #ifdef CONFIG_ALTIVEC
>> > elf_vrreg_t __user *v_regs = sigcontext_vmx_regs(sc);
>> > - unsigned long vrsave;
>> > #endif
>> > struct pt_regs *regs = tsk->thread.regs;
>> > unsigned long msr = regs->msr;
>> > @@ -112,7 +129,6 @@ static long setup_sigcontext(struct sigcontext __user *sc,
>> >
>> > /* save altivec registers */
>> > if (tsk->thread.used_vr) {
>> > - flush_altivec_to_thread(tsk);
>> > /* Copy 33 vec registers (vr0..31 and vscr) to the stack */
>> > err |= __copy_to_user(v_regs, &tsk->thread.vr_state,
>> > 33 * sizeof(vector128));
>> > @@ -124,17 +140,10 @@ static long setup_sigcontext(struct sigcontext __user *sc,
>> > /* We always copy to/from vrsave, it's 0 if we don't have or don't
>> > * use altivec.
>> > */
>> > - vrsave = 0;
>> > - if (cpu_has_feature(CPU_FTR_ALTIVEC)) {
>> > - vrsave = mfspr(SPRN_VRSAVE);
>> > - tsk->thread.vrsave = vrsave;
>> > - }
>> > -
>> > - err |= __put_user(vrsave, (u32 __user *)&v_regs[33]);
>> > + err |= __put_user(tsk->thread.vrsave, (u32 __user *)&v_regs[33]);
>>
>> Previously, if !cpu_has_feature(ALTIVEC), v_regs[33] had vrsave stored,
>> which was set to 0 explicitly. Now we store thread.vrsave instead of the
>> local vrsave. That should be safe - it is initalised to 0 elsewhere.
>>
>> So you don't have to do anything here, this is just letting you know
>> that we checked it and thought about it.
>
> Thanks! I thought about adding a comment/note here as I had to convince
> myself that thread.vrsave is indeed initialized to 0 before making this
> change as well. I will mention it in the word-smithed commit message for
> posterity.
>
>>
>> > #else /* CONFIG_ALTIVEC */
>> > err |= __put_user(0, &sc->v_regs);
>> > #endif /* CONFIG_ALTIVEC */
>> > - flush_fp_to_thread(tsk);
>> > /* copy fpr regs and fpscr */
>> > err |= copy_fpr_to_user(&sc->fp_regs, tsk);
>> >
>> > @@ -150,7 +159,6 @@ static long setup_sigcontext(struct sigcontext __user *sc,
>> > * VMX data.
>> > */
>> > if (tsk->thread.used_vsr && ctx_has_vsx_region) {
>> > - flush_vsx_to_thread(tsk);
>> > v_regs += ELF_NVRREG;
>> > err |= copy_vsx_to_user(v_regs, tsk);
>> > /* set MSR_VSX in the MSR value in the frame to
>> > @@ -655,6 +663,7 @@ SYSCALL_DEFINE3(swapcontext, struct ucontext __user *, old_ctx,
>> > ctx_has_vsx_region = 1;
>> >
>> > if (old_ctx != NULL) {
>> > + prepare_setup_sigcontext(current, ctx_has_vsx_region);
>> > if (!access_ok(old_ctx, ctx_size)
>> > || setup_sigcontext(&old_ctx->uc_mcontext, current, 0, NULL, 0,
>> > ctx_has_vsx_region)
>>
>> I had a think about whether there was a problem with bubbling
>> prepare_setup_sigcontext over the access_ok() test, but given that
>> prepare_setup_sigcontext(current ...) doesn't access any of old_ctx, I'm
>> satisfied that it's OK - no changes needed.
>
> Not sure I understand what you mean by 'bubbling over'?
Yeah sorry, overly flowery language there. I mean that the accesses that
prepare_setup_sigcontext does have moved up - like a bubble in fluid -
from after access_ok to before access_ok.
Kind regards,
Daniel
>>
>>
>> > @@ -842,6 +851,7 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
>> > #endif
>> > {
>> > err |= __put_user(0, &frame->uc.uc_link);
>> > + prepare_setup_sigcontext(tsk, 1);
>>
>> Why do we call with ctx_has_vsx_region = 1 here? It's not immediately
>> clear to me why this is correct, but mpe and Mikey seem pretty convinced
>> that it is.
>
> I think it's because we always have a "complete" sigcontext w/ the VSX
> save area here, unlike in swapcontext where we have to check. Also, the
> following unsafe_setup_sigcontext() is called with ctx_has_vsx_region=1
> so assumes that the VSX data was copied by prepare_setup_sigcontext().
>
>>
>> > err |= setup_sigcontext(&frame->uc.uc_mcontext, tsk, ksig->sig,
>> > NULL, (unsigned long)ksig->ka.sa.sa_handler,
>> > 1);
>>
>>
>> Finally, it's a bit hard to figure out where to put this, but we spent
>> some time making sure that the various things you moved into the
>> prepare_setup_sigcontext() function were called under the same
>> circumstances as they were before, and there were no concerns there.
>
> Thanks for reviewing and double checking my work :)
>
>>
>> Kind regards,
>> Daniel
next prev parent reply other threads:[~2021-02-10 21:07 UTC|newest]
Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-02-03 18:43 [PATCH v5 00/10] Improve signal performance on PPC64 with KUAP Christopher M. Riedl
2021-02-03 18:43 ` [PATCH v5 01/10] powerpc/uaccess: Add unsafe_copy_from_user Christopher M. Riedl
2021-02-05 4:47 ` Daniel Axtens
2021-02-03 18:43 ` [PATCH v5 02/10] powerpc/signal: Add unsafe_copy_{vsx, fpr}_from_user() Christopher M. Riedl
2021-02-05 5:17 ` Daniel Axtens
2021-02-03 18:43 ` [PATCH v5 03/10] powerpc/signal64: Move non-inline functions out of setup_sigcontext() Christopher M. Riedl
2021-02-08 4:44 ` Daniel Axtens
2021-02-10 4:37 ` Christopher M. Riedl
2021-02-10 21:06 ` Daniel Axtens [this message]
2021-02-10 23:51 ` Christopher M. Riedl
2021-02-03 18:43 ` [PATCH v5 04/10] powerpc: Reference param in MSR_TM_ACTIVE() macro Christopher M. Riedl
2021-02-12 4:52 ` Daniel Axtens
2021-02-03 18:43 ` [PATCH v5 05/10] powerpc/signal64: Remove TM ifdefery in middle of if/else block Christopher M. Riedl
2021-02-12 5:21 ` Daniel Axtens
2021-02-17 19:27 ` Christopher M. Riedl
2021-02-18 10:47 ` Michael Ellerman
2021-02-03 18:43 ` [PATCH v5 06/10] powerpc/signal64: Replace setup_sigcontext() w/ unsafe_setup_sigcontext() Christopher M. Riedl
2021-02-12 5:41 ` Daniel Axtens
2021-02-17 19:31 ` Christopher M. Riedl
2021-02-03 18:43 ` [PATCH v5 07/10] powerpc/signal64: Replace restore_sigcontext() w/ unsafe_restore_sigcontext() Christopher M. Riedl
2021-02-12 21:17 ` Daniel Axtens
2021-02-17 19:32 ` Christopher M. Riedl
2021-02-03 18:43 ` [PATCH v5 08/10] powerpc/signal64: Rewrite handle_rt_signal64() to minimise uaccess switches Christopher M. Riedl
2021-02-03 18:43 ` [PATCH v5 09/10] powerpc/signal64: Rewrite rt_sigreturn() " Christopher M. Riedl
2021-02-03 18:43 ` [PATCH v5 10/10] powerpc/signal64: Use __get_user() to copy sigset_t Christopher M. Riedl
2021-02-05 4:40 ` Christopher M. Riedl
2021-02-09 21:45 ` Christophe Leroy
2021-02-10 4:16 ` Christopher M. Riedl
2021-02-12 21:21 ` Daniel Axtens
2021-02-17 19:53 ` 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=87a6sbeipz.fsf@dja-thinkpad.axtens.net \
--to=dja@axtens$(echo .)net \
--cc=cmr@codefail$(echo .)de \
--cc=linuxppc-dev@lists$(echo .)ozlabs.org \
/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