Ticket #102 (new defect)

Opened 3 months ago

Last modified 3 months ago

flashrom: coreboot ROM image file identification heuristic is broken

Reported by: stuge Owned by: stuge
Priority: major Milestone: flashrom v1.0
Component: flashrom Version:
Keywords: rom image heuristic Cc:
Dependencies: Patch Status: there is no patch

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

flashrom_layout_better_coreboot_heuristic.diff (1.1 kB) - added by hailfinger 3 months ago.
fr.idheur.kludge.patch (0.9 kB) - added by stuge 3 months ago.
updated kludge fix for r3408: disable the coreboot image heuristic completely

Change History

Changed 3 months ago by hailfinger

  Changed 3 months ago by hailfinger

New proposed heuristic. This should be almost foolproof.

  Changed 3 months ago by hailfinger

  • patchstatus changed from patch needs work to patch needs review

  Changed 3 months ago by hailfinger

  • status changed from new to closed
  • patchstatus changed from patch needs review to patch has been committed
  • resolution set to fixed

  Changed 3 months ago by stuge

  • priority changed from blocker to major
  • status changed from closed to reopened
  • patchstatus changed from patch has been committed to there is no patch
  • resolution fixed deleted

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

  Changed 3 months 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

follow-up: ↓ 7   Changed 3 months 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 3 months ago by stuge

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

in reply to: ↑ 6   Changed 3 months 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.

  Changed 3 months ago by hailfinger

Updated patch posted to the mailing list.

  Changed 3 months 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.

  Changed 3 months 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.

  Changed 3 months 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.

  Changed 3 months ago by hailfinger

Compilation should now be fixed.

Add/Change #102 (flashrom: coreboot ROM image file identification heuristic is broken)

Author



Action
as new
 
Note: See TracTickets for help on using tickets.