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

Eric Richter erichte at linux.ibm.com
Wed Oct 30 10:25:55 AEDT 2019


Hi,

On 10/28/19 10:47 PM, 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.

Must have missed removing this change when cleaning up the patches, this is no
longer applicable. It will remain "ibm,secureboot-v2", and is completely unrelated
to secvar.

> 
>> +
>>      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.
> 
>> 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.
>

This status was intended for the basic secvar initialization, which is
separate from the individual driver inits. In other words, the OPAL API
should work if status is okay, although a driver failure may cause the API
to always return EMPTY.

> 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".
> 
>> +                        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?
> 

It is a u64, will update to mention that.

This isn't a property intended for the kernel to consume, meant for exposing
to userspace the status of update. Update processing is handled at boot, so
outside of writing to a log, it seemed like the best way to signal status
to a user.

As for the use of OPAL codes: they were convenient to use as they matched the
error cases that likely all backends will share. Would it be more clear to
define a new set of return codes, or represent this information in a
different way?

>> +                        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?
> 

This is correct, I will adjust the description to be less Linux-centric.

>> +    backend:            This node contains any backend-specific
>> +                        information, and is maintained by the backend driver.
> 
> This is under a heading called "Required properties", but it's a node.
> 
>> +
>> +    storage:            This node contains any storage-specific
>> +                        information, and is mainted by the storage driver.
> 
> Ditto.
> 
>> +
>> +    max-var-size:       This property must be exposed as a child of the
>> +                        storage driver, and determines how large a
>> +                        variable can be.
> 
> It should be in a separate section with the node it's a child of.
> 
> Is it a u32? Is that big enough?
> 
> See a reasonable example of how to write a binding here:
>   doc/device-tree/ibm,powerpc-cpu-features/binding.rst
> 
> 
> 
> cheers
> 
> 
>> +Example
>> +-------
>> +
>> +.. code-block:: dts
>> +
>> +    secvar {
>> +        compatible = "ibm,secvar-v1";
>> +        status = "okay";
>> +        os-secure-enforcing = <0x0>;
>> +        update-status = <0x0>;
>> +        storage {
>> +            compatible = "ibm,secboot-tpm-v1";
>> +            status = "okay";
>> +            max-var-size = <0x1000>;
>> +        }
>> +        backend {
>> +            compatible = "ibm,edk2-compat-v1";
>> +            status = "okay";
>> +        }
>> +    };
>> +
>> +Update Status
>> +-------------
>> +
>> +The update status property should be set by the backend driver to a value
>> +that best fits its error condition. The following table defines the
>> +general intent of each error code, check backend specific documentation
>> +for more detail.
>> +
>> ++-----------------+-----------------------------------------------+
>> +| update-status   | Generic Reason                                |
>> ++-----------------|-----------------------------------------------+
>> +| OPAL_SUCCESS    | Updates were found and processed successfully |
>> ++-----------------|-----------------------------------------------+
>> +| OPAL_EMPTY      | No updates were found, none processed         |
>> ++-----------------|-----------------------------------------------+
>> +| OPAL_PARAMETER  | Unable to parse data in the update section    |
>> ++-----------------|-----------------------------------------------+
>> +| OPAL_PERMISSION | Update failed to apply, possible auth failure |
>> ++-----------------|-----------------------------------------------+
>> +| OPAL_HARDWARE   | Misc. storage-related error                   |
>> ++-----------------|-----------------------------------------------+
>> +| OPAL_RESOURCE   | Out of space (somewhere)                      |
>> ++-----------------|-----------------------------------------------+
>> +| OPAL_NO_MEM     | Out of memory                                 |
>> ++-----------------+-----------------------------------------------+
>> +



More information about the Skiboot mailing list