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

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

On Mon, Aug 08, 2022 at 04:31:06PM +0000, Christophe Leroy wrote:
> 
> 
> Le 08/08/2022 à 17:43, gjoyce@linux.vnet.ibm.com a écrit :
> > 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.
> 
> Is it platform specific or architecture specific ?
> 
> > + *
> > + * Copyright (C) 2022 IBM Corporation
> > + *
> > + * These are the accessor functions (read/write) for architecture specific
> > + * variables. Specific architectures can provide overrides.
> 
> "variables" is a very generic word which I think doesn't match what you 
> want to do.
> 
> For me "variables" are local variables and global variables in a C file. 
> Here it seems to be something completely different hence the name is 
> really meaningfull and misleading.
> 
> > + *
> > + */
> > +
> > +#include <linux/kernel.h>
> > +
> > +enum arch_variable_type {
> 
> arch_variable_type ? What's that ? variable types are char, short, long, 
> long long, etc ...
> 
> > +	ARCH_VAR_OPAL_KEY      = 0,     /* SED Opal Authentication Key */
> > +	ARCH_VAR_OTHER         = 1,     /* Other type of variable */
> > +	ARCH_VAR_MAX           = 1,     /* Maximum type value */
> > +};
> 
> Why the hell do you need an enum for two values only ?
> 
> > +
> > +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
> 
> The name is meaningless, too generic.
> 
> 
> > @@ -0,0 +1,25 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Platform variable operations.
> 
> platform versus architecture ?
> 
> > + *
> > + * 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)
> 
> Sorry, to read a variable, I use READ_ONCE or I read it directly.

This is supposed to be used for things like the EFI variables and the
already existing powernv secure variables.

Nonetheless, without adding the plumbing for the existing
implementations it is not clear what it's doing, and the interface is
agruably meaningless.

Hence I would either suggest to provide the plumbing necessary for
existing (secure) variable implementations to make use of the interface,
or use private implementations like all the existing platforms do
without exposing the values in any generic way, and leave that to
somebody who is comfortable with designing a working general inteface
for this.

Thanks

Michal