On Wed, Jan 09, 2019 at 12:47:23PM +0100, Sebastian Andrzej Siewior wrote:
> This is a preparation for the removal of the ->initialized member in the
> fpu struct.
> __fpu__restore_sig() is deactivating the FPU via fpu__drop() and then
> setting manually ->initialized followed by fpu__restore(). The result is
> that it is possible to manipulate fpu->state and the state of registers
> won't be saved/restored on a context switch which would overwrite
> fpu->state.
>
> Don't access the fpu->state while the content is read from user space
> and examined / sanitized. Use a temporary kmalloc() buffer for the
> preparation of the FPU registers and once the state is considered okay,
> load it. Should something go wrong, return with an error and without
> altering the original FPU registers.
>
> The removal of "fpu__initialize()" is a nop because fpu->initialized is
> already set for the user task.
>
> Signed-off-by: Sebastian Andrzej Siewior <[email protected]>
> ---
> arch/x86/include/asm/fpu/signal.h | 2 +-
> arch/x86/kernel/fpu/regset.c | 5 ++--
> arch/x86/kernel/fpu/signal.c | 41 ++++++++++++-------------------
> 3 files changed, 19 insertions(+), 29 deletions(-)
...
> @@ -315,40 +313,33 @@ static int __fpu__restore_sig(void __user *buf, void
> __user *buf_fx, int size)
> * header. Validate and sanitize the copied state.
> */
> struct user_i387_ia32_struct env;
> + union fpregs_state *state;
> int err = 0;
> + void *tmp;
>
> - /*
> - * Drop the current fpu which clears fpu->initialized. This
> ensures
> - * that any context-switch during the copy of the new state,
> - * avoids the intermediate state from getting restored/saved.
> - * Thus avoiding the new restored state from getting corrupted.
> - * We will be ready to restore/save the state only after
> - * fpu->initialized is again set.
> - */
> - fpu__drop(fpu);
> + tmp = kzalloc(sizeof(*state) + fpu_kernel_xstate_size + 64,
> GFP_KERNEL);
> + if (!tmp)
> + return -ENOMEM;
> + state = PTR_ALIGN(tmp, 64);
>
> if (using_compacted_format()) {
> - err = copy_user_to_xstate(&fpu->state.xsave, buf_fx);
> + err = copy_user_to_xstate(&state->xsave, buf_fx);
> } else {
> - err = __copy_from_user(&fpu->state.xsave, buf_fx,
> state_size);
> + err = __copy_from_user(&state->xsave, buf_fx,
> state_size);
>
> if (!err && state_size > offsetof(struct xregs_state,
> header))
> - err =
> validate_xstate_header(&fpu->state.xsave.header);
> + err =
> validate_xstate_header(&state->xsave.header);
> }
>
> if (err || __copy_from_user(&env, buf, sizeof(env))) {
> - fpstate_init(&fpu->state);
> - trace_x86_fpu_init_state(fpu);
> err = -1;
> } else {
> - sanitize_restored_xstate(tsk, &env, xfeatures, fx_only);
> + sanitize_restored_xstate(state, &env,
> + xfeatures, fx_only);
Just let that one stick out - there are other lines in this file already
longer than 80.
Notwithstanding, I don't see anything wrong with this patch.
Acked-by: Borislav Petkov <[email protected]>
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
