Coreboot bug?

[Myles Watson]

src/arch/i386/boot/coreboot_table.c: In function 'write_coreboot_table':
src/arch/i386/boot/coreboot_table.c:492: warning: dereferencing
pointer 'rec_src' does break strict-aliasing rules
src/arch/i386/boot/coreboot_table.c:491: note: initialized from here

#if (CONFIG_HAVE_OPTION_TABLE == 1)
    {
    	struct lb_record *rec_dest, *rec_src;
    	/* Write the option config table... */
    	rec_dest = lb_new_record(head);
    	rec_src = (struct lb_record *)(void *)&option_table;
    	memcpy(rec_dest,  rec_src, rec_src->size);
    	/* Create cmos checksum entry in coreboot table */
    	lb_cmos_checksum(head);
    }
#endif

from option_table.c:
unsigned char option_table[] = {
    0xc8,0x00,0x00,0x00,0x88,0x04,0x00,0x00,0x0c,0x00,
...

from src/include/boot/coreboot_tables.h:
struct lb_record {
    uint32_t tag;		/* tag ID */
    uint32_t size;		/* size of record (in bytes) */
};

Why are we casting it as a lb_record * then using that size field in the copy?

Thanks,
Myles

[Myles Watson]

On Wed, Nov 11, 2009 at 12:31 PM, Myles Watson  wrote:
> src/arch/i386/boot/coreboot_table.c: In function 'write_coreboot_table':
> src/arch/i386/boot/coreboot_table.c:492: warning: dereferencing
> pointer 'rec_src' does break strict-aliasing rules
> src/arch/i386/boot/coreboot_table.c:491: note: initialized from here
>
> #if (CONFIG_HAVE_OPTION_TABLE == 1)
>        {
>                struct lb_record *rec_dest, *rec_src;
>                /* Write the option config table... */
>                rec_dest = lb_new_record(head);
>                rec_src = (struct lb_record *)(void *)&option_table;
>                memcpy(rec_dest,  rec_src, rec_src->size);
>                /* Create cmos checksum entry in coreboot table */
>                lb_cmos_checksum(head);
>        }
> #endif
>
> from option_table.c:
> unsigned char option_table[] = {
>        0xc8,0x00,0x00,0x00,0x88,0x04,0x00,0x00,0x0c,0x00,
> ...
>
> from src/include/boot/coreboot_tables.h:
> struct lb_record {
>        uint32_t tag;           /* tag ID */
>        uint32_t size;          /* size of record (in bytes) */
> };
>
> Why are we casting it as a lb_record * then using that size field in the copy?

All right, I see why it works: 0xc8 is the tag and the size is 0x480
in this case.  Is there a better way to do it that avoids the warning
and makes the code easier to understand?

Thanks,
Myles

[ron minnich]

It's not my favorite piece of code.
> unsigned char option_table[] = {
>        0xc8,0x00,0x00,0x00,0x88,0x04,0x00,0x00,0x0c,0x00,

Second 32 bits 0x88,0x04,0x00,0x00 is the length. We're small endian.
So it's 0x488 or 1160 bytes. Does that match?

So this struct:
> struct lb_record {
>        uint32_t tag;           /* tag ID */
>        uint32_t size;          /* size of record (in bytes) */
> };

is a header of a variable length record.

ron

On Wed, Nov 11, 2009 at 11:31 AM, Myles Watson  wrote:
> src/arch/i386/boot/coreboot_table.c: In function 'write_coreboot_table':
> src/arch/i386/boot/coreboot_table.c:492: warning: dereferencing
> pointer 'rec_src' does break strict-aliasing rules
> src/arch/i386/boot/coreboot_table.c:491: note: initialized from here
>
> #if (CONFIG_HAVE_OPTION_TABLE == 1)
>        {
>                struct lb_record *rec_dest, *rec_src;
>                /* Write the option config table... */
>                rec_dest = lb_new_record(head);
>                rec_src = (struct lb_record *)(void *)&option_table;
>                memcpy(rec_dest,  rec_src, rec_src->size);
>                /* Create cmos checksum entry in coreboot table */
>                lb_cmos_checksum(head);
>        }
> #endif
>
> from option_table.c:
> unsigned char option_table[] = {
>        0xc8,0x00,0x00,0x00,0x88,0x04,0x00,0x00,0x0c,0x00,
> ...
>
> from src/include/boot/coreboot_tables.h:
> struct lb_record {
>        uint32_t tag;           /* tag ID */
>        uint32_t size;          /* size of record (in bytes) */
> };
>
> Why are we casting it as a lb_record * then using that size field in the copy?
>
> Thanks,
> Myles

[Myles Watson]

On Wed, Nov 11, 2009 at 1:50 PM, ron minnich  wrote:
> It's not my favorite piece of code.

:)
> > unsigned char option_table[] = {
> >        0xc8,0x00,0x00,0x00,0x88,0x04,0x00,0x00,0x0c,0x00,
>
> Second 32 bits 0x88,0x04,0x00,0x00 is the length. We're small endian.
> So it's 0x488 or 1160 bytes. Does that match?

Yes, it matches.  There are 116 10-byte lines.
> So this struct:
> > struct lb_record {
> >        uint32_t tag;           /* tag ID */
> >        uint32_t size;          /* size of record (in bytes) */
> > };
>
> is a header of a variable length record.

Thanks.  I should have looked a little farther before I asked.  The
warning looked ominous and it wasn't obvious what the code was doing.

Myles

[Peter Stuge]

Myles Watson wrote:
> > > unsigned char option_table[] = {
> > >        0xc8,0x00,0x00,0x00,0x88,0x04,0x00,0x00,0x0c,0x00,
> >
> > Second 32 bits 0x88,0x04,0x00,0x00 is the length. We're small endian.
> > So it's 0x488 or 1160 bytes. Does that match?
>
> Yes, it matches.  There are 116 10-byte lines.

Maybe make a type with struct lb_record followed by unsigned char [],
then it is more clear what is going on.

//Peter

[ron minnich]

On Wed, Nov 11, 2009 at 1:21 PM, Peter Stuge  wrote:
> Myles Watson wrote:
> > > > unsigned char option_table[] = {
> > > >        0xc8,0x00,0x00,0x00,0x88,0x04,0x00,0x00,0x0c,0x00,
> > >
> > > Second 32 bits 0x88,0x04,0x00,0x00 is the length. We're small endian.
> > > So it's 0x488 or 1160 bytes. Does that match?
> >
> > Yes, it matches.  There are 116 10-byte lines.
>
> Maybe make a type with struct lb_record followed by unsigned char [],
> then it is more clear what is going on.

yes, when this code was written, gcc 2.95 days, that type of thing did
not actually work; no problem now.

ron

[Stefan Reinauer]

Peter Stuge wrote:
> Myles Watson wrote:
> > > > unsigned char option_table[] = {
> > > >        0xc8,0x00,0x00,0x00,0x88,0x04,0x00,0x00,0x0c,0x00,
> > >
> > > Second 32 bits 0x88,0x04,0x00,0x00 is the length. We're small endian.
> > > So it's 0x488 or 1160 bytes. Does that match?
> >
> > Yes, it matches.  There are 116 10-byte lines.
>
> Maybe make a type with struct lb_record followed by unsigned char [],
> then it is more clear what is going on.

Or just get the length via read32.

[Peter Stuge]

Stefan Reinauer wrote:
> > Maybe make a type with struct lb_record followed by unsigned char [],
> > then it is more clear what is going on.
>
> Or just get the length via read32.

I was thinking about how the source code looks, not so much what
happens at run time. But that could also be an improvement!

//Peter

[Myles Watson]

On Wed, Nov 11, 2009 at 3:21 PM, Peter Stuge  wrote:
> Stefan Reinauer wrote:
> > > Maybe make a type with struct lb_record followed by unsigned char [],
> > > then it is more clear what is going on.
> >
> > Or just get the length via read32.
>
> I was thinking about how the source code looks, not so much what
> happens at run time. But that could also be an improvement!

How about this:

Index: src/arch/i386/boot/coreboot_table.c
===================================================================
--- src/arch/i386/boot/coreboot_table.c	(revision 4931)
+++ src/arch/i386/boot/coreboot_table.c	(working copy)
@@ -485,11 +485,10 @@

#if (CONFIG_HAVE_OPTION_TABLE == 1)
    {
-		struct lb_record *rec_dest, *rec_src;
-		/* Write the option config table... */
+		struct lb_record *rec_dest;
+		/* Copy the option config table, it's already a lb_record... */
    	rec_dest = lb_new_record(head);
-		rec_src = (struct lb_record *)(void *)&option_table;
-		memcpy(rec_dest,  rec_src, rec_src->size);
+		memcpy(rec_dest,  &option_table, sizeof(option_table));
    	/* Create cmos checksum entry in coreboot table */
    	lb_cmos_checksum(head);
    }

Since it's already formatted correctly, we don't try to do anything
special.  We just copy it into place.  I guess we could put in a check
to make sure the second u32 equals sizeof(option_table), but it seems
like we implicitly trust the tool that created it.

Boot-tested on qemu.

Signed-off-by: Myles Watson

Thanks,
Myles

[ron minnich]

On Wed, Nov 11, 2009 at 2:39 PM, Myles Watson  wrote:
> memcpy(rec_dest,  rec_src, rec_src->size);
> memcpy(rec_dest,  &option_table, sizeof(option_table));

how can this be right? rec_src->size is 1160, and sizeof(option_table) is 8?

ron

[Myles Watson]

On Wed, Nov 11, 2009 at 3:49 PM, ron minnich  wrote:
> On Wed, Nov 11, 2009 at 2:39 PM, Myles Watson  wrote:
> > memcpy(rec_dest,  rec_src, rec_src->size);
> > memcpy(rec_dest,  &option_table, sizeof(option_table));
>
> how can this be right? rec_src->size is 1160, and sizeof(option_table) is 8?

sizeof(&option_table) is 8, right?

Myles

[Myles Watson]

On Wed, Nov 11, 2009 at 3:50 PM, Myles Watson  wrote:
> On Wed, Nov 11, 2009 at 3:49 PM, ron minnich  wrote:
> > On Wed, Nov 11, 2009 at 2:39 PM, Myles Watson  wrote:
> > > memcpy(rec_dest,  rec_src, rec_src->size);
> > > memcpy(rec_dest,  &option_table, sizeof(option_table));
> >
> > how can this be right? rec_src->size is 1160, and sizeof(option_table) is 8?
>
> sizeof(&option_table) is 8, right?

No.
sizeof(&option_table) is 4
sizeof(rec_dest) is 8.

option_table is a large character array

unsigned char option_table[] = {
       0xc8,0x00,0x00,0x00,0x88,0x04,0x00,0x00,0x0c,0x00,

Thanks,
Myles

[Peter Stuge]

Myles Watson wrote:
> How about this:
> Index: src/arch/i386/boot/coreboot_table.c
>
> --- src/arch/i386/boot/coreboot_table.c	(revision 4931)
> +++ src/arch/i386/boot/coreboot_table.c	(working copy)
> @@ -485,11 +485,10 @@
>
> #if (CONFIG_HAVE_OPTION_TABLE == 1)
>    {
> struct lb_record *rec_dest, *rec_src;
> /* Write the option config table... */
> struct lb_record *rec_dest;
> /* Copy the option config table, it's already a lb_record... */
> rec_dest = lb_new_record(head);
> rec_src = (struct lb_record *)(void *)&option_table;
> memcpy(rec_dest,  rec_src, rec_src->size);
> memcpy(rec_dest,  &option_table, sizeof(option_table));

It is completely unclear to me why it is safe to write beyond the
struct lb_record (maybe it is an elaborate side-effect of the call to
lb_new_record()?) but the code did it before and this new code does
the same thing.

Acked-by: Peter Stuge

[Myles Watson]

On Wed, Nov 11, 2009 at 4:06 PM, Peter Stuge  wrote:
> Myles Watson wrote:
> > How about this:
> > Index: src/arch/i386/boot/coreboot_table.c
> >
> > --- src/arch/i386/boot/coreboot_table.c       (revision 4931)
> > +++ src/arch/i386/boot/coreboot_table.c       (working copy)
> > @@ -485,11 +485,10 @@
> >
> > #if (CONFIG_HAVE_OPTION_TABLE == 1)
> >       {
> > struct lb_record *rec_dest, *rec_src;
> > /* Write the option config table... */
> > struct lb_record *rec_dest;
> > /* Copy the option config table, it's already a lb_record... */
> >
> > rec_dest = lb_new_record(head);
> > rec_src = (struct lb_record *)(void *)&option_table;
> > memcpy(rec_dest,  rec_src, rec_src->size);
> > memcpy(rec_dest,  &option_table, sizeof(option_table));
>
> It is completely unclear to me why it is safe to write beyond the
> struct lb_record

lb_record is just the header.  The data follows it, but isn't a member
of the struct.
> (maybe it is an elaborate side-effect of the call to
> lb_new_record()?)

I think lb_new_record uses the size to find the next header location.
Is that what you meant?
> Acked-by: Peter Stuge

Rev 4935.

Thanks,
Myles

[ron minnich]

On Wed, Nov 11, 2009 at 3:31 PM, Myles Watson  wrote:
> On Wed, Nov 11, 2009 at 4:06 PM, Peter Stuge  wrote:
> > Myles Watson wrote:
> > > How about this:
> > > Index: src/arch/i386/boot/coreboot_table.c
> > >
> > > --- src/arch/i386/boot/coreboot_table.c       (revision 4931)
> > > +++ src/arch/i386/boot/coreboot_table.c       (working copy)
> > > @@ -485,11 +485,10 @@
> > >
> > > #if (CONFIG_HAVE_OPTION_TABLE == 1)
> > >       {
> > > struct lb_record *rec_dest, *rec_src;
> > > /* Write the option config table... */
> > > struct lb_record *rec_dest;
> > > /* Copy the option config table, it's already a lb_record... */
> > >
> > > rec_dest = lb_new_record(head);
> > > rec_src = (struct lb_record *)(void *)&option_table;
> > > memcpy(rec_dest,  rec_src, rec_src->size);
> > > memcpy(rec_dest,  &option_table, sizeof(option_table));
> >
> > It is completely unclear to me why it is safe to write beyond the
> > struct lb_record
>
> lb_record is just the header.  The data follows it, but isn't a member
> of the struct.
> > (maybe it is an elaborate side-effect of the call to
> > lb_new_record()?)
>
> I think lb_new_record uses the size to find the next header location.
> Is that what you meant?
> > Acked-by: Peter Stuge
>
> Rev 4935.

I am almost certain that this change:
> > >         memcpy(rec_dest,  rec_src, rec_src->size);
> > >         memcpy(rec_dest,  &option_table, sizeof(option_table));

completely changes the behavior of the code and is wrong.

I'm willing to be convinced. But sizeof(option_table) is 8 and
rec_src->size is 1160. So you
were copying the whole record and now you're copying a record header.
I don't see how this can be a good idea. Am I missing something?

It seems to me to change a compiler warning you may have broken what
the code is supposed to do.

ron

[Peter Stuge]

ron minnich wrote:
> > > >         memcpy(rec_dest,  rec_src, rec_src->size);
> > > >         memcpy(rec_dest,  &option_table, sizeof(option_table));
>
> completely changes the behavior of the code and is wrong.
>
> I'm willing to be convinced. But sizeof(option_table) is 8

How can that be?

option_table is the long unsigned char [] = { ... } in the source.

//Peter

[ron minnich]

On Wed, Nov 11, 2009 at 3:37 PM, Peter Stuge  wrote:
> ron minnich wrote:
> > > > > memcpy(rec_dest,  rec_src, rec_src->size);
> > > > > memcpy(rec_dest,  &option_table, sizeof(option_table));
> >
> > completely changes the behavior of the code and is wrong.
> >
> > I'm willing to be convinced. But sizeof(option_table) is 8
>
> How can that be?
>
> option_table is the long unsigned char [] = { ... } in the source.

you're right, I'm asleep.
we were using the size in here:

struct lb_record {
       uint32_t tag;           /* tag ID */
       uint32_t size;          /* size of record (in bytes) */
};

is this size the size of the data inclusize of the header or not?

I don't know.

Sorry for my confusion.

ron

[Myles Watson]

On Wed, Nov 11, 2009 at 4:46 PM, ron minnich  wrote:
> On Wed, Nov 11, 2009 at 3:37 PM, Peter Stuge  wrote:
> > ron minnich wrote:
> > > > > > memcpy(rec_dest,  rec_src, rec_src->size);
> > > > > > memcpy(rec_dest,  &option_table, sizeof(option_table));
> > >
> > > completely changes the behavior of the code and is wrong.
> > >
> > > I'm willing to be convinced. But sizeof(option_table) is 8
> >
> > How can that be?
> >
> > option_table is the long unsigned char [] = { ... } in the source.
>
> you're right, I'm asleep.

You're fine.  It didn't work for me as I'd expected.
> we were using the size in here:
>
> struct lb_record {
>       uint32_t tag;           /* tag ID */
>       uint32_t size;          /* size of record (in bytes) */
> };
>
> is this size the size of the data inclusize of the header or not?

Yes.  It is inclusive.

Thanks,
Myles

[Myles Watson]

On Wed, Nov 11, 2009 at 4:37 PM, Peter Stuge  wrote:
> ron minnich wrote:
> > > > > memcpy(rec_dest,  rec_src, rec_src->size);
> > > > > memcpy(rec_dest,  &option_table, sizeof(option_table));
> >
> > completely changes the behavior of the code and is wrong.
> >
> > I'm willing to be convinced. But sizeof(option_table) is 8
>
> How can that be?

Index: src/arch/i386/boot/coreboot_table.c
===================================================================
--- src/arch/i386/boot/coreboot_table.c	(revision 4935)
+++ src/arch/i386/boot/coreboot_table.c	(working copy)
@@ -488,6 +488,11 @@
    	struct lb_record *rec_dest;
    	/* Copy the option config table, it's already a lb_record... */
    	rec_dest = lb_new_record(head);
+		printk_debug("sizeof(option_table) %lx\n", sizeof(option_table));
+		printk_debug("sizeof(&option_table) %lx\n", sizeof(&option_table));
+		printk_debug("sizeof(rec_dest) %lx\n", sizeof(rec_dest));
+		printk_debug("((u32*)option_table)[0] %x (type)\n",
((u32*)&option_table)[0]);
+		printk_debug("((u32*)option_table)[1] %x (size)\n",
((u32*)&option_table)[1]);
    	memcpy(rec_dest,  &option_table, sizeof(option_table));
    	/* Create cmos checksum entry in coreboot table */
    	lb_cmos_checksum(head);

sizeof(option_table) c
sizeof(&option_table) 4
sizeof(rec_dest) 4
sizeof(*rec_dest) 8
((u32*)option_table)[0] c8 (type)
((u32*)option_table)[1] 488 (size)
memcpy(00000518, 0010d91c, c)

I have no idea why 12.  I reverted it.

We could change it to index it as a u32 array, but I thought it would be easy...

Thanks,
Myles

[Myles Watson]

> memcpy(00000518, 0010d91c, c)
>
> I have no idea why 12.  I reverted it.

It turns out that option_table is actually a struct cmos_option_table
in the header file.

Here's a better fix:

Index: src/arch/i386/boot/coreboot_table.c
===================================================================
--- src/arch/i386/boot/coreboot_table.c	(revision 4933)
+++ src/arch/i386/boot/coreboot_table.c	(working copy)
@@ -485,11 +485,12 @@

#if (CONFIG_HAVE_OPTION_TABLE == 1)
    {
-		struct lb_record *rec_dest, *rec_src;
-		/* Write the option config table... */
+		struct lb_record *rec_dest;
+		struct cmos_option_table *ct = &option_table;
+		/* Copy the option config table, it's already a lb_record... */
    	rec_dest = lb_new_record(head);
-		rec_src = (struct lb_record *)(void *)&option_table;
-		memcpy(rec_dest,  rec_src, rec_src->size);
+		printk_debug("memcpy(%p, %p, %x)\n",rec_dest,  &option_table, ct->size);
+		memcpy(rec_dest,  &option_table, ct->size);
    	/* Create cmos checksum entry in coreboot table */
    	lb_cmos_checksum(head);
    }

Here's the output, which is correct:
memcpy(00000518, 0010dc18, 488)

If it gets an ack, I'll take out the printk before committing.

Signed-off-by: Myles Watson

Thanks,
Myles

[ron minnich]

nice.

Acked-by: Ronald G. Minnich

[Myles Watson]

On Wed, Nov 11, 2009 at 5:38 PM, ron minnich  wrote:
> nice.
>
> Acked-by: Ronald G. Minnich

Rev 4937.

Thanks,
Myles

[Peter Stuge]

Myles Watson wrote:
> > It is completely unclear to me why it is safe to write beyond the
> > struct lb_record
>
> lb_record is just the header.  The data follows it, but isn't a
> member of the struct.

Right, but what checks that the data is not colliding with something
else.
> > (maybe it is an elaborate side-effect of the call to
> > lb_new_record()?)
>
> I think lb_new_record uses the size to find the next header
> location. Is that what you meant?

If it gets that far. That would be evaluated for the next record.
What determines how big a new record can be?

//Peter

[Stefan Reinauer]

Peter Stuge wrote:
> Myles Watson wrote:
> > > It is completely unclear to me why it is safe to write beyond the
> > > struct lb_record
> >
> > lb_record is just the header.  The data follows it, but isn't a
> > member of the struct.
>
> Right, but what checks that the data is not colliding with something
> else.

Like?

[Peter Stuge]

Stefan Reinauer wrote:
> > Right, but what checks that the data is not colliding with something
> > else.
>
> Like?

Like another table? Or is this the first thing written - and
everything afterwards is simply appended?

//Peter

[Stefan Reinauer]

Peter Stuge wrote:
> Stefan Reinauer wrote:
> > > Right, but what checks that the data is not colliding with something
> > > else.
> >
> > Like?
>
> Like another table? Or is this the first thing written - and
> everything afterwards is simply appended?

The coreboot table is written to a distinct piece of memory in the high
tables area. It is written sequentially, the table entries are just
appended. (Which does not necessarily mean there's no trouble, hence my
question :-)

Stefan