Re: [flashrom] [RFC] Kill --force

[Carl-Daniel Hailfinger]

On 22.04.2010 16:09, Michael Karcher wrote:
> Am Dienstag, den 13.04.2010, 04:16 +0200 schrieb Carl-Daniel Hailfinger:
> > First step:
> > Remove any suggestions to use --force for probe/read from flashrom
> > output.
>
> Good idea, best would be to have dummy chips added. So chips that can't
> be probed, written or erased, but read by mmap. Like dummy512k and so
> on.

Hm yes. The big problem is handling programmers which only support SPI.
We can't use a mmap dummy there. I have to think about this again.
> > if (chip_to_probe) {
> > for (flash = flashchips; flash && flash->name; flash++)
> > 	if (!strcmp(flash->name, chip_to_probe))
> > 		break;
> > if (!flash || !flash->name) {
> > 	fprintf(stderr, "Error: Unknown chip specified.\n");
> > 	exit(1);
> > }
> > /* Clean up after the check. */
> > flash = NULL;
> > }
>
> do we need this new code block...
> > if (force && read_it && chip_to_probe) {
> > 	printf("Force read (-f -r -c) requested, forcing chip probe success:\n");
> > 	printf("Force read (-f -r -c) requested, pretending the chip is there:\n");
>
> good message change.
> > 	flashes[0] = probe_flash(flashchips, 1);
> > 	if (!flashes[0]) {
> > 		printf("flashrom does not support a flash chip named '%s'.\n", chip_to_probe);
>
> if we have this?

Yes. This message is only triggered if you explicitly force read, and as
such the message is misplaced. I've removed the message here.
> > printf("Found chip "%s %s" (%d KB, %s) at physical address 0x%lx.\n",
> > printf("%s chip "%s %s" (%d KB, %s) at physical address 0x%lx.\n",
> >       force ? "Assuming" : "Found",
>
> Assuming -> Forced?

"Assuming" implies guesswork, "Forced" seems to suggest some success.
> > if (is_laptop) {
> >
> > [..]
> > if (force_laptop) {
> > 	msg_perr("Proceeding anyway because user specified "
> > 		 "laptop=force_I_want_a_brick.\n");
> > } else {
> > 	msg_perr("Aborting.\n");
> > 	exit(1);
> > }
> > }
>
> I understand your motivation of not documenting the force parameter
> here. But I don't like the "security by obscurity" approach in general.
> If we provide the forcing parameter, it should be documented somewhere.
> Leave the undocumented stuff to closed-source software.

OK,
> No real showstoppers, so
>
> Acked-by: Michael Karcher

Thank you for the review and the ack!

Here is a reworked version which incorporates your comments (except the
dummy chip stuff, I still need to come up with something good for those).

--force may have been a good idea back when only developers were using
flashrom, but over the last few months I've seen too many people who
incorrectly believed that --force would solve anything.

One of the problems is that --force has multiple meanings:
- Force chip read by faking probe success.
- Force chip access even if the chip is bigger than max decode size for
  the flash bus.
- Force erase even if erase is known bad.
- Force write even if write is known bad.
- Force writing even if cbtable tells us that this is the wrong image
  for this board.

We should kill --force and replace it with explicit --force-erase or
--force-chip or similar stuff like -p internal:boardmismatch=force.

First step:
- Remove any suggestions to use --force for probe/read from flashrom
  output.
- Don't talk about "success" or "Found chip" if the chip is forced.
- Add a new internal programmer parameter boardmismatch=force. This
  overrides any mismatch detection from cbtable/image comparisons.
- Add a new internal programmer parameter laptop=force_I_want_a_brick.
- Adjust the documentation for --force.
- Clean up the man page a bit whereever it talks about --force or
  laptops.

Additional changes in this patch:
- Add warnings about laptops to the documentation.
- Abort if a laptop is detected. Can be overridden with the programmer
parameter mentioned above.
- Add "Portable" to the list of DMI strings indicating laptops.
- Check if a chip specified with -c is known to flashrom.
- Programmer parameter reliability and consistency fixes.
- More paranoid self-checks.
- Improve documentation.

Signed-off-by: Carl-Daniel Hailfinger

Index: flashrom-forced_stupid/flashrom.8
===================================================================
--- flashrom-forced_stupid/flashrom.8	(Revision 994)
+++ flashrom-forced_stupid/flashrom.8	(Arbeitskopie)
@@ -61,7 +61,10 @@
 More verbose output.
 .TP
 .B "-c, --chip" <chipname>
-Probe only for specified flash ROM chip.
+Probe only for specified flash ROM chip. This option takes the chip name as
+printed by
+.B "flashrom -L"
+without the vendor name. Please note that the chip name is case sensitive.
 .TP
 .B "-m, --mainboard" <[vendor:]part>
 Override mainboard settings.
@@ -77,11 +80,16 @@
 coreboot table is found.
 .TP
 .B "-f, --force"
-Force write without checking whether the ROM image file is really meant
-to be used on this board.
+Force one or more of the following actions:
 .sp
-Note: This check only works while coreboot is running, and only for those
-boards where the coreboot code supports it.
+* Force chip read and pretend the chip is there.
+.sp
+* Force chip access even if the chip is bigger than max decode size for\
+ the flash bus.
+.sp
+* Force erase even if erase is known bad.
+.sp
+* Force write even if write is known bad.
 .TP
 .B "-l, --layout <file>"
 Read ROM layout from
@@ -135,7 +143,7 @@
 but outputs the supported hardware in MediaWiki syntax, so that it can be
 easily pasted into the wiki page at http://www.flashrom.org/.
 .TP
-.B "-p, --programmer <name>[:parameters]"
+.B "-p, --programmer <name>[:parameter[,parameter[,parameter]]]"
 Specify the programmer device. Currently supported are:
 .sp
 .BR "* internal" " (default, for in-system flashing in the mainboard)"
@@ -220,6 +228,13 @@
 enable is going to fail. In any case (success or failure), please report to
 the flashrom mailing list, see below.
 .sp
+On systems running coreboot, flashrom checks whether the desired image matches
+your mainboard. This needs some special board ID to be present in the image.
+If flashrom detects that the image you want to write and the current board
+do not match, it will refuse to write the image unless you specify
+.sp
+.B "flashrom -p internal:boardmismatch=force"
+.sp
 If your mainboard uses an ITE IT87 series Super I/O for LPC<->SPI flash bus
 translation, flashrom should autodetect that configuration. You can use
 .B "flashrom -p internal:it87spiport=portnum"
@@ -228,6 +243,24 @@
 programmer section to use a non-default port for controlling the IT87 series
 Super I/O. In the unlikely case flashrom doesn't detect an active IT87 LPC<->SPI
 bridge, you can try to force recognition by using the it87spi programmer.
+.sp
+Using flashrom on laptops is dangerous and may easily make your hardware
+unusable (see also the BUGS section).  The embedded controller (EC) in these
+machines often interacts badly with flashing. http://www.flashrom.org/Laptops
+has more information. If flash is shared with the EC, erase is guaranteed to
+brick your laptop and write is very likely to brick your laptop.
+Chip read and probe may irritate your EC and cause fan failure, backlight
+failure, sudden poweroff, and other nasty effects.
+flashrom will attempt to detect laptops and abort immediately for safety
+reasons.
+If you want to proceed anyway at your own risk, use
+.sp
+.B "flashrom -p internal:laptop=force_I_want_a_brick"
+.sp
+You have been warned.
+.sp
+We will not help you if you force flashing on a laptop because this is a really
+dumb idea.
 .TP
 .BR "dummy " programmer
 An optional parameter specifies the bus types it
@@ -315,8 +348,14 @@
 .SH BUGS
 Please report any bugs at
 .BR http://www.flashrom.org/trac/flashrom/newticket ","
-or on the flashrom mailing list
-.RB "(" http://www.flashrom.org/mailman/listinfo/flashrom ")."
+or on the flashrom mailing list at
+.BR http://www.flashrom.org/mailman/listinfo/flashrom "."
+.sp
+Using flashrom on laptops is dangerous and may easily make your hardware
+unusable unless you can desolder the flash chip and have a full flash chip
+backup. This is caused by the embedded controller (EC) present in many laptops,
+which interacts badly with any flash attempts. This is a hardware limitation
+and flashrom will attempt to detect it and abort immediately for safety reasons.
 .SH LICENCE
 .B flashrom
 is covered by the GNU General Public License (GPL), version 2. Some files are
Index: flashrom-forced_stupid/flash.h
===================================================================
--- flashrom-forced_stupid/flash.h	(Revision 994)
+++ flashrom-forced_stupid/flash.h	(Arbeitskopie)
@@ -385,6 +385,7 @@
 #if INTERNAL_SUPPORT == 1
 extern int is_laptop;
 extern int force_boardenable;
+extern int force_boardmismatch;
 void probe_superio(void);
 int internal_init(void);
 int internal_shutdown(void);
Index: flashrom-forced_stupid/cli_classic.c
===================================================================
--- flashrom-forced_stupid/cli_classic.c	(Revision 994)
+++ flashrom-forced_stupid/cli_classic.c	(Arbeitskopie)
@@ -248,6 +248,10 @@
    				switch (optarg[namelen]) {
    				case ':':
    					programmer_param = strdup(optarg + namelen + 1);
+						if (!strlen(programmer_param)) {
+							free(programmer_param);
+							programmer_param = NULL;
+						}
    					break;
    				case '\0':
    					break;
@@ -303,6 +307,21 @@
    	cli_classic_usage(argv[0]);
    }

+	if (chip_to_probe) {
+		for (flash = flashchips; flash && flash->name; flash++)
+			if (!strcmp(flash->name, chip_to_probe))
+				break;
+		if (!flash || !flash->name) {
+			fprintf(stderr, "Error: Unknown chip '%s' specified.\n",
+				chip_to_probe);
+			printf("Run flashrom -L to view the hardware supported "
+				"in this flashrom version.\n");
+			exit(1);
+		}
+		/* Clean up after the check. */
+		flash = NULL;
+	}
+		
    if (programmer_init()) {
    	fprintf(stderr, "Error: Programmer initialization failed.\n");
    	exit(1);
@@ -329,18 +348,14 @@
    } else if (!flashes[0]) {
    	printf("No EEPROM/flash device found.\n");
    	if (!force || !chip_to_probe) {
-			printf("If you know which flash chip you have, and if this version of flashrom\n");
-			printf("supports a similar flash chip, you can try to force read your chip. Run:\n");
-			printf("flashrom -f -r -c similar_supported_flash_chip filename\n");
-			printf("\n");
-			printf("Note: flashrom can never write when the flash chip isn't found automatically.\n");
+			printf("Note: flashrom can never write if the flash chip isn't found automatically.\n");
    	}
    	if (force && read_it && chip_to_probe) {
-			printf("Force read (-f -r -c) requested, forcing chip probe success:\n");
+			printf("Force read (-f -r -c) requested, pretending the chip is there:\n");
    		flashes[0] = probe_flash(flashchips, 1);
    		if (!flashes[0]) {
-				printf("flashrom does not support a flash chip named '%s'.\n", chip_to_probe);
-				printf("Run flashrom -L to view the hardware supported in this flashrom version.\n");
+				printf("Probing for flash chip '%s' failed.\n", chip_to_probe);
+				programmer_shutdown();
    			exit(1);
    		}
    		printf("Please note that forced reads most likely contain garbage.\n");
Index: flashrom-forced_stupid/dmi.c
===================================================================
--- flashrom-forced_stupid/dmi.c	(Revision 994)
+++ flashrom-forced_stupid/dmi.c	(Arbeitskopie)
@@ -102,7 +102,8 @@
    }

chassis_type = get_dmi_string("chassis-type");
-	if (chassis_type && !strcmp(chassis_type, "Notebook")) {
+	if (chassis_type && (!strcmp(chassis_type, "Notebook") ||
+			     !strcmp(chassis_type, "Portable"))) {
    	printf_debug("Laptop detected via DMI\n");
    	is_laptop = 1;
    }
Index: flashrom-forced_stupid/flashrom.c
===================================================================
--- flashrom-forced_stupid/flashrom.c	(Revision 994)
+++ flashrom-forced_stupid/flashrom.c	(Arbeitskopie)
@@ -488,7 +488,17 @@
    char *param_pos, *rest, *tmp;
    char *dev = NULL;
    int devlen;
+	int needlelen;

+	needlelen = strlen(needle);
+	if (!needlelen) {
+		msg_gerr("%s: empty needle! Please report a bug at "
+			 "[e-mail]\n", __func__);
+		return NULL;
+	}
+	/* No programmer parameters given. */
+	if (*haystack == NULL)
+		return NULL;
    param_pos = strstr(*haystack, needle);
    do {
    	if (!param_pos)
@@ -924,7 +934,8 @@
    if (!flash || !flash->name)
    	return NULL;

-	printf("Found chip "%s %s" (%d KB, %s) at physical address 0x%lx.\n",
+	printf("%s chip "%s %s" (%d KB, %s) at physical address 0x%lx.\n",
+	       force ? "Assuming" : "Found",
           flash->vendor, flash->name, flash->total_size,
           flashbuses_to_text(flash->bustype), base);

Index: flashrom-forced_stupid/internal.c
===================================================================
--- flashrom-forced_stupid/internal.c	(Revision 994)
+++ flashrom-forced_stupid/internal.c	(Arbeitskopie)
@@ -101,6 +101,7 @@
 #if INTERNAL_SUPPORT == 1
 struct superio superio = {};
 int force_boardenable = 0;
+int force_boardmismatch = 0;

void probe_superio(void)
 {
@@ -117,26 +118,42 @@
 int internal_init(void)
 {
    int ret = 0;
+	int force_laptop = 0;
+	char *arg;

-	if (programmer_param && !strlen(programmer_param)) {
-		free(programmer_param);
-		programmer_param = NULL;
+	arg = extract_param(&programmer_param, "boardenable=", ",:");
+	if (arg && !strcmp(arg,"force")) {
+		force_boardenable = 1;
+	} else if (arg && !strlen(arg)) {
+		msg_perr("Missing argument for boardenable.\n");
+	} else if (arg) {
+		msg_perr("Unknown argument for boardenable: %s\n", arg);
+		exit(1);
    }
-	if (programmer_param) {
-		char *arg;
-		arg = extract_param(&programmer_param, "boardenable=", ",:");
-		if (arg && !strcmp(arg,"force"))
-			force_boardenable = 1;
-		else if (arg)
-			msg_perr("Unknown argument for boardenable: %s\n", arg);
-		free(arg);
+	free(arg);

-		if (strlen(programmer_param))
-			msg_perr("Unhandled programmer parameters: %s\n",
-				programmer_param);
-		free(programmer_param);
-		programmer_param = NULL;
+	arg = extract_param(&programmer_param, "boardmismatch=", ",:");
+	if (arg && !strcmp(arg,"force")) {
+		force_boardmismatch = 1;
+	} else if (arg && !strlen(arg)) {
+		msg_perr("Missing argument for boardmismatch.\n");
+	} else if (arg) {
+		msg_perr("Unknown argument for boardmismatch: %s\n", arg);
+		exit(1);
    }
+	free(arg);
+
+	arg = extract_param(&programmer_param, "laptop=", ",:");
+	if (arg && !strcmp(arg,"force_I_want_a_brick")) {
+		force_laptop = 1;
+	} else if (arg && !strlen(arg)) {
+		msg_perr("Missing argument for laptop.\n");
+	} else if (arg) {
+		msg_perr("Unknown argument for laptop: %s\n", arg);
+		exit(1);
+	}
+	free(arg);
+
    get_io_perms();

/* Initialize PCI access for flash enables */
@@ -155,22 +172,35 @@
    probe_superio();

/* Warn if a laptop is detected. */
-	if (is_laptop)
-		printf("========================================================================\n"
-		       "WARNING! You seem to be running flashrom on a laptop.\n"
-		       "Laptops, notebooks and netbooks are difficult to support and we recommend\n"
-		       "to use the vendor flashing utility. The embedded controller (EC) in these\n"
-		       "machines often interacts badly with flashing.\n"
-		       "See http://www.flashrom.org/Laptops for details.\n"
-		       "========================================================================\n");
+	if (is_laptop) {
+		msg_perr("========================================================================\n"
+			 "WARNING! You seem to be running flashrom on a laptop.\n"
+			 "Laptops, notebooks and netbooks are difficult to support and we recommend\n"
+			 "to use the vendor flashing utility. The embedded controller (EC) in these\n"
+			 "machines often interacts badly with flashing.\n"
+			 "See http://www.flashrom.org/Laptops for details.\n\n"
+			 "If flash is shared with the EC, erase is guaranteed to brick your laptop\n"
+			 "and write may brick your laptop.\n"
+			 "Read and probe may irritate your EC and cause fan failure, backlight\n"
+			 "failure and sudden poweroff.\n"
+			 "You have been warned.\n"
+			 "========================================================================\n");
+		if (force_laptop) {
+			msg_perr("Proceeding anyway because user specified "
+				 "laptop=force_I_want_a_brick\n");
+		} else {
+			msg_perr("Aborting.\n");
+			exit(1);
+		}
+	}

/* try to enable it. Failure IS an option, since not all motherboards
     * really need this to be done, etc., etc.
     */
    ret = chipset_flash_enable();
    if (ret == -2) {
-		printf("WARNING: No chipset found. Flash detection "
-		       "will most likely fail.\n");
+		msg_perr("WARNING: No chipset found. Flash detection "
+			 "will most likely fail.\n");
    }

/* Probe for IT87* LPC->SPI translation unconditionally. */
@@ -182,7 +212,7 @@
     * The error code might have been a warning only.
     * Besides that, we don't check the board enable return code either.
     */
-	return 0; 
+	return 0;
 }

int internal_shutdown(void)
Index: flashrom-forced_stupid/README
===================================================================
--- flashrom-forced_stupid/README	(Revision 994)
+++ flashrom-forced_stupid/README	(Arbeitskopie)
@@ -11,6 +11,12 @@
 It supports a wide range of DIP32, PLCC32, DIP8, SO8/SOIC8, TSOP32, and TSOP40
 chips, which use various protocols such as LPC, FWH, parallel flash, or SPI.

+Do not use flashrom on laptops! The embedded controller (EC) present in many
+laptops interacts badly with any flash attempts and can brick your laptop
+permanently.
+
+Please make a backup of your flash chip before writing to it.
+
 Please see the flashrom(8) manpage.

Index: flashrom-forced_stupid/layout.c
===================================================================
--- flashrom-forced_stupid/layout.c	(Revision 994)
+++ flashrom-forced_stupid/layout.c	(Arbeitskopie)
@@ -111,14 +111,15 @@
        !strcasecmp(mainboard_part, lb_part)) {
    	printf_debug("This firmware image matches this mainboard.\n");
    } else {
-		if (force) {
+		if (force_boardmismatch) {
    		printf("WARNING: This firmware image does not "
    		       "seem to fit to this machine - forcing it.\n");
    	} else {
    		printf("ERROR: Your firmware image (%s:%s) does not "
    		       "appear to\n       be correct for the detected "
-			       "mainboard (%s:%s)\n\nOverride with --force if you "
-			       "are absolutely sure that you\nare using a correct "
+			       "mainboard (%s:%s)\n\nOverride with -p internal:"
+			       "boardmismatch=force if you are absolutely sure "
+			       "that\nyou are using a correct "
    		       "image for this mainboard or override\nthe detected "
    		       "values with --mainboard <vendor>:<mainboard>.\n\n",
    		       mainboard_vendor, mainboard_part, lb_vendor,