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