coreboot-gerrit December 2023

coreboot-gerrit@coreboot.org

1 participants
2354 discussions

[M] Change in coreboot[main]: security/tpm: Retrieve factory configuration for device w/ Google TPM
by Subrata Banik (Code Review) 27 Dec '23

27 Dec '23

Attention is currently required from: Christian Walter, Tim Van Patten. Hello Christian Walter, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/79736?usp=email to look at the new patch set (#3). The following approvals got outdated and were removed: Verified+1 by build bot (Jenkins) Change subject: security/tpm: Retrieve factory configuration for device w/ Google TPM ...................................................................... security/tpm: Retrieve factory configuration for device w/ Google TPM This patch enables retrieval of factory configuration data from Google TPM devices (both Cr50 and Ti50). This patch utilizes vendor-specific command TPM2_CR50_SUB_CMD_GET_FACTORY_CONFIG (68). The factory config space is a 64-bit, one-time programmable. For the unprovisioned one, the read will be 0x0. BUG=b:317880956 TEST=Able to retrieve the factory config from google/screebo. Change-Id: Ifd0e850770152a03aa46d7f8bbb76f7520a59081 Signed-off-by: Subrata Banik <subratabanik(a)google.com> --- M src/security/tpm/tss/tcg-2.0/tss_marshaling.c M src/security/tpm/tss/tcg-2.0/tss_structures.h M src/security/tpm/tss/vendor/cr50/cr50.c M src/security/tpm/tss/vendor/cr50/cr50.h 4 files changed, 57 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/36/79736/3 -- To view, visit https://review.coreboot.org/c/coreboot/+/79736?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: Ifd0e850770152a03aa46d7f8bbb76f7520a59081 Gerrit-Change-Number: 79736 Gerrit-PatchSet: 3 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Tim Van Patten <timvp(a)google.com> Gerrit-Attention: Christian Walter <christian.walter(a)9elements.com> Gerrit-Attention: Tim Van Patten <timvp(a)google.com> Gerrit-MessageType: newpatchset

1 0

[M] Change in coreboot[main]: security/tpm: Retrieve factory configuration for TI50 devices
by Subrata Banik (Code Review) 27 Dec '23

27 Dec '23

Attention is currently required from: Christian Walter, Tim Van Patten. Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/79736?usp=email ) Change subject: security/tpm: Retrieve factory configuration for TI50 devices ...................................................................... Patch Set 2: (3 comments) Commit Message: https://review.coreboot.org/c/coreboot/+/79736/comment/fc799ce3_3bdfcb83 : PS1, Line 9: This patch enables retrieval of factory configuration data from : TI50 TPM devices. > I think it's worth checking with the GSC team to verify the validity of that comment. > > I trust go/cr50-board-id-in-factory more though, since it's explicit about version numbers: > > > To get/set the factory config, it requires Cr50 firmware version >= 0.{5,6}.170, or Ti50 firmware version >= 0.{23,24}.30. > > For reference, Monkey Island is running CR50 `0.5.201`, which is new enough to support the command as well (based on the values listed). > > Further evidence it's supported by the CR50 is the CL that added those values: > > https://crrev.com/c/4421422 > > > cr50: Add get/set factory config command codes. > > The bug that added them (b/275356839) also mentions both the CR50 and TI50. i have executed this cmd on cr50 and ti50 devices, no error return. assuming things are support on both security chip. File src/security/tpm/tss/tcg-2.0/tss_structures.h: https://review.coreboot.org/c/coreboot/+/79736/comment/7ad99147_d324e13f : PS1, Line 359: uint8_t factory_config; > Can we make this a `uint64_t` now, so it matches what the GSC returns and we can avoid growing it in […] Acknowledged File src/security/tpm/tss/vendor/cr50/cr50.c: https://review.coreboot.org/c/coreboot/+/79736/comment/56c23174_c6dfc070 : PS1, Line 213: *factory_config = response->vcr.factory_config; > This looks odd. […] Acknowledged -- To view, visit https://review.coreboot.org/c/coreboot/+/79736?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: Ifd0e850770152a03aa46d7f8bbb76f7520a59081 Gerrit-Change-Number: 79736 Gerrit-PatchSet: 2 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Tim Van Patten <timvp(a)google.com> Gerrit-Attention: Christian Walter <christian.walter(a)9elements.com> Gerrit-Attention: Tim Van Patten <timvp(a)google.com> Gerrit-Comment-Date: Wed, 27 Dec 2023 18:22:02 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com> Comment-In-Reply-To: Tim Van Patten <timvp(a)google.com> Gerrit-MessageType: comment

1 0

[M] Change in coreboot[main]: arch/arm64: Support calling a trusted monitor
by Arthur Heymans (Code Review) 27 Dec '23

27 Dec '23

Attention is currently required from: Benjamin Doron, David Milosevic, Julius Werner. Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/78284?usp=email ) Change subject: arch/arm64: Support calling a trusted monitor ...................................................................... Patch Set 13: (1 comment) File src/arch/arm64/smc.c: https://review.coreboot.org/c/coreboot/+/78284/comment/fcb85c89_3908cbcb : PS11, Line 10: inline uint64_t smc_call(uint32_t function_id, uint64_t arg1, uint64_t arg2, uint64_t arg3, > I actually found that other boards broke if this file was compiled unconditionally. Possibly because CONFIG_ARM64_CURRENT_EL has no value? We can probably drop the assert if the Makefile guarantees it anyways. The coreboot implementation of assert checks if the argument is a buildtime constant and if the argument is false that would trigger failed builds. -- To view, visit https://review.coreboot.org/c/coreboot/+/78284?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I158db0b971aba722b3995d52162146aa406d1644 Gerrit-Change-Number: 78284 Gerrit-PatchSet: 13 Gerrit-Owner: Benjamin Doron <benjamin.doron00(a)gmail.com> Gerrit-Reviewer: Benjamin Doron <benjamin.doron00(a)gmail.com> Gerrit-Reviewer: David Milosevic <David.Milosevic(a)9elements.com> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Lean Sheng Tan <sheng.tan(a)9elements.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Benjamin Doron <benjamin.doron00(a)gmail.com> Gerrit-Attention: Julius Werner <jwerner(a)chromium.org> Gerrit-Attention: David Milosevic <David.Milosevic(a)9elements.com> Gerrit-Comment-Date: Wed, 27 Dec 2023 17:22:40 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Benjamin Doron <benjamin.doron00(a)gmail.com> Comment-In-Reply-To: Julius Werner <jwerner(a)chromium.org> Comment-In-Reply-To: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-MessageType: comment

1 0

[M] Change in coreboot[main]: security/tpm: Retrieve factory configuration for TI50 devices
by Tim Van Patten (Code Review) 27 Dec '23

27 Dec '23

Attention is currently required from: Christian Walter, Subrata Banik. Tim Van Patten has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/79736?usp=email ) Change subject: security/tpm: Retrieve factory configuration for TI50 devices ...................................................................... Patch Set 2: (1 comment) Commit Message: https://review.coreboot.org/c/coreboot/+/79736/comment/0c40b208_d4acc5aa : PS1, Line 9: This patch enables retrieval of factory configuration data from : TI50 TPM devices. I think it's worth checking with the GSC team to verify the validity of that comment. I trust go/cr50-board-id-in-factory more though, since it's explicit about version numbers: > To get/set the factory config, it requires Cr50 firmware version >= 0.{5,6}.170, or Ti50 firmware version >= 0.{23,24}.30. For reference, Monkey Island is running CR50 `0.5.201`, which is new enough to support the command as well (based on the values listed). Further evidence it's supported by the CR50 is the CL that added those values: https://crrev.com/c/4421422 > cr50: Add get/set factory config command codes. The bug that added them (b/275356839) also mentions both the CR50 and TI50. -- To view, visit https://review.coreboot.org/c/coreboot/+/79736?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: Ifd0e850770152a03aa46d7f8bbb76f7520a59081 Gerrit-Change-Number: 79736 Gerrit-PatchSet: 2 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Tim Van Patten <timvp(a)google.com> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Christian Walter <christian.walter(a)9elements.com> Gerrit-Comment-Date: Wed, 27 Dec 2023 16:48:57 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com> Comment-In-Reply-To: Tim Van Patten <timvp(a)google.com> Gerrit-MessageType: comment

1 0

[M] Change in coreboot[main]: security/tpm: Retrieve factory configuration for TI50 devices
by Subrata Banik (Code Review) 27 Dec '23

27 Dec '23

1 0

[S] Change in coreboot[main]: vendorcode/google/chromeos: API to read factory config
by Tim Van Patten (Code Review) 27 Dec '23

27 Dec '23

Attention is currently required from: Subrata Banik. Tim Van Patten has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/79737?usp=email ) Change subject: vendorcode/google/chromeos: API to read factory config ...................................................................... Patch Set 1: (2 comments) File src/vendorcode/google/chromeos/ti50_misc_cmd.c: https://review.coreboot.org/c/coreboot/+/79737/comment/a18917f9_70c44702 : PS1, Line 20: if (rc == TPM_CB_NO_SUCH_COMMAND) { `TPM_CB_NO_SUCH_COMMAND` is not the only value `tlcl_ti50_get_factory_config()` can return, so the other errors are being ignored and treated as "good" (`TPM_CB_MUST_REBOOT`, `TPM_IOERROR`). This should instead be `if (rc != TPM_SUCCESS) {`, with a more generic log message that outputs the value of `rc`. https://review.coreboot.org/c/coreboot/+/79737/comment/6de25076_58fd9e4f : PS1, Line 26: return factory_config; `factory_config` is a `uint64_t`, while this function returns a `uint8_t`. These types need to match. The function should be updated to return `uint64_t`, so it matches what the GSC returns. -- To view, visit https://review.coreboot.org/c/coreboot/+/79737?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I34f47c9a94972534cda656ef624ef12ed5ddeb06 Gerrit-Change-Number: 79737 Gerrit-PatchSet: 1 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-CC: Tim Van Patten <timvp(a)google.com> Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Comment-Date: Wed, 27 Dec 2023 16:25:18 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[M] Change in coreboot[main]: security/tpm: Retrieve factory configuration for TI50 devices
by Tim Van Patten (Code Review) 27 Dec '23

27 Dec '23

Attention is currently required from: Christian Walter, Subrata Banik. Tim Van Patten has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/79736?usp=email ) Change subject: security/tpm: Retrieve factory configuration for TI50 devices ...................................................................... Patch Set 1: (3 comments) Commit Message: https://review.coreboot.org/c/coreboot/+/79736/comment/e42c97a3_a18c773b : PS1, Line 9: This patch enables retrieval of factory configuration data from : TI50 TPM devices. Why is this specific to the TI50? go/cr50-board-id-in-factory states the command is supported by both CR50 and TI50. File src/security/tpm/tss/tcg-2.0/tss_structures.h: https://review.coreboot.org/c/coreboot/+/79736/comment/3803bc9b_9ac8e568 : PS1, Line 359: uint8_t factory_config; Can we make this a `uint64_t` now, so it matches what the GSC returns and we can avoid growing it in the future? File src/security/tpm/tss/vendor/cr50/cr50.c: https://review.coreboot.org/c/coreboot/+/79736/comment/62c73d17_1ca82944 : PS1, Line 213: *factory_config = response->vcr.factory_config; This looks odd. We're receiving a `uint64_t` from the GSC, dropping it down to a `uint8_t` to store it in `vendor_command_response`, and then expanding it back up to `uint64_t` to return it to the user. Why are we doing `uint64_t` -> `uint8_t` -> `uint64_t`? -- To view, visit https://review.coreboot.org/c/coreboot/+/79736?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: Ifd0e850770152a03aa46d7f8bbb76f7520a59081 Gerrit-Change-Number: 79736 Gerrit-PatchSet: 1 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com> Gerrit-CC: Tim Van Patten <timvp(a)google.com> Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Christian Walter <christian.walter(a)9elements.com> Gerrit-Comment-Date: Wed, 27 Dec 2023 16:20:42 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[M] Change in coreboot[main]: arch/arm64: Support calling a trusted monitor
by Benjamin Doron (Code Review) 27 Dec '23

27 Dec '23

Attention is currently required from: Arthur Heymans, David Milosevic, Julius Werner. Benjamin Doron has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/78284?usp=email ) Change subject: arch/arm64: Support calling a trusted monitor ...................................................................... Patch Set 12: (1 comment) File src/arch/arm64/smc.c: https://review.coreboot.org/c/coreboot/+/78284/comment/848c343f_41946ebc : PS11, Line 10: inline uint64_t smc_call(uint32_t function_id, uint64_t arg1, uint64_t arg2, uint64_t arg3, > > It's preferable that `smc()` isn't in the header file, to ensure that callers have to go through ` […] I actually found that other boards broke if this file was compiled unconditionally. Possibly because CONFIG_ARM64_CURRENT_EL has no value? We can probably drop the assert if the Makefile guarantees it anyways. -- To view, visit https://review.coreboot.org/c/coreboot/+/78284?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I158db0b971aba722b3995d52162146aa406d1644 Gerrit-Change-Number: 78284 Gerrit-PatchSet: 12 Gerrit-Owner: Benjamin Doron <benjamin.doron00(a)gmail.com> Gerrit-Reviewer: Benjamin Doron <benjamin.doron00(a)gmail.com> Gerrit-Reviewer: David Milosevic <David.Milosevic(a)9elements.com> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Lean Sheng Tan <sheng.tan(a)9elements.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Julius Werner <jwerner(a)chromium.org> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: David Milosevic <David.Milosevic(a)9elements.com> Gerrit-Comment-Date: Wed, 27 Dec 2023 16:18:44 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Benjamin Doron <benjamin.doron00(a)gmail.com> Comment-In-Reply-To: Julius Werner <jwerner(a)chromium.org> Comment-In-Reply-To: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-MessageType: comment

1 0

[M] Change in coreboot[main]: security/tpm: Retrieve factory configuration for TI50 devices
by Subrata Banik (Code Review) 27 Dec '23

27 Dec '23

Attention is currently required from: Christian Walter. Hello Christian Walter, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/79736?usp=email to look at the new patch set (#2). Change subject: security/tpm: Retrieve factory configuration for TI50 devices ...................................................................... security/tpm: Retrieve factory configuration for TI50 devices This patch enables retrieval of factory configuration data from TI50 TPM devices. This patch utilizes vendor-specific command TPM2_TI50_SUB_CMD_GET_FACTORY_CONFIG. The factory config space is a 64-bit, one-time programmable. For the unprovisioned one, the read will be 0x0. BUG=b:317880956 TEST=Able to retrieve the factory config from google/screebo. Change-Id: Ifd0e850770152a03aa46d7f8bbb76f7520a59081 Signed-off-by: Subrata Banik <subratabanik(a)google.com> --- M src/security/tpm/tss/tcg-2.0/tss_marshaling.c M src/security/tpm/tss/tcg-2.0/tss_structures.h M src/security/tpm/tss/vendor/cr50/cr50.c M src/security/tpm/tss/vendor/cr50/cr50.h 4 files changed, 62 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/36/79736/2 -- To view, visit https://review.coreboot.org/c/coreboot/+/79736?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: Ifd0e850770152a03aa46d7f8bbb76f7520a59081 Gerrit-Change-Number: 79736 Gerrit-PatchSet: 2 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com> Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Christian Walter <christian.walter(a)9elements.com> Gerrit-MessageType: newpatchset

1 0

[S] Change in coreboot[main]: libpayload: Add VBOOT_X86_RSA_ACCELERATION config
by Jérémy Compostella (Code Review) 27 Dec '23

27 Dec '23

Attention is currently required from: Julius Werner, Yu-Ping Wu. Jérémy Compostella has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/79290?usp=email ) Change subject: libpayload: Add VBOOT_X86_RSA_ACCELERATION config ...................................................................... Patch Set 8: (1 comment) Commit Message: https://review.coreboot.org/c/coreboot/+/79290/comment/e43f75ba_5415e9e1 : PS7, Line 7: VBOOT_X86_RSA_SSE2 > VBOOT_X86_RSA_ACCELERATION Done -- To view, visit https://review.coreboot.org/c/coreboot/+/79290?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I801ebd7839261c6bd07fb218e1e36a7108e219bf Gerrit-Change-Number: 79290 Gerrit-PatchSet: 8 Gerrit-Owner: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Yu-Ping Wu <yupingso(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Julius Werner <jwerner(a)chromium.org> Gerrit-Attention: Yu-Ping Wu <yupingso(a)google.com> Gerrit-Comment-Date: Wed, 27 Dec 2023 16:16:40 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Yu-Ping Wu <yupingso(a)google.com> Gerrit-MessageType: comment

1 0

Jump to page:

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit December 2023