| Message ID | 1510421322-27237-3-git-send-email-cclaudio@linux.vnet.ibm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | libstb: add support for secure and trusted boot in P9 | expand |
On Sun, Nov 12, 2017 at 4:28 AM, Claudio Carvalho <cclaudio@linux.vnet.ibm.com> wrote: > In P9, skiboot builds the device tree from the HDAT. These are the > "ibm,secureboot" node changes compared to P8: > > - The Container-Verification-Code (CVC), a.k.a. ROM code, is no longer > stored in a secure ROM with static address. In P9, it is stored in a > hostboot reserved memory and each service provided also has a version, > not only an offset. > > - The hash-algo property is not provided via HDAT, instead it provides > the hw-key-hash-size, which is indeed the information required by the > CVC to verify containers. > > Although the CVC location has changed and the hash-algo is not provided > in the HDAT, the "ibm,secureboot" compatible property doesn't need to be > bumped to "ibm,secureboot-v2" in P9. For now we can initialize the CVC > early during the HDAT parsing and emulate the hash-algo from the > hw-key-hash-size. The "ibm,secureboot" node update can be postponed, > even because currently only skiboot consumes it. > > This parses the iplparams_sysparams HDAT structure and creates the > "ibm,secureboot" in P9. > > Signed-off-by: Claudio Carvalho <cclaudio@linux.vnet.ibm.com> > --- > hdata/spira.c | 39 +++++++++++++++++++++++++++++++++++++++ > hdata/spira.h | 14 ++++++++------ > 2 files changed, 47 insertions(+), 6 deletions(-) > > diff --git a/hdata/spira.c b/hdata/spira.c > index 936fadc..33926ed 100644 > --- a/hdata/spira.c > +++ b/hdata/spira.c > @@ -933,6 +933,42 @@ static void add_nmmu(void) > } > } > > +static void dt_init_secureboot_node(const struct iplparams_sysparams *sysparams) > +{ > + struct dt_node *node; > + u16 sys_sec_setting; > + u16 hw_key_hash_size; > + > + node = dt_new(dt_root, "ibm,secureboot"); > + assert(node); > + > + dt_add_property_string(node, "compatible", "ibm,secureboot-v1"); > + > + sys_sec_setting = be16_to_cpu(sysparams->sys_sec_setting); > + if (sys_sec_setting & SEC_CONTAINER_SIG_CHECKING) > + dt_add_property(node, "secure-enabled", NULL, 0); > + if (sys_sec_setting & SEC_HASHES_EXTENDED_TO_TPM) > + dt_add_property(node, "trusted-enabled", NULL, 0); > + > + hw_key_hash_size = be16_to_cpu(sysparams->hw_key_hash_size); > + > + /* > + * FIXME: replace hash-algo by the hw-key-hash-size property when > + * compatible is bumped to ibm,secureboot-v2 > + */ > + if (hw_key_hash_size == 64) { > + dt_add_property_string(node, "hash-algo", "sha512"); > + dt_add_property(node, "hw-key-hash", sysparams->hw_key_hash, > + hw_key_hash_size); > + } else { > + prlog(PR_ERR, "TPMREL: hw_key_hash_size=%d not supported\n", > + hw_key_hash_size); Hmm... If we're booting in secure mode and can't support it then booting anyway might not be the best idea. See what stewart thinks. > + } > + > + if (be16_to_cpu(sysparams->sys_attributes) & SYS_ATTR_MULTIPLE_TPM) > + prlog(PR_WARNING, "Multiple TPM set, but not supported\n"); What does the multiple TPM bit being set actually mean? And in the case where we have multiple TPMs which one do we use? > +} > + > static void add_iplparams_sys_params(const void *iplp, struct dt_node *node) > { > const struct iplparams_sysparams *p; > @@ -1019,6 +1055,9 @@ static void add_iplparams_sys_params(const void *iplp, struct dt_node *node) > sys_attributes = be32_to_cpu(p->sys_attributes); > if (sys_attributes & SYS_ATTR_RISK_LEVEL) > dt_add_property(node, "elevated-risk-level", NULL, 0); > + > + if (version >= 0x60) > + dt_init_secureboot_node(p); > } > > static void add_iplparams_ipl_params(const void *iplp, struct dt_node *node) > diff --git a/hdata/spira.h b/hdata/spira.h > index 190afad..a9f1313 100644 > --- a/hdata/spira.h > +++ b/hdata/spira.h > @@ -355,6 +355,7 @@ struct iplparams_sysparams { > __be32 abc_bus_speed; > __be32 wxyz_bus_speed; > __be32 sys_eco_mode; > +#define SYS_ATTR_MULTIPLE_TPM PPC_BIT32(0) > #define SYS_ATTR_RISK_LEVEL PPC_BIT32(3) > __be32 sys_attributes; > __be32 mem_scrubbing; > @@ -369,12 +370,13 @@ struct iplparams_sysparams { > uint8_t split_core_mode; /* >= 0x5c */ > uint8_t reserved[3]; > uint8_t sys_vendor[64]; /* >= 0x5f */ > - /* >= 0x60 */ > - __be16 sys_sec_setting; > - __be16 tpm_config_bit; > - __be16 tpm_drawer; > - __be16 reserved2; > - uint8_t hw_key_hash[64]; > +#define SEC_CONTAINER_SIG_CHECKING PPC_BIT16(0) > +#define SEC_HASHES_EXTENDED_TO_TPM PPC_BIT16(1) > + __be16 sys_sec_setting; /* >= 0x60 */ > + __be16 tpm_config_bit; /* >= 0x60 */ > + __be16 tpm_drawer; /* >= 0x60 */ > + __be16 hw_key_hash_size; /* >= 0x60 */ > + uint8_t hw_key_hash[64]; /* >= 0x60 */ > uint8_t sys_family_str[64]; /* vendor,name */ > uint8_t sys_type_str[64]; /* vendor,type */ > } __packed; > -- > 2.7.4 > Quibbles aside, Reviewed-by: Oliver O'Halloran <oohall@gmail.com>
>> diff --git a/hdata/spira.c b/hdata/spira.c >> index 936fadc..33926ed 100644 >> --- a/hdata/spira.c >> +++ b/hdata/spira.c >> @@ -933,6 +933,42 @@ static void add_nmmu(void) >> } >> } >> >> +static void dt_init_secureboot_node(const struct iplparams_sysparams *sysparams) >> +{ >> + struct dt_node *node; >> + u16 sys_sec_setting; >> + u16 hw_key_hash_size; >> + >> + node = dt_new(dt_root, "ibm,secureboot"); >> + assert(node); >> + >> + dt_add_property_string(node, "compatible", "ibm,secureboot-v1"); >> + >> + sys_sec_setting = be16_to_cpu(sysparams->sys_sec_setting); >> + if (sys_sec_setting & SEC_CONTAINER_SIG_CHECKING) >> + dt_add_property(node, "secure-enabled", NULL, 0); >> + if (sys_sec_setting & SEC_HASHES_EXTENDED_TO_TPM) >> + dt_add_property(node, "trusted-enabled", NULL, 0); >> + >> + hw_key_hash_size = be16_to_cpu(sysparams->hw_key_hash_size); >> + >> + /* >> + * FIXME: replace hash-algo by the hw-key-hash-size property when >> + * compatible is bumped to ibm,secureboot-v2 >> + */ >> + if (hw_key_hash_size == 64) { >> + dt_add_property_string(node, "hash-algo", "sha512"); >> + dt_add_property(node, "hw-key-hash", sysparams->hw_key_hash, >> + hw_key_hash_size); >> + } else { >> + prlog(PR_ERR, "TPMREL: hw_key_hash_size=%d not supported\n", >> + hw_key_hash_size); > Hmm... If we're booting in secure mode and can't support it then > booting anyway might not be the best idea. See what stewart thinks. I agree that the sooner we enforce secure mode the better. I proposed to *not* enforce secure mode here basically because: (1) during the hdat parsing we could just do the parsing (2) the libstb, which is initialized early in the boot, would be responsible to enforce secure boot, not only for "ibm,secureboot-v1" in P9, but also for P8 (including the "ibm,secureboot-v1-softrom") >> + } >> + >> + if (be16_to_cpu(sysparams->sys_attributes) & SYS_ATTR_MULTIPLE_TPM) >> + prlog(PR_WARNING, "Multiple TPM set, but not supported\n"); > What does the multiple TPM bit being set actually mean? And in the > case where we have multiple TPMs which one do we use? When the bit is set it means that hostboot found multiple TPM cards and then there should be multiple structures in the TPMREL tuple, one for each TPM. At the moment I would say that multiple TPMs could indicate a hostboot error because multiple TPMs are not supported. However, if in fact there are multiple TPMs, skiboot has a simple rule: register all TPMs and whenever an extend/measure operation is requested, the libstb will call the same operation for each TPM. > >> +} >> + >> static void add_iplparams_sys_params(const void *iplp, struct dt_node *node) >> { >> const struct iplparams_sysparams *p; >> @@ -1019,6 +1055,9 @@ static void add_iplparams_sys_params(const void *iplp, struct dt_node *node) >> sys_attributes = be32_to_cpu(p->sys_attributes); >> if (sys_attributes & SYS_ATTR_RISK_LEVEL) >> dt_add_property(node, "elevated-risk-level", NULL, 0); >> + >> + if (version >= 0x60) >> + dt_init_secureboot_node(p); >> } >> >> static void add_iplparams_ipl_params(const void *iplp, struct dt_node *node) >> diff --git a/hdata/spira.h b/hdata/spira.h >> index 190afad..a9f1313 100644 >> --- a/hdata/spira.h >> +++ b/hdata/spira.h >> @@ -355,6 +355,7 @@ struct iplparams_sysparams { >> __be32 abc_bus_speed; >> __be32 wxyz_bus_speed; >> __be32 sys_eco_mode; >> +#define SYS_ATTR_MULTIPLE_TPM PPC_BIT32(0) >> #define SYS_ATTR_RISK_LEVEL PPC_BIT32(3) >> __be32 sys_attributes; >> __be32 mem_scrubbing; >> @@ -369,12 +370,13 @@ struct iplparams_sysparams { >> uint8_t split_core_mode; /* >= 0x5c */ >> uint8_t reserved[3]; >> uint8_t sys_vendor[64]; /* >= 0x5f */ >> - /* >= 0x60 */ >> - __be16 sys_sec_setting; >> - __be16 tpm_config_bit; >> - __be16 tpm_drawer; >> - __be16 reserved2; >> - uint8_t hw_key_hash[64]; >> +#define SEC_CONTAINER_SIG_CHECKING PPC_BIT16(0) >> +#define SEC_HASHES_EXTENDED_TO_TPM PPC_BIT16(1) >> + __be16 sys_sec_setting; /* >= 0x60 */ >> + __be16 tpm_config_bit; /* >= 0x60 */ >> + __be16 tpm_drawer; /* >= 0x60 */ >> + __be16 hw_key_hash_size; /* >= 0x60 */ >> + uint8_t hw_key_hash[64]; /* >= 0x60 */ >> uint8_t sys_family_str[64]; /* vendor,name */ >> uint8_t sys_type_str[64]; /* vendor,type */ >> } __packed; >> -- >> 2.7.4 >> > Quibbles aside, > > Reviewed-by: Oliver O'Halloran <oohall@gmail.com> >
On 11/11/2017 10:58 PM, Claudio Carvalho wrote: > In P9, skiboot builds the device tree from the HDAT. These are the > "ibm,secureboot" node changes compared to P8: > > - The Container-Verification-Code (CVC), a.k.a. ROM code, is no longer > stored in a secure ROM with static address. In P9, it is stored in a > hostboot reserved memory and each service provided also has a version, > not only an offset. > > - The hash-algo property is not provided via HDAT, instead it provides > the hw-key-hash-size, which is indeed the information required by the > CVC to verify containers. > > Although the CVC location has changed and the hash-algo is not provided > in the HDAT, the "ibm,secureboot" compatible property doesn't need to be > bumped to "ibm,secureboot-v2" in P9. For now we can initialize the CVC > early during the HDAT parsing and emulate the hash-algo from the > hw-key-hash-size. The "ibm,secureboot" node update can be postponed, > even because currently only skiboot consumes it. > > This parses the iplparams_sysparams HDAT structure and creates the > "ibm,secureboot" in P9. > > Signed-off-by: Claudio Carvalho <cclaudio@linux.vnet.ibm.com> > --- > hdata/spira.c | 39 +++++++++++++++++++++++++++++++++++++++ > hdata/spira.h | 14 ++++++++------ > 2 files changed, 47 insertions(+), 6 deletions(-) > > diff --git a/hdata/spira.c b/hdata/spira.c > index 936fadc..33926ed 100644 > --- a/hdata/spira.c > +++ b/hdata/spira.c > @@ -933,6 +933,42 @@ static void add_nmmu(void) > } > } > > +static void dt_init_secureboot_node(const struct iplparams_sysparams *sysparams) > +{ > + struct dt_node *node; > + u16 sys_sec_setting; > + u16 hw_key_hash_size; > + > + node = dt_new(dt_root, "ibm,secureboot"); > + assert(node); > + > + dt_add_property_string(node, "compatible", "ibm,secureboot-v1"); > + > + sys_sec_setting = be16_to_cpu(sysparams->sys_sec_setting); > + if (sys_sec_setting & SEC_CONTAINER_SIG_CHECKING) > + dt_add_property(node, "secure-enabled", NULL, 0); > + if (sys_sec_setting & SEC_HASHES_EXTENDED_TO_TPM) > + dt_add_property(node, "trusted-enabled", NULL, 0); > + > + hw_key_hash_size = be16_to_cpu(sysparams->hw_key_hash_size); > + > + /* > + * FIXME: replace hash-algo by the hw-key-hash-size property when > + * compatible is bumped to ibm,secureboot-v2 > + */ > + if (hw_key_hash_size == 64) { > + dt_add_property_string(node, "hash-algo", "sha512"); > + dt_add_property(node, "hw-key-hash", sysparams->hw_key_hash, > + hw_key_hash_size); > + } else { > + prlog(PR_ERR, "TPMREL: hw_key_hash_size=%d not supported\n", > + hw_key_hash_size); > + } > + > + if (be16_to_cpu(sysparams->sys_attributes) & SYS_ATTR_MULTIPLE_TPM) > + prlog(PR_WARNING, "Multiple TPM set, but not supported\n"); > +} > + > static void add_iplparams_sys_params(const void *iplp, struct dt_node *node) > { > const struct iplparams_sysparams *p; > @@ -1019,6 +1055,9 @@ static void add_iplparams_sys_params(const void *iplp, struct dt_node *node) > sys_attributes = be32_to_cpu(p->sys_attributes); > if (sys_attributes & SYS_ATTR_RISK_LEVEL) > dt_add_property(node, "elevated-risk-level", NULL, 0); > + > + if (version >= 0x60) IMO its not safe to just rely on the version number. We have had number of such issues earlier. May be better add P9 check here and then check whether values are actually populated or not ? Otherwise patch looks good to me. Reviewed-by: Vasant Hegde <hegdevasant@linux.vnet.ibm.com> -Vasant
On Tue, Nov 28, 2017 at 5:10 PM, Vasant Hegde <hegdevasant@linux.vnet.ibm.com> wrote: > On 11/11/2017 10:58 PM, Claudio Carvalho wrote: >> >> In P9, skiboot builds the device tree from the HDAT. These are the >> "ibm,secureboot" node changes compared to P8: >> >> - The Container-Verification-Code (CVC), a.k.a. ROM code, is no longer >> stored in a secure ROM with static address. In P9, it is stored in a >> hostboot reserved memory and each service provided also has a version, >> not only an offset. >> >> - The hash-algo property is not provided via HDAT, instead it provides >> the hw-key-hash-size, which is indeed the information required by the >> CVC to verify containers. >> >> Although the CVC location has changed and the hash-algo is not provided >> in the HDAT, the "ibm,secureboot" compatible property doesn't need to be >> bumped to "ibm,secureboot-v2" in P9. For now we can initialize the CVC >> early during the HDAT parsing and emulate the hash-algo from the >> hw-key-hash-size. The "ibm,secureboot" node update can be postponed, >> even because currently only skiboot consumes it. >> >> This parses the iplparams_sysparams HDAT structure and creates the >> "ibm,secureboot" in P9. >> >> Signed-off-by: Claudio Carvalho <cclaudio@linux.vnet.ibm.com> >> --- >> hdata/spira.c | 39 +++++++++++++++++++++++++++++++++++++++ >> hdata/spira.h | 14 ++++++++------ >> 2 files changed, 47 insertions(+), 6 deletions(-) >> >> diff --git a/hdata/spira.c b/hdata/spira.c >> index 936fadc..33926ed 100644 >> --- a/hdata/spira.c >> +++ b/hdata/spira.c >> @@ -933,6 +933,42 @@ static void add_nmmu(void) >> } >> } >> >> +static void dt_init_secureboot_node(const struct iplparams_sysparams >> *sysparams) >> +{ >> + struct dt_node *node; >> + u16 sys_sec_setting; >> + u16 hw_key_hash_size; >> + >> + node = dt_new(dt_root, "ibm,secureboot"); >> + assert(node); >> + >> + dt_add_property_string(node, "compatible", "ibm,secureboot-v1"); >> + >> + sys_sec_setting = be16_to_cpu(sysparams->sys_sec_setting); >> + if (sys_sec_setting & SEC_CONTAINER_SIG_CHECKING) >> + dt_add_property(node, "secure-enabled", NULL, 0); >> + if (sys_sec_setting & SEC_HASHES_EXTENDED_TO_TPM) >> + dt_add_property(node, "trusted-enabled", NULL, 0); >> + >> + hw_key_hash_size = be16_to_cpu(sysparams->hw_key_hash_size); >> + >> + /* >> + * FIXME: replace hash-algo by the hw-key-hash-size property when >> + * compatible is bumped to ibm,secureboot-v2 >> + */ >> + if (hw_key_hash_size == 64) { >> + dt_add_property_string(node, "hash-algo", "sha512"); >> + dt_add_property(node, "hw-key-hash", >> sysparams->hw_key_hash, >> + hw_key_hash_size); >> + } else { >> + prlog(PR_ERR, "TPMREL: hw_key_hash_size=%d not >> supported\n", >> + hw_key_hash_size); >> + } >> + >> + if (be16_to_cpu(sysparams->sys_attributes) & >> SYS_ATTR_MULTIPLE_TPM) >> + prlog(PR_WARNING, "Multiple TPM set, but not >> supported\n"); >> +} >> + >> static void add_iplparams_sys_params(const void *iplp, struct dt_node >> *node) >> { >> const struct iplparams_sysparams *p; >> @@ -1019,6 +1055,9 @@ static void add_iplparams_sys_params(const void >> *iplp, struct dt_node *node) >> sys_attributes = be32_to_cpu(p->sys_attributes); >> if (sys_attributes & SYS_ATTR_RISK_LEVEL) >> dt_add_property(node, "elevated-risk-level", NULL, 0); >> + >> + if (version >= 0x60) > > > IMO its not safe to just rely on the version number. We have had number of > such issues earlier. > > May be better add P9 check here and then check whether values are actually > populated or not ? I agree. Looking at the size of the idata field might be more reliable. We have had problems with the fields being present and not being populated though... oh well > > Otherwise patch looks good to me. > > Reviewed-by: Vasant Hegde <hegdevasant@linux.vnet.ibm.com> > > -Vasant >
diff --git a/hdata/spira.c b/hdata/spira.c index 936fadc..33926ed 100644 --- a/hdata/spira.c +++ b/hdata/spira.c @@ -933,6 +933,42 @@ static void add_nmmu(void) } } +static void dt_init_secureboot_node(const struct iplparams_sysparams *sysparams) +{ + struct dt_node *node; + u16 sys_sec_setting; + u16 hw_key_hash_size; + + node = dt_new(dt_root, "ibm,secureboot"); + assert(node); + + dt_add_property_string(node, "compatible", "ibm,secureboot-v1"); + + sys_sec_setting = be16_to_cpu(sysparams->sys_sec_setting); + if (sys_sec_setting & SEC_CONTAINER_SIG_CHECKING) + dt_add_property(node, "secure-enabled", NULL, 0); + if (sys_sec_setting & SEC_HASHES_EXTENDED_TO_TPM) + dt_add_property(node, "trusted-enabled", NULL, 0); + + hw_key_hash_size = be16_to_cpu(sysparams->hw_key_hash_size); + + /* + * FIXME: replace hash-algo by the hw-key-hash-size property when + * compatible is bumped to ibm,secureboot-v2 + */ + if (hw_key_hash_size == 64) { + dt_add_property_string(node, "hash-algo", "sha512"); + dt_add_property(node, "hw-key-hash", sysparams->hw_key_hash, + hw_key_hash_size); + } else { + prlog(PR_ERR, "TPMREL: hw_key_hash_size=%d not supported\n", + hw_key_hash_size); + } + + if (be16_to_cpu(sysparams->sys_attributes) & SYS_ATTR_MULTIPLE_TPM) + prlog(PR_WARNING, "Multiple TPM set, but not supported\n"); +} + static void add_iplparams_sys_params(const void *iplp, struct dt_node *node) { const struct iplparams_sysparams *p; @@ -1019,6 +1055,9 @@ static void add_iplparams_sys_params(const void *iplp, struct dt_node *node) sys_attributes = be32_to_cpu(p->sys_attributes); if (sys_attributes & SYS_ATTR_RISK_LEVEL) dt_add_property(node, "elevated-risk-level", NULL, 0); + + if (version >= 0x60) + dt_init_secureboot_node(p); } static void add_iplparams_ipl_params(const void *iplp, struct dt_node *node) diff --git a/hdata/spira.h b/hdata/spira.h index 190afad..a9f1313 100644 --- a/hdata/spira.h +++ b/hdata/spira.h @@ -355,6 +355,7 @@ struct iplparams_sysparams { __be32 abc_bus_speed; __be32 wxyz_bus_speed; __be32 sys_eco_mode; +#define SYS_ATTR_MULTIPLE_TPM PPC_BIT32(0) #define SYS_ATTR_RISK_LEVEL PPC_BIT32(3) __be32 sys_attributes; __be32 mem_scrubbing; @@ -369,12 +370,13 @@ struct iplparams_sysparams { uint8_t split_core_mode; /* >= 0x5c */ uint8_t reserved[3]; uint8_t sys_vendor[64]; /* >= 0x5f */ - /* >= 0x60 */ - __be16 sys_sec_setting; - __be16 tpm_config_bit; - __be16 tpm_drawer; - __be16 reserved2; - uint8_t hw_key_hash[64]; +#define SEC_CONTAINER_SIG_CHECKING PPC_BIT16(0) +#define SEC_HASHES_EXTENDED_TO_TPM PPC_BIT16(1) + __be16 sys_sec_setting; /* >= 0x60 */ + __be16 tpm_config_bit; /* >= 0x60 */ + __be16 tpm_drawer; /* >= 0x60 */ + __be16 hw_key_hash_size; /* >= 0x60 */ + uint8_t hw_key_hash[64]; /* >= 0x60 */ uint8_t sys_family_str[64]; /* vendor,name */ uint8_t sys_type_str[64]; /* vendor,type */ } __packed;
In P9, skiboot builds the device tree from the HDAT. These are the "ibm,secureboot" node changes compared to P8: - The Container-Verification-Code (CVC), a.k.a. ROM code, is no longer stored in a secure ROM with static address. In P9, it is stored in a hostboot reserved memory and each service provided also has a version, not only an offset. - The hash-algo property is not provided via HDAT, instead it provides the hw-key-hash-size, which is indeed the information required by the CVC to verify containers. Although the CVC location has changed and the hash-algo is not provided in the HDAT, the "ibm,secureboot" compatible property doesn't need to be bumped to "ibm,secureboot-v2" in P9. For now we can initialize the CVC early during the HDAT parsing and emulate the hash-algo from the hw-key-hash-size. The "ibm,secureboot" node update can be postponed, even because currently only skiboot consumes it. This parses the iplparams_sysparams HDAT structure and creates the "ibm,secureboot" in P9. Signed-off-by: Claudio Carvalho <cclaudio@linux.vnet.ibm.com> --- hdata/spira.c | 39 +++++++++++++++++++++++++++++++++++++++ hdata/spira.h | 14 ++++++++------ 2 files changed, 47 insertions(+), 6 deletions(-)