i386 entry.S problems

i386 entry.S problems

[Jan Beulich]

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

There appear to be two problems in i386's entry.S:

(1) With CONFIG_REGPARM, lcall7 and lcall27 did not work (they pass the
parameters to the actual handler procedure on the stack). While using
'asmlinkage' on the handler function would have seemed feasible (leaving
aside binary compatibility issues), it not really is because this is a
pointer to a function and gcc does not check that parameter passing
options match for function pointer assignments, thus making it
impossible to detect improper uses.

(2) In the NMI handler, the second of the checks for whether the
interrupted code was in the stack pointer checking code of the debug
fault/trap handler was reversed, leading to the respective fixup getting
applied whenever the interrupted code was at an address higher than the
debug fault/trap handler.

The patch attempts to address both.

Additionally it seems to me that the code early in the NMI handler that
supposedly checks whether the stack pointer is in range for the
subsequent accesses to the outer stack frame is broken, too. Namely, it
uses THREAD_SIZE to check a pointer that actually follows immediately
the TSS and is not even a single page in size, and is also not
THREAD_SIZE aligned. Thus this test is very likely to fail depending on
the exact address the individual CPU's init_tss ends up on (one may even
say guaranteed, since init_tss should in most configurations live
directly after idt_table and thus at 0x800 bytes into a page). I can't
immediately see a correct replacement for the existing code.

Jan

--- linux-2.6.8.1/arch/i386/kernel/entry.S.0	2004-08-14
12:55:09.000000000 +0200
+++ linux-2.6.8.1/arch/i386/kernel/entry.S	2004-09-24
15:25:13.968031736 +0200
@@ -147,7 +147,9 @@ ENTRY(lcall7)
 	pushl %eax
 	SAVE_ALL
 	movl %esp, %ebp
+#if !defined(CONFIG_REGPARM) || __GNUC__ < 3
 	pushl %ebp
+#endif
 	pushl $0x7
 do_lcall:
 	movl EIP(%ebp), %eax	# due to call gates, this is eflags, not
eip..
@@ -156,11 +158,18 @@ do_lcall:
 	movl %eax,EFLAGS(%ebp)	#
 	movl %edx,EIP(%ebp)	# Now we move them to their "normal"
places
 	movl %ecx,CS(%ebp)	#
-	GET_THREAD_INFO_WITH_ESP(%ebp)	# GET_THREAD_INFO
-	movl TI_exec_domain(%ebp), %edx	# Get the execution domain
-	call *EXEC_DOMAIN_handler(%edx)	# Call the handler for the
domain
-	addl $4, %esp
+#if defined(CONFIG_REGPARM) && __GNUC__ >= 3
 	popl %eax
+#endif
+	GET_THREAD_INFO_WITH_ESP(%ebp)	# GET_THREAD_INFO
+	movl TI_exec_domain(%ebp), %ecx	# Get the execution domain
+#if defined(CONFIG_REGPARM) && __GNUC__ >= 3
+	movl %esp, %edx
+#endif
+	call *EXEC_DOMAIN_handler(%ecx)	# Call the handler for the
domain
+#if !defined(CONFIG_REGPARM) || __GNUC__ < 3
+	addl $8, %esp
+#endif
 	jmp resume_userspace
 
 ENTRY(lcall27)
@@ -169,7 +178,9 @@ ENTRY(lcall27)
 	pushl %eax
 	SAVE_ALL
 	movl %esp, %ebp
+#if !defined(CONFIG_REGPARM) || __GNUC__ < 3
 	pushl %ebp
+#endif
 	pushl $0x27
 	jmp do_lcall
 
@@ -531,10 +542,10 @@ nmi_stack_fixup:
 nmi_debug_stack_check:
 	cmpw $__KERNEL_CS,16(%esp)
 	jne nmi_stack_correct
-	cmpl $debug - 1,(%esp)
-	jle nmi_stack_correct
+	cmpl $debug,(%esp)
+	jb nmi_stack_correct
 	cmpl $debug_esp_fix_insn,(%esp)
-	jle nmi_debug_stack_fixup
+	ja nmi_stack_correct
 nmi_debug_stack_fixup:
 	FIX_STACK(24,nmi_stack_correct, 1)
 	jmp nmi_stack_correct


[-- Attachment #2: linux-2.6.8.1-i386-entry.patch --]
[-- Type: application/octet-stream, Size: 1724 bytes --]

--- linux-2.6.8.1/arch/i386/kernel/entry.S.0	2004-08-14 12:55:09.000000000 +0200
+++ linux-2.6.8.1/arch/i386/kernel/entry.S	2004-09-24 15:25:13.968031736 +0200
@@ -147,7 +147,9 @@ ENTRY(lcall7)
 	pushl %eax
 	SAVE_ALL
 	movl %esp, %ebp
+#if !defined(CONFIG_REGPARM) || __GNUC__ < 3
 	pushl %ebp
+#endif
 	pushl $0x7
 do_lcall:
 	movl EIP(%ebp), %eax	# due to call gates, this is eflags, not eip..
@@ -156,11 +158,18 @@ do_lcall:
 	movl %eax,EFLAGS(%ebp)	#
 	movl %edx,EIP(%ebp)	# Now we move them to their "normal" places
 	movl %ecx,CS(%ebp)	#
-	GET_THREAD_INFO_WITH_ESP(%ebp)	# GET_THREAD_INFO
-	movl TI_exec_domain(%ebp), %edx	# Get the execution domain
-	call *EXEC_DOMAIN_handler(%edx)	# Call the handler for the domain
-	addl $4, %esp
+#if defined(CONFIG_REGPARM) && __GNUC__ >= 3
 	popl %eax
+#endif
+	GET_THREAD_INFO_WITH_ESP(%ebp)	# GET_THREAD_INFO
+	movl TI_exec_domain(%ebp), %ecx	# Get the execution domain
+#if defined(CONFIG_REGPARM) && __GNUC__ >= 3
+	movl %esp, %edx
+#endif
+	call *EXEC_DOMAIN_handler(%ecx)	# Call the handler for the domain
+#if !defined(CONFIG_REGPARM) || __GNUC__ < 3
+	addl $8, %esp
+#endif
 	jmp resume_userspace
 
 ENTRY(lcall27)
@@ -169,7 +178,9 @@ ENTRY(lcall27)
 	pushl %eax
 	SAVE_ALL
 	movl %esp, %ebp
+#if !defined(CONFIG_REGPARM) || __GNUC__ < 3
 	pushl %ebp
+#endif
 	pushl $0x27
 	jmp do_lcall
 
@@ -531,10 +542,10 @@ nmi_stack_fixup:
 nmi_debug_stack_check:
 	cmpw $__KERNEL_CS,16(%esp)
 	jne nmi_stack_correct
-	cmpl $debug - 1,(%esp)
-	jle nmi_stack_correct
+	cmpl $debug,(%esp)
+	jb nmi_stack_correct
 	cmpl $debug_esp_fix_insn,(%esp)
-	jle nmi_debug_stack_fixup
+	ja nmi_stack_correct
 nmi_debug_stack_fixup:
 	FIX_STACK(24,nmi_stack_correct, 1)
 	jmp nmi_stack_correct

Re: i386 entry.S problems

[Arjan van de Ven]

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

On Fri, 2004-09-24 at 16:12, Jan Beulich wrote:
> There appear to be two problems in i386's entry.S:
> 
> (1) With CONFIG_REGPARM, lcall7 and lcall27 did not work (they pass the
> parameters to the actual handler procedure on the stack).

I wonder why we still have the lcall7/lcall27 entry points in the
kernel; nothing can legitemately use them and in the last few years they
have only caused a few security issues. Can I ask why you didn't just
remove this code from the kernel ?

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: i386 entry.S problems

[Jan Beulich]

>>> Arjan van de Ven <arjanv@redhat.com> 24.09.04 16:57:08 >>>
>On Fri, 2004-09-24 at 16:12, Jan Beulich wrote:
>> There appear to be two problems in i386's entry.S:
>> 
>> (1) With CONFIG_REGPARM, lcall7 and lcall27 did not work (they pass
the
>> parameters to the actual handler procedure on the stack).
>
>I wonder why we still have the lcall7/lcall27 entry points in the
>kernel; nothing can legitemately use them and in the last few years
they
>have only caused a few security issues. Can I ask why you didn't just
>remove this code from the kernel ?

I wondered about this, too. But I assumed that somewhere something
might live that still uses it. Since I don't know (and I found the entry
useful for some other test I had to do), I didn't dare to...

Jan

Re: i386 entry.S problems

[Alan Cox]

On Gwe, 2004-09-24 at 15:57, Arjan van de Ven wrote:
> I wonder why we still have the lcall7/lcall27 entry points in the
> kernel; nothing can legitemately use them

Linuxabi and the xabi stuff used to use them, but I guess those entry
points belong in the xabi patch not in the kernel really.

Re: i386 entry.S problems

[Christoph Hellwig]

> +#if !defined(CONFIG_REGPARM) || __GNUC__ < 3
>  	pushl %ebp
> +#endif

CONFIG_REGPARM n eeds gcc 3.0 or later

Re: i386 entry.S problems

[Ingo Molnar]

* Arjan van de Ven <arjanv@redhat.com> wrote:

> On Fri, 2004-09-24 at 16:12, Jan Beulich wrote:
> > There appear to be two problems in i386's entry.S:
> > 
> > (1) With CONFIG_REGPARM, lcall7 and lcall27 did not work (they pass the
> > parameters to the actual handler procedure on the stack).
> 
> I wonder why we still have the lcall7/lcall27 entry points in the
> kernel; nothing can legitemately use them and in the last few years
> they have only caused a few security issues. Can I ask why you didn't
> just remove this code from the kernel ?

patch below (against BK-curr) zaps the orphaned lcall7/lcall27 code.

	Ingo

Signed-off-by: Ingo Molnar <mingo@elte.hu>

--- linux/arch/i386/kernel/entry.S.orig	
+++ linux/arch/i386/kernel/entry.S	
@@ -140,40 +140,6 @@ VM_MASK		= 0x00020000
 .previous
 
 
-
-ENTRY(lcall7)
-	pushfl			# We get a different stack layout with call
-				# gates, which has to be cleaned up later..
-	pushl %eax
-	SAVE_ALL
-	movl %esp, %ebp
-	pushl %ebp
-	pushl $0x7
-do_lcall:
-	movl EIP(%ebp), %eax	# due to call gates, this is eflags, not eip..
-	movl CS(%ebp), %edx	# this is eip..
-	movl EFLAGS(%ebp), %ecx	# and this is cs..
-	movl %eax,EFLAGS(%ebp)	#
-	movl %edx,EIP(%ebp)	# Now we move them to their "normal" places
-	movl %ecx,CS(%ebp)	#
-	GET_THREAD_INFO_WITH_ESP(%ebp)	# GET_THREAD_INFO
-	movl TI_exec_domain(%ebp), %edx	# Get the execution domain
-	call *EXEC_DOMAIN_handler(%edx)	# Call the handler for the domain
-	addl $4, %esp
-	popl %eax
-	jmp resume_userspace
-
-ENTRY(lcall27)
-	pushfl			# We get a different stack layout with call
-				# gates, which has to be cleaned up later..
-	pushl %eax
-	SAVE_ALL
-	movl %esp, %ebp
-	pushl %ebp
-	pushl $0x27
-	jmp do_lcall
-
-
 ENTRY(ret_from_fork)
 	pushl %eax
 	call schedule_tail
--- linux/arch/i386/kernel/traps.c.orig	
+++ linux/arch/i386/kernel/traps.c	
@@ -57,8 +57,6 @@
 #include "mach_traps.h"
 
 asmlinkage int system_call(void);
-asmlinkage void lcall7(void);
-asmlinkage void lcall27(void);
 
 struct desc_struct default_ldt[] = { { 0, 0 }, { 0, 0 }, { 0, 0 },
 		{ 0, 0 }, { 0, 0 } };
@@ -1015,11 +1013,6 @@ static void __init set_system_gate(unsig
 	_set_gate(idt_table+n,15,3,addr,__KERNEL_CS);
 }
 
-static void __init set_call_gate(void *a, void *addr)
-{
-	_set_gate(a,12,3,addr,__KERNEL_CS);
-}
-
 static void __init set_task_gate(unsigned int n, unsigned int gdt_entry)
 {
 	_set_gate(idt_table+n,5,0,0,(gdt_entry<<3));
@@ -1064,13 +1057,6 @@ void __init trap_init(void)
 	set_system_gate(SYSCALL_VECTOR,&system_call);
 
 	/*
-	 * default LDT is a single-entry callgate to lcall7 for iBCS
-	 * and a callgate to lcall27 for Solaris/x86 binaries
-	 */
-	set_call_gate(&default_ldt[0],lcall7);
-	set_call_gate(&default_ldt[4],lcall27);
-
-	/*
 	 * Should be a barrier for any external CPU state.
 	 */
 	cpu_init();

Re: i386 entry.S problems

[Andi Kleen]

Ingo Molnar <mingo@elte.hu> writes:
>> 
>> I wonder why we still have the lcall7/lcall27 entry points in the
>> kernel; nothing can legitemately use them and in the last few years
>> they have only caused a few security issues. Can I ask why you didn't
>> just remove this code from the kernel ?
>
> patch below (against BK-curr) zaps the orphaned lcall7/lcall27 code.

I did this for x86-64 long ago :-)

You can zap the default LDT handling in ldt.c then too.

-Andi

Re: i386 entry.S problems

[Jan Beulich]

>>> Christoph Hellwig <hch@infradead.org> 24.09.04 21:12:51 >>>
>> +#if !defined(CONFIG_REGPARM) || __GNUC__ < 3
>>  	pushl %ebp
>> +#endif
>
>CONFIG_REGPARM n eeds gcc 3.0 or later

Not sure what you try to point out here: the additions account for
exactly that.

Re: i386 entry.S problems

[Jan Beulich]

In addition to Andi Kleen's statement: Along with removing the remaining
default LDT handling, wouldn't it be reasonable to then also remove the
ill i386-specific code in kernel/exec_domain.c? Jan

>>> Ingo Molnar <mingo@elte.hu> 26.09.04 13:13:34 >>>

* Arjan van de Ven <arjanv@redhat.com> wrote:

> On Fri, 2004-09-24 at 16:12, Jan Beulich wrote:
> > There appear to be two problems in i386's entry.S:
> > 
> > (1) With CONFIG_REGPARM, lcall7 and lcall27 did not work (they pass
the
> > parameters to the actual handler procedure on the stack).
> 
> I wonder why we still have the lcall7/lcall27 entry points in the
> kernel; nothing can legitemately use them and in the last few years
> they have only caused a few security issues. Can I ask why you
didn't
> just remove this code from the kernel ?

patch below (against BK-curr) zaps the orphaned lcall7/lcall27 code.

	Ingo

Signed-off-by: Ingo Molnar <mingo@elte.hu>

--- linux/arch/i386/kernel/entry.S.orig	
+++ linux/arch/i386/kernel/entry.S	
@@ -140,40 +140,6 @@ VM_MASK		= 0x00020000
 .previous
 
 
-
-ENTRY(lcall7)
-	pushfl			# We get a different stack layout with
call
-				# gates, which has to be cleaned up
later..
-	pushl %eax
-	SAVE_ALL
-	movl %esp, %ebp
-	pushl %ebp
-	pushl $0x7
-do_lcall:
-	movl EIP(%ebp), %eax	# due to call gates, this is eflags, not
eip..
-	movl CS(%ebp), %edx	# this is eip..
-	movl EFLAGS(%ebp), %ecx	# and this is cs..
-	movl %eax,EFLAGS(%ebp)	#
-	movl %edx,EIP(%ebp)	# Now we move them to their "normal"
places
-	movl %ecx,CS(%ebp)	#
-	GET_THREAD_INFO_WITH_ESP(%ebp)	# GET_THREAD_INFO
-	movl TI_exec_domain(%ebp), %edx	# Get the execution domain
-	call *EXEC_DOMAIN_handler(%edx)	# Call the handler for the
domain
-	addl $4, %esp
-	popl %eax
-	jmp resume_userspace
-
-ENTRY(lcall27)
-	pushfl			# We get a different stack layout with
call
-				# gates, which has to be cleaned up
later..
-	pushl %eax
-	SAVE_ALL
-	movl %esp, %ebp
-	pushl %ebp
-	pushl $0x27
-	jmp do_lcall
-
-
 ENTRY(ret_from_fork)
 	pushl %eax
 	call schedule_tail
--- linux/arch/i386/kernel/traps.c.orig	
+++ linux/arch/i386/kernel/traps.c	
@@ -57,8 +57,6 @@
 #include "mach_traps.h"
 
 asmlinkage int system_call(void);
-asmlinkage void lcall7(void);
-asmlinkage void lcall27(void);
 
 struct desc_struct default_ldt[] = { { 0, 0 }, { 0, 0 }, { 0, 0 },
 		{ 0, 0 }, { 0, 0 } };
@@ -1015,11 +1013,6 @@ static void __init set_system_gate(unsig
 	_set_gate(idt_table+n,15,3,addr,__KERNEL_CS);
 }
 
-static void __init set_call_gate(void *a, void *addr)
-{
-	_set_gate(a,12,3,addr,__KERNEL_CS);
-}
-
 static void __init set_task_gate(unsigned int n, unsigned int
gdt_entry)
 {
 	_set_gate(idt_table+n,5,0,0,(gdt_entry<<3));
@@ -1064,13 +1057,6 @@ void __init trap_init(void)
 	set_system_gate(SYSCALL_VECTOR,&system_call);
 
 	/*
-	 * default LDT is a single-entry callgate to lcall7 for iBCS
-	 * and a callgate to lcall27 for Solaris/x86 binaries
-	 */
-	set_call_gate(&default_ldt[0],lcall7);
-	set_call_gate(&default_ldt[4],lcall27);
-
-	/*
 	 * Should be a barrier for any external CPU state.
 	 */
 	cpu_init();

Re: i386 entry.S problems

[Christoph Hellwig]

On Mon, Sep 27, 2004 at 09:37:10AM +0200, Jan Beulich wrote:
> >>> Christoph Hellwig <hch@infradead.org> 24.09.04 21:12:51 >>>
> >> +#if !defined(CONFIG_REGPARM) || __GNUC__ < 3
> >>  	pushl %ebp
> >> +#endif
> >
> >CONFIG_REGPARM n eeds gcc 3.0 or later
> 
> Not sure what you try to point out here: the additions account for
> exactly that.

No, the || __GNUC__ < 3 is superflous.  if CONFIG_REGPARM is defined
and __GNUC__ < 3 you have problems elsewhere already.

Re: i386 entry.S problems

[Jan Beulich]

>>> Christoph Hellwig <hch@infradead.org> 27.09.04 11:00:18 >>>
>On Mon, Sep 27, 2004 at 09:37:10AM +0200, Jan Beulich wrote:
>> >>> Christoph Hellwig <hch@infradead.org> 24.09.04 21:12:51 >>>
>> >> +#if !defined(CONFIG_REGPARM) || __GNUC__ < 3
>> >>  	pushl %ebp
>> >> +#endif
>> >
>> >CONFIG_REGPARM n eeds gcc 3.0 or later
>> 
>> Not sure what you try to point out here: the additions account for
>> exactly that.
>
>No, the || __GNUC__ < 3 is superflous.  if CONFIG_REGPARM is defined
>and __GNUC__ < 3 you have problems elsewhere already.

I don't think so. Otherwise, why would arch/i386/Makefile specifically
deal with this situation?

Re: i386 entry.S problems

[Andi Kleen]

"Jan Beulich" <JBeulich@novell.com> writes:
>
> I don't think so. Otherwise, why would arch/i386/Makefile specifically
> deal with this situation?

It shouldn't be enabled for 2.95, there are known miscompilations
caused by it there.  The i386 Makefile enforces this:

cflags-$(CONFIG_REGPARM) 	+= $(shell if [ $(GCC_VERSION) -ge 0300 ] ; then echo "-mregparm=3"; fi ;)

However this points to a bug in that when someone sets this
on 2.95 the assembly functions who check for CONFIG_REGPARM
explicitely will be subtly miscompiled. Perhaps having 
a #error for this case would be better, although that
would break allyesconfig on prehistoric compilers. Maybe 
it needs to be special cased in autoconf.h

-Andi

Re: i386 entry.S problems

[Jan Beulich]

>>> Andi Kleen <ak@muc.de> 27.09.04 12:58:57 >>>
>"Jan Beulich" <JBeulich@novell.com> writes:
>>
>> I don't think so. Otherwise, why would arch/i386/Makefile
specifically
>> deal with this situation?
>
>It shouldn't be enabled for 2.95, there are known miscompilations
>caused by it there.  The i386 Makefile enforces this:
>
>cflags-$(CONFIG_REGPARM) 	+= $(shell if [ $(GCC_VERSION) -ge 0300
] ; then echo "-mregparm=3"; fi ;)

But that is exactly what I'm trying to account for: As we appear to all
agree, with CONFIG_REGPARM but too old a gcc (which is unknown at
configuration time and thus the user can't be prevented from turning
this option on), mismatches between assembly and C would result. Thus
the need for checking the gcc version in (some) assembly files (of
course this implies that for preprocessing assembly files one would not
try to use a different gcc than that used for compiling the C stuff).

I do, however, believe that it is an inherent weakness of gcc that it
doesn't decorate names of functions employing non-standard parameter
passing schemes in some way (i.e. similar to the __stdcall decoration on
Windows); with such functionality, it'd be much easier and safer to use
mixed schemes.

>However this points to a bug in that when someone sets this
>on 2.95 the assembly functions who check for CONFIG_REGPARM
>explicitely will be subtly miscompiled. Perhaps having 
>a #error for this case would be better, although that
>would break allyesconfig on prehistoric compilers. Maybe 
>it needs to be special cased in autoconf.h

Jan