| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <YMiKC09GUaG5vZta@zn.tnic>
Date: Tue, 15 Jun 2021 13:07:55 +0200
From: Borislav Petkov <bp@...e.de>
To: Thomas Gleixner <tglx@...utronix.de>
Cc: LKML <linux-kernel@...r.kernel.org>,
Andy Lutomirski <luto@...nel.org>,
Dave Hansen <dave.hansen@...ux.intel.com>,
Fenghua Yu <fenghua.yu@...el.com>,
Tony Luck <tony.luck@...el.com>,
Yu-cheng Yu <yu-cheng.yu@...el.com>,
Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
Peter Zijlstra <peterz@...radead.org>,
Kan Liang <kan.liang@...ux.intel.com>
Subject: Re: [patch V2 02/52] x86/fpu: Fix copy_xstate_to_kernel() gap
handling
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
Powered by blists - more mailing lists