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@xxxxxxxxxxxxxxxxxxxxx>
> > > > ---
> > > > 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
