* [PATCH v3 0/2] generic and PowerPC accessor functions for arch keystore @ 2022-08-01 12:34 ` gjoyce 0 siblings, 0 replies; 16+ messages in thread From: gjoyce @ 2022-08-01 12:34 UTC (permalink / raw) To: linux-block Cc: axboe, gjoyce, nayna, jonathan.derrick, brking, akpm, linuxppc-dev, gjoyce From: Greg Joyce <gjoyce@linux.vnet.ibm.com> Architectural neutral functions have been defined for accessing architecture specific variable store. The neutral functions are defined as weak so that they may be superseded by platform specific versions. PowerPC/pseries versions of these functions provide read/write access to the non-volatile PLPKS data store. This functionality allows kernel code such as the block SED opal driver to store authentication keys in a secure permanent store. Greg Joyce (2): lib: define generic accessor functions for arch specific keystore powerpc/pseries: Override lib/arch_vars.c functions arch/powerpc/platforms/pseries/Makefile | 1 + .../platforms/pseries/plpks_arch_ops.c | 167 ++++++++++++++++++ include/linux/arch_vars.h | 23 +++ lib/Makefile | 2 +- lib/arch_vars.c | 25 +++ 5 files changed, 217 insertions(+), 1 deletion(-) create mode 100644 arch/powerpc/platforms/pseries/plpks_arch_ops.c create mode 100644 include/linux/arch_vars.h create mode 100644 lib/arch_vars.c Signed-off-by: Greg Joyce <gjoyce@linux.vnet.ibm.com> base-commit: ff6992735ade75aae3e35d16b17da1008d753d28 -- 2.27.0 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v3 0/2] generic and PowerPC accessor functions for arch keystore @ 2022-08-01 12:34 ` gjoyce 0 siblings, 0 replies; 16+ messages in thread From: gjoyce @ 2022-08-01 12:34 UTC (permalink / raw) To: linux-block Cc: linuxppc-dev, jonathan.derrick, brking, gjoyce, nayna, axboe, akpm, gjoyce From: Greg Joyce <gjoyce@linux.vnet.ibm.com> Architectural neutral functions have been defined for accessing architecture specific variable store. The neutral functions are defined as weak so that they may be superseded by platform specific versions. PowerPC/pseries versions of these functions provide read/write access to the non-volatile PLPKS data store. This functionality allows kernel code such as the block SED opal driver to store authentication keys in a secure permanent store. Greg Joyce (2): lib: define generic accessor functions for arch specific keystore powerpc/pseries: Override lib/arch_vars.c functions arch/powerpc/platforms/pseries/Makefile | 1 + .../platforms/pseries/plpks_arch_ops.c | 167 ++++++++++++++++++ include/linux/arch_vars.h | 23 +++ lib/Makefile | 2 +- lib/arch_vars.c | 25 +++ 5 files changed, 217 insertions(+), 1 deletion(-) create mode 100644 arch/powerpc/platforms/pseries/plpks_arch_ops.c create mode 100644 include/linux/arch_vars.h create mode 100644 lib/arch_vars.c Signed-off-by: Greg Joyce <gjoyce@linux.vnet.ibm.com> base-commit: ff6992735ade75aae3e35d16b17da1008d753d28 -- 2.27.0 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v3 1/2] lib: generic accessor functions for arch keystore 2022-08-01 12:34 ` gjoyce @ 2022-08-01 12:34 ` gjoyce -1 siblings, 0 replies; 16+ messages in thread From: gjoyce @ 2022-08-01 12:34 UTC (permalink / raw) To: linux-block Cc: axboe, gjoyce, nayna, jonathan.derrick, brking, akpm, linuxppc-dev, gjoyce 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; +} -- 2.27.0 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v3 1/2] lib: generic accessor functions for arch keystore @ 2022-08-01 12:34 ` gjoyce 0 siblings, 0 replies; 16+ messages in thread From: gjoyce @ 2022-08-01 12:34 UTC (permalink / raw) To: linux-block Cc: linuxppc-dev, jonathan.derrick, brking, gjoyce, nayna, axboe, akpm, gjoyce 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; +} -- 2.27.0 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v3 1/2] lib: generic accessor functions for arch keystore 2022-08-01 12:34 ` gjoyce @ 2022-08-01 13:40 ` Michal Suchánek -1 siblings, 0 replies; 16+ messages in thread From: Michal Suchánek @ 2022-08-01 13:40 UTC (permalink / raw) To: gjoyce Cc: linux-block, axboe, nayna, jonathan.derrick, brking, akpm, linuxppc-dev, gjoyce 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? Thanks Michal ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 1/2] lib: generic accessor functions for arch keystore @ 2022-08-01 13:40 ` Michal Suchánek 0 siblings, 0 replies; 16+ messages in thread From: Michal Suchánek @ 2022-08-01 13:40 UTC (permalink / raw) To: gjoyce Cc: axboe, nayna, linux-block, jonathan.derrick, brking, akpm, linuxppc-dev, gjoyce 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? Thanks Michal ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 1/2] lib: generic accessor functions for arch keystore 2022-08-01 13:40 ` Michal Suchánek @ 2022-08-01 19:45 ` Nayna -1 siblings, 0 replies; 16+ messages in thread From: Nayna @ 2022-08-01 19:45 UTC (permalink / raw) To: Michal Suchánek Cc: axboe, nayna, linux-block, jonathan.derrick, brking, akpm, linuxppc-dev, gjoyce, gjoyce, Michael Ellerman, Nick Piggin, Christophe Leroy 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 1/2] lib: generic accessor functions for arch keystore @ 2022-08-01 19:45 ` Nayna 0 siblings, 0 replies; 16+ messages in thread From: Nayna @ 2022-08-01 19:45 UTC (permalink / raw) To: Michal Suchánek Cc: axboe, Christophe Leroy, gjoyce, nayna, Nick Piggin, linux-block, jonathan.derrick, brking, akpm, linuxppc-dev, gjoyce 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 1/2] lib: generic accessor functions for arch keystore 2022-08-01 19:45 ` Nayna @ 2022-08-01 20:24 ` Michal Suchánek -1 siblings, 0 replies; 16+ messages in thread From: Michal Suchánek @ 2022-08-01 20:24 UTC (permalink / raw) To: Nayna Cc: axboe, nayna, linux-block, jonathan.derrick, brking, akpm, linuxppc-dev, gjoyce, gjoyce, Michael Ellerman, Nick Piggin, Christophe Leroy 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 1/2] lib: generic accessor functions for arch keystore @ 2022-08-01 20:24 ` Michal Suchánek 0 siblings, 0 replies; 16+ messages in thread From: Michal Suchánek @ 2022-08-01 20:24 UTC (permalink / raw) To: Nayna Cc: axboe, Christophe Leroy, gjoyce, nayna, Nick Piggin, linux-block, jonathan.derrick, brking, akpm, linuxppc-dev, gjoyce 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 1/2] lib: generic accessor functions for arch keystore 2022-08-01 20:24 ` Michal Suchánek @ 2022-08-04 15:41 ` Greg Joyce -1 siblings, 0 replies; 16+ messages in thread From: Greg Joyce @ 2022-08-04 15:41 UTC (permalink / raw) To: Michal Suchánek, Nayna Cc: axboe, nayna, linux-block, jonathan.derrick, brking, akpm, linuxppc-dev, gjoyce, Michael Ellerman, Nick Piggin, Christophe Leroy On Mon, 2022-08-01 at 22:24 +0200, Michal Suchánek wrote: > > > > + > > > > +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? I believe that the interface is generic enough to cover most if not all keystores. However, we're just implementing a PowerVM version since that is our mandate. > > 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? Are you suggesting that we implement architecture specific implementations for every architecture supported by Linux? I'm afraid that we don't have the time (or skills) to do that. The intent is to provide the "weak" versions of the interface functions so that they can be overridden as folks have the time or inclination to provide them for other architectures. -Greg ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 1/2] lib: generic accessor functions for arch keystore @ 2022-08-04 15:41 ` Greg Joyce 0 siblings, 0 replies; 16+ messages in thread From: Greg Joyce @ 2022-08-04 15:41 UTC (permalink / raw) To: Michal Suchánek, Nayna Cc: axboe, Christophe Leroy, nayna, Nick Piggin, linux-block, jonathan.derrick, brking, akpm, linuxppc-dev, gjoyce On Mon, 2022-08-01 at 22:24 +0200, Michal Suchánek wrote: > > > > + > > > > +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? I believe that the interface is generic enough to cover most if not all keystores. However, we're just implementing a PowerVM version since that is our mandate. > > 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? Are you suggesting that we implement architecture specific implementations for every architecture supported by Linux? I'm afraid that we don't have the time (or skills) to do that. The intent is to provide the "weak" versions of the interface functions so that they can be overridden as folks have the time or inclination to provide them for other architectures. -Greg ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 1/2] lib: generic accessor functions for arch keystore 2022-08-01 12:34 ` gjoyce (?) (?) @ 2022-08-02 2:59 ` Michael Ellerman 2022-08-02 22:39 ` Greg Joyce -1 siblings, 1 reply; 16+ messages in thread From: Michael Ellerman @ 2022-08-02 2:59 UTC (permalink / raw) To: gjoyce, linux-block Cc: axboe, gjoyce, nayna, jonathan.derrick, brking, akpm, linuxppc-dev, gjoyce 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 1/2] lib: generic accessor functions for arch keystore 2022-08-02 2:59 ` Michael Ellerman @ 2022-08-02 22:39 ` Greg Joyce 0 siblings, 0 replies; 16+ messages in thread From: Greg Joyce @ 2022-08-02 22:39 UTC (permalink / raw) To: Michael Ellerman, linux-block Cc: axboe, nayna, jonathan.derrick, brking, akpm, linuxppc-dev Michael and Michal, On Tue, 2022-08-02 at 12:59 +1000, Michael Ellerman wrote: > 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. I'm open to something other than "arch_" if that causes confusion. Maybe "plat_"? > 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. Agreed, and that's why an architecture or platform can override the functions. The first parameter also allows for further distinction of how the data could be interpreted, including in multiple ways as you suggest. I wanted to allow for different types of persistent storage. For instance you could have a platform that used a NAND deice for permanent storage that could still use the API as proposed. > 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. I did look at some of the other ways of dynamically registering functions but most of the exisiting examples are centered around a specific device entity which is not the case here. The functionailty is pretty simple so I was hoping to keep the API simple as well. The sys_off_handler functions certainly provide a good way of registering for asynchronous notifications but for this purpose we need synchronous functions that possibly return data. Many of the pci functions end up in platform specific code but they use information from struct pci_dev to route the call to the appropriate place. > 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? Those calls are just using the API to read/write named variables that happen to be keys in this case. I was envisioning that there may be uses other than SED for persistent key/values. > 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. The keyring folks were on the original SED patchset review and I did incorporate a good comment involving use of keyrings. I will copy them again (as well as EFI folks) for the next update. -Greg ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v3 2/2] powerpc/pseries: Override lib/arch_vars.c functions 2022-08-01 12:34 ` gjoyce @ 2022-08-01 12:34 ` gjoyce -1 siblings, 0 replies; 16+ messages in thread From: gjoyce @ 2022-08-01 12:34 UTC (permalink / raw) To: linux-block Cc: axboe, gjoyce, nayna, jonathan.derrick, brking, akpm, linuxppc-dev, gjoyce From: Greg Joyce <gjoyce@linux.vnet.ibm.com> Self Encrypting Drives(SED) make use of POWER LPAR Platform KeyStore for storing its variables. Thus the block subsystem needs to access PowerPC specific functions to read/write objects in PLPKS. Override the default implementations in lib/arch_vars.c file with PowerPC specific versions. Signed-off-by: Greg Joyce <gjoyce@linux.vnet.ibm.com> --- arch/powerpc/platforms/pseries/Makefile | 1 + .../platforms/pseries/plpks_arch_ops.c | 167 ++++++++++++++++++ 2 files changed, 168 insertions(+) create mode 100644 arch/powerpc/platforms/pseries/plpks_arch_ops.c diff --git a/arch/powerpc/platforms/pseries/Makefile b/arch/powerpc/platforms/pseries/Makefile index 14e143b946a3..3a545422eae5 100644 --- a/arch/powerpc/platforms/pseries/Makefile +++ b/arch/powerpc/platforms/pseries/Makefile @@ -29,6 +29,7 @@ obj-$(CONFIG_PPC_SPLPAR) += vphn.o obj-$(CONFIG_PPC_SVM) += svm.o obj-$(CONFIG_FA_DUMP) += rtas-fadump.o obj-$(CONFIG_PSERIES_PLPKS) += plpks.o +obj-$(CONFIG_PSERIES_PLPKS) += plpks_arch_ops.o obj-$(CONFIG_SUSPEND) += suspend.o obj-$(CONFIG_PPC_VAS) += vas.o vas-sysfs.o diff --git a/arch/powerpc/platforms/pseries/plpks_arch_ops.c b/arch/powerpc/platforms/pseries/plpks_arch_ops.c new file mode 100644 index 000000000000..fdea3322f696 --- /dev/null +++ b/arch/powerpc/platforms/pseries/plpks_arch_ops.c @@ -0,0 +1,167 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * POWER Platform arch specific code for SED + * Copyright (C) 2022 IBM Corporation + * + * Define operations for generic kernel subsystems to read/write keys + * from POWER LPAR Platform KeyStore(PLPKS). + * + * List of subsystems/usecase using PLPKS: + * - Self Encrypting Drives(SED) + */ + +#include <linux/kernel.h> +#include <linux/slab.h> +#include <linux/string.h> +#include <linux/ioctl.h> +#include <uapi/linux/sed-opal.h> +#include <linux/sed-opal.h> +#include <linux/arch_vars.h> +#include "plpks.h" + +/* + * variable structure that contains all SED data + */ +struct plpks_sed_object_data { + u_char version; + u_char pad1[7]; + u_long authority; + u_long range; + u_int key_len; + u_char key[32]; +}; + +/* + * ext_type values + * 00 no extension exists + * 01-1F common + * 20-3F AIX + * 40-5F Linux + * 60-7F IBMi + */ + +/* + * This extension is optional for version 1 sed_object_data + */ +struct sed_object_extension { + u8 ext_type; + u8 rsvd[3]; + u8 ext_data[64]; +}; + +#define PKS_SED_OBJECT_DATA_V1 1 +#define PKS_SED_MANGLED_LABEL "/default/pri" +#define PLPKS_SED_COMPONENT "sed-opal" + +#define PLPKS_ARCHVAR_POLICY WORLDREADABLE +#define PLPKS_ARCHVAR_OS_COMMON 4 + +/* + * Read the variable data from PKS given the label + */ +int arch_read_variable(enum arch_variable_type type, char *varname, + void *varbuf, u_int *varlen) +{ + struct plpks_var var; + struct plpks_sed_object_data *data; + u_int offset = 0; + char *buf = (char *)varbuf; + int ret; + + var.name = varname; + var.namelen = strlen(varname); + var.policy = PLPKS_ARCHVAR_POLICY; + var.os = PLPKS_ARCHVAR_OS_COMMON; + var.data = NULL; + var.datalen = 0; + + switch (type) { + case ARCH_VAR_OPAL_KEY: + var.component = PLPKS_SED_COMPONENT; +#ifdef OPAL_AUTH_KEY + if (strcmp(OPAL_AUTH_KEY, varname) == 0) { + var.name = PKS_SED_MANGLED_LABEL; + var.namelen = strlen(varname); + } +#endif + offset = offsetof(struct plpks_sed_object_data, key); + break; + case ARCH_VAR_OTHER: + var.component = ""; + break; + } + + ret = plpks_read_os_var(&var); + if (ret != 0) + return ret; + + if (offset > var.datalen) + offset = 0; + + switch (type) { + case ARCH_VAR_OPAL_KEY: + data = (struct plpks_sed_object_data *)var.data; + *varlen = data->key_len; + break; + case ARCH_VAR_OTHER: + *varlen = var.datalen; + break; + } + + if (var.data) { + memcpy(varbuf, var.data + offset, var.datalen - offset); + buf[*varlen] = '\0'; + kfree(var.data); + } + + return 0; +} + +/* + * Write the variable data to PKS given the label + */ +int arch_write_variable(enum arch_variable_type type, char *varname, + void *varbuf, u_int varlen) +{ + struct plpks_var var; + struct plpks_sed_object_data data; + struct plpks_var_name vname; + + var.name = varname; + var.namelen = strlen(varname); + var.policy = PLPKS_ARCHVAR_POLICY; + var.os = PLPKS_ARCHVAR_OS_COMMON; + var.datalen = varlen; + var.data = varbuf; + + switch (type) { + case ARCH_VAR_OPAL_KEY: + var.component = PLPKS_SED_COMPONENT; +#ifdef OPAL_AUTH_KEY + if (strcmp(OPAL_AUTH_KEY, varname) == 0) { + var.name = PKS_SED_MANGLED_LABEL; + var.namelen = strlen(varname); + } +#endif + var.datalen = sizeof(struct plpks_sed_object_data); + var.data = (u8 *)&data; + + /* initialize SED object */ + data.version = PKS_SED_OBJECT_DATA_V1; + data.authority = 0; + data.range = 0; + data.key_len = varlen; + memcpy(data.key, varbuf, varlen); + break; + case ARCH_VAR_OTHER: + var.component = ""; + break; + } + + /* variable update requires delete first */ + vname.namelen = var.namelen; + vname.name = var.name; + (void)plpks_remove_var(var.component, var.os, vname); + + return plpks_write_var(var); +} -- 2.27.0 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v3 2/2] powerpc/pseries: Override lib/arch_vars.c functions @ 2022-08-01 12:34 ` gjoyce 0 siblings, 0 replies; 16+ messages in thread From: gjoyce @ 2022-08-01 12:34 UTC (permalink / raw) To: linux-block Cc: linuxppc-dev, jonathan.derrick, brking, gjoyce, nayna, axboe, akpm, gjoyce From: Greg Joyce <gjoyce@linux.vnet.ibm.com> Self Encrypting Drives(SED) make use of POWER LPAR Platform KeyStore for storing its variables. Thus the block subsystem needs to access PowerPC specific functions to read/write objects in PLPKS. Override the default implementations in lib/arch_vars.c file with PowerPC specific versions. Signed-off-by: Greg Joyce <gjoyce@linux.vnet.ibm.com> --- arch/powerpc/platforms/pseries/Makefile | 1 + .../platforms/pseries/plpks_arch_ops.c | 167 ++++++++++++++++++ 2 files changed, 168 insertions(+) create mode 100644 arch/powerpc/platforms/pseries/plpks_arch_ops.c diff --git a/arch/powerpc/platforms/pseries/Makefile b/arch/powerpc/platforms/pseries/Makefile index 14e143b946a3..3a545422eae5 100644 --- a/arch/powerpc/platforms/pseries/Makefile +++ b/arch/powerpc/platforms/pseries/Makefile @@ -29,6 +29,7 @@ obj-$(CONFIG_PPC_SPLPAR) += vphn.o obj-$(CONFIG_PPC_SVM) += svm.o obj-$(CONFIG_FA_DUMP) += rtas-fadump.o obj-$(CONFIG_PSERIES_PLPKS) += plpks.o +obj-$(CONFIG_PSERIES_PLPKS) += plpks_arch_ops.o obj-$(CONFIG_SUSPEND) += suspend.o obj-$(CONFIG_PPC_VAS) += vas.o vas-sysfs.o diff --git a/arch/powerpc/platforms/pseries/plpks_arch_ops.c b/arch/powerpc/platforms/pseries/plpks_arch_ops.c new file mode 100644 index 000000000000..fdea3322f696 --- /dev/null +++ b/arch/powerpc/platforms/pseries/plpks_arch_ops.c @@ -0,0 +1,167 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * POWER Platform arch specific code for SED + * Copyright (C) 2022 IBM Corporation + * + * Define operations for generic kernel subsystems to read/write keys + * from POWER LPAR Platform KeyStore(PLPKS). + * + * List of subsystems/usecase using PLPKS: + * - Self Encrypting Drives(SED) + */ + +#include <linux/kernel.h> +#include <linux/slab.h> +#include <linux/string.h> +#include <linux/ioctl.h> +#include <uapi/linux/sed-opal.h> +#include <linux/sed-opal.h> +#include <linux/arch_vars.h> +#include "plpks.h" + +/* + * variable structure that contains all SED data + */ +struct plpks_sed_object_data { + u_char version; + u_char pad1[7]; + u_long authority; + u_long range; + u_int key_len; + u_char key[32]; +}; + +/* + * ext_type values + * 00 no extension exists + * 01-1F common + * 20-3F AIX + * 40-5F Linux + * 60-7F IBMi + */ + +/* + * This extension is optional for version 1 sed_object_data + */ +struct sed_object_extension { + u8 ext_type; + u8 rsvd[3]; + u8 ext_data[64]; +}; + +#define PKS_SED_OBJECT_DATA_V1 1 +#define PKS_SED_MANGLED_LABEL "/default/pri" +#define PLPKS_SED_COMPONENT "sed-opal" + +#define PLPKS_ARCHVAR_POLICY WORLDREADABLE +#define PLPKS_ARCHVAR_OS_COMMON 4 + +/* + * Read the variable data from PKS given the label + */ +int arch_read_variable(enum arch_variable_type type, char *varname, + void *varbuf, u_int *varlen) +{ + struct plpks_var var; + struct plpks_sed_object_data *data; + u_int offset = 0; + char *buf = (char *)varbuf; + int ret; + + var.name = varname; + var.namelen = strlen(varname); + var.policy = PLPKS_ARCHVAR_POLICY; + var.os = PLPKS_ARCHVAR_OS_COMMON; + var.data = NULL; + var.datalen = 0; + + switch (type) { + case ARCH_VAR_OPAL_KEY: + var.component = PLPKS_SED_COMPONENT; +#ifdef OPAL_AUTH_KEY + if (strcmp(OPAL_AUTH_KEY, varname) == 0) { + var.name = PKS_SED_MANGLED_LABEL; + var.namelen = strlen(varname); + } +#endif + offset = offsetof(struct plpks_sed_object_data, key); + break; + case ARCH_VAR_OTHER: + var.component = ""; + break; + } + + ret = plpks_read_os_var(&var); + if (ret != 0) + return ret; + + if (offset > var.datalen) + offset = 0; + + switch (type) { + case ARCH_VAR_OPAL_KEY: + data = (struct plpks_sed_object_data *)var.data; + *varlen = data->key_len; + break; + case ARCH_VAR_OTHER: + *varlen = var.datalen; + break; + } + + if (var.data) { + memcpy(varbuf, var.data + offset, var.datalen - offset); + buf[*varlen] = '\0'; + kfree(var.data); + } + + return 0; +} + +/* + * Write the variable data to PKS given the label + */ +int arch_write_variable(enum arch_variable_type type, char *varname, + void *varbuf, u_int varlen) +{ + struct plpks_var var; + struct plpks_sed_object_data data; + struct plpks_var_name vname; + + var.name = varname; + var.namelen = strlen(varname); + var.policy = PLPKS_ARCHVAR_POLICY; + var.os = PLPKS_ARCHVAR_OS_COMMON; + var.datalen = varlen; + var.data = varbuf; + + switch (type) { + case ARCH_VAR_OPAL_KEY: + var.component = PLPKS_SED_COMPONENT; +#ifdef OPAL_AUTH_KEY + if (strcmp(OPAL_AUTH_KEY, varname) == 0) { + var.name = PKS_SED_MANGLED_LABEL; + var.namelen = strlen(varname); + } +#endif + var.datalen = sizeof(struct plpks_sed_object_data); + var.data = (u8 *)&data; + + /* initialize SED object */ + data.version = PKS_SED_OBJECT_DATA_V1; + data.authority = 0; + data.range = 0; + data.key_len = varlen; + memcpy(data.key, varbuf, varlen); + break; + case ARCH_VAR_OTHER: + var.component = ""; + break; + } + + /* variable update requires delete first */ + vname.namelen = var.namelen; + vname.name = var.name; + (void)plpks_remove_var(var.component, var.os, vname); + + return plpks_write_var(var); +} -- 2.27.0 ^ permalink raw reply related [flat|nested] 16+ messages in thread
end of thread, other threads:[~2022-08-04 15:42 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-08-01 12:34 [PATCH v3 0/2] generic and PowerPC accessor functions for arch keystore gjoyce 2022-08-01 12:34 ` gjoyce 2022-08-01 12:34 ` [PATCH v3 1/2] lib: generic " gjoyce 2022-08-01 12:34 ` gjoyce 2022-08-01 13:40 ` Michal Suchánek 2022-08-01 13:40 ` Michal Suchánek 2022-08-01 19:45 ` Nayna 2022-08-01 19:45 ` Nayna 2022-08-01 20:24 ` Michal Suchánek 2022-08-01 20:24 ` Michal Suchánek 2022-08-04 15:41 ` Greg Joyce 2022-08-04 15:41 ` Greg Joyce 2022-08-02 2:59 ` Michael Ellerman 2022-08-02 22:39 ` Greg Joyce 2022-08-01 12:34 ` [PATCH v3 2/2] powerpc/pseries: Override lib/arch_vars.c functions gjoyce 2022-08-01 12:34 ` gjoyce
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.