[PATCH v5 03/10] powerpc/signal64: Move non-inline functions out of setup_sigcontext()
Daniel Axtens
dja at axtens.net
Thu Feb 11 08:06:00 AEDT 2021
"Christopher M. Riedl" <cmr at 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 at 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
More information about the Linuxppc-dev
mailing list