Re: [tpmdd-devel] [PATCH 1/2] tpm2: add session handle isolation to tpm spaces

[James Bottomley]

On Fri, 2017-01-20 at 15:23 +0200, Jarkko Sakkinen wrote:
> On Thu, Jan 19, 2017 at 07:11:23AM -0500, James Bottomley wrote:
> > On Thu, 2017-01-19 at 13:58 +0200, Jarkko Sakkinen wrote:
> > > On Wed, Jan 18, 2017 at 10:09:46AM -0500, James Bottomley wrote:
> > > > sessions should be isolated during each instance of a tpm 
> > > > space.  This means that spaces shouldn't be able to see each 
> > > > other's sessions and also when a space is closed, all the 
> > > > sessions belonging to it should be flushed.
> > > > 
> > > > This is implemented by adding a session_tbl to the space to 
> > > > track the created session handles.  Sessions can be flushed 
> > > > either by not setting the continueSession attribute in the 
> > > > session table or by an explicit flush.  In the first case we 
> > > > have to mark the session as being ready to flush and explicitly 
> > > > forget it if the command completes successfully and in the 
> > > > second case we have to intercept the flush instruction and
> > > > clear the session from our table.
> > > 
> > > You could do this without these nasty corner cases by arbage 
> > > collecting when a command emits a new session handle.
> > 
> > I could for this patch set.  However, the global session accounting 
> > RFC requires strict accounting, because it needs to know exactly 
> > when to retry a command that failed because we were out of sessions 
> > and because we don't want to needlessly evict a session if there 
> > was one available which we didn't see because of lazy accounting. 
> >  It would be a lot of churn to do it lazily in this patch set and 
> > then switch to strict in that one, so I chose to account sessions
> > strictly always.
> 
> Lazy is kind of ambiguous word so I'll have to check that we have 
> same definition for it in this context.
> 
> I'm talking about not trying to detect if something gets deleted. 
> When something gets created you would go through the global list of
> sessions and check if it is used. If so, it must be that the session 
> was deleted at some point.

That's my terminology too.  We're talking about lazy and strict
tracking of session flushing.

> Your argument is that in this scheme sometimes there might be a
> session marked as "reserved" but it is in fact free. This might lead
> to useless eviction. Am I correct?

Yes, but not just that, it will also lead to over long waits because we
can no longer wake the waiters the moment a session becomes free.

> My argument is that the lazy scheme is more generic (does not require
> special cases). As a subsystem maintainer I tend to be more fond of 
> that kind of solutions. Having special cases raises questios like 
> (for example):
> 
> 1. What if standard gets added something that does not fall into the
>    current set of special cases? You never know.
> 2. What about vendor specific commands? The lazy scheme is compatible
>    with them. The standard does not put any kind of constraints for
>    vendor specific commands.

We rely on the assertion in the Manual that sessions are only returned
in the handle area (as we do for objects).  We also rely on the
guarantee that they're only destroyed by flush or continueSession being
0 in the session attributes.

If some mad vendor introduces a command that creates an object and
doesn't return it in the handle area, we'll get screwed for both
transient objects and sessions.  Sessions also could have issues if
some mad vendor creates a command that flushes them outside of the
above description.  That's why the standard has all these caveats about
handle and session creation.  In theory the vendors are not allowed to
violate them in their own commands ...

> You could solve the problem you are stating by getting the full the
> list of alive sessions with CAP_HANDLES and mark dangling sessions
> as free.

That's a command which produces a huge output ... I'd have to do it at
the end of every input command to get the list of current handles ... I
don't really think it's a better solution.

Let me describe the failure case with strict destruction accounting:
supposing a mad vendor does introduce a command that flushes a session
outside the standards prescribed way.  What happens is that it gets re
-used before the TPM exhausts handles, and tpm2_session_chip_add will
simply to replace what it currently has.  The only consequence is a
single missed wakeup.  So even in the face of vendor failure, this
scheme will work almost all of the time and it will always work better
than a lazy scheme because the failure case gives us properties
identical to the lazy case.  To make this case iron, we should get a
failure on context save, so I can use that failure to drop the handle
and I think the strict scheme will then always perform better than the
lazy scheme (let's call it strict with lazy backup) will that suffice?

> PS. I've started to think that maybe also with sessions it is better
> to have just one change that implements full eviction like we have
> for transient objects after seeing your breakdown. I'm sorry about
> putting you extra trouble doing the isolation only patch. It's better
> to do this right once...

Well, I can put them back together again, but you could just apply them
together as two patches ... they are now bisectable.

James


> /Jarkko
>