[PATCH v3a 2/2] powerpc/pseries: Override lib/arch_vars.c functions

[Christophe Leroy]

Le 08/08/2022 à 17:43, gjoyce at linux.vnet.ibm.com a écrit :
> From: Greg Joyce <gjoyce at 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 at 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)

'enum arch_variable_type' is pointlessly long. And it only has two 
possible values, so should be a 'bool'


> +{
> +	struct plpks_var var;
> +	struct plpks_sed_object_data *data;
> +	u_int offset = 0;

Would be better to set it to 0 in the code that handles ARCH_VAR_OTHER 
and leave it unitialised here.

> +	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) {

A switch for a boolean value is pointless. Just do if/else, it will be a 
lot more readable.

> +	case ARCH_VAR_OPAL_KEY:
> +		var.component = PLPKS_SED_COMPONENT;
> +#ifdef OPAL_AUTH_KEY

#ifdefs in C files should be avoided, refer 
https://docs.kernel.org/process/coding-style.html#conditional-compilation

> +		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;
> +	}

That part of code seem to have a lot in common with 
arch_read_variable(), can you refactor ?

> +
> +	/* variable update requires delete first */
> +	vname.namelen = var.namelen;
> +	vname.name = var.name;
> +	(void)plpks_remove_var(var.component, var.os, vname);

Do you really need that cast to (void) ?

> +
> +	return plpks_write_var(var);
> +}