- flashrom-gerrit - mail.coreboot.org

Change in flashrom[master]: fmap.c: Avoid undefined behaviour with fmap_lsearch([len:=0])
by Nico Huber (Code Review) 03 Feb '22

03 Feb '22

Nico Huber has submitted this change. ( https://review.coreboot.org/c/flashrom/+/61545 ) Change subject: fmap.c: Avoid undefined behaviour with fmap_lsearch([len:=0]) ...................................................................... fmap.c: Avoid undefined behaviour with fmap_lsearch([len:=0]) Calling libflashrom entry-points that internally dispatch to fmap_lsearch() can result in a integer overflow. Therefore validate the length paramter before attempting to use it. BUG=none TEST=`make` Change-Id: Ifb408c55c3b69ddff453dcc704b7389298050473 Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> Spotted-by: Julius Werner <jwerner(a)chromium.org> Reviewed-on: https://review.coreboot.org/c/flashrom/+/61545 Reviewed-by: Julius Werner <jwerner(a)chromium.org> Reviewed-by: Nico Huber <nico.h(a)gmx.de> Reviewed-by: Angel Pons <th3fanbus(a)gmail.com> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> --- M fmap.c 1 file changed, 3 insertions(+), 0 deletions(-) Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved Julius Werner: Looks good to me, approved Angel Pons: Looks good to me, approved diff --git a/fmap.c b/fmap.c index b18cbf7..0236b62 100644 --- a/fmap.c +++ b/fmap.c @@ -96,6 +96,9 @@ off_t offset; bool fmap_found = 0; + if (len < sizeof(struct fmap)) + return -1; + for (offset = 0; offset <= (off_t)(len - sizeof(struct fmap)); offset++) { if (is_valid_fmap((struct fmap *)&buf[offset])) { fmap_found = 1; -- To view, visit https://review.coreboot.org/c/flashrom/+/61545 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ifb408c55c3b69ddff453dcc704b7389298050473 Gerrit-Change-Number: 61545 Gerrit-PatchSet: 3 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

Change in flashrom[master]: fmap.c: Avoid undefined behaviour with fmap_lsearch([len:=0])
by Angel Pons (Code Review) 03 Feb '22

03 Feb '22

Attention is currently required from: Edward O'Callaghan. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/61545 ) Change subject: fmap.c: Avoid undefined behaviour with fmap_lsearch([len:=0]) ...................................................................... Patch Set 2: Code-Review+2 (1 comment) File fmap.c: https://review.coreboot.org/c/flashrom/+/61545/comment/5a79a047_afff5e47 PS1, Line 99: if (len == 0) > Setting comment as resolved. […] If Nico is OK with it, I am as well. -- To view, visit https://review.coreboot.org/c/flashrom/+/61545 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ifb408c55c3b69ddff453dcc704b7389298050473 Gerrit-Change-Number: 61545 Gerrit-PatchSet: 2 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Comment-Date: Thu, 03 Feb 2022 16:55:57 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: fmap.c: Avoid undefined behaviour with fmap_lsearch([len:=0])
by Edward O'Callaghan (Code Review) 03 Feb '22

03 Feb '22

Attention is currently required from: Angel Pons. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/61545 ) Change subject: fmap.c: Avoid undefined behaviour with fmap_lsearch([len:=0]) ...................................................................... Patch Set 2: (1 comment) File fmap.c: https://review.coreboot.org/c/flashrom/+/61545/comment/7de3fada_8a89f646 PS1, Line 99: if (len == 0) > sure, the predicate is more precisely: […] Setting comment as resolved. Obviously reopen if there is further concerns Angel, thanks for the review! -- To view, visit https://review.coreboot.org/c/flashrom/+/61545 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ifb408c55c3b69ddff453dcc704b7389298050473 Gerrit-Change-Number: 61545 Gerrit-PatchSet: 2 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Thu, 03 Feb 2022 08:51:21 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: cli_classic: Replace programmer_shutdown() with libflashrom call
by Edward O'Callaghan (Code Review) 03 Feb '22

03 Feb '22

Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/61584 ) Change subject: cli_classic: Replace programmer_shutdown() with libflashrom call ...................................................................... cli_classic: Replace programmer_shutdown() with libflashrom call flashrom_programmer_shutdown(NULL) is an equiv call to programmer_shutdown() however this further decouples cli from flashrom core logic at link-time, prefering to instead enter via libflashrom instead. BUG=none TEST=`make`. Change-Id: Ie194fa2e891797a29d05d7e9d0c7226fd62c0679 Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M cli_classic.c 1 file changed, 1 insertion(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/84/61584/1 diff --git a/cli_classic.c b/cli_classic.c index 7a89b6e..317172c 100644 --- a/cli_classic.c +++ b/cli_classic.c @@ -955,7 +955,7 @@ out_release: flashrom_layout_release(layout); out_shutdown: - programmer_shutdown(); + flashrom_programmer_shutdown(NULL); out: for (i = 0; i < chipcount; i++) { flashrom_layout_release(flashes[i].default_layout); -- To view, visit https://review.coreboot.org/c/flashrom/+/61584 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ie194fa2e891797a29d05d7e9d0c7226fd62c0679 Gerrit-Change-Number: 61584 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: newchange

1 0

Change in flashrom[master]: cli_classic: Use flashrom_flash_getsize() over raw access
by Edward O'Callaghan (Code Review) 03 Feb '22

03 Feb '22

Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/61583 ) Change subject: cli_classic: Use flashrom_flash_getsize() over raw access ...................................................................... cli_classic: Use flashrom_flash_getsize() over raw access Use native libflashrom entry-points for getting flash size over accessing internal struct members within cli code. BUG=none TEST=`sudo ./flashrom -p internal --get-size`. Change-Id: Iff70a0de1dee517c4e38c45b1540c42326a13340 Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M cli_classic.c 1 file changed, 5 insertions(+), 5 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/83/61583/1 diff --git a/cli_classic.c b/cli_classic.c index fe1eeea..7a89b6e 100644 --- a/cli_classic.c +++ b/cli_classic.c @@ -158,7 +158,7 @@ { int ret; - unsigned long size = flash->chip->total_size * 1024; + unsigned long size = flashrom_flash_getsize(flash); unsigned char *buf = calloc(size, sizeof(unsigned char)); if (!buf) { msg_gerr("Memory allocation failed!\n"); @@ -189,7 +189,7 @@ static int do_write(struct flashctx *const flash, const char *const filename, const char *const referencefile) { - const size_t flash_size = flash->chip->total_size * 1024; + const size_t flash_size = flashrom_flash_getsize(flash); int ret = 1; uint8_t *const newcontents = malloc(flash_size); @@ -225,7 +225,7 @@ static int do_verify(struct flashctx *const flash, const char *const filename) { - const size_t flash_size = flash->chip->total_size * 1024; + const size_t flash_size = flashrom_flash_getsize(flash); int ret = 1; uint8_t *const newcontents = malloc(flash_size); @@ -807,7 +807,7 @@ } if (flash_size) { - printf("%d\n", fill_flash->chip->total_size * 1024); + printf("%ld\n", flashrom_flash_getsize(fill_flash)); goto out_shutdown; } @@ -844,7 +844,7 @@ } free(fmapfile_buffer); } else if (fmap && (flashrom_layout_read_fmap_from_rom(&layout, fill_flash, 0, - fill_flash->chip->total_size * 1024) || process_include_args(layout, include_args))) { + flashrom_flash_getsize(fill_flash)) || process_include_args(layout, include_args))) { ret = 1; goto out_shutdown; } -- To view, visit https://review.coreboot.org/c/flashrom/+/61583 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Iff70a0de1dee517c4e38c45b1540c42326a13340 Gerrit-Change-Number: 61583 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: newchange

1 0

Change in flashrom[master]: cli_classic.: Use flashrom_init() from API
by Edward O'Callaghan (Code Review) 03 Feb '22

03 Feb '22

Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/61582 ) Change subject: cli_classic.: Use flashrom_init() from API ...................................................................... cli_classic.: Use flashrom_init() from API flashrom_init() already does delay calibration and self-checking via the canonical libflashrom API. Port the cli implementation to go via the libflashrom API entry-point natively. BUG=none TEST=`make` Change-Id: I07faeed876f678c35355621a080c7852eed16824 Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M cli_classic.c 1 file changed, 2 insertions(+), 4 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/82/61582/1 diff --git a/cli_classic.c b/cli_classic.c index 8c337eb..fe1eeea 100644 --- a/cli_classic.c +++ b/cli_classic.c @@ -350,7 +350,8 @@ print_version(); print_banner(); - if (selfcheck()) + /* FIXME: Delay calibration should happen in programmer code. */ + if (flashrom_init(1)) exit(1); setbuf(stdout, NULL); @@ -661,9 +662,6 @@ } } - /* FIXME: Delay calibration should happen in programmer code. */ - myusec_calibrate_delay(); - if (programmer_init(prog, pparam)) { msg_perr("Error: Programmer initialization failed.\n"); ret = 1; -- To view, visit https://review.coreboot.org/c/flashrom/+/61582 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I07faeed876f678c35355621a080c7852eed16824 Gerrit-Change-Number: 61582 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: newchange

1 0

Change in flashrom[master]: fmap.c: Avoid undefined behaviour with fmap_lsearch([len:=0])
by Nico Huber (Code Review) 02 Feb '22

02 Feb '22

Attention is currently required from: Edward O'Callaghan, Angel Pons. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/61545 ) Change subject: fmap.c: Avoid undefined behaviour with fmap_lsearch([len:=0]) ...................................................................... Patch Set 2: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/61545 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ifb408c55c3b69ddff453dcc704b7389298050473 Gerrit-Change-Number: 61545 Gerrit-PatchSet: 2 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Thu, 03 Feb 2022 01:09:52 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: internal.c: Seperate out get_params() from internal_init()
by Edward O'Callaghan (Code Review) 02 Feb '22

02 Feb '22

Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/61578 ) Change subject: internal.c: Seperate out get_params() from internal_init() ...................................................................... internal.c: Seperate out get_params() from internal_init() Move all programmer parameter wrangling into its own function. BUG=none TEST=`make` Change-Id: I66bb370eb0466c5c838621762a6ba825c44567d4 Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M internal.c 1 file changed, 29 insertions(+), 15 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/78/61578/1 diff --git a/internal.c b/internal.c index b8e0564..1640781 100644 --- a/internal.c +++ b/internal.c @@ -174,22 +174,15 @@ .chip_writen = fallback_chip_writen, }; -static int internal_init(void) +static int get_params(int *boardenable, int *boardmismatch, + int *force_laptop, int *not_a_laptop, + char **board_vendor, char **board_model) { - int ret = 0; - int force_laptop = 0; - int not_a_laptop = 0; - char *board_vendor = NULL; - char *board_model = NULL; -#if defined(__i386__) || defined(__x86_64__) - const char *cb_vendor = NULL; - const char *cb_model = NULL; -#endif char *arg; arg = extract_programmer_param("boardenable"); if (arg && !strcmp(arg,"force")) { - force_boardenable = 1; + *boardenable = 1; } else if (arg && !strlen(arg)) { msg_perr("Missing argument for boardenable.\n"); free(arg); @@ -203,7 +196,7 @@ arg = extract_programmer_param("boardmismatch"); if (arg && !strcmp(arg,"force")) { - force_boardmismatch = 1; + *boardmismatch = 1; } else if (arg && !strlen(arg)) { msg_perr("Missing argument for boardmismatch.\n"); free(arg); @@ -217,9 +210,9 @@ arg = extract_programmer_param("laptop"); if (arg && !strcmp(arg, "force_I_want_a_brick")) - force_laptop = 1; + *force_laptop = 1; else if (arg && !strcmp(arg, "this_is_not_a_laptop")) - not_a_laptop = 1; + *not_a_laptop = 1; else if (arg && !strlen(arg)) { msg_perr("Missing argument for laptop.\n"); free(arg); @@ -233,7 +226,7 @@ arg = extract_programmer_param("mainboard"); if (arg && strlen(arg)) { - if (board_parse_parameter(arg, &board_vendor, &board_model)) { + if (board_parse_parameter(arg, board_vendor, board_model)) { free(arg); return 1; } @@ -244,6 +237,27 @@ } free(arg); + return 0; +} + +static int internal_init(void) +{ + int ret = 0; + int force_laptop = 0; + int not_a_laptop = 0; + char *board_vendor = NULL; + char *board_model = NULL; +#if defined(__i386__) || defined(__x86_64__) + const char *cb_vendor = NULL; + const char *cb_model = NULL; +#endif + + ret = get_params(&force_boardenable, &force_boardmismatch, + &force_laptop, &not_a_laptop, + &board_vendor, &board_model); + if (ret) + return ret; + /* Default to Parallel/LPC/FWH flash devices. If a known host controller * is found, the host controller init routine sets the * internal_buses_supported bitfield. -- To view, visit https://review.coreboot.org/c/flashrom/+/61578 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I66bb370eb0466c5c838621762a6ba825c44567d4 Gerrit-Change-Number: 61578 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: newchange

1 0

Change in flashrom[master]: fmap.c: Avoid undefined behaviour with fmap_lsearch([len:=0])
by Julius Werner (Code Review) 02 Feb '22

02 Feb '22

Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/61545 ) Change subject: fmap.c: Avoid undefined behaviour with fmap_lsearch([len:=0]) ...................................................................... Patch Set 2: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/61545 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ifb408c55c3b69ddff453dcc704b7389298050473 Gerrit-Change-Number: 61545 Gerrit-PatchSet: 2 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> 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: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Wed, 02 Feb 2022 23:48:53 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: fmap.c: Avoid undefined behaviour with fmap_lsearch([len:=0])
by Edward O'Callaghan (Code Review) 02 Feb '22

02 Feb '22

Attention is currently required from: Nico Huber, Angel Pons, Julius Werner. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/61545 ) Change subject: fmap.c: Avoid undefined behaviour with fmap_lsearch([len:=0]) ...................................................................... Patch Set 1: (2 comments) File fmap.c: https://review.coreboot.org/c/flashrom/+/61545/comment/cb96c3e4_1f726393 PS1, Line 99: if (len == 0) > +1 we should simply check that the calculation below doesn't overflow. sure, the predicate is more precisely: `if ((len < sizeof(struct fmap)) return -1;` https://review.coreboot.org/c/flashrom/+/61545/comment/53e5ad73_ba69f67e PS1, Line 102: (off_t)(len - sizeof(struct fmap) > Hrmm, I added this cast at some point, but it doesn't seem to fix the […] That is right. Rather than fixing via type coercion's, a more explicit function parameter validation in the preamble seems sensible. It's not like C's type-system is so rich to allow for bounding the functions domain rigorously imho. -- To view, visit https://review.coreboot.org/c/flashrom/+/61545 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ifb408c55c3b69ddff453dcc704b7389298050473 Gerrit-Change-Number: 61545 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> 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: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Julius Werner <jwerner(a)chromium.org> Gerrit-Comment-Date: Wed, 02 Feb 2022 23:45:16 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit