Modify

Opened 7 years ago

Closed 6 years ago

#102 closed defect (fixed)

flashrom: coreboot ROM image file identification heuristic is broken

Reported by: stuge Owned by: stuge
Priority: major Milestone: flashrom v1.0
Component: flashrom (please use trac on flashrom.org) Keywords: rom image heuristic
Cc: Dependencies:
Patch Status: patch has been committed

Description

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.

Attachments (3)

flashrom_layout_better_coreboot_heuristic.diff (1.1 KB) - added by hailfinger 7 years ago.
fr.idheur.kludge.patch (915 bytes) - added by stuge 7 years ago.
updated kludge fix for r3408: disable the coreboot image heuristic completely
fr.idheur.remove.patch (4.0 KB) - added by stuge 6 years ago.
Remove flash image identification heuristic

Download all attachments as: .zip

Change History (22)

Changed 7 years ago by hailfinger

comment:1 Changed 7 years ago by hailfinger

New proposed heuristic. This should be almost foolproof.

comment:2 Changed 7 years ago by hailfinger

  • Patch Status changed from patch needs work to patch needs review

comment:3 Changed 7 years ago by hailfinger

  • Patch Status changed from patch needs review to patch has been committed
  • Resolution set to fixed
  • Status changed from new to closed

comment:4 Changed 7 years ago by stuge

  • Patch Status changed from patch has been committed to there is no patch
  • Priority changed from blocker to major
  • Resolution fixed deleted
  • Status changed from closed to reopened

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

comment:5 Changed 7 years ago by 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

comment:6 follow-up: Changed 7 years ago by 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.

Changed 7 years ago by stuge

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

comment:7 in reply to: ↑ 6 Changed 7 years ago by 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.

comment:8 Changed 7 years ago by hailfinger

Updated patch posted to the mailing list.

comment:9 Changed 7 years ago by 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.

comment:10 Changed 7 years ago by stuge

  • Owner changed from somebody to stuge
  • Status changed from reopened to new

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.

comment:11 Changed 7 years ago by 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.

comment:12 Changed 7 years ago by hailfinger

Compilation should now be fixed.

comment:13 Changed 6 years ago by 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@…>

comment:14 Changed 6 years ago by 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.

comment:15 Changed 6 years ago by hailfinger

  • Resolution set to fixed
  • Status changed from new to closed

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

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

comment:16 Changed 6 years ago by stuge

  • Resolution fixed deleted
  • Status changed from closed to reopened

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.

Changed 6 years ago by stuge

Remove flash image identification heuristic

comment:17 Changed 6 years ago by stuge

  • Patch Status changed from there is no patch to patch needs review
  • Status changed from reopened to new

comment:18 Changed 6 years ago by stuge

  • Status changed from new to assigned

comment:19 Changed 6 years ago by stepan

  • Patch Status changed from patch needs review to patch has been committed
  • Resolution set to fixed
  • Status changed from assigned to closed

The check is fine. It should not be removed.

Add Comment

Modify Ticket

Action
as closed The owner will remain stuge.
The resolution will be deleted. Next status will be 'reopened'.
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.