Mime-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset=UTF-8
Date:   Tue, 24 Jan 2023 14:36:11 +1000
Message-Id: <CQ053TUZQIPP.1OHV7MVS4F4HT@bobo>
To:     "Andrew Donnellan" <ajd@linux.ibm.com>,
        <linuxppc-dev@lists.ozlabs.org>, <linux-integrity@vger.kernel.org>
Cc:     <sudhakar@linux.ibm.com>, <bgray@linux.ibm.com>,
        <erichte@linux.ibm.com>, <gregkh@linuxfoundation.org>,
        <nayna@linux.ibm.com>, <linux-kernel@vger.kernel.org>,
        <zohar@linux.ibm.com>, <gjoyce@linux.ibm.com>, <ruscur@russell.cc>,
        <gcwilson@linux.ibm.com>, <joel@jms.id.au>
Subject: Re: [PATCH v4 21/24] powerpc/pseries: Pass PLPKS password on kexec
From:   "Nicholas Piggin" <npiggin@gmail.com>
References: <20230120074306.1326298-1-ajd@linux.ibm.com>
 <20230120074306.1326298-22-ajd@linux.ibm.com>
In-Reply-To: <20230120074306.1326298-22-ajd@linux.ibm.com>
Precedence: bulk

On Fri Jan 20, 2023 at 5:43 PM AEST, Andrew Donnellan wrote:
> From: Russell Currey <ruscur@russell.cc>
>
> 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@russell.cc>
> Signed-off-by: Andrew Donnellan <ajd@linux.ibm.com>
>
> ---
>
> v3: New patch
>
> v4: Fix compile when CONFIG_PSERIES_PLPKS=3Dn (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>
> =20
>  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 =3D NULL, *rmem =3D 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.

> =20
>  	/*
>  	 * Restrict memory usage for kdump kernel by setting up
> @@ -1230,6 +1234,20 @@ int setup_new_fdt_ppc64(const struct kimage *image=
, void *fdt,
>  		}
>  	}
> =20
> +#ifdef CONFIG_PSERIES_PLPKS
> +	// If we have PLPKS active, we need to provide the password
> +	if (plpks_is_available()) {
> +		chosen_offset =3D 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 =3D 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/platfo=
rms/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] =3D { 0 };
>  	u8 *password, consumer =3D PLPKS_OS_OWNER;
> -	int rc;
> +	struct property *prop;
> +	int rc, len;
> +
> +	// Before we generate the password, we may have been booted by kexec an=
d
> +	// 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 =3D of_find_property(of_chosen, "ibm,plpks-pw", &len);
> +	if (prop) {
> +		ospasswordlength =3D (u16)len;
> +		ospassword =3D 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