headers
Carl-Daniel Hailfinger | 8 Oct 02:04

[PATCH] Fix v3 GeodeLX stack and global variable pointer corruption

Fix v3 GeodeLX stack and global variable pointer corruption.
We had a jump instead of a call to stage1_main in geodelx/stage0.S. That
means all accesses to bist and init_detected were off by 8 bytes and
collided with accesses to the global variable pointer.

Found during my cleanup runs.

Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 <at> gmx.net>
Acked-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 <at> gmx.net>

Index: corebootv3-arch_x86_cleanup/arch/x86/geodelx/stage0.S
===================================================================
--- corebootv3-arch_x86_cleanup/arch/x86/geodelx/stage0.S	(Revision 907)
+++ corebootv3-arch_x86_cleanup/arch/x86/geodelx/stage0.S	(Arbeitskopie)
@@ -376,7 +376,7 @@
 	pushl	$0
 	/* First parameter: bist */
 	pushl	%eax
-	jmp	stage1_main
+	call	stage1_main
 	/* We will not go back. */

 /* Reset vector. */

--

-- 
http://www.hailfinger.org/

--
coreboot mailing list: coreboot <at> coreboot.org
http://www.coreboot.org/mailman/listinfo/coreboot
(Continue reading)

Permalink | Reply |
headers
Peter Stuge | 8 Oct 02:29

Re: [PATCH] Fix v3 GeodeLX stack and global variable pointer corruption

Carl-Daniel Hailfinger wrote:
> Fix v3 GeodeLX stack and global variable pointer corruption.
> We had a jump instead of a call to stage1_main in geodelx/stage0.S. That
> means all accesses to bist and init_detected were off by 8 bytes and
> collided with accesses to the global variable pointer.

Can you explain what, if any, effect this bug had or could have had
in practice?

//Peter

--
coreboot mailing list: coreboot <at> coreboot.org
http://www.coreboot.org/mailman/listinfo/coreboot
Permalink | Reply |
headers
Carl-Daniel Hailfinger | 8 Oct 03:05

Re: [PATCH] Fix v3 GeodeLX stack and global variable pointer corruption

On 08.10.2008 02:29, Peter Stuge wrote:
> Carl-Daniel Hailfinger wrote:
>   
>> Fix v3 GeodeLX stack and global variable pointer corruption.
>> We had a jump instead of a call to stage1_main in geodelx/stage0.S. That
>> means all accesses to bist and init_detected were off by 8 bytes and
>> collided with accesses to the global variable pointer.
>>     
>
> Can you explain what, if any, effect this bug had or could have had
> in practice?
>   

Sure.
1.) If gcc had decided to reload bist from stack after initializing the
global variable pointer, bist would have been nonzero, an indicator for
processor failure.
2.) If gcc had decided to use the stack location of bist as a scratch
register (and it probably is free to do so as long as the contents are
restored before returning), it would have clobbered the global variable
pointer, leading to NULL pointer dereferences.
3.) Any accesses to init_detected would have resulted in accessing 4
bytes above the top of stack (0x87ffc-0x87fff), something the rest of
the code deliberately avoids.

Regards,
Carl-Daniel

--

-- 
http://www.hailfinger.org/
(Continue reading)

Permalink | Reply |
headers
Carl-Daniel Hailfinger | 8 Oct 17:56

Re: [PATCH] Fix v3 GeodeLX stack and global variable pointer corruption

Ron?

I believe that fix is really needed and the may have been the culprit of
some real-world unexplained strangeness.

Regards,
Carl-Daniel

On 08.10.2008 03:05, Carl-Daniel Hailfinger wrote:
> On 08.10.2008 02:29, Peter Stuge wrote:
>   
>> Carl-Daniel Hailfinger wrote:
>>   
>>     
>>> Fix v3 GeodeLX stack and global variable pointer corruption.
>>> We had a jump instead of a call to stage1_main in geodelx/stage0.S. That
>>> means all accesses to bist and init_detected were off by 8 bytes and
>>> collided with accesses to the global variable pointer.
>>>     
>>>       
>> Can you explain what, if any, effect this bug had or could have had
>> in practice?
>>   
>>     
>
> Sure.
> 1.) If gcc had decided to reload bist from stack after initializing the
> global variable pointer, bist would have been nonzero, an indicator for
> processor failure.
> 2.) If gcc had decided to use the stack location of bist as a scratch
(Continue reading)

Permalink | Reply |
headers
ron minnich | 8 Oct 18:00

Re: [PATCH] Fix v3 GeodeLX stack and global variable pointer corruption

On Wed, Oct 8, 2008 at 8:56 AM, Carl-Daniel Hailfinger
<c-d.hailfinger.devel.2006 <at> gmx.net> wrote:
> Ron?
>
> I believe that fix is really needed and the may have been the culprit of
> some real-world unexplained strangeness.
>

I see it as a very fine catch. I did not see a problem in practice,
but wow! what a nice find.

ron

--
coreboot mailing list: coreboot <at> coreboot.org
http://www.coreboot.org/mailman/listinfo/coreboot
Permalink | Reply |
headers
Carl-Daniel Hailfinger | 8 Oct 18:12

Re: [PATCH] Fix v3 GeodeLX stack and global variable pointer corruption

On 08.10.2008 18:00, ron minnich wrote:
> On Wed, Oct 8, 2008 at 8:56 AM, Carl-Daniel Hailfinger
> <c-d.hailfinger.devel.2006 <at> gmx.net> wrote:
>   
>> Ron?
>>
>> I believe that fix is really needed and the may have been the culprit of
>> some real-world unexplained strangeness.
>>
>>     
>
> I see it as a very fine catch. I did not see a problem in practice,
> but wow! what a nice find.
>   

Thanks, r909.

Regards,
Carl-Daniel

--

-- 
http://www.hailfinger.org/

--
coreboot mailing list: coreboot <at> coreboot.org
http://www.coreboot.org/mailman/listinfo/coreboot
Permalink | Reply |
headers
Peter Stuge | 8 Oct 18:13

Re: [PATCH] Fix v3 GeodeLX stack and global variable pointer corruption

Carl-Daniel Hailfinger wrote:
> > Can you explain what, if any, effect this bug had or could have
> > had in practice?
> 
> Sure.
> 1.) If gcc had decided to reload bist from stack after initializing the
> global variable pointer, bist would have been nonzero, an indicator for
> processor failure.
> 2.) If gcc had decided to use the stack location of bist as a scratch
> register (and it probably is free to do so as long as the contents are
> restored before returning), it would have clobbered the global variable
> pointer, leading to NULL pointer dereferences.
> 3.) Any accesses to init_detected would have resulted in accessing 4
> bytes above the top of stack (0x87ffc-0x87fff), something the rest of
> the code deliberately avoids.

Thanks! If you add the above to the commit message I say:

Acked-by: Peter Stuge <peter <at> stuge.se>

--
coreboot mailing list: coreboot <at> coreboot.org
http://www.coreboot.org/mailman/listinfo/coreboot
Permalink | Reply |

Gmane