In order to verify the OS kernel on PowerNV systems, secure boot requires
X.509 certificates trusted by the platform. These are stored in secure
variables controlled by OPAL, called OPAL secure variables. In order to
enable users to manage the keys, the secure variables need to be exposed
to userspace.
OPAL provides the runtime services for the kernel to be able to access the
secure variables[1]. This patchset defines the kernel interface for the
OPAL APIs. These APIs are used by the hooks, which load these variables
to the keyring and expose them to the userspace for reading/writing.
The previous version[2] of the patchset added support only for the sysfs
interface. This patch adds two more patches that involves loading of
the firmware trusted keys to the kernel keyring. This patchset is
dependent on the base CONFIG PPC_SECURE_BOOT added by ima arch specific
patches for POWER[3]
Overall, this patchset adds the following support:
* expose secure variables to the kernel via OPAL Runtime API interface
* expose secure variables to the userspace via kernel sysfs interface
* load kernel verification and revocation keys to .platform and
.blacklist keyring respectively.
The secure variables can be read/written using simple linux utilities
cat/hexdump.
For example:
Path to the secure variables is:
/sys/firmware/secvar/vars
Each secure variable is listed as directory.
$ ls -l
total 0
drwxr-xr-x. 2 root root 0 Aug 20 21:20 db
drwxr-xr-x. 2 root root 0 Aug 20 21:20 KEK
drwxr-xr-x. 2 root root 0 Aug 20 21:20 PK
The attributes of each of the secure variables are(for example: PK):
[db]$ ls -l
total 0
-r--r--r--. 1 root root 0 Oct 1 15:10 data
-r--r--r--. 1 root root 65536 Oct 1 15:10 size
--w-------. 1 root root 0 Oct 1 15:12 update
The "data" is used to read the existing variable value using hexdump. The
data is stored in ESL format.
The "update" is used to write a new value using cat. The update is
to be submitted as AUTH file.
[1] Depends on skiboot OPAL API changes which removes metadata from
the API. https://lists.ozlabs.org/pipermail/skiboot/2019-September/015203.html.
[2] https://lkml.org/lkml/2019/6/13/1644
[3] https://lkml.org/lkml/2019/9/27/407
Changelog:
v4:
* rebased to v5.4-rc1
* uses __BIN_ATTR_WO macro to create binary attribute as suggested by
Greg
* removed email id from the file header
* renamed argument keysize to keybufsize in get_next() function
* updated default binary file sizes to 0, as firmware handles checking
against the maximum size
* fixed minor formatting issues in Patch 4/4
* added Greg's and Mimi's Reviewed-by and Ack-by
v3:
* includes Greg's feedbacks:
* fixes in Patch 2/4
* updates the Documentation.
* fixes code feedbacks
* adds SYSFS Kconfig dependency for SECVAR_SYSFS
* fixes mixed tabs and spaces
* removes "name" attribute for each of the variable name based
directories
* fixes using __ATTR_RO() and __BIN_ATTR_RO() and statics and const
* fixes the racing issue by using kobj_type default groups. Also,
fixes the kobject leakage.
* removes extra print messages
* updates patch description for Patch 3/4
* removes file name from Patch 4/4 file header comment and removed
def_bool y from the LOAD_PPC_KEYS Kconfig
* includes Oliver's feedbacks:
* fixes Patch 1/2
* moves OPAL API wrappers after opal_nx_proc_init(), fixed the
naming, types and removed extern.
* fixes spaces
* renames get_variable() to get(), get_next_variable() to get_next()
and set_variable() to set()
* removed get_secvar_ops() and defined secvar_ops as global
* fixes consts and statics
* removes generic secvar_init() and defined platform specific
opal_secar_init()
* updates opal_secvar_supported() to check for secvar support even
before checking the OPAL APIs support and also fixed the error codes.
* addes function that converts OPAL return codes to linux errno
* moves secvar check support in the opal_secvar_init() and defined its
prototype in opal.h
* fixes Patch 2/2
* fixes static/const
* defines macro for max name size
* replaces OPAL error codes with linux errno and also updated error
handling
* moves secvar support check before creating sysfs kobjects in
secvar_sysfs_init()
* fixes spaces
v2:
* removes complete efi-sms from the sysfs implementation and is simplified
* includes Greg's and Oliver's feedbacks:
* adds sysfs documentation
* moves sysfs code to arch/powerpc
* other code related feedbacks.
* adds two new patches to load keys to .platform and .blacklist keyring.
These patches are added to this series as they are also dependent on
OPAL APIs.
Nayna Jain (4):
powerpc/powernv: Add OPAL API interface to access secure variable
powerpc: expose secure variables to userspace via sysfs
x86/efi: move common keyring handler functions to new file
powerpc: load firmware trusted keys/hashes into kernel keyring
Documentation/ABI/testing/sysfs-secvar | 37 ++++
arch/powerpc/Kconfig | 10 +
arch/powerpc/include/asm/opal-api.h | 5 +-
arch/powerpc/include/asm/opal.h | 8 +
arch/powerpc/include/asm/powernv.h | 2 +
arch/powerpc/include/asm/secvar.h | 35 ++++
arch/powerpc/kernel/Makefile | 3 +-
arch/powerpc/kernel/secvar-ops.c | 19 ++
arch/powerpc/kernel/secvar-sysfs.c | 198 ++++++++++++++++++
arch/powerpc/platforms/powernv/Kconfig | 6 +
arch/powerpc/platforms/powernv/Makefile | 1 +
arch/powerpc/platforms/powernv/opal-call.c | 3 +
arch/powerpc/platforms/powernv/opal-secvar.c | 137 ++++++++++++
arch/powerpc/platforms/powernv/opal.c | 5 +
security/integrity/Kconfig | 8 +
security/integrity/Makefile | 6 +-
.../platform_certs/keyring_handler.c | 80 +++++++
.../platform_certs/keyring_handler.h | 32 +++
.../integrity/platform_certs/load_powerpc.c | 86 ++++++++
security/integrity/platform_certs/load_uefi.c | 67 +-----
20 files changed, 679 insertions(+), 69 deletions(-)
create mode 100644 Documentation/ABI/testing/sysfs-secvar
create mode 100644 arch/powerpc/include/asm/secvar.h
create mode 100644 arch/powerpc/kernel/secvar-ops.c
create mode 100644 arch/powerpc/kernel/secvar-sysfs.c
create mode 100644 arch/powerpc/platforms/powernv/opal-secvar.c
create mode 100644 security/integrity/platform_certs/keyring_handler.c
create mode 100644 security/integrity/platform_certs/keyring_handler.h
create mode 100644 security/integrity/platform_certs/load_powerpc.c
--
2.20.1
PowerNV secure variables, which store the keys used for OS kernel
verification, are managed by the firmware. These secure variables need to
be accessed by the userspace for addition/deletion of the certificates.
This patch adds the sysfs interface to expose secure variables for PowerNV
secureboot. The users shall use this interface for manipulating
the keys stored in the secure variables.
Signed-off-by: Nayna Jain <[email protected]>
Reviewed-by: Greg Kroah-Hartman <[email protected]>
---
Documentation/ABI/testing/sysfs-secvar | 37 +++++
arch/powerpc/Kconfig | 10 ++
arch/powerpc/kernel/Makefile | 1 +
arch/powerpc/kernel/secvar-sysfs.c | 198 +++++++++++++++++++++++++
4 files changed, 246 insertions(+)
create mode 100644 Documentation/ABI/testing/sysfs-secvar
create mode 100644 arch/powerpc/kernel/secvar-sysfs.c
diff --git a/Documentation/ABI/testing/sysfs-secvar b/Documentation/ABI/testing/sysfs-secvar
new file mode 100644
index 000000000000..815bd8ec4d5e
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-secvar
@@ -0,0 +1,37 @@
+What: /sys/firmware/secvar
+Date: August 2019
+Contact: Nayna Jain <[email protected]>
+Description: This directory is created if the POWER firmware supports OS
+ secureboot, thereby secure variables. It exposes interface
+ for reading/writing the secure variables
+
+What: /sys/firmware/secvar/vars
+Date: August 2019
+Contact: Nayna Jain <[email protected]>
+Description: This directory lists all the secure variables that are supported
+ by the firmware.
+
+What: /sys/firmware/secvar/vars/<variable name>
+Date: August 2019
+Contact: Nayna Jain <[email protected]>
+Description: Each secure variable is represented as a directory named as
+ <variable_name>. The variable name is unique and is in ASCII
+ representation. The data and size can be determined by reading
+ their respective attribute files.
+
+What: /sys/firmware/secvar/vars/<variable_name>/size
+Date: August 2019
+Contact: Nayna Jain <[email protected]>
+Description: An integer representation of the size of the content of the
+ variable. In other words, it represents the size of the data.
+
+What: /sys/firmware/secvar/vars/<variable_name>/data
+Date: August 2019
+Contact: Nayna Jain <[email protected]>
+Description: A read-only file containing the value of the variable
+
+What: /sys/firmware/secvar/vars/<variable_name>/update
+Date: August 2019
+Contact: Nayna Jain <[email protected]>
+Description: A write-only file that is used to submit the new value for the
+ variable.
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index deb19ec6ba3d..89084e4e5054 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -946,6 +946,16 @@ config PPC_SECURE_BOOT
to enable OS secure boot on systems that have firmware support for
it. If in doubt say N.
+config SECVAR_SYSFS
+ tristate "Enable sysfs interface for POWER secure variables"
+ depends on PPC_SECURE_BOOT
+ depends on SYSFS
+ help
+ POWER secure variables are managed and controlled by firmware.
+ These variables are exposed to userspace via sysfs to enable
+ read/write operations on these variables. Say Y if you have
+ secure boot enabled and want to expose variables to userspace.
+
endmenu
config ISA_DMA_API
diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
index 3cf26427334f..116a3a5c0557 100644
--- a/arch/powerpc/kernel/Makefile
+++ b/arch/powerpc/kernel/Makefile
@@ -162,6 +162,7 @@ obj-y += ucall.o
endif
obj-$(CONFIG_PPC_SECURE_BOOT) += secure_boot.o ima_arch.o secvar-ops.o
+obj-$(CONFIG_SECVAR_SYSFS) += secvar-sysfs.o
# Disable GCOV, KCOV & sanitizers in odd or sensitive code
GCOV_PROFILE_prom_init.o := n
diff --git a/arch/powerpc/kernel/secvar-sysfs.c b/arch/powerpc/kernel/secvar-sysfs.c
new file mode 100644
index 000000000000..87a7cea41523
--- /dev/null
+++ b/arch/powerpc/kernel/secvar-sysfs.c
@@ -0,0 +1,198 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2019 IBM Corporation <[email protected]>
+ *
+ * This code exposes secure variables to user via sysfs
+ */
+
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/compat.h>
+#include <linux/string.h>
+#include <asm/secvar.h>
+
+/*
+ * Since firmware checks the maximum allowed size, currently, it is default to
+ * 0. In future, it will be read from the device tree.
+ */
+#define VARIABLE_MAX_SIZE 0
+/* Approximate value */
+#define NAME_MAX_SIZE 1024
+
+static struct kobject *secvar_kobj;
+static struct kset *secvar_kset;
+
+static ssize_t size_show(struct kobject *kobj, struct kobj_attribute *attr,
+ char *buf)
+{
+ uint64_t dsize;
+ int rc;
+
+ rc = secvar_ops->get(kobj->name, strlen(kobj->name) + 1, NULL, &dsize);
+ if (rc) {
+ pr_err("Error retrieving variable size %d\n", rc);
+ return rc;
+ }
+
+ rc = sprintf(buf, "%llu\n", dsize);
+
+ return rc;
+}
+
+static ssize_t data_read(struct file *filep, struct kobject *kobj,
+ struct bin_attribute *attr, char *buf, loff_t off,
+ size_t count)
+{
+ uint64_t dsize;
+ int rc;
+ char *data;
+
+ rc = secvar_ops->get(kobj->name, strlen(kobj->name) + 1, NULL, &dsize);
+ if (rc) {
+ pr_err("Error getting variable size %d\n", rc);
+ return rc;
+ }
+ pr_debug("dsize is %llu\n", dsize);
+
+ data = kzalloc(dsize, GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+
+ rc = secvar_ops->get(kobj->name, strlen(kobj->name)+1, data, &dsize);
+ if (rc) {
+ pr_err("Error getting variable %d\n", rc);
+ goto data_fail;
+ }
+
+ rc = memory_read_from_buffer(buf, count, &off, data, dsize);
+
+data_fail:
+ kfree(data);
+ return rc;
+}
+
+static ssize_t update_write(struct file *filep, struct kobject *kobj,
+ struct bin_attribute *attr, char *buf, loff_t off,
+ size_t count)
+{
+ int rc;
+
+ pr_debug("count is %ld\n", count);
+ rc = secvar_ops->set(kobj->name, strlen(kobj->name)+1, buf, count);
+ if (rc) {
+ pr_err("Error setting the variable %s\n", kobj->name);
+ return rc;
+ }
+
+ return count;
+}
+
+static struct kobj_attribute size_attr = __ATTR_RO(size);
+
+static struct bin_attribute data_attr = __BIN_ATTR_RO(data, VARIABLE_MAX_SIZE);
+
+static struct bin_attribute update_attr = __BIN_ATTR_WO(update,
+ VARIABLE_MAX_SIZE);
+
+static struct bin_attribute *secvar_bin_attrs[] = {
+ &data_attr,
+ &update_attr,
+ NULL,
+};
+
+static struct attribute *secvar_attrs[] = {
+ &size_attr.attr,
+ NULL,
+};
+
+static const struct attribute_group secvar_attr_group = {
+ .attrs = secvar_attrs,
+ .bin_attrs = secvar_bin_attrs,
+};
+__ATTRIBUTE_GROUPS(secvar_attr);
+
+static struct kobj_type secvar_ktype = {
+ .sysfs_ops = &kobj_sysfs_ops,
+ .default_groups = secvar_attr_groups,
+};
+
+static int secvar_sysfs_load(void)
+{
+ char *name;
+ uint64_t namesize = 0;
+ struct kobject *kobj;
+ int rc;
+
+ name = kzalloc(NAME_MAX_SIZE, GFP_KERNEL);
+ if (!name)
+ return -ENOMEM;
+
+ do {
+ rc = secvar_ops->get_next(name, &namesize, NAME_MAX_SIZE);
+ if (rc) {
+ if (rc != -ENOENT)
+ pr_err("error getting secvar from firmware %d\n",
+ rc);
+ break;
+ }
+
+ kobj = kzalloc(sizeof(*kobj), GFP_KERNEL);
+ if (!kobj)
+ return -ENOMEM;
+
+ kobject_init(kobj, &secvar_ktype);
+
+ rc = kobject_add(kobj, &secvar_kset->kobj, "%s", name);
+ if (rc) {
+ pr_warn("kobject_add error %d for attribute: %s\n", rc,
+ name);
+ kobject_put(kobj);
+ kobj = NULL;
+ }
+
+ if (kobj)
+ kobject_uevent(kobj, KOBJ_ADD);
+
+ } while (!rc);
+
+ kfree(name);
+ return rc;
+}
+
+static int secvar_sysfs_init(void)
+{
+ if (!secvar_ops) {
+ pr_warn("secvar: failed to retrieve secvar operations.\n");
+ return -ENODEV;
+ }
+
+ secvar_kobj = kobject_create_and_add("secvar", firmware_kobj);
+ if (!secvar_kobj) {
+ pr_err("secvar: Failed to create firmware kobj\n");
+ return -ENOMEM;
+ }
+
+ secvar_kset = kset_create_and_add("vars", NULL, secvar_kobj);
+ if (!secvar_kset) {
+ pr_err("secvar: sysfs kobject registration failed.\n");
+ kobject_put(secvar_kobj);
+ return -ENOMEM;
+ }
+
+ secvar_sysfs_load();
+
+ return 0;
+}
+
+static void secvar_sysfs_exit(void)
+{
+ kset_unregister(secvar_kset);
+ kobject_put(secvar_kobj);
+}
+
+module_init(secvar_sysfs_init);
+module_exit(secvar_sysfs_exit);
+
+MODULE_AUTHOR("Nayna Jain <[email protected]>");
+MODULE_DESCRIPTION("sysfs interface to POWER secure variables");
+MODULE_LICENSE("GPL");
--
2.20.1
The keys used to verify the Host OS kernel are managed by firmware as
secure variables. This patch loads the verification keys into the .platform
keyring and revocation hashes into .blacklist keyring. This enables
verification and loading of the kernels signed by the boot time keys which
are trusted by firmware.
Signed-off-by: Nayna Jain <[email protected]>
Reviewed-by: Mimi Zohar <[email protected]>
---
security/integrity/Kconfig | 8 ++
security/integrity/Makefile | 3 +
.../integrity/platform_certs/load_powerpc.c | 86 +++++++++++++++++++
3 files changed, 97 insertions(+)
create mode 100644 security/integrity/platform_certs/load_powerpc.c
diff --git a/security/integrity/Kconfig b/security/integrity/Kconfig
index 0bae6adb63a9..26abee23e4e3 100644
--- a/security/integrity/Kconfig
+++ b/security/integrity/Kconfig
@@ -72,6 +72,14 @@ config LOAD_IPL_KEYS
depends on S390
def_bool y
+config LOAD_PPC_KEYS
+ bool "Enable loading of platform and blacklisted keys for POWER"
+ depends on INTEGRITY_PLATFORM_KEYRING
+ depends on PPC_SECURE_BOOT
+ help
+ Enable loading of keys to the .platform keyring and blacklisted
+ hashes to the .blacklist keyring for powerpc based platforms.
+
config INTEGRITY_AUDIT
bool "Enables integrity auditing support "
depends on AUDIT
diff --git a/security/integrity/Makefile b/security/integrity/Makefile
index 525bf1d6e0db..9eeb6b053de3 100644
--- a/security/integrity/Makefile
+++ b/security/integrity/Makefile
@@ -14,6 +14,9 @@ integrity-$(CONFIG_LOAD_UEFI_KEYS) += platform_certs/efi_parser.o \
platform_certs/load_uefi.o \
platform_certs/keyring_handler.o
integrity-$(CONFIG_LOAD_IPL_KEYS) += platform_certs/load_ipl_s390.o
+integrity-$(CONFIG_LOAD_PPC_KEYS) += platform_certs/efi_parser.o \
+ platform_certs/load_powerpc.o \
+ platform_certs/keyring_handler.o
$(obj)/load_uefi.o: KBUILD_CFLAGS += -fshort-wchar
subdir-$(CONFIG_IMA) += ima
diff --git a/security/integrity/platform_certs/load_powerpc.c b/security/integrity/platform_certs/load_powerpc.c
new file mode 100644
index 000000000000..83d99cde5376
--- /dev/null
+++ b/security/integrity/platform_certs/load_powerpc.c
@@ -0,0 +1,86 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2019 IBM Corporation
+ * Author: Nayna Jain
+ *
+ * - loads keys and hashes stored and controlled by the firmware.
+ */
+#include <linux/kernel.h>
+#include <linux/sched.h>
+#include <linux/cred.h>
+#include <linux/err.h>
+#include <linux/slab.h>
+#include <asm/secure_boot.h>
+#include <asm/secvar.h>
+#include "keyring_handler.h"
+
+/*
+ * Get a certificate list blob from the named secure variable.
+ */
+static __init void *get_cert_list(u8 *key, unsigned long keylen, uint64_t *size)
+{
+ int rc;
+ void *db;
+
+ rc = secvar_ops->get(key, keylen, NULL, size);
+ if (rc) {
+ pr_err("Couldn't get size: %d\n", rc);
+ return NULL;
+ }
+
+ db = kmalloc(*size, GFP_KERNEL);
+ if (!db)
+ return NULL;
+
+ rc = secvar_ops->get(key, keylen, db, size);
+ if (rc) {
+ kfree(db);
+ pr_err("Error reading db var: %d\n", rc);
+ return NULL;
+ }
+
+ return db;
+}
+
+/*
+ * Load the certs contained in the keys databases into the platform trusted
+ * keyring and the blacklisted X.509 cert SHA256 hashes into the blacklist
+ * keyring.
+ */
+static int __init load_powerpc_certs(void)
+{
+ void *db = NULL, *dbx = NULL;
+ uint64_t dbsize = 0, dbxsize = 0;
+ int rc = 0;
+
+ if (!secvar_ops)
+ return -ENODEV;
+
+ /* Get db, and dbx. They might not exist, so it isn't
+ * an error if we can't get them.
+ */
+ db = get_cert_list("db", 3, &dbsize);
+ if (!db) {
+ pr_err("Couldn't get db list from firmware\n");
+ } else {
+ rc = parse_efi_signature_list("powerpc:db", db, dbsize,
+ get_handler_for_db);
+ if (rc)
+ pr_err("Couldn't parse db signatures: %d\n", rc);
+ kfree(db);
+ }
+
+ dbx = get_cert_list("dbx", 3, &dbxsize);
+ if (!dbx) {
+ pr_info("Couldn't get dbx list from firmware\n");
+ } else {
+ rc = parse_efi_signature_list("powerpc:dbx", dbx, dbxsize,
+ get_handler_for_dbx);
+ if (rc)
+ pr_err("Couldn't parse dbx signatures: %d\n", rc);
+ kfree(dbx);
+ }
+
+ return rc;
+}
+late_initcall(load_powerpc_certs);
--
2.20.1
On Tue, 2019-10-01 at 19:41 -0400, Nayna Jain wrote:
> PowerNV secure variables, which store the keys used for OS kernel
> verification, are managed by the firmware. These secure variables need to
> be accessed by the userspace for addition/deletion of the certificates.
>
> This patch adds the sysfs interface to expose secure variables for PowerNV
> secureboot. The users shall use this interface for manipulating
> the keys stored in the secure variables.
>
> Signed-off-by: Nayna Jain <[email protected]>
> Reviewed-by: Greg Kroah-Hartman <[email protected]>
> ---
> Documentation/ABI/testing/sysfs-secvar | 37 +++++
> arch/powerpc/Kconfig | 10 ++
> arch/powerpc/kernel/Makefile | 1 +
> arch/powerpc/kernel/secvar-sysfs.c | 198 +++++++++++++++++++++++++
> 4 files changed, 246 insertions(+)
> create mode 100644 Documentation/ABI/testing/sysfs-secvar
> create mode 100644 arch/powerpc/kernel/secvar-sysfs.c
>
> diff --git a/Documentation/ABI/testing/sysfs-secvar b/Documentation/ABI/testing/sysfs-secvar
> new file mode 100644
> index 000000000000..815bd8ec4d5e
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-secvar
> @@ -0,0 +1,37 @@
> +What: /sys/firmware/secvar
> +Date: August 2019
> +Contact: Nayna Jain <[email protected]>
> +Description: This directory is created if the POWER firmware supports OS
> + secureboot, thereby secure variables. It exposes interface
> + for reading/writing the secure variables
> +
> +What: /sys/firmware/secvar/vars
> +Date: August 2019
> +Contact: Nayna Jain <[email protected]>
> +Description: This directory lists all the secure variables that are supported
> + by the firmware.
> +
> +What: /sys/firmware/secvar/vars/<variable name>
> +Date: August 2019
> +Contact: Nayna Jain <[email protected]>
> +Description: Each secure variable is represented as a directory named as
> + <variable_name>. The variable name is unique and is in ASCII
> + representation. The data and size can be determined by reading
> + their respective attribute files.
> +
> +What: /sys/firmware/secvar/vars/<variable_name>/size
> +Date: August 2019
> +Contact: Nayna Jain <[email protected]>
> +Description: An integer representation of the size of the content of the
> + variable. In other words, it represents the size of the data.
> +
> +What: /sys/firmware/secvar/vars/<variable_name>/data
> +Date: August 2019
> +Contact: Nayna Jain <[email protected]>
> +Description: A read-only file containing the value of the variable
> +
> +What: /sys/firmware/secvar/vars/<variable_name>/update
> +Date: August 2019
> +Contact: Nayna Jain <[email protected]>
> +Description: A write-only file that is used to submit the new value for the
> + variable.
How are the update mechanism's weird requirements communicated to
userspace? The design you've got for the OPAL bits is that the update
requirements depends on the secvar backend, but I see nothing plumbing
through what the secvar backend actually is.
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index deb19ec6ba3d..89084e4e5054 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -946,6 +946,16 @@ config PPC_SECURE_BOOT
> to enable OS secure boot on systems that have firmware support for
> it. If in doubt say N.
>
> +config SECVAR_SYSFS
that should probably be PPC_SECVAR_SYSFS since it's PPC specific
> + tristate "Enable sysfs interface for POWER secure variables"
> + depends on PPC_SECURE_BOOT
> + depends on SYSFS
> + help
> + POWER secure variables are managed and controlled by firmware.
> + These variables are exposed to userspace via sysfs to enable
> + read/write operations on these variables. Say Y if you have
> + secure boot enabled and want to expose variables to userspace.
> +
> endmenu
>
> config ISA_DMA_API
> diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
> index 3cf26427334f..116a3a5c0557 100644
> --- a/arch/powerpc/kernel/Makefile
> +++ b/arch/powerpc/kernel/Makefile
> @@ -162,6 +162,7 @@ obj-y += ucall.o
> endif
>
> obj-$(CONFIG_PPC_SECURE_BOOT) += secure_boot.o ima_arch.o secvar-ops.o
> +obj-$(CONFIG_SECVAR_SYSFS) += secvar-sysfs.o
>
> # Disable GCOV, KCOV & sanitizers in odd or sensitive code
> GCOV_PROFILE_prom_init.o := n
> diff --git a/arch/powerpc/kernel/secvar-sysfs.c b/arch/powerpc/kernel/secvar-sysfs.c
> new file mode 100644
> index 000000000000..87a7cea41523
> --- /dev/null
> +++ b/arch/powerpc/kernel/secvar-sysfs.c
> @@ -0,0 +1,198 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2019 IBM Corporation <[email protected]>
> + *
> + * This code exposes secure variables to user via sysfs
> + */
Adding a pr_fmt for this file would be a good idea. There's a few
pr_err()s in here that would benefit from some context.
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/compat.h>
> +#include <linux/string.h>
> +#include <asm/secvar.h>
> +/*
> + * Since firmware checks the maximum allowed size, currently, it is default to
> + * 0. In future, it will be read from the device tree.
> + */
> +#define VARIABLE_MAX_SIZE 0
I don't see why you aren't reading it from the DT now...
> +/* Approximate value */
> +#define NAME_MAX_SIZE 1024
Approximate?
> +static struct kobject *secvar_kobj;
> +static struct kset *secvar_kset;
> +
> +static ssize_t size_show(struct kobject *kobj, struct kobj_attribute *attr,
> + char *buf)
> +{
> + uint64_t dsize;
> + int rc;
> +
> + rc = secvar_ops->get(kobj->name, strlen(kobj->name) + 1, NULL, &dsize);
> + if (rc) {
> + pr_err("Error retrieving variable size %d\n", rc);
> + return rc;
> + }
> +
> + rc = sprintf(buf, "%llu\n", dsize);
> +
> + return rc;
> +}
> +
> +static ssize_t data_read(struct file *filep, struct kobject *kobj,
> + struct bin_attribute *attr, char *buf, loff_t off,
> + size_t count)
> +{
> + uint64_t dsize;
> + int rc;
> + char *data;
Can you swap the declarations of rc and data.
We try to keep declarations in reverse christmas tree style in
arch/powerpc/. We're pretty bad at enforcing that, but there's no
reason to be gratuitiously different.
> +
> + rc = secvar_ops->get(kobj->name, strlen(kobj->name) + 1, NULL, &dsize);
> + if (rc) {
> + pr_err("Error getting variable size %d\n", rc);
> + return rc;
> + }
> + pr_debug("dsize is %llu\n", dsize);
> +
> + data = kzalloc(dsize, GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + rc = secvar_ops->get(kobj->name, strlen(kobj->name)+1, data, &dsize);
> + if (rc) {
> + pr_err("Error getting variable %d\n", rc);
> + goto data_fail;
> + }
> +
> + rc = memory_read_from_buffer(buf, count, &off, data, dsize);
> +
> +data_fail:
> + kfree(data);
> + return rc;
> +}
> +
> +static ssize_t update_write(struct file *filep, struct kobject *kobj,
> + struct bin_attribute *attr, char *buf, loff_t off,
> + size_t count)
> +{
> + int rc;
> +
> + pr_debug("count is %ld\n", count);
> + rc = secvar_ops->set(kobj->name, strlen(kobj->name)+1, buf, count);
> + if (rc) {
> + pr_err("Error setting the variable %s\n", kobj->name);
> + return rc;
> + }
> +
> + return count;
> +}
> +
> +static struct kobj_attribute size_attr = __ATTR_RO(size);
> +
> +static struct bin_attribute data_attr = __BIN_ATTR_RO(data, VARIABLE_MAX_SIZE);
> +
> +static struct bin_attribute update_attr = __BIN_ATTR_WO(update,
> + VARIABLE_MAX_SIZE);
Isn't this going to be all wrong if VARIABLE_MAX_SIZE is ever non-zero?
> +
> +static struct bin_attribute *secvar_bin_attrs[] = {
> + &data_attr,
> + &update_attr,
> + NULL,
> +};
> +
> +static struct attribute *secvar_attrs[] = {
> + &size_attr.attr,
> + NULL,
> +};
> +
> +static const struct attribute_group secvar_attr_group = {
> + .attrs = secvar_attrs,
> + .bin_attrs = secvar_bin_attrs,
> +};
> +__ATTRIBUTE_GROUPS(secvar_attr);
> +
> +static struct kobj_type secvar_ktype = {
> + .sysfs_ops = &kobj_sysfs_ops,
> + .default_groups = secvar_attr_groups,
> +};
> +
> +static int secvar_sysfs_load(void)
> +{
> + char *name;
> + uint64_t namesize = 0;
> + struct kobject *kobj;
> + int rc;
> +
> + name = kzalloc(NAME_MAX_SIZE, GFP_KERNEL);
> + if (!name)
> + return -ENOMEM;
> +
> + do {
> + rc = secvar_ops->get_next(name, &namesize, NAME_MAX_SIZE);
> + if (rc) {
> + if (rc != -ENOENT)
> + pr_err("error getting secvar from firmware %d\n",
> + rc);
> + break;
> + }
> +
> + kobj = kzalloc(sizeof(*kobj), GFP_KERNEL);
> + if (!kobj)
> + return -ENOMEM;
> +
> + kobject_init(kobj, &secvar_ktype);
> +
> + rc = kobject_add(kobj, &secvar_kset->kobj, "%s", name);
> + if (rc) {
> + pr_warn("kobject_add error %d for attribute: %s\n", rc,
> + name);
> + kobject_put(kobj);
> + kobj = NULL;
> + }
> +
> + if (kobj)
> + kobject_uevent(kobj, KOBJ_ADD);
> +
> + } while (!rc);
> +
> + kfree(name);
> + return rc;
> +}
> +
> +static int secvar_sysfs_init(void)
> +{
> + if (!secvar_ops) {
> + pr_warn("secvar: failed to retrieve secvar operations.\n");
> + return -ENODEV;
> + }
> +
> + secvar_kobj = kobject_create_and_add("secvar", firmware_kobj);
> + if (!secvar_kobj) {
> + pr_err("secvar: Failed to create firmware kobj\n");
> + return -ENOMEM;
> + }
> +
> + secvar_kset = kset_create_and_add("vars", NULL, secvar_kobj);
> + if (!secvar_kset) {
> + pr_err("secvar: sysfs kobject registration failed.\n");
> + kobject_put(secvar_kobj);
> + return -ENOMEM;
> + }
> +
> + secvar_sysfs_load();
> +
> + return 0;
> +}
> +
> +static void secvar_sysfs_exit(void)
> +{
> + kset_unregister(secvar_kset);
> + kobject_put(secvar_kobj);
> +}
> +
> +module_init(secvar_sysfs_init);
> +module_exit(secvar_sysfs_exit);
> +
> +MODULE_AUTHOR("Nayna Jain <[email protected]>");
> +MODULE_DESCRIPTION("sysfs interface to POWER secure variables");
> +MODULE_LICENSE("GPL");
Is there anything that'll force the module to be loaded at runtime?
If not it might be worth making this builtin and turning the OPAL API
bit into a platform device driver. We can instantiate a platform device
from the DT node during opal_init() and the modalias based module
loading should handle the rest for you.
I would like to get people using platform device drivers for random
OPAL provided stuff. All the ~artisinal~hand~crafted~ device-tree
parsing in the powernv platform is getting a bit ridiculous...
Oliver