| Message ID | 20181118124753.18613-17-jarkko.sakkinen@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Removed 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). > > The consequence of this is that this commit removes the remaining nested > calls. > > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> This is still NACK from my side Tomas > --- > drivers/char/tpm/tpm-chip.c | 25 ++++++++++++------------- > 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, 17 insertions(+), 29 deletions(-) > > diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c index > 65f1561eba81..7ad4d9045e4c 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; > > @@ -166,11 +154,17 @@ int tpm_try_get_ops(struct tpm_chip *chip) > > down_read(&chip->ops_sem); > if (!chip->ops) > - goto out_lock; > + goto out_ops; > > mutex_lock(&chip->tpm_mutex); > + rc = tpm_chip_start(chip, 0); > + if (rc) > + goto out_lock; > + > return 0; > out_lock: > + mutex_unlock(&chip->tpm_mutex); > +out_ops: > up_read(&chip->ops_sem); > put_device(&chip->dev); > return rc; > @@ -186,6 +180,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 +558,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 2c79284ffd4e..21ac8da94d90 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); > if (ret < 0) > break; > rc = be32_to_cpu(header->return_code); 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 ced1dc91ba6f..d913715d30aa 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 Sun, Nov 18, 2018 at 10:52:46PM +0000, Winkler, Tomas wrote:
> This is still NACK from my side
Last time you spoke about tboot intervention but I don't see why as even
sending a single command is not atomic in the true sense of the word
i.e. if there was a problem that would already affect the existing code
and would essentially mean that tboot itself is broken.
/Jarkko
> > On Sun, Nov 18, 2018 at 10:52:46PM +0000, Winkler, Tomas wrote: > > This is still NACK from my side > > Last time you spoke about tboot intervention but I don't see why as even > sending a single command is not atomic in the true sense of the word i.e. if > there was a problem that would already affect the existing code and would > essentially mean that tboot itself is broken. So I've consulted the issue, I wasn't not correct in the description. Tboot cannot acquire the locality, unless the host driver relinquish it, so the issue is opposite, driver is expected to relinquish the locality for tboot to work correctly. This is current status, other behavior will need a different implementation on both sides. Hopes that clears the question. Thanks
On 11/18/18 7:47 AM, Jarkko Sakkinen 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). > > The consequence of this is that this commit removes the remaining nested > calls. > > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com> > --- > drivers/char/tpm/tpm-chip.c | 25 ++++++++++++------------- > 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, 17 insertions(+), 29 deletions(-) > > diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c > index 65f1561eba81..7ad4d9045e4c 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; > > @@ -166,11 +154,17 @@ int tpm_try_get_ops(struct tpm_chip *chip) > > down_read(&chip->ops_sem); > if (!chip->ops) > - goto out_lock; > + goto out_ops; > > mutex_lock(&chip->tpm_mutex); > + rc = tpm_chip_start(chip, 0); > + if (rc) > + goto out_lock; > + > return 0; > out_lock: > + mutex_unlock(&chip->tpm_mutex); > +out_ops: > up_read(&chip->ops_sem); > put_device(&chip->dev); > return rc; > @@ -186,6 +180,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 +558,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 2c79284ffd4e..21ac8da94d90 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); > if (ret < 0) > break; > rc = be32_to_cpu(header->return_code); > 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 ced1dc91ba6f..d913715d30aa 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; >
On Mon, Nov 19, 2018 at 01:17:56PM +0000, Winkler, Tomas wrote: > > On Sun, Nov 18, 2018 at 10:52:46PM +0000, Winkler, Tomas wrote: > > > This is still NACK from my side > > > > Last time you spoke about tboot intervention but I don't see why as even > > sending a single command is not atomic in the true sense of the word i.e. if > > there was a problem that would already affect the existing code and would > > essentially mean that tboot itself is broken. > So I've consulted the issue, I wasn't not correct in the description. > Tboot cannot acquire the locality, unless the host driver relinquish > it, so the issue is opposite, driver is expected to relinquish the > locality for tboot to work correctly. This is current status, other > behavior will need a different implementation on both sides. The locality is reliquished after the command sequence (load and save TPM space + command in /dev/tpmX case). Is this really such a big issue? The intention is not keep the locality reserved by the driver. /Jarkko
diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c index 65f1561eba81..7ad4d9045e4c 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; @@ -166,11 +154,17 @@ int tpm_try_get_ops(struct tpm_chip *chip) down_read(&chip->ops_sem); if (!chip->ops) - goto out_lock; + goto out_ops; mutex_lock(&chip->tpm_mutex); + rc = tpm_chip_start(chip, 0); + if (rc) + goto out_lock; + return 0; out_lock: + mutex_unlock(&chip->tpm_mutex); +out_ops: up_read(&chip->ops_sem); put_device(&chip->dev); return rc; @@ -186,6 +180,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 +558,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 2c79284ffd4e..21ac8da94d90 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); if (ret < 0) break; rc = be32_to_cpu(header->return_code); 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 ced1dc91ba6f..d913715d30aa 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 | 25 ++++++++++++------------- 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, 17 insertions(+), 29 deletions(-)