On Wed Feb 10, 2021 at 3:06 PM CST, Daniel Axtens wrote:
> "Christopher M. Riedl" <c...@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 <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.
>
> 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?
Ah yup you are correct. I confused myself with the comment in
swapcontext about the ctx_size. We call prepare_setup_sigcontext() with
current which will always have space. The ctx_size only matters on the
next call to setup_sigcontext() which ends up potentially copying the
VSX region to userspace (v_regs).
TL;DR - yes, I'll remove the ctx_has_vsx_region argument to
prepare_setup_sigcontext() with the next version. Thanks!
>
> >>
> >> 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
