On 5/31/21 3:46 PM, Thomas Gleixner wrote:
> 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())
>
I like it.
Here's my current patch pile. Not even really compile tested -- it's
bedtime. Your patch is copied in verbatim.
Attachment:
my-patches.tar
Description: Unix tar archive
