lkml.org 
[lkml]   [2021]   [Jun]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [patch 3/8] x86/fpu: Invalidate FPU state after a failed XRSTOR from a user buffer
On Wed, Jun 02, 2021 at 11:55:46AM +0200, Thomas Gleixner wrote:
> From: Andy Lutomirski <luto@kernel.org>
>
> If XRSTOR fails due to sufficiently complicated paging errors (e.g.
> concurrent TLB invalidation),

I can't connect "concurrent TLB invalidation" to "sufficiently
complicated paging errors". Can you elaborate pls?

> it may fault with #PF but still modify
> portions of the user extended state.

Yikes, leaky leaky insn.

> If this happens in __fpu_restore_sig() with a victim task's FPU registers
> loaded and the task is preempted by the victim task,

This is probably meaning another task but the only task mentioned here
is a "victim task"?

> the victim task
> resumes with partially corrupt extended state.
>
> Invalidate the FPU registers when XRSTOR fails to prevent this scenario.
>
> Fixes: 1d731e731c4c ("x86/fpu: Add a fastpath to __fpu__restore_sig()")
> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: stable@vger.kernel.org
> ---
> arch/x86/kernel/fpu/signal.c | 21 +++++++++++++++++++++
> 1 file changed, 21 insertions(+)
>
> --- a/arch/x86/kernel/fpu/signal.c
> +++ b/arch/x86/kernel/fpu/signal.c
> @@ -369,6 +369,27 @@ static int __fpu__restore_sig(void __use
> fpregs_unlock();
> return 0;
> }
> +
> + if (test_thread_flag(TIF_NEED_FPU_LOAD)) {
> + /*
> + * The FPU registers do not belong to current, and
> + * we just did an FPU restore operation, restricted

Please get rid of the "we"-personal pronouns formulations.

> + * to the user portion of the register file, and

"register file"? That sounds like comment which belongs in microcode but
not in software. :-)

> + * failed. In the event that the ucode was
> + * unfriendly and modified the registers at all, we
> + * need to make sure that we aren't corrupting an
> + * innocent non-current task's registers.
> + */
> + __cpu_invalidate_fpregs_state();
> + } else {
> + /*
> + * As above, we may have just clobbered current's
> + * user FPU state. We will either successfully
> + * load it or clear it below, so no action is
> + * required here.
> + */
> + }

I'm wondering if that comment can simply be above the TIF_NEED_FPU_LOAD
testing, standalone, instead of having it in an empty else? And then get
rid of that else.

Thx.

--
Regards/Gruss,
Boris.

https://people.kernel.org/tglx/notes-about-netiquette

\
 
 \ /
  Last update: 2021-06-02 17:06    [W:0.270 / U:0.524 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site