From 2a092bbef7ecc3d4ce83b01923940d755b07419b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Iwanicki?= Date: Wed, 17 Sep 2025 14:14:42 +0200 Subject: [PATCH] Fail immediately when trying to write/erase wp regions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This patch introduces chipset-level protections and adds checks that abort writing to flash if any of the requested regions are write-protected by chip, dynamically by a chipset, or are defined as read-only. This change is done so it's harder for user to brick his own platform. Information about read-only regions can easily be missed as flashrom can output a lot of information on screen. Even if you notice you might not know if one of the regions you requested falls inside read-only range, especially if using different names for those regions. If you are flashing multiple regions or ones that partially overlap with read-only parts then that could result in flashrom failing in the middle leaving you in unknown state. This patch was tested with multiple combinations of unprotected/protected regions: - dummy programmer ```sh flashrom -p dummy:hwwp=yes,emulate=S25FL128L --wp-enable \ --wp-range 0x00040000,0x00fc0000 \ -l <(echo '00000000:0004ffff part1') -i part1 -E ``` - internal programmer on Protectli VP6670 with Dasharo UEFI firmware with locked BIOS boot medium (PR0, part of bios region) ```sh flashrom -p internal --ifd -i me -i bios -w test.rom ``` Normal reads and flashing non-protected regions was also tested. Change-Id: Ia0dd847923e20ff0081ceae68984369e98952c2f Signed-off-by: Michał Iwanicki Reviewed-on: https://review.coreboot.org/c/flashrom/+/89222 Reviewed-by: Sergii Dmytruk Tested-by: build bot (Jenkins) Reviewed-by: Anastasia Klimchuk --- doc/release_notes/devel.rst | 15 ++++++++ flashrom.c | 75 +++++++++++++++++++++++++++++++++++++ ichspi.c | 59 +++++++++++++++++++++++------ include/layout.h | 11 ++++++ include/programmer.h | 6 +++ 5 files changed, 154 insertions(+), 12 deletions(-) diff --git a/doc/release_notes/devel.rst b/doc/release_notes/devel.rst index 1e84aa871..3bd25de7a 100644 --- a/doc/release_notes/devel.rst +++ b/doc/release_notes/devel.rst @@ -23,3 +23,18 @@ Added support * Intel Wildcat Lake chipset * Eon EN25QX128A * PUYA P25D80H + +New features +============ + +Fail immediately when trying to write/erase wp regions +------------------------------------------------------ + +This change is done so it's harder for user to brick his own platform. +Information about read-only regions can easily be missed as flashrom +can output a lot of information on screen. Even if you notice you might +not know if one of the regions you requested falls inside read-only +range, especially if using different names for those regions. +If you are flashing multiple regions or ones that partially overlap with +read-only parts then that could result in flashrom failing in the +middle, leaving you in unknown state. diff --git a/flashrom.c b/flashrom.c index b64a2a057..ce844e12c 100644 --- a/flashrom.c +++ b/flashrom.c @@ -384,6 +384,22 @@ void get_flash_region(const struct flashctx *flash, int addr, struct flash_regio } } +void get_protected_ranges(const struct flashctx *flash, struct protected_ranges *ranges) { + if ((flash->mst->buses_supported & BUS_PROG) && flash->mst->opaque.get_protected_ranges) { + flash->mst->opaque.get_protected_ranges(ranges); + } else { + *ranges = (const struct protected_ranges){ 0 }; + } +} + +void release_protected_ranges(const struct flashctx *flash, struct protected_ranges *ranges) { + for (int i = 0; i < ranges->count; ++i) { + free(ranges->ranges[i].name); + } + free(ranges->ranges); + *ranges = (const struct protected_ranges){ 0 }; +} + int check_for_unwritable_regions(const struct flashctx *flash, unsigned int start, unsigned int len) { struct flash_region region; @@ -1817,6 +1833,58 @@ warn_out: return ret; } +static bool can_change_target_regions(struct flashctx *flash) { + bool check_wp = false; + size_t wp_start, wp_len; + enum flashrom_wp_mode mode; + struct flashrom_wp_cfg *cfg = NULL; + const struct romentry *entry = NULL; + const struct flashrom_layout *const layout = get_layout(flash); + struct protected_ranges ranges; + get_protected_ranges(flash, &ranges); + + if (flashrom_wp_cfg_new(&cfg) == FLASHROM_WP_OK && + flashrom_wp_read_cfg(cfg, flash) == FLASHROM_WP_OK) + { + flashrom_wp_get_range(&wp_start, &wp_len, cfg); + mode = flashrom_wp_get_mode(cfg); + if (mode != FLASHROM_WP_MODE_DISABLED && wp_len != 0) + check_wp = true; + } + flashrom_wp_cfg_release(cfg); + + while ((entry = layout_next_included(layout, entry))) { + if (!flash->flags.skip_unwritable_regions && + check_for_unwritable_regions(flash, entry->region.start, + entry->region.end - entry->region.start + 1)) + { + release_protected_ranges(flash, &ranges); + return false; + } + if (check_wp && entry->region.start < wp_start + wp_len && wp_start <= entry->region.end) { + msg_gerr("%s: cannot fully update %s region (%#08"PRIx32"..%#08"PRIx32")" + " due to chip's write-protection.\n", __func__, + entry->region.name, entry->region.start, entry->region.end); + release_protected_ranges(flash, &ranges); + return false; + } + for (int i = 0; i < ranges.count; ++i) { + struct flash_region prot = ranges.ranges[i]; + if (prot.write_prot && prot.start <= entry->region.end && + prot.end >= entry->region.start) { + msg_gerr("%s: cannot fully update %s region (%#08"PRIx32"..%#08"PRIx32")" + " due to chipset write-protection.\n", __func__, + entry->region.name, entry->region.start, entry->region.end); + release_protected_ranges(flash, &ranges); + return false; + } + } + } + + release_protected_ranges(flash, &ranges); + return true; +} + int prepare_flash_access(struct flashctx *const flash, const bool read_it, const bool write_it, const bool erase_it, const bool verify_it) @@ -1831,6 +1899,13 @@ int prepare_flash_access(struct flashctx *const flash, return 1; } + if ((write_it || erase_it) && !flash->flags.force) { + if (!can_change_target_regions(flash)) { + msg_cerr("At least one target region is not fully writable. Aborting.\n"); + return 1; + } + } + if (map_flash(flash) != 0) return 1; diff --git a/ichspi.c b/ichspi.c index a4d783c19..749fab9d7 100644 --- a/ichspi.c +++ b/ichspi.c @@ -1287,6 +1287,13 @@ struct hwseq_data { struct fd_region fd_regions[MAX_FD_REGIONS]; }; +#define MAX_PR_REGISTERS 6 +struct flash_region ranges_data[MAX_PR_REGISTERS]; +struct protected_ranges ranges = { + .count = 0, + .ranges = ranges_data, +}; + static struct hwseq_data *get_hwseq_data_from_context(const struct flashctx *flash) { return flash->mst->opaque.data; @@ -1469,6 +1476,21 @@ static void ich_get_region(const struct flashctx *flash, unsigned int addr, stru region->name = strdup(name); } +static void ich_get_protected_ranges(struct protected_ranges *protected_ranges) { + protected_ranges->count = ranges.count; + protected_ranges->ranges = malloc(sizeof(*ranges.ranges) * ranges.count); + for (int i = 0; i < ranges.count; ++i) { + if (ranges.ranges[i].name == NULL) + protected_ranges->ranges[i].name = strdup(""); + else + protected_ranges->ranges[i].name = strdup(ranges.ranges[i].name); + protected_ranges->ranges[i].start = ranges.ranges[i].start; + protected_ranges->ranges[i].end = ranges.ranges[i].end; + protected_ranges->ranges[i].read_prot = ranges.ranges[i].read_prot; + protected_ranges->ranges[i].write_prot = ranges.ranges[i].write_prot; + } +} + /* Given RDID info, return pointer to entry in flashchips[] */ static const struct flashchip *flash_id_to_entry(uint32_t mfg_id, uint32_t model_id) { @@ -1952,13 +1974,19 @@ static enum ich_access_protection ich9_handle_region_access(struct fd_region *fd #define ICH_PR_PERMS(pr) (((~((pr) >> PR_RP_OFF) & 1) << 0) | \ ((~((pr) >> PR_WP_OFF) & 1) << 1)) -static enum ich_access_protection ich9_handle_pr(const size_t reg_pr0, unsigned int i) +static enum ich_access_protection ich9_handle_pr(const size_t reg_pr0, unsigned int i, struct flash_region *prot) { uint8_t off = reg_pr0 + (i * 4); uint32_t pr = mmio_readl(ich_spibar + off); unsigned int rwperms_idx = ICH_PR_PERMS(pr); enum ich_access_protection rwperms = access_perms_to_protection[rwperms_idx]; + prot->name = NULL; + prot->start = ICH_FREG_BASE(pr); + prot->end = ICH_FREG_LIMIT(pr); + prot->write_prot = (rwperms == WRITE_PROT) || (rwperms == LOCKED); + prot->read_prot = (rwperms == READ_PROT) || (rwperms == LOCKED); + /* From 5 on we have GPR registers and start from 0 again. */ const char *const prefix = i >= 5 ? "G" : ""; if (i >= 5) @@ -2012,16 +2040,17 @@ static const struct spi_master spi_master_ich = { }; static const struct opaque_master opaque_master_ich_hwseq = { - .max_data_read = 64, - .max_data_write = 64, - .probe = ich_hwseq_probe, - .read = ich_hwseq_read, - .write = ich_hwseq_write, - .erase = ich_hwseq_block_erase, - .read_register = ich_hwseq_read_status, - .write_register = ich_hwseq_write_status, - .get_region = ich_get_region, - .shutdown = ich_hwseq_shutdown, + .max_data_read = 64, + .max_data_write = 64, + .probe = ich_hwseq_probe, + .read = ich_hwseq_read, + .write = ich_hwseq_write, + .erase = ich_hwseq_block_erase, + .read_register = ich_hwseq_read_status, + .write_register = ich_hwseq_write_status, + .get_region = ich_get_region, + .get_protected_ranges = ich_get_protected_ranges, + .shutdown = ich_hwseq_shutdown, }; static int init_ich7_spi(void *spibar, enum ich_chipset ich_gen) @@ -2248,7 +2277,13 @@ static int init_ich_default(const struct programmer_cfg *cfg, void *spibar, enum /* if not locked down try to disable PR locks first */ if (!ichspi_lock) ich9_set_pr(reg_pr0, i, 0, 0); - ich_spi_rw_restricted |= ich9_handle_pr(reg_pr0, i); + struct flash_region pr_region = { 0 }; + enum ich_access_protection rwperms = ich9_handle_pr(reg_pr0, i, &pr_region); + ich_spi_rw_restricted |= rwperms; + if (rwperms != NO_PROT) { + ranges_data[ranges.count] = pr_region; + ranges.count += 1; + } } switch (ich_spi_rw_restricted) { diff --git a/include/layout.h b/include/layout.h index ce3dd4ba2..862f749c9 100644 --- a/include/layout.h +++ b/include/layout.h @@ -57,6 +57,11 @@ struct romentry { struct flash_region region; }; +struct protected_ranges { + int count; + struct flash_region *ranges; +}; + struct flashrom_layout; struct layout_include_args; @@ -80,5 +85,11 @@ void prepare_layout_for_extraction(struct flashrom_flashctx *); int layout_sanity_checks(const struct flashrom_flashctx *); int check_for_unwritable_regions(const struct flashrom_flashctx *flash, unsigned int start, unsigned int len); void get_flash_region(const struct flashrom_flashctx *flash, int addr, struct flash_region *region); +/* + * Return chipset-level protections. + * ranges parameter has to be freed by the caller with release_protected_ranges + */ +void get_protected_ranges(const struct flashrom_flashctx *flash, struct protected_ranges *ranges); +void release_protected_ranges(const struct flashrom_flashctx *flash, struct protected_ranges *ranges); #endif /* !__LAYOUT_H__ */ diff --git a/include/programmer.h b/include/programmer.h index 5951f6dce..e169d1563 100644 --- a/include/programmer.h +++ b/include/programmer.h @@ -397,6 +397,12 @@ struct opaque_master { enum flashrom_wp_result (*wp_read_cfg)(struct flashrom_wp_cfg *, struct flashctx *); enum flashrom_wp_result (*wp_get_ranges)(struct flashrom_wp_ranges **, struct flashctx *); void (*get_region)(const struct flashctx *flash, unsigned int addr, struct flash_region *region); + /* + * Returns chipset-level protections (e.g., when SPI controller refuses + * to pass read/write commands to the flash chip based on chipset's + * configuration) + */ + void (*get_protected_ranges)(struct protected_ranges *); int (*shutdown)(void *data); void (*delay) (const struct flashctx *flash, unsigned int usecs); void *data;