[Skiboot] [PATCH v4 04/11] doc: add opal secure variable documentation

[Eric Richter]

Hi Oliver,

I just responded to the email from Michael with some comments before I noticed this
email. I've added some additional comments here as well.

On 10/29/19 9:06 AM, Oliver O'Halloran wrote:
> On Tue, 2019-10-29 at 14:47 +1100, Michael Ellerman wrote:
>> Hi Eric,
>>
>> Eric Richter <erichte at linux.ibm.com> writes:
>>> This patch contains the work-in-progress documentation for the
>>> secure variable design in OPAL. Future revisions of this patch
>>> set will (hopefully) add new pieces of documentation here.
>>
>> The devicetree binding part of this patch (at least) is a hard
>> pre-requisite for me merging Nayna's first kernel series, because her
>> series depends on device tree properties defined here.
>>
>> See:
>>   https://lore.kernel.org/linuxppc-dev/20191024034717.70552-1-nayna@linux.ibm.com/
>>
>> If that series is going to make v5.5 then I need this patch merged into
>> skiboot sooner rather than later.
>>
>> So moving it to the start of the series, or sending it standalone might
>> be a good idea.
>>
>>> diff --git a/doc/device-tree/ibm,secureboot.rst b/doc/device-tree/ibm,secureboot.rst
>>> index 42c4ed7d..b4729a9d 100644
>>> --- a/doc/device-tree/ibm,secureboot.rst
>>> +++ b/doc/device-tree/ibm,secureboot.rst
>>> @@ -21,6 +21,13 @@ Required properties
>>>                                                It described by the ibm,cvc child
>>>                                                node.
>>>  
>>> +                        ibm,secureboot-v3  :  The container-verification-code
>>> +                                              is stored in a reserved memory.
>>> +                                              It described by the ibm,cvc child
>>> +                                              node. Secure variables are
>>> +                                              supported. `secvar` node should
>>> +                                              be created.
>>
>> I don't see the need for this new value.
>>
>> It's identical to "ibm,secureboot-v2" except that additionally secure
>> variables are supported. But support for secure variables is
>> communicated via the existence of a node compatible with ibm,secvar-v1,
>> as documented below.
>>
>> Nayna's latest patches don't look for it.
> 
> The version is mainly for the consumption of skiboot since we need to
> know some parameters of the container verification code provided by
> hostboot. The OS (currently) doesn't consume anything in
> ibm,secureboot.
> 
>>>      secure-enabled:     this property exists when the firmware stack is booting
>>>                          in secure mode (hardware secure boot jumper asserted).
>>>  
>>> @@ -33,6 +40,9 @@ Required properties
>>>  
>>>      hw-key-hash-size:   hw-key-hash size
> 
>  
>>> +    secvar:             this node is created if the platform supports secure
>>> +                        variables. Contains information about the current
>>> +                        secvar status, see 'secvar.rst'.
>>
>> It's a node, not a property, so it shouldn't be listed under "Required
>> properties".
>>
>> But there's no reason to define the location of the secvar node at all,
>> so this should just be removed from this file.
> 
> Yeah, really we just want to locate the node based on the compatible
> string. I still think the node should go under /ibm,opal/ since it's an
> OPAL API service, but whatever.
> 

This is an error, it is in /ibm,opal/ now. Disregard any changes to this file.

>>> diff --git a/doc/device-tree/secvar.rst b/doc/device-tree/secvar.rst
>>> new file mode 100644
>>> index 00000000..ddf15b38
>>> --- /dev/null
>>> +++ b/doc/device-tree/secvar.rst
>>> @@ -0,0 +1,84 @@
>>> +.. _device-tree/ibm,opal/secvar:
>>> +
>>> +secvar
>>> +======
>>> +
>>> +The ``secvar`` node provides secure variable information for the secure
>>> +boot of the target OS.
>>> +
>>> +Required properties
>>> +-------------------
>>> +
>>> +.. code-block:: none
>>
>> The value and meaning of the compatible string is not optional, you must
>> list it here, not just below as an example.
>>
>>> +
>>> +    status:             set to "fail" if the secure variables could not
>>
>> It's not clear to me that using status is necessarily the best idea,
>> given that you also define several other status like properties, and
>> using the standard status property gives you no ability to express *why*
>> the status is failed.
>>
>> But if you're going to use status then the standard values are
>> "disabled" and "okay". Also it's an optional property, and an absent
>> status is equivalent to "okay".
> 
> The DT 0.2 spec also allows for "fail" and "fail-sss" where sss is a
> device-specific reason code.
> 

This is what I intended, although I just noticed from a quick grep that
apparently nowhere else in skiboot this is used. Should I keep the "fail"
status, or change it to "disabled" for consistency?

> 
>>> +                        be initialized, validated, or some other
>>> +                        hardware problem.
>>> +
>>> +    update-status:      contains the return code of the update queue
>>> +                        process run during initialization. Signifies if
>>> +                        updates were processed or not, and if there was
>>> +                        an error. See table below.
>>
>> Encoded how? I assume you mean as a u32, but please say so.
>>
>> This is a bit weird, encoding an OPAL return value in a property. I
>> don't understand how the kernel is going to use it. I don't see any
>> patches posted yet that use it?
>>
>>> +                        TODO: This probably belongs in the backend node.
>>
>> Seems sensible.
>>
>>> +
>>> +    os-secure-enforcing: If this property exists, the system is in
>>> +                        considered to be in "OS secure mode". Kexec
>>> +                        images should be signature checked, etc.
>>
>> That's a bit vague, ie. "considered" is not very clear, and this spec
>> shouldn't talk about things like kexec, that's a Linux implementation
>> detail.
>>
>> What does the presence of this property mean? I think it means that the
>> owner/user of the system has expressed a desire that the boot loader
>> and/or operating system implement "secure boot", ie. only images that
>> are signed by an appropriate key will be loaded. If no such image is
>> found the system must not boot. I think?
> 
> I see it has having the same meaning as the existing
> /ibm,secureboot/secure-enabled property which indicates that firmware
> validated various firmware components as they were loaded. I can't
> think of any situation where the setting of os-secure-enforcing should
> ever be different to the value of secure-enable either.
> 

If there are no keys enrolled, then we can't possibly execute a secure
boot. Other secure boot mechanisms define this as a "Setup Mode", where
secure boot isn't enforced until the top level key (PK) is enrolled. Unless
these machines ship with preloaded keys, they will have to have some
permissive mode to allow the sysadmin to enroll their own keys.

The edk2-compat backend supplied in this set also includes such a mode.

> Suppose we could change the compatible for /ibm,secureboot/ to:
> 	"ibm,secureboot",
> 	"ibm,secureboot-vWHATEVER"
> 
> That would allow the kernel to scan for the more generic compatible and
> enable whatever signature checking mechanism it wants to when secure-
> enforce is set.

Would this be a better fit for the secvar or backend compatible? As far as I'm aware,
/ibm,secureboot/ is entire for firmware secure boot, not OS.

> 
> Oliver
>