Feedback-ID: i4421424f:Fastmail
Message-ID: <71b48934e26a991eaf62c9869a8dfee769e0799d.camel@russell.cc>
Subject: Re: [PATCH v4 24/24] integrity/powerpc: Support loading keys from
 pseries secvar
From:   Russell Currey <ruscur@russell.cc>
To:     Mimi Zohar <zohar@linux.ibm.com>,
        Andrew Donnellan <ajd@linux.ibm.com>,
        linuxppc-dev@lists.ozlabs.org, linux-integrity@vger.kernel.org
Cc:     gregkh@linuxfoundation.org, gcwilson@linux.ibm.com,
        linux-kernel@vger.kernel.org, nayna@linux.ibm.com,
        mpe@ellerman.id.au, gjoyce@linux.ibm.com, sudhakar@linux.ibm.com,
        bgray@linux.ibm.com, erichte@linux.ibm.com, joel@jms.id.au
Date:   Wed, 25 Jan 2023 13:23:59 +1100
In-Reply-To: <57dca1ea3ef66bc0935bdd1dab4536f1151f4004.camel@linux.ibm.com>
References: <20230120074306.1326298-1-ajd@linux.ibm.com>
         <20230120074306.1326298-25-ajd@linux.ibm.com>
         <57dca1ea3ef66bc0935bdd1dab4536f1151f4004.camel@linux.ibm.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.46.3 (3.46.3-1.fc37) 
MIME-Version: 1.0
Precedence: bulk

On Tue, 2023-01-24 at 10:14 -0500, Mimi Zohar wrote:
> On Fri, 2023-01-20 at 18:43 +1100, Andrew Donnellan wrote:
> > From: Russell Currey <ruscur@russell.cc>
> >=20
> > The secvar object format is only in the device tree under powernv.
> > We now have an API call to retrieve it in a generic way, so we
> > should
> > use that instead of having to handle the DT here.
> >=20
> > Add support for pseries secvar, with the "ibm,plpks-sb-v1" format.
> > The object format is expected to be the same, so there shouldn't be
> > any
> > functional differences between objects retrieved from powernv and
> > pseries.
> >=20
> > Signed-off-by: Russell Currey <ruscur@russell.cc>
> > Signed-off-by: Andrew Donnellan <ajd@linux.ibm.com>
> >=20
> > ---
> >=20
> > v3: New patch
> >=20
> > v4: Pass format buffer size (stefanb, npiggin)
> > ---
> > =C2=A0.../integrity/platform_certs/load_powerpc.c=C2=A0=C2=A0=C2=A0=C2=
=A0 | 17 ++++++++++---
> > ----
> > =C2=A01 file changed, 10 insertions(+), 7 deletions(-)
> >=20
> > diff --git a/security/integrity/platform_certs/load_powerpc.c
> > b/security/integrity/platform_certs/load_powerpc.c
> > index dee51606d5f4..d4ce91bf3fec 100644
> > --- a/security/integrity/platform_certs/load_powerpc.c
> > +++ b/security/integrity/platform_certs/load_powerpc.c
> > @@ -10,7 +10,6 @@
> > =C2=A0#include <linux/cred.h>
> > =C2=A0#include <linux/err.h>
> > =C2=A0#include <linux/slab.h>
> > -#include <linux/of.h>
> > =C2=A0#include <asm/secure_boot.h>
> > =C2=A0#include <asm/secvar.h>
> > =C2=A0#include "keyring_handler.h"
> > @@ -59,16 +58,22 @@ static int __init load_powerpc_certs(void)
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0void *db =3D NULL, *dbx=
 =3D NULL;
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0u64 dbsize =3D 0, dbxsi=
ze =3D 0;
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0int rc =3D 0;
> > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0struct device_node *node;
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0ssize_t len;
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0char buf[32];
> > =C2=A0
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (!secvar_ops)
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0return -ENODEV;
> > =C2=A0
> > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0/* The following only applie=
s for the edk2-compat backend.
> > */
> > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0node =3D of_find_compatible_=
node(NULL, NULL, "ibm,edk2-
> > compat-v1");
> > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (!node)
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0len =3D secvar_ops->format(b=
uf, 32);
>=20
> "powerpc/secvar: Handle format string in the consumer"=C2=A0 defines
> opal_secvar_format() for the object format "ibm,secvar-backend".=C2=A0
> Here
> shouldn't it being returning the format for "ibm,edk2-compat-v1"?
>=20

They end up with the same value.  The DT structure on powernv looks
like this:

/proc/device-tree/ibm,opal/secvar:
name             "secvar"
compatible       "ibm,secvar-backend"
		 "ibm,edk2-compat-v1"
format           "ibm,edk2-compat-v1"
max-var-key-len  00000000 00000400
phandle          0000805a (32858)
max-var-size     00000000 00002000

The existing code is checking for a node compatible with "ibm,edk2-
compat-v1", which would match the node above.  opal_secvar_format()
checks for a node compatible with "ibm,secvar-backend" (again, matching
above) and then returns the contents of the "format" string, which is
"ibm,edk2-compat-v1".

Ultimately it's two different ways of doing the same thing, but this
way load_powerpc_certs() doesn't have to interact with the device tree.

- Russell


> Mimi
>=20
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (len <=3D 0)
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0return -ENODEV;
> > =C2=A0
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0// Check for known secure bo=
ot implementations from OPAL or
> > PLPKS
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (strcmp("ibm,edk2-compat-=
v1", buf) && strcmp("ibm,plpks-
> > sb-v1", buf)) {
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0pr_err("Unsupported secvar implementation \"%s\",
> > not loading certs\n", buf);
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0return -ENODEV;
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0}
> > +
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0/*
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 * Get db, and dbx. The=
y might not exist, so it isn't an
> > error if we
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 * can't get them.
> > @@ -103,8 +108,6 @@ static int __init load_powerpc_certs(void)
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0kfree(dbx);
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0}
> > =C2=A0
> > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0of_node_put(node);
> > -
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0return rc;
> > =C2=A0}
> > =C2=A0late_initcall(load_powerpc_certs);
>=20
>=20