Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1560162 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] tpm: add session handles to the save and restore of the  tpm2 space manager James Bottomley <jejb@linux.vnet.ibm.com> - 2017-01-17 00:20 +0100
    Re: [PATCH] tpm: add session handles to the save and restore of the  tpm2 space manager Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2017-01-17 08:30 +0100
      Re: [PATCH] tpm: add session handles to the save and restore of the  tpm2 space manager James Bottomley <jejb@linux.vnet.ibm.com> - 2017-01-17 15:20 +0100
        Re: [PATCH] tpm: add session handles to the save and restore of the  tpm2 space manager Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2017-01-17 17:40 +0100
    Re: [tpmdd-devel] [PATCH] tpm: add session handles to the save and  restore of the tpm2 space manager Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2017-01-17 17:30 +0100

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

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

[toc] | [next] | [standalone]

#1560299

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

[toc] | [prev] | [next] | [standalone]

#1560630

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.

> 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).

> 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.

James

[toc] | [prev] | [next] | [standalone]

#1560777

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

[toc] | [prev] | [next] | [standalone]

#1560767 — Re: [tpmdd-devel] [PATCH] tpm: add session handles to the save and restore of the tpm2 space manager

On Tue, Jan 17, 2017 at 09:01:59AM -0500, Ken Goldman wrote:
> On 1/16/2017 6:18 PM, James Bottomley wrote:
> >
> > 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 identically 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 ...
> 
> I think you have to context save sessions, just as you do with transient 
> objects.  Otherwise, only one process at a time can connect.

Isolation is self-contained step that can be tested and possible
regressions catched.

I could even consider landing isolation in one release and swapping in
subsequent in order to keep the release content more digestable for
upper layer maintainers and risk of causing major regressions small.

/Jarkko

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web