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
