| Message ID | 20191026094553.26635-5-erichte@linux.ibm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add Secure Variable Support | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch master (d75e82dbfbb9443efeb3f9a5921ac23605aab469) |
| snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot | fail | Test snowpatch/job/snowpatch-skiboot on branch master |
| snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot-dco | success | Signed-off-by present |
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 | > ++-----------------+-----------------------------------------------+ > +
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
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.
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 | >> ++-----------------+-----------------------------------------------+ >> +
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 >
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
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
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 >
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
"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 --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
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