[flashrom] [PATCH] Fix out-of-bounds access if all erase functions fail
Stefan Tauner
stefan.tauner at student.tuwien.ac.at
Sun Jul 17 15:46:25 CEST 2011
On Sun, 17 Jul 2011 15:20:59 +0200
Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net> wrote:
> Am 16.07.2011 23:44 schrieb Stefan Tauner:
> > On Sat, 16 Jul 2011 02:13:44 +0200
> > Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net> wrote:
> >
> >
> >> > Am 16.04.2011 13:37 schrieb Stefan Tauner:
> >>
> >>> > > i would change the check_block_eraser method to return different values
> >>> > > for "not suitable" and "not defined/no more methods"
> >>>
> >>> > > (there cant be
> >>> > > holes in the eraser array, right?).
> >>>
> > this is wrong. there can be holes. anyway, my current approach looks
> > similar to yours.
> >
>
> 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. :)
> >> > That would fix the bug and simplify the code at the cost of additional
> >> > lines in the log for the following case:
> >> > erase method 0 is suitable, but fails (no change)
> >> > ... suitable but fails (no change)
> >> > erase method n is not suitable (my version: not printed, your version:
> >> > printed)
> >> > ...not suitable (see above)
> >> > erase method m is not defined (no change)
> >> > ...not defined (no change)
> >> > END OF ARRAY
> >>
> > i am not 100% sure what you mean. i did not specify how/when i would
> > print the state of erase functions... if the behavior of the attached
> > patch has this problem please specify.
> >
>
> AFAICS your current patch does not implement the check_block_eraser
> change you talked about, so your current patch is OK.
nope, ok.
> >> > A few LPC/FWH chips have a chip-erase function which is only available
> >> > in parallel programmer mode (i.e. not in a mainboard), and those would
> >> > get those additional lines.
> >>
> > how and where are those en/disabled?
> >
>
> That code is mostly unfinished. The bustype is special (A/A Mux) and
> mentioned for a few chips in flashchips.c, but we didn't add specific
> support yet.
i have seen the comments... ok.
> > the first attached patch includes a fix similar to yours, but a little
> > bit extended. some modification i have done to test it are in a second
> > patch (not for merge of course). i have done testing with my spi-capable
> > serprog programmer. is the dummy programmer able to simulate such things
> > too? i didnt bother to look :)
> >
>
> The dummy programmer can blacklist SPI commands, but that patch is
> unmerged because I didn't have time to finish the man page entry for
> that feature. I think Maciej Pijanka wrote some something for the man
> page, and I should merge that into my patch and commit it.
:)
> > my patch changes the output a bit... to the better imho.
> > - it removes a line break after each call of walk_eraseregions. what's
> > the purpose of this? it makes holes in the log in the case something
> > in walk_eraseregions ends with \n (lots of things do). this does not
> > fix all problems though... see -EV output below.
> >
>
> Odd. I think that additional linebreak appeared somtime in 2011, and was
> not present before.
>
>
> > - it informs the user about the (potentially long) read of the whole
> > chip after an erase failed. this is very important for slow
> > programmers imho.
> >
>
> OK.
>
>
> > - it adds a message to check_block_eraser in the case of success.
> > since each caller can decide if any output is printed in
> > check_block_eraser this is ok and we can improve the user experience
> > in verbose mode.
> >
> > it also changes the long comment because it is no longer true. we may
> > wanna make it true again by checking the other erasers too and
> > continuing the loop after checking for walk_eraseregions > 0
> > afterwards, but with your/mine patch it is just not true anymore.
> >
> > below is the output of erase runs with different verbosity levels
> > (note that the last erase function succeeds, so there is no test for
> > the actual oob problem... but that is obviously fixed anyway... and i
> > have tested this too.):
> >
> > ./flashrom -pserprog:dev=/dev/ttyU2flasher:115200 -c W25Q32 -E
> > flashrom v0.9.3-r1372 on Linux 2.6.35-30-generic (x86_64), built with libpci 3.1.7, GCC 4.4.5, little endian
> > flashrom is free software, get the source code at http://www.flashrom.org
> >
> > Calibrating delay loop... OK.
> > serprog: Programmer name is "atmegaXXu2 "
> > 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?
> 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".
>
> 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).
> > ./flashrom -pserprog:dev=/dev/ttyU2flasher:115200 -c W25Q32 -EV
> > flashrom v0.9.3-r1372 on Linux 2.6.35-30-generic (x86_64), built with libpci 3.1.7, GCC 4.4.5, little endian
> > flashrom is free software, get the source code at http://www.flashrom.org
> >
> > Calibrating delay loop... OS timer resolution is 1 usecs, 1454M loops per second, 10 myus = 10 us, 100 myus = 99 us, 1000 myus = 994 us, 10000 myus = 10003 us, 4 myus = 4 us, OK.
> > Initializing serprog programmer
> > serprog: connected - attempting to synchronize
> > .
> > serprog: Synchronized
> > serprog: Interface version ok.
> > serprog: Bus support: parallel=off, LPC=off, FWH=off, SPI=on
> > serprog: Maximum write-n length is 16777215
> > serprog: Maximum read-n length is 16777215
> > serprog: Programmer name is "atmegaXXu2 "
> > serprog: Serial buffer size is 65535
> > Probing for Winbond W25Q32, 4096 kB: probe_spi_rdid_generic: id1 0xef, id2 0x4016
> > Chip status register is 00
> > Found chip "Winbond W25Q32" (4096 kB, SPI) on serprog.
> > Note: using internal_delay, because the attached programmer does not support the delay opcode.
> > Erasing and writing flash chip... Looking at erase function 0... looks ok. Trying... 0x000000-0x000fff:ENote: using internal_delay, because the attached programmer does not support the delay opcode.
> >
>
> That "using internal delay" message should appear exactly once, not
> every time someone calls programmer_delay.
that one is from the serprog spi patch, so lets postpone this
discussion. i agree with you though.
>
> > 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.
> >
> >
> > ./flashrom -pserprog:dev=/dev/ttyU2flasher:115200 -c W25Q32 -EVV
> > flashrom v0.9.3-r1372 on Linux 2.6.35-30-generic (x86_64), built with libpci 3.1.7, GCC 4.4.5, little endian
> > flashrom is free software, get the source code at http://www.flashrom.org
> >
> > Calibrating delay loop... OS timer resolution is 1 usecs, 1451M loops per second, 10 myus = 10 us, 100 myus = 99 us, 1000 myus = 1009 us, 10000 myus = 10244 us, 4 myus = 4 us, OK.
> > Initializing serprog programmer
> > serprog: connected - attempting to synchronize
> > .
> > serprog: Synchronized
> > serprog: Interface version ok.
> > serprog: Bus support: parallel=off, LPC=off, FWH=off, SPI=on
> > serprog: Maximum write-n length is 16777215
> > serprog: Maximum read-n length is 16777215
> > serprog: Programmer name is "atmegaXXu2 "
> > serprog: Serial buffer size is 65535
> > Probing for Winbond W25Q32, 4096 kB: serprog_spi_send_command, writecnt=1, readcnt=3
> > RDID returned 0xef 0x40 0x16. probe_spi_rdid_generic: id1 0xef, id2 0x4016
> > serprog_spi_send_command, writecnt=1, readcnt=2
> > Chip status register is 00
> > Found chip "Winbond W25Q32" (4096 kB, SPI) on serprog.
> > serprog_delay usecs=100000
> > Note: using internal_delay, because the attached programmer does not support the delay opcode.
> > serprog_spi_send_command, writecnt=1, readcnt=2
> > Erasing and writing flash chip... Looking at erase function 0... looks ok. Trying... 0x000000-0x000fff:Eserprog_spi_send_command, writecnt=1, readcnt=0
> > serprog_spi_send_command, writecnt=4, readcnt=0
> > serprog_spi_send_command, writecnt=1, readcnt=2
> > serprog_delay usecs=10000
> > Note: using internal_delay, because the attached programmer does not support the delay opcode.
> > serprog_spi_send_command, writecnt=1, readcnt=2
> > serprog_spi_send_command, writecnt=4, readcnt=4096
> > ERASE FAILED!
> >
> > Reading current chip content... serprog_spi_send_command, writecnt=4, readcnt=4194304<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.
> > serprog_shutdown
> >
> >
> > From: Stefan Tauner <stefan.tauner at student.tuwien.ac.at>
> > 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 <stefan.tauner at student.tuwien.ac.at>
> > ---
> > 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."? :)
>
> > 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..."
>
>
> > + ret = walk_eraseregions(flash, k, &erase_and_write_block_helper,
> > + curcontents, newcontents);
> > /* If everything is OK, don't try another erase function. */
> > if (!ret)
> > break;
> > /* Write/erase failed, so try to find out what the current chip
> > - * contents are. If no usable erase functions remain, we could
> > - * abort the loop instead of continuing, the effect is the same.
> > - * The only difference is whether the reason for other unusable
> > - * functions is printed or not. If in doubt, verbosity wins.
> > + * contents are. If no usable erase functions remain, we can
> > + * skip this: the next iteration will break immediately anyway.
> > */
> > if (!usable_erasefunctions)
> > continue;
> > + /* Reading the whole chip may take a while, inform the user even
> > + * in non-verbose mode.
> > + */
> > + msg_cinfo("Reading current chip content... ");
> >
>
> msg_cdbg("Reading current flash chip contents... ");
ok
>
> > if (flash->read(flash, curcontents, 0, size)) {
> > /* Now we are truly screwed. Read failed as well. */
> > - msg_cerr("Can't read anymore!\n");
> > + msg_cerr("Can't read anymore! Aborting.\n");
> > /* We have no idea about the flash chip contents, so
> > * retrying with another erase function is pointless.
> > */
> > break;
> > }
> > + msg_cinfo("done. Trying next erase function.\n");
> >
>
> msg_cdbg("Looking for another erase function.\n");
> That would make the messages more consistent.
yes, good idea
>
> > }
> > /* Free the scratchpad. */
> > free(curcontents);
> >
--
Kind regards/Mit freundlichen Grüßen, Stefan Tauner
More information about the flashrom
mailing list