----- Original Message -----
> From: "Timothy Pearson" <tpear...@raptorengineering.com>
> To: "Jens Axboe" <ax...@kernel.dk>, "regressions"
> <regressi...@lists.linux.dev>, "Michael Ellerman"
> <m...@ellerman.id.au>, "npiggin" <npig...@gmail.com>, "christophe leroy"
> <christophe.le...@csgroup.eu>, "linuxppc-dev"
> <linuxppc-dev@lists.ozlabs.org>
> Sent: Saturday, November 18, 2023 5:45:03 PM
> Subject: [PATCH] powerpc: Don't clobber fr0/vs0 during fp|altivec register
> save
> During floating point and vector save to thread data fr0/vs0 are clobbered
> by the FPSCR/VSCR store routine. This leads to userspace register corruption
> and application data corruption / crash under the following rare condition:
>
> * A userspace thread is executing with VSX/FP mode enabled
> * The userspace thread is making active use of fr0 and/or vs0
> * An IPI is taken in kernel mode, forcing the userspace thread to reschedule
> * The userspace thread is interrupted by the IPI before accessing data it
> previously stored in fr0/vs0
> * The thread being switched in by the IPI has a pending signal
>
> If these exact criteria are met, then the following sequence happens:
>
> * The existing thread FP storage is still valid before the IPI, due to a
> prior call to save_fpu() or store_fp_state(). Note that the current
> fr0/vs0 registers have been clobbered, so the FP/VSX state in registers
> is now invalid pending a call to restore_fp()/restore_altivec().
> * IPI -- FP/VSX register state remains invalid
> * interrupt_exit_user_prepare_main() calls do_notify_resume(),
> due to the pending signal
> * do_notify_resume() eventually calls save_fpu() via giveup_fpu(), which
> merrily reads and saves the invalid FP/VSX state to thread local storage.
> * interrupt_exit_user_prepare_main() calls restore_math(), writing the invalid
> FP/VSX state back to registers.
> * Execution is released to userspace, and the application crashes or corrupts
> data.
>
> Without the pending signal, do_notify_resume() is never called, therefore the
> invalid register state does't matter as it is overwritten nearly immeediately
> by interrupt_exit_user_prepare_main() calling restore_math() before return
> to userspace.
>
> The combination of MariaDB and io_uring is especially good at triggering data
> corruption using the above sequence, see MariaDB bug MDEV-30728.
>
> Restore fr0/vs0 after FPSCR/VSCR store has completed for both the fp and
> altivec register save paths.
>
> Tested under QEMU in kvm mode, running on a Talos II workstation with dual
> POWER9 DD2.2 CPUs.
>
> Tested-by: Timothy Pearson <tpear...@raptorengineering.com>
> Signed-off-by: Timothy Pearson <tpear...@raptorengineering.com>
> ---
> arch/powerpc/kernel/fpu.S | 13 +++++++++++++
> arch/powerpc/kernel/vector.S | 4 ++++
> 2 files changed, 17 insertions(+)
>
> diff --git a/arch/powerpc/kernel/fpu.S b/arch/powerpc/kernel/fpu.S
> index 6a9acfb690c9..2f8f3f93cbb6 100644
> --- a/arch/powerpc/kernel/fpu.S
> +++ b/arch/powerpc/kernel/fpu.S
> @@ -23,6 +23,15 @@
> #include <asm/feature-fixups.h>
>
> #ifdef CONFIG_VSX
> +#define __REST_1FPVSR(n,c,base)
> \
> +BEGIN_FTR_SECTION \
> + b 2f; \
> +END_FTR_SECTION_IFSET(CPU_FTR_VSX); \
> + REST_FPR(n,base); \
> + b 3f; \
> +2: REST_VSR(n,c,base); \
> +3:
> +
> #define __REST_32FPVSRS(n,c,base) \
> BEGIN_FTR_SECTION \
> b 2f; \
> @@ -41,9 +50,11 @@ END_FTR_SECTION_IFSET(CPU_FTR_VSX);
> \
> 2: SAVE_32VSRS(n,c,base); \
> 3:
> #else
> +#define __REST_1FPVSR(n,b,base) REST_FPR(n, base)
> #define __REST_32FPVSRS(n,b,base) REST_32FPRS(n, base)
> #define __SAVE_32FPVSRS(n,b,base) SAVE_32FPRS(n, base)
> #endif
> +#define REST_1FPVSR(n,c,base) __REST_1FPVSR(n,__REG_##c,__REG_##base)
> #define REST_32FPVSRS(n,c,base) __REST_32FPVSRS(n,__REG_##c,__REG_##base)
> #define SAVE_32FPVSRS(n,c,base) __SAVE_32FPVSRS(n,__REG_##c,__REG_##base)
>
> @@ -67,6 +78,7 @@ _GLOBAL(store_fp_state)
> SAVE_32FPVSRS(0, R4, R3)
> mffs fr0
> stfd fr0,FPSTATE_FPSCR(r3)
> + REST_1FPVSR(0, R4, R3)
> blr
> EXPORT_SYMBOL(store_fp_state)
>
> @@ -138,4 +150,5 @@ _GLOBAL(save_fpu)
> 2: SAVE_32FPVSRS(0, R4, R6)
> mffs fr0
> stfd fr0,FPSTATE_FPSCR(r6)
> + REST_1FPVSR(0, R4, R6)
> blr
> diff --git a/arch/powerpc/kernel/vector.S b/arch/powerpc/kernel/vector.S
> index 4094e4c4c77a..8c63b05b421e 100644
> --- a/arch/powerpc/kernel/vector.S
> +++ b/arch/powerpc/kernel/vector.S
> @@ -33,6 +33,8 @@ _GLOBAL(store_vr_state)
> mfvscr v0
> li r4, VRSTATE_VSCR
> stvx v0, r4, r3
> + li r4, 0
> + lvx v0, r4, r3
> blr
> EXPORT_SYMBOL(store_vr_state)
>
> @@ -109,6 +111,8 @@ _GLOBAL(save_altivec)
> mfvscr v0
> li r4,VRSTATE_VSCR
> stvx v0,r4,r7
> + li r4,0
> + lvx v0,r4,r7
> blr
>
> #ifdef CONFIG_VSX
> --
> 2.39.2
Once this (or an equivalent) is merged upstream, I'll take the stable backport
task.
Thanks!
