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. 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. Anyway, this kind of lazy approach where you clean up as new stuff gets created is probably also the most straight forward... /Jarkko
