[v10,00/17] Remove nested TPM operations
mbox series

Message ID 20190116212342.24524-1-jarkko.sakkinen@linux.intel.com
Headers show
Series
  • Remove nested TPM operations
Related show

Message

Jarkko Sakkinen Jan. 16, 2019, 9:23 p.m. UTC
Make the changes necessary to detach TPM space code and TPM activation
code out of the tpm_transmit() flow because of both of these can cause
nested tpm_transmit() calls. The nesteds calls make the whole flow hard
to maintain, and thus, it is better to just fix things now before this
turns into a bigger mess.

v10:
* Use void pointers to avoid unnecessary casts in functions paramaters
  where it makes sense.

v9:
* Fixed again tpm_try_get_ops().
* Added missing reviewed-by's.

v8:
* Re-add the check for ret < 0 after calling tpm_try_transmit() that
  was dropped by mistake while moving code.
* Fix error fallback for tpm_try_get_ops() when tpm_chip_start()
  fails.

v7:
*  Reorganize series so that more trivial and self-contained changes are
   in the head.

v6:
* When tpm_validate_commmand() was moved to tpm2-space.c, the struct for
  the TPM header was incorrectly declared as struct tpm_input_header.
* Fix return value in tpm_validate_command().

v5:
* Add the missing rev's from Stefan Berger.

v4:
* Return 0 from pcrs_show() when tpm1_pcr_read() fails.
* Fix error handling flow in tpm_try_transmit().
* Replace struct tpm_input_header and struct tpm_output_header with
  struct tpm_header.

v3:
* Encapsulate power gating code to tpm_chip_start() and tpm_chip_stop().
* Move TPM power gating code and locking to tpm_try_get_ops() and
  tpm_put_ops().
* Call power gating code directly in tpm_chip_register() and
  tpm2_del_space().

v2:
* Print tpm2_commit_space() error inside tpm2_commit_space()
* Error code was not printed when recv() callback failed. It is
  fixed in this version.
* Added a patch that removes @space from tpm_transmit().
* Fixed a regression in earlier series. Forgot to amend the change
  from the staging area that renames NESTED to UNLOCKED in tpm2-space.c.
Jarkko Sakkinen (17):
  tpm: use tpm_buf in tpm_transmit_cmd() as the IO parameter
  tpm: fix invalid return value in pubek_show()
  tpm: return 0 from pcrs_show() when tpm1_pcr_read() fails
  tpm: print tpm2_commit_space() error inside tpm2_commit_space()
  tpm: declare struct tpm_header
  tpm: access command header through struct in tpm_try_transmit()
  tpm: encapsulate tpm_dev_transmit()
  tpm: call tpm2_flush_space() on error in tpm_try_transmit()
  tpm: clean up tpm_try_transmit() error handling flow
  tpm: move tpm_validate_commmand() to tpm2-space.c
  tpm: move TPM space code out of tpm_transmit()
  tpm: remove @space from tpm_transmit()
  tpm: use tpm_try_get_ops() in tpm-sysfs.c.
  tpm: remove TPM_TRANSMIT_UNLOCKED flag
  tpm: introduce tpm_chip_start() and tpm_chip_stop()
  tpm: take TPM chip power gating out of tpm_transmit()
  tpm: remove @flags from tpm_transmit()

 drivers/char/tpm/tpm-chip.c       | 109 ++++++++++++
 drivers/char/tpm/tpm-dev-common.c |  45 ++++-
 drivers/char/tpm/tpm-interface.c  | 264 ++++++------------------------
 drivers/char/tpm/tpm-sysfs.c      | 138 ++++++++++------
 drivers/char/tpm/tpm.h            |  64 +++-----
 drivers/char/tpm/tpm1-cmd.c       |  28 +---
 drivers/char/tpm/tpm2-cmd.c       |  72 +++-----
 drivers/char/tpm/tpm2-space.c     |  91 +++++++---
 drivers/char/tpm/tpm_i2c_atmel.c  |   5 +-
 drivers/char/tpm/tpm_vtpm_proxy.c |  12 +-
 drivers/char/tpm/xen-tpmfront.c   |   2 +-
 11 files changed, 409 insertions(+), 421 deletions(-)

Comments

Jarkko Sakkinen Jan. 23, 2019, 6:20 p.m. UTC | #1
On Wed, Jan 16, 2019 at 11:23:25PM +0200, Jarkko Sakkinen wrote:
> Make the changes necessary to detach TPM space code and TPM activation
> code out of the tpm_transmit() flow because of both of these can cause
> nested tpm_transmit() calls. The nesteds calls make the whole flow hard
> to maintain, and thus, it is better to just fix things now before this
> turns into a bigger mess.

Any reasons not to merge this soon?

/Jarkko
Stefan Berger Jan. 23, 2019, 6:53 p.m. UTC | #2
On 1/23/19 1:20 PM, Jarkko Sakkinen wrote:
> On Wed, Jan 16, 2019 at 11:23:25PM +0200, Jarkko Sakkinen wrote:
>> Make the changes necessary to detach TPM space code and TPM activation
>> code out of the tpm_transmit() flow because of both of these can cause
>> nested tpm_transmit() calls. The nesteds calls make the whole flow hard
>> to maintain, and thus, it is better to just fix things now before this
>> turns into a bigger mess.
> Any reasons not to merge this soon?

I suppose v10 hasn't changed anything signinficat. So, not from my 
perspective. Were you waiting for more Reviewed-by's?

>
> /Jarkko
>
Winkler, Tomas Jan. 23, 2019, 6:59 p.m. UTC | #3
> 
> On 1/23/19 1:20 PM, Jarkko Sakkinen wrote:
> > On Wed, Jan 16, 2019 at 11:23:25PM +0200, Jarkko Sakkinen wrote:
> >> Make the changes necessary to detach TPM space code and TPM
> >> activation code out of the tpm_transmit() flow because of both of
> >> these can cause nested tpm_transmit() calls. The nesteds calls make
> >> the whole flow hard to maintain, and thus, it is better to just fix
> >> things now before this turns into a bigger mess.
> > Any reasons not to merge this soon?
> 
> I suppose v10 hasn't changed anything signinficat. So, not from my perspective.
> Were you waiting for more Reviewed-by's?
> 
I'm okay, in theory I've cleaned all my concern regarding idle and locality . I'm just want do few more tests on Sunday.

Thanks
Tomas
Jerry Snitselaar Jan. 25, 2019, 1:05 a.m. UTC | #4
On Wed Jan 16 19, Jarkko Sakkinen wrote:
>Make the changes necessary to detach TPM space code and TPM activation
>code out of the tpm_transmit() flow because of both of these can cause
>nested tpm_transmit() calls. The nesteds calls make the whole flow hard
>to maintain, and thus, it is better to just fix things now before this
>turns into a bigger mess.
>
>v10:
>* Use void pointers to avoid unnecessary casts in functions paramaters
>  where it makes sense.
>
>v9:
>* Fixed again tpm_try_get_ops().
>* Added missing reviewed-by's.
>
>v8:
>* Re-add the check for ret < 0 after calling tpm_try_transmit() that
>  was dropped by mistake while moving code.
>* Fix error fallback for tpm_try_get_ops() when tpm_chip_start()
>  fails.
>
>v7:
>*  Reorganize series so that more trivial and self-contained changes are
>   in the head.
>
>v6:
>* When tpm_validate_commmand() was moved to tpm2-space.c, the struct for
>  the TPM header was incorrectly declared as struct tpm_input_header.
>* Fix return value in tpm_validate_command().
>
>v5:
>* Add the missing rev's from Stefan Berger.
>
>v4:
>* Return 0 from pcrs_show() when tpm1_pcr_read() fails.
>* Fix error handling flow in tpm_try_transmit().
>* Replace struct tpm_input_header and struct tpm_output_header with
>  struct tpm_header.
>
>v3:
>* Encapsulate power gating code to tpm_chip_start() and tpm_chip_stop().
>* Move TPM power gating code and locking to tpm_try_get_ops() and
>  tpm_put_ops().
>* Call power gating code directly in tpm_chip_register() and
>  tpm2_del_space().
>
>v2:
>* Print tpm2_commit_space() error inside tpm2_commit_space()
>* Error code was not printed when recv() callback failed. It is
>  fixed in this version.
>* Added a patch that removes @space from tpm_transmit().
>* Fixed a regression in earlier series. Forgot to amend the change
>  from the staging area that renames NESTED to UNLOCKED in tpm2-space.c.
>Jarkko Sakkinen (17):
>  tpm: use tpm_buf in tpm_transmit_cmd() as the IO parameter
>  tpm: fix invalid return value in pubek_show()
>  tpm: return 0 from pcrs_show() when tpm1_pcr_read() fails
>  tpm: print tpm2_commit_space() error inside tpm2_commit_space()
>  tpm: declare struct tpm_header
>  tpm: access command header through struct in tpm_try_transmit()
>  tpm: encapsulate tpm_dev_transmit()
>  tpm: call tpm2_flush_space() on error in tpm_try_transmit()
>  tpm: clean up tpm_try_transmit() error handling flow
>  tpm: move tpm_validate_commmand() to tpm2-space.c
>  tpm: move TPM space code out of tpm_transmit()
>  tpm: remove @space from tpm_transmit()
>  tpm: use tpm_try_get_ops() in tpm-sysfs.c.
>  tpm: remove TPM_TRANSMIT_UNLOCKED flag
>  tpm: introduce tpm_chip_start() and tpm_chip_stop()
>  tpm: take TPM chip power gating out of tpm_transmit()
>  tpm: remove @flags from tpm_transmit()
>
> drivers/char/tpm/tpm-chip.c       | 109 ++++++++++++
> drivers/char/tpm/tpm-dev-common.c |  45 ++++-
> drivers/char/tpm/tpm-interface.c  | 264 ++++++------------------------
> drivers/char/tpm/tpm-sysfs.c      | 138 ++++++++++------
> drivers/char/tpm/tpm.h            |  64 +++-----
> drivers/char/tpm/tpm1-cmd.c       |  28 +---
> drivers/char/tpm/tpm2-cmd.c       |  72 +++-----
> drivers/char/tpm/tpm2-space.c     |  91 +++++++---
> drivers/char/tpm/tpm_i2c_atmel.c  |   5 +-
> drivers/char/tpm/tpm_vtpm_proxy.c |  12 +-
> drivers/char/tpm/xen-tpmfront.c   |   2 +-
> 11 files changed, 409 insertions(+), 421 deletions(-)
>
>-- 
>2.19.1
>

Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>
Jarkko Sakkinen Jan. 29, 2019, 12:31 p.m. UTC | #5
On Wed, Jan 23, 2019 at 01:53:44PM -0500, Stefan Berger wrote:
> On 1/23/19 1:20 PM, Jarkko Sakkinen wrote:
> > On Wed, Jan 16, 2019 at 11:23:25PM +0200, Jarkko Sakkinen wrote:
> > > Make the changes necessary to detach TPM space code and TPM activation
> > > code out of the tpm_transmit() flow because of both of these can cause
> > > nested tpm_transmit() calls. The nesteds calls make the whole flow hard
> > > to maintain, and thus, it is better to just fix things now before this
> > > turns into a bigger mess.
> > Any reasons not to merge this soon?
> 
> I suppose v10 hasn't changed anything signinficat. So, not from my
> perspective. Were you waiting for more Reviewed-by's?

Yeah, for example TPM space touching changes would be good to peer
check with James. I could have easily forgotten some implementation
detail, and it has been very stable piece off code, so don't want
to break it. Guess won't yet try to put this v5.1.

/Jarkko
Jarkko Sakkinen Jan. 29, 2019, 12:33 p.m. UTC | #6
On Wed, Jan 23, 2019 at 06:59:48PM +0000, Winkler, Tomas wrote:
> > 
> > On 1/23/19 1:20 PM, Jarkko Sakkinen wrote:
> > > On Wed, Jan 16, 2019 at 11:23:25PM +0200, Jarkko Sakkinen wrote:
> > >> Make the changes necessary to detach TPM space code and TPM
> > >> activation code out of the tpm_transmit() flow because of both of
> > >> these can cause nested tpm_transmit() calls. The nesteds calls make
> > >> the whole flow hard to maintain, and thus, it is better to just fix
> > >> things now before this turns into a bigger mess.
> > > Any reasons not to merge this soon?
> > 
> > I suppose v10 hasn't changed anything signinficat. So, not from my perspective.
> > Were you waiting for more Reviewed-by's?
> > 
> I'm okay, in theory I've cleaned all my concern regarding idle and
> locality . I'm just want do few more tests on Sunday.

I'm in no rush merging this piece of code, as long as it gets merged
at some point. Thomas, can you take a look at the PTT issue we are
having.

See my discussion with Linus with subject "Getting weird TPM error after
rebasing my tree to security/next-general". You have better knowledge
of the internals so I would really like to hear your feedback on that.
We need to get a fix for that for v5.0.

/Jarkko
Jarkko Sakkinen Jan. 29, 2019, 12:33 p.m. UTC | #7
On Thu, Jan 24, 2019 at 06:05:19PM -0700, Jerry Snitselaar wrote:
> On Wed Jan 16 19, Jarkko Sakkinen wrote:
> > Make the changes necessary to detach TPM space code and TPM activation
> > code out of the tpm_transmit() flow because of both of these can cause
> > nested tpm_transmit() calls. The nesteds calls make the whole flow hard
> > to maintain, and thus, it is better to just fix things now before this
> > turns into a bigger mess.
> > 
> > v10:
> > * Use void pointers to avoid unnecessary casts in functions paramaters
> >  where it makes sense.
> > 
> > v9:
> > * Fixed again tpm_try_get_ops().
> > * Added missing reviewed-by's.
> > 
> > v8:
> > * Re-add the check for ret < 0 after calling tpm_try_transmit() that
> >  was dropped by mistake while moving code.
> > * Fix error fallback for tpm_try_get_ops() when tpm_chip_start()
> >  fails.
> > 
> > v7:
> > *  Reorganize series so that more trivial and self-contained changes are
> >   in the head.
> > 
> > v6:
> > * When tpm_validate_commmand() was moved to tpm2-space.c, the struct for
> >  the TPM header was incorrectly declared as struct tpm_input_header.
> > * Fix return value in tpm_validate_command().
> > 
> > v5:
> > * Add the missing rev's from Stefan Berger.
> > 
> > v4:
> > * Return 0 from pcrs_show() when tpm1_pcr_read() fails.
> > * Fix error handling flow in tpm_try_transmit().
> > * Replace struct tpm_input_header and struct tpm_output_header with
> >  struct tpm_header.
> > 
> > v3:
> > * Encapsulate power gating code to tpm_chip_start() and tpm_chip_stop().
> > * Move TPM power gating code and locking to tpm_try_get_ops() and
> >  tpm_put_ops().
> > * Call power gating code directly in tpm_chip_register() and
> >  tpm2_del_space().
> > 
> > v2:
> > * Print tpm2_commit_space() error inside tpm2_commit_space()
> > * Error code was not printed when recv() callback failed. It is
> >  fixed in this version.
> > * Added a patch that removes @space from tpm_transmit().
> > * Fixed a regression in earlier series. Forgot to amend the change
> >  from the staging area that renames NESTED to UNLOCKED in tpm2-space.c.
> > Jarkko Sakkinen (17):
> >  tpm: use tpm_buf in tpm_transmit_cmd() as the IO parameter
> >  tpm: fix invalid return value in pubek_show()
> >  tpm: return 0 from pcrs_show() when tpm1_pcr_read() fails
> >  tpm: print tpm2_commit_space() error inside tpm2_commit_space()
> >  tpm: declare struct tpm_header
> >  tpm: access command header through struct in tpm_try_transmit()
> >  tpm: encapsulate tpm_dev_transmit()
> >  tpm: call tpm2_flush_space() on error in tpm_try_transmit()
> >  tpm: clean up tpm_try_transmit() error handling flow
> >  tpm: move tpm_validate_commmand() to tpm2-space.c
> >  tpm: move TPM space code out of tpm_transmit()
> >  tpm: remove @space from tpm_transmit()
> >  tpm: use tpm_try_get_ops() in tpm-sysfs.c.
> >  tpm: remove TPM_TRANSMIT_UNLOCKED flag
> >  tpm: introduce tpm_chip_start() and tpm_chip_stop()
> >  tpm: take TPM chip power gating out of tpm_transmit()
> >  tpm: remove @flags from tpm_transmit()
> > 
> > drivers/char/tpm/tpm-chip.c       | 109 ++++++++++++
> > drivers/char/tpm/tpm-dev-common.c |  45 ++++-
> > drivers/char/tpm/tpm-interface.c  | 264 ++++++------------------------
> > drivers/char/tpm/tpm-sysfs.c      | 138 ++++++++++------
> > drivers/char/tpm/tpm.h            |  64 +++-----
> > drivers/char/tpm/tpm1-cmd.c       |  28 +---
> > drivers/char/tpm/tpm2-cmd.c       |  72 +++-----
> > drivers/char/tpm/tpm2-space.c     |  91 +++++++---
> > drivers/char/tpm/tpm_i2c_atmel.c  |   5 +-
> > drivers/char/tpm/tpm_vtpm_proxy.c |  12 +-
> > drivers/char/tpm/xen-tpmfront.c   |   2 +-
> > 11 files changed, 409 insertions(+), 421 deletions(-)
> > 
> > -- 
> > 2.19.1
> > 
> 
> Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>

Thanks!

/Jarkko
Winkler, Tomas Jan. 29, 2019, 2:16 p.m. UTC | #8
> 
> On Wed, Jan 23, 2019 at 06:59:48PM +0000, Winkler, Tomas wrote:
> > >
> > > On 1/23/19 1:20 PM, Jarkko Sakkinen wrote:
> > > > On Wed, Jan 16, 2019 at 11:23:25PM +0200, Jarkko Sakkinen wrote:
> > > >> Make the changes necessary to detach TPM space code and TPM
> > > >> activation code out of the tpm_transmit() flow because of both of
> > > >> these can cause nested tpm_transmit() calls. The nesteds calls
> > > >> make the whole flow hard to maintain, and thus, it is better to
> > > >> just fix things now before this turns into a bigger mess.
> > > > Any reasons not to merge this soon?
> > >
> > > I suppose v10 hasn't changed anything signinficat. So, not from my
> perspective.
> > > Were you waiting for more Reviewed-by's?
> > >
> > I'm okay, in theory I've cleaned all my concern regarding idle and
> > locality . I'm just want do few more tests on Sunday.
> 
> I'm in no rush merging this piece of code, as long as it gets merged at some
> point. Thomas, can you take a look at the PTT issue we are having.
> 
> See my discussion with Linus with subject "Getting weird TPM error after
> rebasing my tree to security/next-general". You have better knowledge of the
> internals so I would really like to hear your feedback on that.
> We need to get a fix for that for v5.0.


Yes, looking into in new ICL, had some issues to bring the platform on.  It's really failing (selftest) , we are looking into it.
Will also testing the nesting removal patches. 

Thanks
Jarkko Sakkinen Jan. 29, 2019, 6:30 p.m. UTC | #9
On Tue, Jan 29, 2019 at 02:16:56PM +0000, Winkler, Tomas wrote:
> Yes, looking into in new ICL, had some issues to bring the platform
> on.  It's really failing (selftest) , we are looking into it.  Will
> also testing the nesting removal patches. 

Great, thank you.

/Jarkko
James Bottomley Jan. 31, 2019, 12:28 a.m. UTC | #10
On Tue, 2019-01-29 at 14:31 +0200, Jarkko Sakkinen wrote:
> On Wed, Jan 23, 2019 at 01:53:44PM -0500, Stefan Berger wrote:
> > On 1/23/19 1:20 PM, Jarkko Sakkinen wrote:
> > > On Wed, Jan 16, 2019 at 11:23:25PM +0200, Jarkko Sakkinen wrote:
> > > > Make the changes necessary to detach TPM space code and TPM
> > > > activation
> > > > code out of the tpm_transmit() flow because of both of these
> > > > can cause
> > > > nested tpm_transmit() calls. The nesteds calls make the whole
> > > > flow hard
> > > > to maintain, and thus, it is better to just fix things now
> > > > before this
> > > > turns into a bigger mess.
> > > 
> > > Any reasons not to merge this soon?
> > 
> > I suppose v10 hasn't changed anything signinficat. So, not from my
> > perspective. Were you waiting for more Reviewed-by's?
> 
> Yeah, for example TPM space touching changes would be good to peer
> check with James. I could have easily forgotten some implementation
> detail, and it has been very stable piece off code, so don't want
> to break it. Guess won't yet try to put this v5.1.

So the implementation detail I was looking for: internal kernel use of
tpm_transmit_cmd() without tpm_find/try_get_ops() doesn't seem to
exist, so I think this is all safe.  You can add my

Reviewed-by: James Bottomley <James.Bottomley@HansenPartnership.com>

But I've got to say I can't test this yet because you've made a huge
problem for me in the tpm security patches: they introduce a kernel
space which now becomes somewhat problematic because the space handling
moved into the device common code.  To get both these things to work
together so I can test it, space handling is going to have to come
slightly down from device common code so the kernel can use it.

James
Jarkko Sakkinen Jan. 31, 2019, 4:11 p.m. UTC | #11
On Wed, Jan 30, 2019 at 04:28:42PM -0800, James Bottomley wrote:
> On Tue, 2019-01-29 at 14:31 +0200, Jarkko Sakkinen wrote:
> > On Wed, Jan 23, 2019 at 01:53:44PM -0500, Stefan Berger wrote:
> > > On 1/23/19 1:20 PM, Jarkko Sakkinen wrote:
> > > > On Wed, Jan 16, 2019 at 11:23:25PM +0200, Jarkko Sakkinen wrote:
> > > > > Make the changes necessary to detach TPM space code and TPM
> > > > > activation
> > > > > code out of the tpm_transmit() flow because of both of these
> > > > > can cause
> > > > > nested tpm_transmit() calls. The nesteds calls make the whole
> > > > > flow hard
> > > > > to maintain, and thus, it is better to just fix things now
> > > > > before this
> > > > > turns into a bigger mess.
> > > > 
> > > > Any reasons not to merge this soon?
> > > 
> > > I suppose v10 hasn't changed anything signinficat. So, not from my
> > > perspective. Were you waiting for more Reviewed-by's?
> > 
> > Yeah, for example TPM space touching changes would be good to peer
> > check with James. I could have easily forgotten some implementation
> > detail, and it has been very stable piece off code, so don't want
> > to break it. Guess won't yet try to put this v5.1.
> 
> So the implementation detail I was looking for: internal kernel use of
> tpm_transmit_cmd() without tpm_find/try_get_ops() doesn't seem to
> exist, so I think this is all safe.  You can add my
> 
> Reviewed-by: James Bottomley <James.Bottomley@HansenPartnership.com>

Thank you. I'll send a new revision soonish.

> But I've got to say I can't test this yet because you've made a huge
> problem for me in the tpm security patches: they introduce a kernel
> space which now becomes somewhat problematic because the space handling
> moved into the device common code.  To get both these things to work
> together so I can test it, space handling is going to have to come
> slightly down from device common code so the kernel can use it.

Yeah, obviously. I'll apply these patches right after the next PR so
you will have a more stable platform to work on after that. Stefan
has tested these, and then there is one full cycle to fix details
if we find issues.

/Jarkko