On Thu, 14 Jul 2011 01:17:50 +0200 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
Am 12.06.2011 09:41 schrieb Stefan Tauner:
On Sun, 12 Jun 2011 02:15:34 +0200 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net 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...