| Message ID | 20180925093647.23723-1-mikey@neuling.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 96dc89d526ef77604376f06220e3d2931a0bfd58 |
| Headers | show |
| Series | powerpc/tm: Avoid possible userspace r1 corruption on reclaim | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | warning | next/apply_patch Patch failed to apply |
| snowpatch_ozlabs/apply_patch | fail | Failed to apply to any branch |
Michael Neuling <mikey@neuling.org> writes: > Current we store the userspace r1 to PACATMSCRATCH before finally > saving it to the thread struct. > > In theory an exception could be taken here (like a machine check or > SLB miss) that could write PACATMSCRATCH and hence corrupt the > userspace r1. The SLB fault currently doesn't touch PACATMSCRATCH, but > others do. > > We've never actually seen this happen but it's theoretically > possible. Either way, the code is fragile as it is. > > This patch saves r1 to the kernel stack (which can't fault) before we > turn MSR[RI] back on. PACATMSCRATCH is still used but only with > MSR[RI] off. We then copy r1 from the kernel stack to the thread > struct once we have MSR[RI] back on. > > Suggested-by: Breno Leitao <leitao@debian.org> > Signed-off-by: Michael Neuling <mikey@neuling.org> > --- > arch/powerpc/kernel/tm.S | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/arch/powerpc/kernel/tm.S b/arch/powerpc/kernel/tm.S > index 701b0f5b09..8207816a1e 100644 > --- a/arch/powerpc/kernel/tm.S > +++ b/arch/powerpc/kernel/tm.S > @@ -178,6 +178,12 @@ _GLOBAL(tm_reclaim) > > std r11, GPR11(r1) /* Temporary stash */ > > + /* Move r1 to kernel stack in case PACATMSCRATCH is used once > + * we turn on RI > + */ I see we still need to send you to Comment Formatting Re-Education Camp. I rewrote it a bit too, to hopefully be clearer? /* * Move the saved user r1 to the kernel stack in case PACATMSCRATCH is * clobbered by an exception once we turn on MSR_RI below. */ ld r11, PACATMSCRATCH(r13) std r11, GPR1(r1) cheers
On Tue, 2018-09-25 at 22:00 +1000, Michael Ellerman wrote: > Michael Neuling <mikey@neuling.org> writes: > > Current we store the userspace r1 to PACATMSCRATCH before finally > > saving it to the thread struct. > > > > In theory an exception could be taken here (like a machine check or > > SLB miss) that could write PACATMSCRATCH and hence corrupt the > > userspace r1. The SLB fault currently doesn't touch PACATMSCRATCH, but > > others do. > > > > We've never actually seen this happen but it's theoretically > > possible. Either way, the code is fragile as it is. > > > > This patch saves r1 to the kernel stack (which can't fault) before we > > turn MSR[RI] back on. PACATMSCRATCH is still used but only with > > MSR[RI] off. We then copy r1 from the kernel stack to the thread > > struct once we have MSR[RI] back on. > > > > Suggested-by: Breno Leitao <leitao@debian.org> > > Signed-off-by: Michael Neuling <mikey@neuling.org> > > --- > > arch/powerpc/kernel/tm.S | 8 +++++++- > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > diff --git a/arch/powerpc/kernel/tm.S b/arch/powerpc/kernel/tm.S > > index 701b0f5b09..8207816a1e 100644 > > --- a/arch/powerpc/kernel/tm.S > > +++ b/arch/powerpc/kernel/tm.S > > @@ -178,6 +178,12 @@ _GLOBAL(tm_reclaim) > > > > std r11, GPR11(r1) /* Temporary stash */ > > > > + /* Move r1 to kernel stack in case PACATMSCRATCH is used once > > + * we turn on RI > > + */ > > I see we still need to send you to Comment Formatting Re-Education Camp. The rest of that file has they style, so I'm just keeping with that. I can submit a patch later to fix them all up. > I rewrote it a bit too, to hopefully be clearer? > > /* > * Move the saved user r1 to the kernel stack in case PACATMSCRATCH is > * clobbered by an exception once we turn on MSR_RI below. > */ > ld r11, PACATMSCRATCH(r13) > std r11, GPR1(r1) Yeah, that's clearer... thanks. Mikey
On Tue, 2018-09-25 at 09:36:47 UTC, Michael Neuling wrote: > Current we store the userspace r1 to PACATMSCRATCH before finally > saving it to the thread struct. > > In theory an exception could be taken here (like a machine check or > SLB miss) that could write PACATMSCRATCH and hence corrupt the > userspace r1. The SLB fault currently doesn't touch PACATMSCRATCH, but > others do. > > We've never actually seen this happen but it's theoretically > possible. Either way, the code is fragile as it is. > > This patch saves r1 to the kernel stack (which can't fault) before we > turn MSR[RI] back on. PACATMSCRATCH is still used but only with > MSR[RI] off. We then copy r1 from the kernel stack to the thread > struct once we have MSR[RI] back on. > > Suggested-by: Breno Leitao <leitao@debian.org> > Signed-off-by: Michael Neuling <mikey@neuling.org> Applied to powerpc fixes, thanks. https://git.kernel.org/powerpc/c/96dc89d526ef77604376f06220e3d2 cheers
Michael Neuling <mikey@neuling.org> writes: > On Tue, 2018-09-25 at 22:00 +1000, Michael Ellerman wrote: >> Michael Neuling <mikey@neuling.org> writes: >> > Current we store the userspace r1 to PACATMSCRATCH before finally >> > saving it to the thread struct. >> > >> > In theory an exception could be taken here (like a machine check or >> > SLB miss) that could write PACATMSCRATCH and hence corrupt the >> > userspace r1. The SLB fault currently doesn't touch PACATMSCRATCH, but >> > others do. >> > >> > We've never actually seen this happen but it's theoretically >> > possible. Either way, the code is fragile as it is. >> > >> > This patch saves r1 to the kernel stack (which can't fault) before we >> > turn MSR[RI] back on. PACATMSCRATCH is still used but only with >> > MSR[RI] off. We then copy r1 from the kernel stack to the thread >> > struct once we have MSR[RI] back on. >> > >> > Suggested-by: Breno Leitao <leitao@debian.org> >> > Signed-off-by: Michael Neuling <mikey@neuling.org> >> > --- >> > arch/powerpc/kernel/tm.S | 8 +++++++- >> > 1 file changed, 7 insertions(+), 1 deletion(-) >> > >> > diff --git a/arch/powerpc/kernel/tm.S b/arch/powerpc/kernel/tm.S >> > index 701b0f5b09..8207816a1e 100644 >> > --- a/arch/powerpc/kernel/tm.S >> > +++ b/arch/powerpc/kernel/tm.S >> > @@ -178,6 +178,12 @@ _GLOBAL(tm_reclaim) >> > >> > std r11, GPR11(r1) /* Temporary stash */ >> > >> > + /* Move r1 to kernel stack in case PACATMSCRATCH is used once >> > + * we turn on RI >> > + */ >> >> I see we still need to send you to Comment Formatting Re-Education Camp. > > The rest of that file has they style, so I'm just keeping with that. I can > submit a patch later to fix them all up. Sure, but let's not add new ones. And yeah fixing them all up in another patch would be good. cheers
diff --git a/arch/powerpc/kernel/tm.S b/arch/powerpc/kernel/tm.S index 701b0f5b09..8207816a1e 100644 --- a/arch/powerpc/kernel/tm.S +++ b/arch/powerpc/kernel/tm.S @@ -178,6 +178,12 @@ _GLOBAL(tm_reclaim) std r11, GPR11(r1) /* Temporary stash */ + /* Move r1 to kernel stack in case PACATMSCRATCH is used once + * we turn on RI + */ + ld r11, PACATMSCRATCH(r13) + std r11, GPR1(r1) + /* Store r13 away so we can free up the scratch SPR for the * SLB fault handler (needed once we start access the * thread_struct) @@ -214,7 +220,7 @@ _GLOBAL(tm_reclaim) SAVE_GPR(8, r7) /* user r8 */ SAVE_GPR(9, r7) /* user r9 */ SAVE_GPR(10, r7) /* user r10 */ - ld r3, PACATMSCRATCH(r13) /* user r1 */ + ld r3, GPR1(r1) /* user r1 */ ld r4, GPR7(r1) /* user r7 */ ld r5, GPR11(r1) /* user r11 */ ld r6, GPR12(r1) /* user r12 */
Current we store the userspace r1 to PACATMSCRATCH before finally saving it to the thread struct. In theory an exception could be taken here (like a machine check or SLB miss) that could write PACATMSCRATCH and hence corrupt the userspace r1. The SLB fault currently doesn't touch PACATMSCRATCH, but others do. We've never actually seen this happen but it's theoretically possible. Either way, the code is fragile as it is. This patch saves r1 to the kernel stack (which can't fault) before we turn MSR[RI] back on. PACATMSCRATCH is still used but only with MSR[RI] off. We then copy r1 from the kernel stack to the thread struct once we have MSR[RI] back on. Suggested-by: Breno Leitao <leitao@debian.org> Signed-off-by: Michael Neuling <mikey@neuling.org> --- arch/powerpc/kernel/tm.S | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)