| Message ID | 20230120074306.1326298-25-ajd@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | pSeries dynamic secure boot secvar interface + platform keyring loading | expand |
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
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"? 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: > > - /* 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".
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.
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);