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

["Michal Suchánek" <msuchanek@suse.de>]

On Mon, Aug 01, 2022 at 03:45:45PM -0400, Nayna wrote:
> 
> 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.
For powervm, not powernv.

If it's not generic enough to cover even powerpc-specific keystores does
such generalization even need to exist?
> 
> Access to EFI variables should be implemented by EFI arch specific interface
> and PowerNV will have to do the same if it needs to.

If such generic interface is desirable it should cover the existing
architectures I think. Otherwise how can you tell if it's usable there?

Thanks

Michal