[Skiboot] [PATCH v3 4/5] doc: add opal secure variable documentation
Oliver O'Halloran
oohall at gmail.com
Mon Sep 23 20:38:09 AEST 2019
On Tue, 2019-09-03 at 16:34 -0500, Eric Richter wrote:
> 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.
>
> V3:
> - removed metadata
> - removed get_size
> - updated _get semantics for size queries
> - added/expanded device tree properties
>
> Signed-off-by: Eric Richter <erichte at linux.ibm.com>
> ---
> doc/device-tree/ibm,secureboot.rst | 10 ++
> doc/device-tree/secvar.rst | 84 +++++++++++++
> doc/opal-api/opal-secvar.rst | 188 +++++++++++++++++++++++++++++
> 3 files changed, 282 insertions(+)
> create mode 100644 doc/device-tree/secvar.rst
> create mode 100644 doc/opal-api/opal-secvar.rst
>
> 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 think changing the compatible string is necessary, or a good
idea. The ibm,secureboot node is largely an implementation detail of
how Hostboot and Skiboot implemented firmware secure boot rather than
being part of the interface OPAL exposes to the OS. I'd be happier if
it wasn't a top-level DT node, but that's where hostboot decided to put
it back in the P8 days.
Anyway, We needed to change the compatible between v1 and v2 because
how we (skiboot) accessed the CVC image changed in a backwards
incompatible manner. The Hostboot<->Skiboot interface isn't really
affected by the presernce of secvars since they're are really an OPAL
API feature rather than something we're interested in consuming
internally.
> 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'.
>
> Obsolete properties
> -------------------
> diff --git a/doc/device-tree/secvar.rst b/doc/device-tree/secvar.rst
> new file mode 100644
> index 00000000..c439a123
> --- /dev/null
> +++ b/doc/device-tree/secvar.rst
> @@ -0,0 +1,84 @@
> +.. _device-tree/ibm,secureboot/secvar:
> +
> +secvar
> +======
> +
> +The ``secvar`` node provides secure variable information for the secure
> +boot of the target OS.
> +
> +Required properties
> +-------------------
> +
> +.. code-block:: none
> +
> + compatible: this property is set based on the current secure
> + variable scheme as set by the platform.
> +
> + status: set to "fail" if the secure variables could not
> + be initialized, validated, or some other
> + catastrophic failure.
>From the point of view of petitboot, what does a catastrophic failure
mean? We can't rely on the secvars being tamper proof?
> +
> + 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
The size of update-status needs to be specificied since it's a signed
int.
> +
> + secure-mode: a u64 bitfield set by the backend to determine
> + what secure mode we should be in, and if host
> + secure boot should be enforced.
> +
> +Example
> +-------
> +
> +.. code-block:: dts
> +
> + secvar {
> + compatible = "ibm,edk2-compat-v1";
The expectations of the individual backends need to be documented
somewhere. For edk2 the backend has pretty wierd semantics around
variable verification so that needs to be documented.
> + status = "okay";
> + secure-mode = "1";
The DTS syntax for a numeric property is:
secure-mode = <1>;
Using secure-mode = "1" sets the property to be a null terminated
string containing just the character '1'.
> + };
> +
> +Update Status
> +-------------
> +
> +The update status property should be set by the backend driver to a value
> +that best fits its error
> condtion.
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 |
Why do we care about OPAL_EMPTY? I'd expect that no updates to process
would be the common condition. Also, if we get an error here what can
we do with that information?
> ++-----------------|-----------------------------------------------+
> +| 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 |
> ++-----------------+-----------------------------------------------+
How does this interact with the status property? The DT spec says that
failed devices should have status="failed-<reason>" so should we be
reporting these conditions there too?
> +Secure Mode
> +-----------
> +
> ++-----------------------+------------------------+
> +| backend specific-bits | generic mode bits |
> ++-----------------------+------------------------+
> +64 32 0
> +
> +The secure mode property should be set by the backend driver. The least
> +significant 32 bits are reserved for generic modes, shared across all
> +possible backends. The other 32 bits are open for backends to determine
> +their own modes.
I'd rather flags (generic and backend specific) went into devicetree
properties rather than into a bitfield, The main niceity of DT over
binary-table based formats is that we can give things meaningful names
so we should be doing that.
> Any kernel must be made aware of any custom modes.
What happens if they aren't? In general we can't assume a distro kernel
is aware of the feature so what should a kernel do if it doesn't
recognise the backend and how to talk to it?
> +At the moment, only one general-purpose bit is defined:
> +
> +``#define SECVAR_SECURE_MODE_ENFORCING 0x1``
> +
> +which signals that a kernel should enforce host secure boot.
You're going to have to be a bit more specific about what "enforce
secure boot" is supposed to mean. Also, you need to document how this
is different to the /ibm,secureboot/secure-mode property. I'm guessing
it's because there might not be any keys for the backend, but this is
documentation so I shouldn't have to guess.
> diff --git a/doc/opal-api/opal-secvar.rst b/doc/opal-api/opal-secvar.rst
> new file mode 100644
> index 00000000..d304db8f
> --- /dev/null
> +++ b/doc/opal-api/opal-secvar.rst
> @@ -0,0 +1,188 @@
> +OPAL Secure Variable API
> +========================
> +
> +Overview
> +--------
> +
> +In order to support host OS secure boot on POWER systems, the platform needs
> +some form of tamper-resistant persistant storage for authorized public keys.
> +Furthermore, these keys must be retrieveable by the host kernel, and new
> +keys must be able to be submitted.
> +
> +OPAL exposes an abstracted "variable" API, in which these keys can be stored
> +and retrieved. At a high level, ``opal_secvar_get`` retrieves a specific
> +variable corresponding to a particular key. ``opal_secvar_get_next`` can be
> +used to iterate through the keys of the stored variables.
> +``opal_secvar_enqueue_update`` can be used to submit a new variable for
> +processing on next boot.
Needs something on how this update call interacts with the backend.
> +
> +OPAL_SECVAR_GET
> +===============
> +::
> +
> + #define OPAL_SECVAR_GET 173
> +
> +``OPAL_SECVAR_GET`` call retrieves a data blob associated with the supplied
> +key.
> +
> +
> +Parameters
> +----------
> +::
> +
> + char *key
> + uint64_t key_len
> + void *data
> + uint64_t *data_size
> +
> +``key``
> + a buffer used to associate with the variable data. May
> +be any encoding, but must not be all zeroes
> +
> +``key_len``
> + size of the key buffer in bytes
> +
> +``data``
> + return buffer to store the data blob of the requested variable if
> +a match was found. May be set to NULL to only query the size into
> +``data_size``
> +
> +``data_size``
> + reference to the size of the ``data`` buffer. OPAL sets this to
> +the size of the requested variable if found.
> +
> +
> +Return Values
> +-------------
> +
> +``OPAL_SUCCESS``
> + the requested data blob was copied successfully. ``data`` was NULL,
> +and the ``data_size`` value was set successfully
> +
> +``OPAL_PARAMETER``
> + ``key`` is NULL. ``key_len`` is zero. ``data_size`` is NULL.
> +
> +``OPAL_EMPTY``
> + no variable with the supplied ``key`` was found
> +
> +``OPAL_PARTIAL``
> + the buffer size provided in ``data_size`` was insufficient.
> +``data_size`` is set to the minimum required size.
> +
> +``OPAL_UNSUPPORTED``
> + secure variables are not supported by the platform
> +
> +``OPAL_RESOURCE``
> + secure variables are supported, but did not initialize properly
> +
> +OPAL_SECVAR_GET_NEXT
> +====================
> +::
> +
> + #define OPAL_SECVAR_GET_NEXT 174
> +
> +``OPAL_SECVAR_GET_NEXT`` returns the key of the next variable in the secure
> +variable bank in sequence.
> +
> +Parameters
> +----------
> +::
> +
> + char *key
> + uint64_t *key_len
> + uint64_t key_buf_size
> +
> +
> +``key``
> + name of the previous variable or empty. The key of the next
> +variable in sequence will be copied to ``key``. If passed as empty,
> +returns the first variable in the bank
> +
> +``key_len``
> + length in bytes of the key in the ``key`` buffer. OPAL sets
> +this to the length in bytes of the next variable in sequence
> +
> +``key_buf_size``
> + maximum size of the ``key`` buffer. The next key will not be
> +copied if this value is less than the length of the next key
> +
> +
> +Return Values
> +-------------
> +
> +``OPAL_SUCCESS``
> + the key and length of the next variable in sequence was copied
> +successfully
> +
> +``OPAL_PARAMETER``
> + ``key`` or ``key_length`` is NULL. ``key_size`` is zero.
> +``key_length`` is impossibly large. No variable with the associated
> +``key`` was found
> +
> +``OPAL_EMPTY``
> + end of list reached
> +
> +``OPAL_PARTIAL``
> + the size specified in ``key_size`` is insufficient for the next
> +variable's key length. ``key_length`` is set to the next variable's
> +length, but ``key`` is untouched
> +
> +``OPAL_UNSUPPORTED``
> + secure variables are not supported by the platform
> +
> +``OPAL_RESOURCE``
> + secure variables are supported, but did not initialize properly
> +
> +OPAL_SECVAR_ENQUEUE_UPDATE
> +==========================
> +::
> +
> + #define OPAL_SECVAR_ENQUEUE_UPDATE 175
> +
> +``OPAL_SECVAR_ENQUEUE`` call appends the supplied variable data to the
> +queue for processing on next boot.
> +
> +Parameters
> +----------
> +::
> +
> + char *key
> + uint64_t key_len
> + void *data
> + uint64_t data_size
> +
> +``key``
> + a buffer used to associate with the variable data. May
> +be any encoding, but must not be all zeroes
> +
> +``key_len``
> + size of the key buffer in bytes
> +
> +``data``
> + buffer containing the blob of data to enqueue
> +
> +``data_size``
> + size of the ``data`` buffer
> +
> +Return Values
> +-------------
> +
> +``OPAL_SUCCESS``
> + the variable was appended to the update queue bank successfully
> +
> +``OPAL_PARAMETER``
> + ``key`` or ``data`` was NULL. ``key`` was empty. ``key_len`` or
> +``data_size`` was zero. ``key_len``, ``data_size`` is larger than
> +the maximum size
Put each condition on a seperate line to make it easier to scan. Sphinx
might be doing this for you in the rendered docs, but I'd like the text
to be readable too.
> +``OPAL_NO_MEM``
> + OPAL was unable to allocate memory for the variable update
> +
> +``OPAL_HARDWARE``
> + OPAL was unable to write the update to persistant storage
> +
> +``OPAL_UNSUPPORTED``
> + secure variables are not supported by the platform
> +
> +``OPAL_RESOURCE``
> + secure variables are supported, but did not initialize properly
More information about the Skiboot
mailing list