Change in ...coreboot[master]: security/vboot: Add VBNV flags to save the Cr50 recovery switch state
Keith Short has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31800
Change subject: security/vboot: Add VBNV flags to save the Cr50 recovery switch state ......................................................................
security/vboot: Add VBNV flags to save the Cr50 recovery switch state
Add flags to save the Cr50 recovery switch state. This ensures that the Cr50 recovery switch state is only read during verstage.
BUG=b:123360379 BRANCH=none TEST=build coreboot on sarien and arcada. Test normal boot and recovery boot on arcada - confirm that that tpm transaction errors are gone.
Change-Id: Id30a7b203e5aac8631971eb102986427b8362a71 Signed-off-by: Keith Short keithshort@chromium.org --- M src/mainboard/google/sarien/chromeos.c M src/security/vboot/vbnv.c M src/security/vboot/vbnv.h M src/security/vboot/vbnv_layout.h 4 files changed, 71 insertions(+), 20 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/31800/1
diff --git a/src/mainboard/google/sarien/chromeos.c b/src/mainboard/google/sarien/chromeos.c index 1e363fd..308b682 100644 --- a/src/mainboard/google/sarien/chromeos.c +++ b/src/mainboard/google/sarien/chromeos.c @@ -20,18 +20,12 @@ #include <variant/gpio.h> #include <vendorcode/google/chromeos/chromeos.h> #include <security/tpm/tss.h> +#include <security/vboot/vbnv.h> #include <device/device.h> #include <intelblocks/pmclib.h> #include <soc/pmc.h> #include <soc/pci_devs.h>
-enum rec_mode_state { - REC_MODE_UNINITIALIZED, - REC_MODE_NOT_REQUESTED, - REC_MODE_REQUESTED, -}; -static enum rec_mode_state saved_rec_mode; - void fill_lb_gpios(struct lb_gpios *gpios) { struct lb_gpio chromeos_gpios[] = { @@ -84,30 +78,33 @@
int get_recovery_mode_switch(void) { - enum rec_mode_state state = saved_rec_mode; + int rec_switch; uint8_t recovery_button_state = 0;
- /* Check the global variable first. */ - if (state == REC_MODE_NOT_REQUESTED) - return 0; - else if (state == REC_MODE_REQUESTED) - return 1; + /* + * Only verstage performs a real check of the Cr50 recovery switch. + * The recovery switch state is cleared on the first access by the AP + * so there's no point in querying the Cr50 at later stages. All other + * stages use the state saved in VBNV. + */ + if (!ENV_VERSTAGE && + !get_recovery_switch_from_vbnv(&rec_switch)) + return rec_switch;
- state = REC_MODE_NOT_REQUESTED; + rec_switch = 0;
/* Read state from the GPIO controlled by servo. */ if (cros_get_gpio_value(CROS_GPIO_REC)) - state = REC_MODE_REQUESTED; + rec_switch = 1; /* Read one-time recovery request from cr50. */ else if (tlcl_cr50_get_recovery_button(&recovery_button_state) == TPM_SUCCESS) - state = recovery_button_state ? - REC_MODE_REQUESTED : REC_MODE_NOT_REQUESTED; + rec_switch = !!recovery_button_state;
/* Store the state in case this is called again in verstage. */ - saved_rec_mode = state; + set_recovery_switch_into_vbnv(rec_switch);
- return state == REC_MODE_REQUESTED; + return rec_switch; }
int get_lid_switch(void) diff --git a/src/security/vboot/vbnv.c b/src/security/vboot/vbnv.c index 636e5e3..8156fc5 100644 --- a/src/security/vboot/vbnv.c +++ b/src/security/vboot/vbnv.c @@ -140,6 +140,42 @@ return vbnv_data(RECOVERY_OFFSET); }
+/* Save the recovery switch state into VBNV. */ +void set_recovery_switch_into_vbnv(int recovery_switch) +{ + uint8_t vbnv_copy[VBOOT_VBNV_BLOCK_SIZE]; + + read_vbnv(vbnv_copy); + + vbnv_copy[MISC_FLAGS_OFFSET] |= MISC_FLAGS_RECOVERY_SWITCH_VALID_MASK; + if (recovery_switch) + vbnv_copy[MISC_FLAGS_OFFSET] |= + MISC_FLAGS_RECOVERY_SWITCH_STATE_MASK; + else + vbnv_copy[MISC_FLAGS_OFFSET] &= + ~MISC_FLAGS_RECOVERY_SWITCH_STATE_MASK; + + vbnv_copy[CRC_OFFSET] = crc8_vbnv(vbnv_copy, CRC_OFFSET); + + save_vbnv(vbnv_copy); +} + +/* Read the recovery switch state from VBNV. */ +int get_recovery_switch_from_vbnv(int *recovery_switch) +{ + uint8_t misc_flags; + vbnv_setup(); + misc_flags = vbnv_data(MISC_FLAGS_OFFSET); + + if (!(misc_flags & MISC_FLAGS_RECOVERY_SWITCH_VALID_MASK)) + return -1; + + *recovery_switch = + !!(misc_flags & MISC_FLAGS_RECOVERY_SWITCH_STATE_MASK); + + return 0; +} + /* Read the BOOT_OPROM_NEEDED flag from VBNV. */ int vboot_wants_oprom(void) { diff --git a/src/security/vboot/vbnv.h b/src/security/vboot/vbnv.h index c8e689f..367a376 100644 --- a/src/security/vboot/vbnv.h +++ b/src/security/vboot/vbnv.h @@ -25,6 +25,22 @@ void regen_vbnv_crc(uint8_t *vbnv_copy); int get_recovery_mode_from_vbnv(void); void set_recovery_mode_into_vbnv(int recovery_reason); + +/** + * Save the recovery switch state into VBNV + * + * @param recovery_switch Current state of the recovery switch. + */ +void set_recovery_switch_into_vbnv(int recovery_switch); +/** + * Get the recovery switch date from VBNV + * + * @param recovery_switch On success, set to the saved recovery switch state. + * + * @return 0 on success, !=0 if recovery switch state not saved. + */ +int get_recovery_switch_from_vbnv(int *recovery_switch); + int vboot_wants_oprom(void);
/* Read the USB Device Controller(UDC) enable flag from VBNV. */ diff --git a/src/security/vboot/vbnv_layout.h b/src/security/vboot/vbnv_layout.h index a9326e4..322fcf7 100644 --- a/src/security/vboot/vbnv_layout.h +++ b/src/security/vboot/vbnv_layout.h @@ -43,7 +43,9 @@ #define DEV_ENABLE_UDC 0x40
#define MISC_FLAGS_OFFSET 8 -#define MISC_FLAGS_BATTERY_CUTOFF_MASK 0x08 +#define MISC_FLAGS_BATTERY_CUTOFF_MASK 0x08 +#define MISC_FLAGS_RECOVERY_SWITCH_VALID_MASK 0x10 +#define MISC_FLAGS_RECOVERY_SWITCH_STATE_MASK 0x20
#define KERNEL_FIELD_OFFSET 11 #define CRC_OFFSET 15
Keith Short has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31800 )
Change subject: security/vboot: Add VBNV flags to save the recovery switch state ......................................................................
Patch Set 2:
This change is ready for review.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31800 )
Change subject: security/vboot: Add VBNV flags to save the recovery switch state ......................................................................
Patch Set 2:
(3 comments)
https://review.coreboot.org/#/c/31800/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31800/2//COMMIT_MSG@14 PS2, Line 14: that tpm the TPM
https://review.coreboot.org/#/c/31800/2//COMMIT_MSG@18 PS2, Line 18: Files Remove.
https://review.coreboot.org/#/c/31800/2/src/security/vboot/vbnv.c File src/security/vboot/vbnv.c:
https://review.coreboot.org/#/c/31800/2/src/security/vboot/vbnv.c@163 PS2, Line 163: /* Read the recovery switch state from VBNV. */ : int get_recovery_switch_from_vbnv(int *recovery_switch) : { : uint8_t misc_flags; : vbnv_setup(); : misc_flags = vbnv_data(MISC_FLAGS_OFFSET); : : if (!(misc_flags & MISC_FLAGS_RECOVERY_SWITCH_VALID_MASK)) : return -1; : : *recovery_switch = : !!(misc_flags & MISC_FLAGS_RECOVERY_SWITCH_STATE_MASK); : : return 0; : } Use CB_SUCCESS and CB_ERROR.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31800 )
Change subject: security/vboot: Add VBNV flags to save the recovery switch state ......................................................................
Patch Set 2:
(3 comments)
https://review.coreboot.org/#/c/31800/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31800/2//COMMIT_MSG@9 PS2, Line 9: Add flags to save the recovery switch state. You should provide more context and reasoning in the commit description.
https://review.coreboot.org/#/c/31800/2/src/security/vboot/vbnv.h File src/security/vboot/vbnv.h:
https://review.coreboot.org/#/c/31800/2/src/security/vboot/vbnv.h@36 PS2, Line 36: date data?
https://review.coreboot.org/#/c/31800/2/src/security/vboot/vbnv.c File src/security/vboot/vbnv.c:
https://review.coreboot.org/#/c/31800/2/src/security/vboot/vbnv.c@163 PS2, Line 163: /* Read the recovery switch state from VBNV. */ : int get_recovery_switch_from_vbnv(int *recovery_switch) : { : uint8_t misc_flags; : vbnv_setup(); : misc_flags = vbnv_data(MISC_FLAGS_OFFSET); : : if (!(misc_flags & MISC_FLAGS_RECOVERY_SWITCH_VALID_MASK)) : return -1; : : *recovery_switch = : !!(misc_flags & MISC_FLAGS_RECOVERY_SWITCH_STATE_MASK); : : return 0; : }
Use CB_SUCCESS and CB_ERROR.
This entire file doesn't use those return values, though it is mainly dealing with returning values.
Keith Short has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/31800 )
Change subject: security/vboot: Add VBNV flags to save the recovery switch state ......................................................................
Abandoned
Replaced by change https://review.coreboot.org/c/coreboot/+/32030
-
Aaron Durbin (Code Review) -
Keith Short (Code Review)
-
Paul Menzel (Code Review)