Re: [PATCH] tpm: fix a race condition tpm2_unseal_trusted()

[Jason Gunthorpe <jgunthorpe@obsidianresearch.com>]

On Thu, Jul 21, 2016 at 12:02:45PM +0300, Jarkko Sakkinen wrote:
> On Wed, Jul 20, 2016 at 03:13:32PM -0600, Jason Gunthorpe wrote:
> > On Wed, Jul 20, 2016 at 11:53:14PM +0300, Jarkko Sakkinen wrote:
> > 
> > > The only use cases I see at the moment for it work this way:
> > > 
> > > 1. Call tpm_try_get_ops.
> > > 2. Send a TPM command.
> > > 3. Call tpm_put_ops.
> > 
> > Right, but that is just a reflection of what the in kernel users are
> > doing today, not necessarily what they should be doing.
> > 
> > We should not break the put/get semantics..
> > 
> > > I did not find any other form of use. The only use is to make sure that
> > > there are no transactions running before the ops are cleared. Or did I
> > > overlook something perhaps?
> > 
> > The put/get is intended to allow a kapi user to hold a ref to tpm
> > without it geting destroyed. It is not intended to be an exclusive lock.
> 
> These operations *are not* exposed to kapi. They are interal to the
> driver. That's why it does not make sense speak about kapi user.

Right now yes, but look at other subsystems and you will see
operations like that, because that is typical design pattern. When I
wrote them I made sure they could be used in that typical way.

We have issues in our kapi users with regards to hot plug and multiple
tpms. Fortunately that basically never happens, but it does indicate
the API is not sufficient..

> You are speaking how it could be used. I'm looking at how it's actually
> used. Shouldn't implementation reflect that rather than future
> prospects?

Well, there are common patterns in the kernel for how these things
work and the get/put stuff does not imply an exclusive lock. That is
why it is called get/put :)

> > Those sorts of compound ops should hold the tpm_mutex manually, not
> > through the get_ops scheme.
> 
> The next best option would be to have unlocked transmit_cmd function.

The locking needs a good scrub, it is one of the last legacy things
left now that doesn't make alot of sense, especially if we need to
lock these compound operations.

The get/put locking cleanup was just a small step toward something
more typical..

Jason