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
