On Wed, Aug 21, 2019 at 11:08:21AM -0400, Nayna Jain wrote:
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-secvar
@@ -0,0 +1,27 @@
+What: /sys/firmware/secvar
+Date: August 2019
+Contact: Nayna Jain<na...@linux.ibm.com>
+Description:
+ This directory exposes interfaces for interacting with
+ the secure variables managed by OPAL firmware.
+
+ This is only for the powerpc/powernv platform.
+
+ Directory:
+ vars: This directory lists all the variables that
+ are supported by the OPAL. The variables are
+ represented in the form of directories with
+ their variable names. The variable name is
+ unique and is in ASCII representation. The data
+ and size can be determined by reading their
+ respective attribute files.
+
+ Each variable directory has the following files:
+ name: An ASCII representation of the variable name
+ data: A read-only file containing the value of the
+ variable
+ size: An integer representation of the size of the
+ content of the variable. In other works, it
+ represents the size of the data
+ update: A write-only file that is used to submit the new
+ value for the variable.
Can you break this out into one-entry-per-file like most other entries
are defined? That makes it easier for tools to parse (specifically the
tool in the tree right now...)
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 42109682b727..b4bdf77837b2 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -925,6 +925,15 @@ config PPC_SECURE_BOOT
allows user to enable OS Secure Boot on PowerPC systems that
have firmware secure boot support.
+config SECVAR_SYSFS
+ tristate "Enable sysfs interface for POWER secure variables"
+ depends on PPC_SECURE_BOOT
No 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.
Mix of tabs and spaces :(
+
endmenu
config ISA_DMA_API
diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
index 9041563f1c74..4ea7b738c3a3 100644
--- a/arch/powerpc/kernel/Makefile
+++ b/arch/powerpc/kernel/Makefile
@@ -158,6 +158,7 @@ obj-$(CONFIG_EPAPR_PARAVIRT) += epapr_paravirt.o
epapr_hcalls.o
obj-$(CONFIG_KVM_GUEST) += kvm.o kvm_emul.o
obj-$(CONFIG_PPC_SECURE_BOOT) += secboot.o ima_arch.o secvar-ops.o
+obj-$(CONFIG_SECVAR_SYSFS) += secvar-sysfs.o
No tab?
# 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..e46986bb29a0
--- /dev/null
+++ b/arch/powerpc/kernel/secvar-sysfs.c
@@ -0,0 +1,210 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2019 IBM Corporation<na...@linux.ibm.com>
+ *
+ * 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/opal.h>
+#include <asm/secvar.h>
+
+//Approximating it for now, it is bound to change.
" " before "A" here please.
+#define VARIABLE_MAX_SIZE 32000
+
+static struct kobject *powerpc_kobj;
+static struct secvar_operations *secvarops;
+struct kset *secvar_kset;
+
+static ssize_t name_show(struct kobject *kobj, struct kobj_attribute *attr,
+ char *buf)
+{
+ return sprintf(buf, "%s", kobj->name);
+}
Why do you need this entry as it is the directory name? Userspace
already "knows" it if they can open this file.
+
+static ssize_t size_show(struct kobject *kobj, struct kobj_attribute *attr,
+ char *buf)
+{
+ unsigned long dsize;
+ int rc;
+
+ rc = secvarops->get_variable(kobj->name, strlen(kobj->name) + 1, NULL,
+ &dsize);
+ if (rc) {
+ pr_err("Error retrieving variable size %d\n", rc);
+ return rc;
+ }
+
+ rc = sprintf(buf, "%ld", 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)
+{
+ unsigned long dsize;
+ int rc;
+ char *data;
+
+ rc = secvarops->get_variable(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 %ld\n", dsize);
+
+ data = kzalloc(dsize, GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+
+ rc = secvarops->get_variable(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 = secvarops->set_variable(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 name_attr =
+__ATTR(name, 0444, name_show, NULL);
__ATTR_RO()?
+
+static struct kobj_attribute size_attr =
+__ATTR(size, 0444, size_show, NULL);
__ATTR_RO()?
+
+static struct bin_attribute data_attr = {
+ .attr = {.name = "data", .mode = 0444},
+ .size = VARIABLE_MAX_SIZE,
+ .read = data_read,
+};
__BIN_ATTR_RO()?
+
+
+static struct bin_attribute update_attr = {
+ .attr = {.name = "update", .mode = 0200},
+ .size = VARIABLE_MAX_SIZE,
+ .write = update_write,
+};
__BIN_ATTR_RO()?
+
+static struct bin_attribute *secvar_bin_attrs[] = {
+ &data_attr,
+ &update_attr,
+ NULL,
+};
+
+static struct attribute *secvar_attrs[] = {
+ &name_attr.attr,
+ &size_attr.attr,
+ NULL,
+};
+
+const struct attribute_group secvar_attr_group = {
+ .attrs = secvar_attrs,
+ .bin_attrs = secvar_bin_attrs,
+};
static?
+
+int secvar_sysfs_load(void)
+{
+
+ char *name;
No blank line. You didn't run this this through checkpatch, did you :(
+ unsigned long namesize;
+ struct kobject *kobj;
+ int status;
+ int rc = 0;
+
+ name = kzalloc(1024, GFP_KERNEL);
Why 1024?
+ if (!name)
+ return -ENOMEM;
+
+ do {
+
+ status = secvarops->get_next_variable(name, &namesize, 1024);
+ if (status != OPAL_SUCCESS)
+ break;
+
+ pr_info("name is %s\n", name);
Please delete debugging messages.
+ kobj = kobject_create_and_add(name, &(secvar_kset->kobj));
+ if (kobj) {
+ rc = sysfs_create_group(kobj, &secvar_attr_group);
You just raced userspace and lost :(
If you set your kobj_type to have the attribute group you will not race
and loose, the core will handle it for you.
+ if (rc)
+ pr_err("Error creating attributes for %s
variable\n",
+ name);
+ } else {
+ pr_err("Error creating sysfs entry for %s variable\n",
+ name);
+ rc = -EINVAL;
+ }
+
+ } while ((status == OPAL_SUCCESS) && (rc == 0));
+
+ kfree(name);
+ return rc;
+}
+
+int secvar_sysfs_init(void)
+{
+ powerpc_kobj = kobject_create_and_add("secvar", firmware_kobj);
+ if (!powerpc_kobj) {
+ pr_err("secvar: Failed to create firmware kobj\n");
+ return -ENODEV;
+ }
+
+ secvar_kset = kset_create_and_add("vars", NULL, powerpc_kobj);
+ if (!secvar_kset) {
+ pr_err("secvar: sysfs kobject registration failed.\n");
You juat leaked a kobject :(
+ return -ENODEV;
+ }
+
+ secvarops = get_secvar_ops();
+ if (!secvarops) {
+ kobject_put(powerpc_kobj);
+ pr_err("secvar: failed to retrieve secvar operations.\n");
+ return -ENODEV;
You just leaked 2 things from above :(
+ }
+
+ secvar_sysfs_load();
+ pr_info("Secure variables sysfs initialized");
Do not be noisy when all goes just fine. The kernel log should be quiet
when all goes well.
