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

[Michael Ellerman <mpe@ellerman.id.au>]

Hi Greg,

gjoyce@linux.vnet.ibm.com writes:
> 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

I don't think "arch" is the right level of abstraction here.

There isn't a standard way to get these variables across a given arch,
they're not defined in the architecture specification etc.

As demonstrated in your patch 2, on powerpc they are coming from a
platform level pseudo device (provided by the PowerVM hypervisor). But
they would come from elsewhere on a bare metal system.

And you could imagine other architectures could support multiple ways to
retrieve these kind of variables from various different places, possibly
more than one place on a given system.

So I think at least you want a way for any device to register itself as
able to provide these variables. Possibly with a chain of handlers,
something like the sys_off_handlers, or maybe there only ever needs to
be one provider, the way pm_power_off (used to) work.

Looking at your patch to block/sed-opal.c:

  https://lore.kernel.org/all/20220718210156.1535955-4-gjoyce@linux.vnet.ibm.com/

It seems like the calls to these arch routines are closely tied to calls
to the keyring API. Should this functionality be part of the keyring
API?

At a mininmum I think you need to get much wider review on something
like this. So I'd suggest the keyring folks and as Michal pointed out,
this seems a bit like EFI variables so it would be good to Cc the
EFI/EFI variable folks.

cheers