#102: flashrom: coreboot ROM image file identification heuristic is broken

List overview All Threads
Download

newer

older

superiotool on Asus M2A-VM

Re: [coreboot] Spi flash

coreboot

2 Jul 2008 2 Jul '08

6:29 p.m.

#102: flashrom: coreboot ROM image file identification heuristic is broken -----------------------------------+---------------------------------------- Reporter: stuge | Owner: somebody Type: defect | Status: new Priority: blocker | Milestone: flashrom v1.0 Component: flashrom | Version: Keywords: rom image heuristic | Dependencies: Patchstatus: patch needs work | -----------------------------------+---------------------------------------- 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.

-- Ticket URL: http://tracker.coreboot.org/trac/coreboot/ticket/102 coreboot http://www.coreboot.org/

Show replies by date

coreboot

3 Jul 3 Jul

4:12 p.m.

#102: flashrom: coreboot ROM image file identification heuristic is broken -------------------------+-------------------------------------------------- Reporter: stuge | Owner: somebody Type: defect | Status: new Priority: blocker | Milestone: flashrom v1.0 Component: flashrom | Version: Resolution: | Keywords: rom image heuristic Dependencies: | Patchstatus: patch needs work -------------------------+--------------------------------------------------

Comment(by hailfinger):

New proposed heuristic. This should be almost foolproof.

-- Ticket URL: http://tracker.coreboot.org/trac/coreboot/ticket/102#comment:1 coreboot http://www.coreboot.org/

coreboot

4:12 p.m.

#102: flashrom: coreboot ROM image file identification heuristic is broken -------------------------+-------------------------------------------------- Reporter: stuge | Owner: somebody Type: defect | Status: new Priority: blocker | Milestone: flashrom v1.0 Component: flashrom | Version: Resolution: | Keywords: rom image heuristic Dependencies: | Patchstatus: patch needs review -------------------------+-------------------------------------------------- Changes (by hailfinger):

* patchstatus: patch needs work => patch needs review

-- Ticket URL: http://tracker.coreboot.org/trac/coreboot/ticket/102#comment:2 coreboot http://www.coreboot.org/

coreboot

4:53 p.m.

#102: flashrom: coreboot ROM image file identification heuristic is broken -------------------------+-------------------------------------------------- Reporter: stuge | Owner: somebody Type: defect | Status: closed Priority: blocker | Milestone: flashrom v1.0 Component: flashrom | Version: Resolution: fixed | Keywords: rom image heuristic Dependencies: | Patchstatus: patch has been committed -------------------------+-------------------------------------------------- Changes (by hailfinger):

* status: new => closed * patchstatus: patch needs review => patch has been committed * resolution: => fixed

-- Ticket URL: http://tracker.coreboot.org/trac/coreboot/ticket/102#comment:3 coreboot http://www.coreboot.org/

coreboot

7:38 p.m.

#102: flashrom: coreboot ROM image file identification heuristic is broken -------------------------+-------------------------------------------------- Reporter: stuge | Owner: somebody Type: defect | Status: reopened Priority: major | Milestone: flashrom v1.0 Component: flashrom | Version: Resolution: | Keywords: rom image heuristic Dependencies: | Patchstatus: there is no patch -------------------------+-------------------------------------------------- Changes (by stuge):

* priority: blocker => major * status: closed => reopened * patchstatus: patch has been committed => there is no patch * resolution: fixed =>

Comment:

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

-- Ticket URL: http://tracker.coreboot.org/trac/coreboot/ticket/102#comment:4 coreboot http://www.coreboot.org/

coreboot

4 Jul 4 Jul

2:40 p.m.

#102: flashrom: coreboot ROM image file identification heuristic is broken -------------------------+-------------------------------------------------- Reporter: stuge | Owner: somebody Type: defect | Status: reopened Priority: major | Milestone: flashrom v1.0 Component: flashrom | Version: Resolution: | Keywords: rom image heuristic Dependencies: | Patchstatus: there is no patch -------------------------+--------------------------------------------------

Comment(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 }}}

-- Ticket URL: http://tracker.coreboot.org/trac/coreboot/ticket/102#comment:5 coreboot http://www.coreboot.org/

coreboot

3:05 p.m.

#102: flashrom: coreboot ROM image file identification heuristic is broken -------------------------+-------------------------------------------------- Reporter: stuge | Owner: somebody Type: defect | Status: reopened Priority: major | Milestone: flashrom v1.0 Component: flashrom | Version: Resolution: | Keywords: rom image heuristic Dependencies: | Patchstatus: there is no patch -------------------------+--------------------------------------------------

Comment(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.

-- Ticket URL: http://tracker.coreboot.org/trac/coreboot/ticket/102#comment:6 coreboot http://www.coreboot.org/

coreboot

6:32 p.m.

#102: flashrom: coreboot ROM image file identification heuristic is broken -------------------------+-------------------------------------------------- Reporter: stuge | Owner: somebody Type: defect | Status: reopened Priority: major | Milestone: flashrom v1.0 Component: flashrom | Version: Resolution: | Keywords: rom image heuristic Dependencies: | Patchstatus: there is no patch -------------------------+--------------------------------------------------

Comment(by hailfinger):

Replying to [comment:6 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.

-- Ticket URL: http://tracker.coreboot.org/trac/coreboot/ticket/102#comment:7 coreboot http://www.coreboot.org/

coreboot

6:33 p.m.

#102: flashrom: coreboot ROM image file identification heuristic is broken -------------------------+-------------------------------------------------- Reporter: stuge | Owner: somebody Type: defect | Status: reopened Priority: major | Milestone: flashrom v1.0 Component: flashrom | Version: Resolution: | Keywords: rom image heuristic Dependencies: | Patchstatus: there is no patch -------------------------+--------------------------------------------------

Comment(by hailfinger):

Updated patch posted to the mailing list.

-- Ticket URL: http://tracker.coreboot.org/trac/coreboot/ticket/102#comment:8 coreboot http://www.coreboot.org/

coreboot

7:39 p.m.

#102: flashrom: coreboot ROM image file identification heuristic is broken -------------------------+-------------------------------------------------- Reporter: stuge | Owner: somebody Type: defect | Status: reopened Priority: major | Milestone: flashrom v1.0 Component: flashrom | Version: Resolution: | Keywords: rom image heuristic Dependencies: | Patchstatus: there is no patch -------------------------+--------------------------------------------------

Comment(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.

-- Ticket URL: http://tracker.coreboot.org/trac/coreboot/ticket/102#comment:9 coreboot http://www.coreboot.org/

coreboot

7:50 p.m.

#102: flashrom: coreboot ROM image file identification heuristic is broken -------------------------+-------------------------------------------------- Reporter: stuge | Owner: stuge Type: defect | Status: new Priority: major | Milestone: flashrom v1.0 Component: flashrom | Version: Resolution: | Keywords: rom image heuristic Dependencies: | Patchstatus: there is no patch -------------------------+-------------------------------------------------- Changes (by stuge):

* owner: somebody => stuge * status: reopened => new

Comment:

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.

-- Ticket URL: http://tracker.coreboot.org/trac/coreboot/ticket/102#comment:10 coreboot http://www.coreboot.org/

coreboot

9:19 p.m.

#102: flashrom: coreboot ROM image file identification heuristic is broken -------------------------+-------------------------------------------------- Reporter: stuge | Owner: stuge Type: defect | Status: new Priority: major | Milestone: flashrom v1.0 Component: flashrom | Version: Resolution: | Keywords: rom image heuristic Dependencies: | Patchstatus: there is no patch -------------------------+--------------------------------------------------

Comment(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.

-- Ticket URL: http://tracker.coreboot.org/trac/coreboot/ticket/102#comment:11 coreboot http://www.coreboot.org/

coreboot

11 Jul 11 Jul

2:09 a.m.

#102: flashrom: coreboot ROM image file identification heuristic is broken -------------------------+-------------------------------------------------- Reporter: stuge | Owner: stuge Type: defect | Status: new Priority: major | Milestone: flashrom v1.0 Component: flashrom | Version: Resolution: | Keywords: rom image heuristic Dependencies: | Patchstatus: there is no patch -------------------------+--------------------------------------------------

Comment(by hailfinger):

Compilation should now be fixed.

-- Ticket URL: http://tracker.coreboot.org/trac/coreboot/ticket/102#comment:12 coreboot http://www.coreboot.org/

coreboot

6 Dec 6 Dec

11:16 p.m.

#102: flashrom: coreboot ROM image file identification heuristic is broken -------------------------+-------------------------------------------------- Reporter: stuge | Owner: stuge Type: defect | Status: new Priority: major | Milestone: flashrom v1.0 Component: flashrom | Version: Resolution: | Keywords: rom image heuristic Dependencies: | Patchstatus: there is no patch -------------------------+--------------------------------------------------

Comment(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@coresystems.de

-- Ticket URL: http://tracker.coreboot.org/trac/coreboot/ticket/102#comment:13 coreboot http://www.coreboot.org/

coreboot

11:37 p.m.

#102: flashrom: coreboot ROM image file identification heuristic is broken -------------------------+-------------------------------------------------- Reporter: stuge | Owner: stuge Type: defect | Status: new Priority: major | Milestone: flashrom v1.0 Component: flashrom | Version: Resolution: | Keywords: rom image heuristic Dependencies: | Patchstatus: there is no patch -------------------------+--------------------------------------------------

Comment(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.

-- Ticket URL: http://tracker.coreboot.org/trac/coreboot/ticket/102#comment:14 coreboot http://www.coreboot.org/

coreboot

7 Dec 7 Dec

12:52 a.m.

#102: flashrom: coreboot ROM image file identification heuristic is broken -------------------------+-------------------------------------------------- Reporter: stuge | Owner: stuge Type: defect | Status: closed Priority: major | Milestone: flashrom v1.0 Component: flashrom | Version: Resolution: fixed | Keywords: rom image heuristic Dependencies: | Patchstatus: there is no patch -------------------------+-------------------------------------------------- Changes (by hailfinger):

* status: new => closed * resolution: => fixed

Comment:

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

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

-- Ticket URL: http://tracker.coreboot.org/trac/coreboot/ticket/102#comment:15 coreboot http://www.coreboot.org/

coreboot

8 Dec 8 Dec

3:18 a.m.

#102: flashrom: coreboot ROM image file identification heuristic is broken -------------------------+-------------------------------------------------- Reporter: stuge | Owner: stuge Type: defect | Status: reopened Priority: major | Milestone: flashrom v1.0 Component: flashrom | Version: Resolution: | Keywords: rom image heuristic Dependencies: | Patchstatus: there is no patch -------------------------+-------------------------------------------------- Changes (by stuge):

* status: closed => reopened * resolution: fixed =>

Comment:

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.

-- Ticket URL: http://tracker.coreboot.org/trac/coreboot/ticket/102#comment:16 coreboot http://www.coreboot.org/

coreboot

3:23 a.m.

#102: flashrom: coreboot ROM image file identification heuristic is broken -------------------------+-------------------------------------------------- Reporter: stuge | Owner: stuge Type: defect | Status: new Priority: major | Milestone: flashrom v1.0 Component: flashrom | Version: Resolution: | Keywords: rom image heuristic Dependencies: | Patchstatus: patch needs review -------------------------+-------------------------------------------------- Changes (by stuge):

* status: reopened => new * patchstatus: there is no patch => patch needs review

-- Ticket URL: http://tracker.coreboot.org/trac/coreboot/ticket/102#comment:17 coreboot http://www.coreboot.org/

Peter Stuge

3:31 a.m.

New subject: #102: flashrom: coreboot ROM image file identification heuristic is broken

coreboot wrote:

...

patchstatus: there is no patch => patch needs review

Attached.

//Peter

coreboot

3:28 a.m.

#102: flashrom: coreboot ROM image file identification heuristic is broken -------------------------+-------------------------------------------------- Reporter: stuge | Owner: stuge Type: defect | Status: assigned Priority: major | Milestone: flashrom v1.0 Component: flashrom | Version: Resolution: | Keywords: rom image heuristic Dependencies: | Patchstatus: patch needs review -------------------------+-------------------------------------------------- Changes (by stuge):

* status: new => assigned

-- Ticket URL: http://tracker.coreboot.org/trac/coreboot/ticket/102#comment:18 coreboot http://www.coreboot.org/

coreboot

2:35 p.m.

#102: flashrom: coreboot ROM image file identification heuristic is broken -------------------------+-------------------------------------------------- Reporter: stuge | Owner: stuge Type: defect | Status: closed Priority: major | Milestone: flashrom v1.0 Component: flashrom | Version: Resolution: fixed | Keywords: rom image heuristic Dependencies: | Patchstatus: patch has been committed -------------------------+-------------------------------------------------- Changes (by stepan):

* status: assigned => closed * patchstatus: patch needs review => patch has been committed * resolution: => fixed

Comment:

The check is fine. It should not be removed.

-- Ticket URL: http://tracker.coreboot.org/trac/coreboot/ticket/102#comment:19 coreboot http://www.coreboot.org/

5611

days inactive

5770

days old

coreboot@coreboot.org

20 comments

2 participants

tags (0)

participants (2)

coreboot
Peter Stuge