[Skiboot] [PATCH v3 4/5] doc: add opal secure variable documentation
Eric Richter
erichte at linux.ibm.com
Thu Oct 3 10:22:34 AEST 2019
On 9/23/19 5:38 AM, Oliver O'Halloran wrote:
> 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.
>
Agreed, will move the secvar node into ibm,opal
> 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?
>
In this case, catastrophic failure is referring to some form of hardware
fault, such as bad memory. Will amend this to be more clear.
The variables are tamper resistant, but not entirely tamper proof due
to the nature of PNOR. If the contents of PNOR do not match the hash in
TPM, the variables cannot be trusted. Therefore, no variables are loaded
and this is considered a failure.
>> +
>> + 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.
>
Agreed. We have some information in progress, next set will include
a compiled document.
>> + 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?
>
There isn't a way to detect if the update section has been tampered with.
Edge case attack, but if an attacker clears the update section containing
a revocation of a compromised key, this would signal to a sysadmin or
petitboot that keys were not actually processed.
The same information may be retrievable from a log perhaps.
>> ++-----------------|-----------------------------------------------+
>> +| 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?
>
This is similar to a "status" for the backend, but a failure to process
updates may be due to user error more than a code failure. Secvar status
failure should only be due to hardware failure, bugs, or as result of
tampering.
Kernels should enforce secure boot if secvar fails to initialize. This
is a notice for sysadmins to debug.
>> +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.
>
In this case, should we collect these backend-specific modes into a new
node inside of secvar? I've been toying with that idea, to contain any
backend-specific logic away from the root secvar node.
>> 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?
>
The generic "enforce-secureboot" mode should be enough to tell the kernel
to avoid unsecure kexec, etc. The kernel would need to be updated to be
aware of extra modes, like audit mode. New or updated backends should
ideally be infrequent.
>> +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.
>
I'll expand the document to explain the (currently only) mode.
This is to enforce host secure boot, whereas "secure-mode" is firmware
secure boot. This tells skiroot (and subsequent kernels) that it must
check kernel signatures. An "audit mode", which only logs instead of
requiring, would not set this flag.
>> 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