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

[Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>]

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