Message ID | 20220809193921.544546-1-svenva@chromium.org (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | tpm: fix potential race condition in suspend/resume | expand |
On Tue, Aug 09, 2022 at 07:39:18PM +0000, Sven van Ashbrook wrote: > Concurrent accesses to the tpm chip are prevented by allowing only a > single thread at a time to obtain a tpm chip reference through > tpm_try_get_ops(). However, the tpm's suspend function does not use > this mechanism, so when the tpm api is called by a kthread which > does not get frozen on suspend (such as the hw_random kthread) > it's possible that the tpm is used when already in suspend, or > in use while in the process of suspending. > > This is seen on certain ChromeOS platforms - low-probability warnings > are generated during suspend. In this case, the tpm attempted to read data > from a tpm chip on an already-suspended bus. > > i2c_designware i2c_designware.1: Transfer while suspended > > Fix: > 1. prevent concurrent execution of tpm accesses and suspend/ > resume, by letting suspend/resume grab the tpm_mutex. > 2. before commencing a tpm access, check if the tpm chip is already > suspended. Fail with -EAGAIN if so. > > Tested by running 6000 suspend/resume cycles back-to-back on a > ChromeOS "brya" device. The intermittent warnings reliably > disappear after applying this patch. No system issues were observed. > > Cc: <stable@vger.kernel.org> > Fixes: e891db1a18bf ("tpm: turn on TPM on suspend for TPM 1.x") > Signed-off-by: Sven van Ashbrook <svenva@chromium.org> > --- > drivers/char/tpm/tpm-interface.c | 16 ++++++++++++++++ > include/linux/tpm.h | 2 ++ > 2 files changed, 18 insertions(+) > > diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c > index 1621ce818705..16ca490fd483 100644 > --- a/drivers/char/tpm/tpm-interface.c > +++ b/drivers/char/tpm/tpm-interface.c > @@ -82,6 +82,11 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip, void *buf, size_t bufsiz) > return -E2BIG; > } > > + if (chip->is_suspended) { > + dev_info(&chip->dev, "blocking transmit while suspended\n"); > + return -EAGAIN; > + } > + > rc = chip->ops->send(chip, buf, count); > if (rc < 0) { > if (rc != -EPIPE) > @@ -394,6 +399,8 @@ int tpm_pm_suspend(struct device *dev) > if (!chip) > return -ENODEV; > > + mutex_lock(&chip->tpm_mutex); > + > if (chip->flags & TPM_CHIP_FLAG_ALWAYS_POWERED) > goto suspended; > > @@ -411,6 +418,11 @@ int tpm_pm_suspend(struct device *dev) > } > > suspended: > + if (!rc) > + chip->is_suspended = true; > + > + mutex_unlock(&chip->tpm_mutex); > + > return rc; > } > EXPORT_SYMBOL_GPL(tpm_pm_suspend); > @@ -426,6 +438,10 @@ int tpm_pm_resume(struct device *dev) > if (chip == NULL) > return -ENODEV; > > + mutex_lock(&chip->tpm_mutex); > + chip->is_suspended = false; > + mutex_unlock(&chip->tpm_mutex); > + > return 0; > } > EXPORT_SYMBOL_GPL(tpm_pm_resume); > diff --git a/include/linux/tpm.h b/include/linux/tpm.h > index d7c67581929f..0fbc1a43ae80 100644 > --- a/include/linux/tpm.h > +++ b/include/linux/tpm.h > @@ -131,6 +131,8 @@ struct tpm_chip { > int dev_num; /* /dev/tpm# */ > unsigned long is_open; /* only one allowed */ > > + bool is_suspended; > + > char hwrng_name[64]; > struct hwrng hwrng; > > -- > 2.37.1.559.g78731f0fdb-goog > What about adding TPM_CHIP_FLAG_SUSPENDED instead? BR, Jarkko
On Thu, Aug 11, 2022 at 1:02 AM Jarkko Sakkinen <jarkko@kernel.org> wrote: > > What about adding TPM_CHIP_FLAG_SUSPENDED instead? Thank you for the feedback, Jarkko. After thinking this over, I believe this patch only moves kernel warnings around. Will re-post soon with a fresh approach, intended to fix the underlying issue rather than the symptom. So please disregard this patch.
On Thu, Aug 11, 2022 at 09:09:38AM -0400, Sven van Ashbrook wrote: > On Thu, Aug 11, 2022 at 1:02 AM Jarkko Sakkinen <jarkko@kernel.org> wrote: > > > > What about adding TPM_CHIP_FLAG_SUSPENDED instead? > > Thank you for the feedback, Jarkko. After thinking this over, I > believe this patch only moves kernel warnings around. Will re-post > soon with a fresh approach, intended to fix the underlying issue > rather than the symptom. > > So please disregard this patch. np BR, Jarkko
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c index 1621ce818705..16ca490fd483 100644 --- a/drivers/char/tpm/tpm-interface.c +++ b/drivers/char/tpm/tpm-interface.c @@ -82,6 +82,11 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip, void *buf, size_t bufsiz) return -E2BIG; } + if (chip->is_suspended) { + dev_info(&chip->dev, "blocking transmit while suspended\n"); + return -EAGAIN; + } + rc = chip->ops->send(chip, buf, count); if (rc < 0) { if (rc != -EPIPE) @@ -394,6 +399,8 @@ int tpm_pm_suspend(struct device *dev) if (!chip) return -ENODEV; + mutex_lock(&chip->tpm_mutex); + if (chip->flags & TPM_CHIP_FLAG_ALWAYS_POWERED) goto suspended; @@ -411,6 +418,11 @@ int tpm_pm_suspend(struct device *dev) } suspended: + if (!rc) + chip->is_suspended = true; + + mutex_unlock(&chip->tpm_mutex); + return rc; } EXPORT_SYMBOL_GPL(tpm_pm_suspend); @@ -426,6 +438,10 @@ int tpm_pm_resume(struct device *dev) if (chip == NULL) return -ENODEV; + mutex_lock(&chip->tpm_mutex); + chip->is_suspended = false; + mutex_unlock(&chip->tpm_mutex); + return 0; } EXPORT_SYMBOL_GPL(tpm_pm_resume); diff --git a/include/linux/tpm.h b/include/linux/tpm.h index d7c67581929f..0fbc1a43ae80 100644 --- a/include/linux/tpm.h +++ b/include/linux/tpm.h @@ -131,6 +131,8 @@ struct tpm_chip { int dev_num; /* /dev/tpm# */ unsigned long is_open; /* only one allowed */ + bool is_suspended; + char hwrng_name[64]; struct hwrng hwrng;
Concurrent accesses to the tpm chip are prevented by allowing only a single thread at a time to obtain a tpm chip reference through tpm_try_get_ops(). However, the tpm's suspend function does not use this mechanism, so when the tpm api is called by a kthread which does not get frozen on suspend (such as the hw_random kthread) it's possible that the tpm is used when already in suspend, or in use while in the process of suspending. This is seen on certain ChromeOS platforms - low-probability warnings are generated during suspend. In this case, the tpm attempted to read data from a tpm chip on an already-suspended bus. i2c_designware i2c_designware.1: Transfer while suspended Fix: 1. prevent concurrent execution of tpm accesses and suspend/ resume, by letting suspend/resume grab the tpm_mutex. 2. before commencing a tpm access, check if the tpm chip is already suspended. Fail with -EAGAIN if so. Tested by running 6000 suspend/resume cycles back-to-back on a ChromeOS "brya" device. The intermittent warnings reliably disappear after applying this patch. No system issues were observed. Cc: <stable@vger.kernel.org> Fixes: e891db1a18bf ("tpm: turn on TPM on suspend for TPM 1.x") Signed-off-by: Sven van Ashbrook <svenva@chromium.org> --- drivers/char/tpm/tpm-interface.c | 16 ++++++++++++++++ include/linux/tpm.h | 2 ++ 2 files changed, 18 insertions(+)