[Skiboot] [PATCH 02/19] hdata/spira: add ibm, secureboot node in P9

[Claudio Carvalho]

>> 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 at gmail.com>
>