Re: [PATCH] tpm: add session handles to the save and restore of the tpm2 space manager

[Jarkko Sakkinen]

On Tue, Jan 17, 2017 at 06:18:12AM -0800, James Bottomley wrote:
> On Tue, 2017-01-17 at 09:23 +0200, Jarkko Sakkinen wrote:
> > On Mon, Jan 16, 2017 at 03:18:45PM -0800, James Bottomley wrote:
> > > On Mon, 2017-01-16 at 12:04 +0200, Jarkko Sakkinen wrote:
> > > > On Fri, Jan 13, 2017 at 11:24:13AM -0800, James Bottomley wrote:
> > > > > Session handles are slightly more difficult to manage because
> > > > > any
> > > > > TPM
> > > > > only has a finite number of allowed handles, even if the
> > > > > session
> > > > > has
> > > > > been saved; so when you context save a session, you must not
> > > > > flush
> > > > > it
> > > > > because that would destroy the ability to context load it (you
> > > > > only
> > > > > flush sessions when you're done with them) and the tpm won't re
> > > > > -use
> > > > > the handle.  Additionally, sessions can be flushed as part of
> > > > > the
> > > > > successful execution of a command if the continueSession
> > > > > attribute
> > > > > is
> > > > > clear, so we have to mark any session we find in the command
> > > > > (using
> > > > > TPM2_HT_TAG_FOR_FLUSH) so it can be cleared from the space if
> > > > > the
> > > > > command successfully executes.
> > > > > 
> > > > > Finally, a session may also be cleared by flushing it, so we
> > > > > have
> > > > > to
> > > > > emulate the TPM2_FlushContext command to see if a session is
> > > > > being
> > > > > flushed and manually clear it from the space.
> > > > > 
> > > > > We also fully flush all sessions on device close.
> > > > 
> > > > Some big overview comments without going deeply into details. I
> > > > will
> > > > use more time for this once the 
> > > > 
> > > > Please do not use handle allocation code for sessions. This
> > > > commit
> > > > makes the implementation a mess. Just use the phandle directly
> > > > and
> > > > have array of session phandles for each space.
> > > > 
> > > > I would also almost require to have at minimum two patches: one
> > > > that
> > > > implements purely isolation and another that implements swapping.
> > > > 
> > > > It might be for example that I want to land TPM spaces with
> > > > session
> > > > isolation to one release and swapping to n+1 because my hunch
> > > > tells
> > > > me that it is better to bake the swapping part for a while.
> > > > 
> > > > One more thing. Maybe commit messages should speak uniformally
> > > > about
> > > > TPM spaces? They are a tool to implement resource manager, not a
> > > > resource manager.
> > > 
> > > Yes, so Ken also had a reply to this which the Mailing List seems
> > > to
> > > have eaten:
> > > 
> > > > This looks like session handles are virtualized.  I believe that
> > > > this 
> > > > will break the HMAC for commands (e.g. TPM2_PolicySecret) that
> > > > have
> > > > a session handle in the handle area.  The session's handle is its
> > > > "Name" and is included in the cpHash (command parameter hash)
> > > > data 
> > > > that is HMACed. 
> > > 
> > > Basically this means that the advice to virtualize session handles
> > > in
> > > the TCG RM document is wrong and we have to use physical handles.
> > > I'll
> > > redo the implementation for this ... and now, since we'll have
> > > nothing
> > > to use as an index, it probably does make sense to have sessions in
> > > a
> > > separate array.  I can also separate isolation from context
> > > switching
> > > ... although I really think this is less optimal: my TPM only
> > > allows
> > > three active context handles, so if we don't context switch them
> > > identially to transient object (which it also only allows three of)
> > > I'm
> > > going to run out.  I actually redid my openssl_tpm_engine patches
> > > so
> > > they use session handles for parameter encryption and HMAC based
> > > authority, so this may end up biting me soon ...
> > > 
> > > James
> > 
> > This might be obvious to you but saying this just in case: everytime 
> > a session handle is created, you must traverse through struct 
> > tpm_space instances and check if they have that physical handle and 
> > remove it so that they don't leak.
> 
> Actually, the code pre-emptively manages handles, so it ensures that we
> actively collect them (in the flush emulation and the command attribute
> processing).  I've got an unpublished patch that actually does global
> session management, but it's part of the resource exhaustion stuff
> which I'm still testing.
> 
> Note that when we save contexts, there's no need even to check.  If a
> handle gets re-used, the old context won't load.

Yes, I understand. In that way swapping simplifies things like it does
with transient objects.

> > I would probably just create a linked list of struct tpm_space
> > instances. Radix tree does not make much sense with the amount of
> > sessions you might except to have on a system simultaneously.
> 
> Global array.  The tpm has a small limit for total number of sessions
> TPM_PT_SESSIONS_MAX (it's about 64 in every TPM I've seen).

[x]

> > Anyway, this kind of lazy approach where you clean up as new stuff
> > gets created is probably also the most straight forward...
> 
> I think we need to manage handles proactively.  The problems come when
> we have more saved contexts than TPM_PT_SESSIONS_MAX, so we need to
> aggressively flush them.

I leave it up to you decide how you want to do it. In some ways swapping
does simplify things so it's not yet obvious whether a kind of breakdown
where isolation would be done as an iterative step would make sense in
the first place.

Forget what I adviced and I'll look forward for a new patch :-)

/Jarkko