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