Re: [PATCH v3 1/2] lib: generic accessor functions for arch keystore

[Nayna <nayna@linux.vnet.ibm.com>]

On 8/1/22 09:40, Michal Suchánek wrote:
> Hello,
>
> On Mon, Aug 01, 2022 at 07:34:25AM -0500, gjoyce@linux.vnet.ibm.com wrote:
>> From: Greg Joyce <gjoyce@linux.vnet.ibm.com>
>>
>> Generic kernel subsystems may rely on platform specific persistent
>> KeyStore to store objects containing sensitive key material. In such case,
>> they need to access architecture specific functions to perform read/write
>> operations on these variables.
>>
>> Define the generic variable read/write prototypes to be implemented by
>> architecture specific versions. The default(weak) implementations of
>> these prototypes return -EOPNOTSUPP unless overridden by architecture
>> versions.
>>
>> Signed-off-by: Greg Joyce <gjoyce@linux.vnet.ibm.com>
>> ---
>>   include/linux/arch_vars.h | 23 +++++++++++++++++++++++
>>   lib/Makefile              |  2 +-
>>   lib/arch_vars.c           | 25 +++++++++++++++++++++++++
>>   3 files changed, 49 insertions(+), 1 deletion(-)
>>   create mode 100644 include/linux/arch_vars.h
>>   create mode 100644 lib/arch_vars.c
>>
>> diff --git a/include/linux/arch_vars.h b/include/linux/arch_vars.h
>> new file mode 100644
>> index 000000000000..9c280ff9432e
>> --- /dev/null
>> +++ b/include/linux/arch_vars.h
>> @@ -0,0 +1,23 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * Platform variable opearations.
>> + *
>> + * Copyright (C) 2022 IBM Corporation
>> + *
>> + * These are the accessor functions (read/write) for architecture specific
>> + * variables. Specific architectures can provide overrides.
>> + *
>> + */
>> +
>> +#include <linux/kernel.h>
>> +
>> +enum arch_variable_type {
>> +	ARCH_VAR_OPAL_KEY      = 0,     /* SED Opal Authentication Key */
>> +	ARCH_VAR_OTHER         = 1,     /* Other type of variable */
>> +	ARCH_VAR_MAX           = 1,     /* Maximum type value */
>> +};
>> +
>> +int arch_read_variable(enum arch_variable_type type, char *varname,
>> +		       void *varbuf, u_int *varlen);
>> +int arch_write_variable(enum arch_variable_type type, char *varname,
>> +			void *varbuf, u_int varlen);
>> diff --git a/lib/Makefile b/lib/Makefile
>> index f99bf61f8bbc..b90c4cb0dbbb 100644
>> --- a/lib/Makefile
>> +++ b/lib/Makefile
>> @@ -48,7 +48,7 @@ obj-y += bcd.o sort.o parser.o debug_locks.o random32.o \
>>   	 bsearch.o find_bit.o llist.o memweight.o kfifo.o \
>>   	 percpu-refcount.o rhashtable.o \
>>   	 once.o refcount.o usercopy.o errseq.o bucket_locks.o \
>> -	 generic-radix-tree.o
>> +	 generic-radix-tree.o arch_vars.o
>>   obj-$(CONFIG_STRING_SELFTEST) += test_string.o
>>   obj-y += string_helpers.o
>>   obj-$(CONFIG_TEST_STRING_HELPERS) += test-string_helpers.o
>> diff --git a/lib/arch_vars.c b/lib/arch_vars.c
>> new file mode 100644
>> index 000000000000..e6f16d7d09c1
>> --- /dev/null
>> +++ b/lib/arch_vars.c
>> @@ -0,0 +1,25 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Platform variable operations.
>> + *
>> + * Copyright (C) 2022 IBM Corporation
>> + *
>> + * These are the accessor functions (read/write) for architecture specific
>> + * variables. Specific architectures can provide overrides.
>> + *
>> + */
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/arch_vars.h>
>> +
>> +int __weak arch_read_variable(enum arch_variable_type type, char *varname,
>> +			      void *varbuf, u_int *varlen)
>> +{
>> +	return -EOPNOTSUPP;
>> +}
>> +
>> +int __weak arch_write_variable(enum arch_variable_type type, char *varname,
>> +			       void *varbuf, u_int varlen)
>> +{
>> +	return -EOPNOTSUPP;
>> +}
>> -- 
> Doesn't EFI already have some variables?
>
> And even powernv?
>
> Shouldn't this generalize the already existing variables?
>
> Or move to powerpc and at least generalize the powerpc ones?

Yes, EFI and PowerNV do have variables, but I am not exactly clear about 
your reference to them in this context. What do you mean by generalize 
already existing variables ?

This interface is actually generalizing calls to access platform 
specific keystores. It is explained in cover letter that this patch is 
defining generic interface and these are default implementations which 
needs to be overridden by arch specific versions.  For PowerVM PLPAR 
Platform KeyStore, the arch specific version is implemented in Patch 2.

Access to EFI variables should be implemented by EFI arch specific 
interface and PowerNV will have to do the same if it needs to.

Hope it helps.

Thanks & Regards,

     - Nayna