Re: [PATCH 01/22] x86/fpu: Remove fpu->initialized usage in __fpu__restore_sig()

[Borislav Petkov]

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 <bigeasy@xxxxxxxxxxxxx>
> ---
>  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 <bp@xxxxxxx>

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.