On Tue, 2023-01-24 at 14:36 +1000, Nicholas Piggin wrote:
> 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
Good catch, thanks
>
> > 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.
Yep, moving this out, though not into plpks_is_available().
>
> >
> > /*
> > * 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.
I'm moving this into a helper in plpks.c since now there's FDT handling
in early boot in there. We can drop plpks_get_password() 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?
Yes this should have been separate, and now has to be anyway since
we're retrieving the password from the FDT in early boot.
>
> > + 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?
As Andrew mentioned, so we don't have a password lingering in the
device tree, though it's not especially useful. We're going to get it
and clear it from the FDT in early boot instead.
>
> Thanks,
> Nick
