[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