September 2022 - flashrom-gerrit

[S] Change in flashrom[master]: cli_classic.c: Early init of layout obscures invalid memory access
by Edward O'Callaghan (Code Review) 20 Sep '22

20 Sep '22

Attention is currently required from: Thomas Heijligen, Angel Pons, Nikolai Artemiev. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/67723 ) Change subject: cli_classic.c: Early init of layout obscures invalid memory access ...................................................................... Patch Set 2: (3 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/67723/comment/fb30da1f_15337832 PS2, Line 7: cli_classic.c: Early init of layout obscures invalid memory access > Can you give the codepath for the invalid memory access and why the patch helps to fix this? […] I agree with the second part of what you said. What happened was that `flashrom_layout_get_region_range()` was called on `layout := NULL` which resulted in a seg fault. This occurred when trying to use `--wp-region=` without a specified `--image=` from the cli. The previous patch in the chain addresses the actual crash and this patch is addressing the issue of how the compiler didn't warn to indicate the underlying issue. Nik, can you follow up here for any further questions and take this over please. Also it would be good to add a unit-test to go with the previous patch in the chain to cover this precise case. File cli_classic.c: https://review.coreboot.org/c/flashrom/+/67723/comment/e37a4ba5_516ca56c PS2, Line 541: struct flashrom_layout *layout = NULL; > This means "no layout", which is true: initially, the flashrom classic CLI doesn't have any layout t […] The issue is not that `NULL` means 'no layout' but rather it is the attempted use of a layout before perhaps there is one available. By early initialisation one obscures this compiler warning that a branch didn't explicitly handle the case 'no layout' before usage. Squelching a compiler warning that can indicate possibly issue with procedural control flow isn't a good idea which made the bug the previous commit in the chain go unnoticed. https://review.coreboot.org/c/flashrom/+/67723/comment/9a8c90aa_6d9cc494 PS2, Line 1110: msg_gdbg("Valid layout could not be found without image.\n"); > What does this message even mean? Nik, can you come up with a better string here for your bug. These patches were made to get you started, please follow up with Angel here. -- To view, visit https://review.coreboot.org/c/flashrom/+/67723 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I3df61b873da27f6945d60916a1c3713dedfc3924 Gerrit-Change-Number: 67723 Gerrit-PatchSet: 2 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Tue, 20 Sep 2022 23:21:24 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Thomas Heijligen <src(a)posteo.de> Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: layout.c: Validate _layout_entry_by_name() arguments before use
by Edward O'Callaghan (Code Review) 20 Sep '22

20 Sep '22

Attention is currently required from: Angel Pons, Nikolai Artemiev. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/67722 ) Change subject: layout.c: Validate _layout_entry_by_name() arguments before use ...................................................................... Patch Set 3: (2 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/67722/comment/509c95f8_e3864e49 PS2, Line 9: It maybe > It may be Done https://review.coreboot.org/c/flashrom/+/67722/comment/172219eb_c7dba7e6 PS2, Line 10: feed with a NULL pointer. > being fed a NULL pointer. Done -- To view, visit https://review.coreboot.org/c/flashrom/+/67722 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I2a19c0e586f8575b8b3c2c02b5afad312efacfc9 Gerrit-Change-Number: 67722 Gerrit-PatchSet: 3 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Tue, 20 Sep 2022 23:10:02 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: layout.c: Validate _layout_entry_by_name() arguments before use
by Edward O'Callaghan (Code Review) 20 Sep '22

20 Sep '22

Attention is currently required from: Edward O'Callaghan, Nikolai Artemiev. Hello build bot (Jenkins), Angel Pons, Nikolai Artemiev, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/67722 to look at the new patch set (#3). Change subject: layout.c: Validate _layout_entry_by_name() arguments before use ...................................................................... layout.c: Validate _layout_entry_by_name() arguments before use It may be the case that a layout could not be derived which would result in layout logic being fed a NULL pointer. Validate this case and be defensive to validate the name argument as well. BUG=b:247055486 TEST=builds Change-Id: I2a19c0e586f8575b8b3c2c02b5afad312efacfc9 Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M layout.c 1 file changed, 19 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/22/67722/3 -- To view, visit https://review.coreboot.org/c/flashrom/+/67722 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I2a19c0e586f8575b8b3c2c02b5afad312efacfc9 Gerrit-Change-Number: 67722 Gerrit-PatchSet: 3 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-MessageType: newpatchset

1 0

[M] Change in flashrom[master]: writeprotect_ranges.c: add more range functions
by Anastasia Klimchuk (Code Review) 20 Sep '22

20 Sep '22

Attention is currently required from: Nico Huber, Angel Pons, Nikolai Artemiev, Sergii Dmytruk. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/66212 ) Change subject: writeprotect_ranges.c: add more range functions ...................................................................... Patch Set 6: (1 comment) Patchset: PS3: > Testing is in progress. Sergii, I realised your chain is waiting for attention for a while. I think we all just read your message "testing is in progress" and decided to wait when you tell the results of testing :) Are there any? The chain has had good rounds of review from several people, so would be great to move it forward. -- To view, visit https://review.coreboot.org/c/flashrom/+/66212 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ied7b27be2ee2426af8f473432e2b01a290de2365 Gerrit-Change-Number: 66212 Gerrit-PatchSet: 6 Gerrit-Owner: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Comment-Date: Tue, 20 Sep 2022 22:45:17 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: drivers: Move (un)map_flash_region from programmer to par_master
by Jonathon Hall (Code Review) 20 Sep '22

20 Sep '22

Attention is currently required from: Thomas Heijligen, Edward O'Callaghan, Nikolai Artemiev, Peter Marheine. Jonathon Hall has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/67695 ) Change subject: drivers: Move (un)map_flash_region from programmer to par_master ...................................................................... Patch Set 5: (1 comment) Patchset: PS3: > maybe just rebase this on my patches, so this one just removes the fallback_map but don't change any […] Per discussion, rebased on 67655, we'll leave 67656 for later since it requires hardware to test. -- To view, visit https://review.coreboot.org/c/flashrom/+/67695 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I9c3df6ae260bcdb246dfb0cd8e043919609b014b Gerrit-Change-Number: 67695 Gerrit-PatchSet: 5 Gerrit-Owner: Jonathon Hall <jonathon.hall(a)puri.sm> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Comment-Date: Tue, 20 Sep 2022 19:47:40 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Thomas Heijligen <src(a)posteo.de> Comment-In-Reply-To: Jonathon Hall <jonathon.hall(a)puri.sm> Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: drivers: Move (un)map_flash_region from programmer to par_master
by Jonathon Hall (Code Review) 20 Sep '22

20 Sep '22

Attention is currently required from: Edward O'Callaghan, Jonathon Hall, Nikolai Artemiev, Peter Marheine. Hello build bot (Jenkins), Edward O'Callaghan, Angel Pons, Nikolai Artemiev, Peter Marheine, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/67695 to look at the new patch set (#5). Change subject: drivers: Move (un)map_flash_region from programmer to par_master ...................................................................... drivers: Move (un)map_flash_region from programmer to par_master Move (un)map_flash_region function pointers from programmer_entry to par_master. Only parallel masters require specialized flash mapping, and that mapping is closely tied to the way the memory is accessed using the other functions in par_master. ICH SPI internal programming now works with flash >16 MB. Previously it would fail due to attempting to mmap 32 MB or more, which is not possible as only the top 16 MB of flash is mapped into the address space. Validate that FWH/LPC programmers provide specialized mapping in register_par_master(); this is needed for chips with FEATURE_REGISTERMAP, which only exist on FWH or LPC buses. dummyflasher.c: Use dummy mapper for parallel buses only. internal.c: Use physmap for parallel buses only. serprog.c: Replace mapper hack for 16 MB limit with max_rom_decode. programmer.c: Update comment in fallback_map(), NULL return is the desired behavior. Change-Id: I9c3df6ae260bcdb246dfb0cd8e043919609b014b Signed-off-by: Jonathon Hall <jonathon.hall(a)puri.sm> --- M atahpt.c M atapromise.c M atavia.c M buspirate_spi.c M ch341a_spi.c M dediprog.c M developerbox_spi.c M digilent_spi.c M drkaiser.c M dummyflasher.c M flashrom.c M ft2232_spi.c M gfxnvidia.c M hwaccess_physmap.c M include/flash.h M include/hwaccess_physmap.h M include/programmer.h M internal.c M it8212.c M jlink_spi.c M linux_mtd.c M linux_spi.c M mediatek_i2c_spi.c M mstarddc_spi.c M ni845x_spi.c M nic3com.c M nicintel.c M nicintel_eeprom.c M nicintel_spi.c M nicnatsemi.c M nicrealtek.c M ogp_spi.c M parade_lspcon.c M parallel.c M pickit2_spi.c M pony_spi.c M programmer.c M raiden_debug_spi.c M rayer_spi.c M realtek_mst_i2c_spi.c M satamv.c M satasii.c M serprog.c M stlinkv3_spi.c M usbblaster_spi.c 45 files changed, 96 insertions(+), 116 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/95/67695/5 -- To view, visit https://review.coreboot.org/c/flashrom/+/67695 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I9c3df6ae260bcdb246dfb0cd8e043919609b014b Gerrit-Change-Number: 67695 Gerrit-PatchSet: 5 Gerrit-Owner: Jonathon Hall <jonathon.hall(a)puri.sm> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Jonathon Hall <jonathon.hall(a)puri.sm> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

[S] Change in flashrom[master]: atapromise.c: Use fallback_map instead of own identical implementation
by Jonathon Hall (Code Review) 20 Sep '22

20 Sep '22

Attention is currently required from: Thomas Heijligen. Jonathon Hall has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/67655 ) Change subject: atapromise.c: Use fallback_map instead of own identical implementation ...................................................................... Patch Set 1: Code-Review+1 -- To view, visit https://review.coreboot.org/c/flashrom/+/67655 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Iea16d42015bdbe838364cc65cff895d9edaf03a7 Gerrit-Change-Number: 67655 Gerrit-PatchSet: 1 Gerrit-Owner: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: Jonathon Hall <jonathon.hall(a)puri.sm> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Comment-Date: Tue, 20 Sep 2022 19:42:35 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: atapromise.c: Use fallback_map instead of own identical implementation
by Thomas Heijligen (Code Review) 20 Sep '22

20 Sep '22

Thomas Heijligen has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/67655 ) Change subject: atapromise.c: Use fallback_map instead of own identical implementation ...................................................................... Set Ready For Review -- To view, visit https://review.coreboot.org/c/flashrom/+/67655 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Iea16d42015bdbe838364cc65cff895d9edaf03a7 Gerrit-Change-Number: 67655 Gerrit-PatchSet: 1 Gerrit-Owner: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Tue, 20 Sep 2022 19:41:47 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: drivers: Move (un)map_flash_region from programmer to par_master
by Thomas Heijligen (Code Review) 20 Sep '22

20 Sep '22

Attention is currently required from: Edward O'Callaghan, Jonathon Hall, Nikolai Artemiev, Peter Marheine. Thomas Heijligen has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/67695 ) Change subject: drivers: Move (un)map_flash_region from programmer to par_master ...................................................................... Patch Set 4: (1 comment) Patchset: PS3: > This patch will supersede CB:67655, but I'll leave CB:67656 separate since it isn't so trivial (unle […] maybe just rebase this on my patches, so this one just removes the fallback_map but don't change any behavior. -- To view, visit https://review.coreboot.org/c/flashrom/+/67695 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I9c3df6ae260bcdb246dfb0cd8e043919609b014b Gerrit-Change-Number: 67695 Gerrit-PatchSet: 4 Gerrit-Owner: Jonathon Hall <jonathon.hall(a)puri.sm> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Jonathon Hall <jonathon.hall(a)puri.sm> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Comment-Date: Tue, 20 Sep 2022 19:06:09 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Thomas Heijligen <src(a)posteo.de> Comment-In-Reply-To: Jonathon Hall <jonathon.hall(a)puri.sm> Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: drivers: Move (un)map_flash_region from programmer to par_master
by Jonathon Hall (Code Review) 20 Sep '22

20 Sep '22

Attention is currently required from: Thomas Heijligen, Edward O'Callaghan, Nikolai Artemiev, Peter Marheine. Jonathon Hall has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/67695 ) Change subject: drivers: Move (un)map_flash_region from programmer to par_master ...................................................................... Patch Set 4: (2 comments) Patchset: PS3: > I've some WIP patches for atavia and atapromise. […] This patch will supersede CB:67655, but I'll leave CB:67656 separate since it isn't so trivial (unless you would rather I merge it into this patch). File serprog.c: https://review.coreboot.org/c/flashrom/+/67695/comment/823dfc3b_9fb5d96a PS2, Line 582: .map_flash_region = serprog_map, > This is a hack to make sure the chips have a max size of 16MB. […] Ah I see it now, I replaced the mapper with max_rom_decode for all bus types. -- To view, visit https://review.coreboot.org/c/flashrom/+/67695 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I9c3df6ae260bcdb246dfb0cd8e043919609b014b Gerrit-Change-Number: 67695 Gerrit-PatchSet: 4 Gerrit-Owner: Jonathon Hall <jonathon.hall(a)puri.sm> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Comment-Date: Tue, 20 Sep 2022 18:19:43 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Thomas Heijligen <src(a)posteo.de> Comment-In-Reply-To: Jonathon Hall <jonathon.hall(a)puri.sm> Gerrit-MessageType: comment

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit September 2022