| Message ID | 20210203184323.20792-4-cmr@codefail.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Improve signal performance on PPC64 with KUAP | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch powerpc/merge (44158b256b30415079588d0fcb1bccbdc2ccd009) |
| snowpatch_ozlabs/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 77 lines checked |
| snowpatch_ozlabs/needsstable | success | Patch has no Fixes tags |
Hi Chris, These two paragraphs are a little confusing and they seem slightly repetitive. But I get the general idea. Two specific comments below: > 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. > 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. > +{ > +#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. 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. > +} > + > /* > * 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. > #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. > @@ -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. > 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. Kind regards, Daniel
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. > > > +{ > > +#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
"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
On Wed Feb 10, 2021 at 3:06 PM CST, Daniel Axtens wrote: > "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? 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
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) +{ +#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 */ +} + /* * 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]); #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) @@ -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); err |= setup_sigcontext(&frame->uc.uc_mcontext, tsk, ksig->sig, NULL, (unsigned long)ksig->ka.sa.sa_handler, 1);
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. 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". Signed-off-by: Christopher M. Riedl <cmr@codefail.de> --- arch/powerpc/kernel/signal_64.c | 32 +++++++++++++++++++++----------- 1 file changed, 21 insertions(+), 11 deletions(-)