[Skiboot] [PATCH v4 04/11] doc: add opal secure variable documentation
Oliver O'Halloran
oohall at gmail.com
Wed Oct 30 01:06:30 AEDT 2019
On Tue, 2019-10-29 at 14:47 +1100, Michael Ellerman wrote:
> Hi Eric,
>
> Eric Richter <erichte at linux.ibm.com> writes:
> > This patch contains the work-in-progress documentation for the
> > secure variable design in OPAL. Future revisions of this patch
> > set will (hopefully) add new pieces of documentation here.
>
> The devicetree binding part of this patch (at least) is a hard
> pre-requisite for me merging Nayna's first kernel series, because her
> series depends on device tree properties defined here.
>
> See:
> https://lore.kernel.org/linuxppc-dev/20191024034717.70552-1-nayna@linux.ibm.com/
>
> If that series is going to make v5.5 then I need this patch merged into
> skiboot sooner rather than later.
>
> So moving it to the start of the series, or sending it standalone might
> be a good idea.
>
> > diff --git a/doc/device-tree/ibm,secureboot.rst b/doc/device-tree/ibm,secureboot.rst
> > index 42c4ed7d..b4729a9d 100644
> > --- a/doc/device-tree/ibm,secureboot.rst
> > +++ b/doc/device-tree/ibm,secureboot.rst
> > @@ -21,6 +21,13 @@ Required properties
> > It described by the ibm,cvc child
> > node.
> >
> > + ibm,secureboot-v3 : The container-verification-code
> > + is stored in a reserved memory.
> > + It described by the ibm,cvc child
> > + node. Secure variables are
> > + supported. `secvar` node should
> > + be created.
>
> I don't see the need for this new value.
>
> It's identical to "ibm,secureboot-v2" except that additionally secure
> variables are supported. But support for secure variables is
> communicated via the existence of a node compatible with ibm,secvar-v1,
> as documented below.
>
> Nayna's latest patches don't look for it.
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
More information about the Skiboot
mailing list