Furquan Shaikh has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/40683 )
Change subject: soc/amd/picasso: Disable secure debug unlock ......................................................................
soc/amd/picasso: Disable secure debug unlock
This change updates PSP_SOFTFUSE to disable secure debug unlock by default and also removes the addition of debug key and debug file to PSP directory. This is not something that we would want to be enabled on shipping devices. If debug capability is required, there should be a separate Kconfig to enable it.
BUG=b:154880818 TEST=Verified that trembyle boots to OS.
Signed-off-by: Furquan Shaikh furquan@google.com Change-Id: I47d8af67989b06242d662c77b7d9db97f624edd5 --- M src/soc/amd/picasso/Makefile.inc 1 file changed, 2 insertions(+), 15 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/83/40683/1
diff --git a/src/soc/amd/picasso/Makefile.inc b/src/soc/amd/picasso/Makefile.inc index e686ab8..577db50 100644 --- a/src/soc/amd/picasso/Makefile.inc +++ b/src/soc/amd/picasso/Makefile.inc @@ -106,11 +106,8 @@ PSP_SMUFW2_SUB1_FILE=$(top)/$(FIRMWARE_LOCATE)/SmuFirmware2RV2.csbin PSP_SMUFW2_SUB2_FILE=$(top)/$(FIRMWARE_LOCATE)/SmuFirmware2PCO.csbin
-# type = 0x9 -PSP_SEC_DBG_KEY_FILE=$(top)/$(FIRMWARE_LOCATE)/RavenSecureDebug_PublicKey.bin - # type = 0xb - See #55758 (NDA) for bit definitions. -PSP_SOFTFUSE="0x0000000010000001" +PSP_SOFTFUSE="0x0000000010000000"
ifeq ($(CONFIG_USE_PSPSCUREOS),y) # types = 0x2, 0xc @@ -118,9 +115,6 @@ PSPTRUSTLETS_FILE=$(top)/$(FIRMWARE_LOCATE)/dr_ftpm_prod_RV.csbin endif
-# type = 0x13 -PSP_SEC_DEBUG_FILE=$(top)/$(FIRMWARE_LOCATE)/secure_unlock_prod_RV.sbin - # type = 0x21 PSP_IKEK_FILE=$(top)/$(FIRMWARE_LOCATE)/PspIkekRV.bin
@@ -135,7 +129,7 @@ # BIOS type = 0x6a PSP_MP2CFG_FILE=$(top)/$(FIRMWARE_LOCATE)/MP2FWConfig.sbin else -PSP_SOFTFUSE="0x0000000030000001" +PSP_SOFTFUSE="0x0000000030000000" endif
# type = 0x28 @@ -214,11 +208,9 @@ OPT_SMUFW1_SUB2_FILE=$(call add_opt_prefix, $(PSP_SMUFW1_SUB2_FILE), --subprogram 2 --smufirmware) OPT_SMUFW2_SUB1_FILE=$(call add_opt_prefix, $(PSP_SMUFW2_SUB1_FILE), --subprogram 1 --smufirmware2) OPT_SMUFW2_SUB2_FILE=$(call add_opt_prefix, $(PSP_SMUFW2_SUB2_FILE), --subprogram 2 --smufirmware2) -OPT_PSP_SEC_DBG_KEY_FILE=$(call add_opt_prefix, $(PSP_SEC_DBG_KEY_FILE), --securedebug) OPT_PSP_SOFTFUSE=$(call add_opt_prefix, $(PSP_SOFTFUSE), --soft-fuse) OPT_PSPSCUREOS_FILE=$(call add_opt_prefix, $(PSPSCUREOS_FILE), --secureos) OPT_PSPTRUSTLETS_FILE=$(call add_opt_prefix, $(PSPTRUSTLETS_FILE), --trustlets) -OPT_SEC_DEBUG_FILE=$(call add_opt_prefix, $(PSP_SEC_DEBUG_FILE), --secdebug) OPT_IKEK_FILE=$(call add_opt_prefix, $(PSP_IKEK_FILE), --ikek) OPT_SECG1_FILE=$(call add_opt_prefix, $(PSP_SECG1_FILE), --subprog 1 --sec-gasket) OPT_SECG2_FILE=$(call add_opt_prefix, $(PSP_SECG2_FILE), --subprog 2 --sec-gasket) @@ -263,7 +255,6 @@ $(obj)/amdfw.rom: $(call strip_quotes, $(CONFIG_AMD_PUBKEY_FILE)) \ $(call strip_quotes, $(PSPBTLDR_FILE)) \ $(call strip_quotes, $(PSPSCUREOS_FILE)) \ - $(call strip_quotes, $(PSP_SEC_DBG_KEY_FILE)) \ $(call strip_quotes, $(PSPTRUSTLETS_FILE)) \ $(call strip_quotes, $(PSP_APCB0_FILE)) \ $(call strip_quotes, $(PSP_APCB1_FILE)) \ @@ -301,7 +292,6 @@ $(call_strip_quotes, $(PSP_DRIVERS_FILE)) \ $(call_strip_quotes, $(PSP_S0I3_FILE)) \ $(call_strip_quotes, $(PSP_IKEK_FILE)) \ - $(call_strip_quotes, $(PSP_SEC_DEBUG_FILE)) \ $(AMDFWTOOL) rm -f $@ @printf " AMDFWTOOL $(subst $(obj)/,,$(@))\n" @@ -309,7 +299,6 @@ $(OPT_AMD_PUBKEY_FILE) \ $(OPT_PSPBTLDR_FILE) \ $(OPT_PSPSCUREOS_FILE) \ - $(OPT_PSP_SEC_DBG_KEY_FILE) \ $(OPT_PSPTRUSTLETS_FILE) \ $(OPT_SMUFW1_SUB2_FILE) \ $(OPT_SMUFW2_SUB2_FILE) \ @@ -355,9 +344,7 @@ $(OPT_DRIVERS_FILE) \ $(OPT_PSP_S0I3_FILE) \ $(OPT_IKEK_FILE) \ - $(OPT_SEC_DEBUG_FILE) \ --combo-capable \ - --token-unlock \ --flashsize $(CONFIG_ROM_SIZE) \ --location $(shell printf "0x%x" $(PICASSO_FWM_POSITION)) \ --output $@
Hello Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40683
to look at the new patch set (#2).
Change subject: soc/amd/picasso: Disable secure debug unlock ......................................................................
soc/amd/picasso: Disable secure debug unlock
This change updates PSP_SOFTFUSE to disable secure debug unlock by default and also removes the addition of debug key and debug file to PSP directory. This is not something that we would want to be enabled on shipping devices. If debug capability is required, there should be a separate Kconfig to enable it.
BUG=b:154880818 TEST=Verified that trembyle boots to OS.
Signed-off-by: Furquan Shaikh furquan@google.com Change-Id: I47d8af67989b06242d662c77b7d9db97f624edd5 --- M src/soc/amd/picasso/Makefile.inc 1 file changed, 2 insertions(+), 15 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/83/40683/2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40683 )
Change subject: soc/amd/picasso: Disable secure debug unlock ......................................................................
Patch Set 2: Code-Review+1
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40683 )
Change subject: soc/amd/picasso: Disable secure debug unlock ......................................................................
Patch Set 2: Code-Review+2
Hello build bot (Jenkins), Raul Rangel, Patrick Georgi, Martin Roth, Marshall Dawson, Angel Pons, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40683
to look at the new patch set (#3).
Change subject: soc/amd/picasso: Enable secure debug unlock conditionally ......................................................................
soc/amd/picasso: Enable secure debug unlock conditionally
This change adds a Kconfig option PSP_UNLOCK_SECURE_DEBUG which when enabled includes secure debug unlock blobs and sets the required softfuses and options for amdfwtool. By default this is set to 'N'.
BUG=b:154880818
Signed-off-by: Furquan Shaikh furquan@google.com Change-Id: I47d8af67989b06242d662c77b7d9db97f624edd5 --- M src/soc/amd/picasso/Kconfig M src/soc/amd/picasso/Makefile.inc 2 files changed, 13 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/83/40683/3
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40683 )
Change subject: soc/amd/picasso: Enable secure debug unlock conditionally ......................................................................
Patch Set 3: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/40683/3/src/soc/amd/picasso/Kconfig File src/soc/amd/picasso/Kconfig:
https://review.coreboot.org/c/coreboot/+/40683/3/src/soc/amd/picasso/Kconfig... PS3, Line 355: bool Is it supposed to lack a prompt?
Hello build bot (Jenkins), Raul Rangel, Patrick Georgi, Martin Roth, Marshall Dawson, Angel Pons, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40683
to look at the new patch set (#4).
Change subject: soc/amd/picasso: Enable secure debug unlock conditionally ......................................................................
soc/amd/picasso: Enable secure debug unlock conditionally
This change adds a Kconfig option PSP_UNLOCK_SECURE_DEBUG which when enabled includes secure debug unlock blobs and sets the required softfuses and options for amdfwtool. By default this is set to 'N'.
BUG=b:154880818
Signed-off-by: Furquan Shaikh furquan@google.com Change-Id: I47d8af67989b06242d662c77b7d9db97f624edd5 --- M src/soc/amd/picasso/Kconfig M src/soc/amd/picasso/Makefile.inc 2 files changed, 13 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/83/40683/4
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40683 )
Change subject: soc/amd/picasso: Enable secure debug unlock conditionally ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40683/3/src/soc/amd/picasso/Kconfig File src/soc/amd/picasso/Kconfig:
https://review.coreboot.org/c/coreboot/+/40683/3/src/soc/amd/picasso/Kconfig... PS3, Line 355: bool
Is it supposed to lack a prompt?
Done
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40683 )
Change subject: soc/amd/picasso: Enable secure debug unlock conditionally ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40683/4/src/soc/amd/picasso/Kconfig File src/soc/amd/picasso/Kconfig:
https://review.coreboot.org/c/coreboot/+/40683/4/src/soc/amd/picasso/Kconfig... PS4, Line 358: Select this item to enable secure debug options in PSP. Can you please elaborate, or add a reference to more information?
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40683 )
Change subject: soc/amd/picasso: Enable secure debug unlock conditionally ......................................................................
Patch Set 4: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/40683/4/src/soc/amd/picasso/Kconfig File src/soc/amd/picasso/Kconfig:
https://review.coreboot.org/c/coreboot/+/40683/4/src/soc/amd/picasso/Kconfig... PS4, Line 358: Select this item to enable secure debug options in PSP.
Can you please elaborate, or add a reference to more information?
I prefer not, as it's NDA information. (There should be one or more references in the source indicating the doc number.)
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40683 )
Change subject: soc/amd/picasso: Enable secure debug unlock conditionally ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40683/4/src/soc/amd/picasso/Kconfig File src/soc/amd/picasso/Kconfig:
https://review.coreboot.org/c/coreboot/+/40683/4/src/soc/amd/picasso/Kconfig... PS4, Line 358: Select this item to enable secure debug options in PSP.
I prefer not, as it's NDA information. […]
Keeping it as is based on Marshall's comment.
Furquan Shaikh has submitted this change. ( https://review.coreboot.org/c/coreboot/+/40683 )
Change subject: soc/amd/picasso: Enable secure debug unlock conditionally ......................................................................
soc/amd/picasso: Enable secure debug unlock conditionally
This change adds a Kconfig option PSP_UNLOCK_SECURE_DEBUG which when enabled includes secure debug unlock blobs and sets the required softfuses and options for amdfwtool. By default this is set to 'N'.
BUG=b:154880818
Signed-off-by: Furquan Shaikh furquan@google.com Change-Id: I47d8af67989b06242d662c77b7d9db97f624edd5 Reviewed-on: https://review.coreboot.org/c/coreboot/+/40683 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Marshall Dawson marshalldawson3rd@gmail.com --- M src/soc/amd/picasso/Kconfig M src/soc/amd/picasso/Makefile.inc 2 files changed, 13 insertions(+), 4 deletions(-)
Approvals: build bot (Jenkins): Verified Marshall Dawson: Looks good to me, approved
diff --git a/src/soc/amd/picasso/Kconfig b/src/soc/amd/picasso/Kconfig index a37f543..fa053f9 100644 --- a/src/soc/amd/picasso/Kconfig +++ b/src/soc/amd/picasso/Kconfig @@ -357,6 +357,12 @@ depends on HAVE_PSP_WHITELIST_FILE default "3rdparty/blobs/soc/amd/picasso/PSP/wtl-rvn.sbin"
+config PSP_UNLOCK_SECURE_DEBUG + bool "Unlock secure debug" + default n + help + Select this item to enable secure debug options in PSP. + endmenu
endif # SOC_AMD_PICASSO diff --git a/src/soc/amd/picasso/Makefile.inc b/src/soc/amd/picasso/Makefile.inc index d7cf9c0..4790ecb 100644 --- a/src/soc/amd/picasso/Makefile.inc +++ b/src/soc/amd/picasso/Makefile.inc @@ -109,19 +109,21 @@ PSP_SMUFW2_SUB1_FILE=$(top)/$(FIRMWARE_LOCATE)/SmuFirmware2RV2.csbin PSP_SMUFW2_SUB2_FILE=$(top)/$(FIRMWARE_LOCATE)/SmuFirmware2PCO.csbin
+ifeq ($(CONFIG_PSP_UNLOCK_SECURE_DEBUG),y) # type = 0x9 PSP_SEC_DBG_KEY_FILE=$(top)/$(FIRMWARE_LOCATE)/RavenSecureDebug_PublicKey.bin +# type = 0x13 +PSP_SEC_DEBUG_FILE=$(top)/$(FIRMWARE_LOCATE)/secure_unlock_prod_RV.sbin # Enable secure debug unlock PSP_SOFTFUSE_BITS += 0 +PSP_TOKEN_UNLOCK="--token-unlock" +endif
ifeq ($(CONFIG_USE_PSPSCUREOS),y) # types = 0x2 PSPSCUREOS_FILE=$(top)/$(FIRMWARE_LOCATE)/psp_os_combined_prod_RV.sbin endif
-# type = 0x13 -PSP_SEC_DEBUG_FILE=$(top)/$(FIRMWARE_LOCATE)/secure_unlock_prod_RV.sbin - # type = 0x21 PSP_IKEK_FILE=$(top)/$(FIRMWARE_LOCATE)/PspIkekRV.bin
@@ -228,6 +230,7 @@ OPT_SMUFW2_SUB1_FILE=$(call add_opt_prefix, $(PSP_SMUFW2_SUB1_FILE), --subprogram 1 --smufirmware2) OPT_SMUFW2_SUB2_FILE=$(call add_opt_prefix, $(PSP_SMUFW2_SUB2_FILE), --subprogram 2 --smufirmware2) OPT_PSP_SEC_DBG_KEY_FILE=$(call add_opt_prefix, $(PSP_SEC_DBG_KEY_FILE), --securedebug) +OPT_TOKEN_UNLOCK=$(call add_opt_prefix, $(PSP_TOKEN_UNLOCK), "") OPT_PSP_SOFTFUSE=$(call add_opt_prefix, $(PSP_SOFTFUSE), --soft-fuse) OPT_PSPSCUREOS_FILE=$(call add_opt_prefix, $(PSPSCUREOS_FILE), --secureos) OPT_SEC_DEBUG_FILE=$(call add_opt_prefix, $(PSP_SEC_DEBUG_FILE), --secdebug) @@ -367,7 +370,7 @@ $(OPT_IKEK_FILE) \ $(OPT_SEC_DEBUG_FILE) \ --combo-capable \ - --token-unlock \ + $(OPT_TOKEN_UNLOCK) \ --flashsize $(CONFIG_ROM_SIZE) \ --location $(shell printf "0x%x" $(PICASSO_FWM_POSITION)) \ --output $@
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40683 )
Change subject: soc/amd/picasso: Enable secure debug unlock conditionally ......................................................................
Patch Set 6:
Automatic boot test returned (PASS/FAIL/TOTAL): 4/0/4 Emulation targets: "QEMU x86 q35/ich9" using payload TianoCore : SUCCESS : https://lava.9esec.io/r/2890 "QEMU x86 q35/ich9" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/2889 "QEMU x86 i440fx/piix4" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/2888 "QEMU AArch64" using payload LinuxBoot_u-root_kexec : SUCCESS : https://lava.9esec.io/r/2887
Please note: This test is under development and might not be accurate at all!
-
9elements QA (Code Review) -
Aaron Durbin (Code Review)
-
Angel Pons (Code Review)
-
Furquan Shaikh (Code Review)
-
Marshall Dawson (Code Review)
-
Paul Menzel (Code Review)