| Message ID | 20221220071626.1426786-5-ajd@linux.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PLPKS bugfixes and enhancements | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/github-powerpc_ppctests | success | Successfully ran 8 jobs. |
| snowpatch_ozlabs/github-powerpc_selftests | success | Successfully ran 8 jobs. |
| snowpatch_ozlabs/github-powerpc_sparse | success | Successfully ran 4 jobs. |
| snowpatch_ozlabs/github-powerpc_kernel_qemu | success | Successfully ran 24 jobs. |
| snowpatch_ozlabs/github-powerpc_clang | success | Successfully ran 6 jobs. |
On Tue, 2022-12-20 at 18:16 +1100, Andrew Donnellan wrote: > From: Nayna Jain <nayna@linux.ibm.com> > > The Platform Keystore provides a signed update interface which can be > used > to create, replace or append to certain variables in the PKS in a > secure > fashion, with the hypervisor requiring that the update be signed > using the > Platform Key. > > Implement an interface to the H_PKS_SIGNED_UPDATE hcall in the plpks > driver to allow signed updates to PKS objects. > > (The plpks driver doesn't need to do any cryptography or otherwise > handle > the actual signed variable contents - that will be handled by > userspace > tooling.) > > Signed-off-by: Nayna Jain <nayna@linux.ibm.com> > [ajd: split patch, rewrite commit message, add timeout handling] > Co-developed-by: Andrew Donnellan <ajd@linux.ibm.com> > Signed-off-by: Andrew Donnellan <ajd@linux.ibm.com> > --- > arch/powerpc/include/asm/hvcall.h | 3 +- > arch/powerpc/platforms/pseries/plpks.c | 81 +++++++++++++++++++++++- > -- > arch/powerpc/platforms/pseries/plpks.h | 5 ++ > 3 files changed, 79 insertions(+), 10 deletions(-) > > diff --git a/arch/powerpc/include/asm/hvcall.h > b/arch/powerpc/include/asm/hvcall.h > index 95fd7f9485d5..33b26c0cb69b 100644 > --- a/arch/powerpc/include/asm/hvcall.h > +++ b/arch/powerpc/include/asm/hvcall.h > @@ -336,7 +336,8 @@ > #define H_SCM_FLUSH 0x44C > #define H_GET_ENERGY_SCALE_INFO 0x450 > #define H_WATCHDOG 0x45C > -#define MAX_HCALL_OPCODE H_WATCHDOG > +#define H_PKS_SIGNED_UPDATE 0x454 > +#define MAX_HCALL_OPCODE H_PKS_SIGNED_UPDATE > > /* Scope args for H_SCM_UNBIND_ALL */ > #define H_UNBIND_SCOPE_ALL (0x1) > diff --git a/arch/powerpc/platforms/pseries/plpks.c > b/arch/powerpc/platforms/pseries/plpks.c > index c5ae00a8a968..9e4401aabf4f 100644 > --- a/arch/powerpc/platforms/pseries/plpks.c > +++ b/arch/powerpc/platforms/pseries/plpks.c > @@ -30,9 +30,9 @@ > #define MAX_NAME_SIZE 239 > #define MAX_DATA_SIZE 4000 > > -#define PKS_FLUSH_MAX_TIMEOUT 5000 //msec > -#define PKS_FLUSH_SLEEP 10 //msec > -#define PKS_FLUSH_SLEEP_RANGE 400 > +#define PKS_MAX_TIMEOUT 5000 // msec > +#define PKS_FLUSH_SLEEP 10 // msec > +#define PKS_FLUSH_SLEEP_RANGE 400 > > static u8 *ospassword; > static u16 ospasswordlength; > @@ -95,6 +95,12 @@ static int pseries_status_to_err(int rc) > err = -ENOENT; > break; > case H_BUSY: > + case H_LONG_BUSY_ORDER_1_MSEC: > + case H_LONG_BUSY_ORDER_10_MSEC: > + case H_LONG_BUSY_ORDER_100_MSEC: > + case H_LONG_BUSY_ORDER_1_SEC: > + case H_LONG_BUSY_ORDER_10_SEC: > + case H_LONG_BUSY_ORDER_100_SEC: > err = -EBUSY; > break; > case H_AUTHORITY: > @@ -198,14 +204,17 @@ static struct label *construct_label(char > *component, u8 varos, u8 *name, > u16 namelen) > { > struct label *label; > - size_t slen; > + size_t slen = 0; > > if (!name || namelen > MAX_NAME_SIZE) > return ERR_PTR(-EINVAL); > > - slen = strlen(component); > - if (component && slen > sizeof(label->attr.prefix)) > - return ERR_PTR(-EINVAL); > + // Support NULL component for signed updates > + if (component) { > + slen = strlen(component); > + if (slen > sizeof(label->attr.prefix)) > + return ERR_PTR(-EINVAL); > + } > > // The label structure must not cross a page boundary, so we > align to the next power of 2 > label = kzalloc(roundup_pow_of_two(sizeof(*label)), > GFP_KERNEL); > @@ -372,7 +381,7 @@ static int plpks_confirm_object_flushed(struct > label *label, > usleep_range(PKS_FLUSH_SLEEP, > PKS_FLUSH_SLEEP + > PKS_FLUSH_SLEEP_RANGE); > timeout = timeout + PKS_FLUSH_SLEEP; > - } while (timeout < PKS_FLUSH_MAX_TIMEOUT); > + } while (timeout < PKS_MAX_TIMEOUT); > > if (timed_out) > rc = -ETIMEDOUT; > @@ -382,6 +391,60 @@ static int plpks_confirm_object_flushed(struct > label *label, > return rc; > } > > +int plpks_signed_update_var(struct plpks_var var, u64 flags) > +{ > + unsigned long retbuf[PLPAR_HCALL9_BUFSIZE] = {0}; > + int rc; > + struct label *label; > + struct plpks_auth *auth; > + u64 continuetoken = 0; > + u64 timeout = 0; > + > + if (!var.data || var.datalen <= 0 || var.namelen > > MAX_NAME_SIZE) > + return -EINVAL; > + > + if (!(var.policy & SIGNEDUPDATE)) > + return -EINVAL; > + > + auth = construct_auth(PKS_OS_OWNER); > + if (IS_ERR(auth)) > + return PTR_ERR(auth); > + > + label = construct_label(var.component, var.os, var.name, > var.namelen); > + if (IS_ERR(label)) { > + rc = PTR_ERR(label); > + goto out; > + } > + > + do { > + rc = plpar_hcall9(H_PKS_SIGNED_UPDATE, retbuf, > + virt_to_phys(auth), > virt_to_phys(label), > + label->size, var.policy, flags, > + virt_to_phys(var.data), > var.datalen, > + continuetoken); > + > + continuetoken = retbuf[0]; > + if (pseries_status_to_err(rc) == -EBUSY) { > + int delay_ms = get_longbusy_msecs(rc); > + mdelay(delay_ms); > + timeout += delay_ms; > + } > + rc = pseries_status_to_err(rc); > + } while (rc == -EBUSY && timeout < PKS_MAX_TIMEOUT); > + > + if (!rc) { > + rc = plpks_confirm_object_flushed(label, auth); > + rc = pseries_status_to_err(rc); Doesn't plpks_confirm_object_flushed() already return a Linux-friendly error code? If I'm reading this right, we'd be replacing any non-zero return code with -EINVAL. > + } > + > + kfree(label); > +out: > + kfree(auth); > + > + return rc; > +} > +EXPORT_SYMBOL(plpks_signed_update_var); > + > int plpks_write_var(struct plpks_var var) > { > unsigned long retbuf[PLPAR_HCALL_BUFSIZE] = { 0 }; > @@ -428,7 +491,7 @@ int plpks_remove_var(char *component, u8 varos, > struct plpks_var_name vname) > struct label *label; > int rc; > > - if (!component || vname.namelen > MAX_NAME_SIZE) > + if (vname.namelen > MAX_NAME_SIZE) > return -EINVAL; > > auth = construct_auth(PKS_OS_OWNER); > diff --git a/arch/powerpc/platforms/pseries/plpks.h > b/arch/powerpc/platforms/pseries/plpks.h > index c89740796660..33cf12809392 100644 > --- a/arch/powerpc/platforms/pseries/plpks.h > +++ b/arch/powerpc/platforms/pseries/plpks.h > @@ -40,6 +40,11 @@ struct plpks_var_name_list { > struct plpks_var_name varlist[]; > }; > > +/** > + * Updates the authenticated variable. It expects NULL as the > component. > + */ > +int plpks_signed_update_var(struct plpks_var var, u64 flags); > + > /** > * Writes the specified var and its data to PKS. > * Any caller of PKS driver should present a valid component type > for
On Wed, 2023-01-04 at 15:04 +1100, Russell Currey wrote: > > + if (!rc) { > > + rc = plpks_confirm_object_flushed(label, auth); > > + rc = pseries_status_to_err(rc); > > Doesn't plpks_confirm_object_flushed() already return a Linux- > friendly > error code? If I'm reading this right, we'd be replacing any non- > zero > return code with -EINVAL. Good catch, will fix.
Andrew Donnellan <ajd@linux.ibm.com> writes: > From: Nayna Jain <nayna@linux.ibm.com> > > The Platform Keystore provides a signed update interface which can be used > to create, replace or append to certain variables in the PKS in a secure > fashion, with the hypervisor requiring that the update be signed using the > Platform Key. > > Implement an interface to the H_PKS_SIGNED_UPDATE hcall in the plpks > driver to allow signed updates to PKS objects. > > (The plpks driver doesn't need to do any cryptography or otherwise handle > the actual signed variable contents - that will be handled by userspace > tooling.) > > Signed-off-by: Nayna Jain <nayna@linux.ibm.com> > [ajd: split patch, rewrite commit message, add timeout handling] > Co-developed-by: Andrew Donnellan <ajd@linux.ibm.com> > Signed-off-by: Andrew Donnellan <ajd@linux.ibm.com> > --- > arch/powerpc/include/asm/hvcall.h | 3 +- > arch/powerpc/platforms/pseries/plpks.c | 81 +++++++++++++++++++++++--- > arch/powerpc/platforms/pseries/plpks.h | 5 ++ > 3 files changed, 79 insertions(+), 10 deletions(-) > > diff --git a/arch/powerpc/include/asm/hvcall.h b/arch/powerpc/include/asm/hvcall.h > index 95fd7f9485d5..33b26c0cb69b 100644 > --- a/arch/powerpc/include/asm/hvcall.h > +++ b/arch/powerpc/include/asm/hvcall.h > @@ -336,7 +336,8 @@ > #define H_SCM_FLUSH 0x44C > #define H_GET_ENERGY_SCALE_INFO 0x450 > #define H_WATCHDOG 0x45C > -#define MAX_HCALL_OPCODE H_WATCHDOG > +#define H_PKS_SIGNED_UPDATE 0x454 > +#define MAX_HCALL_OPCODE H_PKS_SIGNED_UPDATE > > /* Scope args for H_SCM_UNBIND_ALL */ > #define H_UNBIND_SCOPE_ALL (0x1) > diff --git a/arch/powerpc/platforms/pseries/plpks.c b/arch/powerpc/platforms/pseries/plpks.c > index c5ae00a8a968..9e4401aabf4f 100644 > --- a/arch/powerpc/platforms/pseries/plpks.c > +++ b/arch/powerpc/platforms/pseries/plpks.c > @@ -30,9 +30,9 @@ > #define MAX_NAME_SIZE 239 > #define MAX_DATA_SIZE 4000 > > -#define PKS_FLUSH_MAX_TIMEOUT 5000 //msec > -#define PKS_FLUSH_SLEEP 10 //msec > -#define PKS_FLUSH_SLEEP_RANGE 400 > +#define PKS_MAX_TIMEOUT 5000 // msec > +#define PKS_FLUSH_SLEEP 10 // msec > +#define PKS_FLUSH_SLEEP_RANGE 400 > > static u8 *ospassword; > static u16 ospasswordlength; > @@ -95,6 +95,12 @@ static int pseries_status_to_err(int rc) > err = -ENOENT; > break; > case H_BUSY: > + case H_LONG_BUSY_ORDER_1_MSEC: > + case H_LONG_BUSY_ORDER_10_MSEC: > + case H_LONG_BUSY_ORDER_100_MSEC: > + case H_LONG_BUSY_ORDER_1_SEC: > + case H_LONG_BUSY_ORDER_10_SEC: > + case H_LONG_BUSY_ORDER_100_SEC: > err = -EBUSY; > break; > case H_AUTHORITY: > @@ -198,14 +204,17 @@ static struct label *construct_label(char *component, u8 varos, u8 *name, > u16 namelen) > { > struct label *label; > - size_t slen; > + size_t slen = 0; > > if (!name || namelen > MAX_NAME_SIZE) > return ERR_PTR(-EINVAL); > > - slen = strlen(component); > - if (component && slen > sizeof(label->attr.prefix)) > - return ERR_PTR(-EINVAL); > + // Support NULL component for signed updates > + if (component) { > + slen = strlen(component); > + if (slen > sizeof(label->attr.prefix)) > + return ERR_PTR(-EINVAL); > + } > > // The label structure must not cross a page boundary, so we align to the next power of 2 > label = kzalloc(roundup_pow_of_two(sizeof(*label)), GFP_KERNEL); > @@ -372,7 +381,7 @@ static int plpks_confirm_object_flushed(struct label *label, > usleep_range(PKS_FLUSH_SLEEP, > PKS_FLUSH_SLEEP + PKS_FLUSH_SLEEP_RANGE); > timeout = timeout + PKS_FLUSH_SLEEP; > - } while (timeout < PKS_FLUSH_MAX_TIMEOUT); > + } while (timeout < PKS_MAX_TIMEOUT); > > if (timed_out) > rc = -ETIMEDOUT; > @@ -382,6 +391,60 @@ static int plpks_confirm_object_flushed(struct label *label, > return rc; > } > > +int plpks_signed_update_var(struct plpks_var var, u64 flags) > +{ I don't see a reason why var is passed by value here? A pointer would be more typical. cheers
On Fri, 2023-01-06 at 21:54 +1100, Michael Ellerman wrote: > > +int plpks_signed_update_var(struct plpks_var var, u64 flags) > > +{ > > I don't see a reason why var is passed by value here? A pointer would > be > more typical. Will change.
diff --git a/arch/powerpc/include/asm/hvcall.h b/arch/powerpc/include/asm/hvcall.h index 95fd7f9485d5..33b26c0cb69b 100644 --- a/arch/powerpc/include/asm/hvcall.h +++ b/arch/powerpc/include/asm/hvcall.h @@ -336,7 +336,8 @@ #define H_SCM_FLUSH 0x44C #define H_GET_ENERGY_SCALE_INFO 0x450 #define H_WATCHDOG 0x45C -#define MAX_HCALL_OPCODE H_WATCHDOG +#define H_PKS_SIGNED_UPDATE 0x454 +#define MAX_HCALL_OPCODE H_PKS_SIGNED_UPDATE /* Scope args for H_SCM_UNBIND_ALL */ #define H_UNBIND_SCOPE_ALL (0x1) diff --git a/arch/powerpc/platforms/pseries/plpks.c b/arch/powerpc/platforms/pseries/plpks.c index c5ae00a8a968..9e4401aabf4f 100644 --- a/arch/powerpc/platforms/pseries/plpks.c +++ b/arch/powerpc/platforms/pseries/plpks.c @@ -30,9 +30,9 @@ #define MAX_NAME_SIZE 239 #define MAX_DATA_SIZE 4000 -#define PKS_FLUSH_MAX_TIMEOUT 5000 //msec -#define PKS_FLUSH_SLEEP 10 //msec -#define PKS_FLUSH_SLEEP_RANGE 400 +#define PKS_MAX_TIMEOUT 5000 // msec +#define PKS_FLUSH_SLEEP 10 // msec +#define PKS_FLUSH_SLEEP_RANGE 400 static u8 *ospassword; static u16 ospasswordlength; @@ -95,6 +95,12 @@ static int pseries_status_to_err(int rc) err = -ENOENT; break; case H_BUSY: + case H_LONG_BUSY_ORDER_1_MSEC: + case H_LONG_BUSY_ORDER_10_MSEC: + case H_LONG_BUSY_ORDER_100_MSEC: + case H_LONG_BUSY_ORDER_1_SEC: + case H_LONG_BUSY_ORDER_10_SEC: + case H_LONG_BUSY_ORDER_100_SEC: err = -EBUSY; break; case H_AUTHORITY: @@ -198,14 +204,17 @@ static struct label *construct_label(char *component, u8 varos, u8 *name, u16 namelen) { struct label *label; - size_t slen; + size_t slen = 0; if (!name || namelen > MAX_NAME_SIZE) return ERR_PTR(-EINVAL); - slen = strlen(component); - if (component && slen > sizeof(label->attr.prefix)) - return ERR_PTR(-EINVAL); + // Support NULL component for signed updates + if (component) { + slen = strlen(component); + if (slen > sizeof(label->attr.prefix)) + return ERR_PTR(-EINVAL); + } // The label structure must not cross a page boundary, so we align to the next power of 2 label = kzalloc(roundup_pow_of_two(sizeof(*label)), GFP_KERNEL); @@ -372,7 +381,7 @@ static int plpks_confirm_object_flushed(struct label *label, usleep_range(PKS_FLUSH_SLEEP, PKS_FLUSH_SLEEP + PKS_FLUSH_SLEEP_RANGE); timeout = timeout + PKS_FLUSH_SLEEP; - } while (timeout < PKS_FLUSH_MAX_TIMEOUT); + } while (timeout < PKS_MAX_TIMEOUT); if (timed_out) rc = -ETIMEDOUT; @@ -382,6 +391,60 @@ static int plpks_confirm_object_flushed(struct label *label, return rc; } +int plpks_signed_update_var(struct plpks_var var, u64 flags) +{ + unsigned long retbuf[PLPAR_HCALL9_BUFSIZE] = {0}; + int rc; + struct label *label; + struct plpks_auth *auth; + u64 continuetoken = 0; + u64 timeout = 0; + + if (!var.data || var.datalen <= 0 || var.namelen > MAX_NAME_SIZE) + return -EINVAL; + + if (!(var.policy & SIGNEDUPDATE)) + return -EINVAL; + + auth = construct_auth(PKS_OS_OWNER); + if (IS_ERR(auth)) + return PTR_ERR(auth); + + label = construct_label(var.component, var.os, var.name, var.namelen); + if (IS_ERR(label)) { + rc = PTR_ERR(label); + goto out; + } + + do { + rc = plpar_hcall9(H_PKS_SIGNED_UPDATE, retbuf, + virt_to_phys(auth), virt_to_phys(label), + label->size, var.policy, flags, + virt_to_phys(var.data), var.datalen, + continuetoken); + + continuetoken = retbuf[0]; + if (pseries_status_to_err(rc) == -EBUSY) { + int delay_ms = get_longbusy_msecs(rc); + mdelay(delay_ms); + timeout += delay_ms; + } + rc = pseries_status_to_err(rc); + } while (rc == -EBUSY && timeout < PKS_MAX_TIMEOUT); + + if (!rc) { + rc = plpks_confirm_object_flushed(label, auth); + rc = pseries_status_to_err(rc); + } + + kfree(label); +out: + kfree(auth); + + return rc; +} +EXPORT_SYMBOL(plpks_signed_update_var); + int plpks_write_var(struct plpks_var var) { unsigned long retbuf[PLPAR_HCALL_BUFSIZE] = { 0 }; @@ -428,7 +491,7 @@ int plpks_remove_var(char *component, u8 varos, struct plpks_var_name vname) struct label *label; int rc; - if (!component || vname.namelen > MAX_NAME_SIZE) + if (vname.namelen > MAX_NAME_SIZE) return -EINVAL; auth = construct_auth(PKS_OS_OWNER); diff --git a/arch/powerpc/platforms/pseries/plpks.h b/arch/powerpc/platforms/pseries/plpks.h index c89740796660..33cf12809392 100644 --- a/arch/powerpc/platforms/pseries/plpks.h +++ b/arch/powerpc/platforms/pseries/plpks.h @@ -40,6 +40,11 @@ struct plpks_var_name_list { struct plpks_var_name varlist[]; }; +/** + * Updates the authenticated variable. It expects NULL as the component. + */ +int plpks_signed_update_var(struct plpks_var var, u64 flags); + /** * Writes the specified var and its data to PKS. * Any caller of PKS driver should present a valid component type for