| Message ID | 1472000243-7088-1-git-send-email-jarkko.sakkinen@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Jason, I guess this should be now less intrusive than the original one? The main goal was to make it as backportable as possible. /Jarkko On Tue, Aug 23, 2016 at 08:57:22PM -0400, Jarkko Sakkinen wrote: > Unseal and load operations should be done as an atomic operation. This > commit introduces unlocked tpm_transmit() so that tpm2_unseal_trusted() > can do the locking by itself. > > v2: Introduced an unlocked unseal operation instead of changing locking > strategy in order to make less intrusive bug fix and thus more > backportable. > > v3: Have also separate __tpm_transmit() that takes 'flags' in order to > better localize the bug fix and make it easier to backport. > > v4: Cleaned up the control flow in tpm2_unseal_trusted. Added the > missing 'Fixes' line. > > Fixes: 0fe5480303a1 ("keys, trusted: seal/unseal with TPM 2.0 chips") > CC: stable@vger.kernel.org > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > --- > drivers/char/tpm/tpm-interface.c | 16 +++++++++------- > drivers/char/tpm/tpm.h | 25 +++++++++++++++++++++---- > drivers/char/tpm/tpm2-cmd.c | 12 ++++++++---- > 3 files changed, 38 insertions(+), 15 deletions(-) > > diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c > index 43ef0ef..80e702a 100644 > --- a/drivers/char/tpm/tpm-interface.c > +++ b/drivers/char/tpm/tpm-interface.c > @@ -330,8 +330,8 @@ EXPORT_SYMBOL_GPL(tpm_calc_ordinal_duration); > /* > * Internal kernel interface to transmit TPM commands > */ > -ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf, > - size_t bufsiz) > +ssize_t __tpm_transmit(struct tpm_chip *chip, const char *buf, > + size_t bufsiz, unsigned int flags) > { > ssize_t rc; > u32 count, ordinal; > @@ -350,7 +350,8 @@ ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf, > return -E2BIG; > } > > - mutex_lock(&chip->tpm_mutex); > + if (flags & TPM_TRANSMIT_LOCK) > + mutex_lock(&chip->tpm_mutex); > > rc = chip->ops->send(chip, (u8 *) buf, count); > if (rc < 0) { > @@ -393,20 +394,21 @@ out_recv: > dev_err(&chip->dev, > "tpm_transmit: tpm_recv: error %zd\n", rc); > out: > - mutex_unlock(&chip->tpm_mutex); > + if (flags & TPM_TRANSMIT_LOCK) > + mutex_unlock(&chip->tpm_mutex); > return rc; > } > > #define TPM_DIGEST_SIZE 20 > #define TPM_RET_CODE_IDX 6 > > -ssize_t tpm_transmit_cmd(struct tpm_chip *chip, void *cmd, > - int len, const char *desc) > +ssize_t __tpm_transmit_cmd(struct tpm_chip *chip, void *cmd, > + int len, const char *desc, unsigned int flags) > { > struct tpm_output_header *header; > int err; > > - len = tpm_transmit(chip, (u8 *) cmd, len); > + len = __tpm_transmit(chip, cmd, len, flags); > if (len < 0) > return len; > else if (len < TPM_HEADER_SIZE) > diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h > index 6e002c4..0a4abf0 100644 > --- a/drivers/char/tpm/tpm.h > +++ b/drivers/char/tpm/tpm.h > @@ -476,12 +476,29 @@ extern dev_t tpm_devt; > extern const struct file_operations tpm_fops; > extern struct idr dev_nums_idr; > > +enum tpm_transmit_flags { > + TPM_TRANSMIT_LOCK, > +}; > + > +ssize_t __tpm_transmit(struct tpm_chip *chip, const char *buf, > + size_t bufsiz, unsigned int flags); > +ssize_t __tpm_transmit_cmd(struct tpm_chip *chip, void *cmd, int len, > + const char *desc, unsigned int flags); > + > +static inline ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf, > + size_t bufsiz) > +{ > + return __tpm_transmit(chip, buf, bufsiz, TPM_TRANSMIT_LOCK); > +} > + > +static inline ssize_t tpm_transmit_cmd(struct tpm_chip *chip, void *cmd, > + int len, const char *desc) > +{ > + return __tpm_transmit_cmd(chip, cmd, len, desc, TPM_TRANSMIT_LOCK); > +} > + > ssize_t tpm_getcap(struct tpm_chip *chip, __be32 subcap_id, cap_t *cap, > const char *desc); > -ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf, > - size_t bufsiz); > -ssize_t tpm_transmit_cmd(struct tpm_chip *chip, void *cmd, int len, > - const char *desc); > int tpm_get_timeouts(struct tpm_chip *chip); > int tpm1_auto_startup(struct tpm_chip *chip); > int tpm_do_selftest(struct tpm_chip *chip); > diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c > index 499f405..a2a0314 100644 > --- a/drivers/char/tpm/tpm2-cmd.c > +++ b/drivers/char/tpm/tpm2-cmd.c > @@ -576,7 +576,7 @@ static int tpm2_load(struct tpm_chip *chip, > goto out; > } > > - rc = tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "loading blob"); > + rc = __tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "loading blob", 0); > if (!rc) > *blob_handle = be32_to_cpup( > (__be32 *) &buf.data[TPM_HEADER_SIZE]); > @@ -604,7 +604,8 @@ static void tpm2_flush_context(struct tpm_chip *chip, u32 handle) > > tpm_buf_append_u32(&buf, handle); > > - rc = tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "flushing context"); > + rc = __tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "flushing context", > + 0); > if (rc) > dev_warn(&chip->dev, "0x%08x was not flushed, rc=%d\n", handle, > rc); > @@ -635,7 +636,7 @@ static int tpm2_unseal(struct tpm_chip *chip, > options->blobauth /* hmac */, > TPM_DIGEST_SIZE); > > - rc = tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "unsealing"); > + rc = __tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "unsealing", 0); > if (rc > 0) > rc = -EPERM; > > @@ -668,14 +669,17 @@ int tpm2_unseal_trusted(struct tpm_chip *chip, > u32 blob_handle; > int rc; > > + mutex_lock(&chip->tpm_mutex); > rc = tpm2_load(chip, payload, options, &blob_handle); > if (rc) > - return rc; > + goto out; > > rc = tpm2_unseal(chip, payload, options, blob_handle); > > tpm2_flush_context(chip, blob_handle); > > +out: > + mutex_unlock(&chip->tpm_mutex); > return rc; > } > > -- > 2.7.4 > -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Aug 23, 2016 at 08:57:22PM -0400, Jarkko Sakkinen wrote: > + if (flags & TPM_TRANSMIT_LOCK) > + mutex_lock(&chip->tpm_mutex); I think I would invert this. UNLOCKED is the exceptional case, so I'd make the 0 flags lock. If we see UNLOCKED in the caller then we know to audit for locking, 0 is much less obvious. > @@ -576,7 +576,7 @@ static int tpm2_load(struct tpm_chip *chip, > goto out; > } > > - rc = tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "loading blob"); > + rc = __tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "loading blob", 0); All these points should accept a flags too and the caller should pass in the TPM_TRASNMIT_UNLOCKED if it needs it.. > + mutex_lock(&chip->tpm_mutex); > rc = tpm2_load(chip, payload, options, &blob_handle); > if (rc) So when we read here we see the pattern: > + mutex_lock(&chip->tpm_mutex); > rc = tpm2_load(chip, payload, options, &blob_handle, TPM_TRASNMIT_UNLOCKED); Which is much easier to audit.. Jason -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Aug 25, 2016 at 12:30:59PM -0600, Jason Gunthorpe wrote: > On Tue, Aug 23, 2016 at 08:57:22PM -0400, Jarkko Sakkinen wrote: > > > + if (flags & TPM_TRANSMIT_LOCK) > > + mutex_lock(&chip->tpm_mutex); > > I think I would invert this. UNLOCKED is the exceptional case, so I'd > make the 0 flags lock. If we see UNLOCKED in the caller then we know > to audit for locking, 0 is much less obvious. I'm fine with either way. > > @@ -576,7 +576,7 @@ static int tpm2_load(struct tpm_chip *chip, > > goto out; > > } > > > > - rc = tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "loading blob"); > > + rc = __tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "loading blob", 0); > > All these points should accept a flags too and the caller should pass > in the TPM_TRASNMIT_UNLOCKED if it needs it.. For this bug fix it makes sense to implement it the way I did because it needs to be applied to multiple releases (I think I've underlined this in my changelog). If you think this is high priority, I can make the next revision into patch set of two patches. The second patch would implement the change you suggested. /Jarkko -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Aug 25, 2016 at 05:06:10PM -0400, Jarkko Sakkinen wrote: > On Thu, Aug 25, 2016 at 12:30:59PM -0600, Jason Gunthorpe wrote: > > On Tue, Aug 23, 2016 at 08:57:22PM -0400, Jarkko Sakkinen wrote: > > > > > + if (flags & TPM_TRANSMIT_LOCK) > > > + mutex_lock(&chip->tpm_mutex); > > > > I think I would invert this. UNLOCKED is the exceptional case, so I'd > > make the 0 flags lock. If we see UNLOCKED in the caller then we know > > to audit for locking, 0 is much less obvious. > > I'm fine with either way. > > > > @@ -576,7 +576,7 @@ static int tpm2_load(struct tpm_chip *chip, > > > goto out; > > > } > > > > > > - rc = tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "loading blob"); > > > + rc = __tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "loading blob", 0); > > > > All these points should accept a flags too and the caller should pass > > in the TPM_TRASNMIT_UNLOCKED if it needs it.. > > For this bug fix it makes sense to implement it the way I did because it > needs to be applied to multiple releases (I think I've underlined this > in my changelog). You shouldn't compromise the mainline kernel to ease backporting, I'm not sure why adding a flags to tpm2_load would be a problem for the -stable kernels? It is generally better to make the backports move the older kernels closer to mainline than to have them be something else, it makes it easier to apply future backport fixes. > If you think this is high priority, I can make the next revision into > patch set of two patches. The second patch would implement the change > you suggested. Yes, I think it is important the locking requirement be very clear from the code. Jason -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c index 43ef0ef..80e702a 100644 --- a/drivers/char/tpm/tpm-interface.c +++ b/drivers/char/tpm/tpm-interface.c @@ -330,8 +330,8 @@ EXPORT_SYMBOL_GPL(tpm_calc_ordinal_duration); /* * Internal kernel interface to transmit TPM commands */ -ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf, - size_t bufsiz) +ssize_t __tpm_transmit(struct tpm_chip *chip, const char *buf, + size_t bufsiz, unsigned int flags) { ssize_t rc; u32 count, ordinal; @@ -350,7 +350,8 @@ ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf, return -E2BIG; } - mutex_lock(&chip->tpm_mutex); + if (flags & TPM_TRANSMIT_LOCK) + mutex_lock(&chip->tpm_mutex); rc = chip->ops->send(chip, (u8 *) buf, count); if (rc < 0) { @@ -393,20 +394,21 @@ out_recv: dev_err(&chip->dev, "tpm_transmit: tpm_recv: error %zd\n", rc); out: - mutex_unlock(&chip->tpm_mutex); + if (flags & TPM_TRANSMIT_LOCK) + mutex_unlock(&chip->tpm_mutex); return rc; } #define TPM_DIGEST_SIZE 20 #define TPM_RET_CODE_IDX 6 -ssize_t tpm_transmit_cmd(struct tpm_chip *chip, void *cmd, - int len, const char *desc) +ssize_t __tpm_transmit_cmd(struct tpm_chip *chip, void *cmd, + int len, const char *desc, unsigned int flags) { struct tpm_output_header *header; int err; - len = tpm_transmit(chip, (u8 *) cmd, len); + len = __tpm_transmit(chip, cmd, len, flags); if (len < 0) return len; else if (len < TPM_HEADER_SIZE) diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h index 6e002c4..0a4abf0 100644 --- a/drivers/char/tpm/tpm.h +++ b/drivers/char/tpm/tpm.h @@ -476,12 +476,29 @@ extern dev_t tpm_devt; extern const struct file_operations tpm_fops; extern struct idr dev_nums_idr; +enum tpm_transmit_flags { + TPM_TRANSMIT_LOCK, +}; + +ssize_t __tpm_transmit(struct tpm_chip *chip, const char *buf, + size_t bufsiz, unsigned int flags); +ssize_t __tpm_transmit_cmd(struct tpm_chip *chip, void *cmd, int len, + const char *desc, unsigned int flags); + +static inline ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf, + size_t bufsiz) +{ + return __tpm_transmit(chip, buf, bufsiz, TPM_TRANSMIT_LOCK); +} + +static inline ssize_t tpm_transmit_cmd(struct tpm_chip *chip, void *cmd, + int len, const char *desc) +{ + return __tpm_transmit_cmd(chip, cmd, len, desc, TPM_TRANSMIT_LOCK); +} + ssize_t tpm_getcap(struct tpm_chip *chip, __be32 subcap_id, cap_t *cap, const char *desc); -ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf, - size_t bufsiz); -ssize_t tpm_transmit_cmd(struct tpm_chip *chip, void *cmd, int len, - const char *desc); int tpm_get_timeouts(struct tpm_chip *chip); int tpm1_auto_startup(struct tpm_chip *chip); int tpm_do_selftest(struct tpm_chip *chip); diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c index 499f405..a2a0314 100644 --- a/drivers/char/tpm/tpm2-cmd.c +++ b/drivers/char/tpm/tpm2-cmd.c @@ -576,7 +576,7 @@ static int tpm2_load(struct tpm_chip *chip, goto out; } - rc = tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "loading blob"); + rc = __tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "loading blob", 0); if (!rc) *blob_handle = be32_to_cpup( (__be32 *) &buf.data[TPM_HEADER_SIZE]); @@ -604,7 +604,8 @@ static void tpm2_flush_context(struct tpm_chip *chip, u32 handle) tpm_buf_append_u32(&buf, handle); - rc = tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "flushing context"); + rc = __tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "flushing context", + 0); if (rc) dev_warn(&chip->dev, "0x%08x was not flushed, rc=%d\n", handle, rc); @@ -635,7 +636,7 @@ static int tpm2_unseal(struct tpm_chip *chip, options->blobauth /* hmac */, TPM_DIGEST_SIZE); - rc = tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "unsealing"); + rc = __tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "unsealing", 0); if (rc > 0) rc = -EPERM; @@ -668,14 +669,17 @@ int tpm2_unseal_trusted(struct tpm_chip *chip, u32 blob_handle; int rc; + mutex_lock(&chip->tpm_mutex); rc = tpm2_load(chip, payload, options, &blob_handle); if (rc) - return rc; + goto out; rc = tpm2_unseal(chip, payload, options, blob_handle); tpm2_flush_context(chip, blob_handle); +out: + mutex_unlock(&chip->tpm_mutex); return rc; }
Unseal and load operations should be done as an atomic operation. This commit introduces unlocked tpm_transmit() so that tpm2_unseal_trusted() can do the locking by itself. v2: Introduced an unlocked unseal operation instead of changing locking strategy in order to make less intrusive bug fix and thus more backportable. v3: Have also separate __tpm_transmit() that takes 'flags' in order to better localize the bug fix and make it easier to backport. v4: Cleaned up the control flow in tpm2_unseal_trusted. Added the missing 'Fixes' line. Fixes: 0fe5480303a1 ("keys, trusted: seal/unseal with TPM 2.0 chips") CC: stable@vger.kernel.org Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> --- drivers/char/tpm/tpm-interface.c | 16 +++++++++------- drivers/char/tpm/tpm.h | 25 +++++++++++++++++++++---- drivers/char/tpm/tpm2-cmd.c | 12 ++++++++---- 3 files changed, 38 insertions(+), 15 deletions(-)