| Message ID | 20190116212342.24524-1-jarkko.sakkinen@linux.intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Remove nested TPM operations | expand |
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
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 >
> > 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
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>
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
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
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
> > 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
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
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
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