Re: [patch V2 5/8] x86/fpu: Sanitize xstateregs_set()

[Borislav Petkov]

On Wed, Jun 02, 2021 at 06:01:13PM +0200, Thomas Gleixner wrote:
> xstateregs_set() operates on a stopped task and tries to copy the provided
> buffer into the tasks fpu.state.xsave buffer.

task's

> 
> Any error while copying or invalid state detected after copying results in
> wiping the target tasks FPU state completely including supervisor states.

Ditto.

> 
> That's just wrong. The caller supplied invalid data or has a problem with
> unmapped memory, so there is absolutely no justification to wreckage the
> target.
> 
> Fix this with the following modifications:
> 
>  1) If data has to be copied from userspace, allocate a buffer and copy from
>     user first.
> 
>  2) Use copy_kernel_to_xstate() unconditionally so that header checking
>     works correctly.
> 
>  3) Return on error without wreckaging the target state.
> 
> This prevents corrupting supervisor states and lets the caller deal with
> the problem it caused in the first place.
> 
> Make validate_user_xstate_header() static as this was the last user
> outside of xstate.c

Niice.


> @@ -120,32 +124,21 @@ int xstateregs_set(struct task_struct *t
>  	if (pos != 0 || count != fpu_user_xstate_size)
>  		return -EFAULT;
>  
> -	xsave = &fpu->state.xsave;
> -
> -	fpu__prepare_write(fpu);
> -
> -	if (using_compacted_format()) {
> -		if (kbuf)
> -			ret = copy_kernel_to_xstate(xsave, kbuf);
> -		else
> -			ret = copy_user_to_xstate(xsave, ubuf);
> -	} else {
> -		ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, xsave, 0, -1);
> -		if (!ret)
> -			ret = validate_user_xstate_header(&xsave->header);
> +	if (!kbuf) {
> +		xbuf = vmalloc(count);
> +		if (!xbuf)
> +			return -ENOMEM;

<---- newline here.

Btw, AFAICT and if I'm staring properly at the bowels of ptrace, that
ENOMEM would percolate up the chain when ptrace(2) is called.

The ptrace(2) manpage doesn't mention ENOMEM and I guess this is a small
ABI change. I say "small" because it is still a negative error and
properly written luserspace code should check it anyway...

Or am I being overly cautious here?

 ( Frankly, can you blame me, with that madness called xstate?! )

> +		ret = user_regset_copyin(&pos, &count, NULL, &ubuf, xbuf, 0, -1);
> +		if (ret)
> +			goto out;
>  	}
>  
> -	/*
> -	 * mxcsr reserved bits must be masked to zero for security reasons.
> -	 */
> -	xsave->i387.mxcsr &= mxcsr_feature_mask;
> -
> -	/*
> -	 * In case of failure, mark all states as init:
> -	 */
> -	if (ret)
> -		fpstate_init(&fpu->state);
> +	xsave = &fpu->state.xsave;
> +	fpu__prepare_write(fpu);
> +	ret = copy_kernel_to_xstate(xsave, kbuf ? kbuf : xbuf);

Uff, three buffers in a single function. How about we call "xbuf"
"tmp_buf" so that it is even more visible what it is and it differs more
from the other <letter>buf thingies?

>  /*
> - * Convert from a ptrace or sigreturn standard-format user-space buffer to
> - * kernel XSAVES format and copy to the target thread. This is called from
> - * xstateregs_set(), as well as potentially from the sigreturn() and
> - * rt_sigreturn() system calls.
> + * Convert from a sigreturn standard-format user-space buffer to kernel
> + * XSAVES format and copy to the target thread. This is called from the

Yeah, I guess we call that the "compacted" format. Just getting our
terminology simplified because this is one helluva mess, as we all know.

> + * sigreturn() and rt_sigreturn() system calls.
>   */
>  int copy_user_to_xstate(struct xregs_state *xsave, const void __user *ubuf)
>  {

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette