On Mon, May 31, 2021, at 11:56 AM, Thomas Gleixner wrote:
> On Mon, May 31 2021 at 12:01, Thomas Gleixner wrote:
>
> > On Sat, May 29 2021 at 22:12, Andy Lutomirski wrote:
> >> /*
> >> - * Clear the FPU state back to init state.
> >> - *
> >> - * Called by sys_execve(), by the signal handler code and by various
> >> - * error paths.
> >> + * Reset current's user FPU states to the init states. The caller promises
> >> + * that current's supervisor states (in memory or CPU regs as appropriate)
> >> + * as well as the XSAVE header in memory are intact.
> >> */
> >> -static void fpu__clear(struct fpu *fpu, bool user_only)
> >> +void fpu__clear_user_states(struct fpu *fpu)
> >> {
> >> WARN_ON_FPU(fpu != ¤t->thread.fpu);
> >>
> >> if (!static_cpu_has(X86_FEATURE_FPU)) {
> >
> > This can only be safely called if XSAVES is available. So this check is
> > bogus as it actually should check for !XSAVES. And if at all it should
> > be:
> >
> > if (WARN_ON_ONCE(!XSAVES))
> > ....
> >
> > This is exactly the stuff which causes subtle problems down the road.
> >
> > I have no idea why you are insisting on having this conditional at the
> > call site. It's just an invitation for trouble because someone finds
> > this function and calls it unconditionally. And he will miss the
> > 'promise' part in the comment as I did.
>
> And of course there is:
>
> __fpu__restore_sig()
>
> if (!buf) {
> fpu__clear_user_states(fpu);
> return 0;
> }
>
> and
>
> handle_signal()
>
> if (!failed)
> fpu__clear_user_states(fpu);
This looks okay.
Really there are two callers of fpu__clear_all() that are special:
execve: Just in case some part of the xstate buffer mode that’s supposed to be invariant got corrupted or in case there is some side channel that can leak the INIT-but-not-zeroed contents of a state to user code, we should really wipe the memory completely across privilege boundaries.
__fpu__restore_sig: the utterly daft copy from user space needs special recovery.
Maybe the right solution is to rename it. Instead of fpu__clear_all(), how about fpu__wipe_and_reset()?
>
> which invoke that function unconditionally.
>
> Thanks,
>
> tglx
>
