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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 2 Jun 2021 17:06:29 +0200
From:   Borislav Petkov <bp@...en8.de>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     LKML <linux-kernel@...r.kernel.org>, x86@...nel.org,
        Andy Lutomirski <luto@...nel.org>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Fenghua Yu <fenghua.yu@...el.com>,
        Tony Luck <tony.luck@...el.com>,
        Yu-cheng Yu <yu-cheng.yu@...el.com>, stable@...r.kernel.org
Subject: Re: [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@...nel.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@...nel.org>
> Signed-off-by: Thomas Gleixner <tglx@...utronix.de>
> Cc: stable@...r.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