Re: [Xen-devel] Patch "x86/entry/64: Remove %ebx handling from error_entry/exit" has been added to the 4.9-stable tree
On Fri, 2018-12-07 at 12:18 +, David Woodhouse wrote: > > > #else > > + struct multicall_space mc = __xen_mc_entry(0); > > + MULTI_set_segment_base(mc.mc, SEGBASE_GS_USER_SEL, 0); > > + > > loadsegment(fs, 0); > > #endif > > That seems to boot and run, at least. > > I'm going to experiment with sticking a SCHEDOP_yield in the batch > *after* the update_descriptor requests, to see if I can trigger the > original problem a bit quicker than my current method — which involves > running a hundred machines for a day or two. That SCHEDOP_yield and some debug output in xen_failsafe_callback shows that it's nice and easy to reproduce now without the above MULTI_set_segment_base() call. And stopped happening when I add the MULTI_set_segment_base() call back again. I'll call that 'tested'. But now we're making a hypercall to clear user %gs even in the case where none of the descriptors have changed; we should probably stop doing that... Testing this (incremental) variant now. --- a/arch/x86/xen/enlighten_pv.c +++ b/arch/x86/xen/enlighten_pv.c @@ -688,7 +688,7 @@ static inline bool desc_equal(const struct desc_struct *d1, } static void load_TLS_descriptor(struct thread_struct *t, - unsigned int cpu, unsigned int i) + unsigned int cpu, unsigned int i, int flush_gs) { struct desc_struct *shadow = &per_cpu(shadow_tls_desc, cpu).desc[i]; struct desc_struct *gdt; @@ -698,6 +698,15 @@ static void load_TLS_descriptor(struct thread_struct *t, if (desc_equal(shadow, &t->tls_array[i])) return; + /* +* If the current user %gs points to a descriptor we're changing, +* zero it first to avoid taking a fault if Xen preempts this +* vCPU between now and the time that %gs is later loaded with +* the new value. +*/ + if ((flush_gs >> 3) == GDT_ENTRY_TLS_MIN + i) + MULTI_set_segment_base(mc.mc, SEGBASE_GS_USER_SEL, 0); + *shadow = t->tls_array[i]; gdt = get_cpu_gdt_table(cpu); @@ -709,7 +718,7 @@ static void load_TLS_descriptor(struct thread_struct *t, static void xen_load_tls(struct thread_struct *t, unsigned int cpu) { - xen_mc_batch(); + u16 flush_gs = 0; /* * XXX sleazy hack: If we're being called in a lazy-cpu zone @@ -723,17 +732,19 @@ static void xen_load_tls(struct thread_struct *t, unsigned int cpu) * * For x86_64, we need to zero %fs, otherwise we may get an * exception between the new %fs descriptor being loaded and -* %fs being effectively cleared at __switch_to(). We can't -* just zero %gs, but we do need to clear the selector in -* case of a Xen vCPU context switch before it gets reloaded -* which would also cause a fault. +* %fs being effectively cleared at __switch_to(). +* +* We may also need to zero %gs, if it refers to a descriptor +* which we are clearing. Otherwise a Xen vCPU context switch +* before it gets reloaded could also cause a fault. Since +* clearing the user %gs is another hypercall, do that only if +* it's necessary. */ if (paravirt_get_lazy_mode() == PARAVIRT_LAZY_CPU) { #ifdef CONFIG_X86_32 lazy_load_gs(0); #else - struct multicall_space mc = __xen_mc_entry(0); - MULTI_set_segment_base(mc.mc, SEGBASE_GS_USER_SEL, 0); + savesegment(gs, flush_gs); loadsegment(fs, 0); #endif @@ -741,9 +752,9 @@ static void xen_load_tls(struct thread_struct *t, unsigned int cpu) xen_mc_batch(); - load_TLS_descriptor(t, cpu, 0); - load_TLS_descriptor(t, cpu, 1); - load_TLS_descriptor(t, cpu, 2); + load_TLS_descriptor(t, cpu, 0, flush_gs); + load_TLS_descriptor(t, cpu, 1, flush_gs); + load_TLS_descriptor(t, cpu, 2, flush_gs); { struct multicall_space mc = __xen_mc_entry(0); smime.p7s Description: S/MIME cryptographic signature ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] Patch "x86/entry/64: Remove %ebx handling from error_entry/exit" has been added to the 4.9-stable tree
On Thu, 2018-12-06 at 20:27 +, David Woodhouse wrote: > On Thu, 2018-12-06 at 10:49 -0800, Andy Lutomirski wrote: > > > On Dec 6, 2018, at 9:36 AM, Andrew Cooper < > > > andrew.coop...@citrix.com> wrote: > > > Basically - what is happening is that xen_load_tls() is > > > invalidating the > > > %gs selector while %gs is still non-NUL. > > > > > > If this happens to intersect with a vcpu reschedule, %gs (being > > > non-NUL) > > > takes precedence over KERNGSBASE, and faults when Xen tries to > > > reload > > > it. This results in the failsafe callback being invoked. > > > > > > I think the correct course of action is to use > > > xen_load_gs_index(0) > > > (poorly named - it is a hypercall which does swapgs; mov to %gs; > > > swapgs) > > > before using update_descriptor() to invalidate the segment. > > > > > > That will reset %gs to 0 without touching KERNGSBASE, and can be > > > queued > > > in the same multicall as the update_descriptor() hypercall. > > > > Sounds good to me as long as we skip it on native. > > Like this? > #else > + struct multicall_space mc = __xen_mc_entry(0); > + MULTI_set_segment_base(mc.mc, SEGBASE_GS_USER_SEL, 0); > + > loadsegment(fs, 0); > #endif That seems to boot and run, at least. I'm going to experiment with sticking a SCHEDOP_yield in the batch *after* the update_descriptor requests, to see if I can trigger the original problem a bit quicker than my current method — which involves running a hundred machines for a day or two. Still wondering if the better fix is just to adjust the comments to admit that xen_failsafe_callback catches the race condition and fixes it up perfectly, by just letting the %gs selector be zero for a while? smime.p7s Description: S/MIME cryptographic signature ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] Patch "x86/entry/64: Remove %ebx handling from error_entry/exit" has been added to the 4.9-stable tree
On Thu, 2018-12-06 at 10:49 -0800, Andy Lutomirski wrote: > > On Dec 6, 2018, at 9:36 AM, Andrew Cooper wrote: > > Basically - what is happening is that xen_load_tls() is invalidating the > > %gs selector while %gs is still non-NUL. > > > > If this happens to intersect with a vcpu reschedule, %gs (being non-NUL) > > takes precedence over KERNGSBASE, and faults when Xen tries to reload > > it. This results in the failsafe callback being invoked. > > > > I think the correct course of action is to use xen_load_gs_index(0) > > (poorly named - it is a hypercall which does swapgs; mov to %gs; swapgs) > > before using update_descriptor() to invalidate the segment. > > > > That will reset %gs to 0 without touching KERNGSBASE, and can be queued > > in the same multicall as the update_descriptor() hypercall. > > Sounds good to me as long as we skip it on native. Like this? The other option is just to declare that we don't care. On the rare occasion that it does happen to preempt and then take the trap on reloading, xen_failsafe_callback is actually doing the right thing and just leaving %gs as zero. We'd just need to fix the comments so they explicitly note this case is handled there too. At the moment it just says 'Retry the IRET', as I noted before. diff --git a/arch/x86/include/asm/xen/hypercall.h b/arch/x86/include/asm/xen/hypercall.h index ef05bea7010d..e8b383b24246 100644 --- a/arch/x86/include/asm/xen/hypercall.h +++ b/arch/x86/include/asm/xen/hypercall.h @@ -520,4 +520,15 @@ MULTI_stack_switch(struct multicall_entry *mcl, trace_xen_mc_entry(mcl, 2); } +static inline void +MULTI_set_segment_base(struct multicall_entry *mcl, + int reg, unsigned long value) +{ + mcl->op = __HYPERVISOR_set_segment_base; + mcl->args[0] = reg; + mcl->args[1] = value; + + trace_xen_mc_entry(mcl, 2); +} + #endif /* _ASM_X86_XEN_HYPERCALL_H */ diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c index 2f6787fc7106..722f1f51e20c 100644 --- a/arch/x86/xen/enlighten_pv.c +++ b/arch/x86/xen/enlighten_pv.c @@ -527,6 +527,8 @@ static void load_TLS_descriptor(struct thread_struct *t, static void xen_load_tls(struct thread_struct *t, unsigned int cpu) { + xen_mc_batch(); + /* * XXX sleazy hack: If we're being called in a lazy-cpu zone * and lazy gs handling is enabled, it means we're in a @@ -537,24 +539,24 @@ static void xen_load_tls(struct thread_struct *t, unsigned int cpu) * This will go away as soon as Xen has been modified to not * save/restore %gs for normal hypercalls. * -* On x86_64, this hack is not used for %gs, because gs points -* to KERNEL_GS_BASE (and uses it for PDA references), so we -* must not zero %gs on x86_64 -* * For x86_64, we need to zero %fs, otherwise we may get an * exception between the new %fs descriptor being loaded and -* %fs being effectively cleared at __switch_to(). +* %fs being effectively cleared at __switch_to(). We can't +* just zero %gs, but we do need to clear the selector in +* case of a Xen vCPU context switch before it gets reloaded +* which would also cause a fault. */ if (paravirt_get_lazy_mode() == PARAVIRT_LAZY_CPU) { #ifdef CONFIG_X86_32 lazy_load_gs(0); #else + struct multicall_space mc = __xen_mc_entry(0); + MULTI_set_segment_base(mc.mc, SEGBASE_GS_USER_SEL, 0); + loadsegment(fs, 0); #endif } - xen_mc_batch(); - load_TLS_descriptor(t, cpu, 0); load_TLS_descriptor(t, cpu, 1); load_TLS_descriptor(t, cpu, 2); smime.p7s Description: S/MIME cryptographic signature ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] Patch "x86/entry/64: Remove %ebx handling from error_entry/exit" has been added to the 4.9-stable tree
> On Dec 6, 2018, at 9:36 AM, Andrew Cooper wrote: > >> On 06/12/2018 17:10, David Woodhouse wrote: >> On Wed, 2018-11-28 at 08:44 -0800, Andy Lutomirski wrote: Can we assume it's always from kernel? The Xen code definitely seems to handle invoking this from both kernel and userspace contexts. >>> I learned that my comment here was wrong shortly after the patch landed :( >> Turns out the only place I see it getting called from is under >> __context_switch(). >> >> #7 [8801144a7cf0] new_xen_failsafe_callback at a028028a >> [kmod_ebxfix] >> #8 [8801144a7d90] xen_hypercall_update_descriptor at 8100114a >> #9 [8801144a7db8] xen_hypercall_update_descriptor at 8100114a >> #10 [8801144a7df0] xen_mc_flush at 81006ab9 >> #11 [8801144a7e30] xen_end_context_switch at 81004e12 >> #12 [8801144a7e48] __switch_to at 81016582 >> #13 [8801144a7ea0] __schedule at 815d2b37 >> >> That …114a in xen_hypercall_update_descriptor is the 'pop' instruction >> right after the syscall; it's happening when Xen is preempting the >> domain in the hypercall and then reloads the segment registers to run >> that vCPU again later. >> >> [ 44185.225289] WARN: RDX: RSI: RDI: >> 000abbd76060 >> >> The update_descriptor hypercall args (rdi, rsi) were 0xabbd76060 and 0 >> respectively — it was setting a descriptor at that address, to zero. >> >> Xen then failed to load the selector 0x63 into the %gs register (since >> that descriptor has just been wiped?), leaving it zero. >> >> [ 44185.225256] WARN: xen_failsafe_callback from >> xen_hypercall_update_descriptor+0xa/0x40 >> [ 44185.225263] WARN: DS: 2b/2b ES: 2b/2b FS: 0/0 GS:0/63 >> >> This is on context switch from a 32-bit task to idle. So >> xen_failsafe_callback is returning to the "faulting" instruction, with >> a comment saying "Retry the IRET", but in fact is just continuing on >> its merry way with %gs unexpectedly set to zero. >> >> In fact I think this is probably fine in practice, since it's about to >> get explicitly set a few lines further down in __context_switch(). But >> it's odd enough, and far enough away from what's actually said by the >> comments, that I'm utterly unsure. >> >> In xen_load_tls() we explicitly only do the lazy_load_gs(0) for the >> 32-bit kernel. Is that really right? > > Basically - what is happening is that xen_load_tls() is invalidating the > %gs selector while %gs is still non-NUL. > > If this happens to intersect with a vcpu reschedule, %gs (being non-NUL) > takes precedence over KERNGSBASE, and faults when Xen tries to reload > it. This results in the failsafe callback being invoked. > > I think the correct course of action is to use xen_load_gs_index(0) > (poorly named - it is a hypercall which does swapgs; mov to %gs; swapgs) > before using update_descriptor() to invalidate the segment. > > That will reset %gs to 0 without touching KERNGSBASE, and can be queued > in the same multicall as the update_descriptor() hypercall. Sounds good to me as long as we skip it on native. ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] Patch "x86/entry/64: Remove %ebx handling from error_entry/exit" has been added to the 4.9-stable tree
On 06/12/2018 17:10, David Woodhouse wrote: > On Wed, 2018-11-28 at 08:44 -0800, Andy Lutomirski wrote: >>> Can we assume it's always from kernel? The Xen code definitely seems to >>> handle invoking this from both kernel and userspace contexts. >> I learned that my comment here was wrong shortly after the patch landed :( > Turns out the only place I see it getting called from is under > __context_switch(). > > #7 [8801144a7cf0] new_xen_failsafe_callback at a028028a > [kmod_ebxfix] > #8 [8801144a7d90] xen_hypercall_update_descriptor at 8100114a > #9 [8801144a7db8] xen_hypercall_update_descriptor at 8100114a > #10 [8801144a7df0] xen_mc_flush at 81006ab9 > #11 [8801144a7e30] xen_end_context_switch at 81004e12 > #12 [8801144a7e48] __switch_to at 81016582 > #13 [8801144a7ea0] __schedule at 815d2b37 > > That …114a in xen_hypercall_update_descriptor is the 'pop' instruction > right after the syscall; it's happening when Xen is preempting the > domain in the hypercall and then reloads the segment registers to run > that vCPU again later. > > [ 44185.225289] WARN: RDX: RSI: RDI: > 000abbd76060 > > The update_descriptor hypercall args (rdi, rsi) were 0xabbd76060 and 0 > respectively — it was setting a descriptor at that address, to zero. > > Xen then failed to load the selector 0x63 into the %gs register (since > that descriptor has just been wiped?), leaving it zero. > > [ 44185.225256] WARN: xen_failsafe_callback from > xen_hypercall_update_descriptor+0xa/0x40 > [ 44185.225263] WARN: DS: 2b/2b ES: 2b/2b FS: 0/0 GS:0/63 > > This is on context switch from a 32-bit task to idle. So > xen_failsafe_callback is returning to the "faulting" instruction, with > a comment saying "Retry the IRET", but in fact is just continuing on > its merry way with %gs unexpectedly set to zero. > > In fact I think this is probably fine in practice, since it's about to > get explicitly set a few lines further down in __context_switch(). But > it's odd enough, and far enough away from what's actually said by the > comments, that I'm utterly unsure. > > In xen_load_tls() we explicitly only do the lazy_load_gs(0) for the > 32-bit kernel. Is that really right? Basically - what is happening is that xen_load_tls() is invalidating the %gs selector while %gs is still non-NUL. If this happens to intersect with a vcpu reschedule, %gs (being non-NUL) takes precedence over KERNGSBASE, and faults when Xen tries to reload it. This results in the failsafe callback being invoked. I think the correct course of action is to use xen_load_gs_index(0) (poorly named - it is a hypercall which does swapgs; mov to %gs; swapgs) before using update_descriptor() to invalidate the segment. That will reset %gs to 0 without touching KERNGSBASE, and can be queued in the same multicall as the update_descriptor() hypercall. ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] Patch "x86/entry/64: Remove %ebx handling from error_entry/exit" has been added to the 4.9-stable tree
On Wed, 2018-11-28 at 08:44 -0800, Andy Lutomirski wrote: > > Can we assume it's always from kernel? The Xen code definitely seems to > > handle invoking this from both kernel and userspace contexts. > > I learned that my comment here was wrong shortly after the patch landed :( Turns out the only place I see it getting called from is under __context_switch(). #7 [8801144a7cf0] new_xen_failsafe_callback at a028028a [kmod_ebxfix] #8 [8801144a7d90] xen_hypercall_update_descriptor at 8100114a #9 [8801144a7db8] xen_hypercall_update_descriptor at 8100114a #10 [8801144a7df0] xen_mc_flush at 81006ab9 #11 [8801144a7e30] xen_end_context_switch at 81004e12 #12 [8801144a7e48] __switch_to at 81016582 #13 [8801144a7ea0] __schedule at 815d2b37 That …114a in xen_hypercall_update_descriptor is the 'pop' instruction right after the syscall; it's happening when Xen is preempting the domain in the hypercall and then reloads the segment registers to run that vCPU again later. [ 44185.225289] WARN: RDX: RSI: RDI: 000abbd76060 The update_descriptor hypercall args (rdi, rsi) were 0xabbd76060 and 0 respectively — it was setting a descriptor at that address, to zero. Xen then failed to load the selector 0x63 into the %gs register (since that descriptor has just been wiped?), leaving it zero. [ 44185.225256] WARN: xen_failsafe_callback from xen_hypercall_update_descriptor+0xa/0x40 [ 44185.225263] WARN: DS: 2b/2b ES: 2b/2b FS: 0/0 GS:0/63 This is on context switch from a 32-bit task to idle. So xen_failsafe_callback is returning to the "faulting" instruction, with a comment saying "Retry the IRET", but in fact is just continuing on its merry way with %gs unexpectedly set to zero. In fact I think this is probably fine in practice, since it's about to get explicitly set a few lines further down in __context_switch(). But it's odd enough, and far enough away from what's actually said by the comments, that I'm utterly unsure. In xen_load_tls() we explicitly only do the lazy_load_gs(0) for the 32-bit kernel. Is that really right? smime.p7s Description: S/MIME cryptographic signature ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] Patch "x86/entry/64: Remove %ebx handling from error_entry/exit" has been added to the 4.9-stable tree
> On Nov 28, 2018, at 6:56 AM, David Woodhouse wrote: > >> On Wed, 2018-08-22 at 09:19 +0200, gre...@linuxfoundation.org wrote: >> This is a note to let you know that I've just added the patch titled >> >>x86/entry/64: Remove %ebx handling from error_entry/exit >> >> to the 4.9-stable tree which can be found at: >> >> http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary >> >> The filename of the patch is: >> x86-entry-64-remove-ebx-handling-from-error_entry-exit.patch >> and it can be found in the queue-4.9 subdirectory. > > Can we have it for 4.4 too, please? > >> [ Note to stable maintainers: this should probably get applied to all >> kernels. If you're nervous about that, a more conservative fix to >> add xorl %ebx,%ebx; incl %ebx before the jump to error_exit should >> also fix the problem. ] > > Can we assume it's always from kernel? The Xen code definitely seems to > handle invoking this from both kernel and userspace contexts. I learned that my comment here was wrong shortly after the patch landed :( ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] Patch "x86/entry/64: Remove %ebx handling from error_entry/exit" has been added to the 4.9-stable tree
On Wed, Nov 28, 2018 at 02:56:32PM +, David Woodhouse wrote: On Wed, 2018-08-22 at 09:19 +0200, gre...@linuxfoundation.org wrote: This is a note to let you know that I've just added the patch titled x86/entry/64: Remove %ebx handling from error_entry/exit to the 4.9-stable tree which can be found at: http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary The filename of the patch is: x86-entry-64-remove-ebx-handling-from-error_entry-exit.patch and it can be found in the queue-4.9 subdirectory. Can we have it for 4.4 too, please? [ Note to stable maintainers: this should probably get applied to all kernels. If you're nervous about that, a more conservative fix to add xorl %ebx,%ebx; incl %ebx before the jump to error_exit should also fix the problem. ] Can we assume it's always from kernel? The Xen code definitely seems to handle invoking this from both kernel and userspace contexts. Shouldn't %ebx get set to !(regs->rsp & 3) ? Either way, let's just do it in the stable tree exactly the same way it's done upstream. - * On entry, EBX is a "return to kernel mode" flag: Re-introduce the typo 'EBS' here, to make the patch apply cleanly to 4.4. It's only removing that line anyway. Or just cherry-pick upstream commit 75ca5b22260ef7 first. Queued for 4.4. I've just grabbed the extra spellcheck fix as well. -- Thanks, Sasha ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] Patch "x86/entry/64: Remove %ebx handling from error_entry/exit" has been added to the 4.9-stable tree
On Wed, 2018-08-22 at 09:19 +0200, gre...@linuxfoundation.org wrote: > This is a note to let you know that I've just added the patch titled > > x86/entry/64: Remove %ebx handling from error_entry/exit > > to the 4.9-stable tree which can be found at: > > http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary > > The filename of the patch is: > x86-entry-64-remove-ebx-handling-from-error_entry-exit.patch > and it can be found in the queue-4.9 subdirectory. Can we have it for 4.4 too, please? > [ Note to stable maintainers: this should probably get applied to all > kernels. If you're nervous about that, a more conservative fix to > add xorl %ebx,%ebx; incl %ebx before the jump to error_exit should > also fix the problem. ] Can we assume it's always from kernel? The Xen code definitely seems to handle invoking this from both kernel and userspace contexts. Shouldn't %ebx get set to !(regs->rsp & 3) ? Either way, let's just do it in the stable tree exactly the same way it's done upstream. > - * On entry, EBX is a "return to kernel mode" flag: Re-introduce the typo 'EBS' here, to make the patch apply cleanly to 4.4. It's only removing that line anyway. Or just cherry-pick upstream commit 75ca5b22260ef7 first. smime.p7s Description: S/MIME cryptographic signature ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] Patch "x86/entry/64: Remove %ebx handling from error_entry/exit" has been added to the 4.9-stable tree
This is a note to let you know that I've just added the patch titled x86/entry/64: Remove %ebx handling from error_entry/exit to the 4.9-stable tree which can be found at: http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary The filename of the patch is: x86-entry-64-remove-ebx-handling-from-error_entry-exit.patch and it can be found in the queue-4.9 subdirectory. If you, or anyone else, feels it should not be added to the stable tree, please let know about it. From b3681dd548d06deb2e1573890829dff4b15abf46 Mon Sep 17 00:00:00 2001 From: Andy Lutomirski Date: Sun, 22 Jul 2018 11:05:09 -0700 Subject: x86/entry/64: Remove %ebx handling from error_entry/exit From: Andy Lutomirski commit b3681dd548d06deb2e1573890829dff4b15abf46 upstream. error_entry and error_exit communicate the user vs. kernel status of the frame using %ebx. This is unnecessary -- the information is in regs->cs. Just use regs->cs. This makes error_entry simpler and makes error_exit more robust. It also fixes a nasty bug. Before all the Spectre nonsense, the xen_failsafe_callback entry point returned like this: ALLOC_PT_GPREGS_ON_STACK SAVE_C_REGS SAVE_EXTRA_REGS ENCODE_FRAME_POINTER jmp error_exit And it did not go through error_entry. This was bogus: RBX contained garbage, and error_exit expected a flag in RBX. Fortunately, it generally contained *nonzero* garbage, so the correct code path was used. As part of the Spectre fixes, code was added to clear RBX to mitigate certain speculation attacks. Now, depending on kernel configuration, RBX got zeroed and, when running some Wine workloads, the kernel crashes. This was introduced by: commit 3ac6d8c787b8 ("x86/entry/64: Clear registers for exceptions/interrupts, to reduce speculation attack surface") With this patch applied, RBX is no longer needed as a flag, and the problem goes away. I suspect that malicious userspace could use this bug to crash the kernel even without the offending patch applied, though. [ Historical note: I wrote this patch as a cleanup before I was aware of the bug it fixed. ] [ Note to stable maintainers: this should probably get applied to all kernels. If you're nervous about that, a more conservative fix to add xorl %ebx,%ebx; incl %ebx before the jump to error_exit should also fix the problem. ] Reported-and-tested-by: M. Vefa Bicakci Signed-off-by: Andy Lutomirski Cc: Boris Ostrovsky Cc: Borislav Petkov Cc: Brian Gerst Cc: Dave Hansen Cc: Denys Vlasenko Cc: Dominik Brodowski Cc: Greg KH Cc: H. Peter Anvin Cc: Josh Poimboeuf Cc: Juergen Gross Cc: Linus Torvalds Cc: Peter Zijlstra Cc: Thomas Gleixner Cc: sta...@vger.kernel.org Cc: xen-devel@lists.xenproject.org Fixes: 3ac6d8c787b8 ("x86/entry/64: Clear registers for exceptions/interrupts, to reduce speculation attack surface") Link: http://lkml.kernel.org/r/b5010a090d3586b2d6e06c7ad3ec5542d1241c45.1532282627.git.l...@kernel.org Signed-off-by: Ingo Molnar Signed-off-by: Sarah Newman Signed-off-by: Greg Kroah-Hartman --- arch/x86/entry/entry_64.S | 20 1 file changed, 4 insertions(+), 16 deletions(-) --- a/arch/x86/entry/entry_64.S +++ b/arch/x86/entry/entry_64.S @@ -774,7 +774,7 @@ ENTRY(\sym) call\do_sym - jmp error_exit /* %ebx: no swapgs flag */ + jmp error_exit .endif END(\sym) .endm @@ -1043,7 +1043,6 @@ END(paranoid_exit) /* * Save all registers in pt_regs, and switch gs if needed. - * Return: EBX=0: came from user mode; EBX=1: otherwise */ ENTRY(error_entry) cld @@ -1056,7 +1055,6 @@ ENTRY(error_entry) * the kernel CR3 here. */ SWITCH_KERNEL_CR3 - xorl%ebx, %ebx testb $3, CS+8(%rsp) jz .Lerror_kernelspace @@ -1087,7 +1085,6 @@ ENTRY(error_entry) * for these here too. */ .Lerror_kernelspace: - incl%ebx leaqnative_irq_return_iret(%rip), %rcx cmpq%rcx, RIP+8(%rsp) je .Lerror_bad_iret @@ -1119,28 +1116,19 @@ ENTRY(error_entry) /* * Pretend that the exception came from user mode: set up pt_regs -* as if we faulted immediately after IRET and clear EBX so that -* error_exit knows that we will be returning to user mode. +* as if we faulted immediately after IRET. */ mov %rsp, %rdi callfixup_bad_iret mov %rax, %rsp - decl%ebx jmp .Lerror_entry_from_usermode_after_swapgs END(error_entry) - -/* - * On entry, EBX is a "return to kernel mode" flag: - * 1: already in kernel mode, don't need SWAPGS - * 0: user gsbase is loaded, we need SWAPGS and standard preparation for return to usermode - */ ENTRY(error_exit) - movl%ebx, %eax DISABLE_INTERRUPTS(CLBR_NONE) TRACE_IRQS_OFF - testl %eax, %eax - j
Re: [Xen-devel] [PATCH] x86/entry/64: Remove %ebx handling from error_entry/exit
On Thu, Aug 16, 2018 at 8:19 AM, Greg KH wrote: > On Fri, Aug 10, 2018 at 12:23:45AM -0700, Sarah Newman wrote: >> On 08/09/2018 05:41 AM, David Woodhouse wrote: >> > On Wed, 2018-08-08 at 10:35 -0700, Sarah Newman wrote: >> >> commit b3681dd548d06deb2e1573890829dff4b15abf46 upstream. >> >> >> >> This version applies to v4.9. >> > >> > I think you can kill the 'xorl %ebx,%ebx' from error_entry too but yes, >> > this does want to go to 4.9 and earlier because the 'Fixes:' tag is a >> > bit of a lie — the problem existed before that, at least in theory. >> >> The commit 2140a9942 "x86/entry/64: Relax pvops stub clobber >> specifications" was what removed the "movl %ebx, %eax" line later on >> originally, but it was the commit 3ac6d8c787b8 that removed the >> 'xorl %ebx,%ebx'. So these weren't matched. >> >> I don't know if it's a concern, but if someone had gone to the effort of >> backporting the original commit 3ac6d8c787b83, adding the removal of >> 'xorl %ebx,%ebx' to this patch would create merge conflicts. >> For that reason and given the line is harmless, should it be left in? > > I need some kind of agreement here for me to know what to do with this > patch... {hint} > I would remove the xorl. If there's an actual candidate patch, I'd be happy to read it. ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] x86/entry/64: Remove %ebx handling from error_entry/exit
On Fri, Aug 10, 2018 at 12:23:45AM -0700, Sarah Newman wrote: > On 08/09/2018 05:41 AM, David Woodhouse wrote: > > On Wed, 2018-08-08 at 10:35 -0700, Sarah Newman wrote: > >> commit b3681dd548d06deb2e1573890829dff4b15abf46 upstream. > >> > >> This version applies to v4.9. > > > > I think you can kill the 'xorl %ebx,%ebx' from error_entry too but yes, > > this does want to go to 4.9 and earlier because the 'Fixes:' tag is a > > bit of a lie — the problem existed before that, at least in theory. > > The commit 2140a9942 "x86/entry/64: Relax pvops stub clobber > specifications" was what removed the "movl %ebx, %eax" line later on > originally, but it was the commit 3ac6d8c787b8 that removed the > 'xorl %ebx,%ebx'. So these weren't matched. > > I don't know if it's a concern, but if someone had gone to the effort of > backporting the original commit 3ac6d8c787b83, adding the removal of > 'xorl %ebx,%ebx' to this patch would create merge conflicts. > For that reason and given the line is harmless, should it be left in? I need some kind of agreement here for me to know what to do with this patch... {hint} thanks, greg k-h ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] x86/entry/64: Remove %ebx handling from error_entry/exit
On 08/09/2018 05:41 AM, David Woodhouse wrote: > On Wed, 2018-08-08 at 10:35 -0700, Sarah Newman wrote: >> commit b3681dd548d06deb2e1573890829dff4b15abf46 upstream. >> >> This version applies to v4.9. > > I think you can kill the 'xorl %ebx,%ebx' from error_entry too but yes, > this does want to go to 4.9 and earlier because the 'Fixes:' tag is a > bit of a lie — the problem existed before that, at least in theory. The commit 2140a9942 "x86/entry/64: Relax pvops stub clobber specifications" was what removed the "movl %ebx, %eax" line later on originally, but it was the commit 3ac6d8c787b8 that removed the 'xorl %ebx,%ebx'. So these weren't matched. I don't know if it's a concern, but if someone had gone to the effort of backporting the original commit 3ac6d8c787b83, adding the removal of 'xorl %ebx,%ebx' to this patch would create merge conflicts. For that reason and given the line is harmless, should it be left in? > >> From Andy Lutomirski, original author: >> >> error_entry and error_exit communicate the user vs kernel status of >> the frame using %ebx. This is unnecessary -- the information is in >> regs->cs. Just use regs->cs. >> >> This makes error_entry simpler and makes error_exit more robust. >> >> It also fixes a nasty bug. Before all the Spectre nonsense, The >> xen_failsafe_callback entry point returned like this: >> >> ALLOC_PT_GPREGS_ON_STACK >> SAVE_C_REGS >> SAVE_EXTRA_REGS >> ENCODE_FRAME_POINTER >> jmp error_exit >> >> And it did not go through error_entry. This was bogus: RBX >> contained garbage, and error_exit expected a flag in RBX. >> Fortunately, it generally contained *nonzero* garbage, so the >> correct code path was used. As part of the Spectre fixes, code was >> added to clear RBX to mitigate certain speculation attacks. Now, >> depending on kernel configuration, RBX got zeroed and, when running >> some Wine workloads, the kernel crashes. This was introduced by: >> >> commit 3ac6d8c787b8 ("x86/entry/64: Clear registers for >> exceptions/interrupts, to reduce speculation attack surface") >> >> With this patch applied, RBX is no longer needed as a flag, and the >> problem goes away. >> >> I suspect that malicious userspace could use this bug to crash the >> kernel even without the offending patch applied, though. >> >> [Historical note: I wrote this patch as a cleanup before I was aware >> of the bug it fixed.] >> >> [Note to stable maintainers: this should probably get applied to all >> kernels.] >> >> Cc: Brian Gerst >> Cc: Borislav Petkov >> Cc: Dominik Brodowski >> Cc: Ingo Molnar >> Cc: "H. Peter Anvin" >> Cc: Thomas Gleixner >> Cc: Boris Ostrovsky >> Cc: Juergen Gross >> Cc: xen-devel@lists.xenproject.org >> Cc: x...@kernel.org >> Cc: sta...@vger.kernel.org >> Cc: Andy Lutomirski >> Fixes: 3ac6d8c787b8 ("x86/entry/64: Clear registers for >> exceptions/interrupts, to reduce speculation attack surface") >> Reported-and-tested-by: "M. Vefa Bicakci" >> Signed-off-by: Andy Lutomirski >> Signed-off-by: Sarah Newman >> --- >> arch/x86/entry/entry_64.S | 19 --- >> 1 file changed, 4 insertions(+), 15 deletions(-) >> >> diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S >> index d58d8dc..0dab47a 100644 >> --- a/arch/x86/entry/entry_64.S >> +++ b/arch/x86/entry/entry_64.S >> @@ -774,7 +774,7 @@ ENTRY(\sym) >> >> call\do_sym >> >> -jmp error_exit /* %ebx: no >> swapgs flag */ >> +jmp error_exit >> .endif >> END(\sym) >> .endm >> @@ -1043,7 +1043,6 @@ END(paranoid_exit) >> >> /* >> * Save all registers in pt_regs, and switch gs if needed. >> - * Return: EBX=0: came from user mode; EBX=1: otherwise >> */ >> ENTRY(error_entry) >> cld >> @@ -1087,7 +1086,6 @@ ENTRY(error_entry) >> * for these here too. >> */ >> .Lerror_kernelspace: >> -incl%ebx >> leaqnative_irq_return_iret(%rip), %rcx >> cmpq%rcx, RIP+8(%rsp) >> je .Lerror_bad_iret >> @@ -1119,28 +1117,19 @@ ENTRY(error_entry) >> >> /* >> * Pretend that the exception came from user mode: set up >> pt_regs >> - * as if we faulted immediately after IRET and clear EBX so >> that >> - * error_exit knows that we will be returning to user mode. >> + * as if we faulted immediately after IRET. >> */ >> mov %rsp, %rdi >> callfixup_bad_iret >> mov %rax, %rsp >> -decl%ebx >> jmp .Lerror_entry_from_usermode_after_swapgs >> END(error_entry) >> >> - >> -/* >> - * On entry, EBX is a "return to kernel mode" flag: >> - * 1: already in kernel mode, don't need SWAPGS >> - * 0: user gsbase is loaded, we need SWAPGS and standard >> preparation for return to usermode >> - */ >> ENTRY(error_exit) >> -movl%ebx, %eax >> DISABLE_INTERRUPTS(CLBR_NONE) >> TRACE_IRQS_OFF >> -testl %eax, %eax >> -jnz retint_kernel >> +testb
Re: [Xen-devel] [PATCH] x86/entry/64: Remove %ebx handling from error_entry/exit
On Wed, 2018-08-08 at 10:35 -0700, Sarah Newman wrote: > commit b3681dd548d06deb2e1573890829dff4b15abf46 upstream. > > This version applies to v4.9. I think you can kill the 'xorl %ebx,%ebx' from error_entry too but yes, this does want to go to 4.9 and earlier because the 'Fixes:' tag is a bit of a lie — the problem existed before that, at least in theory. > From Andy Lutomirski, original author: > > error_entry and error_exit communicate the user vs kernel status of > the frame using %ebx. This is unnecessary -- the information is in > regs->cs. Just use regs->cs. > > This makes error_entry simpler and makes error_exit more robust. > > It also fixes a nasty bug. Before all the Spectre nonsense, The > xen_failsafe_callback entry point returned like this: > > ALLOC_PT_GPREGS_ON_STACK > SAVE_C_REGS > SAVE_EXTRA_REGS > ENCODE_FRAME_POINTER > jmp error_exit > > And it did not go through error_entry. This was bogus: RBX > contained garbage, and error_exit expected a flag in RBX. > Fortunately, it generally contained *nonzero* garbage, so the > correct code path was used. As part of the Spectre fixes, code was > added to clear RBX to mitigate certain speculation attacks. Now, > depending on kernel configuration, RBX got zeroed and, when running > some Wine workloads, the kernel crashes. This was introduced by: > > commit 3ac6d8c787b8 ("x86/entry/64: Clear registers for > exceptions/interrupts, to reduce speculation attack surface") > > With this patch applied, RBX is no longer needed as a flag, and the > problem goes away. > > I suspect that malicious userspace could use this bug to crash the > kernel even without the offending patch applied, though. > > [Historical note: I wrote this patch as a cleanup before I was aware > of the bug it fixed.] > > [Note to stable maintainers: this should probably get applied to all > kernels.] > > Cc: Brian Gerst > Cc: Borislav Petkov > Cc: Dominik Brodowski > Cc: Ingo Molnar > Cc: "H. Peter Anvin" > Cc: Thomas Gleixner > Cc: Boris Ostrovsky > Cc: Juergen Gross > Cc: xen-devel@lists.xenproject.org > Cc: x...@kernel.org > Cc: sta...@vger.kernel.org > Cc: Andy Lutomirski > Fixes: 3ac6d8c787b8 ("x86/entry/64: Clear registers for > exceptions/interrupts, to reduce speculation attack surface") > Reported-and-tested-by: "M. Vefa Bicakci" > Signed-off-by: Andy Lutomirski > Signed-off-by: Sarah Newman > --- > arch/x86/entry/entry_64.S | 19 --- > 1 file changed, 4 insertions(+), 15 deletions(-) > > diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S > index d58d8dc..0dab47a 100644 > --- a/arch/x86/entry/entry_64.S > +++ b/arch/x86/entry/entry_64.S > @@ -774,7 +774,7 @@ ENTRY(\sym) > > call\do_sym > > - jmp error_exit /* %ebx: no > swapgs flag */ > + jmp error_exit > .endif > END(\sym) > .endm > @@ -1043,7 +1043,6 @@ END(paranoid_exit) > > /* > * Save all registers in pt_regs, and switch gs if needed. > - * Return: EBX=0: came from user mode; EBX=1: otherwise > */ > ENTRY(error_entry) > cld > @@ -1087,7 +1086,6 @@ ENTRY(error_entry) > * for these here too. > */ > .Lerror_kernelspace: > - incl%ebx > leaqnative_irq_return_iret(%rip), %rcx > cmpq%rcx, RIP+8(%rsp) > je .Lerror_bad_iret > @@ -1119,28 +1117,19 @@ ENTRY(error_entry) > > /* > * Pretend that the exception came from user mode: set up > pt_regs > - * as if we faulted immediately after IRET and clear EBX so > that > - * error_exit knows that we will be returning to user mode. > + * as if we faulted immediately after IRET. > */ > mov %rsp, %rdi > callfixup_bad_iret > mov %rax, %rsp > - decl%ebx > jmp .Lerror_entry_from_usermode_after_swapgs > END(error_entry) > > - > -/* > - * On entry, EBX is a "return to kernel mode" flag: > - * 1: already in kernel mode, don't need SWAPGS > - * 0: user gsbase is loaded, we need SWAPGS and standard > preparation for return to usermode > - */ > ENTRY(error_exit) > - movl%ebx, %eax > DISABLE_INTERRUPTS(CLBR_NONE) > TRACE_IRQS_OFF > - testl %eax, %eax > - jnz retint_kernel > + testb $3, CS(%rsp) > + jz retint_kernel > jmp retint_user > END(error_exit) > smime.p7s Description: S/MIME cryptographic signature ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH] x86/entry/64: Remove %ebx handling from error_entry/exit
commit b3681dd548d06deb2e1573890829dff4b15abf46 upstream. This version applies to v4.9. From Andy Lutomirski, original author: error_entry and error_exit communicate the user vs kernel status of the frame using %ebx. This is unnecessary -- the information is in regs->cs. Just use regs->cs. This makes error_entry simpler and makes error_exit more robust. It also fixes a nasty bug. Before all the Spectre nonsense, The xen_failsafe_callback entry point returned like this: ALLOC_PT_GPREGS_ON_STACK SAVE_C_REGS SAVE_EXTRA_REGS ENCODE_FRAME_POINTER jmp error_exit And it did not go through error_entry. This was bogus: RBX contained garbage, and error_exit expected a flag in RBX. Fortunately, it generally contained *nonzero* garbage, so the correct code path was used. As part of the Spectre fixes, code was added to clear RBX to mitigate certain speculation attacks. Now, depending on kernel configuration, RBX got zeroed and, when running some Wine workloads, the kernel crashes. This was introduced by: commit 3ac6d8c787b8 ("x86/entry/64: Clear registers for exceptions/interrupts, to reduce speculation attack surface") With this patch applied, RBX is no longer needed as a flag, and the problem goes away. I suspect that malicious userspace could use this bug to crash the kernel even without the offending patch applied, though. [Historical note: I wrote this patch as a cleanup before I was aware of the bug it fixed.] [Note to stable maintainers: this should probably get applied to all kernels.] Cc: Brian Gerst Cc: Borislav Petkov Cc: Dominik Brodowski Cc: Ingo Molnar Cc: "H. Peter Anvin" Cc: Thomas Gleixner Cc: Boris Ostrovsky Cc: Juergen Gross Cc: xen-devel@lists.xenproject.org Cc: x...@kernel.org Cc: sta...@vger.kernel.org Cc: Andy Lutomirski Fixes: 3ac6d8c787b8 ("x86/entry/64: Clear registers for exceptions/interrupts, to reduce speculation attack surface") Reported-and-tested-by: "M. Vefa Bicakci" Signed-off-by: Andy Lutomirski Signed-off-by: Sarah Newman --- arch/x86/entry/entry_64.S | 19 --- 1 file changed, 4 insertions(+), 15 deletions(-) diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S index d58d8dc..0dab47a 100644 --- a/arch/x86/entry/entry_64.S +++ b/arch/x86/entry/entry_64.S @@ -774,7 +774,7 @@ ENTRY(\sym) call\do_sym - jmp error_exit /* %ebx: no swapgs flag */ + jmp error_exit .endif END(\sym) .endm @@ -1043,7 +1043,6 @@ END(paranoid_exit) /* * Save all registers in pt_regs, and switch gs if needed. - * Return: EBX=0: came from user mode; EBX=1: otherwise */ ENTRY(error_entry) cld @@ -1087,7 +1086,6 @@ ENTRY(error_entry) * for these here too. */ .Lerror_kernelspace: - incl%ebx leaqnative_irq_return_iret(%rip), %rcx cmpq%rcx, RIP+8(%rsp) je .Lerror_bad_iret @@ -1119,28 +1117,19 @@ ENTRY(error_entry) /* * Pretend that the exception came from user mode: set up pt_regs -* as if we faulted immediately after IRET and clear EBX so that -* error_exit knows that we will be returning to user mode. +* as if we faulted immediately after IRET. */ mov %rsp, %rdi callfixup_bad_iret mov %rax, %rsp - decl%ebx jmp .Lerror_entry_from_usermode_after_swapgs END(error_entry) - -/* - * On entry, EBX is a "return to kernel mode" flag: - * 1: already in kernel mode, don't need SWAPGS - * 0: user gsbase is loaded, we need SWAPGS and standard preparation for return to usermode - */ ENTRY(error_exit) - movl%ebx, %eax DISABLE_INTERRUPTS(CLBR_NONE) TRACE_IRQS_OFF - testl %eax, %eax - jnz retint_kernel + testb $3, CS(%rsp) + jz retint_kernel jmp retint_user END(error_exit) -- 1.9.1 ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] Patch "x86/entry/64: Remove %ebx handling from error_entry/exit" has been added to the 4.17-stable tree
This is a note to let you know that I've just added the patch titled x86/entry/64: Remove %ebx handling from error_entry/exit to the 4.17-stable tree which can be found at: http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary The filename of the patch is: x86-entry-64-remove-ebx-handling-from-error_entry-exit.patch and it can be found in the queue-4.17 subdirectory. If you, or anyone else, feels it should not be added to the stable tree, please let know about it. From b3681dd548d06deb2e1573890829dff4b15abf46 Mon Sep 17 00:00:00 2001 From: Andy Lutomirski Date: Sun, 22 Jul 2018 11:05:09 -0700 Subject: x86/entry/64: Remove %ebx handling from error_entry/exit From: Andy Lutomirski commit b3681dd548d06deb2e1573890829dff4b15abf46 upstream. error_entry and error_exit communicate the user vs. kernel status of the frame using %ebx. This is unnecessary -- the information is in regs->cs. Just use regs->cs. This makes error_entry simpler and makes error_exit more robust. It also fixes a nasty bug. Before all the Spectre nonsense, the xen_failsafe_callback entry point returned like this: ALLOC_PT_GPREGS_ON_STACK SAVE_C_REGS SAVE_EXTRA_REGS ENCODE_FRAME_POINTER jmp error_exit And it did not go through error_entry. This was bogus: RBX contained garbage, and error_exit expected a flag in RBX. Fortunately, it generally contained *nonzero* garbage, so the correct code path was used. As part of the Spectre fixes, code was added to clear RBX to mitigate certain speculation attacks. Now, depending on kernel configuration, RBX got zeroed and, when running some Wine workloads, the kernel crashes. This was introduced by: commit 3ac6d8c787b8 ("x86/entry/64: Clear registers for exceptions/interrupts, to reduce speculation attack surface") With this patch applied, RBX is no longer needed as a flag, and the problem goes away. I suspect that malicious userspace could use this bug to crash the kernel even without the offending patch applied, though. [ Historical note: I wrote this patch as a cleanup before I was aware of the bug it fixed. ] [ Note to stable maintainers: this should probably get applied to all kernels. If you're nervous about that, a more conservative fix to add xorl %ebx,%ebx; incl %ebx before the jump to error_exit should also fix the problem. ] Reported-and-tested-by: M. Vefa Bicakci Signed-off-by: Andy Lutomirski Cc: Boris Ostrovsky Cc: Borislav Petkov Cc: Brian Gerst Cc: Dave Hansen Cc: Denys Vlasenko Cc: Dominik Brodowski Cc: Greg KH Cc: H. Peter Anvin Cc: Josh Poimboeuf Cc: Juergen Gross Cc: Linus Torvalds Cc: Peter Zijlstra Cc: Thomas Gleixner Cc: sta...@vger.kernel.org Cc: xen-devel@lists.xenproject.org Fixes: 3ac6d8c787b8 ("x86/entry/64: Clear registers for exceptions/interrupts, to reduce speculation attack surface") Link: http://lkml.kernel.org/r/b5010a090d3586b2d6e06c7ad3ec5542d1241c45.1532282627.git.l...@kernel.org Signed-off-by: Ingo Molnar Signed-off-by: Greg Kroah-Hartman --- arch/x86/entry/entry_64.S | 18 -- 1 file changed, 4 insertions(+), 14 deletions(-) --- a/arch/x86/entry/entry_64.S +++ b/arch/x86/entry/entry_64.S @@ -981,7 +981,7 @@ ENTRY(\sym) call\do_sym - jmp error_exit /* %ebx: no swapgs flag */ + jmp error_exit .endif END(\sym) .endm @@ -1222,7 +1222,6 @@ END(paranoid_exit) /* * Save all registers in pt_regs, and switch GS if needed. - * Return: EBX=0: came from user mode; EBX=1: otherwise */ ENTRY(error_entry) UNWIND_HINT_FUNC @@ -1269,7 +1268,6 @@ ENTRY(error_entry) * for these here too. */ .Lerror_kernelspace: - incl%ebx leaqnative_irq_return_iret(%rip), %rcx cmpq%rcx, RIP+8(%rsp) je .Lerror_bad_iret @@ -1303,28 +1301,20 @@ ENTRY(error_entry) /* * Pretend that the exception came from user mode: set up pt_regs -* as if we faulted immediately after IRET and clear EBX so that -* error_exit knows that we will be returning to user mode. +* as if we faulted immediately after IRET. */ mov %rsp, %rdi callfixup_bad_iret mov %rax, %rsp - decl%ebx jmp .Lerror_entry_from_usermode_after_swapgs END(error_entry) - -/* - * On entry, EBX is a "return to kernel mode" flag: - * 1: already in kernel mode, don't need SWAPGS - * 0: user gsbase is loaded, we need SWAPGS and standard preparation for return to usermode - */ ENTRY(error_exit) UNWIND_HINT_REGS DISABLE_INTERRUPTS(CLBR_ANY) TRACE_IRQS_OFF - testl %ebx, %ebx - jnz retint_kernel + testb $3, CS(%rsp) + jz retint_kernel jmp retint_user END(error_exit) Patches currently in stable-queue which might be from l...@kernel.org are queue-4.17/x86-entry-
[Xen-devel] Patch "x86/entry/64: Remove %ebx handling from error_entry/exit" has been added to the 4.14-stable tree
This is a note to let you know that I've just added the patch titled x86/entry/64: Remove %ebx handling from error_entry/exit to the 4.14-stable tree which can be found at: http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary The filename of the patch is: x86-entry-64-remove-ebx-handling-from-error_entry-exit.patch and it can be found in the queue-4.14 subdirectory. If you, or anyone else, feels it should not be added to the stable tree, please let know about it. From b3681dd548d06deb2e1573890829dff4b15abf46 Mon Sep 17 00:00:00 2001 From: Andy Lutomirski Date: Sun, 22 Jul 2018 11:05:09 -0700 Subject: x86/entry/64: Remove %ebx handling from error_entry/exit From: Andy Lutomirski commit b3681dd548d06deb2e1573890829dff4b15abf46 upstream. error_entry and error_exit communicate the user vs. kernel status of the frame using %ebx. This is unnecessary -- the information is in regs->cs. Just use regs->cs. This makes error_entry simpler and makes error_exit more robust. It also fixes a nasty bug. Before all the Spectre nonsense, the xen_failsafe_callback entry point returned like this: ALLOC_PT_GPREGS_ON_STACK SAVE_C_REGS SAVE_EXTRA_REGS ENCODE_FRAME_POINTER jmp error_exit And it did not go through error_entry. This was bogus: RBX contained garbage, and error_exit expected a flag in RBX. Fortunately, it generally contained *nonzero* garbage, so the correct code path was used. As part of the Spectre fixes, code was added to clear RBX to mitigate certain speculation attacks. Now, depending on kernel configuration, RBX got zeroed and, when running some Wine workloads, the kernel crashes. This was introduced by: commit 3ac6d8c787b8 ("x86/entry/64: Clear registers for exceptions/interrupts, to reduce speculation attack surface") With this patch applied, RBX is no longer needed as a flag, and the problem goes away. I suspect that malicious userspace could use this bug to crash the kernel even without the offending patch applied, though. [ Historical note: I wrote this patch as a cleanup before I was aware of the bug it fixed. ] [ Note to stable maintainers: this should probably get applied to all kernels. If you're nervous about that, a more conservative fix to add xorl %ebx,%ebx; incl %ebx before the jump to error_exit should also fix the problem. ] Reported-and-tested-by: M. Vefa Bicakci Signed-off-by: Andy Lutomirski Cc: Boris Ostrovsky Cc: Borislav Petkov Cc: Brian Gerst Cc: Dave Hansen Cc: Denys Vlasenko Cc: Dominik Brodowski Cc: Greg KH Cc: H. Peter Anvin Cc: Josh Poimboeuf Cc: Juergen Gross Cc: Linus Torvalds Cc: Peter Zijlstra Cc: Thomas Gleixner Cc: sta...@vger.kernel.org Cc: xen-devel@lists.xenproject.org Fixes: 3ac6d8c787b8 ("x86/entry/64: Clear registers for exceptions/interrupts, to reduce speculation attack surface") Link: http://lkml.kernel.org/r/b5010a090d3586b2d6e06c7ad3ec5542d1241c45.1532282627.git.l...@kernel.org Signed-off-by: Ingo Molnar Signed-off-by: Greg Kroah-Hartman --- arch/x86/entry/entry_64.S | 18 -- 1 file changed, 4 insertions(+), 14 deletions(-) --- a/arch/x86/entry/entry_64.S +++ b/arch/x86/entry/entry_64.S @@ -933,7 +933,7 @@ ENTRY(\sym) call\do_sym - jmp error_exit /* %ebx: no swapgs flag */ + jmp error_exit .endif END(\sym) .endm @@ -1166,7 +1166,6 @@ END(paranoid_exit) /* * Save all registers in pt_regs, and switch GS if needed. - * Return: EBX=0: came from user mode; EBX=1: otherwise */ ENTRY(error_entry) UNWIND_HINT_FUNC @@ -1213,7 +1212,6 @@ ENTRY(error_entry) * for these here too. */ .Lerror_kernelspace: - incl%ebx leaqnative_irq_return_iret(%rip), %rcx cmpq%rcx, RIP+8(%rsp) je .Lerror_bad_iret @@ -1247,28 +1245,20 @@ ENTRY(error_entry) /* * Pretend that the exception came from user mode: set up pt_regs -* as if we faulted immediately after IRET and clear EBX so that -* error_exit knows that we will be returning to user mode. +* as if we faulted immediately after IRET. */ mov %rsp, %rdi callfixup_bad_iret mov %rax, %rsp - decl%ebx jmp .Lerror_entry_from_usermode_after_swapgs END(error_entry) - -/* - * On entry, EBX is a "return to kernel mode" flag: - * 1: already in kernel mode, don't need SWAPGS - * 0: user gsbase is loaded, we need SWAPGS and standard preparation for return to usermode - */ ENTRY(error_exit) UNWIND_HINT_REGS DISABLE_INTERRUPTS(CLBR_ANY) TRACE_IRQS_OFF - testl %ebx, %ebx - jnz retint_kernel + testb $3, CS(%rsp) + jz retint_kernel jmp retint_user END(error_exit) Patches currently in stable-queue which might be from l...@kernel.org are queue-4.14/x86-entry-
Re: [Xen-devel] [PATCH] x86/entry/64: Remove %ebx handling from error_entry/exit
On Mon, Jul 23, 2018 at 12:25 AM, Greg KH wrote: > On Sun, Jul 22, 2018 at 11:05:09AM -0700, Andy Lutomirski wrote: >> error_entry and error_exit communicate the user vs kernel status of >> the frame using %ebx. This is unnecessary -- the information is in >> regs->cs. Just use regs->cs. >> >> This makes error_entry simpler and makes error_exit more robust. >> >> It also fixes a nasty bug. Before all the Spectre nonsense, The >> xen_failsafe_callback entry point returned like this: >> >> ALLOC_PT_GPREGS_ON_STACK >> SAVE_C_REGS >> SAVE_EXTRA_REGS >> ENCODE_FRAME_POINTER >> jmp error_exit >> >> And it did not go through error_entry. This was bogus: RBX >> contained garbage, and error_exit expected a flag in RBX. >> Fortunately, it generally contained *nonzero* garbage, so the >> correct code path was used. As part of the Spectre fixes, code was >> added to clear RBX to mitigate certain speculation attacks. Now, >> depending on kernel configuration, RBX got zeroed and, when running >> some Wine workloads, the kernel crashes. This was introduced by: >> >> commit 3ac6d8c787b8 ("x86/entry/64: Clear registers for >> exceptions/interrupts, to reduce speculation attack surface") >> >> With this patch applied, RBX is no longer needed as a flag, and the >> problem goes away. >> >> I suspect that malicious userspace could use this bug to crash the >> kernel even without the offending patch applied, though. >> >> [Historical note: I wrote this patch as a cleanup before I was aware >> of the bug it fixed.] >> >> [Note to stable maintainers: this should probably get applied to all >> kernels. If you're nervous about that, a more conservative fix to >> add xorl %ebx,%ebx; incl %ebx before the jump to error_exit should >> also fix the problem.] >> >> Cc: Brian Gerst >> Cc: Borislav Petkov >> Cc: Dominik Brodowski >> Cc: Ingo Molnar >> Cc: "H. Peter Anvin" >> Cc: Thomas Gleixner >> Cc: Boris Ostrovsky >> Cc: Juergen Gross >> Cc: xen-devel@lists.xenproject.org >> Cc: x...@kernel.org >> Cc: sta...@vger.kernel.org >> Fixes: 3ac6d8c787b8 ("x86/entry/64: Clear registers for >> exceptions/interrupts, to reduce speculation attack surface") >> Reported-and-tested-by: "M. Vefa Bicakci" >> Signed-off-by: Andy Lutomirski >> --- >> >> I could also submit the conservative fix tagged for -stable and respin >> this on top of it. Ingo, Greg, what do you prefer? > > I don't care, this patch looks good to me to take as-is for the stable > trees. If you trust it in Linus's tree, it should be fine for others :) > My concern is more that something may work differently in older kernels and there might be some subtle issue. I'd be surprised, but still. ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] x86/entry/64: Remove %ebx handling from error_entry/exit
On Sun, Jul 22, 2018 at 11:05:09AM -0700, Andy Lutomirski wrote: > error_entry and error_exit communicate the user vs kernel status of > the frame using %ebx. This is unnecessary -- the information is in > regs->cs. Just use regs->cs. > > This makes error_entry simpler and makes error_exit more robust. > > It also fixes a nasty bug. Before all the Spectre nonsense, The > xen_failsafe_callback entry point returned like this: > > ALLOC_PT_GPREGS_ON_STACK > SAVE_C_REGS > SAVE_EXTRA_REGS > ENCODE_FRAME_POINTER > jmp error_exit > > And it did not go through error_entry. This was bogus: RBX > contained garbage, and error_exit expected a flag in RBX. > Fortunately, it generally contained *nonzero* garbage, so the > correct code path was used. As part of the Spectre fixes, code was > added to clear RBX to mitigate certain speculation attacks. Now, > depending on kernel configuration, RBX got zeroed and, when running > some Wine workloads, the kernel crashes. This was introduced by: > > commit 3ac6d8c787b8 ("x86/entry/64: Clear registers for > exceptions/interrupts, to reduce speculation attack surface") > > With this patch applied, RBX is no longer needed as a flag, and the > problem goes away. > > I suspect that malicious userspace could use this bug to crash the > kernel even without the offending patch applied, though. > > [Historical note: I wrote this patch as a cleanup before I was aware > of the bug it fixed.] > > [Note to stable maintainers: this should probably get applied to all > kernels. If you're nervous about that, a more conservative fix to > add xorl %ebx,%ebx; incl %ebx before the jump to error_exit should > also fix the problem.] > > Cc: Brian Gerst > Cc: Borislav Petkov > Cc: Dominik Brodowski > Cc: Ingo Molnar > Cc: "H. Peter Anvin" > Cc: Thomas Gleixner > Cc: Boris Ostrovsky > Cc: Juergen Gross > Cc: xen-devel@lists.xenproject.org > Cc: x...@kernel.org > Cc: sta...@vger.kernel.org > Fixes: 3ac6d8c787b8 ("x86/entry/64: Clear registers for > exceptions/interrupts, to reduce speculation attack surface") > Reported-and-tested-by: "M. Vefa Bicakci" > Signed-off-by: Andy Lutomirski > --- > > I could also submit the conservative fix tagged for -stable and respin > this on top of it. Ingo, Greg, what do you prefer? I don't care, this patch looks good to me to take as-is for the stable trees. If you trust it in Linus's tree, it should be fine for others :) thanks, greg k-h ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH] x86/entry/64: Remove %ebx handling from error_entry/exit
error_entry and error_exit communicate the user vs kernel status of the frame using %ebx. This is unnecessary -- the information is in regs->cs. Just use regs->cs. This makes error_entry simpler and makes error_exit more robust. It also fixes a nasty bug. Before all the Spectre nonsense, The xen_failsafe_callback entry point returned like this: ALLOC_PT_GPREGS_ON_STACK SAVE_C_REGS SAVE_EXTRA_REGS ENCODE_FRAME_POINTER jmp error_exit And it did not go through error_entry. This was bogus: RBX contained garbage, and error_exit expected a flag in RBX. Fortunately, it generally contained *nonzero* garbage, so the correct code path was used. As part of the Spectre fixes, code was added to clear RBX to mitigate certain speculation attacks. Now, depending on kernel configuration, RBX got zeroed and, when running some Wine workloads, the kernel crashes. This was introduced by: commit 3ac6d8c787b8 ("x86/entry/64: Clear registers for exceptions/interrupts, to reduce speculation attack surface") With this patch applied, RBX is no longer needed as a flag, and the problem goes away. I suspect that malicious userspace could use this bug to crash the kernel even without the offending patch applied, though. [Historical note: I wrote this patch as a cleanup before I was aware of the bug it fixed.] [Note to stable maintainers: this should probably get applied to all kernels. If you're nervous about that, a more conservative fix to add xorl %ebx,%ebx; incl %ebx before the jump to error_exit should also fix the problem.] Cc: Brian Gerst Cc: Borislav Petkov Cc: Dominik Brodowski Cc: Ingo Molnar Cc: "H. Peter Anvin" Cc: Thomas Gleixner Cc: Boris Ostrovsky Cc: Juergen Gross Cc: xen-devel@lists.xenproject.org Cc: x...@kernel.org Cc: sta...@vger.kernel.org Fixes: 3ac6d8c787b8 ("x86/entry/64: Clear registers for exceptions/interrupts, to reduce speculation attack surface") Reported-and-tested-by: "M. Vefa Bicakci" Signed-off-by: Andy Lutomirski --- I could also submit the conservative fix tagged for -stable and respin this on top of it. Ingo, Greg, what do you prefer? arch/x86/entry/entry_64.S | 18 -- 1 file changed, 4 insertions(+), 14 deletions(-) diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S index 73a522d53b53..8ae7ffda8f98 100644 --- a/arch/x86/entry/entry_64.S +++ b/arch/x86/entry/entry_64.S @@ -981,7 +981,7 @@ ENTRY(\sym) call\do_sym - jmp error_exit /* %ebx: no swapgs flag */ + jmp error_exit .endif END(\sym) .endm @@ -1222,7 +1222,6 @@ END(paranoid_exit) /* * Save all registers in pt_regs, and switch GS if needed. - * Return: EBX=0: came from user mode; EBX=1: otherwise */ ENTRY(error_entry) UNWIND_HINT_FUNC @@ -1269,7 +1268,6 @@ ENTRY(error_entry) * for these here too. */ .Lerror_kernelspace: - incl%ebx leaqnative_irq_return_iret(%rip), %rcx cmpq%rcx, RIP+8(%rsp) je .Lerror_bad_iret @@ -1303,28 +1301,20 @@ ENTRY(error_entry) /* * Pretend that the exception came from user mode: set up pt_regs -* as if we faulted immediately after IRET and clear EBX so that -* error_exit knows that we will be returning to user mode. +* as if we faulted immediately after IRET. */ mov %rsp, %rdi callfixup_bad_iret mov %rax, %rsp - decl%ebx jmp .Lerror_entry_from_usermode_after_swapgs END(error_entry) - -/* - * On entry, EBX is a "return to kernel mode" flag: - * 1: already in kernel mode, don't need SWAPGS - * 0: user gsbase is loaded, we need SWAPGS and standard preparation for return to usermode - */ ENTRY(error_exit) UNWIND_HINT_REGS DISABLE_INTERRUPTS(CLBR_ANY) TRACE_IRQS_OFF - testl %ebx, %ebx - jnz retint_kernel + testb $3, CS(%rsp) + jz retint_kernel jmp retint_user END(error_exit) -- 2.17.1 ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel