[PATCH v2 2/4] powerpc: expose secure variables to userspace via sysfs
Nayna
nayna at linux.vnet.ibm.com
Mon Aug 26 23:25:25 AEST 2019
On 08/21/2019 12:30 PM, Greg Kroah-Hartman wrote:
> 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<nayna at 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<nayna at 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.
>
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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/linuxppc-dev/attachments/20190826/fff9377d/attachment-0001.htm>
More information about the Linuxppc-dev
mailing list