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.

[stepan]

Let's get this out of the door.

Carl-Daniel: Is the code tested on a couple of coreboot and non-coreboot images?
Especially it should be tested with ck804 or mcp55 coreboot images.

If so, this is
Acked-by: Stefan Reinauer <stepan@…>

[stuge]

Hold on, this is a bit tricky.

This is about flashrom trying to identify the image it is about to flash. After identification it checks the vendor/board information in the image against that in a coreboot table, and will warn the user if the image seems unsuitable for the board flashrom is running on, and require -f to override a mismatch.

The fact is that we are having a very hard time coming up with a test which determines whether an image is coreboot or not to begin with, and we're also having some difficulties with the technical details of how to store image metadata in the image itself.

I suggest that we completely remove the image detection for 1.0 and add the feature back in at a later time when we actually have something close to a good solution. We have already spent far too much effort on this problem, and it's sole purpose is to warn users when they are crossflashing on a board which is running coreboot. I don't think we need this.

[hailfinger]

An alternative patch was committed in r3420 which fixes this bug.

To be honest, having a better signature in coreboot images would help.

[stuge]

I still think we should remove this check.

One benefit of flashrom is that it permits hassle free crossflashing. I guess the crossflashing will often be factory BIOS images, in which case this check does not run. Only when the user booted coreboot and wants to flash another coreboot image will flashrom suddenly require -f to perform the requested operation.

Please comment.

[stuge]

Remove flash image identification heuristic

[stepan]

The check is fine. It should not be removed.