Re: [coreboot] r783 - coreboot-v3/arch/x86

[Carl-Daniel Hailfinger]

On 18.08.2008 15:56, Stefan Reinauer wrote:
> Carl-Daniel Hailfinger asked:
> > /* we can't statically init this hack. */
> > /* Why can't we statically init this hack? */
>
> No global variables in stage 1.

Hm. The variable is only used locally. We could at least use automatic
init for it.

Now I see it. The reason is that we would get a buffer overflow. Nasty.
struct lb_memory has a zero-sized map array, but we use one element.
struct lb_memory {
        u32 tag;
        u32 size;
        struct lb_memory_range map[0];
};

The fix is to declare
struct lb_memory_one_map {
        u32 tag;
        u32 size;
        struct lb_memory_range map[1];
};
and cast it to struct lb_memory.

Compile-tested patch follows.

Signed-off-by: Carl-Daniel Hailfinger

Index: corebootv3-stage1_elfload_cleanup/include/tables.h
===================================================================
--- corebootv3-stage1_elfload_cleanup/include/tables.h	(Revision 783)
+++ corebootv3-stage1_elfload_cleanup/include/tables.h	(Arbeitskopie)
@@ -135,6 +135,12 @@
    struct lb_memory_range map[0];
 };

+struct lb_memory_one_map {
+	u32 tag;
+	u32 size;
+	struct lb_memory_range map[1];
+};
+
 #define LB_TAG_HWRPB	0x0002
 struct lb_hwrpb {
    u32 tag;
Index: corebootv3-stage1_elfload_cleanup/arch/x86/stage1.c
===================================================================
--- corebootv3-stage1_elfload_cleanup/arch/x86/stage1.c	(Revision 783)
+++ corebootv3-stage1_elfload_cleanup/arch/x86/stage1.c	(Arbeitskopie)
@@ -127,18 +127,16 @@
    void *entry;
 #ifdef CONFIG_PAYLOAD_ELF_LOADER
    struct mem_file result;
-	int elfboot_mem(struct lb_memory *mem, void *where, int size);

/* Why can't we statically init this hack? */
-	unsigned char faker[64];
-	struct lb_memory *mem = (struct lb_memory*) faker;
+	struct lb_memory_one_map mem;

-	mem->tag = LB_TAG_MEMORY;
-	mem->size = 28;
-	mem->map[0].start.lo = mem->map[0].start.hi = 0;
-	mem->map[0].size.lo = (32*1024*1024);
-	mem->map[0].size.hi = 0;
-	mem->map[0].type = LB_MEM_RAM;
+	mem.tag = LB_TAG_MEMORY;
+	mem.size = sizeof(mem);
+	mem.map[0].start.lo = mem.map[0].start.hi = 0;
+	mem.map[0].size.lo = (32*1024*1024);
+	mem.map[0].size.hi = 0;
+	mem.map[0].type = LB_MEM_RAM;
 #endif /* CONFIG_PAYLOAD_ELF_LOADER */

post_code(POST_STAGE1_MAIN);
@@ -221,7 +219,7 @@
 #ifdef CONFIG_PAYLOAD_ELF_LOADER
    ret = find_file(&archive, "normal/payload", &result);
    if (! ret)
-		legacy(&archive, "normal/payload", (void *)UNCOMPRESS_AREA, mem);
+		legacy(&archive, "normal/payload", (void *)UNCOMPRESS_AREA, (struct lb_memory *)&mem);
 #endif /* CONFIG_PAYLOAD_ELF_LOADER */

entry = load_file_segments(&archive, "normal/payload");