Attention is currently required from: Nico Huber, Miklós Márton, Anastasia Klimchuk. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/56637 )
Change subject: ni845x_spi: Fix signed - unsigned comparisons ......................................................................
Patch Set 1:
(4 comments)
File ni845x_spi.c:
https://review.coreboot.org/c/flashrom/+/56637/comment/16b4c97b_f2f0c5c2 PS1, Line 188: strtol Use `strtoul()` instead?
https://review.coreboot.org/c/flashrom/+/56637/comment/4c73ee78_4e30046c PS1, Line 483: const Unrelated change?
https://review.coreboot.org/c/flashrom/+/56637/comment/792c1a0f_08cc56ac PS1, Line 558: strlen(CS_str) For another patch: This is an use-after-free bug.
https://review.coreboot.org/c/flashrom/+/56637/comment/ddc15a34_a0f2be3c PS1, Line 556: CS_number = CS_str[0] - '0'; : free(CS_str); : if (strlen(CS_str) > 1 || 7 < CS_number) { : msg_perr("Only CS 0-7 supported\n"); : return 1; : } The assignment to `CS_number` can underflow. I'd do the parsing as follows:
if (CS_str) { if (CS_str[0] >= '0' && CS_str[0] <= '7' && CS_str[1] == '\0') { CS_number = CS_str[0] - '0'; free(CS_str); } else { free(CS_str); msg_perr("Only CS 0-7 supported\n"); return 1; } }
The evaluation order of the three checks in the inner if-block is very important. The first check ensures that the length of `CS_str` is not zero, so that the third check doesn't cause undefined behavior.