Re: [PATCH v3a 1/2] lib: generic accessor functions for arch keystore
From: Christophe Leroy <hidden>
Date: 2022-08-08 16:31:15
Also in:
keyrings, linux-efi, linuxppc-dev
Le 08/08/2022 à 17:43, gjoyce@linux.vnet.ibm.com a écrit :
quoted hunk ↗ jump to hunk
From: Greg Joyce <redacted> 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 <redacted> --- 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.cdiff --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 ?
quoted hunk ↗ jump to hunk
+ +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.odiff --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.
quoted hunk ↗ jump to hunk
@@ -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.
+{
+ return -EOPNOTSUPP;
+}
+
+int __weak arch_write_variable(enum arch_variable_type type, char *varname,
+ void *varbuf, u_int varlen)
+{
+ return -EOPNOTSUPP;
+}