flashrom: coreboot ROM image file identification heuristic is broken

[stuge]

Non-coreboot ROM images are incorrectly identified as coreboot images, and arbitrary data is used in flashrom code.

The heuristic is far too simplistic, we need a proper signature in all coreboot images. The suggested fix is to add a LAR header to the ROM image, I like that too.

When an image is incorrectly identified, junk data used by flashrom typically causes flashrom to segfault.

[hailfinger]

New proposed heuristic. This should be almost foolproof.

[stuge]

Reopening, because ultimately we want to be using a LAR file or header for this info, both in v2 and especially in v3.

[anonymous]

The current version does not work on my system

gcc -Os -Wall -Werror    -c -o layout.o layout.c
cc1: warnings being treated as errors
layout.c: In function ‘show_id’:
layout.c:68: error: cast from pointer to integer of different size
layout.c:70: error: cast from pointer to integer of different size
make: *** [layout.o] Error 1

[anonymous]

Is this what the author possibly meant?

Index: layout.c
===================================================================
--- layout.c    (revision 3412)
+++ layout.c    (working copy)
@@ -65,9 +65,9 @@
         */
        if ((*walk) == 0 || ((*walk) & 0x3ff) != 0 || *walk > size ||
                *(walk - 1) > size || *(walk - 2) > size ||
-               (!isprint((const char *)(bios + size - *(walk - 1))) &&
+               (!isprint(*(const char *)(bios + size - *(walk - 1))) &&
                ((const char *)(bios + size - *(walk - 1)))) ||
-               (!isprint((const char *)(bios + size - *(walk - 2))) &&
+               (!isprint(*(const char *)(bios + size - *(walk - 2))) &&
                ((const char *)(bios + size - *(walk - 2))))) {
                printf("Flash image seems to be a legacy BIOS. Disabling checks.\n");
                mainboard_vendor = def_name;

Was this code ever tested or reviewed? It obviously never worked like that.

[stuge]

updated kludge fix for r3408: disable the coreboot image heuristic completely

[hailfinger]

Replying to anonymous:

Is this what the author possibly meant?

Not completely. The patch below is a pure warning fix, but it doesn't fix the implementation of the mechanism completely.

{{{ Index: layout.c =================================================================== --- layout.c (revision 3412) +++ layout.c (working copy) @@ -65,9 +65,9 @@ */

if ((*walk) == 0 ((*walk) & 0x3ff) != 0 *walk > size

*(walk - 1) > size *(walk - 2) > size - (!isprint((const char *)(bios + size - *(walk - 1))) && + (!isprint(*(const char *)(bios + size - *(walk - 1))) && ((const char *)(bios + size - *(walk - 1)))) - (!isprint((const char *)(bios + size - *(walk - 2))) && + (!isprint(*(const char *)(bios + size - *(walk - 2))) && ((const char *)(bios + size - *(walk - 2))))) { printf("Flash image seems to be a legacy BIOS. Disabling checks.\n"); mainboard_vendor = def_name; }}} Was this code ever tested or reviewed? It obviously never worked like that.

It compiled cleanly without warnings on my machine (gcc 4.2.1), otherwise I wouldn't have committed it.

And I would be very careful with claiming "it obviously never worked". If it was so obvious, why did you miss 50% of the pointer dereference bugs? Probably because you didn't read the code and only looked at compiler warnings.

[hailfinger]

Updated patch posted to the mailing list.

[anonymous]

Oh, it's absolutely not obvious _how_ the code works. All I said is it is obvious that that very code never _worked_ in the side cases it was supposed to handle.

[stuge]

Please don't spend more time on the current code. If it works for you, great, if it does not work for you, please apply fr.idheur.kludge.patch attached to this ticket.

I've started work on the LAR patch. Let's try that out when it's done.

[hailfinger]

Peter, the LAR patch will be difficult unless you're willing to put the ID in the top boot block.

The trick is to have both a generic coreboot marker and the ID strings in a place that's always mapped. That way, we can apply board-specific ROM enable or readout functions automatically. Other ways lead to disaster and madness.

[hailfinger]

Compilation should now be fixed.