| Submitter | Myles Watson |
|---|---|
| Date | 2009-11-11 22:39:49 |
| Message ID | <2831fecf0911111439qccf7747te7297f5b7fce2146@mail.gmail.com> |
| Download | mbox | patch |
| Permalink | /patch/554/ |
| State | Accepted |
| Headers | show |
Comments
On Wed, Nov 11, 2009 at 2:39 PM, Myles Watson <mylesgw@gmail.com> 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
On Wed, Nov 11, 2009 at 3:49 PM, ron minnich <rminnich@gmail.com> wrote: > On Wed, Nov 11, 2009 at 2:39 PM, Myles Watson <mylesgw@gmail.com> 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
On Wed, Nov 11, 2009 at 3:50 PM, Myles Watson <mylesgw@gmail.com> wrote: > On Wed, Nov 11, 2009 at 3:49 PM, ron minnich <rminnich@gmail.com> wrote: >> On Wed, Nov 11, 2009 at 2:39 PM, Myles Watson <mylesgw@gmail.com> 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
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 <peter@stuge.se>
On Wed, Nov 11, 2009 at 4:06 PM, Peter Stuge <peter@stuge.se> 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 <peter@stuge.se> Rev 4935. Thanks, Myles
On Wed, Nov 11, 2009 at 3:31 PM, Myles Watson <mylesgw@gmail.com> wrote: > On Wed, Nov 11, 2009 at 4:06 PM, Peter Stuge <peter@stuge.se> 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 <peter@stuge.se> > 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
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
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
On Wed, Nov 11, 2009 at 3:37 PM, Peter Stuge <peter@stuge.se> 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
On Wed, Nov 11, 2009 at 4:46 PM, ron minnich <rminnich@gmail.com> wrote: > On Wed, Nov 11, 2009 at 3:37 PM, Peter Stuge <peter@stuge.se> 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
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?
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
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
Patch
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); }