Re: [patch] x86: fix ESP corruption CPU bug (take 2)

Re: [patch] x86: fix ESP corruption CPU bug (take 2)

[Jan Beulich]

Stas, Petr,

I know it's been a while since this was discussed and integrated into
mainline, but I just now came across this, and following all of the
original discussion that I was able to locate I didn't see any mention
of a potential different approach to solving the problem which, as it
would appear to me, requires much less code changes: Instead of
allocating a separate stack to set intermediately, the 16-bit stack
segment could be mapped directly onto the normal, flat kernel stack,
thus requiring only an in-place stack change (without any copying of
contents) and an adjustment to the __switch_to code to change the base
address of that segment. Since this approach seems fairly natural I'm
sure it was actually considered, and I'd be curious to learn why it
wasn't chosen.

Thanks, Jan

Re: [patch] x86: fix ESP corruption CPU bug (take 2)

[Stas Sergeev]

Hello.

Jan Beulich wrote:
> mainline, but I just now came across this, and following all of the
> original discussion that I was able to locate I didn't see any mention
> of a potential different approach to solving the problem which, as it
There were at least 3 fundamentally different
approaches proposed by different people, I implemented
and posted all of them. At least 2 approaches were
publically discussed. Both did what you say. The
one that didn't, wasn't publically discussed either
(the one that ended up in 2.6.12, in fact).
So it is a bit odd that you weren't able to find
the code that does what you say. :)

> would appear to me, requires much less code changes: Instead of
> allocating a separate stack to set intermediately, the 16-bit stack
> segment could be mapped directly onto the normal, flat kernel stack,
Do you mean, eg, this?
http://www.ussg.iu.edu/hypermail/linux/kernel/0409.2/1533.html

Relevant quote:
---
> ring1 stacks must be per-CPU.
I allocate it on a ring0 stack. Noone seem to
suggest that. Is this flawed for some reasons?
---

Re: [patch] x86: fix ESP corruption CPU bug (take 2)

[Jan Beulich]

>>> Stas Sergeev <stsp@aknet.ru> 11.09.05 16:02:24 >>>
>Jan Beulich wrote:
>> mainline, but I just now came across this, and following all of the
>> original discussion that I was able to locate I didn't see any
mention
>> of a potential different approach to solving the problem which, as
it
>There were at least 3 fundamentally different
>approaches proposed by different people, I implemented
>and posted all of them. At least 2 approaches were
>publically discussed. Both did what you say. The
>one that didn't, wasn't publically discussed either
>(the one that ended up in 2.6.12, in fact).
>So it is a bit odd that you weren't able to find
>the code that does what you say. :)
>
>> would appear to me, requires much less code changes: Instead of
>> allocating a separate stack to set intermediately, the 16-bit stack
>> segment could be mapped directly onto the normal, flat kernel
stack,
>Do you mean, eg, this?
>http://www.ussg.iu.edu/hypermail/linux/kernel/0409.2/1533.html 

No, I don't. This talks about going through ring 1 intermediately,
which isn't what I have in mind.

>Relevant quote:
>---
>> ring1 stacks must be per-CPU.
>I allocate it on a ring0 stack. Noone seem to
>suggest that. Is this flawed for some reasons?
>---

Jan

Re: [patch] x86: fix ESP corruption CPU bug (take 2)

[Stas Sergeev]

Hi.

Jan Beulich wrote:
>>Do you mean, eg, this?
>>http://www.ussg.iu.edu/hypermail/linux/kernel/0409.2/1533.html 
> No, I don't. This talks about going through ring 1 intermediately,
> which isn't what I have in mind.
Well, like I said, 2 approaches do use the
kernel stack for the 16bit stack. One approach
uses ring-1 trampoline, the other one doesn't.
The posting I pointed to, was explicit about
the stack usage, but as for the ring-0 approach
while still using the kernel stack - here it is:
http://www.ussg.iu.edu/hypermail/linux/kernel/0410.0/1402.html

Is this what you mean? This is pretty much all
about it, the third approach is in the kernel,
and there were no more, even under discussion.

Re: [patch] x86: fix ESP corruption CPU bug (take 2)

[Jan Beulich]

>>>Do you mean, eg, this?
>>>http://www.ussg.iu.edu/hypermail/linux/kernel/0409.2/1533.html 
>> No, I don't. This talks about going through ring 1 intermediately,
>> which isn't what I have in mind.
>Well, like I said, 2 approaches do use the
>kernel stack for the 16bit stack. One approach
>uses ring-1 trampoline, the other one doesn't.
>The posting I pointed to, was explicit about
>the stack usage, but as for the ring-0 approach
>while still using the kernel stack - here it is:
>http://www.ussg.iu.edu/hypermail/linux/kernel/0410.0/1402.html 
>
>Is this what you mean? This is pretty much all
>about it, the third approach is in the kernel,
>and there were no more, even under discussion.

Yes, this comes close. Still, I'm more interested to understand why
this approach was *not* chosen, which doesn't seem to be covered by any
of the (only two) followups.

Thanks again, Jan

Re: [patch] x86: fix ESP corruption CPU bug (take 2)

[Stas Sergeev]

Hi.

Jan Beulich wrote:
> Yes, this comes close. Still, I'm more interested to understand why
> this approach was *not* chosen, which doesn't seem to be covered by any
> of the (only two) followups.
Yes, that was discussed privately.
But I am very intrigued why are you asking.
I thought this problem is very specific to
the virtualizers, like dosemu, maybe wine or
vmware, but not much more. Could you please
clarify what exactly problem does this solve
for you? I'm just curious.

As for why the other approach was developed,
here are the main points that I can recall:
- Run-time GDT patching is UGLY.
- Switching back to the 32bit stack is extremely
tricky in an NMI handler. Proving that this will
work even when the exception handler is being
NMIed, was nearly impossible (though I think I
got that part right).
- Overall the patch was so fragile and hackish-looking,
that Linus complained and proposed the plan to
get rid of the most of fragility. That included
allocating a separate per-cpu stacks and rewriting
the fixups mostly in C. He also proposed a lot of
the optimizations to make the C-based patch as
fast as an asmish one. That all made the patch
overall much better (and much bigger, too) and
helped to fix the regressions quickly (there were
quite a few, but they were not the bugs in the
patch itself :)

[patch] x86: fix ESP corruption CPU bug

[Stas Sergeev]

[-- Attachment #1: Type: text/plain, Size: 1817 bytes --]

Hi Alan.

Attached patch works around the corruption
of the high word of the ESP register, which
is the official bug of x86 CPUs. The bug
triggers only when the one is using the
16bit stack segment, and is described here:
http://www.intel.com/design/intarch/specupdt/27287402.PDF

Patch helps running many apps under dosemu,
and, according to the comments found in
Wine sources, also helps Wine.

Also the patch closes the "information leak",
which is that the half of the kernel's %esp
value gets leaked to user-space.

The initial discussion about the problem
can be found here:
http://lkml.org/lkml/2004/9/16/254
On a later stages the development of that
patch was driven mainly by Linus, but it
was in private.

This patch adds only 6 instructions for
the fast kernel-->user return path, 6
instructions on an exception path and
5 instructions on an NMI path. All the
rest of the patch gets executed only
for the two apps in that world, namely,
dosemu and wine (and VMWare?).
That's why it should be relatevely easy
to make sure the patch doesn't do any
harm for the normal apps, and so it is
safe, and probably fits into an -ac tree.

How it works:
- Allocates the per-cpu data for 16bit
stacks and sets the per-cpu GDT entries
for them.
- On a return to userspace, checks whether
the SS is from LDT and is 16 bit.
- If it is, the iret frame gets copied
to the 16bit per-cpu stack and stack gets
switched to the 16bit one, while the
higher word of %esp gets preloaded with
the proper value.
- On an exceptions the check is performed
to see if we are on a 16bit stack, and
if we are - switch to the 32bit one.


Alan, would it be possible to apply that
patch to an -ac tree?


Acked-by: Linus Torvalds <torvalds@osdl.org>
Acked-by: Petr Vandrovec <vandrove@vc.cvut.cz>
Signed-off-by: Stas Sergeev <stsp@aknet.ru>


[-- Attachment #2: linux-2.6.11-ac2-stk7.diff --]
[-- Type: text/x-patch, Size: 11163 bytes --]

diff -ur linux-2.6.11-ac2/arch/i386/kernel/cpu/common.c linux-2.6.11-ac2-stk/arch/i386/kernel/cpu/common.c
--- linux-2.6.11-ac2/arch/i386/kernel/cpu/common.c	2005-03-13 17:59:45.000000000 +0300
+++ linux-2.6.11-ac2-stk/arch/i386/kernel/cpu/common.c	2005-03-13 18:17:47.000000000 +0300
@@ -21,6 +21,10 @@
 DEFINE_PER_CPU(struct desc_struct, cpu_gdt_table[GDT_ENTRIES]);
 EXPORT_PER_CPU_SYMBOL(cpu_gdt_table);
 
+unsigned char cpu_16bit_stack[CPU_16BIT_STACK_SIZE];
+DEFINE_PER_CPU(unsigned char, cpu_16bit_stack[CPU_16BIT_STACK_SIZE]);
+EXPORT_PER_CPU_SYMBOL(cpu_16bit_stack);
+
 static int cachesize_override __initdata = -1;
 static int disable_x86_fxsr __initdata = 0;
 static int disable_x86_serial_nr __initdata = 1;
@@ -557,6 +561,7 @@
 	int cpu = smp_processor_id();
 	struct tss_struct * t = &per_cpu(init_tss, cpu);
 	struct thread_struct *thread = &current->thread;
+	__u32 stk16_off = (__u32)&per_cpu(cpu_16bit_stack, cpu);
 
 	if (cpu_test_and_set(cpu, cpu_initialized)) {
 		printk(KERN_WARNING "CPU#%d already initialized!\n", cpu);
@@ -579,6 +584,13 @@
 	 */
 	memcpy(&per_cpu(cpu_gdt_table, cpu), cpu_gdt_table,
 	       GDT_SIZE);
+
+	/* Set up GDT entry for 16bit stack */
+	*(__u64 *)&(per_cpu(cpu_gdt_table, cpu)[GDT_ENTRY_ESPFIX_SS]) |=
+		((((__u64)stk16_off) << 16) & 0x000000ffffff0000ULL) |
+		((((__u64)stk16_off) << 32) & 0xff00000000000000ULL) |
+		(CPU_16BIT_STACK_SIZE - 1);
+
 	cpu_gdt_descr[cpu].size = GDT_SIZE - 1;
 	cpu_gdt_descr[cpu].address =
 	    (unsigned long)&per_cpu(cpu_gdt_table, cpu);
diff -ur linux-2.6.11-ac2/arch/i386/kernel/entry.S linux-2.6.11-ac2-stk/arch/i386/kernel/entry.S
--- linux-2.6.11-ac2/arch/i386/kernel/entry.S	2005-03-13 17:59:45.000000000 +0300
+++ linux-2.6.11-ac2-stk/arch/i386/kernel/entry.S	2005-03-13 18:17:47.000000000 +0300
@@ -47,6 +47,7 @@
 #include <asm/segment.h>
 #include <asm/smp.h>
 #include <asm/page.h>
+#include <asm/desc.h>
 #include "irq_vectors.h"
 
 #define nr_syscalls ((syscall_table_size)/4)
@@ -78,7 +79,7 @@
 #define preempt_stop		cli
 #else
 #define preempt_stop
-#define resume_kernel		restore_all
+#define resume_kernel		restore_nocheck
 #endif
 
 #define SAVE_ALL \
@@ -122,24 +123,6 @@
 .previous
 
 
-#define RESTORE_ALL	\
-	RESTORE_REGS	\
-	addl $4, %esp;	\
-1:	iret;		\
-.section .fixup,"ax";   \
-2:	sti;		\
-	movl $(__USER_DS), %edx; \
-	movl %edx, %ds; \
-	movl %edx, %es; \
-	movl $11,%eax;	\
-	call do_exit;	\
-.previous;		\
-.section __ex_table,"a";\
-	.align 4;	\
-	.long 1b,2b;	\
-.previous
-
-
 ENTRY(ret_from_fork)
 	pushl %eax
 	call schedule_tail
@@ -163,7 +146,7 @@
 	movl EFLAGS(%esp), %eax		# mix EFLAGS and CS
 	movb CS(%esp), %al
 	testl $(VM_MASK | 3), %eax
-	jz resume_kernel		# returning to kernel or vm86-space
+	jz resume_kernel
 ENTRY(resume_userspace)
  	cli				# make sure we don't miss an interrupt
 					# setting need_resched or sigpending
@@ -178,7 +161,7 @@
 ENTRY(resume_kernel)
 	cli
 	cmpl $0,TI_preempt_count(%ebp)	# non-zero preempt_count ?
-	jnz restore_all
+	jnz restore_nocheck
 need_resched:
 	movl TI_flags(%ebp), %ecx	# need_resched set ?
 	testb $_TIF_NEED_RESCHED, %cl
@@ -189,6 +172,31 @@
 	jmp need_resched
 #endif
 
+ldt_ss:
+	larl OLDSS(%esp), %eax
+	jnz restore_nocheck
+	testl $0x00400000, %eax		# returning to 32bit stack?
+	jnz restore_nocheck		# allright, normal return
+	/* If returning to userspace with 16bit stack,
+	 * try to fix the higher word of ESP, as the CPU
+	 * won't restore it.
+	 * This is an "official" bug of all the x86-compatible
+	 * CPUs, which we can try to work around to make
+	 * dosemu and wine happy. */
+	subl $8, %esp		# reserve space for switch16 pointer
+	cli
+	movl %esp, %eax
+	/* Set up the 16bit stack frame with switch32 pointer on top,
+	 * and a switch16 pointer on top of the current frame. */
+	call setup_x86_bogus_stack
+	RESTORE_REGS
+	lss 20+4(%esp), %esp	# switch to 16bit stack
+1:	iret
+.section __ex_table,"a"
+	.align 4
+	.long 1b,iret_exc
+.previous
+
 /* SYSENTER_RETURN points to after the "sysenter" instruction in
    the vsyscall page.  See vsyscall-sysentry.S, which defines the symbol.  */
 
@@ -257,8 +265,31 @@
 	movl TI_flags(%ebp), %ecx
 	testw $_TIF_ALLWORK_MASK, %cx	# current->work
 	jne syscall_exit_work
+
 restore_all:
-	RESTORE_ALL
+	movl EFLAGS(%esp), %eax		# mix EFLAGS, SS and CS
+	movb OLDSS(%esp), %ah
+	movb CS(%esp), %al
+	andl $(VM_MASK | (4 << 8) | 3), %eax
+	cmpl $((4 << 8) | 3), %eax
+	je ldt_ss			# returning to user-space with LDT SS
+restore_nocheck:
+	RESTORE_REGS
+	addl $4, %esp
+1:	iret
+.section .fixup,"ax"
+iret_exc:
+	sti
+	movl $__USER_DS, %edx
+	movl %edx, %ds
+	movl %edx, %es
+	movl $11,%eax
+	call do_exit
+.previous
+.section __ex_table,"a"
+	.align 4
+	.long 1b,iret_exc
+.previous
 
 	# perform work that needs to be done immediately before resumption
 	ALIGN
@@ -334,6 +365,27 @@
 	movl $-ENOSYS,EAX(%esp)
 	jmp resume_userspace
 
+#define FIXUP_ESPFIX_STACK \
+	movl %esp, %eax; \
+	/* switch to 32bit stack using the pointer on top of 16bit stack */ \
+	lss %ss:CPU_16BIT_STACK_SIZE-8, %esp; \
+	/* copy data from 16bit stack to 32bit stack */ \
+	call fixup_x86_bogus_stack; \
+	/* put ESP to the proper location */ \
+	movl %eax, %esp;
+#define UNWIND_ESPFIX_STACK \
+	pushl %eax; \
+	movl %ss, %eax; \
+	/* see if on 16bit stack */ \
+	cmpw $__ESPFIX_SS, %ax; \
+	jne 28f; \
+	movl $__KERNEL_DS, %edx; \
+	movl %edx, %ds; \
+	movl %edx, %es; \
+	/* switch to 32bit stack */ \
+	FIXUP_ESPFIX_STACK \
+28:	popl %eax;
+
 /*
  * Build the entry stubs and pointer table with
  * some assembler magic.
@@ -388,7 +440,9 @@
 	pushl %ecx
 	pushl %ebx
 	cld
-	movl %es, %ecx
+	pushl %es
+	UNWIND_ESPFIX_STACK
+	popl %ecx
 	movl ES(%esp), %edi		# get the function address
 	movl ORIG_EAX(%esp), %edx	# get the error code
 	movl %eax, ORIG_EAX(%esp)
@@ -470,6 +524,11 @@
  * fault happened on the sysenter path.
  */
 ENTRY(nmi)
+	pushl %eax
+	movl %ss, %eax
+	cmpw $__ESPFIX_SS, %ax
+	popl %eax
+	je nmi_16bit_stack
 	cmpl $sysenter_entry,(%esp)
 	je nmi_stack_fixup
 	pushl %eax
@@ -489,7 +548,7 @@
 	xorl %edx,%edx		# zero error code
 	movl %esp,%eax		# pt_regs pointer
 	call do_nmi
-	RESTORE_ALL
+	jmp restore_all
 
 nmi_stack_fixup:
 	FIX_STACK(12,nmi_stack_correct, 1)
@@ -505,6 +564,29 @@
 	FIX_STACK(24,nmi_stack_correct, 1)
 	jmp nmi_stack_correct
 
+nmi_16bit_stack:
+	/* create the pointer to lss back */
+	pushl %ss
+	pushl %esp
+	movzwl %sp, %esp
+	addw $4, (%esp)
+	/* copy the iret frame of 12 bytes */
+	.rept 3
+	pushl 16(%esp)
+	.endr
+	pushl %eax
+	SAVE_ALL
+	FIXUP_ESPFIX_STACK		# %eax == %esp
+	xorl %edx,%edx			# zero error code
+	call do_nmi
+	RESTORE_REGS
+	lss 12+4(%esp), %esp		# back to 16bit stack
+1:	iret
+.section __ex_table,"a"
+	.align 4
+	.long 1b,iret_exc
+.previous
+
 ENTRY(int3)
 	pushl $-1			# mark this as an int
 	SAVE_ALL
diff -ur linux-2.6.11-ac2/arch/i386/kernel/head.S linux-2.6.11-ac2-stk/arch/i386/kernel/head.S
--- linux-2.6.11-ac2/arch/i386/kernel/head.S	2005-03-13 17:56:19.000000000 +0300
+++ linux-2.6.11-ac2-stk/arch/i386/kernel/head.S	2005-03-13 18:17:47.000000000 +0300
@@ -512,7 +512,7 @@
 	.quad 0x00009a0000000000	/* 0xc0 APM CS 16 code (16 bit) */
 	.quad 0x0040920000000000	/* 0xc8 APM DS    data */
 
-	.quad 0x0000000000000000	/* 0xd0 - unused */
+	.quad 0x0000920000000000	/* 0xd0 - ESPFIX 16-bit SS */
 	.quad 0x0000000000000000	/* 0xd8 - unused */
 	.quad 0x0000000000000000	/* 0xe0 - unused */
 	.quad 0x0000000000000000	/* 0xe8 - unused */
diff -ur linux-2.6.11-ac2/arch/i386/kernel/traps.c linux-2.6.11-ac2-stk/arch/i386/kernel/traps.c
--- linux-2.6.11-ac2/arch/i386/kernel/traps.c	2005-03-13 17:59:45.000000000 +0300
+++ linux-2.6.11-ac2-stk/arch/i386/kernel/traps.c	2005-03-13 18:17:47.000000000 +0300
@@ -895,6 +895,51 @@
 #endif
 }
 
+fastcall void setup_x86_bogus_stack(unsigned char * stk)
+{
+	unsigned long *switch16_ptr, *switch32_ptr;
+	struct pt_regs *regs;
+	unsigned long stack_top, stack_bot;
+	unsigned short iret_frame16_off;
+	int cpu = smp_processor_id();
+	/* reserve the space on 32bit stack for the magic switch16 pointer */
+	memmove(stk, stk + 8, sizeof(struct pt_regs));
+	switch16_ptr = (unsigned long *)(stk + sizeof(struct pt_regs));
+	regs = (struct pt_regs *)stk;
+	/* now the switch32 on 16bit stack */
+	stack_bot = (unsigned long)&per_cpu(cpu_16bit_stack, cpu);
+	stack_top = stack_bot +	CPU_16BIT_STACK_SIZE;
+	switch32_ptr = (unsigned long *)(stack_top - 8);
+	iret_frame16_off = CPU_16BIT_STACK_SIZE - 8 - 20;
+	/* copy iret frame on 16bit stack */
+	memcpy((void *)(stack_bot + iret_frame16_off), &regs->eip, 20);
+	/* fill in the switch pointers */
+	switch16_ptr[0] = (regs->esp & 0xffff0000) | iret_frame16_off;
+	switch16_ptr[1] = __ESPFIX_SS;
+	switch32_ptr[0] = (unsigned long)stk + sizeof(struct pt_regs) +
+		8 - CPU_16BIT_STACK_SIZE;
+	switch32_ptr[1] = __KERNEL_DS;
+}
+
+fastcall unsigned char * fixup_x86_bogus_stack(unsigned short sp)
+{
+	unsigned long *switch32_ptr;
+	unsigned char *stack16, *stack32;
+	unsigned long stack_top, stack_bot;
+	int len;
+	int cpu = smp_processor_id();
+	stack_bot = (unsigned long)&per_cpu(cpu_16bit_stack, cpu);
+	stack_top = stack_bot +	CPU_16BIT_STACK_SIZE;
+	switch32_ptr = (unsigned long *)(stack_top - 8);
+	/* copy the data from 16bit stack to 32bit stack */
+	len = CPU_16BIT_STACK_SIZE - 8 - sp;
+	stack16 = (unsigned char *)(stack_bot + sp);
+	stack32 = (unsigned char *)
+		(switch32_ptr[0] + CPU_16BIT_STACK_SIZE - 8 - len);
+	memcpy(stack32, stack16, len);
+	return stack32;
+}
+
 /*
  *  'math_state_restore()' saves the current math information in the
  * old math state array, and gets the new ones from the current task
diff -ur linux-2.6.11-ac2/include/asm-i386/desc.h linux-2.6.11-ac2-stk/include/asm-i386/desc.h
--- linux-2.6.11-ac2/include/asm-i386/desc.h	2005-03-13 17:57:12.000000000 +0300
+++ linux-2.6.11-ac2-stk/include/asm-i386/desc.h	2005-03-13 18:17:47.000000000 +0300
@@ -4,6 +4,8 @@
 #include <asm/ldt.h>
 #include <asm/segment.h>
 
+#define CPU_16BIT_STACK_SIZE 1024
+
 #ifndef __ASSEMBLY__
 
 #include <linux/preempt.h>
@@ -15,6 +17,9 @@
 extern struct desc_struct cpu_gdt_table[GDT_ENTRIES];
 DECLARE_PER_CPU(struct desc_struct, cpu_gdt_table[GDT_ENTRIES]);
 
+extern unsigned char cpu_16bit_stack[CPU_16BIT_STACK_SIZE];
+DECLARE_PER_CPU(unsigned char, cpu_16bit_stack[CPU_16BIT_STACK_SIZE]);
+
 struct Xgt_desc_struct {
 	unsigned short size;
 	unsigned long address __attribute__((packed));
diff -ur linux-2.6.11-ac2/include/asm-i386/segment.h linux-2.6.11-ac2-stk/include/asm-i386/segment.h
--- linux-2.6.11-ac2/include/asm-i386/segment.h	2005-01-04 03:59:37.000000000 +0300
+++ linux-2.6.11-ac2-stk/include/asm-i386/segment.h	2005-03-13 18:17:47.000000000 +0300
@@ -38,7 +38,7 @@
  *  24 - APM BIOS support
  *  25 - APM BIOS support 
  *
- *  26 - unused
+ *  26 - ESPFIX small SS
  *  27 - unused
  *  28 - unused
  *  29 - unused
@@ -71,6 +71,9 @@
 #define GDT_ENTRY_PNPBIOS_BASE		(GDT_ENTRY_KERNEL_BASE + 6)
 #define GDT_ENTRY_APMBIOS_BASE		(GDT_ENTRY_KERNEL_BASE + 11)
 
+#define GDT_ENTRY_ESPFIX_SS		(GDT_ENTRY_KERNEL_BASE + 14)
+#define __ESPFIX_SS (GDT_ENTRY_ESPFIX_SS * 8)
+
 #define GDT_ENTRY_DOUBLEFAULT_TSS	31
 
 /*

Re: [patch] x86: fix ESP corruption CPU bug

[Pavel Machek]

Hi!

> @@ -257,8 +265,31 @@
>  	movl TI_flags(%ebp), %ecx
>  	testw $_TIF_ALLWORK_MASK, %cx	# current->work
>  	jne syscall_exit_work
> +
>  restore_all:
> -	RESTORE_ALL
> +	movl EFLAGS(%esp), %eax		# mix EFLAGS, SS and CS
> +	movb OLDSS(%esp), %ah
> +	movb CS(%esp), %al
> +	andl $(VM_MASK | (4 << 8) | 3), %eax
> +	cmpl $((4 << 8) | 3), %eax
> +	je ldt_ss			# returning to user-space with LDT SS

All common linux apps use same %ss, no? Perhaps it would be more
efficient to just check if %ss == 0x7b, and proceed directly to
restore_nocheck if not?

Or perhaps we could only enable this code after application loads
custom ldt?

								Pavel

-- 
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!

Re: [patch] x86: fix ESP corruption CPU bug

[Stas Sergeev]

Hi.

Pavel Machek wrote:
>> +	andl $(VM_MASK | (4 << 8) | 3), %eax
>> +	cmpl $((4 << 8) | 3), %eax
>> +	je ldt_ss			# returning to user-space with LDT SS
> All common linux apps use same %ss, no? Perhaps it would be more
> efficient to just check if %ss == 0x7b, and proceed directly to
> restore_nocheck if not?
Such an optimization will cost three more
instructions, one of which is a "taken"
jump. It seems like the "taken" jump on
a fast path is not good, while now it is
only 5 instructions with a not-taken jump.
I am not sure here, but I think the current
solution is better (depends on how bad the
"taken" jump is, and how bad it is to have
the three extra insns for that optimization
purpose).

> Or perhaps we could only enable this code
> after application loads custom ldt?
The good thing here is that the code
actually does what you say, i.e. it jumps
to ldt_ss only when the app has loaded
the custom ldt and loaded that selector
to %ss. The way it is implemented, is
probably different from what you mean,
I assume you mean the new per-thread flag?
But I don't see how it can be more optimal,
i.e. you propose to check whether or not
the app altered the ldt (which can just be
the old glibc I think), while the current
solution is to also check whether it was
loaded to %ss (so the glibc case is avoided,
IIRC glibc used to load %gs with LDT selector).
I.e. since right now we jump to ldt_ss only
when the %ss is loaded with an LDT selector,
I think the extra checks are not needed.

Re: [patch] x86: fix ESP corruption CPU bug

[Linus Torvalds]

On Sun, 13 Mar 2005, Stas Sergeev wrote:
>
> Such an optimization will cost three more
> instructions, one of which is a "taken"
> jump.

I think Pavel missed the fact that you have to check the VM86 bit in
eflags before you check SS, since otherwise SS doesn't mean anything
special at all (ie checking for just the normal SS isn't correct: you
could have a 16-bit SS that looks normal, but is actually a vm86 segment).

Pavel: for the same reason you have to check the low two bits of CS too, 
since if they are zero, then SS hasn't been saved on the stack at all, so 
comparing it against some normal value is meaningless.

That said, the "ldt_ss" case should be moved _after_ the conditional
tests, since most CPU's out there will do static prediction based on
forward/backwards direction if the branch predictor isn't primed. And so
since it's an unusual case, the branch should be a forward branch, which
is usually the not-taken one (this depends on the core, of course, and you
could also add the prefix byte to mark it explicitly predict-not-taken for 
the newer cores that support it).

			Linus

[patch] x86: fix ESP corruption CPU bug (take 2)

[Stas Sergeev]

[-- Attachment #1: Type: text/plain, Size: 964 bytes --]

Hello.

Linus Torvalds wrote:
> I think Pavel missed the fact that you have to check the VM86 bit in
Ah, thanks, I must have forgotten the
essentials of the own patch :(

> That said, the "ldt_ss" case should be moved _after_ the conditional
> tests, since most CPU's out there will do static prediction based on<>
> forward/backwards direction if the branch predictor isn't primed.
OK, I moved the "ldt_ss" below the check.
The new patch is attached.


Alan, can you please apply that to an -ac
tree?

----
Summary:
Attached patch works around the corruption
of the high word of the ESP register, which
is the official bug of x86 CPUs. The bug
triggers only when the one is using the
16bit stack segment, and is described here:
http://www.intel.com/design/intarch/specupdt/27287402.PDF


Acked-by: Linus Torvalds <torvalds@osdl.org>
Acked-by: Petr Vandrovec <vandrove@vc.cvut.cz>
Acked-by: Pavel Machek <pavel@ucw.cz>
Signed-off-by: Stas Sergeev <stsp@aknet.ru>


[-- Attachment #2: linux-2.6.11-ac2-stk7a.diff --]
[-- Type: text/x-patch, Size: 10969 bytes --]

diff -ur linux-2.6.11-ac2/arch/i386/kernel/cpu/common.c linux-2.6.11-ac2-stk/arch/i386/kernel/cpu/common.c
--- linux-2.6.11-ac2/arch/i386/kernel/cpu/common.c	2005-03-13 17:59:45.000000000 +0300
+++ linux-2.6.11-ac2-stk/arch/i386/kernel/cpu/common.c	2005-03-13 20:36:12.000000000 +0300
@@ -21,6 +21,10 @@
 DEFINE_PER_CPU(struct desc_struct, cpu_gdt_table[GDT_ENTRIES]);
 EXPORT_PER_CPU_SYMBOL(cpu_gdt_table);
 
+unsigned char cpu_16bit_stack[CPU_16BIT_STACK_SIZE];
+DEFINE_PER_CPU(unsigned char, cpu_16bit_stack[CPU_16BIT_STACK_SIZE]);
+EXPORT_PER_CPU_SYMBOL(cpu_16bit_stack);
+
 static int cachesize_override __initdata = -1;
 static int disable_x86_fxsr __initdata = 0;
 static int disable_x86_serial_nr __initdata = 1;
@@ -557,6 +561,7 @@
 	int cpu = smp_processor_id();
 	struct tss_struct * t = &per_cpu(init_tss, cpu);
 	struct thread_struct *thread = &current->thread;
+	__u32 stk16_off = (__u32)&per_cpu(cpu_16bit_stack, cpu);
 
 	if (cpu_test_and_set(cpu, cpu_initialized)) {
 		printk(KERN_WARNING "CPU#%d already initialized!\n", cpu);
@@ -579,6 +584,13 @@
 	 */
 	memcpy(&per_cpu(cpu_gdt_table, cpu), cpu_gdt_table,
 	       GDT_SIZE);
+
+	/* Set up GDT entry for 16bit stack */
+	*(__u64 *)&(per_cpu(cpu_gdt_table, cpu)[GDT_ENTRY_ESPFIX_SS]) |=
+		((((__u64)stk16_off) << 16) & 0x000000ffffff0000ULL) |
+		((((__u64)stk16_off) << 32) & 0xff00000000000000ULL) |
+		(CPU_16BIT_STACK_SIZE - 1);
+
 	cpu_gdt_descr[cpu].size = GDT_SIZE - 1;
 	cpu_gdt_descr[cpu].address =
 	    (unsigned long)&per_cpu(cpu_gdt_table, cpu);
diff -ur linux-2.6.11-ac2/arch/i386/kernel/entry.S linux-2.6.11-ac2-stk/arch/i386/kernel/entry.S
--- linux-2.6.11-ac2/arch/i386/kernel/entry.S	2005-03-13 17:59:45.000000000 +0300
+++ linux-2.6.11-ac2-stk/arch/i386/kernel/entry.S	2005-03-14 00:45:16.000000000 +0300
@@ -47,6 +47,7 @@
 #include <asm/segment.h>
 #include <asm/smp.h>
 #include <asm/page.h>
+#include <asm/desc.h>
 #include "irq_vectors.h"
 
 #define nr_syscalls ((syscall_table_size)/4)
@@ -78,7 +79,7 @@
 #define preempt_stop		cli
 #else
 #define preempt_stop
-#define resume_kernel		restore_all
+#define resume_kernel		restore_nocheck
 #endif
 
 #define SAVE_ALL \
@@ -122,24 +123,6 @@
 .previous
 
 
-#define RESTORE_ALL	\
-	RESTORE_REGS	\
-	addl $4, %esp;	\
-1:	iret;		\
-.section .fixup,"ax";   \
-2:	sti;		\
-	movl $(__USER_DS), %edx; \
-	movl %edx, %ds; \
-	movl %edx, %es; \
-	movl $11,%eax;	\
-	call do_exit;	\
-.previous;		\
-.section __ex_table,"a";\
-	.align 4;	\
-	.long 1b,2b;	\
-.previous
-
-
 ENTRY(ret_from_fork)
 	pushl %eax
 	call schedule_tail
@@ -163,7 +146,7 @@
 	movl EFLAGS(%esp), %eax		# mix EFLAGS and CS
 	movb CS(%esp), %al
 	testl $(VM_MASK | 3), %eax
-	jz resume_kernel		# returning to kernel or vm86-space
+	jz resume_kernel
 ENTRY(resume_userspace)
  	cli				# make sure we don't miss an interrupt
 					# setting need_resched or sigpending
@@ -178,7 +161,7 @@
 ENTRY(resume_kernel)
 	cli
 	cmpl $0,TI_preempt_count(%ebp)	# non-zero preempt_count ?
-	jnz restore_all
+	jnz restore_nocheck
 need_resched:
 	movl TI_flags(%ebp), %ecx	# need_resched set ?
 	testb $_TIF_NEED_RESCHED, %cl
@@ -257,8 +240,56 @@
 	movl TI_flags(%ebp), %ecx
 	testw $_TIF_ALLWORK_MASK, %cx	# current->work
 	jne syscall_exit_work
+
 restore_all:
-	RESTORE_ALL
+	movl EFLAGS(%esp), %eax		# mix EFLAGS, SS and CS
+	movb OLDSS(%esp), %ah
+	movb CS(%esp), %al
+	andl $(VM_MASK | (4 << 8) | 3), %eax
+	cmpl $((4 << 8) | 3), %eax
+	je ldt_ss			# returning to user-space with LDT SS
+restore_nocheck:
+	RESTORE_REGS
+	addl $4, %esp
+1:	iret
+.section .fixup,"ax"
+iret_exc:
+	sti
+	movl $__USER_DS, %edx
+	movl %edx, %ds
+	movl %edx, %es
+	movl $11,%eax
+	call do_exit
+.previous
+.section __ex_table,"a"
+	.align 4
+	.long 1b,iret_exc
+.previous
+
+ldt_ss:
+	larl OLDSS(%esp), %eax
+	jnz restore_nocheck
+	testl $0x00400000, %eax		# returning to 32bit stack?
+	jnz restore_nocheck		# allright, normal return
+	/* If returning to userspace with 16bit stack,
+	 * try to fix the higher word of ESP, as the CPU
+	 * won't restore it.
+	 * This is an "official" bug of all the x86-compatible
+	 * CPUs, which we can try to work around to make
+	 * dosemu and wine happy. */
+	subl $8, %esp		# reserve space for switch16 pointer
+	cli
+	movl %esp, %eax
+	/* Set up the 16bit stack frame with switch32 pointer on top,
+	 * and a switch16 pointer on top of the current frame. */
+	call setup_x86_bogus_stack
+	RESTORE_REGS
+	lss 20+4(%esp), %esp	# switch to 16bit stack
+1:	iret
+.section __ex_table,"a"
+	.align 4
+	.long 1b,iret_exc
+.previous
 
 	# perform work that needs to be done immediately before resumption
 	ALIGN
@@ -334,6 +365,27 @@
 	movl $-ENOSYS,EAX(%esp)
 	jmp resume_userspace
 
+#define FIXUP_ESPFIX_STACK \
+	movl %esp, %eax; \
+	/* switch to 32bit stack using the pointer on top of 16bit stack */ \
+	lss %ss:CPU_16BIT_STACK_SIZE-8, %esp; \
+	/* copy data from 16bit stack to 32bit stack */ \
+	call fixup_x86_bogus_stack; \
+	/* put ESP to the proper location */ \
+	movl %eax, %esp;
+#define UNWIND_ESPFIX_STACK \
+	pushl %eax; \
+	movl %ss, %eax; \
+	/* see if on 16bit stack */ \
+	cmpw $__ESPFIX_SS, %ax; \
+	jne 28f; \
+	movl $__KERNEL_DS, %edx; \
+	movl %edx, %ds; \
+	movl %edx, %es; \
+	/* switch to 32bit stack */ \
+	FIXUP_ESPFIX_STACK \
+28:	popl %eax;
+
 /*
  * Build the entry stubs and pointer table with
  * some assembler magic.
@@ -388,7 +440,9 @@
 	pushl %ecx
 	pushl %ebx
 	cld
-	movl %es, %ecx
+	pushl %es
+	UNWIND_ESPFIX_STACK
+	popl %ecx
 	movl ES(%esp), %edi		# get the function address
 	movl ORIG_EAX(%esp), %edx	# get the error code
 	movl %eax, ORIG_EAX(%esp)
@@ -470,6 +524,11 @@
  * fault happened on the sysenter path.
  */
 ENTRY(nmi)
+	pushl %eax
+	movl %ss, %eax
+	cmpw $__ESPFIX_SS, %ax
+	popl %eax
+	je nmi_16bit_stack
 	cmpl $sysenter_entry,(%esp)
 	je nmi_stack_fixup
 	pushl %eax
@@ -489,7 +548,7 @@
 	xorl %edx,%edx		# zero error code
 	movl %esp,%eax		# pt_regs pointer
 	call do_nmi
-	RESTORE_ALL
+	jmp restore_all
 
 nmi_stack_fixup:
 	FIX_STACK(12,nmi_stack_correct, 1)
@@ -505,6 +564,29 @@
 	FIX_STACK(24,nmi_stack_correct, 1)
 	jmp nmi_stack_correct
 
+nmi_16bit_stack:
+	/* create the pointer to lss back */
+	pushl %ss
+	pushl %esp
+	movzwl %sp, %esp
+	addw $4, (%esp)
+	/* copy the iret frame of 12 bytes */
+	.rept 3
+	pushl 16(%esp)
+	.endr
+	pushl %eax
+	SAVE_ALL
+	FIXUP_ESPFIX_STACK		# %eax == %esp
+	xorl %edx,%edx			# zero error code
+	call do_nmi
+	RESTORE_REGS
+	lss 12+4(%esp), %esp		# back to 16bit stack
+1:	iret
+.section __ex_table,"a"
+	.align 4
+	.long 1b,iret_exc
+.previous
+
 ENTRY(int3)
 	pushl $-1			# mark this as an int
 	SAVE_ALL
diff -ur linux-2.6.11-ac2/arch/i386/kernel/head.S linux-2.6.11-ac2-stk/arch/i386/kernel/head.S
--- linux-2.6.11-ac2/arch/i386/kernel/head.S	2005-03-13 17:56:19.000000000 +0300
+++ linux-2.6.11-ac2-stk/arch/i386/kernel/head.S	2005-03-13 20:36:12.000000000 +0300
@@ -512,7 +512,7 @@
 	.quad 0x00009a0000000000	/* 0xc0 APM CS 16 code (16 bit) */
 	.quad 0x0040920000000000	/* 0xc8 APM DS    data */
 
-	.quad 0x0000000000000000	/* 0xd0 - unused */
+	.quad 0x0000920000000000	/* 0xd0 - ESPFIX 16-bit SS */
 	.quad 0x0000000000000000	/* 0xd8 - unused */
 	.quad 0x0000000000000000	/* 0xe0 - unused */
 	.quad 0x0000000000000000	/* 0xe8 - unused */
diff -ur linux-2.6.11-ac2/arch/i386/kernel/traps.c linux-2.6.11-ac2-stk/arch/i386/kernel/traps.c
--- linux-2.6.11-ac2/arch/i386/kernel/traps.c	2005-03-13 17:59:45.000000000 +0300
+++ linux-2.6.11-ac2-stk/arch/i386/kernel/traps.c	2005-03-13 20:36:12.000000000 +0300
@@ -895,6 +895,51 @@
 #endif
 }
 
+fastcall void setup_x86_bogus_stack(unsigned char * stk)
+{
+	unsigned long *switch16_ptr, *switch32_ptr;
+	struct pt_regs *regs;
+	unsigned long stack_top, stack_bot;
+	unsigned short iret_frame16_off;
+	int cpu = smp_processor_id();
+	/* reserve the space on 32bit stack for the magic switch16 pointer */
+	memmove(stk, stk + 8, sizeof(struct pt_regs));
+	switch16_ptr = (unsigned long *)(stk + sizeof(struct pt_regs));
+	regs = (struct pt_regs *)stk;
+	/* now the switch32 on 16bit stack */
+	stack_bot = (unsigned long)&per_cpu(cpu_16bit_stack, cpu);
+	stack_top = stack_bot +	CPU_16BIT_STACK_SIZE;
+	switch32_ptr = (unsigned long *)(stack_top - 8);
+	iret_frame16_off = CPU_16BIT_STACK_SIZE - 8 - 20;
+	/* copy iret frame on 16bit stack */
+	memcpy((void *)(stack_bot + iret_frame16_off), &regs->eip, 20);
+	/* fill in the switch pointers */
+	switch16_ptr[0] = (regs->esp & 0xffff0000) | iret_frame16_off;
+	switch16_ptr[1] = __ESPFIX_SS;
+	switch32_ptr[0] = (unsigned long)stk + sizeof(struct pt_regs) +
+		8 - CPU_16BIT_STACK_SIZE;
+	switch32_ptr[1] = __KERNEL_DS;
+}
+
+fastcall unsigned char * fixup_x86_bogus_stack(unsigned short sp)
+{
+	unsigned long *switch32_ptr;
+	unsigned char *stack16, *stack32;
+	unsigned long stack_top, stack_bot;
+	int len;
+	int cpu = smp_processor_id();
+	stack_bot = (unsigned long)&per_cpu(cpu_16bit_stack, cpu);
+	stack_top = stack_bot +	CPU_16BIT_STACK_SIZE;
+	switch32_ptr = (unsigned long *)(stack_top - 8);
+	/* copy the data from 16bit stack to 32bit stack */
+	len = CPU_16BIT_STACK_SIZE - 8 - sp;
+	stack16 = (unsigned char *)(stack_bot + sp);
+	stack32 = (unsigned char *)
+		(switch32_ptr[0] + CPU_16BIT_STACK_SIZE - 8 - len);
+	memcpy(stack32, stack16, len);
+	return stack32;
+}
+
 /*
  *  'math_state_restore()' saves the current math information in the
  * old math state array, and gets the new ones from the current task
diff -ur linux-2.6.11-ac2/include/asm-i386/desc.h linux-2.6.11-ac2-stk/include/asm-i386/desc.h
--- linux-2.6.11-ac2/include/asm-i386/desc.h	2005-03-13 17:57:12.000000000 +0300
+++ linux-2.6.11-ac2-stk/include/asm-i386/desc.h	2005-03-13 20:36:12.000000000 +0300
@@ -4,6 +4,8 @@
 #include <asm/ldt.h>
 #include <asm/segment.h>
 
+#define CPU_16BIT_STACK_SIZE 1024
+
 #ifndef __ASSEMBLY__
 
 #include <linux/preempt.h>
@@ -15,6 +17,9 @@
 extern struct desc_struct cpu_gdt_table[GDT_ENTRIES];
 DECLARE_PER_CPU(struct desc_struct, cpu_gdt_table[GDT_ENTRIES]);
 
+extern unsigned char cpu_16bit_stack[CPU_16BIT_STACK_SIZE];
+DECLARE_PER_CPU(unsigned char, cpu_16bit_stack[CPU_16BIT_STACK_SIZE]);
+
 struct Xgt_desc_struct {
 	unsigned short size;
 	unsigned long address __attribute__((packed));
diff -ur linux-2.6.11-ac2/include/asm-i386/segment.h linux-2.6.11-ac2-stk/include/asm-i386/segment.h
--- linux-2.6.11-ac2/include/asm-i386/segment.h	2005-01-04 03:59:37.000000000 +0300
+++ linux-2.6.11-ac2-stk/include/asm-i386/segment.h	2005-03-13 20:36:12.000000000 +0300
@@ -38,7 +38,7 @@
  *  24 - APM BIOS support
  *  25 - APM BIOS support 
  *
- *  26 - unused
+ *  26 - ESPFIX small SS
  *  27 - unused
  *  28 - unused
  *  29 - unused
@@ -71,6 +71,9 @@
 #define GDT_ENTRY_PNPBIOS_BASE		(GDT_ENTRY_KERNEL_BASE + 6)
 #define GDT_ENTRY_APMBIOS_BASE		(GDT_ENTRY_KERNEL_BASE + 11)
 
+#define GDT_ENTRY_ESPFIX_SS		(GDT_ENTRY_KERNEL_BASE + 14)
+#define __ESPFIX_SS (GDT_ENTRY_ESPFIX_SS * 8)
+
 #define GDT_ENTRY_DOUBLEFAULT_TSS	31
 
 /*

Re: [patch] x86: fix ESP corruption CPU bug (take 2)

[Alan Cox]

On Mon, Mar 14, 2005 at 01:06:36AM +0300, Stas Sergeev wrote:
> Alan, can you please apply that to an -ac
> tree?

Ask Andrew Morton as it belongs in the -mm tree

Re: [patch] x86: fix ESP corruption CPU bug (take 2)

[Stas Sergeev]

Hi!

Alan Cox wrote:
>> Alan, can you please apply that to an -ac
>> tree?
> Ask Andrew Morton as it belongs in the -mm tree
Actually I tried that already. Andrew
had nothing against that patch personally,
as well as Linus, but after all that didn't
work:
http://lkml.org/lkml/2005/1/3/260

So it can't be applied to -mm, and not
depending on the kgdb-ga patch allowed for
some extra optimization.
So I have to try the -ac as the last resort.
I realized you won't be pleased with that
too much. If you are confident it doesn't
fit the -ac tree by whatever means, I can
understand that. (In that case I'll much
appreciate the suggestion what other tree
should I try.)

Re: [patch] x86: fix ESP corruption CPU bug (take 2)

[Andrew Morton]

Stas Sergeev <stsp@aknet.ru> wrote:
>
> Alan Cox wrote:
>  >> Alan, can you please apply that to an -ac
>  >> tree?
>  > Ask Andrew Morton as it belongs in the -mm tree
>  Actually I tried that already.

I added this patch to -mm.

> Andrew
>  had nothing against that patch personally,
>  as well as Linus, but after all that didn't
>  work:
>  http://lkml.org/lkml/2005/1/3/260
> 
>  So it can't be applied to -mm, and not
>  depending on the kgdb-ga patch allowed for
>  some extra optimization.

The rule is:

- If the patch patches something which is in Linus's kernel, prepare a
  diff against Linus's latest kernel.

- If the patch patches something which is only in -mm, prepare a patch
  against -mm.

In this case, I merged the patch prior to the kgdb patch and then fixed
up the fallout.

(If that causes kgdb to break in non-obvious-to-me ways then I might come
calling "help".  We'll see)

Re: [patch] x86: fix ESP corruption CPU bug (take 2)

[Stas Sergeev]

Hello.

Andrew Morton wrote:
> I added this patch to -mm.
Many thanks!

Alan, sorry for bothering you with that.


> - If the patch patches something which is in Linus's kernel, prepare a
>   diff against Linus's latest kernel.
> - If the patch patches something which is only in -mm, prepare a patch
>   against -mm.
> In this case, I merged the patch prior to the kgdb patch and then fixed
> up the fallout.
> (If that causes kgdb to break in non-obvious-to-me ways then I might come
> calling "help".
Thanks for explanations. That's what I
tried to find in your "The perfect patch"
guidelines many times, but it is not there
in such details.