On Wed, May 27, 2020 at 12:38 AM Al Viro <v...@zeniv.linux.org.uk> wrote:
>
> On Mon, May 25, 2020 at 12:45:35AM +0100, Al Viro wrote:
> > On Wed, May 13, 2020 at 04:33:49AM +0100, Al Viro wrote:
> >
> > > FWIW, what I'm going to do is
> > > * make all callers of copy_regset_to_user() pass 0 as pos
> > > (there are very few exceptions - one on arm64, three on sparc32
> > > and five on sparc64; I hadn't dealt with arm64 one yet, but all
> > > cases on sparc are handled)
> >
> > [snip]
> >
> > Any of that would be easy to backport, though. Several questions
> > regaring XSAVE and friends:
> >
> > * do we ever run on XSAVE/XSAVES-capable hardware with XFEATURE_FP
> > turned off?
> >
> > * is it possible for x86 to have gaps between the state components
> > area as reported by CPUID 0x0d? IOW, can area for feature 2
> > (XFEATURE_YMM) to start *not* at 0x200 and can area for N start
> > not right after the end of area for N-1 for some N > 2?
> >
> > I think I have an easy-to-backport solution, but I'm really confused
> > about XFEATURE_FP situation...
>
> Folks, could you test the following?
>
> copy_xstate_to_kernel(): don't leave parts of destination uninitialized
>
> copy the corresponding pieces of init_fpstate into the gaps instead.
>
> Signed-off-by: Al Viro <v...@zeniv.linux.org.uk>
Tested-by: Alexander Potapenko <gli...@google.com>
> ---
> diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
> index 32b153d38748..6a54e83d5589 100644
> --- a/arch/x86/kernel/fpu/xstate.c
> +++ b/arch/x86/kernel/fpu/xstate.c
> @@ -957,18 +957,31 @@ static inline bool xfeatures_mxcsr_quirk(u64 xfeatures)
> return true;
> }
>
> -/*
> - * This is similar to user_regset_copyout(), but will not add offset to
> - * the source data pointer or increment pos, count, kbuf, and ubuf.
> - */
> -static inline void
> -__copy_xstate_to_kernel(void *kbuf, const void *data,
> - unsigned int offset, unsigned int size, unsigned int
> size_total)
> +static void fill_gap(unsigned to, void **kbuf, unsigned *pos, unsigned
> *count)
> {
> - if (offset < size_total) {
> - unsigned int copy = min(size, size_total - offset);
> + if (*pos < to) {
> + unsigned size = to - *pos;
> +
> + if (size > *count)
> + size = *count;
> + memcpy(*kbuf, (void *)&init_fpstate.xsave + *pos, size);
> + *kbuf += size;
> + *pos += size;
> + *count -= size;
> + }
> +}
>
> - memcpy(kbuf + offset, data, copy);
> +static void copy_part(unsigned offset, unsigned size, void *from,
> + void **kbuf, unsigned *pos, unsigned *count)
> +{
> + fill_gap(offset, kbuf, pos, count);
> + if (size > *count)
> + size = *count;
> + if (size) {
> + memcpy(*kbuf, from, size);
> + *kbuf += size;
> + *pos += size;
> + *count -= size;
> }
> }
>
> @@ -981,8 +994,9 @@ __copy_xstate_to_kernel(void *kbuf, const void *data,
> */
> int copy_xstate_to_kernel(void *kbuf, struct xregs_state *xsave, unsigned
> int offset_start, unsigned int size_total)
> {
> - unsigned int offset, size;
> struct xstate_header header;
> + const unsigned off_mxcsr = offsetof(struct fxregs_state, mxcsr);
> + unsigned count = size_total;
> int i;
>
> /*
> @@ -998,46 +1012,42 @@ int copy_xstate_to_kernel(void *kbuf, struct
> xregs_state *xsave, unsigned int of
> header.xfeatures = xsave->header.xfeatures;
> header.xfeatures &= ~XFEATURE_MASK_SUPERVISOR;
>
> + if (header.xfeatures & XFEATURE_MASK_FP)
> + copy_part(0, off_mxcsr,
> + &xsave->i387, &kbuf, &offset_start, &count);
> + if (header.xfeatures & (XFEATURE_MASK_SSE | XFEATURE_MASK_YMM))
> + copy_part(off_mxcsr, MXCSR_AND_FLAGS_SIZE,
> + &xsave->i387.mxcsr, &kbuf, &offset_start, &count);
> + if (header.xfeatures & XFEATURE_MASK_FP)
> + copy_part(offsetof(struct fxregs_state, st_space), 128,
> + &xsave->i387.st_space, &kbuf, &offset_start,
> &count);
> + if (header.xfeatures & XFEATURE_MASK_SSE)
> + copy_part(xstate_offsets[XFEATURE_MASK_SSE], 256,
> + &xsave->i387.xmm_space, &kbuf, &offset_start,
> &count);
> + /*
> + * Fill xsave->i387.sw_reserved value for ptrace frame:
> + */
> + copy_part(offsetof(struct fxregs_state, sw_reserved), 48,
> + xstate_fx_sw_bytes, &kbuf, &offset_start, &count);
> /*
> * Copy xregs_state->header:
> */
> - offset = offsetof(struct xregs_state, header);
> - size = sizeof(header);
> -
> - __copy_xstate_to_kernel(kbuf, &header, offset, size, size_total);
> + copy_part(offsetof(struct xregs_state, header), sizeof(header),
> + &header, &kbuf, &offset_start, &count);
>
> - for (i = 0; i < XFEATURE_MAX; i++) {
> + for (i = FIRST_EXTENDED_XFEATURE; i < XFEATURE_MAX; i++) {
> /*
> * Copy only in-use xstates:
> */
> if ((header.xfeatures >> i) & 1) {
> void *src = __raw_xsave_addr(xsave, i);
>
> - offset = xstate_offsets[i];
> - size = xstate_sizes[i];
> -
> - /* The next component has to fit fully into the
> output buffer: */
> - if (offset + size > size_total)
> - break;
> -
> - __copy_xstate_to_kernel(kbuf, src, offset, size,
> size_total);
> + copy_part(xstate_offsets[i], xstate_sizes[i],
> + src, &kbuf, &offset_start, &count);
> }
>
> }
> -
> - if (xfeatures_mxcsr_quirk(header.xfeatures)) {
> - offset = offsetof(struct fxregs_state, mxcsr);
> - size = MXCSR_AND_FLAGS_SIZE;
> - __copy_xstate_to_kernel(kbuf, &xsave->i387.mxcsr, offset,
> size, size_total);
> - }
> -
> - /*
> - * Fill xsave->i387.sw_reserved value for ptrace frame:
> - */
> - offset = offsetof(struct fxregs_state, sw_reserved);
> - size = sizeof(xstate_fx_sw_bytes);
> -
> - __copy_xstate_to_kernel(kbuf, xstate_fx_sw_bytes, offset, size,
> size_total);
> + fill_gap(size_total, &kbuf, &offset_start, &count);
>
> return 0;
> }
--
Alexander Potapenko
Software Engineer
Google Germany GmbH
Erika-Mann-Straße, 33
80636 München
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
