Re: [PATCH v4 21/24] powerpc/pseries: Pass PLPKS password on kexec

[Nicholas Piggin]

On Fri Jan 20, 2023 at 5:43 PM AEST, Andrew Donnellan wrote:
> From: Russell Currey <ruscur@xxxxxxxxxx>
>
> Before interacting with the PLPKS, we ask the hypervisor to generate a
> password for the current boot, which is then required for most further
> PLPKS operations.
>
> If we kexec into a new kernel, the new kernel will try and fail to
> generate a new password, as the password has already been set.
>
> Pass the password through to the new kernel via the device tree, in
> /chosen/plpks-pw. Check for the presence of this property before trying

In /chosen/ibm,plpks-pw

> to generate a new password - if it exists, use the existing password and
> remove it from the device tree.
>
> Signed-off-by: Russell Currey <ruscur@xxxxxxxxxx>
> Signed-off-by: Andrew Donnellan <ajd@xxxxxxxxxxxxx>
>
> ---
>
> v3: New patch
>
> v4: Fix compile when CONFIG_PSERIES_PLPKS=n (snowpatch)
>
>     Fix error handling on fdt_path_offset() call (ruscur)
> ---
>  arch/powerpc/kexec/file_load_64.c      | 18 ++++++++++++++++++
>  arch/powerpc/platforms/pseries/plpks.c | 18 +++++++++++++++++-
>  2 files changed, 35 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/kexec/file_load_64.c b/arch/powerpc/kexec/file_load_64.c
> index af8854f9eae3..0c9130af60cc 100644
> --- a/arch/powerpc/kexec/file_load_64.c
> +++ b/arch/powerpc/kexec/file_load_64.c
> @@ -27,6 +27,7 @@
>  #include <asm/kexec_ranges.h>
>  #include <asm/crashdump-ppc64.h>
>  #include <asm/prom.h>
> +#include <asm/plpks.h>
>  
>  struct umem_info {
>  	u64 *buf;		/* data buffer for usable-memory property */
> @@ -1156,6 +1157,9 @@ int setup_new_fdt_ppc64(const struct kimage *image, void *fdt,
>  {
>  	struct crash_mem *umem = NULL, *rmem = NULL;
>  	int i, nr_ranges, ret;
> +#ifdef CONFIG_PSERIES_PLPKS
> +	int chosen_offset;
> +#endif

Could put this in plpks_is_available and avoid an ifdef.

>  
>  	/*
>  	 * Restrict memory usage for kdump kernel by setting up
> @@ -1230,6 +1234,20 @@ int setup_new_fdt_ppc64(const struct kimage *image, void *fdt,
>  		}
>  	}
>  
> +#ifdef CONFIG_PSERIES_PLPKS
> +	// If we have PLPKS active, we need to provide the password
> +	if (plpks_is_available()) {
> +		chosen_offset = fdt_path_offset(fdt, "/chosen");
> +		if (chosen_offset < 0) {
> +			pr_err("Can't find chosen node: %s\n",
> +			       fdt_strerror(chosen_offset));
> +			goto out;
> +		}
> +		ret = fdt_setprop(fdt, chosen_offset, "ibm,plpks-pw",
> +				  plpks_get_password(), plpks_get_passwordlen());
> +	}
> +#endif // CONFIG_PSERIES_PLPKS

I think if you define plpks_get_password and plpks_get_passwordlen as
BUILD_BUG_ON when PLPKS is not configured and plpks_is_available as
false, you could remove the ifdef entirely.

> +
>  out:
>  	kfree(rmem);
>  	kfree(umem);
> diff --git a/arch/powerpc/platforms/pseries/plpks.c b/arch/powerpc/platforms/pseries/plpks.c
> index b3c7410a4f13..0350f10e1755 100644
> --- a/arch/powerpc/platforms/pseries/plpks.c
> +++ b/arch/powerpc/platforms/pseries/plpks.c
> @@ -16,6 +16,7 @@
>  #include <linux/slab.h>
>  #include <linux/string.h>
>  #include <linux/types.h>
> +#include <linux/of.h>
>  #include <asm/hvcall.h>
>  #include <asm/machdep.h>
>  #include <asm/plpks.h>
> @@ -126,7 +127,22 @@ static int plpks_gen_password(void)
>  {
>  	unsigned long retbuf[PLPAR_HCALL_BUFSIZE] = { 0 };
>  	u8 *password, consumer = PLPKS_OS_OWNER;
> -	int rc;
> +	struct property *prop;
> +	int rc, len;
> +
> +	// Before we generate the password, we may have been booted by kexec and
> +	// provided with a previous password.  Check for that first.

So not really generating the password then. Should it be in a different
function the caller makes first?

> +	prop = of_find_property(of_chosen, "ibm,plpks-pw", &len);
> +	if (prop) {
> +		ospasswordlength = (u16)len;
> +		ospassword = kzalloc(ospasswordlength, GFP_KERNEL);
> +		if (!ospassword) {
> +			of_remove_property(of_chosen, prop);
> +			return -ENOMEM;
> +		}
> +		memcpy(ospassword, prop->value, len);
> +		return of_remove_property(of_chosen, prop);

Why do you remove the property afterward?

Thanks,
Nick