Re: [patch V2 02/52] x86/fpu: Fix copy_xstate_to_kernel() gap handling

[Borislav Petkov <bp@suse.de>]

On Mon, Jun 14, 2021 at 05:44:10PM +0200, Thomas Gleixner wrote:
>   2) Keeping track of the last copied state in the target buffer and
>      explicitely zero it when there is a feature or alignment gap.

WARNING: 'explicitely' may be misspelled - perhaps 'explicitly'?
#93: 
     explicitely zero it when there is a feature or alignment gap.
     ^^^^^^^^^^^

> -static void fill_gap(struct membuf *to, unsigned *last, unsigned offset)
> +static void copy_feature(bool from_xstate, struct membuf *to, void *xstate,
> +			 void *init_xstate, unsigned int size)
>  {
> -	if (*last >= offset)
> -		return;
> -	membuf_write(to, (void *)&init_fpstate.xsave + *last, offset - *last);
> -	*last = offset;
> -}
> -
> -static void copy_part(struct membuf *to, unsigned *last, unsigned offset,
> -		      unsigned size, void *from)
> -{
> -	fill_gap(to, last, offset);
> -	membuf_write(to, from, size);
> -	*last = offset + size;
> +	membuf_write(to, from_xstate ? xstate : init_xstate, size);

I wonder - since we're making this code more robust anyway - whether
we should add an additional assertion here to check whether that
membuf.left is < size and warn.

It is cheap and having an additional check here would probably catch
some ptrace insanity or so, who knows...

> @@ -1120,41 +1110,68 @@ void copy_xstate_to_kernel(struct membuf
>  	header.xfeatures = xsave->header.xfeatures;
>  	header.xfeatures &= xfeatures_mask_user();
>  
> -	if (header.xfeatures & XFEATURE_MASK_FP)
> -		copy_part(&to, &last, 0, off_mxcsr, &xsave->i387);
> -	if (header.xfeatures & (XFEATURE_MASK_SSE | XFEATURE_MASK_YMM))
> -		copy_part(&to, &last, off_mxcsr,
> -			  MXCSR_AND_FLAGS_SIZE, &xsave->i387.mxcsr);
> -	if (header.xfeatures & XFEATURE_MASK_FP)
> -		copy_part(&to, &last, offsetof(struct fxregs_state, st_space),
> -			  128, &xsave->i387.st_space);
> -	if (header.xfeatures & XFEATURE_MASK_SSE)
> -		copy_part(&to, &last, xstate_offsets[XFEATURE_SSE],
> -			  256, &xsave->i387.xmm_space);
> -	/*
> -	 * Fill xsave->i387.sw_reserved value for ptrace frame:
> -	 */
> -	copy_part(&to, &last, offsetof(struct fxregs_state, sw_reserved),
> -		  48, xstate_fx_sw_bytes);
> -	/*
> -	 * Copy xregs_state->header:
> -	 */
> -	copy_part(&to, &last, offsetof(struct xregs_state, header),
> -		  sizeof(header), &header);
> +	/* Copy FP state up to MXCSR */
> +	copy_feature(header.xfeatures & XFEATURE_MASK_FP, &to, &xsave->i387,
> +		     &xinit->i387, off_mxcsr);
> +
> +	/* Copy MXCSR when SSE or YMM are set in the feature mask */
> +	copy_feature(header.xfeatures & (XFEATURE_MASK_SSE | XFEATURE_MASK_YMM),
> +		     &to, &xsave->i387.mxcsr, &xinit->i387.mxcsr,
> +		     MXCSR_AND_FLAGS_SIZE);

Yah, this copies a whopping 8 bytes:

        u32                     mxcsr;          /* MXCSR Register State */
        u32                     mxcsr_mask;     /* MXCSR Mask           */

I know, I know, it was like that before but dammit, that's obscure.

> +	/* Copy the remaining FP state */
> +	copy_feature(header.xfeatures & XFEATURE_MASK_FP,
> +		     &to, &xsave->i387.st_space, &xinit->i387.st_space,
> +		     sizeof(xsave->i387.st_space));
> +
> +	/* Copy the SSE state - shared with YMM */
> +	copy_feature(header.xfeatures & (XFEATURE_MASK_SSE | XFEATURE_MASK_YMM),
> +		     &to, &xsave->i387.xmm_space, &xinit->i387.xmm_space,
> +		     16 * 16);

Why not

	sizeof(xsave->i387.xmm_space)

?

> +
> +	/* Zero the padding area */
> +	membuf_zero(&to, sizeof(xsave->i387.padding));
> +
> +	/* Copy xsave->i387.sw_reserved */
> +	membuf_write(&to, xstate_fx_sw_bytes, sizeof(xsave->i387.sw_reserved));
> +
> +	/* Copy the user space relevant state of @xsave->header */
> +	membuf_write(&to, &header, sizeof(header));
> +
> +	zerofrom = offsetof(struct xregs_state, extended_state_area);
>  
>  	for (i = FIRST_EXTENDED_XFEATURE; i < XFEATURE_MAX; i++) {
>  		/*
> -		 * Copy only in-use xstates:
> +		 * The ptrace buffer is XSAVE format which is non-compacted.

... "is in XSAVE, non-compacted format."

> +		 * In non-compacted format disabled features still occupy
		     ^			  ^
		    the			  ,

> +		 * state space, but there is no state to copy from in the
> +		 * compacted init_fpstate. The gap tracking will zero this
> +		 * later.
> +		 */
> +		if (!(xfeatures_mask_user() & BIT_ULL(i)))
> +			continue;
> +
> +		/*
> +		 * If there was a feature or alignment gap, zero the space
> +		 * in the destination buffer.
>  		 */
> -		if ((header.xfeatures >> i) & 1) {
> -			void *src = __raw_xsave_addr(xsave, i);
> +		if (zerofrom < xstate_offsets[i])
> +			membuf_zero(&to, xstate_offsets[i] - zerofrom);
>  
> -			copy_part(&to, &last, xstate_offsets[i],
> -				  xstate_sizes[i], src);
> -		}
> +		copy_feature(header.xfeatures & BIT_ULL(i), &to,
> +			     __raw_xsave_addr(xsave, i),
> +			     __raw_xsave_addr(xinit, i),
> +			     xstate_sizes[i]);
>  
> +		/*
> +		 * Keep track of the last copied state in the non-compacted
> +		 * target buffer for gap zeroing.
> +		 */
> +		zerofrom = xstate_offsets[i] + xstate_sizes[i];
>  	}
> -	fill_gap(&to, &last, size);
> +
> +	if (to.left)
> +		membuf_zero(&to, to.left);
>  }

Yah, I can certainly follow what's going on here but, mapping that
compacted buffer to the uncompacted, XSAVE one is certainly making my
head spin.

Yah, FPU state handling is nasty.

-- 
Regards/Gruss,
    Boris.

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