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

Jarkko Sakkinen
2017-01-20T17:58:05Z

On Fri, Jan 20, 2017 at 09:39:13AM -0500, James Bottomley wrote:
> 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.

Sure forgot this last comment. It's really irrelevant. I'll reply
properly later on.

> James

/Jarkko