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.
/Jarkko
