- flashrom-gerrit - mail.coreboot.org

Change in flashrom[master]: dummyflasher.c: Extract params processing into a separate function
by Angel Pons (Code Review) 25 May '21

25 May '21

Attention is currently required from: Nico Huber, Namyoon Woo, Edward O'Callaghan, Anastasia Klimchuk. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/54748 ) Change subject: dummyflasher.c: Extract params processing into a separate function ...................................................................... Patch Set 2: (3 comments) Patchset: PS2: I'm sorry that you'll have to manually rebase this change, but I *really* had to make CB:54905 CB:54908 CB:54909 CB:54910 to blow off some steam after noticing these issues. File dummyflasher.c: https://review.coreboot.org/c/flashrom/+/54748/comment/1d5e1d11_ac41efe2 PS2, Line 971: free(status); : if (errno != 0 || status == endptr) { > For another patch: this has an use-after-free bug, just like the one CB:46551 fixed. CB:54909 takes care of it. https://review.coreboot.org/c/flashrom/+/54748/comment/a55b7321_2bda9960 PS2, Line 1014: > I would initialise `data->flashchip_contents` here: […] I moved around the initialisation in CB:54908 -- To view, visit https://review.coreboot.org/c/flashrom/+/54748 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I04f55f77bb4703f1d88b2191c45a22be3c97bf87 Gerrit-Change-Number: 54748 Gerrit-PatchSet: 2 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: Namyoon Woo <namyoon(a)google.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: Namyoon Woo <namyoon(a)google.com> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Tue, 25 May 2021 11:12:02 +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]: dummyflasher.c: Fix typo in log message
by Angel Pons (Code Review) 25 May '21

25 May '21

Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/54910 ) Change subject: dummyflasher.c: Fix typo in log message ...................................................................... dummyflasher.c: Fix typo in log message multipler ---> multiple Change-Id: I16fbc0072c82ac152af5f1d65df196cdd113e03b Signed-off-by: Angel Pons <th3fanbus(a)gmail.com> --- M dummyflasher.c 1 file changed, 1 insertion(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/10/54910/1 diff --git a/dummyflasher.c b/dummyflasher.c index 5109483..cf657df 100644 --- a/dummyflasher.c +++ b/dummyflasher.c @@ -841,7 +841,7 @@ if (tmp) { size = strtol(tmp, NULL, 10); if (size <= 0 || (size % 1024 != 0)) { - msg_perr("%s: Chip size is not a multipler of 1024: %s\n", + msg_perr("%s: Chip size is not a multiple of 1024: %s\n", __func__, tmp); free(tmp); return 1; -- To view, visit https://review.coreboot.org/c/flashrom/+/54910 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I16fbc0072c82ac152af5f1d65df196cdd113e03b Gerrit-Change-Number: 54910 Gerrit-PatchSet: 1 Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: newchange

1 0

Change in flashrom[master]: dummyflasher.c: Move `flashchip_contents` allocation
by Angel Pons (Code Review) 25 May '21

25 May '21

Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/54908 ) Change subject: dummyflasher.c: Move `flashchip_contents` allocation ...................................................................... dummyflasher.c: Move `flashchip_contents` allocation Place `flashchip_contents` allocation next to the code that initialises the newly-allocated buffer. This also avoids leaking it if the handling of `spi_status` fails. Change-Id: Ie907ac88dfe4ca018c97d9ce6ce042b4ffacf36a Signed-off-by: Angel Pons <th3fanbus(a)gmail.com> --- M dummyflasher.c 1 file changed, 6 insertions(+), 6 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/08/54908/1 diff --git a/dummyflasher.c b/dummyflasher.c index 8955254..78f3837 100644 --- a/dummyflasher.c +++ b/dummyflasher.c @@ -967,12 +967,6 @@ } free(tmp); - data->flashchip_contents = malloc(data->emu_chip_size); - if (!data->flashchip_contents) { - msg_perr("Out of memory!\n"); - return 1; - } - #ifdef EMULATE_SPI_CHIP status = extract_programmer_param("spi_status"); if (status) { @@ -990,6 +984,12 @@ } #endif + data->flashchip_contents = malloc(data->emu_chip_size); + if (!data->flashchip_contents) { + msg_perr("Out of memory!\n"); + return 1; + } + msg_pdbg("Filling fake flash chip with 0x%02x, size %i\n", data->erase_to_zero ? 0x00 : 0xff, data->emu_chip_size); memset(data->flashchip_contents, data->erase_to_zero ? 0x00 : 0xff, data->emu_chip_size); -- To view, visit https://review.coreboot.org/c/flashrom/+/54908 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ie907ac88dfe4ca018c97d9ce6ce042b4ffacf36a Gerrit-Change-Number: 54908 Gerrit-PatchSet: 1 Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: newchange

1 0

Change in flashrom[master]: dummyflasher.c: Prevent use-after-free bug
by Angel Pons (Code Review) 25 May '21

25 May '21

Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/54909 ) Change subject: dummyflasher.c: Prevent use-after-free bug ...................................................................... dummyflasher.c: Prevent use-after-free bug The memory for the `status` string is aliased by the `endptr` pointer. 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: I1b56834004fe18918213a7df0a09a8a7ecb56985 Signed-off-by: Angel Pons <th3fanbus(a)gmail.com> --- M dummyflasher.c 1 file changed, 2 insertions(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/09/54909/1 diff --git a/dummyflasher.c b/dummyflasher.c index 78f3837..5109483 100644 --- a/dummyflasher.c +++ b/dummyflasher.c @@ -973,12 +973,13 @@ char *endptr; errno = 0; data->emu_status = strtoul(status, &endptr, 0); - free(status); if (errno != 0 || status == endptr) { + free(status); msg_perr("Error: initial status register specified, " "but the value could not be converted.\n"); return 1; } + free(status); msg_pdbg("Initial status register is set to 0x%02x.\n", data->emu_status); } -- To view, visit https://review.coreboot.org/c/flashrom/+/54909 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I1b56834004fe18918213a7df0a09a8a7ecb56985 Gerrit-Change-Number: 54909 Gerrit-PatchSet: 1 Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: newchange

1 0

Change in flashrom[master]: dummyflasher.c: Inline data fetch for spi workers
by Edward O'Callaghan (Code Review) 25 May '21

25 May '21

Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/54906 ) Change subject: dummyflasher.c: Inline data fetch for spi workers ...................................................................... dummyflasher.c: Inline data fetch for spi workers The spi callbacks do not need par logic to fetch the data field. Instead of going though get_data_from_context() just fetch 'data' directly out of mst.spi. This leads us towards a path of removing dummy_buses_supported from global state. BUG=none BRANCH=none TEST=`./flashrom -r /tmp/fwupdater.apnSQQ -p dummy:emulate=VARIABLE_SIZE,image=test_update.sh.tmp.emu,size=8388608` Change-Id: I63b8d8861df75f52f241f09614146990fdfe59ed Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M dummyflasher.c 1 file changed, 2 insertions(+), 2 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/06/54906/1 diff --git a/dummyflasher.c b/dummyflasher.c index 4661f3d..587fc61 100644 --- a/dummyflasher.c +++ b/dummyflasher.c @@ -129,7 +129,7 @@ static int dummy_spi_write_256(struct flashctx *flash, const uint8_t *buf, unsigned int start, unsigned int len) { - struct emu_data *emu_data = get_data_from_context(flash); + struct emu_data *emu_data = flash->mst->spi.data; return spi_write_chunked(flash, buf, start, len, emu_data->spi_write_256_chunksize); } @@ -570,7 +570,7 @@ unsigned char *readarr) { unsigned int i; - struct emu_data *emu_data = get_data_from_context(flash); + struct emu_data *emu_data = flash->mst->spi.data; if (!emu_data) { msg_perr("No data in flash context!\n"); return 1; -- To view, visit https://review.coreboot.org/c/flashrom/+/54906 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I63b8d8861df75f52f241f09614146990fdfe59ed Gerrit-Change-Number: 54906 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: newchange

1 0

Change in flashrom[master]: dummyflasher.c: Get rid of get_data_from_context()
by Edward O'Callaghan (Code Review) 25 May '21

25 May '21

Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/54907 ) Change subject: dummyflasher.c: Get rid of get_data_from_context() ...................................................................... dummyflasher.c: Get rid of get_data_from_context() Relying on the global state 'dummy_buses_supported' to determine the member master struct [mst.par or mst.spi] is both buggy and ultimately unnecessary. It became apart after commit 4eef651ff503f81b77 just how fragile this really was as the 'defaults' simultaneously selected both buses causing get_data_from_context() to fall-though however memory happened to workout by chance due to the union. With the member master structs now being struct fields the subtle bug is more apparent. BUG=none BRANCH=none TEST=`./flashrom -r /tmp/fwupdater.apnSQQ -p dummy:emulate=VARIABLE_SIZE,image=test_update.sh.tmp.emu,size=8388608` Change-Id: I07a34faf50ff0679cb3d6bc683142f82160010b1 Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M dummyflasher.c 1 file changed, 2 insertions(+), 14 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/07/54907/1 diff --git a/dummyflasher.c b/dummyflasher.c index 587fc61..b7cfab8 100644 --- a/dummyflasher.c +++ b/dummyflasher.c @@ -103,18 +103,6 @@ #endif #endif -static enum chipbustype dummy_buses_supported = BUS_NONE; - -static struct emu_data* get_data_from_context(const struct flashctx *flash) -{ - if (dummy_buses_supported & BUS_NONSPI) - return (struct emu_data *)flash->mst->par.data; - else if (dummy_buses_supported & BUS_SPI) - return (struct emu_data *)flash->mst->spi.data; - - return NULL; /* buses was set to BUS_NONE. */ -} - void *dummy_map(const char *descr, uintptr_t phys_addr, size_t len) { msg_pspew("%s: Mapping %s, 0x%zx bytes at 0x%0*" PRIxPTR "\n", @@ -688,7 +676,7 @@ /* Convert the parameters to lowercase. */ tolower_string(bustext); - dummy_buses_supported = BUS_NONE; + enum chipbustype dummy_buses_supported = BUS_NONE; if (strstr(bustext, "parallel")) { dummy_buses_supported |= BUS_PARALLEL; msg_pdbg("Enabling support for %s flash.\n", "parallel"); @@ -1041,7 +1029,7 @@ int probe_variable_size(struct flashctx *flash) { unsigned int i; - const struct emu_data *emu_data = get_data_from_context(flash); + const struct emu_data *emu_data = flash->mst->spi.data; /* Skip the probing if we don't emulate this chip. */ if (!emu_data || emu_data->emu_chip != EMULATE_VARIABLE_SIZE) -- To view, visit https://review.coreboot.org/c/flashrom/+/54907 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I07a34faf50ff0679cb3d6bc683142f82160010b1 Gerrit-Change-Number: 54907 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: newchange

1 0

Change in flashrom[master]: dummyflasher.c: Extract params processing into a separate function
by Angel Pons (Code Review) 25 May '21

25 May '21

Attention is currently required from: Nico Huber, Namyoon Woo, Edward O'Callaghan, Anastasia Klimchuk. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/54748 ) Change subject: dummyflasher.c: Extract params processing into a separate function ...................................................................... Patch Set 2: (4 comments) File dummyflasher.c: https://review.coreboot.org/c/flashrom/+/54748/comment/edc69df6_255d1d72 PS2, Line 971: free(status); : if (errno != 0 || status == endptr) { For another patch: this has an use-after-free bug, just like the one CB:46551 fixed. https://review.coreboot.org/c/flashrom/+/54748/comment/f2b3e60e_8c711f43 PS2, Line 975: return 1; This will result in `data->flashchip_contents` leaking. I'd move the initialisation out of init_data(), see comment below. https://review.coreboot.org/c/flashrom/+/54748/comment/56c51421_15cfc44e PS2, Line 1014: I would initialise `data->flashchip_contents` here: data->flashchip_contents = malloc(data->emu_chip_size); if (!data->flashchip_contents) { msg_perr("Out of memory!\n"); free(data); return 1; } https://review.coreboot.org/c/flashrom/+/54748/comment/50bfd870_76ed5651 PS2, Line 1035: emu_persistent_image Meh, this is leaking: CB:54905 -- To view, visit https://review.coreboot.org/c/flashrom/+/54748 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I04f55f77bb4703f1d88b2191c45a22be3c97bf87 Gerrit-Change-Number: 54748 Gerrit-PatchSet: 2 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: Namyoon Woo <namyoon(a)google.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: Namyoon Woo <namyoon(a)google.com> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Tue, 25 May 2021 10:53:07 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: dummyflasher.c: Don't leak `emu_persistent_image`
by Angel Pons (Code Review) 25 May '21

25 May '21

Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/54905 ) Change subject: dummyflasher.c: Don't leak `emu_persistent_image` ...................................................................... dummyflasher.c: Don't leak `emu_persistent_image` Ensure `emu_persistent_image` doesn't end up leaking memory. Change-Id: I76529973cefcc6a1472681e1f4da8239fcbf07a6 Signed-off-by: Angel Pons <th3fanbus(a)gmail.com> --- M dummyflasher.c 1 file changed, 3 insertions(+), 2 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/05/54905/1 diff --git a/dummyflasher.c b/dummyflasher.c index 2f39c98..8955254 100644 --- a/dummyflasher.c +++ b/dummyflasher.c @@ -646,9 +646,8 @@ write_buf_to_file(emu_data->flashchip_contents, emu_data->emu_chip_size, emu_data->emu_persistent_image); - free(emu_data->emu_persistent_image); - emu_data->emu_persistent_image = NULL; } + free(emu_data->emu_persistent_image); free(emu_data->flashchip_contents); } #endif @@ -1012,6 +1011,7 @@ if (read_buf_from_file(data->flashchip_contents, data->emu_chip_size, data->emu_persistent_image)) { msg_perr("Unable to read %s\n", data->emu_persistent_image); + free(data->emu_persistent_image); free(data->flashchip_contents); return 1; } @@ -1023,6 +1023,7 @@ dummy_init_out: if (register_shutdown(dummy_shutdown, data)) { + free(data->emu_persistent_image); free(data->flashchip_contents); free(data); return 1; -- To view, visit https://review.coreboot.org/c/flashrom/+/54905 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I76529973cefcc6a1472681e1f4da8239fcbf07a6 Gerrit-Change-Number: 54905 Gerrit-PatchSet: 1 Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: newchange

1 0

Change in flashrom[master]: par_masters: Reshuffle to remove forward declarations
by Angel Pons (Code Review) 25 May '21

25 May '21

Attention is currently required from: Edward O'Callaghan. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/54873 ) Change subject: par_masters: Reshuffle to remove forward declarations ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/54873 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I88e89992380195fee7c9de7ec57502ab980ec5df Gerrit-Change-Number: 54873 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(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, 25 May 2021 09:07:06 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: buspirate_spi.c: Refactor singleton states into reentrant pattern
by Anastasia Klimchuk (Code Review) 25 May '21

25 May '21

Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52958 ) Change subject: buspirate_spi.c: Refactor singleton states into reentrant pattern ...................................................................... Patch Set 3: (2 comments) File buspirate_spi.c: https://review.coreboot.org/c/flashrom/+/52958/comment/a8785a3c_e81c42ef PS3, Line 58: unsigned char **bp_commbuf > does this actually need double indirection? I think so, because the pointer can change after realloc (and the size of the buffer can also change) - but please correct me if I am wrong. https://review.coreboot.org/c/flashrom/+/52958/comment/ca94ff25_803e2b85 PS3, Line 221: unsigned char *const bp_commbuf = bp_data->bp_commbuf; > perhaps just move this up a few lines to the functions preamble so that the branch line above can be […] I thought that buspirate_commbuf_grow can change the pointer, and I can have this local variable as constant pointer after buspirate_commbuf_grow. Both your comments have the same answer, depending on whether realloc can change the pointer? -- To view, visit https://review.coreboot.org/c/flashrom/+/52958 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I418bbfff15fb126b042fbc9be09dbf59f4d243b8 Gerrit-Change-Number: 52958 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: 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: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Tue, 25 May 2021 05:21:47 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: comment

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit