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

[Russell Currey]

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