linuxppc-dev.lists.ozlabs.org archive mirror
 help / color / mirror / Atom feed
From: Nayna <nayna@linux.vnet.ibm.com>
To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-efi@vger.kernel.org,
	Ard Biesheuvel <ard.biesheuvel@linaro.org>,
	Eric Ricther <erichte@linux.ibm.com>,
	Nayna Jain <nayna@linux.ibm.com>,
	linux-kernel@vger.kernel.org, Mimi Zohar <zohar@linux.ibm.com>,
	Claudio Carvalho <cclaudio@linux.ibm.com>,
	Matthew Garret <matthew.garret@nebula.com>,
	linuxppc-dev@ozlabs.org, Paul Mackerras <paulus@samba.org>,
	Jeremy Kerr <jk@ozlabs.org>, Elaine Palmer <erpalmer@us.ibm.com>,
	linux-integrity@vger.kernel.org,
	George Wilson <gcwilson@linux.ibm.com>
Subject: Re: [PATCH 2/2] powerpc: expose secure variables via sysfs
Date: Fri, 14 Jun 2019 09:13:24 -0400	[thread overview]
Message-ID: <2f4a3ab4-8e9c-a8fc-ba1a-71cf6a9fbb59@linux.vnet.ibm.com> (raw)
In-Reply-To: <20190614063443.GB17056@kroah.com>



On 06/14/2019 02:34 AM, Greg Kroah-Hartman wrote:
> On Thu, Jun 13, 2019 at 04:50:27PM -0400, Nayna Jain wrote:
>> As part of PowerNV secure boot support, OS verification keys are stored
>> and controlled by OPAL as secure variables. These need to be exposed to
>> the userspace so that sysadmins can perform key management tasks.
>>
>> This patch adds the support to expose secure variables via a sysfs
>> interface It reuses the the existing efi defined hooks and backend in
>> order to maintain the compatibility with the userspace tools.
>>
>> Though it reuses a great deal of efi, POWER platforms do not use EFI.
>> A new config, POWER_SECVAR_SYSFS, is defined to enable this new sysfs
>> interface.
>>
>> Signed-off-by: Nayna Jain <nayna@linux.ibm.com>
>> ---
>>   arch/powerpc/Kconfig                 |   2 +
>>   drivers/firmware/Makefile            |   1 +
>>   drivers/firmware/efi/efivars.c       |   2 +-
>>   drivers/firmware/powerpc/Kconfig     |  12 +
>>   drivers/firmware/powerpc/Makefile    |   3 +
>>   drivers/firmware/powerpc/efi_error.c |  46 ++++
>>   drivers/firmware/powerpc/secvar.c    | 326 +++++++++++++++++++++++++++
>>   7 files changed, 391 insertions(+), 1 deletion(-)
>>   create mode 100644 drivers/firmware/powerpc/Kconfig
>>   create mode 100644 drivers/firmware/powerpc/Makefile
>>   create mode 100644 drivers/firmware/powerpc/efi_error.c
>>   create mode 100644 drivers/firmware/powerpc/secvar.c
> If you add/remove/modify sysfs files, you also need to update the
> relevant Documentation/ABI/ entry as well.  Please add something there
> to describe your new files when you resend the next version of this
> patch series.
>
>> diff --git a/drivers/firmware/powerpc/Kconfig b/drivers/firmware/powerpc/Kconfig
>> new file mode 100644
>> index 000000000000..e0303fc517d5
>> --- /dev/null
>> +++ b/drivers/firmware/powerpc/Kconfig
>> @@ -0,0 +1,12 @@
>> +config POWER_SECVAR_SYSFS
>> +	tristate "Enable sysfs interface for POWER secure variables"
>> +	default n
> default is always n, no need to list it.
>
>> --- /dev/null
>> +++ b/drivers/firmware/powerpc/efi_error.c
>> @@ -0,0 +1,46 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) 2019 IBM Corporation
>> + * Author: Nayna Jain <nayna@linux.ibm.com>
>> + *
>> + * efi_error.c
>> + *      - Error codes as understood by efi based tools
>> + *      Taken from drivers/firmware/efi/efi.c
> Why not just export the symbol from the original file instead of
> duplicating it here?
>
>> +static int convert_buffer_to_efi_guid(u8 *buffer, efi_guid_t *guid)
>> +{
>> +	u32 *a1;
>> +	u16 *a2;
>> +	u16 *a3;
>> +
>> +	a1 = kzalloc(4, GFP_KERNEL);
> No error checking in this function for memory issues at all?
>
>> +	memcpy(a1, buffer, 4);
>> +	*a1 = be32_to_cpu(*a1);
>> +
>> +	a2 = kzalloc(2, GFP_KERNEL);
>> +	memcpy(a2, buffer+4, 2);
>> +	*a2 = be16_to_cpu(*a2);
>> +
>> +	a3 = kzalloc(2, GFP_KERNEL);
>> +	memcpy(a3, buffer+6, 2);
>> +	*a3 = be16_to_cpu(*a3);
>> +
>> +	*guid = EFI_GUID(*a1, *a2, *a3, *(buffer + 8),
>> +			*(buffer + 9),
>> +			*(buffer + 10),
>> +			*(buffer + 11),
>> +			*(buffer + 12),
>> +			*(buffer + 13),
>> +			*(buffer + 14),
>> +			*(buffer + 15));
>> +
>> +	kfree(a1);
>> +	kfree(a2);
>> +	kfree(a3);
>> +	return 0;
>> +}
>> +static efi_status_t powerpc_get_next_variable(unsigned long *name_size,
>> +					      efi_char16_t *name,
>> +					      efi_guid_t *vendor)
>> +{
>> +	int rc;
>> +	u8 *key;
>> +	int namesize;
>> +	unsigned long keylen;
>> +	unsigned long keysize = 1024;
>> +	unsigned long *mdsize;
>> +	u8 *mdata = NULL;
>> +	efi_guid_t guid;
>> +
>> +	if (ucs2_strnlen(name, 1024) > 0) {
>> +		createkey(name, &key, &keylen);
>> +	} else {
>> +		keylen = 0;
>> +		key = kzalloc(1024, GFP_KERNEL);
>> +	}
>> +
>> +	pr_info("%s: powerpc get next variable, key is %s\n", __func__, key);
> Don't put debugging info like this in the kernel log of everyone :(
>
>> +
>> +	rc = opal_get_next_variable(key, &keylen, keysize);
>> +	if (rc) {
>> +		kfree(key);
>> +		return opal_to_efi_status(rc);
>> +	}
>> +
>> +	mdsize = kzalloc(sizeof(unsigned long), GFP_KERNEL);
> No error checking?
>
>> +	rc = opal_get_variable_size(key, keylen, mdsize, NULL);
>> +	if (rc)
>> +		goto out;
>> +
>> +	if (*mdsize <= 0)
>> +		goto out;
>> +
>> +	mdata = kzalloc(*mdsize, GFP_KERNEL);
>> +
>> +	rc = opal_get_variable(key, keylen, mdata, mdsize, NULL, NULL);
>> +	if (rc)
>> +		goto out;
>> +
>> +	if (*mdsize > 0) {
>> +		namesize = *mdsize - sizeof(efi_guid_t) - sizeof(u32);
>> +		if (namesize > 0) {
>> +			memset(&guid, 0, sizeof(efi_guid_t));
>> +			convert_buffer_to_efi_guid(mdata + namesize, &guid);
>> +			memcpy(vendor, &guid, sizeof(efi_guid_t));
>> +			memset(name, 0, namesize + 2);
>> +			memcpy(name, mdata, namesize);
>> +			*name_size = namesize + 2;
>> +			name[namesize++] = 0;
>> +			name[namesize] = 0;
>> +		}
>> +	}
>> +
>> +out:
>> +	kfree(mdsize);
>> +	kfree(mdata);
>> +
>> +	return opal_to_efi_status(rc);
>> +}
>> +
>> +static efi_status_t powerpc_set_variable(efi_char16_t *name, efi_guid_t *vendor,
>> +					 u32 attr, unsigned long data_size,
>> +					 void *data)
>> +{
>> +	int rc;
>> +	u8 *key;
>> +	unsigned long keylen;
>> +	u8 *metadata;
>> +	unsigned long mdsize;
>> +
>> +	if (!name)
>> +		return EFI_INVALID_PARAMETER;
>> +
>> +	if (!vendor)
>> +		return EFI_INVALID_PARAMETER;
>> +
>> +	createkey(name, &key, &keylen);
>> +	pr_info("%s: nayna key is %s\n", __func__, key);
> Again, please remove all of your debugging code when resending.
>
>> +
>> +	createmetadata(name, vendor, &attr, &metadata, &mdsize);
>> +
>> +	rc = opal_set_variable(key, keylen, metadata, mdsize, data, data_size);
>> +
>> +	return opal_to_efi_status(rc);
>> +}
>> +
>> +
>> +static const struct efivar_operations efivar_ops = {
>> +	.get_variable = powerpc_get_variable,
>> +	.set_variable = powerpc_set_variable,
>> +	.get_next_variable = powerpc_get_next_variable,
>> +};
>> +
>> +
>> +static __init int power_secvar_init(void)
>> +{
>> +	int rc = 0;
>> +	unsigned long ver = 0;
>> +
>> +	rc = opal_variable_version(&ver);
>> +	if (ver != BACKEND_TC_COMPAT_V1) {
>> +		pr_info("Compatible backend unsupported\n");
>> +		return -1;
> Do not make up error numbers, use the defined values please.

Thanks Greg for the review !!

I will address everything in the next version.

Thanks & Regards,
         - Nayna

  reply	other threads:[~2019-06-14 13:16 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-06-13 20:50 [PATCH 0/2] powerpc/powernv: expose secure variables to userspace Nayna Jain
2019-06-13 20:50 ` [PATCH 1/2] powerpc/powernv: add OPAL APIs for secure variables Nayna Jain
2019-06-13 20:50 ` [PATCH 2/2] powerpc: expose secure variables via sysfs Nayna Jain
2019-06-14  6:34   ` Greg Kroah-Hartman
2019-06-14 13:13     ` Nayna [this message]
2019-07-05  6:05   ` Michael Ellerman
2019-07-23 14:35     ` Nayna
2019-07-24  9:52       ` Oliver O'Halloran
2019-07-22 10:19   ` Oliver O'Halloran

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=2f4a3ab4-8e9c-a8fc-ba1a-71cf6a9fbb59@linux.vnet.ibm.com \
    --to=nayna@linux.vnet.ibm.com \
    --cc=ard.biesheuvel@linaro.org \
    --cc=cclaudio@linux.ibm.com \
    --cc=erichte@linux.ibm.com \
    --cc=erpalmer@us.ibm.com \
    --cc=gcwilson@linux.ibm.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=jk@ozlabs.org \
    --cc=linux-efi@vger.kernel.org \
    --cc=linux-integrity@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linuxppc-dev@ozlabs.org \
    --cc=matthew.garret@nebula.com \
    --cc=nayna@linux.ibm.com \
    --cc=paulus@samba.org \
    --cc=zohar@linux.ibm.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).