hammer: Flash protection broken |
|||||
Issue description- hammer, at current m/master: e3336f4c8d4fb59137d35f87f4a42d22848aabcd 1. Enable WP: dut-control -p 9000 fw_wp_vref:pp3300 fw_wp_en:on fw_wp:on 2. In EC console: > flashwp true > reboot Board hangs. Doing a bisect shows that: 72afc55bd9d3f12fa62609b61dfbfe300a277dd3 is the first bad commit commit 72afc55bd9d3f12fa62609b61dfbfe300a277dd3 Author: Gwendal Grignou <gwendal@chromium.org> Date: Fri Apr 3 15:48:23 2015 -0700 stm32: cleanup flash-f by using constant from register.h Use constants from registers.h, to easily support other ECs. Fix indentation in registers.h BRANCH=none TEST=compile + following patches tested on STM32F411 BUG=None Change-Id: Iecb3ce759a5c4ff13463e7df1cb7e03fc1ce6f69 Signed-off-by: Gwendal Grignou <gwendal@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/264030 Reviewed-by: Alexandru M Stan <amstan@chromium.org>
,
Jul 6 2017
2 issues that I found and that fix the problem above on hammer (I did _not_ test other boards: we should at least test on elm and one STM32F4 user): https://chromium-review.googlesource.com/561036 chip/stm32/registers.h: Fix STM32_FLASH_OPT_LOCKED polarity https://chromium-review.googlesource.com/561037 chip/stm32/flash-f: Fix incorrect WP computation
,
Jul 6 2017
Turns out this is only half of the problem, with the 2 CL in #2, this still does not protect all flash, as it should:
> flashwp true
> reboot
> flashwp all
> flashinfo
Usable: 128 KB
Write: 2 B (ideal 2 B)
Erase: 2048 B (to 1-bits)
Protect: 4096 B
Flags: wp_gpio_asserted ro_at_boot ro_now
Protected now:
YYYYYYYY YYY..... ........ ........
(Flags should be:
Flags: wp_gpio_asserted ro_at_boot all_at_boot ro_now rw_at_boot rollback_at_boot
so that the next reboot will protect all flash
)
bisect points at that same CL again...
,
Jul 6 2017
,
Jul 6 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform/ec/+/c869b0f15c8d73f97cba4a6b2d3e0715a8a3c367 commit c869b0f15c8d73f97cba4a6b2d3e0715a8a3c367 Author: Nicolas Boichat <drinkcat@google.com> Date: Thu Jul 06 12:00:28 2017 chip/stm32/registers.h: Fix STM32_FLASH_OPT_LOCKED polarity We currently set STM32_FLASH_OPT_LOCKED to (STM32_FLASH_CR & FLASH_CR_OPTWRE), however the bit is set when option byte are _unlocked_. From STM32F0 Reference Manual: Bit 9 OPTWRE: Option byte write enable When set, the option byte can be programmed. This bit is set on writing the correct key sequence to the FLASH_OPTKEYR register. This bit can be reset by software BRANCH=none BUG= chromium:739608 TEST=Flash hammer, flashwp true; reboot; flashinfo => hammer does not hang on reboot, RO is protected Change-Id: I1b6eb5d638534ece90d6d5164586f49bdb0c151d Reviewed-on: https://chromium-review.googlesource.com/561036 Commit-Ready: Nicolas Boichat <drinkcat@chromium.org> Tested-by: Nicolas Boichat <drinkcat@chromium.org> Reviewed-by: Wei-Ning Huang <wnhuang@chromium.org> Reviewed-by: Vincent Palatin <vpalatin@chromium.org> [modify] https://crrev.com/c869b0f15c8d73f97cba4a6b2d3e0715a8a3c367/chip/stm32/registers.h
,
Jul 6 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform/ec/+/a58f5545c62283adbc7926635acbb6357f0d89b0 commit a58f5545c62283adbc7926635acbb6357f0d89b0 Author: Nicolas Boichat <drinkcat@google.com> Date: Thu Jul 06 12:00:28 2017 chip/stm32/flash-f: Fix incorrect WP computation PSTATE is already included in WP_BANK_OFFSET + WP_BANK_COUNT, so this change is not only unnecessary, but also harmful. BRANCH=none BUG= chromium:739608 TEST=Flash hammer, flashwp true; reboot; flashinfo => RO is protected Change-Id: I31048c0156eff354fbcc6ae5828a6ef313b56b97 Reviewed-on: https://chromium-review.googlesource.com/561037 Commit-Ready: Nicolas Boichat <drinkcat@chromium.org> Tested-by: Nicolas Boichat <drinkcat@chromium.org> Reviewed-by: Vincent Palatin <vpalatin@chromium.org> [modify] https://crrev.com/a58f5545c62283adbc7926635acbb6357f0d89b0/chip/stm32/flash-f.c
,
Jul 7 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform/ec/+/241e7e3a01f36b41b9a4e5ba60ad77173f96890c commit 241e7e3a01f36b41b9a4e5ba60ad77173f96890c Author: Nicolas Boichat <drinkcat@google.com> Date: Fri Jul 07 09:40:03 2017 chip/stm32/flash-f: Clear option byte write enable/erase operation when done Before 72afc55bd9d3 "stm32: cleanup flash-f by using constant from register.h" lock() function would simply do: STM32_FLASH_CR = FLASH_CR_LOCK; which would clear all other bits in STM32_FLASH_CR, including FLASH_CR_OPTER and FLASH_CR_OPTWRE. This allow preserve_optb to work, as it does: 1. erase_optb a. unlock() b. Set FLASH_CR_OPTER c. lock() (clears FLASH_CR_OPTER!) 2. write_optb a. unlock() b. Set FLASH_CR_OPTPG c. Write option byte d. Clear FLASH_CR_OPTPG e. lock() After the patch, we now have: STM32_FLASH_CR |= FLASH_CR_LOCK; which seems more correct. However, 1.c. does not clear FLASH_CR_OPTER, and 2.b. ends up with both FLASH_CR_OPTPG and FLASH_CR_OPTER set, and the programming operation does not do anything. This patches does 3 things: - Rename FLASH_CR_OPTSTRT to FLASH_CR_OPTER, as that's the correct register name for STM32F0 and STM32F3. - Fix the above by clearing FLASH_CR_OPTER in erase_optb - Also clear FLASH_CR_OPTWRE in lock(). Not strictly necessary, but this seems to be the right thing to do. BRANCH=none BUG= chromium:739608 TEST=On hammer, type flashwp true; reboot; flashwp all; reboot flashinfo => All flash is protected Change-Id: Ic276545ae3c0bdb685c7b117a7f896ec341731bb Reviewed-on: https://chromium-review.googlesource.com/562839 Commit-Ready: Nicolas Boichat <drinkcat@chromium.org> Tested-by: Nicolas Boichat <drinkcat@chromium.org> Reviewed-by: Wei-Ning Huang <wnhuang@chromium.org> Reviewed-by: Vincent Palatin <vpalatin@chromium.org> [modify] https://crrev.com/241e7e3a01f36b41b9a4e5ba60ad77173f96890c/chip/stm32/registers.h [modify] https://crrev.com/241e7e3a01f36b41b9a4e5ba60ad77173f96890c/chip/stm32/flash-f.c
,
Jul 7 2017
Presumably fixed, but, please be extra careful with flash protection in the future....
,
Sep 22 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform/ec/+/2d0012d759601c317e3a259d44651d8ad2e756d5 commit 2d0012d759601c317e3a259d44651d8ad2e756d5 Author: Nicolas Boichat <drinkcat@google.com> Date: Fri Sep 22 15:51:35 2017 chip/stm32/registers.h: Fix STM32_FLASH_OPT_LOCKED polarity We currently set STM32_FLASH_OPT_LOCKED to (STM32_FLASH_CR & FLASH_CR_OPTWRE), however the bit is set when option byte are _unlocked_. From STM32F0 Reference Manual: Bit 9 OPTWRE: Option byte write enable When set, the option byte can be programmed. This bit is set on writing the correct key sequence to the FLASH_OPTKEYR register. This bit can be reset by software BRANCH=none BUG= chromium:739608 TEST=Flash hammer, flashwp true; reboot; flashinfo => hammer does not hang on reboot, RO is protected Change-Id: I1b6eb5d638534ece90d6d5164586f49bdb0c151d Reviewed-on: https://chromium-review.googlesource.com/561036 Commit-Ready: Nicolas Boichat <drinkcat@chromium.org> Tested-by: Nicolas Boichat <drinkcat@chromium.org> Reviewed-by: Wei-Ning Huang <wnhuang@chromium.org> Reviewed-by: Vincent Palatin <vpalatin@chromium.org> (cherry picked from commit c869b0f15c8d73f97cba4a6b2d3e0715a8a3c367) Reviewed-on: https://chromium-review.googlesource.com/678678 Commit-Queue: Vincent Palatin <vpalatin@chromium.org> Tested-by: Vincent Palatin <vpalatin@chromium.org> Trybot-Ready: Vincent Palatin <vpalatin@chromium.org> [modify] https://crrev.com/2d0012d759601c317e3a259d44651d8ad2e756d5/chip/stm32/registers.h
,
Sep 22 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform/ec/+/d96dbbfc8fe95a68f1914ae1a7ad235c8c0b7653 commit d96dbbfc8fe95a68f1914ae1a7ad235c8c0b7653 Author: Nicolas Boichat <drinkcat@google.com> Date: Fri Sep 22 15:53:32 2017 chip/stm32/flash-f: Clear option byte write enable/erase operation when done Before 72afc55bd9d3 "stm32: cleanup flash-f by using constant from register.h" lock() function would simply do: STM32_FLASH_CR = FLASH_CR_LOCK; which would clear all other bits in STM32_FLASH_CR, including FLASH_CR_OPTER and FLASH_CR_OPTWRE. This allow preserve_optb to work, as it does: 1. erase_optb a. unlock() b. Set FLASH_CR_OPTER c. lock() (clears FLASH_CR_OPTER!) 2. write_optb a. unlock() b. Set FLASH_CR_OPTPG c. Write option byte d. Clear FLASH_CR_OPTPG e. lock() After the patch, we now have: STM32_FLASH_CR |= FLASH_CR_LOCK; which seems more correct. However, 1.c. does not clear FLASH_CR_OPTER, and 2.b. ends up with both FLASH_CR_OPTPG and FLASH_CR_OPTER set, and the programming operation does not do anything. This patches does 3 things: - Rename FLASH_CR_OPTSTRT to FLASH_CR_OPTER, as that's the correct register name for STM32F0 and STM32F3. - Fix the above by clearing FLASH_CR_OPTER in erase_optb - Also clear FLASH_CR_OPTWRE in lock(). Not strictly necessary, but this seems to be the right thing to do. BRANCH=none BUG= chromium:739608 TEST=On hammer, type flashwp true; reboot; flashwp all; reboot flashinfo => All flash is protected Change-Id: Ic276545ae3c0bdb685c7b117a7f896ec341731bb Reviewed-on: https://chromium-review.googlesource.com/562839 Commit-Ready: Nicolas Boichat <drinkcat@chromium.org> Tested-by: Nicolas Boichat <drinkcat@chromium.org> Reviewed-by: Wei-Ning Huang <wnhuang@chromium.org> Reviewed-by: Vincent Palatin <vpalatin@chromium.org> (cherry picked from commit 241e7e3a01f36b41b9a4e5ba60ad77173f96890c) Reviewed-on: https://chromium-review.googlesource.com/678679 Commit-Queue: Vincent Palatin <vpalatin@chromium.org> Tested-by: Vincent Palatin <vpalatin@chromium.org> Trybot-Ready: Vincent Palatin <vpalatin@chromium.org> [modify] https://crrev.com/d96dbbfc8fe95a68f1914ae1a7ad235c8c0b7653/chip/stm32/registers.h [modify] https://crrev.com/d96dbbfc8fe95a68f1914ae1a7ad235c8c0b7653/chip/stm32/flash-f.c
,
Jan 22 2018
|
|||||
►
Sign in to add a comment |
|||||
Comment 1 by drinkcat@chromium.org
, Jul 6 2017