On 08/21/2019 12:30 PM, Greg Kroah-Hartman wrote:

On Wed, Aug 21, 2019 at 11:08:21AM -0400, Nayna Jain wrote:

quoted

--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-secvar

@@ -0,0 +1,27 @@
+What:		/sys/firmware/secvar
+Date:		August 2019
+Contact:	Nayna Jain<nayna@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...)

quoted

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?

quoted

+        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 :(

quoted

+
  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?

quoted

  
  # 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<nayna@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.

quoted

+#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.

quoted

+
+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()?

quoted

+
+static struct kobj_attribute size_attr =
+__ATTR(size, 0444, size_show, NULL);

__ATTR_RO()?

quoted

+
+static struct bin_attribute data_attr = {
+	.attr = {.name = "data", .mode = 0444},
+	.size = VARIABLE_MAX_SIZE,
+	.read = data_read,
+};

__BIN_ATTR_RO()?

quoted

+
+
+static struct bin_attribute update_attr = {
+	.attr = {.name = "update", .mode = 0200},
+	.size = VARIABLE_MAX_SIZE,
+	.write = update_write,
+};

__BIN_ATTR_RO()?

quoted

+
+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?

quoted

+
+int secvar_sysfs_load(void)
+{
+
+	char *name;

No blank line.  You didn't run this this through checkpatch, did you :(

quoted

+	unsigned long namesize;
+	struct kobject *kobj;
+	int status;
+	int rc = 0;
+
+	name = kzalloc(1024, GFP_KERNEL);

Why 1024?

quoted

+	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.

quoted

+		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.

quoted

+			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 :(

quoted

+		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 :(

quoted

+	}
+
+	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.

Thanks Greg for feedback. I just posted v3 version with the fixes 
suggested by you and Oliver.

Currently, the name length as 1024 is taken from examples of efivars. 
Probably a smaller one is fine.
In v3 version, it is still 1024 but made it #define.

Thanks & Regards,
       - Nayna