September 2022 - flashrom-gerrit

[S] Change in flashrom[1.1.x]: jlink_spi: Reduce transfer size
by Nico Huber (Code Review) 26 Sep '22

26 Sep '22

Attention is currently required from: zapb, Angel Pons. Hello zapb, build bot (Jenkins), Angel Pons, I'd like you to do a code review. Please visit https://review.coreboot.org/c/flashrom/+/67855 to review the following change. Change subject: jlink_spi: Reduce transfer size ...................................................................... jlink_spi: Reduce transfer size The maximum transfer size is too large for some devices and results in an USB timeout. Change-Id: If2c00b1524ec56740bdfe290096c3546cf375d73 Signed-off-by: Marc Schink <dev(a)zapb.de> Reviewed-on: https://review.coreboot.org/c/flashrom/+/48379 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Miklós Márton <martonmiklosqdev(a)gmail.com> Reviewed-by: Angel Pons <th3fanbus(a)gmail.com> --- M jlink_spi.c 1 file changed, 18 insertions(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/55/67855/1 diff --git a/jlink_spi.c b/jlink_spi.c index 08a9ba9..cf732b0 100644 --- a/jlink_spi.c +++ b/jlink_spi.c @@ -34,7 +34,7 @@ * Maximum number of bytes that can be transferred at once via the JTAG * interface, see jaylink_jtag_io(). */ -#define JTAG_MAX_TRANSFER_SIZE (UINT16_MAX / 8) +#define JTAG_MAX_TRANSFER_SIZE (32768 / 8) /* * Default base frequency in Hz. Used when the base frequency can not be -- To view, visit https://review.coreboot.org/c/flashrom/+/67855 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: 1.1.x Gerrit-Change-Id: If2c00b1524ec56740bdfe290096c3546cf375d73 Gerrit-Change-Number: 67855 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Reviewer: zapb <dev(a)zapb.de> Gerrit-Attention: zapb <dev(a)zapb.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: newchange

1 0

[S] Change in flashrom[1.1.x]: flashrom.8: add missing entry for `--flash-contents`
by Nico Huber (Code Review) 26 Sep '22

26 Sep '22

Attention is currently required from: Michael Niewöhner. Hello build bot (Jenkins), Michael Niewöhner, I'd like you to do a code review. Please visit https://review.coreboot.org/c/flashrom/+/67856 to review the following change. Change subject: flashrom.8: add missing entry for `--flash-contents` ...................................................................... flashrom.8: add missing entry for `--flash-contents` Change-Id: I64a8200a86329bd26a2069c5dc39430de9f8ba09 Signed-off-by: Michael Niewöhner <foss(a)mniewoehner.de> Reviewed-on: https://review.coreboot.org/c/flashrom/+/57807 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Nico Huber <nico.h(a)gmx.de> --- M flashrom.8.tmpl 1 file changed, 20 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/56/67856/1 diff --git a/flashrom.8.tmpl b/flashrom.8.tmpl index c1a228b..5192d0b 100644 --- a/flashrom.8.tmpl +++ b/flashrom.8.tmpl @@ -242,6 +242,13 @@ .B <imagename> from flash layout. .TP +.B "\-\-flash\-contents <ref\-file>" +The file contents of +.BR <ref\-file> +will be used to decide which parts of the flash need to be written. Providing +this saves an initial read of the full flash chip. Be careful, if the provided +data doesn't actually match the flash contents, results are undefined. +.TP .B "\-L, \-\-list\-supported" List the flash chips, chipsets, mainboards, and external programmers (including PCI, USB, parallel port, and serial port based devices) -- To view, visit https://review.coreboot.org/c/flashrom/+/67856 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: 1.1.x Gerrit-Change-Id: I64a8200a86329bd26a2069c5dc39430de9f8ba09 Gerrit-Change-Number: 67856 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-MessageType: newchange

1 0

[M] Change in flashrom[1.1.x]: linux_mtd: Disable buffering on the mtd device
by Nico Huber (Code Review) 26 Sep '22

26 Sep '22

Attention is currently required from: Hung-Te Lin, Douglas Anderson, Angel Pons, Nikolai Artemiev, Patrick Rudolph. Hello Hung-Te Lin, build bot (Jenkins), Douglas Anderson, Angel Pons, Patrick Rudolph, I'd like you to do a code review. Please visit https://review.coreboot.org/c/flashrom/+/67854 to review the following change. Change subject: linux_mtd: Disable buffering on the mtd device ...................................................................... linux_mtd: Disable buffering on the mtd device We open the device node for the MTD device with this: dev_fp = fopen(dev_path, "r+") In C fopen() is allowed to provide _buffered_ access to the file. That means that the standard library is allowed to read ahead and/or return cached data. That's really not what we want for something like this. Let's turn it off. This fixes a problem where flashrom would sometimes fail to "verify" that it erased the flash. The error message would look something like this: Erasing and writing flash chip... FAILED at 0x0000e220! Expected=0xff, Found=0xe9, failed byte count from 0x0000e200-0x0000e2ff: 0xdc failed byte count from 0x0000e000-0x0000efff: 0xffffffff ERASE_FAILED FAILED! Uh oh. Erase/write failed. Checking if anything changed. After the failure I could read the flash device with a new invocation of flashrom and I would see that, indeed, the erase had worked. Tracing in the kernel showed that when the failure happened we saw a pattern that looked like this: * Read 0x0b00 bytes starting at 0x0000d000 * Read 0x1000 bytes starting at 0x0000db00 * Erase 0x1000 bytes starting at 0x0000e000 ...and then there was _not_ a read after the erase. It can be assumed that, since userspace had already read 0xdb00 - 0xeaff that it was looking at old buffered data after the erase. Signed-off-by: Douglas Anderson <dianders(a)chromium.org> Change-Id: I989afd83a33013b2756a0090d6b08245613215c6 Reviewed-on: https://review.coreboot.org/c/flashrom/+/50155 Reviewed-by: Hung-Te Lin <hungte(a)chromium.org> Reviewed-by: Patrick Rudolph <siro(a)das-labor.org> Reviewed-by: Angel Pons <th3fanbus(a)gmail.com> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> --- M linux_mtd.c 1 file changed, 50 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/54/67854/1 diff --git a/linux_mtd.c b/linux_mtd.c index ae8bef2..75dce1f 100644 --- a/linux_mtd.c +++ b/linux_mtd.c @@ -340,6 +340,10 @@ msg_perr("Cannot open file stream for %s\n", dev_path); goto linux_mtd_setup_exit; } + ret = setvbuf(dev_fp, NULL, _IONBF, 0); + if (ret) + msg_pwarn("Failed to set MTD device to unbuffered: %d\n", ret); + msg_pinfo("Opened %s successfully\n", dev_path); ret = 0; -- To view, visit https://review.coreboot.org/c/flashrom/+/67854 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: 1.1.x Gerrit-Change-Id: I989afd83a33013b2756a0090d6b08245613215c6 Gerrit-Change-Number: 67854 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Douglas Anderson <dianders(a)chromium.org> Gerrit-Reviewer: Hung-Te Lin <hungte(a)chromium.org> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Hung-Te Lin <hungte(a)chromium.org> Gerrit-Attention: Douglas Anderson <dianders(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-MessageType: newchange

1 0

[S] Change in flashrom[1.1.x]: chipset_enable.c: Validate physmap() return rcrb value
by Nico Huber (Code Review) 26 Sep '22

26 Sep '22

Attention is currently required from: Sam McNally, Edward O'Callaghan, Angel Pons. Hello Sam McNally, build bot (Jenkins), Edward O'Callaghan, Angel Pons, I'd like you to do a code review. Please visit https://review.coreboot.org/c/flashrom/+/67853 to review the following change. Change subject: chipset_enable.c: Validate physmap() return rcrb value ...................................................................... chipset_enable.c: Validate physmap() return rcrb value Validate the physical mapping in enable_flash_silvermont(). Change-Id: Icc5a799a06b3f310d9a191fa5eb99b255b20d79d Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> Reviewed-on: https://review.coreboot.org/c/flashrom/+/48225 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Sam McNally <sammc(a)google.com> Reviewed-by: Angel Pons <th3fanbus(a)gmail.com> --- M chipset_enable.c 1 file changed, 18 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/53/67853/1 diff --git a/chipset_enable.c b/chipset_enable.c index 6f75d66..33abe07 100644 --- a/chipset_enable.c +++ b/chipset_enable.c @@ -916,6 +916,8 @@ /* Handle GCS (in RCRB) */ void *rcrb = physmap("BYT RCRB", rcba, 4); + if (rcrb == ERROR_PTR) + return ERROR_FATAL; enable_flash_ich_report_gcs(dev, ich_generation, rcrb); physunmap(rcrb, 4); -- To view, visit https://review.coreboot.org/c/flashrom/+/67853 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: 1.1.x Gerrit-Change-Id: Icc5a799a06b3f310d9a191fa5eb99b255b20d79d Gerrit-Change-Number: 67853 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Sam McNally <sammc(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Sam McNally <sammc(a)google.com> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: newchange

1 0

[S] Change in flashrom[1.1.x]: chipset_enable.c: check return value from rphysmap() call
by Nico Huber (Code Review) 26 Sep '22

26 Sep '22

Attention is currently required from: Sam McNally, Edward O'Callaghan, Angel Pons. Hello Sam McNally, build bot (Jenkins), Edward O'Callaghan, Angel Pons, I'd like you to do a code review. Please visit https://review.coreboot.org/c/flashrom/+/67852 to review the following change. Change subject: chipset_enable.c: check return value from rphysmap() call ...................................................................... chipset_enable.c: check return value from rphysmap() call Port from the ChromiumOS fork of flashrom. Change-Id: I8075fe5f80ac0da5280d2f0de6829ed3a2496476 Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> Reviewed-on: https://review.coreboot.org/c/flashrom/+/46444 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Sam McNally <sammc(a)google.com> Reviewed-by: Angel Pons <th3fanbus(a)gmail.com> --- M chipset_enable.c 1 file changed, 18 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/52/67852/1 diff --git a/chipset_enable.c b/chipset_enable.c index 6280876..6f75d66 100644 --- a/chipset_enable.c +++ b/chipset_enable.c @@ -930,6 +930,8 @@ uint32_t sbase = pci_read_long(dev, 0x54) & 0xfffffe00; msg_pdbg("SPI_BASE_ADDRESS = 0x%x\n", sbase); void *spibar = rphysmap("BYT SBASE", sbase, 512); /* Last defined address on Bay Trail is 0x100 */ + if (spibar == ERROR_PTR) + return ERROR_FATAL; /* Enable Flash Writes. * Silvermont-based: BCR at SBASE + 0xFC (some bits of BCR are also accessible via BC at IBASE + 0x1C). -- To view, visit https://review.coreboot.org/c/flashrom/+/67852 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: 1.1.x Gerrit-Change-Id: I8075fe5f80ac0da5280d2f0de6829ed3a2496476 Gerrit-Change-Number: 67852 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Sam McNally <sammc(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Sam McNally <sammc(a)google.com> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: newchange

1 0

[S] Change in flashrom[1.1.x]: it87spi.c: Prevent use-after-free bug
by Nico Huber (Code Review) 26 Sep '22

26 Sep '22

Attention is currently required from: Angel Pons. Hello build bot (Jenkins), Angel Pons, I'd like you to do a code review. Please visit https://review.coreboot.org/c/flashrom/+/67851 to review the following change. Change subject: it87spi.c: Prevent use-after-free bug ...................................................................... it87spi.c: Prevent use-after-free bug The memory for the `param` string is aliased by `dualbiosindex_suffix`. Moreover, `errno` could have been modified by the call to `free()`. Therefore, only free the former when there are no more uses of either. Change-Id: I79f18f6077c77c0cbb8bfa431e17f9b079f11c95 Signed-off-by: Angel Pons <th3fanbus(a)gmail.com> Reviewed-on: https://review.coreboot.org/c/flashrom/+/46551 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Nico Huber <nico.h(a)gmx.de> --- M it87spi.c 1 file changed, 19 insertions(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/51/67851/1 diff --git a/it87spi.c b/it87spi.c index df9f664..7517be4 100644 --- a/it87spi.c +++ b/it87spi.c @@ -135,12 +135,13 @@ char *dualbiosindex_suffix; errno = 0; long chip_index = strtol(param, &dualbiosindex_suffix, 0); - free(param); if (errno != 0 || *dualbiosindex_suffix != '\0' || chip_index < 0 || chip_index > 1) { msg_perr("DualBIOS: Invalid chip index requested - choose 0 or 1.\n"); + free(param); exit_conf_mode_ite(port); return 1; } + free(param); if (chip_index != (tmp & 1)) { msg_pdbg("DualBIOS: Previous chip index: %d\n", tmp & 1); sio_write(port, 0xEF, (tmp & 0xFE) | chip_index); -- To view, visit https://review.coreboot.org/c/flashrom/+/67851 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: 1.1.x Gerrit-Change-Id: I79f18f6077c77c0cbb8bfa431e17f9b079f11c95 Gerrit-Change-Number: 67851 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: newchange

1 0

[S] Change in flashrom[1.1.x]: spi25: Debug flashrom crash when Write Protect is ON
by Nico Huber (Code Review) 26 Sep '22

26 Sep '22

Attention is currently required from: Stefan Reinauer, Edward O'Callaghan, SANTHOSH JANARDHANA HASSAN. Hello build bot (Jenkins), Stefan Reinauer, Edward O'Callaghan, SANTHOSH JANARDHANA HASSAN, I'd like you to do a code review. Please visit https://review.coreboot.org/c/flashrom/+/67850 to review the following change. Change subject: spi25: Debug flashrom crash when Write Protect is ON ...................................................................... spi25: Debug flashrom crash when Write Protect is ON When hardware write protect is applied, flashrom crashed and generate coredump. spi_disable_blockprotect_generic() calls flash->chip->printlock() method when disable was failed, but this method is optional, can be NULL depends on type of flashrom chip. NULL pointer check before call is added to avoid crash. BRANCH=none BUG=b:129083894 TEST=Run on Mistral P2 (On CR50 console, run "wp disable") flashrom --wp-range 0 0x400000 flashrom --wp-enable (On CR50 console, run "wp enable") flashrom -r /tmp/test.bin Verify "Block protection could not be disabled!" is shown, but flash read completes. Signed-off-by: Yuji Sasaki <sasakiy(a)chromium.org> Change-Id: I81094ab5f16a85871fc9869a2e285eddbbbdec4e Reviewed-on: https://chromium-review.googlesource.com/1535140 Commit-Ready: ChromeOS CL Exonerator Bot <chromiumos-cl-exonerator(a)appspot.gserviceaccount.com> Tested-by: Edward O'Callaghan <quasisec(a)chromium.org> Reviewed-by: Stefan Reinauer <reinauer(a)google.com> Reviewed-by: SANTHOSH JANARDHANA HASSAN <sahassan(a)google.com> Reviewed-on: https://review.coreboot.org/c/flashrom/+/40468 Reviewed-by: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> --- M spi25_statusreg.c 1 file changed, 38 insertions(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/50/67850/1 diff --git a/spi25_statusreg.c b/spi25_statusreg.c index 4cf7023..83d1089 100644 --- a/spi25_statusreg.c +++ b/spi25_statusreg.c @@ -182,7 +182,8 @@ status = spi_read_status_register(flash); if ((status & bp_mask) != 0) { msg_cerr("Block protection could not be disabled!\n"); - flash->chip->printlock(flash); + if (flash->chip->printlock) + flash->chip->printlock(flash); return 1; } msg_cdbg("disabled.\n"); -- To view, visit https://review.coreboot.org/c/flashrom/+/67850 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: 1.1.x Gerrit-Change-Id: I81094ab5f16a85871fc9869a2e285eddbbbdec4e Gerrit-Change-Number: 67850 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: SANTHOSH JANARDHANA HASSAN <sahassan(a)google.com> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Yuji Sasaki <sasakiy(a)chromium.org> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: SANTHOSH JANARDHANA HASSAN <sahassan(a)google.com> Gerrit-MessageType: newchange

1 0

[S] Change in flashrom[1.1.x]: dummyflasher: Add error check for file read
by Nico Huber (Code Review) 26 Sep '22

26 Sep '22

Attention is currently required from: David Hendricks, Jacob Garber. Hello build bot (Jenkins), David Hendricks, Jacob Garber, I'd like you to do a code review. Please visit https://review.coreboot.org/c/flashrom/+/67849 to review the following change. Change subject: dummyflasher: Add error check for file read ...................................................................... dummyflasher: Add error check for file read Print an error message and return if the read from emu_persistent_image fails. Change-Id: Icd1a72f9171e547f2081ba4bc53834a17ef7fcab Signed-off-by: Jacob Garber <jgarber1(a)ualberta.ca> Found-by: Coverity CID 1403912 Reviewed-on: https://review.coreboot.org/c/flashrom/+/34845 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: David Hendricks <david.hendricks(a)gmail.com> --- M dummyflasher.c 1 file changed, 23 insertions(+), 2 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/49/67849/1 diff --git a/dummyflasher.c b/dummyflasher.c index 53801d9..3964226 100644 --- a/dummyflasher.c +++ b/dummyflasher.c @@ -378,8 +378,12 @@ if (image_stat.st_size == emu_chip_size) { msg_pdbg("matches.\n"); msg_pdbg("Reading %s\n", emu_persistent_image); - read_buf_from_file(flashchip_contents, emu_chip_size, - emu_persistent_image); + if (read_buf_from_file(flashchip_contents, emu_chip_size, + emu_persistent_image)) { + msg_perr("Unable to read %s\n", emu_persistent_image); + free(flashchip_contents); + return 1; + } } else { msg_pdbg("doesn't match.\n"); } -- To view, visit https://review.coreboot.org/c/flashrom/+/67849 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: 1.1.x Gerrit-Change-Id: Icd1a72f9171e547f2081ba4bc53834a17ef7fcab Gerrit-Change-Number: 67849 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Jacob Garber <jgarber1(a)ualberta.ca> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Attention: Jacob Garber <jgarber1(a)ualberta.ca> Gerrit-MessageType: newchange

1 0

[S] Change in flashrom[1.1.x]: linux_spi: Use fgets() to read buffer size
by Nico Huber (Code Review) 26 Sep '22

26 Sep '22

Attention is currently required from: David Hendricks, Jacob Garber. Hello build bot (Jenkins), David Hendricks, Jacob Garber, I'd like you to do a code review. Please visit https://review.coreboot.org/c/flashrom/+/67848 to review the following change. Change subject: linux_spi: Use fgets() to read buffer size ...................................................................... linux_spi: Use fgets() to read buffer size Since fread() returns the number of bytes read, this currently will only check for errors if it returns 0 (i.e. the file was empty). However, it is possible for fread() to encounter an error after reading a few bytes, which this doesn't catch. Fix this by using fgets() instead, which will return NULL if EOF or an error is encountered, and is simpler anyway. Change-Id: I4f37c70e97149b87c6344e63a57d11ddde7638c4 Signed-off-by: Jacob Garber <jgarber1(a)ualberta.ca> Found-by: Coverity CID 1403824 Reviewed-on: https://review.coreboot.org/c/flashrom/+/34848 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: David Hendricks <david.hendricks(a)gmail.com> --- M linux_spi.c 1 file changed, 21 insertions(+), 2 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/48/67848/1 diff --git a/linux_spi.c b/linux_spi.c index 711ab4a..bcac8cb 100644 --- a/linux_spi.c +++ b/linux_spi.c @@ -138,8 +138,7 @@ } char buf[10]; - memset(buf, 0, sizeof(buf)); - if (!fread(buf, 1, sizeof(buf) - 1, fp)) { + if (!fgets(buf, sizeof(buf), fp)) { if (feof(fp)) msg_pwarn("Cannot read %s: file is empty.\n", BUF_SIZE_FROM_SYSFS); else -- To view, visit https://review.coreboot.org/c/flashrom/+/67848 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: 1.1.x Gerrit-Change-Id: I4f37c70e97149b87c6344e63a57d11ddde7638c4 Gerrit-Change-Number: 67848 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Jacob Garber <jgarber1(a)ualberta.ca> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Attention: Jacob Garber <jgarber1(a)ualberta.ca> Gerrit-MessageType: newchange

1 0

[S] Change in flashrom[1.1.x]: pickit2_spi: Fix "dead" assignment
by Nico Huber (Code Review) 26 Sep '22

26 Sep '22

Attention is currently required from: Elyes Haouas. Hello Elyes Haouas, I'd like you to do a code review. Please visit https://review.coreboot.org/c/flashrom/+/67846 to review the following change. Change subject: pickit2_spi: Fix "dead" assignment ...................................................................... pickit2_spi: Fix "dead" assignment We never read the first 'ret'. Let's check the first 'ret' and exit if it failed. Also, print the version only when the command succeeded. Found-by: scan-build 7.0.1-8 Change-Id: I4aac5e1f3bd0604b079e1fdd9b7f09f1f4fc2d7f Signed-off-by: Elyes HAOUAS <ehaouas(a)noos.fr> Reviewed-on: https://review.coreboot.org/c/flashrom/+/34403 Tested-by: Nico Huber <nico.h(a)gmx.de> Reviewed-by: Nico Huber <nico.h(a)gmx.de> --- M pickit2_spi.c 1 file changed, 26 insertions(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/46/67846/1 diff --git a/pickit2_spi.c b/pickit2_spi.c index 4354025..1d1f08c 100644 --- a/pickit2_spi.c +++ b/pickit2_spi.c @@ -119,14 +119,20 @@ uint8_t command[CMD_LENGTH] = {CMD_GET_VERSION, CMD_END_OF_BUFFER}; ret = usb_interrupt_write(pickit2_handle, ENDPOINT_OUT, (char *)command, CMD_LENGTH, DFLT_TIMEOUT); + + if (ret != CMD_LENGTH) { + msg_perr("Command Get Firmware Version failed!\n"); + return 1; + } + ret = usb_interrupt_read(pickit2_handle, ENDPOINT_IN, (char *)command, CMD_LENGTH, DFLT_TIMEOUT); - msg_pdbg("PICkit2 Firmware Version: %d.%d\n", (int)command[0], (int)command[1]); if (ret != CMD_LENGTH) { msg_perr("Command Get Firmware Version failed (%s)!\n", usb_strerror()); return 1; } + msg_pdbg("PICkit2 Firmware Version: %d.%d\n", (int)command[0], (int)command[1]); return 0; } -- To view, visit https://review.coreboot.org/c/flashrom/+/67846 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: 1.1.x Gerrit-Change-Id: I4aac5e1f3bd0604b079e1fdd9b7f09f1f4fc2d7f Gerrit-Change-Number: 67846 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Elyes Haouas <ehaouas(a)noos.fr> Gerrit-Attention: Elyes Haouas <ehaouas(a)noos.fr> Gerrit-MessageType: newchange

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit September 2022