From: Andy Lutomirski luto@kernel.org
If XRSTOR fails due to sufficiently complicated paging errors (e.g. concurrent TLB invalidation), it may fault with #PF but still modify portions of the user extended state.
If this happens in __fpu_restore_sig() with a victim task's FPU registers loaded and the task is preempted by the 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 + * to the user portion of the register file, and + * 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. + */ + } + fpregs_unlock(); } else { /*
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.
On 6/2/21 8:06 AM, Borislav Petkov wrote:
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?
Think "complex microarchitectural conditions".
How about:
As far as I can tell, both Intel and AMD consider it to be architecturally valid for XRSTOR to fail with #PF but nonetheless change user state. The actual conditions under which this might occur are unclear [1], but it seems plausible that this might be triggered if one sibling thread unmaps a page and invalidates the shared TLB while another sibling thread is executing XRSTOR on the page in question.
__fpu__restore_sig() can execute XRSTOR while the hardware registers are preserved on behalf of a different victim task (using the fpu_fpregs_owner_ctx mechanism), and, in theory, XRSTOR could fail but modify the registers. If this happens, then there is a window in which __fpu__restore_sig() could schedule out and the victim task could schedule back in without reloading its own FPU registers. This would result in part of the FPU state that __fpu__restore_sig() was attempting to load leaking into the victim task's user-visible state.
Invalidate preserved FPU registers on XRSTOR failure to prevent this situation from corrupting any state.
[1] Frequent readers of the errata lists might imagine "complex microarchitectural conditions".
* 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.
I'm fine either way.
On Thu, Jun 03, 2021 at 10:30:05AM -0700, Andy Lutomirski wrote:
Think "complex microarchitectural conditions".
Ah, the magic phrase.
How about:
As far as I can tell, both Intel and AMD consider it to be architecturally valid for XRSTOR to fail with #PF but nonetheless change user state. The actual conditions under which this might occur are unclear [1], but it seems plausible that this might be triggered if one sibling thread unmaps a page and invalidates the shared TLB while another sibling thread is executing XRSTOR on the page in question.
__fpu__restore_sig() can execute XRSTOR while the hardware registers are preserved on behalf of a different victim task (using the fpu_fpregs_owner_ctx mechanism), and, in theory, XRSTOR could fail but modify the registers. If this happens, then there is a window in which __fpu__restore_sig() could schedule out and the victim task could schedule back in without reloading its own FPU registers. This would result in part of the FPU state that __fpu__restore_sig() was attempting to load leaking into the victim task's user-visible state.
Invalidate preserved FPU registers on XRSTOR failure to prevent this situation from corrupting any state.
[1] Frequent readers of the errata lists might imagine "complex microarchitectural conditions".
Yap, very nice, thanks!
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.
I'm fine either way.
Ok, then let's aim for common, no-surprise-there patterns as we're in a mine field here anyway.
Thx.
linux-stable-mirror@lists.linaro.org
-
Andy Lutomirski -
Borislav Petkov -
Thomas Gleixner