RE: [BUG] in i386 semaphores.

RE: [BUG] in i386 semaphores.

[Aleksey Gorelov]

 

>-----Original Message-----
>From: Richard B. Johnson [mailto:root@chaos.analogic.com] 
>Sent: Monday, October 18, 2004 12:49 PM
>__up is declared 'asmlinkage', which means that conventional
>'C' calling convention is used, that eax and/or edx/eax pair
>is used for return values and the input values are pushed on
>  the stack. The last parameter passed is a pointer in %ecx.

Can not disagree here, except that %ecx is the only parameter.
eax/edx are just saved in the stack, and then restored (see comments
before the assembly in semaphore.c).
>
>Therefore, the called 'C' function, that 'knows' only about
>the first parameter passed, will get the correct pointer value.
>The 'C' function also never changes the value of that pointer,
>only something that it points to.

This is an assumption. AFAIK, according to C standard, formal parameters
are
COPIED from actual parameters. Formal parameters can not be used outside
the function, and as far as they are not constant ones, C compiler
is free to mofidy them after their last use, and use the memory for
other purposes.
>
>Now, wake_up() gets a pointer to the variable sem->wait. wake_up()
>never modifies 'sem', only sem->wait.

Well, build the kernel with DEBUG_KERNEL disabled with gcc 3.2 for
instance,
objdump it and check __up() code. You'll be surprised. And this is NOT
gcc issue.
>>  As one can see, actual parameter in %ecx is not only being copied in
>> formal parameter sem (which is correct), but also being 
>restored from it
>> after function call via %ecx (which is incorrect). Since formal
>> parameter is not a constant one, it may be overwritten inside C
>> function, or gcc may (and in fact does that in some cases) use it for
>> something else.
>> If we want to keep %ecx, correct behavior would be
>>
>
>
>> 	pushl %ecx
>> 	pushl %ecx
>> 	call _up
>> 	add $4, %ecx
              ^^^^
              %esp

Sorry, there was a typo as specified above. Of cause, you still need
push/pop eax & edx, that was ommited for brevity, cause it is not really
relevant.

>> 	popl %ecx
>>
>
>Very wrong. In fact, it would unbalance the stack. The register
>value, %ecx must be restored exactly as the code does it. One
>can't assume that %ecx magically got changed and needs to be
>'corrected'.
>
>Maybe you meant:
>
> 	pushl	%eax
> 	pushl	%edx
      pushl %ecx   # <- you still need to keep ecx 
> 	pushl	%ecx
> 	call	__up
> 	addl	$0x04, %esp	# Bypass ecx on stack
      popl  %ecx   # <- this one as well
> 	popl	%edx
> 	popl	%eax

I meant as indicated above.

Aleks.

RE: [BUG] in i386 semaphores.

[Richard B. Johnson]

On Mon, 18 Oct 2004, Aleksey Gorelov wrote:

>
>
>> -----Original Message-----
>> From: Richard B. Johnson [mailto:root@chaos.analogic.com]
>> Sent: Monday, October 18, 2004 12:49 PM
>> __up is declared 'asmlinkage', which means that conventional
>> 'C' calling convention is used, that eax and/or edx/eax pair
>> is used for return values and the input values are pushed on
>>  the stack. The last parameter passed is a pointer in %ecx.
>
> Can not disagree here, except that %ecx is the only parameter.
> eax/edx are just saved in the stack, and then restored (see comments
> before the assembly in semaphore.c).
>>
>> Therefore, the called 'C' function, that 'knows' only about
>> the first parameter passed, will get the correct pointer value.
>> The 'C' function also never changes the value of that pointer,
>> only something that it points to.
>
> This is an assumption. AFAIK, according to C standard, formal parameters
> are
> COPIED from actual parameters. Formal parameters can not be used outside
> the function, and as far as they are not constant ones, C compiler
> is free to mofidy them after their last use, and use the memory for
> other purposes.
>>

So you are saying that the stack area that was used by the passed-
parameter (that was never modified) can be destroyed at the whim of
the compiler?

void funct(int param)
{
    printf("%d\n", param);
    compiler_destroy(param);  // Not coded, just done?
}


Some C-compiler expert has to answer that question. If the
answer is true, then you need this patch:


--- linux-2.6.8/arch/i386/kernel/semaphore.c.orig	2004-08-14 01:36:56.000000000 -0400
+++ linux-2.6.8/arch/i386/kernel/semaphore.c	2004-10-19 08:06:15.000000000 -0400
@@ -198,9 +198,11 @@
  #endif
  	"pushl %eax\n\t"
  	"pushl %edx\n\t"
-	"pushl %ecx\n\t"
+	"pushl %ecx\n\t"	// Register to save
+	"pushl %ecx\n\t"	// Passed parameter
  	"call __down\n\t"
-	"popl %ecx\n\t"
+	"addl $0x04, %esp\t\n"	// Bypass corrupted parameter
+	"popl %ecx\n\t"		// Restore original
  	"popl %edx\n\t"
  	"popl %eax\n\t"
  #if defined(CONFIG_FRAME_POINTER)
@@ -220,9 +222,11 @@
  	"movl  %esp,%ebp\n\t"
  #endif
  	"pushl %edx\n\t"
-	"pushl %ecx\n\t"
+	"pushl %ecx\n\t"	// Save register
+	"pushl %ecx\n\t"	// Passed parameter
  	"call __down_interruptible\n\t"
-	"popl %ecx\n\t"
+	"addl $0x04, %esp\n\t"	// Bypass corrupted parameter
+	"popl %ecx\n\t"		// Restore register
  	"popl %edx\n\t"
  #if defined(CONFIG_FRAME_POINTER)
  	"movl %ebp,%esp\n\t"
@@ -241,9 +245,11 @@
  	"movl  %esp,%ebp\n\t"
  #endif
  	"pushl %edx\n\t"
-	"pushl %ecx\n\t"
+	"pushl %ecx\n\t"		// Save register
+	"pushl %ecx\n\t"		// Passed parameter
  	"call __down_trylock\n\t"
-	"popl %ecx\n\t"
+	"addl $0x04, %esp\n\t"		// Bypass corrupted parameter
+	"popl %ecx\n\t"			// Restore register
  	"popl %edx\n\t"
  #if defined(CONFIG_FRAME_POINTER)
  	"movl %ebp,%esp\n\t"
@@ -259,9 +265,11 @@
  "__up_wakeup:\n\t"
  	"pushl %eax\n\t"
  	"pushl %edx\n\t"
-	"pushl %ecx\n\t"
+	"pushl %ecx\n\t"	// Save register
+	"pushl %ecx\n\t"	// Passed parameter
  	"call __up\n\t"
-	"popl %ecx\n\t"
+	"addl $0x04, %esp\n\t"	// Bypass corrupted parameter
+	"popl %ecx\n\t"		// Restore register
  	"popl %edx\n\t"
  	"popl %eax\n\t"
  	"ret"



...and if you need this patch, then there is no reason for
the 'helper' functions at all and the code should be rewritten
to get rid of them.

>> Now, wake_up() gets a pointer to the variable sem->wait. wake_up()
>> never modifies 'sem', only sem->wait.
>
> Well, build the kernel with DEBUG_KERNEL disabled with gcc 3.2 for
> instance,
> objdump it and check __up() code. You'll be surprised. And this is NOT
> gcc issue.

I'm not sure about that. I don't think the 'C' compiler is allowed
to whack at things that were never touched by the code. For instance,
we have "main(int argc, char *argv[], char *env[]);" Normally main()
is only declared to have two arguments, but the environment strings
are really there also. Can the 'C' compiler destroy them? I think
not.

>>>  As one can see, actual parameter in %ecx is not only being copied in
>>> formal parameter sem (which is correct), but also being
>> restored from it
>>> after function call via %ecx (which is incorrect). Since formal
>>> parameter is not a constant one, it may be overwritten inside C
>>> function, or gcc may (and in fact does that in some cases) use it for
>>> something else.
>>> If we want to keep %ecx, correct behavior would be
>>>
>>
>>
>>> 	pushl %ecx
>>> 	pushl %ecx
>>> 	call _up
>>> 	add $4, %ecx
>              ^^^^
>              %esp
>
> Sorry, there was a typo as specified above. Of cause, you still need
> push/pop eax & edx, that was ommited for brevity, cause it is not really
> relevant.
>
>>> 	popl %ecx
>>>
>>
>> Very wrong. In fact, it would unbalance the stack. The register
>> value, %ecx must be restored exactly as the code does it. One
>> can't assume that %ecx magically got changed and needs to be
>> 'corrected'.
>>
>> Maybe you meant:
>>
>> 	pushl	%eax
>> 	pushl	%edx
>      pushl %ecx   # <- you still need to keep ecx
>> 	pushl	%ecx
>> 	call	__up
>> 	addl	$0x04, %esp	# Bypass ecx on stack
>      popl  %ecx   # <- this one as well
>> 	popl	%edx
>> 	popl	%eax
>
> I meant as indicated above.
>
> Aleks.
>

Cheers,
Dick Johnson
Penguin : Linux version 2.6.8 on an i686 machine (5537.79 BogoMips).
             Note 96.31% of all statistics are fiction.

Re: [BUG] in i386 semaphores.

[Richard B. Johnson]

On Mon, 18 Oct 2004, Aleksey Gorelov wrote:

>
> Hi.
>
> I sent this yesterday, but seems like it missed the list somehow :
>
>  There are several assembly 'helpers' in arch/i386/semaphore.c, for
> example, __up_wakeup. __up_wakeup's purpose is to call C function:
> asmlinkage void __up(struct semaphore *sem)
>
> this is how it does it:
>
> 	pushl %eax
> 	pushl %edx
> 	pushl %ecx
> 	call __up
> 	popl %ecx
> 	popl %edx
> 	popl %eax
>

__up is declared 'asmlinkage', which means that conventional
'C' calling convention is used, that eax and/or edx/eax pair
is used for return values and the input values are pushed on
  the stack. The last parameter passed is a pointer in %ecx.

Therefore, the called 'C' function, that 'knows' only about
the first parameter passed, will get the correct pointer value.
The 'C' function also never changes the value of that pointer,
only something that it points to.

Now, wake_up() gets a pointer to the variable sem->wait. wake_up()
never modifies 'sem', only sem->wait.

>  As one can see, actual parameter in %ecx is not only being copied in
> formal parameter sem (which is correct), but also being restored from it
> after function call via %ecx (which is incorrect). Since formal
> parameter is not a constant one, it may be overwritten inside C
> function, or gcc may (and in fact does that in some cases) use it for
> something else.
> If we want to keep %ecx, correct behavior would be
>


> 	pushl %ecx
> 	pushl %ecx
> 	call _up
> 	add $4, %ecx
> 	popl %ecx
>

Very wrong. In fact, it would unbalance the stack. The register
value, %ecx must be restored exactly as the code does it. One
can't assume that %ecx magically got changed and needs to be
'corrected'.

Maybe you meant:

 	pushl	%eax
 	pushl	%edx
 	pushl	%ecx
 	call	__up
 	addl	$0x04, %esp	# Bypass ecx on stack
 	popl	%edx
 	popl	%eax

At least the stack would be correct and the return-address wouldn't
be clobbered. However, now ecx ends up being whatever value some
called 'C' function left it. This will result in a system failure
because previous code expected all registers to be preserved.

> Above applies for other functions in semaphore.c in latest 2.6 kernels.
> 2.4 might also be affected.
>
> Thanks,
> Aleks.
>

It 'taint broke. Don't "fix" it.


Cheers,
Dick Johnson
Penguin : Linux version 2.6.8 on an i686 machine (5537.79 BogoMips).
             Note 96.31% of all statistics are fiction.

[BUG] in i386 semaphores.

[Aleksey Gorelov]

 
Hi.

I sent this yesterday, but seems like it missed the list somehow :
	 
  There are several assembly 'helpers' in arch/i386/semaphore.c, for
example, __up_wakeup. __up_wakeup's purpose is to call C function:
asmlinkage void __up(struct semaphore *sem)
	 
this is how it does it:
 
	pushl %eax
	pushl %edx
	pushl %ecx
	call __up
	popl %ecx
	popl %edx
	popl %eax
	 
  As one can see, actual parameter in %ecx is not only being copied in
formal parameter sem (which is correct), but also being restored from it
after function call via %ecx (which is incorrect). Since formal
parameter is not a constant one, it may be overwritten inside C
function, or gcc may (and in fact does that in some cases) use it for
something else.
If we want to keep %ecx, correct behavior would be

	pushl %ecx
	pushl %ecx
	call _up
	add $4, %ecx
	popl %ecx
	 
Above applies for other functions in semaphore.c in latest 2.6 kernels.
2.4 might also be affected.
	 
Thanks,
Aleks.