flashrom.c: Fail immediately when trying to write/erase wp regions #17
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
m-iwanicki
force-pushed
the
refuse-flashing-if-protected
branch
2 times, most recently
from
6892f59 to
4dc3fa9
Compare
Can you change this so that only attempting to flash the actually protected ranges will throw an error? For example if someone has WP_RO protected but only wants to flash RW_SECTION_* |
m-iwanicki
force-pushed
the
refuse-flashing-if-protected
branch
from
337da02 to
e7b4efa
Compare
Tests@mkopec
|
m-iwanicki
force-pushed
the
refuse-flashing-if-protected
branch
from
e7b4efa to
ababbe1
Compare
|
Seems to work as described 👍 tested on NV40MB (Tiger Lake) |
mkopec
requested changes
Dec 13, 2024
| @@ -20,3 +20,6 @@ | |||
| /util/ich_descriptors_tool/.obj | |||
|
|
|||
| target/ | |||
|
|
|||
| subprojects/cmocka-*/ | |||
| subprojects/packagecache/ | |||
There was a problem hiding this comment.
Either remove or split into a separate commit, please
There was a problem hiding this comment.
11e7895: those are folders generated by meson test.
flashrom.c
Outdated
| @@ -1752,6 +1796,13 @@ int prepare_flash_access(struct flashctx *const flash, | |||
| return 1; | |||
| } | |||
|
|
|||
| if ((write_it || erase_it) && !flash->flags.force) { | |||
| if(write_protect_check(flash)) { | |||
There was a problem hiding this comment.
Suggested change
| if(write_protect_check(flash)) { | |
| if (write_protect_check(flash)) { |
Please make code style consistent
| @@ -1289,6 +1289,13 @@ struct hwseq_data { | |||
| struct fd_region fd_regions[MAX_FD_REGIONS]; | |||
| }; | |||
|
|
|||
| #define MAX_PR_REGISTERS 6 | |||
There was a problem hiding this comment.
does this come from a specification?
There was a problem hiding this comment.
No, I used largest value that could be returned in
Line 2108 in ababbe1
| { | ||
| 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->base = ICH_FREG_BASE(pr); | ||
| prot->limit = ICH_FREG_LIMIT(pr); | ||
| prot->write_prot = rwperms == WRITE_PROT; |
There was a problem hiding this comment.
Suggested change
| prot->write_prot = rwperms == WRITE_PROT; | |
| prot->write_prot = (rwperms == WRITE_PROT) || (rwperms == LOCKED); |
I think, at least
There was a problem hiding this comment.
I assumed based on
Line 2267 in ababbe1
If I'm reading this correctly in case
ich_spi_rw_restricted == LOCKED then warning will only mention that's it's read-protected
ichspi.c
Outdated
| prot->base = ICH_FREG_BASE(pr); | ||
| prot->limit = ICH_FREG_LIMIT(pr); | ||
| prot->write_prot = rwperms == WRITE_PROT; | ||
| prot->read_prot = rwperms == READ_PROT || rwperms == LOCKED; |
There was a problem hiding this comment.
Suggested change
| prot->read_prot = rwperms == READ_PROT || rwperms == LOCKED; | |
| prot->read_prot = (rwperms ==READ_PROT) || (rwperms == LOCKED); |
Change-Id: Ic39b4acf1da73d093ce3a7df71c1c549d92240d5 Signed-off-by: Michał Iwanicki <[email protected]>
Change-Id: I989006eae97f5e015df20026b6f5361ad41223c1 Signed-off-by: Michał Iwanicki <[email protected]>
Change-Id: Iee172044959df8efd7148adb3a918a7dad963f6f Signed-off-by: Michał Iwanicki <[email protected]>
Change-Id: I1cdcfacaf61285bf4d7789610461a3f02ea4c64b Signed-off-by: Michał Iwanicki <[email protected]>
m-iwanicki
force-pushed
the
refuse-flashing-if-protected
branch
from
ababbe1 to
c80f032
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
write_protect_checkcan be deleted and PR0 check will still happen but only one region at a time, so if we write/erase multiple regions (e.g.-i fd -i bios) then first region might get written if it is read-write.In case of e.g.
FREG1: BIOS region (0x00580000-0x00ffffff) is read-write.&PR0: Warning: 0x00b00000-0x00ffffff is read-only.write/erase would still fail even if we tried to flash only unprotected parts of region e.g.0x00580000-0x00590000To force write/erase add
--forceargument.meson test:
Tested on Protectli VP6670:
Try to flash whole chip:
Try to flash protected part:
Try to flash unprotected part:
WP protection checked with dummy programmer:
unprotected part:
Protected part: