diff mbox series

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

Message ID 20191026094553.26635-5-erichte@linux.ibm.com
State Superseded
Headers show
Series Add Secure Variable Support | expand

Checks

Context Check Description
snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot-dco success Signed-off-by present
snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot fail Test snowpatch/job/snowpatch-skiboot on branch master
snowpatch_ozlabs/apply_patch success Successfully applied on branch master (d75e82dbfbb9443efeb3f9a5921ac23605aab469)

Commit Message

Eric Richter Oct. 26, 2019, 9:45 a.m. UTC
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

V4:
 - updated for new device tree changes

Signed-off-by: Eric Richter <erichte@linux.ibm.com>
---
 doc/device-tree/ibm,secureboot.rst |  10 ++
 doc/device-tree/secvar.rst         |  84 +++++++++++++
 doc/opal-api/opal-secvar.rst       | 192 +++++++++++++++++++++++++++++
 3 files changed, 286 insertions(+)
 create mode 100644 doc/device-tree/secvar.rst
 create mode 100644 doc/opal-api/opal-secvar.rst

Comments

Michael Ellerman Oct. 29, 2019, 3:47 a.m. UTC | #1
Hi Eric,

Eric Richter <erichte@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                                 |
> ++-----------------+-----------------------------------------------+
> +
Oliver O'Halloran Oct. 29, 2019, 2:06 p.m. UTC | #2
On Tue, 2019-10-29 at 14:47 +1100, Michael Ellerman wrote:
> Hi Eric,
> 
> Eric Richter <erichte@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.

The version is mainly for the consumption of skiboot since we need to
know some parameters of the container verification code provided by
hostboot. The OS (currently) doesn't consume anything in
ibm,secureboot.

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

Yeah, really we just want to locate the node based on the compatible
string. I still think the node should go under /ibm,opal/ since it's an
OPAL API service, but whatever.

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

The DT 0.2 spec also allows for "fail" and "fail-sss" where sss is a
device-specific reason code.


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

I see it has having the same meaning as the existing
/ibm,secureboot/secure-enabled property which indicates that firmware
validated various firmware components as they were loaded. I can't
think of any situation where the setting of os-secure-enforcing should
ever be different to the value of secure-enable either.

Suppose we could change the compatible for /ibm,secureboot/ to:
	"ibm,secureboot",
	"ibm,secureboot-vWHATEVER"

That would allow the kernel to scan for the more generic compatible and
enable whatever signature checking mechanism it wants to when secure-
enforce is set.

Oliver
Oliver O'Halloran Oct. 29, 2019, 3:11 p.m. UTC | #3
On Sat, 2019-10-26 at 04:45 -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
> 
> V4:
>  - updated for new device tree changes
> 
> Signed-off-by: Eric Richter <erichte@linux.ibm.com>
> ---
>  doc/device-tree/ibm,secureboot.rst |  10 ++
>  doc/device-tree/secvar.rst         |  84 +++++++++++++
>  doc/opal-api/opal-secvar.rst       | 192 +++++++++++++++++++++++++++++
>  3 files changed, 286 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.
> +
>      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..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
> +
> +    status:             set to "fail" if the secure variables could not
> +                        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.
> +                        TODO: This probably belongs in the backend node.
> +
> +    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.
> +
> +    backend:            This node contains any backend-specific
> +                        information, and is maintained by the backend driver.
> +
> +    storage:            This node contains any storage-specific
> +                        information, and is mainted by the storage driver.
> +
> +    max-var-size:       This property must be exposed as a child of the
> +                        storage driver, and determines how large a
> +                        variable can be.
> +
> +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";
> +        }

I don't know how much sense it makes to have storage and backend as
seperate nodes rather than having a single secvar node and dumping
everything in there.

The API has a single flat key-space so any limitations of the backend
and storage drivers are really limitations that apply to the API as a
whole. If you look at Nayna's patches you can see that having max-var-
size in the storage node is kind of awkward since you need to look up
the node somehow. You can find it using compatible (creates a
dependency to the specific backend) or by assuming the storage node is
actually named "storage." The latter is preferable, but is there even a
compelling reason to even tell the kernel what the storage driver is?
The whole point of having the Secvar API is to abstract away details of
how the secvars are stored.

There's a better argument for having the backend node since we need to
communicate how updates are handled. However, couldn't that be done
using a property?

Oliver

ps. You probably want to include the total space available for secvars
in the DT.
Eric Richter Oct. 29, 2019, 11:25 p.m. UTC | #4
Hi,

On 10/28/19 10:47 PM, Michael Ellerman wrote:
> Hi Eric,
> 
> Eric Richter <erichte@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                                 |
>> ++-----------------+-----------------------------------------------+
>> +
Eric Richter Oct. 29, 2019, 11:38 p.m. UTC | #5
Hi Oliver,

I just responded to the email from Michael with some comments before I noticed this
email. I've added some additional comments here as well.

On 10/29/19 9:06 AM, Oliver O'Halloran wrote:
> On Tue, 2019-10-29 at 14:47 +1100, Michael Ellerman wrote:
>> Hi Eric,
>>
>> Eric Richter <erichte@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.
> 
> The version is mainly for the consumption of skiboot since we need to
> know some parameters of the container verification code provided by
> hostboot. The OS (currently) doesn't consume anything in
> ibm,secureboot.
> 
>>>      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.
> 
> Yeah, really we just want to locate the node based on the compatible
> string. I still think the node should go under /ibm,opal/ since it's an
> OPAL API service, but whatever.
> 

This is an error, it is in /ibm,opal/ now. Disregard any changes to 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".
> 
> The DT 0.2 spec also allows for "fail" and "fail-sss" where sss is a
> device-specific reason code.
> 

This is what I intended, although I just noticed from a quick grep that
apparently nowhere else in skiboot this is used. Should I keep the "fail"
status, or change it to "disabled" for consistency?

> 
>>> +                        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?
> 
> I see it has having the same meaning as the existing
> /ibm,secureboot/secure-enabled property which indicates that firmware
> validated various firmware components as they were loaded. I can't
> think of any situation where the setting of os-secure-enforcing should
> ever be different to the value of secure-enable either.
> 

If there are no keys enrolled, then we can't possibly execute a secure
boot. Other secure boot mechanisms define this as a "Setup Mode", where
secure boot isn't enforced until the top level key (PK) is enrolled. Unless
these machines ship with preloaded keys, they will have to have some
permissive mode to allow the sysadmin to enroll their own keys.

The edk2-compat backend supplied in this set also includes such a mode.

> Suppose we could change the compatible for /ibm,secureboot/ to:
> 	"ibm,secureboot",
> 	"ibm,secureboot-vWHATEVER"
> 
> That would allow the kernel to scan for the more generic compatible and
> enable whatever signature checking mechanism it wants to when secure-
> enforce is set.

Would this be a better fit for the secvar or backend compatible? As far as I'm aware,
/ibm,secureboot/ is entire for firmware secure boot, not OS.

> 
> Oliver
>
Michael Ellerman Oct. 31, 2019, 2:46 a.m. UTC | #6
Eric Richter <erichte@linux.ibm.com> writes:
> On 10/28/19 10:47 PM, Michael Ellerman wrote:
>> Eric Richter <erichte@linux.ibm.com> writes:
...
>>> +    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.

But are we expecting a user to look at the device tree to find the
status? That seems suboptimal.

Shouldn't the status be exported via sysfs along with the other secvar
stuff?

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

Using the OPAL codes is fine, if the kernel is going to consume them.

If userspace is going to consume them then I think you'd be better off
using Linux (I guess POSIX) error codes, or possibly a string.

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

Thanks.

cheers
Michael Ellerman Oct. 31, 2019, 2:49 a.m. UTC | #7
Eric Richter <erichte@linux.ibm.com> writes:
> On 10/29/19 9:06 AM, Oliver O'Halloran wrote:
>> On Tue, 2019-10-29 at 14:47 +1100, Michael Ellerman wrote:
>>> Eric Richter <erichte@linux.ibm.com> writes:
>>>
>>>> +
>>>> +    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".
>> 
>> The DT 0.2 spec also allows for "fail" and "fail-sss" where sss is a
>> device-specific reason code.
>
> This is what I intended, although I just noticed from a quick grep that
> apparently nowhere else in skiboot this is used. Should I keep the "fail"
> status, or change it to "disabled" for consistency?

No it's fine to use "fail". I didn't realise that was a specified value
as I've only ever seen "disabled" in the wild.

cheers
Eric Richter Oct. 31, 2019, 3:06 a.m. UTC | #8
On 10/30/19 9:46 PM, Michael Ellerman wrote:
> Eric Richter <erichte@linux.ibm.com> writes:
>> On 10/28/19 10:47 PM, Michael Ellerman wrote:
>>> Eric Richter <erichte@linux.ibm.com> writes:
> ...
>>>> +    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.
> 
> But are we expecting a user to look at the device tree to find the
> status? That seems suboptimal.
> 
We don't currently have any tools that check for this, but this information
should be available. Ideally, a script, or petitboot should report the status
of the update to the user.

> Shouldn't the status be exported via sysfs along with the other secvar
> stuff?
> 

...and so maybe it should belong in sysfs as well, but the information still needs
to be handed to the kernel in some form.

>> 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?
> 
> Using the OPAL codes is fine, if the kernel is going to consume them.
> 

If the value is exposed in sysfs, should the kernel translate the value to
something else? A Linux/POSIX error code or a user-friendly string?

A sysadmin would likely inspect the OPAL msglog for more detail if there were
an error, so I would suspect keeping this as a programmatic value would make
more sense.

> If userspace is going to consume them then I think you'd be better off
> using Linux (I guess POSIX) error codes, or possibly a string.
>
>>>> +
>>>> +    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.
> 
> Thanks.
> 
> cheers
>
Oliver O'Halloran Oct. 31, 2019, 6:26 a.m. UTC | #9
On Thu, Oct 31, 2019 at 2:07 PM Eric Richter <erichte@linux.ibm.com> wrote:
>
> On 10/30/19 9:46 PM, Michael Ellerman wrote:
> > Eric Richter <erichte@linux.ibm.com> writes:
> >> On 10/28/19 10:47 PM, Michael Ellerman wrote:
> >>> Eric Richter <erichte@linux.ibm.com> writes:
> > ...
> >>>> +    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.
> >
> > But are we expecting a user to look at the device tree to find the
> > status? That seems suboptimal.
> >
> We don't currently have any tools that check for this, but this information
> should be available. Ideally, a script, or petitboot should report the status
> of the update to the user.

Sounds kind of janky, but whatever. I'm fine with this provided we do
enough pre-validation of updates to ensure that we should never see an
update failure unless the PNOR has been tampered with.

> > Shouldn't the status be exported via sysfs along with the other secvar
> > stuff?
> >
>
> ...and so maybe it should belong in sysfs as well, but the information still needs
> to be handed to the kernel in some form.

Yes, something about the expected format for updates should be in sysfs.

> >> 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?
> >
> > Using the OPAL codes is fine, if the kernel is going to consume them.
>
> If the value is exposed in sysfs, should the kernel translate the value to
> something else? A Linux/POSIX error code or a user-friendly string?

I don't have any strong opinions about what the encoding should be,
but but don't use Linux or POSIX errno values. We try to maintain the
pretense of OPAL being OS-independent so baking in Linux return codes
isn't acceptable. I'm also fairly sure POSIX doesn't actually specify
the values of the error codes, just their symbolic names. Either stick
with what you're currently doing and use the OPAL codes, defines a new
enum for secvar specific errors or put in a string.

> A sysadmin would likely inspect the OPAL msglog for more detail if there were
> an error, so I would suspect keeping this as a programmatic value would make
> more sense.

I've had people ask me where to find the OPAL msglog even after they'd
worked with OPAL systems for multiple years. Don't bet on people
knowing that they should be looking in the OPAL log.

Oliver
Michael Ellerman Nov. 2, 2019, 11:15 a.m. UTC | #10
"Oliver O'Halloran" <oohall@gmail.com> writes:
> On Thu, Oct 31, 2019 at 2:07 PM Eric Richter <erichte@linux.ibm.com> wrote:
>> On 10/30/19 9:46 PM, Michael Ellerman wrote:
>> > Eric Richter <erichte@linux.ibm.com> writes:
>> >> On 10/28/19 10:47 PM, Michael Ellerman wrote:
>> >>> Eric Richter <erichte@linux.ibm.com> writes:
>> > ...
>> >>>> +    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.
>> >
>> > But are we expecting a user to look at the device tree to find the
>> > status? That seems suboptimal.
>> >
>> We don't currently have any tools that check for this, but this information
>> should be available. Ideally, a script, or petitboot should report the status
>> of the update to the user.
>
> Sounds kind of janky, but whatever. I'm fine with this provided we do
> enough pre-validation of updates to ensure that we should never see an
> update failure unless the PNOR has been tampered with.
...
>
>> >> 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?
>> >
>> > Using the OPAL codes is fine, if the kernel is going to consume them.
>>
>> If the value is exposed in sysfs, should the kernel translate the value to
>> something else? A Linux/POSIX error code or a user-friendly string?
>
> I don't have any strong opinions about what the encoding should be,
> but but don't use Linux or POSIX errno values. We try to maintain the
> pretense of OPAL being OS-independent so baking in Linux return codes
> isn't acceptable. I'm also fairly sure POSIX doesn't actually specify
> the values of the error codes, just their symbolic names. Either stick
> with what you're currently doing and use the OPAL codes, defines a new
> enum for secvar specific errors or put in a string.

Let's just go with OPAL codes.

The kernel can either interpret them before exposing them via sysfs, or
we document that the sysfs API is using OPAL error codes for this
particular attribute.

cheers
diff mbox series

Patch

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.
+
     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..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
+
+    status:             set to "fail" if the secure variables could not
+                        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.
+                        TODO: This probably belongs in the backend node.
+
+    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.
+
+    backend:            This node contains any backend-specific
+                        information, and is maintained by the backend driver.
+
+    storage:            This node contains any storage-specific
+                        information, and is mainted by the storage driver.
+
+    max-var-size:       This property must be exposed as a child of the
+                        storage driver, and determines how large a
+                        variable can be.
+
+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                                 |
++-----------------+-----------------------------------------------+
+
diff --git a/doc/opal-api/opal-secvar.rst b/doc/opal-api/opal-secvar.rst
new file mode 100644
index 00000000..f07c570a
--- /dev/null
+++ b/doc/opal-api/opal-secvar.rst
@@ -0,0 +1,192 @@ 
+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.
+
+OPAL_SECVAR_GET
+===============
+::
+
+   #define OPAL_SECVAR_GET                         176
+
+``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                        177
+
+``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                    178
+
+``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
+
+``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