Re: [patch 3/8] x86/fpu: Invalidate FPU state after a failed XRSTOR from a user buffer

[Borislav Petkov]

On Wed, Jun 02, 2021 at 11:55:46AM +0200, Thomas Gleixner wrote:
> From: Andy Lutomirski <luto@xxxxxxxxxx>
> 
> 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@xxxxxxxxxx>
> Signed-off-by: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx
> ---
>  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