Attention is currently required from: Edward O'Callaghan, Angel Pons.
Sam McNally has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/54906 )
Change subject: dummyflasher.c: Inline data fetch for spi workers
......................................................................
Patch Set 1: Code-Review+2
--
To view, visit https://review.coreboot.org/c/flashrom/+/54906
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings…
[View More]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-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Sam McNally <sammc(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Comment-Date: Tue, 25 May 2021 12:22:39 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
[View Less]
Attention is currently required from: Nico Huber, Edward O'Callaghan, Anastasia Klimchuk.
Angel Pons 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:
(1 comment)
File buspirate_spi.c:
https://review.coreboot.org/c/flashrom/+/52958/comment/abb279d6_b2938e2b
PS3, Line 58: unsigned char **…
[View More]bp_commbuf
> I think so, because the pointer can change after realloc (and the size of the buffer can also change […]
I'd just pass a pointer to `struct bp_spi_data`
--
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: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Tue, 25 May 2021 11:16:34 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org>
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: comment
[View Less]
Attention is currently required from: Nico Huber, Edward O'Callaghan, Anastasia Klimchuk.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/54889 )
Change subject: spi_master: Make use of new register_spi_master() API
......................................................................
Patch Set 1: Code-Review+2
--
To view, visit https://review.coreboot.org/c/flashrom/+/54889
To unsubscribe, or for help writing mail filters, visit https://review.…
[View More]coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: If72f54c28a95b402b3565fd14ea481f734e1c970
Gerrit-Change-Number: 54889
Gerrit-PatchSet: 1
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: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Tue, 25 May 2021 11:13:34 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
[View Less]
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 …
[View More]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
[View Less]
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: …
[View More]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
[View Less]
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 …
[View More]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
[View Less]
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 …
[View More]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
[View Less]