| Message ID | 20191031164637.48901-14-samitolvanen@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3,01/17] arm64: mm: avoid x18 in idmap_kpti_install_ng_mappings | expand |
On Thu, Oct 31, 2019 at 9:47 AM <samitolvanen@google.com> wrote: > > Don't lose the current task's shadow stack when the CPU is suspended. > > Signed-off-by: Sami Tolvanen <samitolvanen@google.com> > --- > arch/arm64/include/asm/suspend.h | 2 +- > arch/arm64/mm/proc.S | 9 +++++++++ > 2 files changed, 10 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/include/asm/suspend.h b/arch/arm64/include/asm/suspend.h > index 8939c87c4dce..0cde2f473971 100644 > --- a/arch/arm64/include/asm/suspend.h > +++ b/arch/arm64/include/asm/suspend.h > @@ -2,7 +2,7 @@ > #ifndef __ASM_SUSPEND_H > #define __ASM_SUSPEND_H > > -#define NR_CTX_REGS 12 > +#define NR_CTX_REGS 13 > #define NR_CALLEE_SAVED_REGS 12 > > /* > diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S > index fdabf40a83c8..0e7c353c9dfd 100644 > --- a/arch/arm64/mm/proc.S > +++ b/arch/arm64/mm/proc.S > @@ -49,6 +49,8 @@ > * cpu_do_suspend - save CPU registers context > * > * x0: virtual address of context pointer > + * > + * This must be kept in sync with struct cpu_suspend_ctx in <asm/suspend.h>. > */ > ENTRY(cpu_do_suspend) > mrs x2, tpidr_el0 > @@ -73,6 +75,9 @@ alternative_endif > stp x8, x9, [x0, #48] > stp x10, x11, [x0, #64] > stp x12, x13, [x0, #80] > +#ifdef CONFIG_SHADOW_CALL_STACK > + str x18, [x0, #96] > +#endif > ret > ENDPROC(cpu_do_suspend) > > @@ -89,6 +94,10 @@ ENTRY(cpu_do_resume) > ldp x9, x10, [x0, #48] > ldp x11, x12, [x0, #64] > ldp x13, x14, [x0, #80] > +#ifdef CONFIG_SHADOW_CALL_STACK > + ldr x18, [x0, #96] > + str xzr, [x0, #96] How come we zero out x0+#96, but not for other offsets? Is this str necessary? > +#endif > msr tpidr_el0, x2 > msr tpidrro_el0, x3 > msr contextidr_el1, x4 > -- > 2.24.0.rc0.303.g954a862665-goog >
On Thu, Oct 31, 2019 at 10:18 AM Nick Desaulniers <ndesaulniers@google.com> wrote: > > +#ifdef CONFIG_SHADOW_CALL_STACK > > + ldr x18, [x0, #96] > > + str xzr, [x0, #96] > > How come we zero out x0+#96, but not for other offsets? Is this str necessary? It clears the shadow stack pointer from the sleep state buffer, which is not strictly speaking necessary, but leaves one fewer place to find it. Sami
On Thu, Oct 31, 2019 at 10:27 AM Sami Tolvanen <samitolvanen@google.com> wrote: > > On Thu, Oct 31, 2019 at 10:18 AM Nick Desaulniers > <ndesaulniers@google.com> wrote: > > > +#ifdef CONFIG_SHADOW_CALL_STACK > > > + ldr x18, [x0, #96] > > > + str xzr, [x0, #96] > > > > How come we zero out x0+#96, but not for other offsets? Is this str necessary? > > It clears the shadow stack pointer from the sleep state buffer, which > is not strictly speaking necessary, but leaves one fewer place to find > it. That sounds like a good idea. Consider adding comments or to the commit message so that the str doesn't get removed accidentally in the future. Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
On Thu, Oct 31, 2019 at 10:35 AM Nick Desaulniers <ndesaulniers@google.com> wrote: > > On Thu, Oct 31, 2019 at 10:27 AM Sami Tolvanen <samitolvanen@google.com> wrote: > > > > On Thu, Oct 31, 2019 at 10:18 AM Nick Desaulniers > > <ndesaulniers@google.com> wrote: > > > > +#ifdef CONFIG_SHADOW_CALL_STACK > > > > + ldr x18, [x0, #96] > > > > + str xzr, [x0, #96] > > > > > > How come we zero out x0+#96, but not for other offsets? Is this str necessary? > > > > It clears the shadow stack pointer from the sleep state buffer, which > > is not strictly speaking necessary, but leaves one fewer place to find > > it. > > That sounds like a good idea. Consider adding comments or to the > commit message so that the str doesn't get removed accidentally in the > future. Sure, I'll add a comment in the next version. Sami
On Thu, Oct 31, 2019 at 10:34:53AM -0700, Nick Desaulniers wrote: > On Thu, Oct 31, 2019 at 10:27 AM Sami Tolvanen <samitolvanen@google.com> wrote: > > > > On Thu, Oct 31, 2019 at 10:18 AM Nick Desaulniers > > <ndesaulniers@google.com> wrote: > > > > +#ifdef CONFIG_SHADOW_CALL_STACK > > > > + ldr x18, [x0, #96] > > > > + str xzr, [x0, #96] > > > > > > How come we zero out x0+#96, but not for other offsets? Is this str necessary? > > > > It clears the shadow stack pointer from the sleep state buffer, which > > is not strictly speaking necessary, but leaves one fewer place to find > > it. > > That sounds like a good idea. Consider adding comments or to the > commit message so that the str doesn't get removed accidentally in the > future. > Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> Yeah, with the comment added: Reviewed-by: Kees Cook <keescook@chromium.org> -Kees
diff --git a/arch/arm64/include/asm/suspend.h b/arch/arm64/include/asm/suspend.h index 8939c87c4dce..0cde2f473971 100644 --- a/arch/arm64/include/asm/suspend.h +++ b/arch/arm64/include/asm/suspend.h @@ -2,7 +2,7 @@ #ifndef __ASM_SUSPEND_H #define __ASM_SUSPEND_H -#define NR_CTX_REGS 12 +#define NR_CTX_REGS 13 #define NR_CALLEE_SAVED_REGS 12 /* diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S index fdabf40a83c8..0e7c353c9dfd 100644 --- a/arch/arm64/mm/proc.S +++ b/arch/arm64/mm/proc.S @@ -49,6 +49,8 @@ * cpu_do_suspend - save CPU registers context * * x0: virtual address of context pointer + * + * This must be kept in sync with struct cpu_suspend_ctx in <asm/suspend.h>. */ ENTRY(cpu_do_suspend) mrs x2, tpidr_el0 @@ -73,6 +75,9 @@ alternative_endif stp x8, x9, [x0, #48] stp x10, x11, [x0, #64] stp x12, x13, [x0, #80] +#ifdef CONFIG_SHADOW_CALL_STACK + str x18, [x0, #96] +#endif ret ENDPROC(cpu_do_suspend) @@ -89,6 +94,10 @@ ENTRY(cpu_do_resume) ldp x9, x10, [x0, #48] ldp x11, x12, [x0, #64] ldp x13, x14, [x0, #80] +#ifdef CONFIG_SHADOW_CALL_STACK + ldr x18, [x0, #96] + str xzr, [x0, #96] +#endif msr tpidr_el0, x2 msr tpidrro_el0, x3 msr contextidr_el1, x4
Don't lose the current task's shadow stack when the CPU is suspended. Signed-off-by: Sami Tolvanen <samitolvanen@google.com> --- arch/arm64/include/asm/suspend.h | 2 +- arch/arm64/mm/proc.S | 9 +++++++++ 2 files changed, 10 insertions(+), 1 deletion(-)