| Message ID | 20181108141541.12832-17-jarkko.sakkinen@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Remove nested TPM operations | expand |
> Call tpm_chip_start() and tpm_chip_stop() in > > * tpm_try_get_ops() and tpm_put_ops() > * tpm_chip_register() > * tpm2_del_space() > > And remove these calls from tpm_transmit(). The core reason for this change > is that in tpm_vtpm_proxy a locality change requires a virtual TPM command > (a command made up just for that driver). > I don't think you can do that, locality has to be request for each command, as for example tboot can request higher locality any time. Same for cmd_ready()/go_idle() powergatin, you will prevent the whole platform entering power save state. Thanks Tomas > The consequence of this is that this commit removes the remaining nested > calls. > > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > --- > drivers/char/tpm/tpm-chip.c | 19 ++++++------------- > drivers/char/tpm/tpm-interface.c | 4 ---- > drivers/char/tpm/tpm.h | 9 --------- > drivers/char/tpm/tpm2-space.c | 5 ++++- > drivers/char/tpm/tpm_vtpm_proxy.c | 3 +-- > 5 files changed, 11 insertions(+), 29 deletions(-) > > diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c index > 65f1561eba81..87570182f75e 100644 > --- a/drivers/char/tpm/tpm-chip.c > +++ b/drivers/char/tpm/tpm-chip.c > @@ -41,9 +41,6 @@ static int tpm_request_locality(struct tpm_chip *chip, > unsigned int flags) { > int rc; > > - if (flags & TPM_TRANSMIT_NESTED) > - return 0; > - > if (!chip->ops->request_locality) > return 0; > > @@ -59,9 +56,6 @@ static void tpm_relinquish_locality(struct tpm_chip > *chip, unsigned int flags) { > int rc; > > - if (flags & TPM_TRANSMIT_NESTED) > - return; > - > if (!chip->ops->relinquish_locality) > return; > > @@ -74,9 +68,6 @@ static void tpm_relinquish_locality(struct tpm_chip > *chip, unsigned int flags) > > static int tpm_cmd_ready(struct tpm_chip *chip, unsigned int flags) { > - if (flags & TPM_TRANSMIT_NESTED) > - return 0; > - > if (!chip->ops->cmd_ready) > return 0; > > @@ -85,9 +76,6 @@ static int tpm_cmd_ready(struct tpm_chip *chip, > unsigned int flags) > > static int tpm_go_idle(struct tpm_chip *chip, unsigned int flags) { > - if (flags & TPM_TRANSMIT_NESTED) > - return 0; > - > if (!chip->ops->go_idle) > return 0; > > @@ -169,7 +157,7 @@ int tpm_try_get_ops(struct tpm_chip *chip) > goto out_lock; > > mutex_lock(&chip->tpm_mutex); > - return 0; > + return tpm_chip_start(chip, 0); > out_lock: > up_read(&chip->ops_sem); > put_device(&chip->dev); > @@ -186,6 +174,7 @@ EXPORT_SYMBOL_GPL(tpm_try_get_ops); > */ > void tpm_put_ops(struct tpm_chip *chip) { > + tpm_chip_stop(chip, 0); > mutex_unlock(&chip->tpm_mutex); > up_read(&chip->ops_sem); > put_device(&chip->dev); > @@ -563,7 +552,11 @@ int tpm_chip_register(struct tpm_chip *chip) { > int rc; > > + rc = tpm_chip_start(chip, 0); > + if (rc) > + return rc; > rc = tpm_auto_startup(chip); > + tpm_chip_stop(chip, 0); > if (rc) > return rc; > > diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm- > interface.c > index 888c9923fca1..5c04c0d9aaba 100644 > --- a/drivers/char/tpm/tpm-interface.c > +++ b/drivers/char/tpm/tpm-interface.c > @@ -168,11 +168,7 @@ ssize_t tpm_transmit(struct tpm_chip *chip, u8 > *buf, size_t bufsiz, > memcpy(save, buf, save_size); > > for (;;) { > - ret = tpm_chip_start(chip, flags); > - if (ret) > - return ret; > ret = tpm_try_transmit(chip, buf, bufsiz, flags); > - tpm_chip_stop(chip, flags); > > rc = be32_to_cpu(header->return_code); > if (rc != TPM2_RC_RETRY && rc != TPM2_RC_TESTING) diff -- > git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h index > c42a75710b70..f9d56dfd0d20 100644 > --- a/drivers/char/tpm/tpm.h > +++ b/drivers/char/tpm/tpm.h > @@ -485,15 +485,6 @@ extern const struct file_operations tpm_fops; > extern const struct file_operations tpmrm_fops; extern struct idr > dev_nums_idr; > > -/** > - * enum tpm_transmit_flags - flags for tpm_transmit() > - * > - * %TPM_TRANSMIT_NESTED: discard setup steps (power management, > locality) > - */ > -enum tpm_transmit_flags { > - TPM_TRANSMIT_NESTED = BIT(0), > -}; > - > ssize_t tpm_transmit(struct tpm_chip *chip, u8 *buf, size_t bufsiz, > unsigned int flags); > ssize_t tpm_transmit_cmd(struct tpm_chip *chip, struct tpm_buf *buf, diff - > -git a/drivers/char/tpm/tpm2-space.c b/drivers/char/tpm/tpm2-space.c > index 341d4b90ba2f..8ed063b54040 100644 > --- a/drivers/char/tpm/tpm2-space.c > +++ b/drivers/char/tpm/tpm2-space.c > @@ -60,7 +60,10 @@ int tpm2_init_space(struct tpm_space *space) void > tpm2_del_space(struct tpm_chip *chip, struct tpm_space *space) { > mutex_lock(&chip->tpm_mutex); > - tpm2_flush_sessions(chip, space); > + if (!tpm_chip_start(chip, 0)) { > + tpm2_flush_sessions(chip, space); > + tpm_chip_stop(chip, 0); > + } > mutex_unlock(&chip->tpm_mutex); > kfree(space->context_buf); > kfree(space->session_buf); > diff --git a/drivers/char/tpm/tpm_vtpm_proxy.c > b/drivers/char/tpm/tpm_vtpm_proxy.c > index e8a1da2810a9..a4bb60e163cc 100644 > --- a/drivers/char/tpm/tpm_vtpm_proxy.c > +++ b/drivers/char/tpm/tpm_vtpm_proxy.c > @@ -417,8 +417,7 @@ static int vtpm_proxy_request_locality(struct > tpm_chip *chip, int locality) > > proxy_dev->state |= STATE_DRIVER_COMMAND; > > - rc = tpm_transmit_cmd(chip, &buf, 0, TPM_TRANSMIT_NESTED, > - "attempting to set locality"); > + rc = tpm_transmit_cmd(chip, &buf, 0, 0, "attempting to set locality"); > > proxy_dev->state &= ~STATE_DRIVER_COMMAND; > > -- > 2.19.1
On Thu, Nov 08, 2018 at 06:38:59PM +0000, Winkler, Tomas wrote: > > Call tpm_chip_start() and tpm_chip_stop() in > > > > * tpm_try_get_ops() and tpm_put_ops() > > * tpm_chip_register() > > * tpm2_del_space() > > > > And remove these calls from tpm_transmit(). The core reason for this change > > is that in tpm_vtpm_proxy a locality change requires a virtual TPM command > > (a command made up just for that driver). > > > I don't think you can do that, locality has to be request for each > command, as for example tboot can request higher locality any time. That could be a potential problem. How tboot intervention gets prevented without this patch? > Same for cmd_ready()/go_idle() powergatin, you will prevent the whole > platform entering power save state. Why would you want that in a middle of using the TPM anyway? > Thanks > Tomas /Jarkko
> > On Thu, Nov 08, 2018 at 06:38:59PM +0000, Winkler, Tomas wrote: > > > Call tpm_chip_start() and tpm_chip_stop() in > > > > > > * tpm_try_get_ops() and tpm_put_ops() > > > * tpm_chip_register() > > > * tpm2_del_space() > > > > > > And remove these calls from tpm_transmit(). The core reason for this > > > change is that in tpm_vtpm_proxy a locality change requires a > > > virtual TPM command (a command made up just for that driver). > > > > > I don't think you can do that, locality has to be request for each > > command, as for example tboot can request higher locality any time. > > That could be a potential problem. How tboot intervention gets prevented > without this patch? As it was said, need to request locality and relinquish it for each command, I believe thought this is not required for client platforms only for servers. > > > Same for cmd_ready()/go_idle() powergatin, you will prevent the whole > > platform entering power save state. > > Why would you want that in a middle of using the TPM anyway? If I'm reading this patch correctly you do cmd_ready when tpm is opened, it doesn't mean there is a real traffic, so why to keep it awake. Thanks Tomas
On Fri, Nov 09, 2018 at 09:37:48PM +0000, Winkler, Tomas wrote: > > On Thu, Nov 08, 2018 at 06:38:59PM +0000, Winkler, Tomas wrote: > > > > Call tpm_chip_start() and tpm_chip_stop() in > > > > > > > > * tpm_try_get_ops() and tpm_put_ops() > > > > * tpm_chip_register() > > > > * tpm2_del_space() > > > > > > > > And remove these calls from tpm_transmit(). The core reason for this > > > > change is that in tpm_vtpm_proxy a locality change requires a > > > > virtual TPM command (a command made up just for that driver). > > > > > > > I don't think you can do that, locality has to be request for each > > > command, as for example tboot can request higher locality any time. > > > > That could be a potential problem. How tboot intervention gets prevented > > without this patch? > As it was said, need to request locality and relinquish it for each > command, I believe thought this is not required for client platforms > only for servers. And what I'm trying to under is why so. If the intervention can happen at any time that would imply that even if you would request and relinquish locality for a single TPM command, the intervention could happen in the middle. That is why I'm asking why without this patch things are just fine. /Jarkko
> -----Original Message----- > From: Jarkko Sakkinen [mailto:jarkko.sakkinen@linux.intel.com] > Sent: Tuesday, November 13, 2018 13:12 > To: Winkler, Tomas <tomas.winkler@intel.com> > Cc: linux-integrity@vger.kernel.org; linux-security-module@vger.kernel.org; > James Bottomley <James.Bottomley@HansenPartnership.com>; Struk, > Tadeusz <tadeusz.struk@intel.com>; Stefan Berger > <stefanb@linux.vnet.ibm.com>; Nayna Jain <nayna@linux.ibm.com>; Peter > Huewe <peterhuewe@gmx.de>; Jason Gunthorpe <jgg@ziepe.ca>; Arnd > Bergmann <arnd@arndb.de>; Greg Kroah-Hartman > <gregkh@linuxfoundation.org>; open list <linux-kernel@vger.kernel.org> > Subject: Re: [PATCH v5 16/17] tpm: take TPM chip power gating out of > tpm_transmit() > > On Fri, Nov 09, 2018 at 09:37:48PM +0000, Winkler, Tomas wrote: > > > On Thu, Nov 08, 2018 at 06:38:59PM +0000, Winkler, Tomas wrote: > > > > > Call tpm_chip_start() and tpm_chip_stop() in > > > > > > > > > > * tpm_try_get_ops() and tpm_put_ops() > > > > > * tpm_chip_register() > > > > > * tpm2_del_space() > > > > > > > > > > And remove these calls from tpm_transmit(). The core reason for > > > > > this change is that in tpm_vtpm_proxy a locality change requires > > > > > a virtual TPM command (a command made up just for that driver). > > > > > > > > > I don't think you can do that, locality has to be request for > > > > each command, as for example tboot can request higher locality any > time. > > > > > > That could be a potential problem. How tboot intervention gets > > > prevented without this patch? > > As it was said, need to request locality and relinquish it for each > > command, I believe thought this is not required for client platforms > > only for servers. > > And what I'm trying to under is why so. > > If the intervention can happen at any time that would imply that even if you > would request and relinquish locality for a single TPM command, the > intervention could happen in the middle. That is why I'm asking why without > this patch things are just fine. Yes, w/o this constrain it would be okay to request locality only once, we can ask tboot ask again but at the time the requirement was that locality can be taken of at any point, I believe that the locality won't be granted till a single command is completed. Anyhow still the power gating is wrong in this patch do not ignore that part. Thanks Tomas
On Tue, Nov 13, 2018 at 11:58:58AM +0000, Winkler, Tomas wrote: > Yes, w/o this constrain it would be okay to request locality only once, > we can ask tboot ask again but at the time the requirement was that locality can be taken of at any point, > I believe that the locality won't be granted till a single command is completed. But then tboot could race between requesting a locality and sending the command (like immediately after the locality has been granted). > Anyhow still the power gating is wrong in this patch do not ignore > that part. I can certainly take steps back with gating. It is not a blocker for other changes but I do need to explain it in the changelog what problem does cause. I'm not ignoring it. I just don't understad what is wrong. > Thanks > Tomas /Jarkko
diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c index 65f1561eba81..87570182f75e 100644 --- a/drivers/char/tpm/tpm-chip.c +++ b/drivers/char/tpm/tpm-chip.c @@ -41,9 +41,6 @@ static int tpm_request_locality(struct tpm_chip *chip, unsigned int flags) { int rc; - if (flags & TPM_TRANSMIT_NESTED) - return 0; - if (!chip->ops->request_locality) return 0; @@ -59,9 +56,6 @@ static void tpm_relinquish_locality(struct tpm_chip *chip, unsigned int flags) { int rc; - if (flags & TPM_TRANSMIT_NESTED) - return; - if (!chip->ops->relinquish_locality) return; @@ -74,9 +68,6 @@ static void tpm_relinquish_locality(struct tpm_chip *chip, unsigned int flags) static int tpm_cmd_ready(struct tpm_chip *chip, unsigned int flags) { - if (flags & TPM_TRANSMIT_NESTED) - return 0; - if (!chip->ops->cmd_ready) return 0; @@ -85,9 +76,6 @@ static int tpm_cmd_ready(struct tpm_chip *chip, unsigned int flags) static int tpm_go_idle(struct tpm_chip *chip, unsigned int flags) { - if (flags & TPM_TRANSMIT_NESTED) - return 0; - if (!chip->ops->go_idle) return 0; @@ -169,7 +157,7 @@ int tpm_try_get_ops(struct tpm_chip *chip) goto out_lock; mutex_lock(&chip->tpm_mutex); - return 0; + return tpm_chip_start(chip, 0); out_lock: up_read(&chip->ops_sem); put_device(&chip->dev); @@ -186,6 +174,7 @@ EXPORT_SYMBOL_GPL(tpm_try_get_ops); */ void tpm_put_ops(struct tpm_chip *chip) { + tpm_chip_stop(chip, 0); mutex_unlock(&chip->tpm_mutex); up_read(&chip->ops_sem); put_device(&chip->dev); @@ -563,7 +552,11 @@ int tpm_chip_register(struct tpm_chip *chip) { int rc; + rc = tpm_chip_start(chip, 0); + if (rc) + return rc; rc = tpm_auto_startup(chip); + tpm_chip_stop(chip, 0); if (rc) return rc; diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c index 888c9923fca1..5c04c0d9aaba 100644 --- a/drivers/char/tpm/tpm-interface.c +++ b/drivers/char/tpm/tpm-interface.c @@ -168,11 +168,7 @@ ssize_t tpm_transmit(struct tpm_chip *chip, u8 *buf, size_t bufsiz, memcpy(save, buf, save_size); for (;;) { - ret = tpm_chip_start(chip, flags); - if (ret) - return ret; ret = tpm_try_transmit(chip, buf, bufsiz, flags); - tpm_chip_stop(chip, flags); rc = be32_to_cpu(header->return_code); if (rc != TPM2_RC_RETRY && rc != TPM2_RC_TESTING) diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h index c42a75710b70..f9d56dfd0d20 100644 --- a/drivers/char/tpm/tpm.h +++ b/drivers/char/tpm/tpm.h @@ -485,15 +485,6 @@ extern const struct file_operations tpm_fops; extern const struct file_operations tpmrm_fops; extern struct idr dev_nums_idr; -/** - * enum tpm_transmit_flags - flags for tpm_transmit() - * - * %TPM_TRANSMIT_NESTED: discard setup steps (power management, locality) - */ -enum tpm_transmit_flags { - TPM_TRANSMIT_NESTED = BIT(0), -}; - ssize_t tpm_transmit(struct tpm_chip *chip, u8 *buf, size_t bufsiz, unsigned int flags); ssize_t tpm_transmit_cmd(struct tpm_chip *chip, struct tpm_buf *buf, diff --git a/drivers/char/tpm/tpm2-space.c b/drivers/char/tpm/tpm2-space.c index 341d4b90ba2f..8ed063b54040 100644 --- a/drivers/char/tpm/tpm2-space.c +++ b/drivers/char/tpm/tpm2-space.c @@ -60,7 +60,10 @@ int tpm2_init_space(struct tpm_space *space) void tpm2_del_space(struct tpm_chip *chip, struct tpm_space *space) { mutex_lock(&chip->tpm_mutex); - tpm2_flush_sessions(chip, space); + if (!tpm_chip_start(chip, 0)) { + tpm2_flush_sessions(chip, space); + tpm_chip_stop(chip, 0); + } mutex_unlock(&chip->tpm_mutex); kfree(space->context_buf); kfree(space->session_buf); diff --git a/drivers/char/tpm/tpm_vtpm_proxy.c b/drivers/char/tpm/tpm_vtpm_proxy.c index e8a1da2810a9..a4bb60e163cc 100644 --- a/drivers/char/tpm/tpm_vtpm_proxy.c +++ b/drivers/char/tpm/tpm_vtpm_proxy.c @@ -417,8 +417,7 @@ static int vtpm_proxy_request_locality(struct tpm_chip *chip, int locality) proxy_dev->state |= STATE_DRIVER_COMMAND; - rc = tpm_transmit_cmd(chip, &buf, 0, TPM_TRANSMIT_NESTED, - "attempting to set locality"); + rc = tpm_transmit_cmd(chip, &buf, 0, 0, "attempting to set locality"); proxy_dev->state &= ~STATE_DRIVER_COMMAND;
Call tpm_chip_start() and tpm_chip_stop() in * tpm_try_get_ops() and tpm_put_ops() * tpm_chip_register() * tpm2_del_space() And remove these calls from tpm_transmit(). The core reason for this change is that in tpm_vtpm_proxy a locality change requires a virtual TPM command (a command made up just for that driver). The consequence of this is that this commit removes the remaining nested calls. Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> --- drivers/char/tpm/tpm-chip.c | 19 ++++++------------- drivers/char/tpm/tpm-interface.c | 4 ---- drivers/char/tpm/tpm.h | 9 --------- drivers/char/tpm/tpm2-space.c | 5 ++++- drivers/char/tpm/tpm_vtpm_proxy.c | 3 +-- 5 files changed, 11 insertions(+), 29 deletions(-)