Re: [PATCH 1/2] tpm2: add session handle context saving and restoring to the space code

[Jarkko Sakkinen]

On Thu, Jan 26, 2017 at 08:26:19AM -0800, James Bottomley wrote:
> On Thu, 2017-01-26 at 14:51 +0200, Jarkko Sakkinen wrote:
> [...]
> > > diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> > > index c48255e..b77fc60 100644
> > > --- a/drivers/char/tpm/tpm.h
> > > +++ b/drivers/char/tpm/tpm.h
> > > @@ -159,6 +159,8 @@ enum tpm2_cc_attrs {
> > >  struct tpm_space {
> > >  	u32 context_tbl[3];
> > >  	u8 *context_buf;
> > > +	u32 session_tbl[6];
> > > +	u8 *session_buf;
> > >  };
> > >  
> > >  enum tpm_chip_flags {
> > > @@ -588,4 +590,5 @@ int tpm2_prepare_space(struct tpm_chip *chip,
> > > struct tpm_space *space, u32 cc,
> > >  		       u8 *cmd);
> > >  int tpm2_commit_space(struct tpm_chip *chip, struct tpm_space
> > > *space,
> > >  		      u32 cc, u8 *buf, size_t bufsiz);
> > > +void tpm2_flush_space(struct tpm_chip *chip, struct tpm_space
> > > *space);
> > 
> > Why the extra parameter?
> 
> Because it was called from tpms-dev in release to flush all the
> sessions still active.  At that point the work space is gone, so we
> have to call it with the real space.  However, I realised that callsite
> didn't hold the tpm_mutex like it should and that we don't need to
> flush the contexts because they'll be guaranteed empty, so I added a
> new function tpm2_kill_space() that does this.
> 
> > >  #endif
> > > diff --git a/drivers/char/tpm/tpm2-space.c b/drivers/char/tpm/tpm2
> > > -space.c
> > > index 7fd2fc5..ba4310a 100644
> > > --- a/drivers/char/tpm/tpm2-space.c
> > > +++ b/drivers/char/tpm/tpm2-space.c
> > > @@ -59,11 +59,16 @@ static int tpm2_load_context(struct tpm_chip
> > > *chip, u8 *buf,
> > >  
> > >  	rc = tpm_transmit_cmd(chip, NULL, tbuf.data, PAGE_SIZE, 4,
> > >  			      TPM_TRANSMIT_UNLOCKED, NULL);
> > > +
> > 
> > cruft
> 
> Removed.
> 
> [...] 
> > > @@ -215,17 +265,20 @@ int tpm2_prepare_space(struct tpm_chip *chip,
> > > struct tpm_space *space, u32 cc,
> > >  
> > >  	memcpy(&chip->work_space.context_tbl, &space->context_tbl,
> > >  	       sizeof(space->context_tbl));
> > > +	memcpy(&chip->work_space.session_tbl, &space->session_tbl,
> > > +	       sizeof(space->session_tbl));
> > >  	memcpy(chip->work_space.context_buf, space->context_buf,
> > > PAGE_SIZE);
> > > +	memcpy(chip->work_space.session_buf, space->session_buf,
> > > PAGE_SIZE);
> > 
> > For transient objects the rollback is straight forward and totally
> > predictable. Given that with sessions you always keep some 
> > information in the TPM the rollback would be a bit more complicated.
> 
> There is basically no rollback unless you really want to understand
> what the commands did.  If we get a fault on the context save or load
> that isn't one we understand, like TPM_RC_HANDLE meaning the session
> won't load or TPM_RC_REFERENCE_H0 meaning the session no longer exists,
> then I think the only option is flushing everything.
> 
> I'd like us to agree on a hard failure model: if we get some
> unexplained error during our context loads or saves, we should clear
> out the entire space (which would allow us to use pointers instead of
> copyring) but we don't.  So we follow the soft failure.  However,
> sessions will mostly fail to load after this with RPM_RC_HANDLE, so we
> get the equivalent of soft failure for transients and hard failure for
> sessions.
> 
> > Now your code seems to just keep the previous session_buf, doesn't
> > it? Does that always work or not?
> 
> Yes, after tpm_flush_space, the session memory is gone and all the
> sessions will refuse to load with RC_HANDLE, so we end up effectively
> clearing them out.  It seemed better to do it this way than to try to
> special case all the session stuff.

Maybe it would make sense to have a comment in code to state this?
Otherwise, I'm fine with this semantics.

> > PS. I have a high-level idea of attack vectors that are prevented by
> > having meta-data for session inside the TPM but can you point me to
> > the correct place in the TPM 2.0 specification that discusses about
> > this?
> 
> The problem is replay.  If I'm snooping your TPM commands and I capture
> your session context, if we don't have replay detection, I can re-load
> your session HMAC and replay your command because the session has the
> authorization or the encryption.  The TPM designers thought the only
> way to avoid replay was to use a counter which was part of the session
> context which increments every time a session is saved.  On loading you
> check that the counters match and fail if they don't.  The only way to
> implement this is to keep a memory of the counter in the TPM, hence the
> annoying vestigial sessions.
> 
> It's note 2 of the architecture Part 1 guide, chapter "15.4 Session
> Handles (MSO=02_16 and 03_16 )"
> 
> James

Thank you.

Once these are in shape I think we have something that could be put into
a release, don't you think?

/Jarkko