This series exposes an interface to userspace for reading and writing
secure variables contained within the PowerVM LPAR Platform KeyStore
(PLPKS) for the purpose of configuring dynamic secure boot, and adds
the glue required to load keys from the PLPKS into the platform keyring.
This series builds on past work by Nayna Jain[0] in exposing PLPKS
variables to userspace. Rather than being a generic interface for
interacting with the keystore, however, we use the existing powerpc
secvar infrastructure to only expose objects in the keystore used
for dynamic secure boot. This has the benefit of leveraging an
existing interface and making the implementation relatively minimal.
This series integrates a previous series to fix some bugs in PLPKS
and implement support for signed updates[1], and a cleanup patch from
Michael Ellerman[2].
There are a few relevant details to note about the implementation:
* New additions to the secvar API: format(), max_size(), config_attrs,
var_names
* New optional sysfs directory "config/" for arbitrary ASCII variables
* Some OPAL-specific code has been relocated from secvar-sysfs.c to
powernv platform code. Would appreciate any powernv testing!
* Variable names are fixed and only those used for secure boot are
exposed. This is not a generic PLPKS interface, but also doesn't
preclude one being added in future.
With this series, both powernv and pseries platforms support dynamic
secure boot through the same interface.
[0]: https://lore.kernel.org/linuxppc-dev/[email protected]/
[1]: https://lore.kernel.org/linuxppc-dev/[email protected]/
[2]: https://lore.kernel.org/linuxppc-dev/[email protected]/
v1: https://lore.kernel.org/linuxppc-dev/[email protected]/
v2: https://lore.kernel.org/linuxppc-dev/[email protected]/
=================
Changes in v3:
Integrate Andrew's PLPKS bugfixes and enhancements series and Michael
Ellerman's u64 cleanup patch into this series (and update the other
patches to use u64)
New patches to load keys from the PLPKS into the kernel's platform
keyring (ruscur)
New patches to pass PLPKS password to new kernels when kexecing
(ruscur)
Improve handling of format strings (ruscur)
Clean up secvar error messages (ajd)
Merge config attributes into secvar_operations (mpe)
Add a new static variable names API rather than (ab)using get_next()
(ajd/mpe)
Warning message when PAGE_SIZE is smaller than the max object size
(ajd)
Move plpks.h to the include directory, and move a bunch of constants
in there with a consistent naming scheme
Refresh PLPKS config values whenever plpks_get_usedspace() is called
(ajd)
Extra validation on PLPKS config values (ruscur)
Return maxobjlabelsize to userspace as is without subtracting overhead (ruscur)
Fix error code handling in plpks_confirm_object_flushed() (ruscur)
Pass plpks_var struct to plpks_signed_update_var() by reference (mpe)
Make the data buffer in plpks_read_var() caller-allocated to reduce
number of allocations/copies (mpe)
Rework the Kconfig options so that PSERIES_PLPKS is a hidden option,
turned on by enabling PPC_SECURE_BOOT, and the PLPKS secvar code is
activated by PPC_SECVAR_SYSFS to match powernv (ajd)
Use machine_arch_initcall() rather than device_initcall() so we don't
break powernv (mpe)
Improve ABI documentation (mpe)
Return -EIO on most read errors (mpe)
Add "grubdbx" variable (Sudhakar)
Use utf8s_to_utf16s() rather than our own "UCS-2" conversion code (mpe)
Fix SB_VERSION data length (ruscur)
Stop prepending policy data on read (ruscur)
Don't print errors to the kernel log when reading non-existent
variables (Sudhakar)
Miscellaneous code style, checkpatch cleanups
Changes in v2:
Remove unnecessary config vars from sysfs and document the others,
thanks to review from Greg. If we end up needing to expose more, we
can add them later and update the docs.
Use sysfs_emit() instead of sprintf() for all sysfs strings
Change the size of the sysfs binary attributes to include the 8-byte
flags header, preventing truncation of large writes.
Andrew Donnellan (8):
powerpc/secvar: Clean up init error messages
powerpc/secvar: Allow backend to populate static list of variable
names
powerpc/secvar: Warn when PAGE_SIZE is smaller than max object size
powerpc/secvar: Don't print error on ENOENT when reading variables
powerpc/pseries: Fix handling of PLPKS object flushing timeout
powerpc/pseries: Fix alignment of PLPKS structures and buffers
powerpc/pseries: Make caller pass buffer to plpks_read_var()
powerpc/pseries: Turn PSERIES_PLPKS into a hidden option
Michael Ellerman (1):
powerpc/secvar: Use u64 in secvar_operations
Nayna Jain (2):
powerpc/pseries: Expose PLPKS config values, support additional fields
powerpc/pseries: Implement signed update for PLPKS objects
Russell Currey (13):
powerpc/secvar: WARN_ON_ONCE() if multiple secvar ops are set
powerpc/secvar: Use sysfs_emit() instead of sprintf()
powerpc/secvar: Handle format string in the consumer
powerpc/secvar: Handle max object size in the consumer
powerpc/secvar: Extend sysfs to include config vars
powerpc/pseries: Move plpks.h to include directory
powerpc/pseries: Move PLPKS constants to header file
powerpc/pseries: Log hcall return codes for PLPKS debug
powerpc/pseries: Add helpers to get PLPKS password
powerpc/pseries: Pass PLPKS password on kexec
powerpc/pseries: Implement secvars for dynamic secure boot
integrity/powerpc: Improve error handling & reporting when loading
certs
integrity/powerpc: Support loading keys from pseries secvar
Documentation/ABI/testing/sysfs-secvar | 75 +++-
arch/powerpc/Kconfig | 1 +
arch/powerpc/include/asm/hvcall.h | 3 +-
arch/powerpc/include/asm/plpks.h | 187 ++++++++++
arch/powerpc/include/asm/secvar.h | 19 +-
arch/powerpc/kernel/secvar-ops.c | 4 +-
arch/powerpc/kernel/secvar-sysfs.c | 168 +++++----
arch/powerpc/kexec/file_load_64.c | 17 +-
arch/powerpc/platforms/powernv/opal-secvar.c | 53 ++-
arch/powerpc/platforms/pseries/Kconfig | 11 +-
arch/powerpc/platforms/pseries/Makefile | 4 +-
arch/powerpc/platforms/pseries/plpks-secvar.c | 214 +++++++++++
arch/powerpc/platforms/pseries/plpks.c | 338 ++++++++++++++----
arch/powerpc/platforms/pseries/plpks.h | 71 ----
.../integrity/platform_certs/load_powerpc.c | 47 ++-
15 files changed, 965 insertions(+), 247 deletions(-)
create mode 100644 arch/powerpc/include/asm/plpks.h
create mode 100644 arch/powerpc/platforms/pseries/plpks-secvar.c
delete mode 100644 arch/powerpc/platforms/pseries/plpks.h
--
2.39.0
From: Russell Currey <[email protected]>
Move plpks.h from platforms/pseries/ to include/asm/. This is necessary
for later patches to make use of the PLPKS from code in other subsystems.
Signed-off-by: Russell Currey <[email protected]>
Signed-off-by: Andrew Donnellan <[email protected]>
---
v3: New patch
---
.../powerpc/{platforms/pseries => include/asm}/plpks.h | 10 +++++++---
arch/powerpc/platforms/pseries/plpks.c | 3 +--
2 files changed, 8 insertions(+), 5 deletions(-)
rename arch/powerpc/{platforms/pseries => include/asm}/plpks.h (89%)
diff --git a/arch/powerpc/platforms/pseries/plpks.h b/arch/powerpc/include/asm/plpks.h
similarity index 89%
rename from arch/powerpc/platforms/pseries/plpks.h
rename to arch/powerpc/include/asm/plpks.h
index 275ccd86bfb5..8295502ee93b 100644
--- a/arch/powerpc/platforms/pseries/plpks.h
+++ b/arch/powerpc/include/asm/plpks.h
@@ -6,8 +6,10 @@
* Platform keystore for pseries LPAR(PLPKS).
*/
-#ifndef _PSERIES_PLPKS_H
-#define _PSERIES_PLPKS_H
+#ifndef _ASM_POWERPC_PLPKS_H
+#define _ASM_POWERPC_PLPKS_H
+
+#ifdef CONFIG_PSERIES_PLPKS
#include <linux/types.h>
#include <linux/list.h>
@@ -68,4 +70,6 @@ int plpks_read_fw_var(struct plpks_var *var);
*/
int plpks_read_bootloader_var(struct plpks_var *var);
-#endif
+#endif // CONFIG_PSERIES_PLPKS
+
+#endif // _ASM_POWERPC_PLPKS_H
diff --git a/arch/powerpc/platforms/pseries/plpks.c b/arch/powerpc/platforms/pseries/plpks.c
index 4edd1585e245..955117f81e50 100644
--- a/arch/powerpc/platforms/pseries/plpks.c
+++ b/arch/powerpc/platforms/pseries/plpks.c
@@ -18,8 +18,7 @@
#include <linux/types.h>
#include <asm/hvcall.h>
#include <asm/machdep.h>
-
-#include "plpks.h"
+#include <asm/plpks.h>
#define PKS_FW_OWNER 0x1
#define PKS_BOOTLOADER_OWNER 0x2
--
2.39.0
Currently, plpks_read_var() allocates a buffer to pass to the
H_PKS_READ_OBJECT hcall, then allocates another buffer, of the caller's
preferred size if necessary, into which the data is copied, and returns
that buffer to the caller.
This is a bit over the top - while we probably still want to allocate a
separate buffer to pass to the hypervisor in the hcall, we can let the
caller allocate the final buffer and specify the size.
Don't allocate var->data in plpks_read_var(), instead expect the caller to
allocate it. If the caller needs to discover the size, it can set
var->data to NULL and var->datalen will be populated. Update header file
to document this.
Suggested-by: Michael Ellerman <[email protected]>
Signed-off-by: Andrew Donnellan <[email protected]>
Signed-off-by: Russell Currey <[email protected]>
---
v3: New patch (mpe)
---
arch/powerpc/include/asm/plpks.h | 12 ++++++++++++
arch/powerpc/platforms/pseries/plpks.c | 11 ++++-------
2 files changed, 16 insertions(+), 7 deletions(-)
diff --git a/arch/powerpc/include/asm/plpks.h b/arch/powerpc/include/asm/plpks.h
index e7204e6c0ca4..0c49969b0864 100644
--- a/arch/powerpc/include/asm/plpks.h
+++ b/arch/powerpc/include/asm/plpks.h
@@ -88,16 +88,28 @@ int plpks_remove_var(char *component, u8 varos,
/**
* Returns the data for the specified os variable.
+ *
+ * Caller must allocate a buffer in var->data with length in var->datalen.
+ * If no buffer is provided, var->datalen will be populated with the object's
+ * size.
*/
int plpks_read_os_var(struct plpks_var *var);
/**
* Returns the data for the specified firmware variable.
+ *
+ * Caller must allocate a buffer in var->data with length in var->datalen.
+ * If no buffer is provided, var->datalen will be populated with the object's
+ * size.
*/
int plpks_read_fw_var(struct plpks_var *var);
/**
* Returns the data for the specified bootloader variable.
+ *
+ * Caller must allocate a buffer in var->data with length in var->datalen.
+ * If no buffer is provided, var->datalen will be populated with the object's
+ * size.
*/
int plpks_read_bootloader_var(struct plpks_var *var);
diff --git a/arch/powerpc/platforms/pseries/plpks.c b/arch/powerpc/platforms/pseries/plpks.c
index 96a026a37285..5d9c6a3b2014 100644
--- a/arch/powerpc/platforms/pseries/plpks.c
+++ b/arch/powerpc/platforms/pseries/plpks.c
@@ -578,17 +578,14 @@ static int plpks_read_var(u8 consumer, struct plpks_var *var)
goto out_free_output;
}
- if (var->datalen == 0 || var->datalen > retbuf[0])
+ if (!var->data || var->datalen > retbuf[0])
var->datalen = retbuf[0];
- var->data = kzalloc(var->datalen, GFP_KERNEL);
- if (!var->data) {
- rc = -ENOMEM;
- goto out_free_output;
- }
var->policy = retbuf[1];
- memcpy(var->data, output, var->datalen);
+ if (var->data)
+ memcpy(var->data, output, var->datalen);
+
rc = 0;
out_free_output:
--
2.39.0
From: Russell Currey <[email protected]>
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 <[email protected]>
Signed-off-by: Andrew Donnellan <[email protected]>
---
v3: New patch
---
.../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..8fa899616d92 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[SECVAR_MAX_FORMAT_LEN];
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);
+ 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.0
From: Russell Currey <[email protected]>
The code that handles the format string in secvar-sysfs.c is entirely
OPAL specific, so create a new "format" op in secvar_operations to make
the secvar code more generic. No functional change.
Signed-off-by: Russell Currey <[email protected]>
Signed-off-by: Andrew Donnellan <[email protected]>
---
v2: Use sysfs_emit() instead of sprintf() (gregkh)
v3: Enforce format string size limit (ruscur)
---
arch/powerpc/include/asm/secvar.h | 3 +++
arch/powerpc/kernel/secvar-sysfs.c | 23 ++++--------------
arch/powerpc/platforms/powernv/opal-secvar.c | 25 ++++++++++++++++++++
3 files changed, 33 insertions(+), 18 deletions(-)
diff --git a/arch/powerpc/include/asm/secvar.h b/arch/powerpc/include/asm/secvar.h
index 07ba36f868a7..8b6475589120 100644
--- a/arch/powerpc/include/asm/secvar.h
+++ b/arch/powerpc/include/asm/secvar.h
@@ -11,12 +11,15 @@
#include <linux/types.h>
#include <linux/errno.h>
+#define SECVAR_MAX_FORMAT_LEN 30 // max length of string returned by ->format()
+
extern const struct secvar_operations *secvar_ops;
struct secvar_operations {
int (*get)(const char *key, u64 key_len, u8 *data, u64 *data_size);
int (*get_next)(const char *key, u64 *key_len, u64 keybufsize);
int (*set)(const char *key, u64 key_len, u8 *data, u64 data_size);
+ ssize_t (*format)(char *buf);
};
#ifdef CONFIG_PPC_SECURE_BOOT
diff --git a/arch/powerpc/kernel/secvar-sysfs.c b/arch/powerpc/kernel/secvar-sysfs.c
index 462cacc0ca60..d3858eedd72c 100644
--- a/arch/powerpc/kernel/secvar-sysfs.c
+++ b/arch/powerpc/kernel/secvar-sysfs.c
@@ -21,26 +21,13 @@ static struct kset *secvar_kset;
static ssize_t format_show(struct kobject *kobj, struct kobj_attribute *attr,
char *buf)
{
- ssize_t rc = 0;
- struct device_node *node;
- const char *format;
-
- node = of_find_compatible_node(NULL, NULL, "ibm,secvar-backend");
- if (!of_device_is_available(node)) {
- rc = -ENODEV;
- goto out;
- }
+ char tmp[SECVAR_MAX_FORMAT_LEN];
+ ssize_t len = secvar_ops->format(tmp);
- rc = of_property_read_string(node, "format", &format);
- if (rc)
- goto out;
+ if (len <= 0)
+ return -EIO;
- rc = sysfs_emit(buf, "%s\n", format);
-
-out:
- of_node_put(node);
-
- return rc;
+ return sysfs_emit(buf, "%s\n", tmp);
}
diff --git a/arch/powerpc/platforms/powernv/opal-secvar.c b/arch/powerpc/platforms/powernv/opal-secvar.c
index ef89861569e0..623c6839e66c 100644
--- a/arch/powerpc/platforms/powernv/opal-secvar.c
+++ b/arch/powerpc/platforms/powernv/opal-secvar.c
@@ -98,10 +98,35 @@ static int opal_set_variable(const char *key, u64 ksize, u8 *data, u64 dsize)
return opal_status_to_err(rc);
}
+static ssize_t opal_secvar_format(char *buf)
+{
+ ssize_t rc = 0;
+ struct device_node *node;
+ const char *format;
+
+ node = of_find_compatible_node(NULL, NULL, "ibm,secvar-backend");
+ if (!of_device_is_available(node)) {
+ rc = -ENODEV;
+ goto out;
+ }
+
+ rc = of_property_read_string(node, "format", &format);
+ if (rc)
+ goto out;
+
+ rc = snprintf(buf, SECVAR_MAX_FORMAT_LEN, "%s", format);
+
+out:
+ of_node_put(node);
+
+ return rc;
+}
+
static const struct secvar_operations opal_secvar_ops = {
.get = opal_get_variable,
.get_next = opal_get_next_variable,
.set = opal_set_variable,
+ .format = opal_secvar_format,
};
static int opal_secvar_probe(struct platform_device *pdev)
--
2.39.0
From: Russell Currey <[email protected]>
The plpks code converts hypervisor return codes into their Linux
equivalents so that users can understand them. Having access to the
original return codes is really useful for debugging, so add a
pr_debug() so we don't lose information from the conversion.
Signed-off-by: Russell Currey <[email protected]>
Signed-off-by: Andrew Donnellan <[email protected]>
---
arch/powerpc/platforms/pseries/plpks.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/powerpc/platforms/pseries/plpks.c b/arch/powerpc/platforms/pseries/plpks.c
index 796ed5544ee5..96a026a37285 100644
--- a/arch/powerpc/platforms/pseries/plpks.c
+++ b/arch/powerpc/platforms/pseries/plpks.c
@@ -117,6 +117,8 @@ static int pseries_status_to_err(int rc)
err = -EINVAL;
}
+ pr_debug("Converted hypervisor code %d to Linux %d\n", rc, err);
+
return err;
}
--
2.39.0
On Wed Jan 18, 2023 at 4:10 PM AEST, Andrew Donnellan wrote:
> From: Russell Currey <[email protected]>
>
> The code that handles the format string in secvar-sysfs.c is entirely
> OPAL specific, so create a new "format" op in secvar_operations to make
> the secvar code more generic. No functional change.
>
> Signed-off-by: Russell Currey <[email protected]>
> Signed-off-by: Andrew Donnellan <[email protected]>
>
> ---
>
> v2: Use sysfs_emit() instead of sprintf() (gregkh)
>
> v3: Enforce format string size limit (ruscur)
> ---
> arch/powerpc/include/asm/secvar.h | 3 +++
> arch/powerpc/kernel/secvar-sysfs.c | 23 ++++--------------
> arch/powerpc/platforms/powernv/opal-secvar.c | 25 ++++++++++++++++++++
> 3 files changed, 33 insertions(+), 18 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/secvar.h b/arch/powerpc/include/asm/secvar.h
> index 07ba36f868a7..8b6475589120 100644
> --- a/arch/powerpc/include/asm/secvar.h
> +++ b/arch/powerpc/include/asm/secvar.h
> @@ -11,12 +11,15 @@
> #include <linux/types.h>
> #include <linux/errno.h>
>
> +#define SECVAR_MAX_FORMAT_LEN 30 // max length of string returned by ->format()
> +
> extern const struct secvar_operations *secvar_ops;
>
> struct secvar_operations {
> int (*get)(const char *key, u64 key_len, u8 *data, u64 *data_size);
> int (*get_next)(const char *key, u64 *key_len, u64 keybufsize);
> int (*set)(const char *key, u64 key_len, u8 *data, u64 data_size);
> + ssize_t (*format)(char *buf);
Maybe pass the buf size as an argument here? Which is a bit less error
prone and more flexible than finding the right #define for it.
Thanks,
Nick
On Wed Jan 18, 2023 at 4:10 PM AEST, Andrew Donnellan wrote:
> From: Russell Currey <[email protected]>
>
> The code that handles the format string in secvar-sysfs.c is entirely
> OPAL specific, so create a new "format" op in secvar_operations to make
> the secvar code more generic. No functional change.
>
> Signed-off-by: Russell Currey <[email protected]>
> Signed-off-by: Andrew Donnellan <[email protected]>
>
> ---
>
> v2: Use sysfs_emit() instead of sprintf() (gregkh)
>
> v3: Enforce format string size limit (ruscur)
> ---
> arch/powerpc/include/asm/secvar.h | 3 +++
> arch/powerpc/kernel/secvar-sysfs.c | 23 ++++--------------
> arch/powerpc/platforms/powernv/opal-secvar.c | 25 ++++++++++++++++++++
> 3 files changed, 33 insertions(+), 18 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/secvar.h b/arch/powerpc/include/asm/secvar.h
> index 07ba36f868a7..8b6475589120 100644
> --- a/arch/powerpc/include/asm/secvar.h
> +++ b/arch/powerpc/include/asm/secvar.h
> @@ -11,12 +11,15 @@
> #include <linux/types.h>
> #include <linux/errno.h>
>
> +#define SECVAR_MAX_FORMAT_LEN 30 // max length of string returned by ->format()
> +
> extern const struct secvar_operations *secvar_ops;
>
> struct secvar_operations {
> int (*get)(const char *key, u64 key_len, u8 *data, u64 *data_size);
> int (*get_next)(const char *key, u64 *key_len, u64 keybufsize);
> int (*set)(const char *key, u64 key_len, u8 *data, u64 data_size);
> + ssize_t (*format)(char *buf);
> };
>
> #ifdef CONFIG_PPC_SECURE_BOOT
> diff --git a/arch/powerpc/kernel/secvar-sysfs.c b/arch/powerpc/kernel/secvar-sysfs.c
> index 462cacc0ca60..d3858eedd72c 100644
> --- a/arch/powerpc/kernel/secvar-sysfs.c
> +++ b/arch/powerpc/kernel/secvar-sysfs.c
> @@ -21,26 +21,13 @@ static struct kset *secvar_kset;
> static ssize_t format_show(struct kobject *kobj, struct kobj_attribute *attr,
> char *buf)
> {
> - ssize_t rc = 0;
> - struct device_node *node;
> - const char *format;
> -
> - node = of_find_compatible_node(NULL, NULL, "ibm,secvar-backend");
> - if (!of_device_is_available(node)) {
> - rc = -ENODEV;
> - goto out;
> - }
> + char tmp[SECVAR_MAX_FORMAT_LEN];
> + ssize_t len = secvar_ops->format(tmp);
>
> - rc = of_property_read_string(node, "format", &format);
> - if (rc)
> - goto out;
> + if (len <= 0)
> + return -EIO;
AFAIKS this does have a functional change, it loses the return value.
Why not return len if it is < 0, and -EIO if len == 0?
Thanks,
Nick
On Thu, 2023-01-19 at 11:17 +1000, Nicholas Piggin wrote:
> On Wed Jan 18, 2023 at 4:10 PM AEST, Andrew Donnellan wrote:
> > From: Russell Currey <[email protected]>
> >
> > The code that handles the format string in secvar-sysfs.c is
> > entirely
> > OPAL specific, so create a new "format" op in secvar_operations to
> > make
> > the secvar code more generic. No functional change.
> >
> > Signed-off-by: Russell Currey <[email protected]>
> > Signed-off-by: Andrew Donnellan <[email protected]>
> >
> > ---
> >
> > v2: Use sysfs_emit() instead of sprintf() (gregkh)
> >
> > v3: Enforce format string size limit (ruscur)
> > ---
> > arch/powerpc/include/asm/secvar.h | 3 +++
> > arch/powerpc/kernel/secvar-sysfs.c | 23 ++++------------
> > --
> > arch/powerpc/platforms/powernv/opal-secvar.c | 25
> > ++++++++++++++++++++
> > 3 files changed, 33 insertions(+), 18 deletions(-)
> >
> > diff --git a/arch/powerpc/include/asm/secvar.h
> > b/arch/powerpc/include/asm/secvar.h
> > index 07ba36f868a7..8b6475589120 100644
> > --- a/arch/powerpc/include/asm/secvar.h
> > +++ b/arch/powerpc/include/asm/secvar.h
> > @@ -11,12 +11,15 @@
> > #include <linux/types.h>
> > #include <linux/errno.h>
> >
> > +#define SECVAR_MAX_FORMAT_LEN 30 // max length of string returned
> > by ->format()
> > +
> > extern const struct secvar_operations *secvar_ops;
> >
> > struct secvar_operations {
> > int (*get)(const char *key, u64 key_len, u8 *data, u64
> > *data_size);
> > int (*get_next)(const char *key, u64 *key_len, u64
> > keybufsize);
> > int (*set)(const char *key, u64 key_len, u8 *data, u64
> > data_size);
> > + ssize_t (*format)(char *buf);
> > };
> >
> > #ifdef CONFIG_PPC_SECURE_BOOT
> > diff --git a/arch/powerpc/kernel/secvar-sysfs.c
> > b/arch/powerpc/kernel/secvar-sysfs.c
> > index 462cacc0ca60..d3858eedd72c 100644
> > --- a/arch/powerpc/kernel/secvar-sysfs.c
> > +++ b/arch/powerpc/kernel/secvar-sysfs.c
> > @@ -21,26 +21,13 @@ static struct kset *secvar_kset;
> > static ssize_t format_show(struct kobject *kobj, struct
> > kobj_attribute *attr,
> > char *buf)
> > {
> > - ssize_t rc = 0;
> > - struct device_node *node;
> > - const char *format;
> > -
> > - node = of_find_compatible_node(NULL, NULL, "ibm,secvar-
> > backend");
> > - if (!of_device_is_available(node)) {
> > - rc = -ENODEV;
> > - goto out;
> > - }
> > + char tmp[SECVAR_MAX_FORMAT_LEN];
> > + ssize_t len = secvar_ops->format(tmp);
> >
> > - rc = of_property_read_string(node, "format", &format);
> > - if (rc)
> > - goto out;
> > + if (len <= 0)
> > + return -EIO;
>
> AFAIKS this does have a functional change, it loses the return value.
> Why not return len if it is < 0, and -EIO if len == 0?
In v2 mpe suggested the following:
I'm not sure you should pass that raw error back to sysfs. Some of
the
values could be confusing, eg. if you return -EINVAL it looks like a
parameter to the read() syscall was invalid. Might be better to just
return -EIO.
Following that advice, I don't think we should return something other
than -EIO, but we should at least pr_err() to document the error - this
isn't something that should ever fail.
>
> Thanks,
> Nick