[PATCH] tpm: only export stand alone version of flush context command

[PATCH] tpm: only export stand alone version of flush context command

[James Bottomley]

Remove the currently exported version of flush context, which is
designed for tpm core internal use only and substitute a corrected
version that does the necessary tpm ops get/put.  This fixes a bug
with trusted keys in that some TIS TPMs are unable to flush the
loaded secret because the status register isn't reading correctly.

Fixes: 45477b3fe3d1 ("security: keys: trusted: fix lost handle flush")
Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 drivers/char/tpm/tpm.h                    |  1 +
 drivers/char/tpm/tpm2-cmd.c               | 23 ++++++++++++++++++++++-
 include/linux/tpm.h                       |  2 +-
 security/keys/trusted-keys/trusted_tpm2.c |  2 +-
 4 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 21ac88d4076c..cba09be7ce23 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -240,6 +240,7 @@ int tpm2_prepare_space(struct tpm_chip *chip, struct tpm_space *space, u8 *cmd,
 		       size_t cmdsiz);
 int tpm2_commit_space(struct tpm_chip *chip, struct tpm_space *space, void *buf,
 		      size_t *bufsiz);
+void tpm2_flush_context(struct tpm_chip *chip, u32 handle);
 
 void tpm_bios_log_setup(struct tpm_chip *chip);
 void tpm_bios_log_teardown(struct tpm_chip *chip);
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index 9b84158c5a9e..d5aaea72d578 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -362,7 +362,28 @@ void tpm2_flush_context(struct tpm_chip *chip, u32 handle)
 	tpm_transmit_cmd(chip, &buf, 0, "flushing context");
 	tpm_buf_destroy(&buf);
 }
-EXPORT_SYMBOL_GPL(tpm2_flush_context);
+
+/**
+ * tpm2_flush_context_cmd() - execute a TPM2_FlushContext command
+ * @chip:	TPM chip to use
+ * @handle:	context handle
+ *
+ * This version of the command is designed to be used outside the
+ * TPM core so acquires and releases the tpm ops.
+ */
+void tpm2_flush_context_cmd(struct tpm_chip *chip, u32 handle)
+{
+	int rc;
+
+	rc = tpm_try_get_ops(chip);
+	if (rc) {
+		dev_err(&chip->dev, "Failed to acquire tpm ops for flush\n");
+		return;
+	}
+	tpm2_flush_context(chip, handle);
+	tpm_put_ops(chip);
+}
+EXPORT_SYMBOL_GPL(tpm2_flush_context_cmd);
 
 struct tpm2_get_cap_out {
 	u8 more_data;
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index ae2482510f8c..c4ca52138e8b 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -411,7 +411,7 @@ extern int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
 extern int tpm_send(struct tpm_chip *chip, void *cmd, size_t buflen);
 extern int tpm_get_random(struct tpm_chip *chip, u8 *data, size_t max);
 extern struct tpm_chip *tpm_default_chip(void);
-void tpm2_flush_context(struct tpm_chip *chip, u32 handle);
+void tpm2_flush_context_cmd(struct tpm_chip *chip, u32 handle);
 #else
 static inline int tpm_is_tpm2(struct tpm_chip *chip)
 {
diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c
index 08ec7f48f01d..38bb33333cdf 100644
--- a/security/keys/trusted-keys/trusted_tpm2.c
+++ b/security/keys/trusted-keys/trusted_tpm2.c
@@ -309,7 +309,7 @@ int tpm2_unseal_trusted(struct tpm_chip *chip,
 		return rc;
 
 	rc = tpm2_unseal_cmd(chip, payload, options, blob_handle);
-	tpm2_flush_context(chip, blob_handle);
+	tpm2_flush_context_cmd(chip, blob_handle);
 
 	return rc;
 }
-- 
2.26.2

Re: [PATCH] tpm: only export stand alone version of flush context command

[Jarkko Sakkinen]

On Sun, Sep 27, 2020 at 04:17:40PM -0700, James Bottomley wrote:
> Remove the currently exported version of flush context, which is
> designed for tpm core internal use only and substitute a corrected
> version that does the necessary tpm ops get/put.  This fixes a bug
> with trusted keys in that some TIS TPMs are unable to flush the
> loaded secret because the status register isn't reading correctly.
> 
> Fixes: 45477b3fe3d1 ("security: keys: trusted: fix lost handle flush")
> Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
> ---
>  drivers/char/tpm/tpm.h                    |  1 +
>  drivers/char/tpm/tpm2-cmd.c               | 23 ++++++++++++++++++++++-
>  include/linux/tpm.h                       |  2 +-
>  security/keys/trusted-keys/trusted_tpm2.c |  2 +-
>  4 files changed, 25 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index 21ac88d4076c..cba09be7ce23 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -240,6 +240,7 @@ int tpm2_prepare_space(struct tpm_chip *chip, struct tpm_space *space, u8 *cmd,
>  		       size_t cmdsiz);
>  int tpm2_commit_space(struct tpm_chip *chip, struct tpm_space *space, void *buf,
>  		      size_t *bufsiz);
> +void tpm2_flush_context(struct tpm_chip *chip, u32 handle);
>  
>  void tpm_bios_log_setup(struct tpm_chip *chip);
>  void tpm_bios_log_teardown(struct tpm_chip *chip);
> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
> index 9b84158c5a9e..d5aaea72d578 100644
> --- a/drivers/char/tpm/tpm2-cmd.c
> +++ b/drivers/char/tpm/tpm2-cmd.c
> @@ -362,7 +362,28 @@ void tpm2_flush_context(struct tpm_chip *chip, u32 handle)
>  	tpm_transmit_cmd(chip, &buf, 0, "flushing context");
>  	tpm_buf_destroy(&buf);
>  }
> -EXPORT_SYMBOL_GPL(tpm2_flush_context);
> +
> +/**
> + * tpm2_flush_context_cmd() - execute a TPM2_FlushContext command
> + * @chip:	TPM chip to use
> + * @handle:	context handle
> + *
> + * This version of the command is designed to be used outside the
> + * TPM core so acquires and releases the tpm ops.
> + */
> +void tpm2_flush_context_cmd(struct tpm_chip *chip, u32 handle)
> +{
> +	int rc;
> +
> +	rc = tpm_try_get_ops(chip);
> +	if (rc) {
> +		dev_err(&chip->dev, "Failed to acquire tpm ops for flush\n");
> +		return;
> +	}
> +	tpm2_flush_context(chip, handle);
> +	tpm_put_ops(chip);
> +}
> +EXPORT_SYMBOL_GPL(tpm2_flush_context_cmd);

Otherwise fine but please rename the existing function as
__tpm2_flush_context() and exported as tpm2_flush_context().

>  struct tpm2_get_cap_out {
>  	u8 more_data;
> diff --git a/include/linux/tpm.h b/include/linux/tpm.h
> index ae2482510f8c..c4ca52138e8b 100644
> --- a/include/linux/tpm.h
> +++ b/include/linux/tpm.h
> @@ -411,7 +411,7 @@ extern int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
>  extern int tpm_send(struct tpm_chip *chip, void *cmd, size_t buflen);
>  extern int tpm_get_random(struct tpm_chip *chip, u8 *data, size_t max);
>  extern struct tpm_chip *tpm_default_chip(void);
> -void tpm2_flush_context(struct tpm_chip *chip, u32 handle);
> +void tpm2_flush_context_cmd(struct tpm_chip *chip, u32 handle);
>  #else
>  static inline int tpm_is_tpm2(struct tpm_chip *chip)
>  {
> diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c
> index 08ec7f48f01d..38bb33333cdf 100644
> --- a/security/keys/trusted-keys/trusted_tpm2.c
> +++ b/security/keys/trusted-keys/trusted_tpm2.c
> @@ -309,7 +309,7 @@ int tpm2_unseal_trusted(struct tpm_chip *chip,
>  		return rc;
>  
>  	rc = tpm2_unseal_cmd(chip, payload, options, blob_handle);
> -	tpm2_flush_context(chip, blob_handle);
> +	tpm2_flush_context_cmd(chip, blob_handle);
>  
>  	return rc;

It will also elimintate this change.

Thank you.

/Jarkko

Re: [PATCH] tpm: only export stand alone version of flush context command

[James Bottomley]

On Mon, 2020-09-28 at 03:11 +0300, Jarkko Sakkinen wrote:
> On Sun, Sep 27, 2020 at 04:17:40PM -0700, James Bottomley wrote:
> > Remove the currently exported version of flush context, which is
> > designed for tpm core internal use only and substitute a corrected
> > version that does the necessary tpm ops get/put.  This fixes a bug
> > with trusted keys in that some TIS TPMs are unable to flush the
> > loaded secret because the status register isn't reading correctly.
> > 
> > Fixes: 45477b3fe3d1 ("security: keys: trusted: fix lost handle
> > flush")
> > Signed-off-by: James Bottomley <
> > James.Bottomley@HansenPartnership.com>
> > ---
> >  drivers/char/tpm/tpm.h                    |  1 +
> >  drivers/char/tpm/tpm2-cmd.c               | 23
> > ++++++++++++++++++++++-
> >  include/linux/tpm.h                       |  2 +-
> >  security/keys/trusted-keys/trusted_tpm2.c |  2 +-
> >  4 files changed, 25 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> > index 21ac88d4076c..cba09be7ce23 100644
> > --- a/drivers/char/tpm/tpm.h
> > +++ b/drivers/char/tpm/tpm.h
> > @@ -240,6 +240,7 @@ int tpm2_prepare_space(struct tpm_chip *chip,
> > struct tpm_space *space, u8 *cmd,
> >  		       size_t cmdsiz);
> >  int tpm2_commit_space(struct tpm_chip *chip, struct tpm_space
> > *space, void *buf,
> >  		      size_t *bufsiz);
> > +void tpm2_flush_context(struct tpm_chip *chip, u32 handle);
> >  
> >  void tpm_bios_log_setup(struct tpm_chip *chip);
> >  void tpm_bios_log_teardown(struct tpm_chip *chip);
> > diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-
> > cmd.c
> > index 9b84158c5a9e..d5aaea72d578 100644
> > --- a/drivers/char/tpm/tpm2-cmd.c
> > +++ b/drivers/char/tpm/tpm2-cmd.c
> > @@ -362,7 +362,28 @@ void tpm2_flush_context(struct tpm_chip *chip,
> > u32 handle)
> >  	tpm_transmit_cmd(chip, &buf, 0, "flushing context");
> >  	tpm_buf_destroy(&buf);
> >  }
> > -EXPORT_SYMBOL_GPL(tpm2_flush_context);
> > +
> > +/**
> > + * tpm2_flush_context_cmd() - execute a TPM2_FlushContext command
> > + * @chip:	TPM chip to use
> > + * @handle:	context handle
> > + *
> > + * This version of the command is designed to be used outside the
> > + * TPM core so acquires and releases the tpm ops.
> > + */
> > +void tpm2_flush_context_cmd(struct tpm_chip *chip, u32 handle)
> > +{
> > +	int rc;
> > +
> > +	rc = tpm_try_get_ops(chip);
> > +	if (rc) {
> > +		dev_err(&chip->dev, "Failed to acquire tpm ops for
> > flush\n");
> > +		return;
> > +	}
> > +	tpm2_flush_context(chip, handle);
> > +	tpm_put_ops(chip);
> > +}
> > +EXPORT_SYMBOL_GPL(tpm2_flush_context_cmd);
> 
> Otherwise fine but please rename the existing function as
> __tpm2_flush_context() and exported as tpm2_flush_context().

If I do this it churns the code base more: we have one external
consumer and four internal ones, so now each of the internal ones would
have to become __tpm_flush_context().  We also have precedence for the
xxx_cmd form with tpm2_unseal_cmd, tpm2_load_cmd.

> >  struct tpm2_get_cap_out {
> >  	u8 more_data;
> > diff --git a/include/linux/tpm.h b/include/linux/tpm.h
> > index ae2482510f8c..c4ca52138e8b 100644
> > --- a/include/linux/tpm.h
> > +++ b/include/linux/tpm.h
> > @@ -411,7 +411,7 @@ extern int tpm_pcr_extend(struct tpm_chip
> > *chip, u32 pcr_idx,
> >  extern int tpm_send(struct tpm_chip *chip, void *cmd, size_t
> > buflen);
> >  extern int tpm_get_random(struct tpm_chip *chip, u8 *data, size_t
> > max);
> >  extern struct tpm_chip *tpm_default_chip(void);
> > -void tpm2_flush_context(struct tpm_chip *chip, u32 handle);
> > +void tpm2_flush_context_cmd(struct tpm_chip *chip, u32 handle);
> >  #else
> >  static inline int tpm_is_tpm2(struct tpm_chip *chip)
> >  {
> > diff --git a/security/keys/trusted-keys/trusted_tpm2.c
> > b/security/keys/trusted-keys/trusted_tpm2.c
> > index 08ec7f48f01d..38bb33333cdf 100644
> > --- a/security/keys/trusted-keys/trusted_tpm2.c
> > +++ b/security/keys/trusted-keys/trusted_tpm2.c
> > @@ -309,7 +309,7 @@ int tpm2_unseal_trusted(struct tpm_chip *chip,
> >  		return rc;
> >  
> >  	rc = tpm2_unseal_cmd(chip, payload, options, blob_handle);
> > -	tpm2_flush_context(chip, blob_handle);
> > +	tpm2_flush_context_cmd(chip, blob_handle);
> >  
> >  	return rc;
> 
> It will also elimintate this change.

Well, yes, but it will add four other changes.

James

Re: [PATCH] tpm: only export stand alone version of flush context command

[Jarkko Sakkinen]

On Sun, Sep 27, 2020 at 06:03:37PM -0700, James Bottomley wrote:
> On Mon, 2020-09-28 at 03:11 +0300, Jarkko Sakkinen wrote:
> > On Sun, Sep 27, 2020 at 04:17:40PM -0700, James Bottomley wrote:
> > > Remove the currently exported version of flush context, which is
> > > designed for tpm core internal use only and substitute a corrected
> > > version that does the necessary tpm ops get/put.  This fixes a bug
> > > with trusted keys in that some TIS TPMs are unable to flush the
> > > loaded secret because the status register isn't reading correctly.
> > > 
> > > Fixes: 45477b3fe3d1 ("security: keys: trusted: fix lost handle
> > > flush")
> > > Signed-off-by: James Bottomley <
> > > James.Bottomley@HansenPartnership.com>
> > > ---
> > >  drivers/char/tpm/tpm.h                    |  1 +
> > >  drivers/char/tpm/tpm2-cmd.c               | 23
> > > ++++++++++++++++++++++-
> > >  include/linux/tpm.h                       |  2 +-
> > >  security/keys/trusted-keys/trusted_tpm2.c |  2 +-
> > >  4 files changed, 25 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> > > index 21ac88d4076c..cba09be7ce23 100644
> > > --- a/drivers/char/tpm/tpm.h
> > > +++ b/drivers/char/tpm/tpm.h
> > > @@ -240,6 +240,7 @@ int tpm2_prepare_space(struct tpm_chip *chip,
> > > struct tpm_space *space, u8 *cmd,
> > >  		       size_t cmdsiz);
> > >  int tpm2_commit_space(struct tpm_chip *chip, struct tpm_space
> > > *space, void *buf,
> > >  		      size_t *bufsiz);
> > > +void tpm2_flush_context(struct tpm_chip *chip, u32 handle);
> > >  
> > >  void tpm_bios_log_setup(struct tpm_chip *chip);
> > >  void tpm_bios_log_teardown(struct tpm_chip *chip);
> > > diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-
> > > cmd.c
> > > index 9b84158c5a9e..d5aaea72d578 100644
> > > --- a/drivers/char/tpm/tpm2-cmd.c
> > > +++ b/drivers/char/tpm/tpm2-cmd.c
> > > @@ -362,7 +362,28 @@ void tpm2_flush_context(struct tpm_chip *chip,
> > > u32 handle)
> > >  	tpm_transmit_cmd(chip, &buf, 0, "flushing context");
> > >  	tpm_buf_destroy(&buf);
> > >  }
> > > -EXPORT_SYMBOL_GPL(tpm2_flush_context);
> > > +
> > > +/**
> > > + * tpm2_flush_context_cmd() - execute a TPM2_FlushContext command
> > > + * @chip:	TPM chip to use
> > > + * @handle:	context handle
> > > + *
> > > + * This version of the command is designed to be used outside the
> > > + * TPM core so acquires and releases the tpm ops.
> > > + */
> > > +void tpm2_flush_context_cmd(struct tpm_chip *chip, u32 handle)
> > > +{
> > > +	int rc;
> > > +
> > > +	rc = tpm_try_get_ops(chip);
> > > +	if (rc) {
> > > +		dev_err(&chip->dev, "Failed to acquire tpm ops for
> > > flush\n");
> > > +		return;
> > > +	}
> > > +	tpm2_flush_context(chip, handle);
> > > +	tpm_put_ops(chip);
> > > +}
> > > +EXPORT_SYMBOL_GPL(tpm2_flush_context_cmd);
> > 
> > Otherwise fine but please rename the existing function as
> > __tpm2_flush_context() and exported as tpm2_flush_context().
> 
> If I do this it churns the code base more: we have one external
> consumer and four internal ones, so now each of the internal ones would
> have to become __tpm_flush_context().  We also have precedence for the
> xxx_cmd form with tpm2_unseal_cmd, tpm2_load_cmd.

There are no internals version of aforementioned functions, but in the
sense of common convention for such that encapsulate a single TPM
command and nothing more or less, your argument make sense.

But it is somewhat common pattern to prefix internal/unlocked version
with two underscores. So summarizing this I think that the best names
would be __tpm2_flush_context_cmd() and tpm2_flush_context_cmd().

But now that I looked at your patch, I remembered the reason why the
function in question does not take ops, albeit I'm not fully in the
page why this was not properly implemented in trusted_tpm2.c.

The principal idea was that the client, e.g. trusted keys would take
the ops and execute series of commands and then return ops. Otherwise,
there is a probel in atomicity, i.e. someone could race between unseal
and flush.

int tpm2_unseal_trusted(struct tpm_chip *chip,
			struct trusted_key_payload *payload,
			struct trusted_key_options *options)
{
	u32 blob_handle;
	int rc;

	rc = tpm_try_get_ops(chip);
	if (rc)
		goto out;

	rc = tpm2_load_cmd(chip, payload, options, &blob_handle);
	if (rc)
		goto out;

	rc = tpm2_unseal_cmd(chip, payload, options, blob_handle);
	tpm2_flush_context(chip, blob_handle);

out:
	tpm_put_ops(chip);
	return rc;
}

In addition to this fix, I think we should put a note to kdoc of each
exported function that please grab the ops before using.

/Jarkko

Re: [PATCH] tpm: only export stand alone version of flush context command

[Jarkko Sakkinen]

On Mon, Sep 28, 2020 at 02:20:50PM +0300, Jarkko Sakkinen wrote:
> On Sun, Sep 27, 2020 at 06:03:37PM -0700, James Bottomley wrote:
> > On Mon, 2020-09-28 at 03:11 +0300, Jarkko Sakkinen wrote:
> > > On Sun, Sep 27, 2020 at 04:17:40PM -0700, James Bottomley wrote:
> > > > Remove the currently exported version of flush context, which is
> > > > designed for tpm core internal use only and substitute a corrected
> > > > version that does the necessary tpm ops get/put.  This fixes a bug
> > > > with trusted keys in that some TIS TPMs are unable to flush the
> > > > loaded secret because the status register isn't reading correctly.
> > > > 
> > > > Fixes: 45477b3fe3d1 ("security: keys: trusted: fix lost handle
> > > > flush")
> > > > Signed-off-by: James Bottomley <
> > > > James.Bottomley@HansenPartnership.com>
> > > > ---
> > > >  drivers/char/tpm/tpm.h                    |  1 +
> > > >  drivers/char/tpm/tpm2-cmd.c               | 23
> > > > ++++++++++++++++++++++-
> > > >  include/linux/tpm.h                       |  2 +-
> > > >  security/keys/trusted-keys/trusted_tpm2.c |  2 +-
> > > >  4 files changed, 25 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> > > > index 21ac88d4076c..cba09be7ce23 100644
> > > > --- a/drivers/char/tpm/tpm.h
> > > > +++ b/drivers/char/tpm/tpm.h
> > > > @@ -240,6 +240,7 @@ int tpm2_prepare_space(struct tpm_chip *chip,
> > > > struct tpm_space *space, u8 *cmd,
> > > >  		       size_t cmdsiz);
> > > >  int tpm2_commit_space(struct tpm_chip *chip, struct tpm_space
> > > > *space, void *buf,
> > > >  		      size_t *bufsiz);
> > > > +void tpm2_flush_context(struct tpm_chip *chip, u32 handle);
> > > >  
> > > >  void tpm_bios_log_setup(struct tpm_chip *chip);
> > > >  void tpm_bios_log_teardown(struct tpm_chip *chip);
> > > > diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-
> > > > cmd.c
> > > > index 9b84158c5a9e..d5aaea72d578 100644
> > > > --- a/drivers/char/tpm/tpm2-cmd.c
> > > > +++ b/drivers/char/tpm/tpm2-cmd.c
> > > > @@ -362,7 +362,28 @@ void tpm2_flush_context(struct tpm_chip *chip,
> > > > u32 handle)
> > > >  	tpm_transmit_cmd(chip, &buf, 0, "flushing context");
> > > >  	tpm_buf_destroy(&buf);
> > > >  }
> > > > -EXPORT_SYMBOL_GPL(tpm2_flush_context);
> > > > +
> > > > +/**
> > > > + * tpm2_flush_context_cmd() - execute a TPM2_FlushContext command
> > > > + * @chip:	TPM chip to use
> > > > + * @handle:	context handle
> > > > + *
> > > > + * This version of the command is designed to be used outside the
> > > > + * TPM core so acquires and releases the tpm ops.
> > > > + */
> > > > +void tpm2_flush_context_cmd(struct tpm_chip *chip, u32 handle)
> > > > +{
> > > > +	int rc;
> > > > +
> > > > +	rc = tpm_try_get_ops(chip);
> > > > +	if (rc) {
> > > > +		dev_err(&chip->dev, "Failed to acquire tpm ops for
> > > > flush\n");
> > > > +		return;
> > > > +	}
> > > > +	tpm2_flush_context(chip, handle);
> > > > +	tpm_put_ops(chip);
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(tpm2_flush_context_cmd);
> > > 
> > > Otherwise fine but please rename the existing function as
> > > __tpm2_flush_context() and exported as tpm2_flush_context().
> > 
> > If I do this it churns the code base more: we have one external
> > consumer and four internal ones, so now each of the internal ones would
> > have to become __tpm_flush_context().  We also have precedence for the
> > xxx_cmd form with tpm2_unseal_cmd, tpm2_load_cmd.
> 
> There are no internals version of aforementioned functions, but in the
> sense of common convention for such that encapsulate a single TPM
> command and nothing more or less, your argument make sense.
> 
> But it is somewhat common pattern to prefix internal/unlocked version
> with two underscores. So summarizing this I think that the best names
> would be __tpm2_flush_context_cmd() and tpm2_flush_context_cmd().
> 
> But now that I looked at your patch, I remembered the reason why the
> function in question does not take ops, albeit I'm not fully in the
> page why this was not properly implemented in trusted_tpm2.c.
> 
> The principal idea was that the client, e.g. trusted keys would take
> the ops and execute series of commands and then return ops. Otherwise,
> there is a probel in atomicity, i.e. someone could race between unseal
> and flush.
> 
> int tpm2_unseal_trusted(struct tpm_chip *chip,
> 			struct trusted_key_payload *payload,
> 			struct trusted_key_options *options)
> {
> 	u32 blob_handle;
> 	int rc;
> 
> 	rc = tpm_try_get_ops(chip);
> 	if (rc)
> 		goto out;
> 
> 	rc = tpm2_load_cmd(chip, payload, options, &blob_handle);
> 	if (rc)
> 		goto out;
> 
> 	rc = tpm2_unseal_cmd(chip, payload, options, blob_handle);
> 	tpm2_flush_context(chip, blob_handle);
> 
> out:
> 	tpm_put_ops(chip);
> 	return rc;
> }
> 
> In addition to this fix, I think we should put a note to kdoc of each
> exported function that please grab the ops before using.

I'll find the correct fixes tag. Your commit that is in the fixes tag
did not causue this regression.

/Jarkko

Re: [PATCH] tpm: only export stand alone version of flush context command

[Jarkko Sakkinen]

On Mon, Sep 28, 2020 at 02:34:05PM +0300, Jarkko Sakkinen wrote:
> On Mon, Sep 28, 2020 at 02:20:50PM +0300, Jarkko Sakkinen wrote:
> > On Sun, Sep 27, 2020 at 06:03:37PM -0700, James Bottomley wrote:
> > > On Mon, 2020-09-28 at 03:11 +0300, Jarkko Sakkinen wrote:
> > > > On Sun, Sep 27, 2020 at 04:17:40PM -0700, James Bottomley wrote:
> > > > > Remove the currently exported version of flush context, which is
> > > > > designed for tpm core internal use only and substitute a corrected
> > > > > version that does the necessary tpm ops get/put.  This fixes a bug
> > > > > with trusted keys in that some TIS TPMs are unable to flush the
> > > > > loaded secret because the status register isn't reading correctly.
> > > > > 
> > > > > Fixes: 45477b3fe3d1 ("security: keys: trusted: fix lost handle
> > > > > flush")
> > > > > Signed-off-by: James Bottomley <
> > > > > James.Bottomley@HansenPartnership.com>
> > > > > ---
> > > > >  drivers/char/tpm/tpm.h                    |  1 +
> > > > >  drivers/char/tpm/tpm2-cmd.c               | 23
> > > > > ++++++++++++++++++++++-
> > > > >  include/linux/tpm.h                       |  2 +-
> > > > >  security/keys/trusted-keys/trusted_tpm2.c |  2 +-
> > > > >  4 files changed, 25 insertions(+), 3 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> > > > > index 21ac88d4076c..cba09be7ce23 100644
> > > > > --- a/drivers/char/tpm/tpm.h
> > > > > +++ b/drivers/char/tpm/tpm.h
> > > > > @@ -240,6 +240,7 @@ int tpm2_prepare_space(struct tpm_chip *chip,
> > > > > struct tpm_space *space, u8 *cmd,
> > > > >  		       size_t cmdsiz);
> > > > >  int tpm2_commit_space(struct tpm_chip *chip, struct tpm_space
> > > > > *space, void *buf,
> > > > >  		      size_t *bufsiz);
> > > > > +void tpm2_flush_context(struct tpm_chip *chip, u32 handle);
> > > > >  
> > > > >  void tpm_bios_log_setup(struct tpm_chip *chip);
> > > > >  void tpm_bios_log_teardown(struct tpm_chip *chip);
> > > > > diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-
> > > > > cmd.c
> > > > > index 9b84158c5a9e..d5aaea72d578 100644
> > > > > --- a/drivers/char/tpm/tpm2-cmd.c
> > > > > +++ b/drivers/char/tpm/tpm2-cmd.c
> > > > > @@ -362,7 +362,28 @@ void tpm2_flush_context(struct tpm_chip *chip,
> > > > > u32 handle)
> > > > >  	tpm_transmit_cmd(chip, &buf, 0, "flushing context");
> > > > >  	tpm_buf_destroy(&buf);
> > > > >  }
> > > > > -EXPORT_SYMBOL_GPL(tpm2_flush_context);
> > > > > +
> > > > > +/**
> > > > > + * tpm2_flush_context_cmd() - execute a TPM2_FlushContext command
> > > > > + * @chip:	TPM chip to use
> > > > > + * @handle:	context handle
> > > > > + *
> > > > > + * This version of the command is designed to be used outside the
> > > > > + * TPM core so acquires and releases the tpm ops.
> > > > > + */
> > > > > +void tpm2_flush_context_cmd(struct tpm_chip *chip, u32 handle)
> > > > > +{
> > > > > +	int rc;
> > > > > +
> > > > > +	rc = tpm_try_get_ops(chip);
> > > > > +	if (rc) {
> > > > > +		dev_err(&chip->dev, "Failed to acquire tpm ops for
> > > > > flush\n");
> > > > > +		return;
> > > > > +	}
> > > > > +	tpm2_flush_context(chip, handle);
> > > > > +	tpm_put_ops(chip);
> > > > > +}
> > > > > +EXPORT_SYMBOL_GPL(tpm2_flush_context_cmd);
> > > > 
> > > > Otherwise fine but please rename the existing function as
> > > > __tpm2_flush_context() and exported as tpm2_flush_context().
> > > 
> > > If I do this it churns the code base more: we have one external
> > > consumer and four internal ones, so now each of the internal ones would
> > > have to become __tpm_flush_context().  We also have precedence for the
> > > xxx_cmd form with tpm2_unseal_cmd, tpm2_load_cmd.
> > 
> > There are no internals version of aforementioned functions, but in the
> > sense of common convention for such that encapsulate a single TPM
> > command and nothing more or less, your argument make sense.
> > 
> > But it is somewhat common pattern to prefix internal/unlocked version
> > with two underscores. So summarizing this I think that the best names
> > would be __tpm2_flush_context_cmd() and tpm2_flush_context_cmd().
> > 
> > But now that I looked at your patch, I remembered the reason why the
> > function in question does not take ops, albeit I'm not fully in the
> > page why this was not properly implemented in trusted_tpm2.c.
> > 
> > The principal idea was that the client, e.g. trusted keys would take
> > the ops and execute series of commands and then return ops. Otherwise,
> > there is a probel in atomicity, i.e. someone could race between unseal
> > and flush.
> > 
> > int tpm2_unseal_trusted(struct tpm_chip *chip,
> > 			struct trusted_key_payload *payload,
> > 			struct trusted_key_options *options)
> > {
> > 	u32 blob_handle;
> > 	int rc;
> > 
> > 	rc = tpm_try_get_ops(chip);
> > 	if (rc)
> > 		goto out;
> > 
> > 	rc = tpm2_load_cmd(chip, payload, options, &blob_handle);
> > 	if (rc)
> > 		goto out;
> > 
> > 	rc = tpm2_unseal_cmd(chip, payload, options, blob_handle);
> > 	tpm2_flush_context(chip, blob_handle);
> > 
> > out:
> > 	tpm_put_ops(chip);
> > 	return rc;
> > }
> > 
> > In addition to this fix, I think we should put a note to kdoc of each
> > exported function that please grab the ops before using.
> 
> I'll find the correct fixes tag. Your commit that is in the fixes tag
> did not causue this regression.

I'll provide a fix for this. The regression popped up in:

  2e19e10131a0 ("KEYS: trusted: Move TPM2 trusted keys code")

While I started to fix this, I also found a regression originating from:

  41ab999c80f1 ("tpm: Move tpm_get_random api into the TPM device driver")

I.e. the return value of tpm_get_random() is not handled correctly. That
has been there for a while...

/Jarkko

Re: [PATCH] tpm: only export stand alone version of flush context command

[James Bottomley]

On Mon, 2020-09-28 at 14:20 +0300, Jarkko Sakkinen wrote:
> On Sun, Sep 27, 2020 at 06:03:37PM -0700, James Bottomley wrote:
> > On Mon, 2020-09-28 at 03:11 +0300, Jarkko Sakkinen wrote:
> > > On Sun, Sep 27, 2020 at 04:17:40PM -0700, James Bottomley wrote:
[...]
> > > > +EXPORT_SYMBOL_GPL(tpm2_flush_context_cmd);
> > > 
> > > Otherwise fine but please rename the existing function as
> > > __tpm2_flush_context() and exported as tpm2_flush_context().
> > 
> > If I do this it churns the code base more: we have one external
> > consumer and four internal ones, so now each of the internal ones
> > would have to become __tpm_flush_context().  We also have
> > precedence for the xxx_cmd form with tpm2_unseal_cmd,
> > tpm2_load_cmd.
> 
> There are no internals version of aforementioned functions, but in
> the sense of common convention for such that encapsulate a single TPM
> command and nothing more or less, your argument make sense.

By internal, I mean use within the tpm core that doesn't require
get/put ops ... there are four of them.

> But it is somewhat common pattern to prefix internal/unlocked version
> with two underscores. So summarizing this I think that the best names
> would be __tpm2_flush_context_cmd() and tpm2_flush_context_cmd().
> 
> But now that I looked at your patch, I remembered the reason why the
> function in question does not take ops, albeit I'm not fully in the
> page why this was not properly implemented in trusted_tpm2.c.
> 
> The principal idea was that the client, e.g. trusted keys would take
> the ops and execute series of commands and then return ops.
> Otherwise, there is a probel in atomicity, i.e. someone could race
> between unseal and flush.
> 
> int tpm2_unseal_trusted(struct tpm_chip *chip,
> 			struct trusted_key_payload *payload,
> 			struct trusted_key_options *options)
> {
> 	u32 blob_handle;
> 	int rc;
> 
> 	rc = tpm_try_get_ops(chip);
> 	if (rc)
> 		goto out;
> 
> 	rc = tpm2_load_cmd(chip, payload, options, &blob_handle);
> 	if (rc)
> 		goto out;
> 
> 	rc = tpm2_unseal_cmd(chip, payload, options, blob_handle);
> 	tpm2_flush_context(chip, blob_handle);
> 
> out:
> 	tpm_put_ops(chip);
> 	return rc;
> }
> 
> In addition to this fix, I think we should put a note to kdoc of each
> exported function that please grab the ops before using.

Well, um, that's precisely what this function originally did when it
was inside drivers/char/tpm.  You told the guy who did the move into
security/keys/trusted-keys to convert everything to use tpm_send which
encapsulates the get/put operation, which is why we now have the flush
bug.  If you really want it done like this, then I'd recommend moving
everything back to drivers/char/tpm so we don't have to do a global
exposure of a load of tpm internal functions (i.e. move them from
drivers/char/tmp.h to include/linux/tpm.h and do an export on them).

James

Re: [PATCH] tpm: only export stand alone version of flush context command

[Jarkko Sakkinen]

On Mon, Sep 28, 2020 at 07:50:31AM -0700, James Bottomley wrote:
> On Mon, 2020-09-28 at 14:20 +0300, Jarkko Sakkinen wrote:
> > On Sun, Sep 27, 2020 at 06:03:37PM -0700, James Bottomley wrote:
> > > On Mon, 2020-09-28 at 03:11 +0300, Jarkko Sakkinen wrote:
> > > > On Sun, Sep 27, 2020 at 04:17:40PM -0700, James Bottomley wrote:
> [...]
> > > > > +EXPORT_SYMBOL_GPL(tpm2_flush_context_cmd);
> > > > 
> > > > Otherwise fine but please rename the existing function as
> > > > __tpm2_flush_context() and exported as tpm2_flush_context().
> > > 
> > > If I do this it churns the code base more: we have one external
> > > consumer and four internal ones, so now each of the internal ones
> > > would have to become __tpm_flush_context().  We also have
> > > precedence for the xxx_cmd form with tpm2_unseal_cmd,
> > > tpm2_load_cmd.
> > 
> > There are no internals version of aforementioned functions, but in
> > the sense of common convention for such that encapsulate a single TPM
> > command and nothing more or less, your argument make sense.
> 
> By internal, I mean use within the tpm core that doesn't require
> get/put ops ... there are four of them.
> 
> > But it is somewhat common pattern to prefix internal/unlocked version
> > with two underscores. So summarizing this I think that the best names
> > would be __tpm2_flush_context_cmd() and tpm2_flush_context_cmd().
> > 
> > But now that I looked at your patch, I remembered the reason why the
> > function in question does not take ops, albeit I'm not fully in the
> > page why this was not properly implemented in trusted_tpm2.c.
> > 
> > The principal idea was that the client, e.g. trusted keys would take
> > the ops and execute series of commands and then return ops.
> > Otherwise, there is a probel in atomicity, i.e. someone could race
> > between unseal and flush.
> > 
> > int tpm2_unseal_trusted(struct tpm_chip *chip,
> > 			struct trusted_key_payload *payload,
> > 			struct trusted_key_options *options)
> > {
> > 	u32 blob_handle;
> > 	int rc;
> > 
> > 	rc = tpm_try_get_ops(chip);
> > 	if (rc)
> > 		goto out;
> > 
> > 	rc = tpm2_load_cmd(chip, payload, options, &blob_handle);
> > 	if (rc)
> > 		goto out;
> > 
> > 	rc = tpm2_unseal_cmd(chip, payload, options, blob_handle);
> > 	tpm2_flush_context(chip, blob_handle);
> > 
> > out:
> > 	tpm_put_ops(chip);
> > 	return rc;
> > }
> > 
> > In addition to this fix, I think we should put a note to kdoc of each
> > exported function that please grab the ops before using.
> 
> Well, um, that's precisely what this function originally did when it
> was inside drivers/char/tpm.  You told the guy who did the move into
> security/keys/trusted-keys to convert everything to use tpm_send which
> encapsulates the get/put operation, which is why we now have the flush
> bug.  If you really want it done like this, then I'd recommend moving
> everything back to drivers/char/tpm so we don't have to do a global
> exposure of a load of tpm internal functions (i.e. move them from
> drivers/char/tmp.h to include/linux/tpm.h and do an export on them).

My BuildRoot test image did not include the patch. I was wondering why I
did not bump into deadlock with the fix candidate :-/ Forgot export
LINUX_OVERRIDE_SRCDIR.

But you are absolutely correct, thanks for recalling. I made a mistake
there.

I do disagree though that this should be moved back to drivers/char/tpm,
as also TPM 1.x code lives in trusted-keys. It is good to have API for
doing sequences TPM commands and keep the core in drivers/char/tpm.

If you look at tpm_send() it is in essence just simply locking TPM and
and calling tpm_transmit_cmd(). And tpm_transmit_cmd() is already an
exported symbol. It only needs to be declared in include/linux/tpm.h.

I'd suggest that I refine my series to call tpm_transmit_cmd() and we
have a fairly clean solution where the load sequence is atomic.

/Jarkko>

Re: [PATCH] tpm: only export stand alone version of flush context command

[Jarkko Sakkinen]

On Mon, Sep 28, 2020 at 07:31:18PM +0300, Jarkko Sakkinen wrote:
> > Well, um, that's precisely what this function originally did when it
> > was inside drivers/char/tpm.  You told the guy who did the move into
> > security/keys/trusted-keys to convert everything to use tpm_send which
> > encapsulates the get/put operation, which is why we now have the flush
> > bug.  If you really want it done like this, then I'd recommend moving
> > everything back to drivers/char/tpm so we don't have to do a global
> > exposure of a load of tpm internal functions (i.e. move them from
> > drivers/char/tmp.h to include/linux/tpm.h and do an export on them).
> 
> My BuildRoot test image did not include the patch. I was wondering why I
> did not bump into deadlock with the fix candidate :-/ Forgot export
> LINUX_OVERRIDE_SRCDIR.
> 
> But you are absolutely correct, thanks for recalling. I made a mistake
> there.
> 
> I do disagree though that this should be moved back to drivers/char/tpm,
> as also TPM 1.x code lives in trusted-keys. It is good to have API for
> doing sequences TPM commands and keep the core in drivers/char/tpm.
> 
> If you look at tpm_send() it is in essence just simply locking TPM and
> and calling tpm_transmit_cmd(). And tpm_transmit_cmd() is already an
> exported symbol. It only needs to be declared in include/linux/tpm.h.
> 
> I'd suggest that I refine my series to call tpm_transmit_cmd() and we
> have a fairly clean solution where the load sequence is atomic.

I see that it is perfectly fine to make tpm_transmit_cmd() globally
callable. It is already used by tpm_vtpm_proxy and does have clear
semantics.

The way you use it is just:

1. tpm_try_get_ops
2. Use tpm_transmit_cmd() N times.
3. tpm_put_ops

If we moved TPM 2.x trusted keys code back to drivers/char/tpm,for the
sake of consistency the same would have to be done for TPM 1.2 code. I'd
rather fix the regression and be done with it.

Or if reverted like that, also asym_tpm.c code should also live inside
the TPM driver directory.

All this work with tpm_buf and the locking functions makes most sense if
it gives ability for callers to build their own TPM commands

I'm right now building test image with v3 of my fixes (this time
properly included to the kernel image). I also uploaded the (untested)
patches over here:

https://git.kernel.org/pub/scm/linux/kernel/git/jarkko/linux-tpmdd.git/log/?h=trusted-fix

/Jarkko

Re: [PATCH] tpm: only export stand alone version of flush context command

[James Bottomley]

On Mon, 2020-09-28 at 20:07 +0300, Jarkko Sakkinen wrote:
> On Mon, Sep 28, 2020 at 07:31:18PM +0300, Jarkko Sakkinen wrote:
> > > Well, um, that's precisely what this function originally did when
> > > it was inside drivers/char/tpm.  You told the guy who did the
> > > move into security/keys/trusted-keys to convert everything to use
> > > tpm_send which encapsulates the get/put operation, which is why
> > > we now have the flush bug.  If you really want it done like this,
> > > then I'd recommend moving everything back to drivers/char/tpm so
> > > we don't have to do a global exposure of a load of tpm internal
> > > functions (i.e. move them from drivers/char/tmp.h to
> > > include/linux/tpm.h and do an export on them).
> > 
> > My BuildRoot test image did not include the patch. I was wondering
> > why I did not bump into deadlock with the fix candidate :-/ Forgot
> > export LINUX_OVERRIDE_SRCDIR.
> > 
> > But you are absolutely correct, thanks for recalling. I made a
> > mistake there.
> > 
> > I do disagree though that this should be moved back to
> > drivers/char/tpm, as also TPM 1.x code lives in trusted-keys. It is
> > good to have API for doing sequences TPM commands and keep the core
> > in drivers/char/tpm.

I think tpm2_load_cmd is likely going to have to move back anyway just
because more things than trusted keys need to use it.  I can't really
see any other use for the seal/unseal so they can stay in trusted keys
until someone finds a use for them.

> > If you look at tpm_send() it is in essence just simply locking TPM
> > and and calling tpm_transmit_cmd(). And tpm_transmit_cmd() is
> > already an exported symbol. It only needs to be declared in
> > include/linux/tpm.h.
> > 
> > I'd suggest that I refine my series to call tpm_transmit_cmd() and
> > we have a fairly clean solution where the load sequence is atomic.
> 
> I see that it is perfectly fine to make tpm_transmit_cmd() globally
> callable. It is already used by tpm_vtpm_proxy and does have clear
> semantics.
> 
> The way you use it is just:
> 
> 1. tpm_try_get_ops
> 2. Use tpm_transmit_cmd() N times.
> 3. tpm_put_ops
> 
> If we moved TPM 2.x trusted keys code back to drivers/char/tpm,for
> the sake of consistency the same would have to be done for TPM 1.2
> code. I'd rather fix the regression and be done with it.
> 
> Or if reverted like that, also asym_tpm.c code should also live
> inside the TPM driver directory.
> 
> All this work with tpm_buf and the locking functions makes most sense
> if it gives ability for callers to build their own TPM commands
> 
> I'm right now building test image with v3 of my fixes (this time
> properly included to the kernel image). I also uploaded the
> (untested) patches over here:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/jarkko/linux-tpmdd.git/log/?h=trusted-fix

I think we can do that ... in which case the fix for the tis interrupt
trigger also becomes a get/put ops around the tpm2_get_tpm_pt.

After the transformation is complete, tpm_send() becomes obsolete,
doesn't it, so it can be removed?

James

Re: [PATCH] tpm: only export stand alone version of flush context command

[Jarkko Sakkinen]

On Mon, Sep 28, 2020 at 10:40:55AM -0700, James Bottomley wrote:
> On Mon, 2020-09-28 at 20:07 +0300, Jarkko Sakkinen wrote:
> > On Mon, Sep 28, 2020 at 07:31:18PM +0300, Jarkko Sakkinen wrote:
> > > > Well, um, that's precisely what this function originally did when
> > > > it was inside drivers/char/tpm.  You told the guy who did the
> > > > move into security/keys/trusted-keys to convert everything to use
> > > > tpm_send which encapsulates the get/put operation, which is why
> > > > we now have the flush bug.  If you really want it done like this,
> > > > then I'd recommend moving everything back to drivers/char/tpm so
> > > > we don't have to do a global exposure of a load of tpm internal
> > > > functions (i.e. move them from drivers/char/tmp.h to
> > > > include/linux/tpm.h and do an export on them).
> > > 
> > > My BuildRoot test image did not include the patch. I was wondering
> > > why I did not bump into deadlock with the fix candidate :-/ Forgot
> > > export LINUX_OVERRIDE_SRCDIR.
> > > 
> > > But you are absolutely correct, thanks for recalling. I made a
> > > mistake there.
> > > 
> > > I do disagree though that this should be moved back to
> > > drivers/char/tpm, as also TPM 1.x code lives in trusted-keys. It is
> > > good to have API for doing sequences TPM commands and keep the core
> > > in drivers/char/tpm.
> 
> I think tpm2_load_cmd is likely going to have to move back anyway just
> because more things than trusted keys need to use it.  I can't really
> see any other use for the seal/unseal so they can stay in trusted keys
> until someone finds a use for them.

We can obviously do that if there are multiple customers for it.

> > > If you look at tpm_send() it is in essence just simply locking TPM
> > > and and calling tpm_transmit_cmd(). And tpm_transmit_cmd() is
> > > already an exported symbol. It only needs to be declared in
> > > include/linux/tpm.h.
> > > 
> > > I'd suggest that I refine my series to call tpm_transmit_cmd() and
> > > we have a fairly clean solution where the load sequence is atomic.
> > 
> > I see that it is perfectly fine to make tpm_transmit_cmd() globally
> > callable. It is already used by tpm_vtpm_proxy and does have clear
> > semantics.
> > 
> > The way you use it is just:
> > 
> > 1. tpm_try_get_ops
> > 2. Use tpm_transmit_cmd() N times.
> > 3. tpm_put_ops
> > 
> > If we moved TPM 2.x trusted keys code back to drivers/char/tpm,for
> > the sake of consistency the same would have to be done for TPM 1.2
> > code. I'd rather fix the regression and be done with it.
> > 
> > Or if reverted like that, also asym_tpm.c code should also live
> > inside the TPM driver directory.
> > 
> > All this work with tpm_buf and the locking functions makes most sense
> > if it gives ability for callers to build their own TPM commands
> > 
> > I'm right now building test image with v3 of my fixes (this time
> > properly included to the kernel image). I also uploaded the
> > (untested) patches over here:
> > 
> > https://git.kernel.org/pub/scm/linux/kernel/git/jarkko/linux-tpmdd.git/log/?h=trusted-fix
> 
> I think we can do that ... in which case the fix for the tis interrupt
> trigger also becomes a get/put ops around the tpm2_get_tpm_pt.
> 
> After the transformation is complete, tpm_send() becomes obsolete,
> doesn't it, so it can be removed?

Yes.

BTW, while doing this I think I noticed what was wrong in my test kernel
when I tested your series that introduces ASN.1 keys. I'll test both
before sending update to my fix. Hopefully I can give today tested-by
tags to that series.

> James

/Jarkko

Re: [PATCH] tpm: only export stand alone version of flush context command

[James Bottomley]

On Mon, 2020-09-28 at 21:13 +0300, Jarkko Sakkinen wrote:
> On Mon, Sep 28, 2020 at 10:40:55AM -0700, James Bottomley wrote:
> > On Mon, 2020-09-28 at 20:07 +0300, Jarkko Sakkinen wrote:
> > > On Mon, Sep 28, 2020 at 07:31:18PM +0300, Jarkko Sakkinen wrote:
> > > > > Well, um, that's precisely what this function originally did
> > > > > when it was inside drivers/char/tpm.  You told the guy who
> > > > > did the move into security/keys/trusted-keys to convert
> > > > > everything to use tpm_send which encapsulates the get/put
> > > > > operation, which is why we now have the flush bug.  If you
> > > > > really want it done like this, then I'd recommend moving
> > > > > everything back to drivers/char/tpm so we don't have to do a
> > > > > global exposure of a load of tpm internal functions (i.e.
> > > > > move them from drivers/char/tmp.h to include/linux/tpm.h and
> > > > > do an export on them).
> > > > 
> > > > My BuildRoot test image did not include the patch. I was
> > > > wondering why I did not bump into deadlock with the fix
> > > > candidate :-/
> > > >  Forgot export LINUX_OVERRIDE_SRCDIR.
> > > > 
> > > > But you are absolutely correct, thanks for recalling. I made a
> > > > mistake there.
> > > > 
> > > > I do disagree though that this should be moved back to
> > > > drivers/char/tpm, as also TPM 1.x code lives in trusted-keys.
> > > > It is good to have API for doing sequences TPM commands and
> > > > keep the core in drivers/char/tpm.
> > 
> > I think tpm2_load_cmd is likely going to have to move back anyway
> > just because more things than trusted keys need to use it.  I can't
> > really see any other use for the seal/unseal so they can stay in
> > trusted keys until someone finds a use for them.
> 
> We can obviously do that if there are multiple customers for it.

Yes, let's just leave everything where it is until there's a use case
for moving it.

> > > > If you look at tpm_send() it is in essence just simply locking
> > > > TPM and and calling tpm_transmit_cmd(). And tpm_transmit_cmd()
> > > > is already an exported symbol. It only needs to be declared in
> > > > include/linux/tpm.h.
> > > > 
> > > > I'd suggest that I refine my series to call tpm_transmit_cmd()
> > > > and we have a fairly clean solution where the load sequence is
> > > > atomic.
> > > 
> > > I see that it is perfectly fine to make tpm_transmit_cmd()
> > > globally callable. It is already used by tpm_vtpm_proxy and does
> > > have clear semantics.
> > > 
> > > The way you use it is just:
> > > 
> > > 1. tpm_try_get_ops
> > > 2. Use tpm_transmit_cmd() N times.
> > > 3. tpm_put_ops
> > > 
> > > If we moved TPM 2.x trusted keys code back to
> > > drivers/char/tpm,for the sake of consistency the same would have
> > > to be done for TPM 1.2 code. I'd rather fix the regression and be
> > > done with it.
> > > 
> > > Or if reverted like that, also asym_tpm.c code should also live
> > > inside the TPM driver directory.
> > > 
> > > All this work with tpm_buf and the locking functions makes most
> > > sense if it gives ability for callers to build their own TPM
> > > commands
> > > 
> > > I'm right now building test image with v3 of my fixes (this time
> > > properly included to the kernel image). I also uploaded the
> > > (untested) patches over here:
> > > 
> > > https://git.kernel.org/pub/scm/linux/kernel/git/jarkko/linux-tpmdd.git/log/?h=trusted-fix
> > 
> > I think we can do that ... in which case the fix for the tis
> > interrupt trigger also becomes a get/put ops around the
> > tpm2_get_tpm_pt.
> > 
> > After the transformation is complete, tpm_send() becomes obsolete,
> > doesn't it, so it can be removed?
> 
> Yes.
> 
> BTW, while doing this I think I noticed what was wrong in my test
> kernel when I tested your series that introduces ASN.1 keys. I'll
> test both before sending update to my fix. Hopefully I can give today
> tested-by tags to that series.

Great ... the trusted key code doesn't use tpm_send, but the policy
additions do so the latter will need updating (again).

James

Re: [PATCH] tpm: only export stand alone version of flush context command

[Jarkko Sakkinen]

On Mon, Sep 28, 2020 at 11:26:41AM -0700, James Bottomley wrote:
> > Yes.
> > 
> > BTW, while doing this I think I noticed what was wrong in my test
> > kernel when I tested your series that introduces ASN.1 keys. I'll
> > test both before sending update to my fix. Hopefully I can give today
> > tested-by tags to that series.
> 
> Great ... the trusted key code doesn't use tpm_send, but the policy
> additions do so the latter will need updating (again).

Yes, I know. I can still test both by separately applying them on top
of the current master. If you can after that do the update (and after
the fixes have been applied), I can still get everything to the same
PR.

Sorry for latency because of some SGX discussions (this time retpoline
and CET collision), I'll try to get the testing done ASAP.

/Jarkko