Re: [flashrom] [PATCH] Fix out-of-bounds access if all erase functions fail

[Carl-Daniel Hailfinger]

Am 17.07.2011 15:46 schrieb Stefan Tauner:
> On Sun, 17 Jul 2011 15:20:59 +0200 Carl-Daniel Hailfinger wrote:
> > Am 16.07.2011 23:44 schrieb Stefan Tauner:
> > > On Sat, 16 Jul 2011 02:13:44 +0200 Carl-Daniel Hailfinger wrote:
> > > > > Am 16.04.2011 13:37 schrieb Stefan Tauner:
> >
> > [holes eraser array]
> > Where did you find holes in that array? If a patch contains flash chip
> > definitions with holes, it should be rejected.
>
> i did not find one in flashchips.c, but it is possible to introduce
> them via the probing function theoretically. should not happen, will
> probably never happen, would guard against it anyway if possible. :)

We should guard against that in a selfcheck function on startup, not at
write/erase time.
> > > Found chip "Winbond W25Q32" (4096 kB, SPI) on serprog.
> > > Erasing and writing flash chip... ERASE FAILED!
> > > Reading current chip content... <very long pause>done. Trying next erase function.
> > > Done.
> > >
> > > without the new "Reading current chip content..." there would be
> > > only "ERASE FAILED!" be printed for a quite long time. i think that's
> > > scary for users, and that they may hit ctrl+c because of it.
> >
> > Sorry, I don't buy that argument. flashrom already emits too much
> > information in non-verbose mode, and this is not going to improve the
> > situation.
>
> 5 lines + header for an erase cycle where one erase function fails is
> too much?

Not in itself, but I doubt the messages will fit in 5 lines in the near
future when someone discovers that we need more default verbosity in
other places, and by then a quiet mode will be needed for flashrom.
> > If people freak out, there is a good chance they will freak
> > out already during write, and won't wait for the scary "ERASE FAILED"
> > error message.
>
> during write there the last characters are "..." not "FAILED". i hope
> we can agree that this is a major difference in the perspective of a
> user :)
> at least for me "..." indicates "it does something which might take a
> while, dont worry".

I am unhappy about the inconsistency this creates for messages about
reading the flash chip. Depending on the flash chip and the operation,
flashrom may read the full flash chip 8 times: Once during startup, once
for each failed erase method (up to 6), once for final verify.
Individual write methods may have internal verify/read steps as well.
Only reads after a failed erase would be printed at default verbosity,
and that inconsistency worries me.
> > You once said that you think flashrom floods the user with too much
> > output. Did you change your mind?
>
> that was *only* about ichspi in verbose mode. for non-verbose mode i
> think it could inform the user more about what it does in general
> (alternative would be to have at least a progress bar.. i know wip).

I think the Chromium branch of flashrom has a totally different user
experience because it changed lots of output levels. We might want to
take a look at their approach and check if it fits our needs as well.
> > > ./flashrom -pserprog:dev=/dev/ttyU2flasher:115200 -c W25Q32 -EV
> > > [...]
> > > Erasing and writing flash chip... Looking at erase function 0... looks ok. Trying... 0x000000-0x000fff:E
> > > ERASE FAILED!
> > >
> > > Reading current chip content... <very long pause>done. Trying next erase function.
> > > Looking at erase function 1... block erase function found, but eraseblock layout is not defined. Looking for another erase function.
> > > Looking at erase function 2... eraseblock layout is known, but matching block erase function is not implemented. Looking for another erase function.
> > > Looking at erase function 3... not defined. Looking for another erase function.
> > > Looking at erase function 4... looks ok. Trying... 0x000000-0x3fffff:S
> > > Done.
> > >
> > > nice verbose output explaining what really happens.. the extra line
> > > break is from inside the erase_and_write_block_helper i think.
> > >
> > > From: Stefan Tauner 
> > > Date: Sat, 16 Jul 2011 22:53:24 +0200
> > > Subject: [PATCH 1/2] Fix out-of-bounds access if all erase functions fail
> > > Signed-off-by: Stefan Tauner
> > >
> > > flashrom.c |   29 +++++++++++++++++++----------
> > >  1 files changed, 19 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/flashrom.c b/flashrom.c
> > > index 998a18f..25cd43e 100644
> > > --- a/flashrom.c
> > > +++ b/flashrom.c
> > > @@ -1502,6 +1502,8 @@ static int check_block_eraser(const struct flashchip *flash, int k, int log)
> > >    			 "eraseblock layout is not defined. ");
> > >    	return 1;
> > >    }
> > > if (log)
> > > msg_cdbg("looks ok. ");
> >
> > Unneeded verbosity.
>
> i beg to differ. i think it makes the verbose output more readable for
> someone who does not know the code (well). i think there should always
> be a terminating print for messages ending with "..." like this one.
> maybe just "ok."? :)

Unless I'm mistaken, the original "trying..." message fulfilled exactly
that purpose.
> > > return 0;
> > >  }
> > >
> > > @@ -1522,34 +1524,41 @@ int erase_and_write_flash(struct flashchip *flash, uint8_t *oldcontents, uint8_t
> > >    memcpy(curcontents, oldcontents, size);
> > >
> > > for (k = 0; k < NUM_ERASEFUNCTIONS; k++) {
> > > msg_cdbg("Looking at blockwise erase function %i... ", k);
> > > if (check_block_eraser(flash, k, 1) && usable_erasefunctions) {
> > > if (!usable_erasefunctions) {
> > > 	msg_cdbg("No usable erase functions left.\n");
> > > 	break;
> > > }
> > > msg_cdbg("Looking at erase function %i... ", k);
> > > if (check_block_eraser(flash, k, 1)) {
> > > msg_cdbg("Looking for another erase function.\n");
> > > continue;
> > > }
> > > usable_erasefunctions--;
> > > msg_cdbg("trying... ");
> > > ret = walk_eraseregions(flash, k, &erase_and_write_block_helper, curcontents, newcontents);
> > > msg_cdbg("\n");
> > > msg_cdbg("Trying... ");
> >
> > Why did you change trying->Trying? Consistency?
>
> to have correct output together with the "looks ok." print above:
> "Looking at erase function 4... looks ok. Trying..."
> vs.
> "Looking at erase function 4... looks ok. trying..."

Compare that to the current message:

"Looking at erase function 4... trying..."

The current message does not mislead users to assume that the erase
function is OK. We will only know that after it has been tried, and the
user does not care about flashrom internals.

My main objection to this patch is that it combines a bugfix with
cosmetics, and the cosmetics distract from bugfixing. The bugfix part
(and the ret=1 initialization you mentioned) would have been easily
reviewable/ackable, yet we're investing hours into dicussing how the
output should look like.

Here is my take on the bugfix part with minimal message changes:

Fix out-of-bounds access if all erase functions fail.
Fix detection of unchanged chip contents on erase failure.
Return error if no usable erase functions exist.
Print a debug message for the chip read after erase failure.

Thanks to Stefan Tauner for spotting the last two problems.

Signed-off-by: Carl-Daniel Hailfinger