Re: [PATCH v4 24/24] integrity/powerpc: Support loading keys from pseries secvar

[Mimi Zohar]

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);
> > 
> > 
>