On Thu, 14 Jul 2011 01:17:50 +0200 Carl-Daniel Hailfinger <[email protected]> wrote:
> Am 12.06.2011 09:41 schrieb Stefan Tauner: > > On Sun, 12 Jun 2011 02:15:34 +0200 > > Carl-Daniel Hailfinger <[email protected]> wrote: > > > > > >> Am 08.06.2011 04:55 schrieb Stefan Tauner: > >> > >>> +void pretty_print_ich_descriptor_component(void) > >>> +{ > >>> + const char * str_freq[8] = { > >>> + "20 MHz", /* 000 */ > >>> + "33 MHz", /* 001 */ > >>> + "reserved/illegal", /* 010 */ > >>> + "reserved/illegal", /* 011 */ > >>> + "50 MHz", /* 100 */ > >>> + "reserved/illegal", /* 101 */ > >>> + "reserved/illegal", /* 110 */ > >>> + "reserved/illegal" /* 111 */ > >>> + }; > >>> + const char * str_mem[8] = { > >>> + "512kB", > >>> + "1 MB", > >>> + "2 MB", > >>> + "4 MB", > >>> + "8 MB", > >>> + "16 MB", > >>> + "undocumented/illegal", > >>> + "reserved/illegal" > >>> + }; > >>> + > >>> + msg_pdbg("\n"); > >>> + msg_pdbg("=== FCBA ===\n"); > >>> + msg_pdbg("FLCOMP 0x%8.8x\n", fcba.FLCOMP); > >>> + msg_pdbg("FLILL 0x%8.8x\n", fcba.FLILL ); > >>> + msg_pdbg("\n"); > >>> + msg_pdbg("--- FCBA details ---\n"); > >>> + msg_pdbg("0x%2.2x freq_read_id %s\n", > >>> + fcba.freq_read_id , str_freq[fcba.freq_read_id ]); > >>> > >>> > >> Out-of-bounds access if fcba.freq_read_id>=8 > >> > > hm. well if that happens then the compiler is REALLY doing something > > unexpected because .freq_read_id is defined to be 3 (unsigned) bits > > wide. > > > > Hm yes. A comment (/* freq_read_id has 3 bits */) or something to that > effect would be nice and improve code auditability (no need to look up > stuff elsewhere). after the changes i have done i am no longer sure this is a good idea. they dont change anything directly related, but commenting this seems very redundant if you look at the patch as a whole. it would be required in quite some places although the definition is only one click away. this is quite easier to review than for example do_ich9_spi_frap, which uses an argument to index into similar arrays and does no check whatsoever. if you insist though, i can add them anyway. > Can we add some selftest during flashrom startup which checks for one or > two special hand-made similar structs if it worked and complains about a > compiler problem otherwise? That would allow using structs with all > benefits and without the risk. > > To be honest, I had hoped some C expert would chime in and lecture us > about how to do this correctly. > Michael? i could use the structs we introduce with this patch in the startup checking code. i would do two tests: writing to big fields and verify that reading out the matching smaller fields in the same range are correct, and vice-versa. we could also add such a check to the makefile tests, which is probably better because it is a compile-time/compiler check not a runtime check? side note: doesnt that apply to other startup checks too? > > > in some cases we should certainly check the values and this is in the > > case of redirection; i.e. when we look up an offset to another field in > > some descriptor register which could potentially be garbage. > > > > Indeed. done. this was quite some work to get the checked boundaries correct and tied. now i know why there are so many overflow errors out there... not just because everyone is lazy. :) > >>> + } > >>> + /* straps */ > >>> + /* FIXME: detect chipset correctly */ > >>> + switch (CHIPSET_SERIES_5_IBEX_PEAK) { > >>> > >>> > >> WTF? > >> > > yes. :) > > there is afaik no way to distinguish between the chipsets from the > > descriptor alone. this is not a big problem for flashrom. it can deduce > > this from the pci ids of the system. but for the stand alone utility it > > is a problem. > > we could add pci id detection directly here but this would make it > > necessary to either include flashrom's pci stuff (which - from a quick > > look - is not as good modularized as i would like it be for this case, > > i have not tried yet to use it though) or to force the user to supply > > the chipset to use (maybe with a safe - if there is one - default). > > > > Parsing a file should be hardware-independent. Heck, if someone wants to > use a non-PCI ARM machine that should still work fine. Requiring a > command line argument is absolutely fine. Use the enum below, and inside > flashrom map PCI IDs to that enum, and in the separate tool map command > line parameters to that enum. inside flashrom we don't need to know (yet), so there is no problem... i have added a parameter to the tool, that selects the chipset. > > regarding packing: > > > >> If enough space remains, a bit-field that immediately follows another > >> bit-field in a structure shall be packed into adjacent bits of the same > >> unit. > >> If insufficient space remains, whether a bit-field that does not fit is > >> put into the next unit or overlaps adjacent units is > >> implementation-defined. > >> > > so i think if i use uint32_t for all (overlapping) fields it should be > > safe? > > > > As long as a field does not span two different uint32_t, it should be > fine, yes. fixed. > >> Why an empty struct? > >> > > not done yet. i wanted feedback first ;) > > this information is not available publicly btw. > > apart from what mazzoo has supplied, i do only have the data for ibex > > peak/5 series. > > > > Oh, that's unfortunate. I should ask my Intel contacts if there is > anything they might be able to share legally. Can you ping me about that > in a few days (I hope to find the right people by then)? ping! :) also what about the authors of the MEI kernel module? i have not messaged them because i presume they won't be allowed to help us anyway and did not want to bother them. but since i have written that letter/blog thingy now i could just spam them anyway. i would like to hear your opinion on this before i do so because you brought it up lately yourself... -- Kind regards/Mit freundlichen Grüßen, Stefan Tauner _______________________________________________ flashrom mailing list [email protected] http://www.flashrom.org/mailman/listinfo/flashrom
