<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<p><br>
</p>
<br>
<div class="moz-cite-prefix">On 08/21/2019 12:30 PM, Greg
Kroah-Hartman wrote:<br>
</div>
<blockquote type="cite" cite="mid:20190821163018.GA28571@kroah.com">
<pre wrap="">On Wed, Aug 21, 2019 at 11:08:21AM -0400, Nayna Jain wrote:
</pre>
<blockquote type="cite">
<pre wrap="">--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-secvar
@@ -0,0 +1,27 @@
+What: /sys/firmware/secvar
+Date: August 2019
+Contact: Nayna Jain <a class="moz-txt-link-rfc2396E" href="mailto:nayna@linux.ibm.com"><nayna@linux.ibm.com></a>
+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.
</pre>
</blockquote>
<pre wrap="">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...)
</pre>
<blockquote type="cite">
<pre wrap="">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
</pre>
</blockquote>
<pre wrap="">No depends on SYSFS?
</pre>
<blockquote type="cite">
<pre wrap="">+ 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.
</pre>
</blockquote>
<pre wrap="">Mix of tabs and spaces :(
</pre>
<blockquote type="cite">
<pre wrap="">+
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
</pre>
</blockquote>
<pre wrap="">No tab?
</pre>
<blockquote type="cite">
<pre wrap="">
# 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 <a class="moz-txt-link-rfc2396E" href="mailto:nayna@linux.ibm.com"><nayna@linux.ibm.com></a>
+ *
+ * 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.
</pre>
</blockquote>
<pre wrap="">" " before "A" here please.
</pre>
<blockquote type="cite">
<pre wrap="">+#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);
+}
</pre>
</blockquote>
<pre wrap="">Why do you need this entry as it is the directory name? Userspace
already "knows" it if they can open this file.
</pre>
<blockquote type="cite">
<pre wrap="">+
+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);
</pre>
</blockquote>
<pre wrap="">__ATTR_RO()?
</pre>
<blockquote type="cite">
<pre wrap="">+
+static struct kobj_attribute size_attr =
+__ATTR(size, 0444, size_show, NULL);
</pre>
</blockquote>
<pre wrap="">__ATTR_RO()?
</pre>
<blockquote type="cite">
<pre wrap="">+
+static struct bin_attribute data_attr = {
+ .attr = {.name = "data", .mode = 0444},
+ .size = VARIABLE_MAX_SIZE,
+ .read = data_read,
+};
</pre>
</blockquote>
<pre wrap="">__BIN_ATTR_RO()?
</pre>
<blockquote type="cite">
<pre wrap="">+
+
+static struct bin_attribute update_attr = {
+ .attr = {.name = "update", .mode = 0200},
+ .size = VARIABLE_MAX_SIZE,
+ .write = update_write,
+};
</pre>
</blockquote>
<pre wrap="">__BIN_ATTR_RO()?
</pre>
<blockquote type="cite">
<pre wrap="">+
+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,
+};
</pre>
</blockquote>
<pre wrap="">static?
</pre>
<blockquote type="cite">
<pre wrap="">+
+int secvar_sysfs_load(void)
+{
+
+ char *name;
</pre>
</blockquote>
<pre wrap="">No blank line. You didn't run this this through checkpatch, did you :(
</pre>
<blockquote type="cite">
<pre wrap="">+ unsigned long namesize;
+ struct kobject *kobj;
+ int status;
+ int rc = 0;
+
+ name = kzalloc(1024, GFP_KERNEL);
</pre>
</blockquote>
<pre wrap="">Why 1024?
</pre>
<blockquote type="cite">
<pre wrap="">+ 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);
</pre>
</blockquote>
<pre wrap="">Please delete debugging messages.
</pre>
<blockquote type="cite">
<pre wrap="">+ kobj = kobject_create_and_add(name, &(secvar_kset->kobj));
+ if (kobj) {
+ rc = sysfs_create_group(kobj, &secvar_attr_group);
</pre>
</blockquote>
<pre wrap="">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.
</pre>
<blockquote type="cite">
<pre wrap="">+ 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");
</pre>
</blockquote>
<pre wrap="">You juat leaked a kobject :(
</pre>
<blockquote type="cite">
<pre wrap="">+ return -ENODEV;
+ }
+
+ secvarops = get_secvar_ops();
+ if (!secvarops) {
+ kobject_put(powerpc_kobj);
+ pr_err("secvar: failed to retrieve secvar operations.\n");
+ return -ENODEV;
</pre>
</blockquote>
<pre wrap="">You just leaked 2 things from above :(
</pre>
<blockquote type="cite">
<pre wrap="">+ }
+
+ secvar_sysfs_load();
+ pr_info("Secure variables sysfs initialized");
</pre>
</blockquote>
<pre wrap="">Do not be noisy when all goes just fine. The kernel log should be quiet
when all goes well.
</pre>
</blockquote>
<br>
Thanks Greg for feedback. I just posted v3 version with the fixes
suggested by you and Oliver.<br>
<br>
Currently, the name length as 1024 is taken from examples of
efivars. Probably a smaller one is fine.<br>
In v3 version, it is still 1024 but made it #define.<br>
<br>
Thanks & Regards,<br>
- Nayna<br>
<br>
</body>
</html>