[coreboot] [PATCH] flashrom: Improve coreboot image detection heuristic

Carl-Daniel Hailfinger c-d.hailfinger.devel.2006 at gmx.net
Fri Jul 4 18:51:00 CEST 2008


On 04.07.2008 18:25, Peter Stuge wrote:
> On Fri, Jul 04, 2008 at 06:11:54PM +0200, Carl-Daniel Hailfinger wrote:
>   
>> +		(!isprint(*(const char *)(bios + size - *(walk - 1))) &&
>> +		(*(const char *)(bios + size - *(walk - 1)))) ||
>> +		(!isprint(*(const char *)(bios + size - *(walk - 2))) &&
>> +		(*(const char *)(bios + size - *(walk - 2))))) {
>>     
>
> NAK. I would like to fix this in a better way, or not at all. A four
> line long condition can simply not be the best way, even in the short
> term.
>   

Hm. I could make that a lot more readable. How about this? By the way,
the whole show_id function is and was completely broken when compiled
for 64bit.

Index: flashrom-tmp1/layout.c
===================================================================
--- flashrom-tmp1/layout.c	(Revision 3412)
+++ flashrom-tmp1/layout.c	(Arbeitskopie)
@@ -44,7 +44,9 @@
 
 int show_id(uint8_t *bios, int size, int force)
 {
+#warning This code is completely broken on 64bit
 	unsigned int *walk;
+	unsigned int mb_part_offset, mb_vendor_offset;
 
 	walk = (unsigned int *)(bios + size - 0x10);
 	walk--;
@@ -63,12 +65,14 @@
 	 * are outside the image of if the start of ID strings are nonsensical
 	 * (nonprintable and not \0).
 	 */
+	mb_part_offset = *(walk - 1);
+	mb_vendor_offset = *(walk - 2);
 	if ((*walk) == 0 || ((*walk) & 0x3ff) != 0 || *walk > size ||
-		*(walk - 1) > size || *(walk - 2) > size ||
-		(!isprint((const char *)(bios + size - *(walk - 1))) &&
-		((const char *)(bios + size - *(walk - 1)))) ||
-		(!isprint((const char *)(bios + size - *(walk - 2))) &&
-		((const char *)(bios + size - *(walk - 2))))) {
+		mb_part_offset > size || mb_vendor_offset > size ||
+		(!isprint(*(const char *)(bios + size - mb_part_offset)) &&
+		(*(const char *)(bios + size - mb_part_offset))) ||
+		(!isprint(*(const char *)(bios + size - mb_vendor_offset)) &&
+		(*(const char *)(bios + size - mb_vendor_offset)))) {
 		printf("Flash image seems to be a legacy BIOS. Disabling checks.\n");
 		mainboard_vendor = def_name;
 		mainboard_part = def_name;


-- 
http://www.hailfinger.org/





More information about the coreboot mailing list