On Wed, 2023-01-25 at 13:23 +1100, Russell Currey wrote: > 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@xxxxxxxxxx> > > > > > > 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. > > > > > > 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. > > > > > > Signed-off-by: Russell Currey <ruscur@xxxxxxxxxx> > > > Signed-off-by: Andrew Donnellan <ajd@xxxxxxxxxxxxx> > > > > > > --- > > > > > > v3: New patch > > > > > > v4: Pass format buffer size (stefanb, npiggin) > > > --- > > > .../integrity/platform_certs/load_powerpc.c | 17 ++++++++++--- > > > ---- > > > 1 file changed, 10 insertions(+), 7 deletions(-) > > > > > > 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 @@ > > > #include <linux/cred.h> > > > #include <linux/err.h> > > > #include <linux/slab.h> > > > -#include <linux/of.h> > > > #include <asm/secure_boot.h> > > > #include <asm/secvar.h> > > > #include "keyring_handler.h" > > > @@ -59,16 +58,22 @@ static int __init load_powerpc_certs(void) > > > void *db = NULL, *dbx = NULL; > > > u64 dbsize = 0, dbxsize = 0; > > > int rc = 0; > > > - struct device_node *node; > > > + ssize_t len; > > > + char buf[32]; > > > > > > if (!secvar_ops) > > > return -ENODEV; > > > > > > - /* The following only applies for the edk2-compat backend. > > > */ > > > - node = of_find_compatible_node(NULL, NULL, "ibm,edk2- > > > compat-v1"); > > > - if (!node) > > > + len = secvar_ops->format(buf, 32); > > > > "powerpc/secvar: Handle format string in the consumer" defines > > opal_secvar_format() for the object format "ibm,secvar-backend". > > Here > > shouldn't it being returning the format for "ibm,edk2-compat-v1"? > > > > 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. Agreed. Thank you for the explanation. To simplify review, I suggest either adding this explanation in the patch description or stage the change by replacing the existing "ibm,edk2-compat-v1" usage first. thanks, Mimi > > > > Mimi > > > > > + if (len <= 0) > > > return -ENODEV; > > > > > > + // Check for known secure boot implementations from OPAL or > > > PLPKS > > > + if (strcmp("ibm,edk2-compat-v1", buf) && strcmp("ibm,plpks- > > > sb-v1", buf)) { > > > + pr_err("Unsupported secvar implementation \"%s\", > > > not loading certs\n", buf); > > > + return -ENODEV; > > > + } > > > + > > > /* > > > * Get db, and dbx. They might not exist, so it isn't an > > > error if we > > > * can't get them. > > > @@ -103,8 +108,6 @@ static int __init load_powerpc_certs(void) > > > kfree(dbx); > > > } > > > > > > - of_node_put(node); > > > - > > > return rc; > > > } > > > late_initcall(load_powerpc_certs); > > > > >