Re: [RFC v2 1/2] x86/fpu: Fix state corruption in __fpu__restore_sig()

[Thomas Gleixner <tglx@linutronix.de>]

On Mon, May 31 2021 at 12:30, Andy Lutomirski wrote:
> On Mon, May 31, 2021, at 11:56 AM, Thomas Gleixner wrote:
>> 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.

Looks okay is meh... That other stuff obviously looked okay as well...

That FPU code is an unpenetrable mess.

> 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()?

The right solution is to just use copy_user_to_xstate() unconditionally.

That fixes the issue even without cleaning up that fpu_clear() mess
which we want to do nevertheless.

I have a similar fix for the related xstateregs_set() trainwreck, but
that really needs to allocate a buffer because it's operating on a
different task contrary to signal handling.

I'm too tired now to test the xstateregs_set() muck, but if you want to
have a look:

     https://tglx.de/~tglx/patches.tar

Thanks,

        tglx
---
--- a/arch/x86/include/asm/fpu/xstate.h
+++ b/arch/x86/include/asm/fpu/xstate.h
@@ -112,8 +112,4 @@ void copy_supervisor_to_kernel(struct xr
 void copy_dynamic_supervisor_to_kernel(struct xregs_state *xstate, u64 mask);
 void copy_kernel_to_dynamic_supervisor(struct xregs_state *xstate, u64 mask);
 
-
-/* Validate an xstate header supplied by userspace (ptrace or sigreturn) */
-int validate_user_xstate_header(const struct xstate_header *hdr);
-
 #endif
--- a/arch/x86/kernel/fpu/signal.c
+++ b/arch/x86/kernel/fpu/signal.c
@@ -405,14 +405,7 @@ static int __fpu__restore_sig(void __use
 	if (use_xsave() && !fx_only) {
 		u64 init_bv = xfeatures_mask_user() & ~user_xfeatures;
 
-		if (using_compacted_format()) {
-			ret = copy_user_to_xstate(&fpu->state.xsave, buf_fx);
-		} else {
-			ret = __copy_from_user(&fpu->state.xsave, buf_fx, state_size);
-
-			if (!ret && state_size > offsetof(struct xregs_state, header))
-				ret = validate_user_xstate_header(&fpu->state.xsave.header);
-		}
+		ret = copy_user_to_xstate(&fpu->state.xsave, buf_fx);
 		if (ret)
 			goto err_out;
 
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -515,7 +515,7 @@ int using_compacted_format(void)
 }
 
 /* Validate an xstate header supplied by userspace (ptrace or sigreturn) */
-int validate_user_xstate_header(const struct xstate_header *hdr)
+static int validate_user_xstate_header(const struct xstate_header *hdr)
 {
 	/* No unknown or supervisor features may be set */
 	if (hdr->xfeatures & ~xfeatures_mask_user())