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

Borislav Petkov
2021-06-15T11:09:01Z

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