On Wed, Jan 09, 2019 at 12:47:42PM +0100, Sebastian Andrzej Siewior wrote:
> The !32bit+fxsr case loads the new state from user memory. In case we
^^^^^^^^^^^
Let's "decrypt" that: "The 64-bit case where fast FXSAVE/FXRSTOR are used... "
But looking at the patch, it is not only about the fxsr but also the
use_xsave() case. So pls write out exactly what you mean here.
Ditto for the patch title.
> restore the FPU state on return to userland we can't do this. It would
> be required to disable preemption in order to avoid a context switch
> which would set TIF_NEED_FPU_LOAD. If this happens before the "restore"
> operation then the loaded registers would become volatile.
>
> Disabling preemption while accessing user memory requires to disable the
> pagefault handler. An error during XRSTOR would then mean that either a
> page fault occured (and we have to retry with enabled page fault
> handler) or a #GP occured because the xstate is bogus (after all the
> sig-handler can modify it).
>
> In order to avoid that mess, copy the FPU state from userland, validate
> it and then load it. The copy_users_…() helper are basically the old
> helper except that they operate on kernel memory and the fault handler
> just sets the error value and the caller handles it.
>
> Signed-off-by: Sebastian Andrzej Siewior <[email protected]>
> ---
> arch/x86/include/asm/fpu/internal.h | 32 ++++++++++-----
> arch/x86/kernel/fpu/signal.c | 62 +++++++++++++++++++++++------
> 2 files changed, 71 insertions(+), 23 deletions(-)
>
> diff --git a/arch/x86/include/asm/fpu/internal.h
> b/arch/x86/include/asm/fpu/internal.h
> index 16ea30235b025..672e51bc0e9b5 100644
> --- a/arch/x86/include/asm/fpu/internal.h
> +++ b/arch/x86/include/asm/fpu/internal.h
> @@ -120,6 +120,21 @@ extern void fpstate_sanitize_xstate(struct fpu *fpu);
> err; \
> })
>
> +#define kernel_insn_norestore(insn, output, input...)
> \
> +({ \
> + int err; \
> + asm volatile("1:" #insn "\n\t" \
> + "2:\n" \
> + ".section .fixup,\"ax\"\n" \
> + "3: movl $-1,%[err]\n" \
> + " jmp 2b\n" \
> + ".previous\n" \
> + _ASM_EXTABLE(1b, 3b) \
> + : [err] "=r" (err), output \
> + : "0"(0), input); \
> + err; \
> +})
user_insn above looks unused - just repurpose it.
> +
> #define kernel_insn(insn, output, input...) \
> asm volatile("1:" #insn "\n\t" \
> "2:\n" \
> @@ -140,15 +155,15 @@ static inline void copy_kernel_to_fxregs(struct
> fxregs_state *fx)
> }
> }
>
> -static inline int copy_user_to_fxregs(struct fxregs_state __user *fx)
> +static inline int copy_users_to_fxregs(struct fxregs_state *fx)
Why "users" ?
> {
> if (IS_ENABLED(CONFIG_X86_32))
> - return user_insn(fxrstor %[fx], "=m" (*fx), [fx] "m" (*fx));
> + return kernel_insn_norestore(fxrstor %[fx], "=m" (*fx), [fx]
> "m" (*fx));
> else if (IS_ENABLED(CONFIG_AS_FXSAVEQ))
> - return user_insn(fxrstorq %[fx], "=m" (*fx), [fx] "m" (*fx));
> + return kernel_insn_norestore(fxrstorq %[fx], "=m" (*fx), [fx]
> "m" (*fx));
>
> /* See comment in copy_fxregs_to_kernel() below. */
> - return user_insn(rex64/fxrstor (%[fx]), "=m" (*fx), [fx] "R" (fx),
> + return kernel_insn_norestore(rex64/fxrstor (%[fx]), "=m" (*fx), [fx]
> "R" (fx),
> "m" (*fx));
> }
>
> @@ -157,9 +172,9 @@ static inline void copy_kernel_to_fregs(struct
> fregs_state *fx)
> kernel_insn(frstor %[fx], "=m" (*fx), [fx] "m" (*fx));
> }
>
> -static inline int copy_user_to_fregs(struct fregs_state __user *fx)
> +static inline int copy_users_to_fregs(struct fregs_state *fx)
> {
> - return user_insn(frstor %[fx], "=m" (*fx), [fx] "m" (*fx));
> + return kernel_insn_norestore(frstor %[fx], "=m" (*fx), [fx] "m" (*fx));
> }
>
> static inline void copy_fxregs_to_kernel(struct fpu *fpu)
> @@ -339,16 +354,13 @@ static inline void copy_kernel_to_xregs(struct
> xregs_state *xstate, u64 mask)
> /*
> * Restore xstate from user space xsave area.
> */
> -static inline int copy_user_to_xregs(struct xregs_state __user *buf, u64
> mask)
> +static inline int copy_users_to_xregs(struct xregs_state *xstate, u64 mask)
> {
> - struct xregs_state *xstate = ((__force struct xregs_state *)buf);
> u32 lmask = mask;
> u32 hmask = mask >> 32;
> int err;
>
> - stac();
> XSTATE_OP(XRSTOR, xstate, lmask, hmask, err);
> - clac();
>
> return err;
> }
> diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
> index 970091fb011e9..4ed5c400cac58 100644
> --- a/arch/x86/kernel/fpu/signal.c
> +++ b/arch/x86/kernel/fpu/signal.c
> @@ -217,7 +217,8 @@ sanitize_restored_xstate(union fpregs_state *state,
> */
> xsave->i387.mxcsr &= mxcsr_feature_mask;
>
> - convert_to_fxsr(&state->fxsave, ia32_env);
> + if (ia32_env)
> + convert_to_fxsr(&state->fxsave, ia32_env);
> }
> }
>
> @@ -299,28 +300,63 @@ static int __fpu__restore_sig(void __user *buf, void
> __user *buf_fx, int size)
> kfree(tmp);
> return err;
> } else {
> + union fpregs_state *state;
> + void *tmp;
> int ret;
>
> + tmp = kzalloc(sizeof(*state) + fpu_kernel_xstate_size + 64,
> GFP_KERNEL);
> + if (!tmp)
> + return -ENOMEM;
<---- newline here.
> + state = PTR_ALIGN(tmp, 64);
> +
> /*
> * For 64-bit frames and 32-bit fsave frames, restore the user
> * state to the registers directly (with exceptions handled).
> */
> - if (use_xsave()) {
> - if ((unsigned long)buf_fx % 64 || fx_only) {
> + if ((unsigned long)buf_fx % 64)
> + fx_only = 1;
> +
> + if (use_xsave() && !fx_only) {
> + u64 init_bv = xfeatures_mask & ~xfeatures;
Define that init_bv in the function prologue above and then you don't
need to define it again here and below in a narrower scope.
> +
> + if (using_compacted_format()) {
> + ret = copy_user_to_xstate(&state->xsave,
> buf_fx);
> + } else {
> + ret = __copy_from_user(&state->xsave, buf_fx,
> state_size);
> +
> + if (!ret && state_size > offsetof(struct
> xregs_state, header))
> + ret =
> validate_xstate_header(&state->xsave.header);
> + }
> + if (ret)
> + goto err_out;
<---- newline here.
> + sanitize_restored_xstate(state, NULL, xfeatures,
> + fx_only);
Let that stick out.
And then do that here:
init_bv = xfeatures_mask & ~xfeatures;> +
> + if (unlikely(init_bv))
> + copy_kernel_to_xregs(&init_fpstate.xsave,
> init_bv);
and add a newline here so that this code above belongs together visually.
> + ret = copy_users_to_xregs(&state->xsave, xfeatures);
> +
...
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
