* [PATCH] powerpc/64/tm: Don't let userspace set regs->trap via sigreturn
@ 2020-04-01 2:38 Michael Ellerman
2020-04-01 12:53 ` Michael Ellerman
0 siblings, 1 reply; 2+ messages in thread
From: Michael Ellerman @ 2020-04-01 2:38 UTC (permalink / raw)
To: linuxppc-dev; +Cc: mikey, npiggin, gromero
In restore_tm_sigcontexts() we take the trap value directly from the
user sigcontext with no checking:
err |= __get_user(regs->trap, &sc->gp_regs[PT_TRAP]);
This means we can be in the kernel with an arbitrary regs->trap value.
Although that's not immediately problematic, there is a risk we could
trigger one of the uses of CHECK_FULL_REGS():
#define CHECK_FULL_REGS(regs) BUG_ON(regs->trap & 1)
It can also cause us to unnecessarily save non-volatile GPRs again in
save_nvgprs(), which shouldn't be problematic but is still wrong.
It's also possible it could trick the syscall restart machinery, which
relies on regs->trap not being == 0xc00 (see 9a81c16b5275 ("powerpc:
fix double syscall restarts")), though I haven't been able to make
that happen.
Finally it doesn't match the behaviour of the non-TM case, in
restore_sigcontext() which zeroes regs->trap.
So change restore_tm_sigcontexts() to zero regs->trap.
This was discovered while testing Nick's upcoming rewrite of the
syscall entry path. In that series the call to save_nvgprs() prior to
signal handling (do_notify_resume()) is removed, which leaves the
low-bit of regs->trap uncleared which can then trigger the FULL_REGS()
WARNs in setup_tm_sigcontexts().
Fixes: 2b0a576d15e0 ("powerpc: Add new transactional memory state to the signal context")
Cc: stable@vger•kernel.org # v3.9+
Signed-off-by: Michael Ellerman <mpe@ellerman•id.au>
---
arch/powerpc/kernel/signal_64.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
index 84ed2e77ef9c..adfde59cf4ba 100644
--- a/arch/powerpc/kernel/signal_64.c
+++ b/arch/powerpc/kernel/signal_64.c
@@ -473,8 +473,10 @@ static long restore_tm_sigcontexts(struct task_struct *tsk,
err |= __get_user(tsk->thread.ckpt_regs.ccr,
&sc->gp_regs[PT_CCR]);
+ /* Don't allow userspace to set the trap value */
+ regs->trap = 0;
+
/* These regs are not checkpointed; they can go in 'regs'. */
- err |= __get_user(regs->trap, &sc->gp_regs[PT_TRAP]);
err |= __get_user(regs->dar, &sc->gp_regs[PT_DAR]);
err |= __get_user(regs->dsisr, &sc->gp_regs[PT_DSISR]);
err |= __get_user(regs->result, &sc->gp_regs[PT_RESULT]);
base-commit: 7074695ac6fb965d478f373b95bc5c636e9f21b0
--
2.25.1
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH] powerpc/64/tm: Don't let userspace set regs->trap via sigreturn
2020-04-01 2:38 [PATCH] powerpc/64/tm: Don't let userspace set regs->trap via sigreturn Michael Ellerman
@ 2020-04-01 12:53 ` Michael Ellerman
0 siblings, 0 replies; 2+ messages in thread
From: Michael Ellerman @ 2020-04-01 12:53 UTC (permalink / raw)
To: Michael Ellerman, linuxppc-dev; +Cc: mikey, npiggin, gromero
On Wed, 2020-04-01 at 02:38:36 UTC, Michael Ellerman wrote:
> In restore_tm_sigcontexts() we take the trap value directly from the
> user sigcontext with no checking:
>
> err |= __get_user(regs->trap, &sc->gp_regs[PT_TRAP]);
>
> This means we can be in the kernel with an arbitrary regs->trap value.
>
> Although that's not immediately problematic, there is a risk we could
> trigger one of the uses of CHECK_FULL_REGS():
>
> #define CHECK_FULL_REGS(regs) BUG_ON(regs->trap & 1)
>
> It can also cause us to unnecessarily save non-volatile GPRs again in
> save_nvgprs(), which shouldn't be problematic but is still wrong.
>
> It's also possible it could trick the syscall restart machinery, which
> relies on regs->trap not being == 0xc00 (see 9a81c16b5275 ("powerpc:
> fix double syscall restarts")), though I haven't been able to make
> that happen.
>
> Finally it doesn't match the behaviour of the non-TM case, in
> restore_sigcontext() which zeroes regs->trap.
>
> So change restore_tm_sigcontexts() to zero regs->trap.
>
> This was discovered while testing Nick's upcoming rewrite of the
> syscall entry path. In that series the call to save_nvgprs() prior to
> signal handling (do_notify_resume()) is removed, which leaves the
> low-bit of regs->trap uncleared which can then trigger the FULL_REGS()
> WARNs in setup_tm_sigcontexts().
>
> Fixes: 2b0a576d15e0 ("powerpc: Add new transactional memory state to the signal context")
> Cc: stable@vger•kernel.org # v3.9+
> Signed-off-by: Michael Ellerman <mpe@ellerman•id.au>
Applied to powerpc next.
https://git.kernel.org/powerpc/c/c7def7fbdeaa25feaa19caf4a27c5d10bd8789e4
cheers
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2020-04-01 13:57 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-04-01 2:38 [PATCH] powerpc/64/tm: Don't let userspace set regs->trap via sigreturn Michael Ellerman
2020-04-01 12:53 ` Michael Ellerman
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox