nested functions used by multiboot2 loader corrupt stack

nested functions used by multiboot2 loader corrupt stack

[Robert Millan]

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


I got pretty confused at this one.  Maybe someone can sort this out.  I'm
afraid I can't :-(

It seems that at some point when loading multiboot2 images, our stack is
corrupted for no apparent reason and one of the hooks in our nested function
calls ends up jumping to the wrong place.

This hangs qemu 0.9.0, but qemu 0.9.1 aborts with "triple fault" message.

I added a few printf calls to trace what's going on, and switched to serial
terminal so that the output can be captured.  My debugging patch is attached.
This is the output:

grub_mb2_load_elf: going to call grub_elf32_load using grub_mb2_arch_elf32_hook=0x7ffc72c as hook
grub_elf32_load: going to call grub_elf32_phdr_iterate using grub_elf32_load_segment=0x7dda4 as hook, and _load_hook=0x7ffc72c as hook's hook
grub_elf32_phdr_iterate: going to call hook=0x7dda4 using hook_arg=0x7ffc72c as hook
grub_elf32_load_segment: going to call load_hook=0x7dd9c
qemu: fatal: triple fault
EAX=0004be50 EBX=0004bf30 ECX=0008de66 EDX=0007dd2c
ESI=0004be50 EDI=0007dd9c EBP=0007dd3c ESP=0007dd10
EIP=0007dda0 EFL=00000002 [-------] CPL=0 II=0 A20=1 SMM=0 HLT=0

This seems to indicate that grub_elf32_phdr_iterate() called its hook, 0x7dda4,
aka grub_elf32_load_segment() with proper hook_arg parameter = 0x7ffc72c, aka
grub_mb2_arch_elf32_hook().

When grub_elf32_load_segment() starts, its hook_arg parameter (now known as
load_hook) has been corrupted and now points at 0x7dd9c.  The other two
parameters in this function are not tainted, only the third one is.

I'm not sure how to proceed from here.  I really miss a debugger in these
cases :-(

Also attaching the sample multiboot2 program I used.  I'm not sure of its
correctness, but nevertheless GRUB shouldn't crash because of incorrect
images; specially not at this point.

-- 
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What use is a phone call… if you are unable to speak?
(as seen on /.)

[-- Attachment #2: debug_multiboot2.diff --]
[-- Type: text/x-diff, Size: 2184 bytes --]

diff -x CVS -ur ../grub2/kern/elf.c ./kern/elf.c
--- ../grub2/kern/elf.c	2007-07-22 01:32:26.000000000 +0200
+++ ./kern/elf.c	2008-01-16 23:43:17.000000000 +0100
@@ -160,6 +160,8 @@
 		    (unsigned long) phdr->p_paddr,
 		    (unsigned long) phdr->p_memsz,
 		    (unsigned long) phdr->p_filesz);
+      grub_printf ("grub_elf32_phdr_iterate: going to call hook=%p using hook_arg=%p as hook\n", hook, hook_arg);
+
       if (hook (elf, phdr, hook_arg))
 	break;
     }
@@ -230,6 +232,7 @@
       return 0;
 
     load_addr = phdr->p_paddr;
+    grub_printf ("grub_elf32_load_segment: going to call load_hook=%p\n", load_hook);
     if (load_hook && load_hook (phdr, &load_addr))
       return 1;
 
@@ -271,6 +274,8 @@
     return 0;
   }
 
+  grub_printf ("grub_elf32_load: going to call grub_elf32_phdr_iterate using grub_elf32_load_segment=%p as hook, and _load_hook=%p as hook's hook\n", grub_elf32_load_segment, _load_hook);
+
   err = grub_elf32_phdr_iterate (_elf, grub_elf32_load_segment, _load_hook);
 
   if (base)
diff -x CVS -ur ../grub2/loader/i386/pc/multiboot2.c ./loader/i386/pc/multiboot2.c
--- ../grub2/loader/i386/pc/multiboot2.c	2007-07-25 02:44:02.000000000 +0200
+++ ./loader/i386/pc/multiboot2.c	2008-01-16 23:48:40.000000000 +0100
@@ -27,7 +27,12 @@
 grub_err_t
 grub_mb2_arch_elf32_hook (Elf32_Phdr *phdr, UNUSED grub_addr_t *addr)
 {
-  Elf32_Addr paddr = phdr->p_paddr;
+  Elf32_Addr paddr;
+
+  grub_printf ("success! we reached grub_mb2_arch_elf32_hook\n");
+  while (1);
+
+  paddr = phdr->p_paddr;
 
   if ((paddr < grub_os_area_addr)
       || (paddr + phdr->p_memsz > grub_os_area_addr + grub_os_area_size))
diff -x CVS -ur ../grub2/loader/multiboot2.c ./loader/multiboot2.c
--- ../grub2/loader/multiboot2.c	2007-07-25 02:44:02.000000000 +0200
+++ ./loader/multiboot2.c	2008-01-16 23:42:17.000000000 +0100
@@ -279,6 +279,7 @@
   if (grub_elf_is_elf32 (elf))
     {
       entry = elf->ehdr.ehdr32.e_entry;
+      grub_printf ("grub_mb2_load_elf: going to call grub_elf32_load using grub_mb2_arch_elf32_hook=%p as hook\n", grub_mb2_arch_elf32_hook);
       err = grub_elf32_load (elf, grub_mb2_arch_elf32_hook, &kern_base,
 			     &kern_size);
     }

[-- Attachment #3: hello.S --]
[-- Type: text/plain, Size: 323 bytes --]

/* Trivial multiboot2 program.  If it works, you'll see a red A in the screen.

   gcc -fno-builtin -nostdinc -m32 -nostdlib -Wl,-N -Wl,-Ttext -Wl,0x100000 -o hello hello.S
*/

	.file	"startup.S"
	.text
	.globl	start, _start

	.long	0xe85250d6
	.long	0
	.long	-0xe85250d6

start:
_start:
	movw $0x4141, 0xb8000
	jmp _start

Re: nested functions used by multiboot2 loader corrupt stack

[Bean]

On Jan 17, 2008 7:05 AM, Robert Millan <rmh@aybabtu.com> wrote:
>
> I got pretty confused at this one.  Maybe someone can sort this out.  I'm
> afraid I can't :-(
>
> It seems that at some point when loading multiboot2 images, our stack is
> corrupted for no apparent reason and one of the hooks in our nested function
> calls ends up jumping to the wrong place.
>
> This hangs qemu 0.9.0, but qemu 0.9.1 aborts with "triple fault" message.
>
> I added a few printf calls to trace what's going on, and switched to serial
> terminal so that the output can be captured.  My debugging patch is attached.
> This is the output:
>
> grub_mb2_load_elf: going to call grub_elf32_load using grub_mb2_arch_elf32_hook=0x7ffc72c as hook
> grub_elf32_load: going to call grub_elf32_phdr_iterate using grub_elf32_load_segment=0x7dda4 as hook, and _load_hook=0x7ffc72c as hook's hook
> grub_elf32_phdr_iterate: going to call hook=0x7dda4 using hook_arg=0x7ffc72c as hook
> grub_elf32_load_segment: going to call load_hook=0x7dd9c
> qemu: fatal: triple fault
> EAX=0004be50 EBX=0004bf30 ECX=0008de66 EDX=0007dd2c
> ESI=0004be50 EDI=0007dd9c EBP=0007dd3c ESP=0007dd10
> EIP=0007dda0 EFL=00000002 [-------] CPL=0 II=0 A20=1 SMM=0 HLT=0
>
> This seems to indicate that grub_elf32_phdr_iterate() called its hook, 0x7dda4,
> aka grub_elf32_load_segment() with proper hook_arg parameter = 0x7ffc72c, aka
> grub_mb2_arch_elf32_hook().
>
> When grub_elf32_load_segment() starts, its hook_arg parameter (now known as
> load_hook) has been corrupted and now points at 0x7dd9c.  The other two
> parameters in this function are not tainted, only the third one is.
>
> I'm not sure how to proceed from here.  I really miss a debugger in these
> cases :-(
>
> Also attaching the sample multiboot2 program I used.  I'm not sure of its
> correctness, but nevertheless GRUB shouldn't crash because of incorrect
> images; specially not at this point.

You need to add NESTED_FUNC_ATTR to nested callback function that use
local variable. here is the patch:

diff --git a/kern/elf.c b/kern/elf.c
index b362949..4978a27 100644
--- a/kern/elf.c
+++ b/kern/elf.c
@@ -139,7 +139,7 @@ grub_elf32_load_phdrs (grub_elf_t elf)

 static grub_err_t
 grub_elf32_phdr_iterate (grub_elf_t elf,
-			 int (*hook) (grub_elf_t, Elf32_Phdr *, void *),
+			 int NESTED_FUNC_ATTR (*hook) (grub_elf_t, Elf32_Phdr *, void *),
 			 void *hook_arg)
 {
   Elf32_Phdr *phdrs;
@@ -219,9 +219,8 @@ grub_elf32_load (grub_elf_t _elf,
grub_elf32_load_hook_t _load_hook,
   grub_size_t load_size = 0;
   grub_err_t err;

-  auto int grub_elf32_load_segment (grub_elf_t elf, Elf32_Phdr *phdr,
-				    void *hook);
-  int grub_elf32_load_segment (grub_elf_t elf, Elf32_Phdr *phdr, void *hook)
+  auto int NESTED_FUNC_ATTR grub_elf32_load_segment (grub_elf_t elf,
Elf32_Phdr *phdr, void *hook);
+  int NESTED_FUNC_ATTR grub_elf32_load_segment (grub_elf_t elf,
Elf32_Phdr *phdr, void *hook)
   {
     grub_elf32_load_hook_t load_hook = (grub_elf32_load_hook_t) hook;
     grub_addr_t load_addr;


-- 
Bean

Re: nested functions used by multiboot2 loader corrupt stack

[Robert Millan]

On Thu, Jan 17, 2008 at 04:15:23PM +0800, Bean wrote:
> 
> You need to add NESTED_FUNC_ATTR to nested callback function that use
> local variable. here is the patch:

Glad to see you found the reason!

But I don't get it.  I see that:

#define NESTED_FUNC_ATTR __attribute__ ((__regparm__ (2)))

so does this mean in one of the calls the caller and callee disagreed about
how the third param is passed?

Also, we have a lot of nested functions without this macro.  How does one
distinguish the ones that need it from the ones that don't?

-- 
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What use is a phone call… if you are unable to speak?
(as seen on /.)

Re: nested functions used by multiboot2 loader corrupt stack

[Bean]

On Jan 17, 2008 8:21 PM, Robert Millan <rmh@aybabtu.com> wrote:
> On Thu, Jan 17, 2008 at 04:15:23PM +0800, Bean wrote:
> >
> > You need to add NESTED_FUNC_ATTR to nested callback function that use
> > local variable. here is the patch:
>
> Glad to see you found the reason!
>
> But I don't get it.  I see that:
>
> #define NESTED_FUNC_ATTR __attribute__ ((__regparm__ (2)))
>
> so does this mean in one of the calls the caller and callee disagreed about
> how the third param is passed?
>
> Also, we have a lot of nested functions without this macro.  How does one
> distinguish the ones that need it from the ones that don't?

Embedded function used %ecx to store the pointer to it's parent's
stack. However, the program is compiled using option -mregparm=3,
which means it can use up to 3 registry to pass parameter.In
grub_elf32_load_segment, there are three parameter elf, phdr and hook,
which will take up %eax, %edx and %ecx. The value of %ecx, hook, will
be overwritten. Use NESTED_FUNC_ATTR ensure that only the first two
parameter will be passed using registry

This problem can occur when the following conditions are true:

1, Use embedded function as callback.
2, The embedded function use local variable in it's parent's stack.
3, The embedded function has at least three parameters.

-- 
Bean

Re: nested functions used by multiboot2 loader corrupt stack

[Robert Millan]

On Thu, Jan 17, 2008 at 11:47:50PM +0800, Bean wrote:
> 
> Embedded function used %ecx to store the pointer to it's parent's
> stack. However, the program is compiled using option -mregparm=3,
> which means it can use up to 3 registry to pass parameter.In
> grub_elf32_load_segment, there are three parameter elf, phdr and hook,
> which will take up %eax, %edx and %ecx. The value of %ecx, hook, will
> be overwritten. Use NESTED_FUNC_ATTR ensure that only the first two
> parameter will be passed using registry
> 
> This problem can occur when the following conditions are true:
> 
> 1, Use embedded function as callback.
> 2, The embedded function use local variable in it's parent's stack.
> 3, The embedded function has at least three parameters.

Thanks for the explanation, I think I got the idea now.

I reviewed all GRUB code for other instances of this bug, and only found
the equivalent 64-bit versions of the functions you fixed to be affected.

Just committed a fix based on your patch (plus the 64-bit ones).

-- 
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What use is a phone call… if you are unable to speak?
(as seen on /.)