diff mbox series

[v4,16/24] powerpc/pseries: Implement signed update for PLPKS objects

Message ID 20230120074306.1326298-17-ajd@linux.ibm.com (mailing list archive)
State New, archived
Headers show
Series pSeries dynamic secure boot secvar interface + platform keyring loading | expand

Commit Message

Andrew Donnellan Jan. 20, 2023, 7:42 a.m. UTC
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, add timeout handling and misc cleanups]
Co-developed-by: Andrew Donnellan <ajd@linux.ibm.com>
Signed-off-by: Andrew Donnellan <ajd@linux.ibm.com>
Signed-off-by: Russell Currey <ruscur@russell.cc>

---

v3: Merge plpks fixes and signed update series with secvar series

    Fix error code handling in plpks_confirm_object_flushed() (ruscur)

    Pass plpks_var struct to plpks_signed_update_var() by reference (mpe)

    Consistent constant naming scheme (ruscur)

v4: Fix MAX_HCALL_OPCODE rebasing issue (npiggin)
---
 arch/powerpc/include/asm/hvcall.h      |  1 +
 arch/powerpc/include/asm/plpks.h       |  5 ++
 arch/powerpc/platforms/pseries/plpks.c | 71 ++++++++++++++++++++++++--
 3 files changed, 72 insertions(+), 5 deletions(-)

Comments

Nicholas Piggin Jan. 24, 2023, 4:16 a.m. UTC | #1
On Fri Jan 20, 2023 at 5:42 PM AEST, 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, add timeout handling and misc cleanups]
> Co-developed-by: Andrew Donnellan <ajd@linux.ibm.com>
> Signed-off-by: Andrew Donnellan <ajd@linux.ibm.com>
> Signed-off-by: Russell Currey <ruscur@russell.cc>
>
> ---
>
> v3: Merge plpks fixes and signed update series with secvar series
>
>     Fix error code handling in plpks_confirm_object_flushed() (ruscur)
>
>     Pass plpks_var struct to plpks_signed_update_var() by reference (mpe)
>
>     Consistent constant naming scheme (ruscur)
>
> v4: Fix MAX_HCALL_OPCODE rebasing issue (npiggin)
> ---
>  arch/powerpc/include/asm/hvcall.h      |  1 +
>  arch/powerpc/include/asm/plpks.h       |  5 ++
>  arch/powerpc/platforms/pseries/plpks.c | 71 ++++++++++++++++++++++++--
>  3 files changed, 72 insertions(+), 5 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/hvcall.h b/arch/powerpc/include/asm/hvcall.h
> index 95fd7f9485d5..c099780385dd 100644
> --- a/arch/powerpc/include/asm/hvcall.h
> +++ b/arch/powerpc/include/asm/hvcall.h
> @@ -335,6 +335,7 @@
>  #define H_RPT_INVALIDATE	0x448
>  #define H_SCM_FLUSH		0x44C
>  #define H_GET_ENERGY_SCALE_INFO	0x450
> +#define H_PKS_SIGNED_UPDATE	0x454
>  #define H_WATCHDOG		0x45C
>  #define MAX_HCALL_OPCODE	H_WATCHDOG
>  
> diff --git a/arch/powerpc/include/asm/plpks.h b/arch/powerpc/include/asm/plpks.h
> index 7c5f51a9af7c..e7204e6c0ca4 100644
> --- a/arch/powerpc/include/asm/plpks.h
> +++ b/arch/powerpc/include/asm/plpks.h
> @@ -68,6 +68,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
> diff --git a/arch/powerpc/platforms/pseries/plpks.c b/arch/powerpc/platforms/pseries/plpks.c
> index 1189246b03dc..796ed5544ee5 100644
> --- a/arch/powerpc/platforms/pseries/plpks.c
> +++ b/arch/powerpc/platforms/pseries/plpks.c
> @@ -81,6 +81,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:

This is a bit sad to maintain here. It's duplicating bits with
hvcs_convert, and a bunch of open coded places. Probably not the
series to do anything about. Would be nice if we could standardise
it though.

> @@ -184,14 +190,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 > PLPKS_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);
> +	}

Is this already a bug? Code checks for component != NULL but previously
calls strlen which would oops on NULL component AFAIKS. Granted nothing
is actually using any of this these days.

It already seems like it's supposed to be allowed to rad NULL component
with read_var though? Why the differences, why not always allow NULL
component? (I assume there is some reason, I just don't know anything
about secvar or secure boot).

>  
>  	// 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);
> @@ -397,6 +406,58 @@ static int plpks_confirm_object_flushed(struct label *label,
>  	return pseries_status_to_err(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 > PLPKS_MAX_NAME_SIZE)
> +		return -EINVAL;
> +
> +	if (!(var->policy & PLPKS_SIGNEDUPDATE))
> +		return -EINVAL;
> +
> +	auth = construct_auth(PLPKS_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 < PLPKS_MAX_TIMEOUT);
> +
> +	if (!rc)
> +		rc = plpks_confirm_object_flushed(label, auth);
> +
> +	kfree(label);
> +out:
> +	kfree(auth);
> +
> +	return rc;
> +}
> +EXPORT_SYMBOL(plpks_signed_update_var);

Sorry I missed it before -- can this be a _GPL export?

Thanks,
Nick
Andrew Donnellan Jan. 30, 2023, 4:43 a.m. UTC | #2
On Tue, 2023-01-24 at 14:16 +1000, Nicholas Piggin wrote:
> > diff --git a/arch/powerpc/platforms/pseries/plpks.c
> > b/arch/powerpc/platforms/pseries/plpks.c
> > index 1189246b03dc..796ed5544ee5 100644
> > --- a/arch/powerpc/platforms/pseries/plpks.c
> > +++ b/arch/powerpc/platforms/pseries/plpks.c
> > @@ -81,6 +81,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:
> 
> This is a bit sad to maintain here. It's duplicating bits with
> hvcs_convert, and a bunch of open coded places. Probably not the
> series to do anything about. Would be nice if we could standardise
> it though.

Agreed - though we're not going to touch it in this series.

> 
> > @@ -184,14 +190,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 > PLPKS_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);
> > +       }
> 
> Is this already a bug? Code checks for component != NULL but
> previously
> calls strlen which would oops on NULL component AFAIKS. Granted
> nothing
> is actually using any of this these days.

True, it should have been checking for NULL first, but as you say no-
one is using it.

> 
> It already seems like it's supposed to be allowed to rad NULL
> component
> with read_var though? Why the differences, why not always allow NULL
> component? (I assume there is some reason, I just don't know anything
> about secvar or secure boot).

I think the comment confuses more than it clarifies, I'll remove it.

As you say, read_var() should work fine with component == NULL, though
write_var() checks it. The only rule I can find in the spec is that
signed update calls *must* set the component to NULL. I'm seeking
clarification on that.

> > +EXPORT_SYMBOL(plpks_signed_update_var);
> 
> Sorry I missed it before -- can this be a _GPL export?

Indeed it should be - actually, I should check if I can get rid of the
export completely...
Russell Currey Jan. 31, 2023, 4:23 a.m. UTC | #3
On Mon, 2023-01-30 at 15:43 +1100, Andrew Donnellan wrote:
> On Tue, 2023-01-24 at 14:16 +1000, Nicholas Piggin wrote:
> > > diff --git a/arch/powerpc/platforms/pseries/plpks.c
> > > b/arch/powerpc/platforms/pseries/plpks.c
> > > index 1189246b03dc..796ed5544ee5 100644
> > > --- a/arch/powerpc/platforms/pseries/plpks.c
> > > +++ b/arch/powerpc/platforms/pseries/plpks.c
> > > @@ -81,6 +81,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:
> > 
> > This is a bit sad to maintain here. It's duplicating bits with
> > hvcs_convert, and a bunch of open coded places. Probably not the
> > series to do anything about. Would be nice if we could standardise
> > it though.
> 
> Agreed - though we're not going to touch it in this series.
> 
> > 
> > > @@ -184,14 +190,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 > PLPKS_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);
> > > +       }
> > 
> > Is this already a bug? Code checks for component != NULL but
> > previously
> > calls strlen which would oops on NULL component AFAIKS. Granted
> > nothing
> > is actually using any of this these days.
> 
> True, it should have been checking for NULL first, but as you say no-
> one is using it.
> 
> > 
> > It already seems like it's supposed to be allowed to rad NULL
> > component
> > with read_var though? Why the differences, why not always allow
> > NULL
> > component? (I assume there is some reason, I just don't know
> > anything
> > about secvar or secure boot).
> 
> I think the comment confuses more than it clarifies, I'll remove it.
> 
> As you say, read_var() should work fine with component == NULL,
> though
> write_var() checks it. The only rule I can find in the spec is that
> signed update calls *must* set the component to NULL. I'm seeking
> clarification on that.

Signed update calls *must* set the component to NULL.

We could just call construct_label() with NULL as the component
directly but I think it's better to explicitly check var->component and
return so the caller knows what they're trying to do is wrong.

> 
> > > +EXPORT_SYMBOL(plpks_signed_update_var);
> > 
> > Sorry I missed it before -- can this be a _GPL export?
> 
> Indeed it should be - actually, I should check if I can get rid of
> the
> export completely...
>
diff mbox series

Patch

diff --git a/arch/powerpc/include/asm/hvcall.h b/arch/powerpc/include/asm/hvcall.h
index 95fd7f9485d5..c099780385dd 100644
--- a/arch/powerpc/include/asm/hvcall.h
+++ b/arch/powerpc/include/asm/hvcall.h
@@ -335,6 +335,7 @@ 
 #define H_RPT_INVALIDATE	0x448
 #define H_SCM_FLUSH		0x44C
 #define H_GET_ENERGY_SCALE_INFO	0x450
+#define H_PKS_SIGNED_UPDATE	0x454
 #define H_WATCHDOG		0x45C
 #define MAX_HCALL_OPCODE	H_WATCHDOG
 
diff --git a/arch/powerpc/include/asm/plpks.h b/arch/powerpc/include/asm/plpks.h
index 7c5f51a9af7c..e7204e6c0ca4 100644
--- a/arch/powerpc/include/asm/plpks.h
+++ b/arch/powerpc/include/asm/plpks.h
@@ -68,6 +68,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
diff --git a/arch/powerpc/platforms/pseries/plpks.c b/arch/powerpc/platforms/pseries/plpks.c
index 1189246b03dc..796ed5544ee5 100644
--- a/arch/powerpc/platforms/pseries/plpks.c
+++ b/arch/powerpc/platforms/pseries/plpks.c
@@ -81,6 +81,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:
@@ -184,14 +190,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 > PLPKS_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);
@@ -397,6 +406,58 @@  static int plpks_confirm_object_flushed(struct label *label,
 	return pseries_status_to_err(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 > PLPKS_MAX_NAME_SIZE)
+		return -EINVAL;
+
+	if (!(var->policy & PLPKS_SIGNEDUPDATE))
+		return -EINVAL;
+
+	auth = construct_auth(PLPKS_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 < PLPKS_MAX_TIMEOUT);
+
+	if (!rc)
+		rc = plpks_confirm_object_flushed(label, auth);
+
+	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 };
@@ -443,7 +504,7 @@  int plpks_remove_var(char *component, u8 varos, struct plpks_var_name vname)
 	struct label *label;
 	int rc;
 
-	if (!component || vname.namelen > PLPKS_MAX_NAME_SIZE)
+	if (vname.namelen > PLPKS_MAX_NAME_SIZE)
 		return -EINVAL;
 
 	auth = construct_auth(PLPKS_OS_OWNER);