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

[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.

> 
> 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.

> 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?

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 :)

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.
 - 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.
 - 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.


./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.
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

-- 
Kind regards/Mit freundlichen Grüßen, Stefan Tauner
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Fix-out-of-bounds-access-if-all-erase-functions-fail.patch
Type: text/x-patch
Size: 2828 bytes
Desc: not available
URL: <http://www.flashrom.org/pipermail/flashrom/attachments/20110716/21bfe7ed/attachment.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-test-case-used-for-Fix-out-of-bounds-access-if-all-e.patch
Type: text/x-patch
Size: 1590 bytes
Desc: not available
URL: <http://www.flashrom.org/pipermail/flashrom/attachments/20110716/21bfe7ed/attachment-0001.patch>