[Skiboot] [PATCH v4 04/11] doc: add opal secure variable documentation
Michael Ellerman
mpe at ellerman.id.au
Tue Oct 29 14:47:47 AEDT 2019
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.
> +
> 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.
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?
> + 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?
> + 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