Improve coreboot image detection heuristic in flashrom. It's not absolutely perfect, but the likelihood of this check to fail is 0.000000000000000000000000013 (1.3*10^-26) which is good enough for me.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Index: flashrom-tmp1/layout.c =================================================================== --- flashrom-tmp1/layout.c (Revision 3407) +++ flashrom-tmp1/layout.c (Arbeitskopie) @@ -21,6 +21,7 @@ #include <stdio.h> #include <stdlib.h> #include <string.h> +#include <ctype.h> #include <stdint.h> #include "flash.h"
@@ -57,7 +58,18 @@ walk--; }
- if ((*walk) == 0 || ((*walk) & 0x3ff) != 0) { + /* + * Check if coreboot last image size is 0 or not a multiple of 1k or + * bigger than the chip or if the pointers to vendor ID or mainboard ID + * are outside the image of if the start of ID strings are nonsensical + * (nonprintable and not \0). + */ + 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))))) { printf("Flash image seems to be a legacy BIOS. Disabling checks.\n"); mainboard_vendor = def_name; mainboard_part = def_name;
Carl-Daniel Hailfinger wrote:
Improve coreboot image detection heuristic in flashrom. It's not absolutely perfect, but the likelihood of this check to fail is 0.000000000000000000000000013 (1.3*10^-26) which is good enough for me.
I think at some point we should go to a better concept of doing things, but it seems what you are saying is that your code will probably not fail on any bios that has been developed to date, as I doubt it is more than 10^26 bioses.
Acked-by: Stefan Reinauer stepan@coresystems.de
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Index: flashrom-tmp1/layout.c
--- flashrom-tmp1/layout.c (Revision 3407) +++ flashrom-tmp1/layout.c (Arbeitskopie) @@ -21,6 +21,7 @@ #include <stdio.h> #include <stdlib.h> #include <string.h> +#include <ctype.h> #include <stdint.h> #include "flash.h"
@@ -57,7 +58,18 @@ walk--; }
- if ((*walk) == 0 || ((*walk) & 0x3ff) != 0) {
- /*
* Check if coreboot last image size is 0 or not a multiple of 1k or* bigger than the chip or if the pointers to vendor ID or mainboard ID* are outside the image of if the start of ID strings are nonsensical* (nonprintable and not \0).*/- 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))))) {
On 03.07.2008 16:37, Stefan Reinauer wrote:
Carl-Daniel Hailfinger wrote:
Improve coreboot image detection heuristic in flashrom. It's not absolutely perfect, but the likelihood of this check to fail is 0.000000000000000000000000013 (1.3*10^-26) which is good enough for me.
I think at some point we should go to a better concept of doing things, but it seems what you are saying is that your code will probably not fail on any bios that has been developed to date, as I doubt it is more than 10^26 bioses.
I calculated the likelihood of a false positive detection of coreboot for a random image. All reports we got about segfaults will be fixed as well, so I'm reasonably positive this bandaid will last.
Acked-by: Stefan Reinauer stepan@coresystems.de
Thanks, r3408.
Regards, Carl-Daniel
On Thu, Jul 03, 2008 at 04:28:54PM +0200, Carl-Daniel Hailfinger wrote:
- 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))))) {
I am in total disbelief. I should probably not be wasting any more of my time on cleanups and restructuring.
But let's discuss a technical aspect. Hhow does this new heuristic relate to v3 larballs?
//Peter
On 03.07.2008 18:46, Peter Stuge wrote:
On Thu, Jul 03, 2008 at 04:28:54PM +0200, Carl-Daniel Hailfinger wrote:
- 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))))) {I am in total disbelief. I should probably not be wasting any more of my time on cleanups and restructuring.
Well, this is the only reliable fix until somebody steps forward to implement LAR recognition for flashrom and fake LAR headers for v2 and we agree on a standard for encapsulating vendor/model information in a LAR. I don't see that happening in the next few weeks.
But let's discuss a technical aspect. Hhow does this new heuristic relate to v3 larballs?
If it worked with v3 before, it will still work.
Regards, Carl-Daniel
On Thu, Jul 03, 2008 at 07:30:40PM +0200, Carl-Daniel Hailfinger wrote:
Well, this is the only reliable fix until somebody steps forward to implement LAR recognition for flashrom and fake LAR headers for v2 and we agree on a standard for encapsulating vendor/model information in a LAR. I don't see that happening in the next few weeks.
I think that could happen. :)
Anyway, I have reopened as major, to track the LAR way.
But let's discuss a technical aspect. Hhow does this new heuristic relate to v3 larballs?
If it worked with v3 before, it will still work.
It did not. Ok, v3 wasn't considered.
//Peter
Carl-Daniel Hailfinger wrote:
On 03.07.2008 18:46, Peter Stuge wrote:
On Thu, Jul 03, 2008 at 04:28:54PM +0200, Carl-Daniel Hailfinger wrote:
- 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))))) {I am in total disbelief. I should probably not be wasting any more of my time on cleanups and restructuring.
Well, this is the only reliable fix until somebody steps forward to implement LAR recognition for flashrom and fake LAR headers for v2 and we agree on a standard for encapsulating vendor/model information in a LAR. I don't see that happening in the next few weeks.
Reliable yes, but reliably wrong, unfortunately. It checks whether a pointer is printable, which makes absolutely no sense. Shame on me I acked this.
On 04.07.2008 15:28, Stefan Reinauer wrote:
Carl-Daniel Hailfinger wrote:
On 03.07.2008 18:46, Peter Stuge wrote:
On Thu, Jul 03, 2008 at 04:28:54PM +0200, Carl-Daniel Hailfinger wrote:
- 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))))) {I am in total disbelief. I should probably not be wasting any more of my time on cleanups and restructuring.
Well, this is the only reliable fix until somebody steps forward to implement LAR recognition for flashrom and fake LAR headers for v2 and we agree on a standard for encapsulating vendor/model information in a LAR. I don't see that happening in the next few weeks.
Reliable yes, but reliably wrong, unfortunately. It checks whether a pointer is printable, which makes absolutely no sense. Shame on me I acked this.
Sorry. I had a correct version, but it seems I pressed undo before saving.
Fix coreboot image detection heuristic.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Index: flashrom-tmp1/layout.c =================================================================== --- flashrom-tmp1/layout.c (Revision 3412) +++ flashrom-tmp1/layout.c (Arbeitskopie) @@ -65,10 +65,10 @@ */ 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))))) { + (!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))))) { printf("Flash image seems to be a legacy BIOS. Disabling checks.\n"); mainboard_vendor = def_name; mainboard_part = def_name;
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.
//Peter
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;
Carl-Daniel Hailfinger wrote:
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
Makes sense due to -Werror ;-)
Carl-Daniel Hailfinger wrote:
Sorry. I had a correct version, but it seems I pressed undo before saving.
Fix coreboot image detection heuristic.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
I wonder whether it makes sense to explicitly say !=0 for the non-isprint parts to express that those are not booleans but checks whether a char is 0 I'm still not sure this really shows the correct behavior, but I guess it will compile on 64bit systems like this.
Acked-by: Stefan Reinauer stepan@coresystems.de
Index: flashrom-tmp1/layout.c
--- flashrom-tmp1/layout.c (Revision 3412) +++ flashrom-tmp1/layout.c (Arbeitskopie) @@ -65,10 +65,10 @@ */ 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))))) {
(!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))))) {
On 04.07.2008 19:17, Stefan Reinauer wrote:
Carl-Daniel Hailfinger wrote:
Sorry. I had a correct version, but it seems I pressed undo before saving.
Fix coreboot image detection heuristic.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
I wonder whether it makes sense to explicitly say !=0 for the non-isprint parts to express that those are not booleans but checks whether a char is 0
I came to the conclusion that matching a possibly empty string in the ROM does not make sense.
Cleaned up patch follows. It compiles for me, is a LOT more readable and even removes hard-to-follow code from layout.c.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Index: layout.c =================================================================== --- layout.c (Revision 3412) +++ layout.c (Arbeitskopie) @@ -45,7 +45,12 @@ int show_id(uint8_t *bios, int size, int force) { unsigned int *walk; + unsigned int mb_part_offset, mb_vendor_offset; + char *mb_part, *mb_vendor;
+ mainboard_vendor = def_name; + mainboard_part = def_name; + walk = (unsigned int *)(bios + size - 0x10); walk--;
@@ -63,25 +68,27 @@ * are outside the image of if the start of ID strings are nonsensical * (nonprintable and not \0). */ - 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 = *(walk - 1); + mb_vendor_offset = *(walk - 2); + if ((*walk) == 0 || ((*walk) & 0x3ff) != 0 || (*walk) > size || + mb_part_offset > size || mb_vendor_offset > size) { printf("Flash image seems to be a legacy BIOS. Disabling checks.\n"); - mainboard_vendor = def_name; - mainboard_part = def_name; return 0; } + + mb_part = (char *)(bios + size - mb_part_offset); + mb_vendor = (char *)(bios + size - mb_vendor_offset); + if (!isprint(*mb_part) || !isprint(*mb_vendor)) { + printf("Flash image seems to have garbage in the ID location." + " Disabling checks.\n"); + return 0; + }
printf_debug("coreboot last image size " "(not ROM size) is %d bytes.\n", *walk);
- walk--; - mainboard_part = strdup((const char *)(bios + size - *walk)); - walk--; - mainboard_vendor = strdup((const char *)(bios + size - *walk)); + mainboard_part = strdup(mb_part); + mainboard_vendor = strdup(mb_vendor); printf_debug("Manufacturer: %s\n", mainboard_vendor); printf_debug("Mainboard ID: %s\n", mainboard_part);
On 04.07.2008 21:00, Carl-Daniel Hailfinger wrote:
On 04.07.2008 19:17, Stefan Reinauer wrote:
Carl-Daniel Hailfinger wrote:
Sorry. I had a correct version, but it seems I pressed undo before saving.
Fix coreboot image detection heuristic.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
I wonder whether it makes sense to explicitly say !=0 for the non-isprint parts to express that those are not booleans but checks whether a char is 0
I came to the conclusion that matching a possibly empty string in the ROM does not make sense.
Cleaned up patch follows. It compiles for me, is a LOT more readable and even removes hard-to-follow code from layout.c.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
And this one has an additional compile fix for NetBSD. Thanks to Peter for suggesting a fix and Jonathan for testing. It was sort of Acked by Stefan.
Regards, Carl-Daniel
Index: flashrom-tmp1/layout.c =================================================================== --- flashrom-tmp1/layout.c (Revision 3419) +++ flashrom-tmp1/layout.c (Arbeitskopie) @@ -45,7 +45,12 @@ int show_id(uint8_t *bios, int size, int force) { unsigned int *walk; + unsigned int mb_part_offset, mb_vendor_offset; + char *mb_part, *mb_vendor;
+ mainboard_vendor = def_name; + mainboard_part = def_name; + walk = (unsigned int *)(bios + size - 0x10); walk--;
@@ -63,25 +68,28 @@ * are outside the image of if the start of ID strings are nonsensical * (nonprintable and not \0). */ - 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 = *(walk - 1); + mb_vendor_offset = *(walk - 2); + if ((*walk) == 0 || ((*walk) & 0x3ff) != 0 || (*walk) > size || + mb_part_offset > size || mb_vendor_offset > size) { printf("Flash image seems to be a legacy BIOS. Disabling checks.\n"); - mainboard_vendor = def_name; - mainboard_part = def_name; return 0; } + + mb_part = (char *)(bios + size - mb_part_offset); + mb_vendor = (char *)(bios + size - mb_vendor_offset); + if (!isprint((unsigned char)*mb_part) || + !isprint((unsigned char)*mb_vendor)) { + printf("Flash image seems to have garbage in the ID location." + " Disabling checks.\n"); + return 0; + }
printf_debug("coreboot last image size " "(not ROM size) is %d bytes.\n", *walk);
- walk--; - mainboard_part = strdup((const char *)(bios + size - *walk)); - walk--; - mainboard_vendor = strdup((const char *)(bios + size - *walk)); + mainboard_part = strdup(mb_part); + mainboard_vendor = strdup(mb_vendor); printf_debug("Manufacturer: %s\n", mainboard_vendor); printf_debug("Mainboard ID: %s\n", mainboard_part);
Carl-Daniel Hailfinger wrote:
But let's discuss a technical aspect. Hhow does this new heuristic relate to v3 larballs?
If it worked with v3 before, it will still work.
The mechanism does not work for v3 at all. It never did.
-
Carl-Daniel Hailfinger -
Peter Stuge -
Stefan Reinauer