From: Russell Currey <ruscur@russell.cc>
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@russell.cc>
Signed-off-by: Andrew Donnellan <ajd@linux.ibm.com>
---
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);
+ 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);
--
2.39.0On Fri, 2023-01-20 at 18:43 +1100, Andrew Donnellan wrote:
> From: Russell Currey <ruscur@russell.cc>
>
> 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@russell.cc>
> Signed-off-by: Andrew Donnellan <ajd@linux.ibm.com>
>
> ---
>
> 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"?
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);
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>
> >
> > 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@russell.cc>
> > Signed-off-by: Andrew Donnellan <ajd@linux.ibm.com>
> >
> > ---
> >
> > 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.
- Russell
> 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);
>
>
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@russell.cc>
> > >
> > > 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@russell.cc>
> > > Signed-off-by: Andrew Donnellan <ajd@linux.ibm.com>
> > >
> > > ---
> > >
> > > 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);
> >
> >
>
On Tue, 2023-01-24 at 21:47 -0500, Mimi Zohar wrote: > 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. Will clarify in the commit message of the next revision. -- Andrew Donnellan OzLabs, ADL Canberra ajd@linux.ibm.com IBM Australia Limited
On Tue, 2023-01-24 at 10:14 -0500, Mimi Zohar wrote: > > - /* 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"? opal_secvar_format() doesn't return "ibm,secvar-backend", it searches for the device tree node named "ibm,secvar-backend", then reads and returns the contents of the property "format" under that node. The expected content of the format property is "ibm,edk2-compat-v1". -- Andrew Donnellan OzLabs, ADL Canberra ajd@linux.ibm.com IBM Australia Limited
On Fri Jan 20, 2023 at 5:43 PM AEST, Andrew Donnellan wrote: > From: Russell Currey <ruscur@russell.cc> > > 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@russell.cc> > Signed-off-by: Andrew Donnellan <ajd@linux.ibm.com> > > --- > > 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); sizeof(buf)? Thanks, Nick
© 2016 - 2025 Red Hat, Inc.