On Sun, 17 Jul 2011 15:20:59 +0200 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@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@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@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@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;
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);
* 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./* 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);