Re: [PATCH v4 21/24] powerpc/pseries: Pass PLPKS password on kexec
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
> >
> > 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
> > Signed-off-by: Andrew Donnellan
> >
> > ---
> >
> > 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
> > #include
> > #include
> > +#include
> >
> > 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
> > #include
> > #include
> > +#include
> > #include
> > #include
> > #include
> > @@ -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", );
> > + 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
Re: [PATCH v4 21/24] powerpc/pseries: Pass PLPKS password on kexec
Andrew Donnellan writes:
> On Tue, 2023-01-24 at 14:36 +1000, Nicholas Piggin wrote:
>>
>> > + prop = of_find_property(of_chosen, "ibm,plpks-pw", );
>> > + 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?
>
> Because otherwise the password will be sitting around in /proc/device-
> tree for the world to go and read.
The above removes it from the unflattened tree, but it will still exist
in the flattened tree, which is readable by root in /sys/firmware/fdt.
I'm not sure if that's a major security problem, but it does seem risky.
To scrub it from the flat tree you'd need to have an early_init_dt style
routine that finds the password early, saves the value somewhere for the
plpks driver, and then zeroes out the value in the flat tree. See the
example of rng-seed in early_init_dt_scan_chosen().
cheers
Re: [PATCH v4 21/24] powerpc/pseries: Pass PLPKS password on kexec
On Tue, 2023-01-24 at 14:36 +1000, Nicholas Piggin wrote:
>
> > + prop = of_find_property(of_chosen, "ibm,plpks-pw", );
> > + 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?
Because otherwise the password will be sitting around in /proc/device-
tree for the world to go and read.
--
Andrew DonnellanOzLabs, ADL Canberra
a...@linux.ibm.com IBM Australia Limited
Re: [PATCH v4 21/24] powerpc/pseries: Pass PLPKS password on kexec
On Fri Jan 20, 2023 at 5:43 PM AEST, Andrew Donnellan wrote:
> From: Russell Currey
>
> 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
> Signed-off-by: Andrew Donnellan
>
> ---
>
> 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
> #include
> #include
> +#include
>
> 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
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -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", );
> + 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
