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

Change in flashrom[master]: lspcon_i2c_spi: support a devpath option
by Angel Pons (Code Review) 12 Apr '21

12 Apr '21

Attention is currently required from: Peter Marheine. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/51967 ) Change subject: lspcon_i2c_spi: support a devpath option ...................................................................... Patch Set 3: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/51967 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Id2adf8a307b9205ce5e5804a6c3e22f19d0c34c9 Gerrit-Change-Number: 51967 Gerrit-PatchSet: 3 Gerrit-Owner: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Daniel Campello <campello(a)chromium.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Comment-Date: Mon, 12 Apr 2021 08:08:52 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

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

12 Apr '21

Attention is currently required from: Edward O'Callaghan. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52256 ) Change subject: ft2232_spi.c: Refactor singleton states into reentrant pattern ...................................................................... Patch Set 2: (23 comments) Patchset: PS2: thank you ! File ft2232_spi.c: https://review.coreboot.org/c/flashrom/+/52256/comment/e4cf2127_098b7881 PS1, Line 108: struct ftdi_context *ftdic_context; > instead of making this a pointer how about just `struct ftdi_context ftdic_context;` Done https://review.coreboot.org/c/flashrom/+/52256/comment/42f8b39d_e094fcb8 PS1, Line 168: struct ftdi_context *ftdic = spi_data->ftdic_context; > remove I modified this line but I keep it because ftdic is used 4 times here. https://review.coreboot.org/c/flashrom/+/52256/comment/8926ed9d_b62e1759 PS1, Line 186: free(ftdic); > remove Done https://review.coreboot.org/c/flashrom/+/52256/comment/d756b4e7_00b228a3 PS1, Line 198: s > & Done https://review.coreboot.org/c/flashrom/+/52256/comment/7702e290_6363250d PS1, Line 322: struct ftdi_context *ftdic; > `struct ftdi_context ftdic;` Done https://review.coreboot.org/c/flashrom/+/52256/comment/4823a08d_ac2c2a03 PS1, Line 534: ftdic = calloc(1, sizeof(struct ftdi_context)); : if (!ftdic) { : msg_perr("Unable to allocate memory for ftdi_context.\n"); : return SPI_GENERIC_ERROR; : } > remove Done https://review.coreboot.org/c/flashrom/+/52256/comment/db0af36b_e3b5b89d PS1, Line 540: ftdic > & Done https://review.coreboot.org/c/flashrom/+/52256/comment/82f60510_2dcc9a45 PS1, Line 542: free(ftdic); > remove Done https://review.coreboot.org/c/flashrom/+/52256/comment/d75a8eab_126cca48 PS1, Line 546: f > & Done https://review.coreboot.org/c/flashrom/+/52256/comment/651d1193_71a61fb2 PS1, Line 547: f > & Done https://review.coreboot.org/c/flashrom/+/52256/comment/67a68714_8ee2a3df PS1, Line 551: f > & Done https://review.coreboot.org/c/flashrom/+/52256/comment/5f5a6cad_030a721d PS1, Line 556: f > & Done https://review.coreboot.org/c/flashrom/+/52256/comment/c4fc9043_2558a401 PS1, Line 557: free(ftdic); > remove Done https://review.coreboot.org/c/flashrom/+/52256/comment/f5543ea5_e1c94ef3 PS1, Line 561: f > ftdic. […] Done https://review.coreboot.org/c/flashrom/+/52256/comment/d1315459_98fd335c PS1, Line 562: ftdic->type > ftdic. […] Done https://review.coreboot.org/c/flashrom/+/52256/comment/57d46a7f_c49b725f PS1, Line 566: f > & Done https://review.coreboot.org/c/flashrom/+/52256/comment/38f4d8ce_3732642a PS1, Line 567: f > & Done https://review.coreboot.org/c/flashrom/+/52256/comment/03bcf9b2_64c2f116 PS1, Line 618: f > & Done https://review.coreboot.org/c/flashrom/+/52256/comment/686290d2_fd4c1081 PS1, Line 626: spi_data->ftdic_context = ftdic; > memcpy() the local stack allocated one into the field. Done https://review.coreboot.org/c/flashrom/+/52256/comment/7abdd72a_6009df85 PS1, Line 639: f > & Done https://review.coreboot.org/c/flashrom/+/52256/comment/43a77e4a_164fe1fa PS1, Line 641: f > & Done https://review.coreboot.org/c/flashrom/+/52256/comment/bcdf5041_5aeebbe0 PS1, Line 644: free(ftdic); > remove Done -- To view, visit https://review.coreboot.org/c/flashrom/+/52256 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I67518a58b4f35e0edaf06ac09c9374bdf06db0df Gerrit-Change-Number: 52256 Gerrit-PatchSet: 2 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> 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: Mon, 12 Apr 2021 05:15:59 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: comment

1 0

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

12 Apr '21

Attention is currently required from: Anastasia Klimchuk. Hello build bot (Jenkins), Edward O'Callaghan, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/52256 to look at the new patch set (#2). Change subject: ft2232_spi.c: Refactor singleton states into reentrant pattern ...................................................................... ft2232_spi.c: Refactor singleton states into reentrant pattern Move global singleton states into a struct and store within the spi_master data field for the life-time of the driver. This is one of the steps on the way to move spi_master data memory management behind the initialisation API, for more context see other patches under the same topic "register_master_api". TEST=builds BUG=b:140394053 Change-Id: I67518a58b4f35e0edaf06ac09c9374bdf06db0df Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org> --- M ft2232_spi.c 1 file changed, 53 insertions(+), 34 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/56/52256/2 -- To view, visit https://review.coreboot.org/c/flashrom/+/52256 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I67518a58b4f35e0edaf06ac09c9374bdf06db0df Gerrit-Change-Number: 52256 Gerrit-PatchSet: 2 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: ft2232_spi.c: Refactor singleton states into reentrant pattern
by Edward O'Callaghan (Code Review) 12 Apr '21

12 Apr '21

Attention is currently required from: Anastasia Klimchuk. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52256 ) Change subject: ft2232_spi.c: Refactor singleton states into reentrant pattern ...................................................................... Patch Set 1: (22 comments) File ft2232_spi.c: https://review.coreboot.org/c/flashrom/+/52256/comment/9c2d2dda_f49356c3 PS1, Line 108: struct ftdi_context *ftdic_context; instead of making this a pointer how about just `struct ftdi_context ftdic_context;` https://review.coreboot.org/c/flashrom/+/52256/comment/50e78642_ce2b60cb PS1, Line 168: struct ftdi_context *ftdic = spi_data->ftdic_context; remove https://review.coreboot.org/c/flashrom/+/52256/comment/6bfcd85d_7d650ddb PS1, Line 186: free(ftdic); remove https://review.coreboot.org/c/flashrom/+/52256/comment/3f8af588_a2dc4b2c PS1, Line 198: s & https://review.coreboot.org/c/flashrom/+/52256/comment/5dfcbdca_b2739b63 PS1, Line 322: struct ftdi_context *ftdic; `struct ftdi_context ftdic;` https://review.coreboot.org/c/flashrom/+/52256/comment/ec787c70_90b0b995 PS1, Line 534: ftdic = calloc(1, sizeof(struct ftdi_context)); : if (!ftdic) { : msg_perr("Unable to allocate memory for ftdi_context.\n"); : return SPI_GENERIC_ERROR; : } remove https://review.coreboot.org/c/flashrom/+/52256/comment/ad506d08_7dd81e27 PS1, Line 540: ftdic & https://review.coreboot.org/c/flashrom/+/52256/comment/98c8148d_0e301d3b PS1, Line 542: free(ftdic); remove https://review.coreboot.org/c/flashrom/+/52256/comment/f864be64_acffd3a7 PS1, Line 546: f & https://review.coreboot.org/c/flashrom/+/52256/comment/1ec0d13b_69b25f1b PS1, Line 547: f & https://review.coreboot.org/c/flashrom/+/52256/comment/b8098cff_5a841c94 PS1, Line 551: f & https://review.coreboot.org/c/flashrom/+/52256/comment/ca612059_0d8644c9 PS1, Line 556: f & https://review.coreboot.org/c/flashrom/+/52256/comment/89ac3ba3_f6cbf16d PS1, Line 557: free(ftdic); remove https://review.coreboot.org/c/flashrom/+/52256/comment/1e8b1840_16192fce PS1, Line 561: f ftdic.type https://review.coreboot.org/c/flashrom/+/52256/comment/d892d295_1ef687d5 PS1, Line 562: ftdic->type ftdic.type https://review.coreboot.org/c/flashrom/+/52256/comment/0c5c8f19_b7f45a49 PS1, Line 566: f & https://review.coreboot.org/c/flashrom/+/52256/comment/38225c9a_824cd034 PS1, Line 567: f & https://review.coreboot.org/c/flashrom/+/52256/comment/e8468ba1_9c79044a PS1, Line 618: f & https://review.coreboot.org/c/flashrom/+/52256/comment/48836d5c_24beafc1 PS1, Line 626: spi_data->ftdic_context = ftdic; memcpy() the local stack allocated one into the field. https://review.coreboot.org/c/flashrom/+/52256/comment/d662fd23_da854a27 PS1, Line 639: f & https://review.coreboot.org/c/flashrom/+/52256/comment/6a2ccfba_6ca30d8e PS1, Line 641: f & https://review.coreboot.org/c/flashrom/+/52256/comment/74f7e6f4_b1a5c5aa PS1, Line 644: free(ftdic); remove -- To view, visit https://review.coreboot.org/c/flashrom/+/52256 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I67518a58b4f35e0edaf06ac09c9374bdf06db0df Gerrit-Change-Number: 52256 Gerrit-PatchSet: 1 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Mon, 12 Apr 2021 03:46:45 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: lspcon_i2c_spi: support a devpath option
by Peter Marheine (Code Review) 12 Apr '21

12 Apr '21

Attention is currently required from: Angel Pons. Peter Marheine has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/51967 ) Change subject: lspcon_i2c_spi: support a devpath option ...................................................................... Patch Set 3: (4 comments) File lspcon_i2c_spi.c: https://review.coreboot.org/c/flashrom/+/51967/comment/67689049_f24a3d89 PS2, Line 457: __func__); > nit: `__func__` probably fits on the previous line Done https://review.coreboot.org/c/flashrom/+/51967/comment/b17fd475_96290503 PS2, Line 463: __func__); > nit: `__func__` most likely fits on the previous line Done https://review.coreboot.org/c/flashrom/+/51967/comment/1512b0d9_1d11c202 PS2, Line 489: > nit: replace 8 spaces with a tab Done https://review.coreboot.org/c/flashrom/+/51967/comment/fb3374fa_ec8dea86 PS2, Line 492: } else if (device_path != NULL) { > The first two cases are error paths that return an error, whereas the last two cases are the "normal […] Done -- To view, visit https://review.coreboot.org/c/flashrom/+/51967 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Id2adf8a307b9205ce5e5804a6c3e22f19d0c34c9 Gerrit-Change-Number: 51967 Gerrit-PatchSet: 3 Gerrit-Owner: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Daniel Campello <campello(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Mon, 12 Apr 2021 00:18:19 +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]: lspcon_i2c_spi: support a devpath option
by Peter Marheine (Code Review) 12 Apr '21

12 Apr '21

Attention is currently required from: Peter Marheine. Hello build bot (Jenkins), Edward O'Callaghan, Angel Pons, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/51967 to look at the new patch set (#3). Change subject: lspcon_i2c_spi: support a devpath option ...................................................................... lspcon_i2c_spi: support a devpath option Some callers may find it easier to provide the path to an I2C device at which to communicate with the device, rather than specify the bus number- doing so might involve trying to parse a path to extract the number only for flashrom to do the reverse, which is error-prone and unnecessary. This change adds support for a `devpath` option, continuing to allow `bus` and requiring only one of them be specified. TEST=Verified --flash-size outputs correct values with both devpath=/dev/i2c-7 and bus=7, as well as noting that one is required if neither is specified and only one may be specified if both are given. Signed-off-by: Peter Marheine <pmarheine(a)chromium.org> Change-Id: Id2adf8a307b9205ce5e5804a6c3e22f19d0c34c9 --- M i2c_helper.h M i2c_helper_linux.c M lspcon_i2c_spi.c 3 files changed, 79 insertions(+), 50 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/67/51967/3 -- To view, visit https://review.coreboot.org/c/flashrom/+/51967 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Id2adf8a307b9205ce5e5804a6c3e22f19d0c34c9 Gerrit-Change-Number: 51967 Gerrit-PatchSet: 3 Gerrit-Owner: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Daniel Campello <campello(a)chromium.org> Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

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

12 Apr '21

Anastasia Klimchuk has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/52256 ) Change subject: ft2232_spi.c: Refactor singleton states into reentrant pattern ...................................................................... ft2232_spi.c: Refactor singleton states into reentrant pattern Move global singleton states into a struct and store within the spi_master data field for the life-time of the driver. This is one of the steps on the way to move spi_master data memory management behind the initialisation API, for more context see other patches under the same topic "register_master_api". TEST=builds BUG=b:140394053 Change-Id: I67518a58b4f35e0edaf06ac09c9374bdf06db0df Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org> --- M ft2232_spi.c 1 file changed, 43 insertions(+), 13 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/56/52256/1 diff --git a/ft2232_spi.c b/ft2232_spi.c index 1fb795b..dc7cab0 100644 --- a/ft2232_spi.c +++ b/ft2232_spi.c @@ -98,13 +98,15 @@ * The pin signal direction bit offsets follow the same order; 0 means that * pin at the matching bit index is an input, 1 means pin is an output. * - * The default values (set below) are used for most devices: + * The default values (set below in ft2232_spi_init) are used for most devices: * value: 0x08 CS=high, DI=low, DO=low, SK=low * dir: 0x0b CS=output, DI=input, DO=output, SK=output */ -static uint8_t pinlvl = 0x08; -static uint8_t pindir = 0x0b; -static struct ftdi_context ftdic_context; +struct ft2232_data { + uint8_t pinlvl; + uint8_t pindir; + struct ftdi_context *ftdic_context; +}; static const char *get_ft2232_devicename(int ft2232_vid, int ft2232_type) { @@ -162,7 +164,8 @@ static int ft2232_shutdown(void *data) { int f; - struct ftdi_context *ftdic = (struct ftdi_context *) data; + struct ft2232_data *spi_data = (struct ft2232_data *) data; + struct ftdi_context *ftdic = spi_data->ftdic_context; unsigned char buf[3]; msg_pdbg("Releasing I/Os\n"); @@ -180,6 +183,8 @@ return f; } + free(ftdic); + free(spi_data); return 0; } @@ -189,7 +194,8 @@ const unsigned char *writearr, unsigned char *readarr) { - struct ftdi_context *ftdic = &ftdic_context; + struct ft2232_data *spi_data = flash->mst->spi.data; + struct ftdi_context *ftdic = spi_data->ftdic_context; static unsigned char *buf = NULL; /* failed is special. We use bitwise ops, but it is essentially bool. */ int i = 0, ret = 0, failed = 0; @@ -220,8 +226,8 @@ */ msg_pspew("Assert CS#\n"); buf[i++] = SET_BITS_LOW; - buf[i++] = ~ 0x08 & pinlvl; /* assert CS (3rd) bit only */ - buf[i++] = pindir; + buf[i++] = ~ 0x08 & spi_data->pinlvl; /* assert CS (3rd) bit only */ + buf[i++] = spi_data->pindir; if (writecnt) { buf[i++] = MPSSE_DO_WRITE | MPSSE_WRITE_NEG; @@ -261,8 +267,8 @@ msg_pspew("De-assert CS#\n"); buf[i++] = SET_BITS_LOW; - buf[i++] = pinlvl; - buf[i++] = pindir; + buf[i++] = spi_data->pinlvl; + buf[i++] = spi_data->pindir; ret = send_buf(ftdic, buf, i); failed |= ret; if (ret) @@ -271,7 +277,7 @@ return failed ? -1 : 0; } -static const struct spi_master spi_master_ft2232 = { +static struct spi_master spi_master_ft2232 = { .features = SPI_MASTER_4BA, .max_data_read = 64 * 1024, .max_data_write = 256, @@ -286,7 +292,6 @@ int ft2232_spi_init(void) { int ret = 0; - struct ftdi_context *ftdic = &ftdic_context; unsigned char buf[512]; int ft2232_vid = FTDI_VID; int ft2232_type = FTDI_FT4232H_PID; @@ -312,6 +317,11 @@ char *arg; double mpsse_clk; + uint8_t pinlvl = 0x08; + uint8_t pindir = 0x0b; + struct ftdi_context *ftdic; + struct ft2232_data *spi_data; + arg = extract_programmer_param("type"); if (arg) { if (!strcasecmp(arg, "2232H")) { @@ -521,8 +531,15 @@ (ft2232_interface == INTERFACE_B) ? "B" : (ft2232_interface == INTERFACE_C) ? "C" : "D"); + ftdic = calloc(1, sizeof(struct ftdi_context)); + if (!ftdic) { + msg_perr("Unable to allocate memory for ftdi_context.\n"); + return SPI_GENERIC_ERROR; + } + if (ftdi_init(ftdic) < 0) { msg_perr("ftdi_init failed.\n"); + free(ftdic); return -3; } @@ -537,6 +554,7 @@ if (f < 0 && f != -5) { msg_perr("Unable to open FTDI device: %d (%s)\n", f, ftdi_get_error_string(ftdic)); + free(ftdic); return -4; } @@ -602,7 +620,17 @@ goto ftdi_err; } - register_shutdown(ft2232_shutdown, ftdic); + spi_data = calloc(1, sizeof(struct ft2232_data)); + spi_data->pinlvl = pinlvl; + spi_data->pindir = pindir; + spi_data->ftdic_context = ftdic; + + spi_master_ft2232.data = spi_data; + + if (register_shutdown(ft2232_shutdown, spi_data)) { + free(spi_data); + goto ftdi_err; + } register_spi_master(&spi_master_ft2232); return 0; @@ -612,6 +640,8 @@ msg_perr("Unable to close FTDI device: %d (%s)\n", f, ftdi_get_error_string(ftdic)); } + + free(ftdic); return ret; } -- To view, visit https://review.coreboot.org/c/flashrom/+/52256 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I67518a58b4f35e0edaf06ac09c9374bdf06db0df Gerrit-Change-Number: 52256 Gerrit-PatchSet: 1 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: newchange

1 0

Change in flashrom[master]: linux_mtd: prevent corruption of flash when stdout/stderr is closed
by Angel Pons (Code Review) 09 Apr '21

09 Apr '21

Attention is currently required from: Nico Huber, Jack Rosenthal. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52215 ) Change subject: linux_mtd: prevent corruption of flash when stdout/stderr is closed ...................................................................... Patch Set 1: Code-Review+1 -- To view, visit https://review.coreboot.org/c/flashrom/+/52215 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I751c9dd88ad1d30283b94bd2185b4f8f25569c8f Gerrit-Change-Number: 52215 Gerrit-PatchSet: 1 Gerrit-Owner: Jack Rosenthal <jrosenth(a)chromium.org> 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: Jack Rosenthal <jrosenth(a)chromium.org> Gerrit-Comment-Date: Fri, 09 Apr 2021 16:14:30 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: linux_mtd: prevent corruption of flash when stdout/stderr is closed
by Jack Rosenthal (Code Review) 09 Apr '21

09 Apr '21

Jack Rosenthal has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/52215 ) Change subject: linux_mtd: prevent corruption of flash when stdout/stderr is closed ...................................................................... linux_mtd: prevent corruption of flash when stdout/stderr is closed While it's not posixly-correct, it's possible that a user, script, or application may attempt to start flashrom with stdout or stderr closed. If this happens with an mtd device, it's possible that we'll get a file descriptor of 1 or 2, and flashrom will send garbage debug logs to the flash: # bash -c "exec >&- flashrom ..." Observed corruption: 43 40 45 42 45 44 00 00 00 00 00 00 01 00 00 00 |C@EBED..........| 00 02 00 00 63 65 73 73 66 75 6c 6c 79 0a 46 6f |....cessfully.Fo| 75 6e 64 20 50 72 6f 67 72 61 6d 6d 65 72 20 66 |und Programmer f| 6c 61 73 68 20 63 68 69 70 20 22 4f 70 61 71 75 |lash chip "Opaqu| 65 20 66 6c 61 73 68 20 63 68 69 70 22 20 28 38 |e flash chip" (8| 31 39 32 20 6b 42 2c 20 50 72 6f 67 72 61 6d 6d |192 kB, Programm| 65 72 2d 73 70 65 63 69 66 69 63 29 20 6d 61 70 |er-specific) map| 70 65 64 20 61 74 20 70 68 79 73 69 63 61 6c 20 |ped at physical | 61 64 64 72 65 73 73 20 30 78 30 30 30 30 30 30 |address 0x000000| 30 30 2e 0a ff ff ff ff ff ff ff ff ff ff ff ff |00..............| ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................| ... While for most applications, closing stdout or stderr would just lead to obsure bugs, for flashrom, we should have extra safety guards, as this could mean that we might be bricking a device instead. Add a basic safety check. Signed-off-by: Jack Rosenthal <jrosenth(a)chromium.org> Change-Id: I751c9dd88ad1d30283b94bd2185b4f8f25569c8f --- M linux_mtd.c 1 file changed, 20 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/15/52215/1 diff --git a/linux_mtd.c b/linux_mtd.c index 22702e9..aa4fe4f 100644 --- a/linux_mtd.c +++ b/linux_mtd.c @@ -340,6 +340,26 @@ msg_perr("Cannot open file stream for %s\n", dev_path); goto linux_mtd_setup_exit; } + + /* + * Safety guard against accidentally writing stdout or stderr + * to the flash. In case flashrom was started with stdout or + * stderr closed, it's possible that we will get fd 1 or fd 2 + * for the flash device. We don't want to accidentally write + * debug messages to the flash device, corrupting the + * flash. + */ + if (fileno(dev_fp) <= 2) { + fclose(dev_fp); + /* + * This message may not actually make it to the user, + * given stdout/stderr may be closed. But we can at + * least try. + */ + msg_perr("stdout or stderr is closed\n"); + 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); -- To view, visit https://review.coreboot.org/c/flashrom/+/52215 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I751c9dd88ad1d30283b94bd2185b4f8f25569c8f Gerrit-Change-Number: 52215 Gerrit-PatchSet: 1 Gerrit-Owner: Jack Rosenthal <jrosenth(a)chromium.org> Gerrit-MessageType: newchange

1 0

Change in flashrom[master]: libflashrom: Return progress state to the library user
by Daniel Campello (Code Review) 09 Apr '21

09 Apr '21

Attention is currently required from: Richard Hughes, Edward O'Callaghan, Patrick Rudolph. Daniel Campello has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/49643 ) Change subject: libflashrom: Return progress state to the library user ...................................................................... Patch Set 4: Code-Review+1 -- To view, visit https://review.coreboot.org/c/flashrom/+/49643 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I7197572bb7f19e3bdb2bde855d70a0f50fd3854c Gerrit-Change-Number: 49643 Gerrit-PatchSet: 4 Gerrit-Owner: Richard Hughes <richard(a)hughsie.com> Gerrit-Reviewer: Daniel Campello <campello(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-CC: Richard Hughes <hughsient(a)gmail.com> Gerrit-CC: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Richard Hughes <richard(a)hughsie.com> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Comment-Date: Fri, 09 Apr 2021 15:25:45 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit April 2021