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

[Oliver]

On Tue, Nov 28, 2017 at 5:10 PM, Vasant Hegde
<hegdevasant at 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 at 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 at linux.vnet.ibm.com>
>
> -Vasant
>