July 2021 - flashrom-gerrit - mail.coreboot.org

Change in flashrom[master]: meson/: Add missing config for NI845_SPI drv
by Angel Pons (Code Review) 27 Jul '21

27 Jul '21

Attention is currently required from: Edward O'Callaghan. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/54849 ) Change subject: meson/: Add missing config for NI845_SPI drv ...................................................................... Patch Set 1: Code-Review+1 -- To view, visit https://review.coreboot.org/c/flashrom/+/54849 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I85387e94f1b159733d0124f1a6dfde1e3d16827f Gerrit-Change-Number: 54849 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Comment-Date: Tue, 27 Jul 2021 13:47:30 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ni845x_spi: handle PROGRAMFILES(X86) env var properly
by Angel Pons (Code Review) 27 Jul '21

27 Jul '21

Attention is currently required from: Nico Huber, Miklós Márton. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/56636 ) Change subject: ni845x_spi: handle PROGRAMFILES(X86) env var properly ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: Would escaping the parentheses work? -- To view, visit https://review.coreboot.org/c/flashrom/+/56636 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I397619a5038567d649a417ce6b9d8ac9e1c8c67b Gerrit-Change-Number: 56636 Gerrit-PatchSet: 1 Gerrit-Owner: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-Comment-Date: Tue, 27 Jul 2021 13:46:30 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ni845x_spi: Fix signed - unsigned comparisons
by Angel Pons (Code Review) 27 Jul '21

27 Jul '21

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: (1 comment) File ni845x_spi.c: https://review.coreboot.org/c/flashrom/+/56637/comment/8c67118b_8ad1dd45 PS1, Line 558: strlen(CS_str) > For another patch: This is an use-after-free bug. Note that the alternative approach proposed below avoids the use-after-free bug. -- To view, visit https://review.coreboot.org/c/flashrom/+/56637 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I48ef927aa28433fb0e3b3a1f3fb2e797095e53bd Gerrit-Change-Number: 56637 Gerrit-PatchSet: 1 Gerrit-Owner: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Tue, 27 Jul 2021 13:35:34 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ni845x_spi: Fix signed - unsigned comparisons
by Angel Pons (Code Review) 27 Jul '21

27 Jul '21

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. -- To view, visit https://review.coreboot.org/c/flashrom/+/56637 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I48ef927aa28433fb0e3b3a1f3fb2e797095e53bd Gerrit-Change-Number: 56637 Gerrit-PatchSet: 1 Gerrit-Owner: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Tue, 27 Jul 2021 13:35:02 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ni845x_spi: Fix signed - unsigned comparisons
by Miklós Márton (Code Review) 27 Jul '21

27 Jul '21

Miklós Márton has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/56637 ) Change subject: ni845x_spi: Fix signed - unsigned comparisons ...................................................................... ni845x_spi: Fix signed - unsigned comparisons Change-Id: I48ef927aa28433fb0e3b3a1f3fb2e797095e53bd Signed-off-by: Miklós Márton <martonmiklosqdev(a)gmail.com> --- M ni845x_spi.c 1 file changed, 4 insertions(+), 4 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/37/56637/1 diff --git a/ni845x_spi.c b/ni845x_spi.c index 2153510..b19ddd4 100644 --- a/ni845x_spi.c +++ b/ni845x_spi.c @@ -185,7 +185,7 @@ device_pid = pid; - if (!serial || strtol(serial, NULL, 16) == serial_as_number) + if (!serial || (unsigned long int)strtol(serial, NULL, 16) == serial_as_number) break; if (found_devices_count > 1) { @@ -222,7 +222,7 @@ uint16_t *set_io_voltage_mV, enum voltage_coerce_mode coerce_mode) { - int i = 0; + unsigned int i = 0; uint8_t selected_voltage_100mV = 0; uint8_t requested_io_voltage_100mV = 0; @@ -480,7 +480,7 @@ return 0; } -static int ni845x_spi_transmit(struct flashctx *flash, +static int ni845x_spi_transmit(const struct flashctx *flash, unsigned int write_cnt, unsigned int read_cnt, const unsigned char *write_arr, @@ -555,7 +555,7 @@ if (CS_str) { CS_number = CS_str[0] - '0'; free(CS_str); - if (strlen(CS_str) > 1 || CS_number < 0 || 7 < CS_number) { + if (strlen(CS_str) > 1 || 7 < CS_number) { msg_perr("Only CS 0-7 supported\n"); return 1; } -- To view, visit https://review.coreboot.org/c/flashrom/+/56637 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I48ef927aa28433fb0e3b3a1f3fb2e797095e53bd Gerrit-Change-Number: 56637 Gerrit-PatchSet: 1 Gerrit-Owner: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-MessageType: newchange

1 0

Change in flashrom[master]: meson/: Add missing config for NI845_SPI drv
by Miklós Márton (Code Review) 27 Jul '21

27 Jul '21

Attention is currently required from: Edward O'Callaghan. Miklós Márton has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/54849 ) Change subject: meson/: Add missing config for NI845_SPI drv ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: Hi folks, I have a VM installation with mingw setup which is capable to build the ni845x_spi. (Again \o/) I could do some tests with the build with meson. Which version shall I install from meson? -- To view, visit https://review.coreboot.org/c/flashrom/+/54849 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I85387e94f1b159733d0124f1a6dfde1e3d16827f Gerrit-Change-Number: 54849 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Comment-Date: Tue, 27 Jul 2021 12:43:01 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ni845x_spi: handle PROGRAMFILES(X86) env var properly
by Miklós Márton (Code Review) 27 Jul '21

27 Jul '21

Miklós Márton has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/56636 ) Change subject: ni845x_spi: handle PROGRAMFILES(X86) env var properly ...................................................................... ni845x_spi: handle PROGRAMFILES(X86) env var properly The PROGRAMFILES(X86) envvar contains brackets which could not be interpreted by the Makefile's interpreter. A sed based tweak have been added to extract the variable value from the env command output. The prefixed include and linker path with this (now correctly extracted) prefix only added to the compilation flags if it differ from the PROGRAMFILES variable. Change-Id: I397619a5038567d649a417ce6b9d8ac9e1c8c67b Signed-off-by: Miklós Márton <martonmiklosqdev(a)gmail.com> --- M Makefile 1 file changed, 11 insertions(+), 2 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/36/56636/1 diff --git a/Makefile b/Makefile index a021267..27f66a9 100644 --- a/Makefile +++ b/Makefile @@ -744,9 +744,18 @@ # if the user did not specified the NI-845x headers/lib path # do a guess for both 32 and 64 bit Windows versions NI845X_LIBS += -L'${PROGRAMFILES}\National Instruments\NI-845x\MS Visual C' -NI845X_LIBS += -L'${PROGRAMFILES(x86)}\National Instruments\NI-845x\MS Visual C' NI845X_INCLUDES += -I'${PROGRAMFILES}\National Instruments\NI-845x\MS Visual C' -NI845X_INCLUDES += -I'${PROGRAMFILES(x86)}\National Instruments\NI-845x\MS Visual C' + +# hack to access env variable containing brackets... +PROGRAMFILES_X86DIR = $(shell env | sed -n "s/^PROGRAMFILES(X86)=//p") + +ifneq ($(PROGRAMFILES_X86DIR),) +ifneq ($(PROGRAMFILES_X86DIR), ${PROGRAMFILES}) +NI845X_LIBS += -L'$(PROGRAMFILES_X86DIR)\National Instruments\NI-845x\MS Visual C' +NI845X_INCLUDES += -I'$(PROGRAMFILES_X86DIR)\National Instruments\NI-845x\MS Visual C' +endif +endif + else NI845X_LIBS += -L'$(CONFIG_NI845X_LIBRARY_PATH)' NI845X_INCLUDES += -I'$(CONFIG_NI845X_LIBRARY_PATH)' -- To view, visit https://review.coreboot.org/c/flashrom/+/56636 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I397619a5038567d649a417ce6b9d8ac9e1c8c67b Gerrit-Change-Number: 56636 Gerrit-PatchSet: 1 Gerrit-Owner: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-MessageType: newchange

1 0

Change in flashrom[master]: tests: Add tests to erase a chip
by Edward O'Callaghan (Code Review) 27 Jul '21

27 Jul '21

Attention is currently required from: Angel Pons, Anastasia Klimchuk. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/56501 ) Change subject: tests: Add tests to erase a chip ...................................................................... Patch Set 4: Code-Review+1 (5 comments) Patchset: PS4: Definitely split this like you suggested but the first test looks great now! File tests/chip.c: https://review.coreboot.org/c/flashrom/+/56501/comment/1107942f_d7dac37b PS4, Line 25: int `unsigned` unless negative unlocks encode something special? https://review.coreboot.org/c/flashrom/+/56501/comment/6afff89b_9a264512 PS4, Line 52: memcpy(&g_chip_state.buf[start], buf, len); validate len does not exceed the actual buffer length. Possibly with a test assert https://review.coreboot.org/c/flashrom/+/56501/comment/cb30a29c_6a8a2cc4 PS4, Line 59: g_chip_state.unlock_calls++; : return 0; this does depend on the chip I suppose but another possible behavior is the following: ``` g_chip_state.unlock_calls++; if (g_chip_state.unlock_calls > 1) return -1; // chip already unlocked return 0; ``` not sure if that finds anything interesting or is just a wast of time? https://review.coreboot.org/c/flashrom/+/56501/comment/31de3859_2c76e3ce PS4, Line 71: memset(&g_chip_state.buf[blockaddr], 0xff, blocklen); ditto, ensure blocklen does not exceed the global buffer size. -- To view, visit https://review.coreboot.org/c/flashrom/+/56501 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I6f74bfe4e02244d24d6c837cc3d551251e7b4898 Gerrit-Change-Number: 56501 Gerrit-PatchSet: 4 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Tue, 27 Jul 2021 10:40:31 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: tests: Add tests to erase a chip
by Nico Huber (Code Review) 27 Jul '21

27 Jul '21

Attention is currently required from: Edward O'Callaghan, Angel Pons, Anastasia Klimchuk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/56501 ) Change subject: tests: Add tests to erase a chip ...................................................................... Patch Set 4: (1 comment) Patchset: PS2: > The newest test that I have added uses real spi_send_command, however some previous tests are mocking the same function. Means, we have to have two tests executables where one of executables uses real spi_send_command and second one wraps spi_send_command. Not sure if you don't know or if you tried and it turned out to be the worse solution: with `--wrap` you can literally wrap the original function. It should be available as __real_spi_send_command(). So with a little abstraction we could have a __wrap_spi_send_command() that would do the checks that it currently does and optionally calls the real spi_send_command(). -- To view, visit https://review.coreboot.org/c/flashrom/+/56501 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I6f74bfe4e02244d24d6c837cc3d551251e7b4898 Gerrit-Change-Number: 56501 Gerrit-PatchSet: 4 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Tue, 27 Jul 2021 09:19:18 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: spi_master: Use new API to register shutdown function
by Miklós Márton (Code Review) 27 Jul '21

27 Jul '21

Attention is currently required from: Nico Huber, Thomas Heijligen, Angel Pons, Anastasia Klimchuk. Miklós Márton has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/56103 ) Change subject: spi_master: Use new API to register shutdown function ...................................................................... Patch Set 3: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/56103 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ib60300f9ddb295a255d5ef3f8da0e07064207140 Gerrit-Change-Number: 56103 Gerrit-PatchSet: 3 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Tue, 27 Jul 2021 09:03:09 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit July 2021