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 <c...@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.
>
> > +{
> > +#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?
>
> 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 :/
>
>
> > +}
> > +
> > /*
> > * 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'?
>
>
> > @@ -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
