On 2019-04-26 12:04:55 [-0700], Dave Hansen wrote:
> I'm looking at the code and having a bit of a hard time connecting it to
> what you're saying here.
>
> We are in copy_fpstate_to_sigframe(), right? Let's assume we are
> using_compacted_format(). If copy_fpregs_to_sigframe() fails to copy,
> we return immediately and never get to the save_xstate_epilog() code in
> question.
>
> So, to even get to save_xstate_epilog(), we had to do a *successful*
> copy_fpstate_to_sigframe() which, on XSAVE systems will use
> copy_xregs_to_user() which already uses plain XSAVE (not XSAVEOPT).
We are in:
copy_fpstate_to_sigframe()
|
copy_fpregs_to_sigframe() fails.
|
using_compacted_format()
|
copy_xstate_to_user()
|
__copy_xstate_to_user() the xstate_header
|
for (i = 0; i < XFEATURE_MAX; i++)
copy only XFEATURE_SSE + XFEATURE_YMM, other aren't set.
|
xfeatures_mxcsr_quirk() true, we copy
|
__copy_xstate_to_user() the xstate_fx_sw_bytes
Now. The FP part of the xsave has never been touched which means the
bytes are not initialized. The are is filled with random conten on user
stack. The saved xfeatures say only (XFEATURE_SSE | XFEATURE_YMM) so a
XRESTOR of that gives us exactly what we stored.
> save_xstate_epilog() goes and tries to set XFEATURE_MASK_FPSSE on this
> XSAVE (literally the XSAVE instruction) generated header.
Correct. But we never saved the "data" for XFEATURE_MASK_FP and, as noted
above, the content is random garbage on the stack.
Be setting XFEATURE_MASK_FPSSE we claim that suddenly that the FP part
of the XSAVE area is valid which is not the case.
> But, if we're dealing with header.xfeatures generated by an XSAVE with
> the RFBM=-1, I don't understand how header.xfeatures could ever *not*
> have XFEATURE_MASK_FPSSE set. The only way would be if we had gotten
> here after using FXSAVE, but I believe that's impossible since those
> systems have use_xsave()==0.
Look at the code path above. XSAVES stored the state in task's
&fpu->state.xsave using XSAVES and we copied the *saved* bits so it
looks like XSAVE. XFEATURE_MASK_FP was not set, so we skipped that area.
> IOW, I think the patch is right, but I'm not sure I totally agree with
> the reasoning.
As I described in the path description: XSAVE (the fastpath, the code
before I started touching it) would also not set XFEATURE_MASK_FP in
xfeatures *but* would set FP area to its ini-state. Therefore the
unconditional OR of XFEATURE_MASK_FPSSE does not hurt because the FP
area was initialized to its initstate. The following restore would load
a valid FP state.
Sebastian
