Re: [patch V2 08/52] x86/fpu: Sanitize xstateregs_set()

[Borislav Petkov]

On Mon, Jun 14, 2021 at 05:44:16PM +0200, Thomas Gleixner wrote:
> @@ -108,10 +110,10 @@ int xstateregs_set(struct task_struct *t
>  		  const void *kbuf, const void __user *ubuf)
>  {
>  	struct fpu *fpu = &target->thread.fpu;
> -	struct xregs_state *xsave;
> +	struct xregs_state *tmpbuf = NULL;
>  	int ret;
>  
> -	if (!boot_cpu_has(X86_FEATURE_XSAVE))
> +	if (!static_cpu_has(X86_FEATURE_XSAVE))

cpu_feature_enabled() - we're going to use only that thing from now on
for simplicity.

> +	if (!kbuf) {
> +		tmpbuf = vmalloc(count);
> +		if (!tmpbuf)
> +			return -ENOMEM;
> +
> +		if (copy_from_user(tmpbuf, ubuf, count)) {
> +			ret = -EFAULT;
> +			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);
> +	fpu__prepare_write(fpu);

Yikes, why isn't this function called

fpu_invalidate_state(fpu)

?!

As in, what it does...

> @@ -1196,14 +1196,16 @@ int copy_kernel_to_xstate(struct xregs_s
>  	 */
>  	xsave->header.xfeatures |= hdr.xfeatures;
>  
> +	/* mxcsr reserved bits must be masked to zero for historical reasons. */

Wasn't that comment supposed to get some love?

https://lkml.kernel.org/r/87k0n0w3p8.ffs@xxxxxxxxxxxxxxxxxxxxxxx

> +	xsave->i387.mxcsr &= mxcsr_feature_mask;
> +

Thx.

-- 
Regards/Gruss,
    Boris.

SUSE Software Solutions Germany GmbH, GF: Felix Imendörffer, HRB 36809, AG Nürnberg