| Message ID | 20241029223647.35209-1-jarkko@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | tpm: set TPM_CHIP_FLAG_SUSPENDED early | expand |
On Wed, Oct 30, 2024 at 12:36:47AM +0200, Jarkko Sakkinen wrote: > Setting TPM_CHIP_FLAG_SUSPENDED in the end of tpm_pm_suspend() can be racy > according to the bug report, as this leaves window for tpm_hwrng_read() to > be called while the operation is in progress. Move setting of the flag > into the beginning. > > Cc: stable@vger.kernel.org # v6.4+ > Fixes: 99d464506255 ("tpm: Prevent hwrng from activating during resume") > Reported-by: Mike Seo <mikeseohyungjin@gmail.com> > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219383 > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org> > --- > drivers/char/tpm/tpm-interface.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c > index 8134f002b121..3f96bc8b95df 100644 > --- a/drivers/char/tpm/tpm-interface.c > +++ b/drivers/char/tpm/tpm-interface.c > @@ -370,6 +370,8 @@ int tpm_pm_suspend(struct device *dev) > if (!chip) > return -ENODEV; > > + chip->flags |= TPM_CHIP_FLAG_SUSPENDED; > + > if (chip->flags & TPM_CHIP_FLAG_ALWAYS_POWERED) > goto suspended; > > @@ -390,8 +392,6 @@ int tpm_pm_suspend(struct device *dev) > } > > suspended: > - chip->flags |= TPM_CHIP_FLAG_SUSPENDED; > - > if (rc) > dev_err(dev, "Ignoring error %d while suspending\n", rc); > return 0; > -- > 2.47.0 > Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>
On Wed Oct 30, 2024 at 10:09 PM EET, Jerry Snitselaar wrote: > On Wed, Oct 30, 2024 at 12:36:47AM +0200, Jarkko Sakkinen wrote: > > Setting TPM_CHIP_FLAG_SUSPENDED in the end of tpm_pm_suspend() can be racy > > according to the bug report, as this leaves window for tpm_hwrng_read() to > > be called while the operation is in progress. Move setting of the flag > > into the beginning. > > > > Cc: stable@vger.kernel.org # v6.4+ > > Fixes: 99d464506255 ("tpm: Prevent hwrng from activating during resume") > > Reported-by: Mike Seo <mikeseohyungjin@gmail.com> > > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219383 > > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org> > > --- > > drivers/char/tpm/tpm-interface.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c > > index 8134f002b121..3f96bc8b95df 100644 > > --- a/drivers/char/tpm/tpm-interface.c > > +++ b/drivers/char/tpm/tpm-interface.c > > @@ -370,6 +370,8 @@ int tpm_pm_suspend(struct device *dev) > > if (!chip) > > return -ENODEV; > > > > + chip->flags |= TPM_CHIP_FLAG_SUSPENDED; > > + > > if (chip->flags & TPM_CHIP_FLAG_ALWAYS_POWERED) > > goto suspended; > > > > @@ -390,8 +392,6 @@ int tpm_pm_suspend(struct device *dev) > > } > > > > suspended: > > - chip->flags |= TPM_CHIP_FLAG_SUSPENDED; > > - > > if (rc) > > dev_err(dev, "Ignoring error %d while suspending\n", rc); > > return 0; > > -- > > 2.47.0 > > > > Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com> Thanks but I actually started to look at the function: https://elixir.bootlin.com/linux/v6.11.5/source/drivers/char/tpm/tpm-interface.c#L365 The absolutely safe-play way considering concurrency would be to do tpm_try_get_ops() before checking any flags. That way tpm_hwrng_read() is guaranteed not conflict. So the way I would fix this instead would be to (untested wrote inline here): int tpm_pm_suspend(struct device *dev) { struct tpm_chip *chip = dev_get_drvdata(dev); int rc = 0; if (!chip) return -ENODEV; rc = tpm_try_get_ops(chip); if (rc) { chip->flags = |= TPM_CHIP_FLAG_SUSPENDED; return rc; } /* ... */ suspended: chip->flags |= TPM_CHIP_FLAG_SUSPENDED; tpm_put_ops(chip); It does not really affect performance but guarantees that tpm_hwrng_read() is guaranteed either fully finish or never happens given that both sides take chip->lock. So I'll put one more round of this and then this should be stable and fully fixed. BR, Jarkko
On Thu, Oct 31, 2024 at 01:36:46AM +0200, Jarkko Sakkinen wrote: > On Wed Oct 30, 2024 at 10:09 PM EET, Jerry Snitselaar wrote: > > On Wed, Oct 30, 2024 at 12:36:47AM +0200, Jarkko Sakkinen wrote: > > > Setting TPM_CHIP_FLAG_SUSPENDED in the end of tpm_pm_suspend() can be racy > > > according to the bug report, as this leaves window for tpm_hwrng_read() to > > > be called while the operation is in progress. Move setting of the flag > > > into the beginning. > > > > > > Cc: stable@vger.kernel.org # v6.4+ > > > Fixes: 99d464506255 ("tpm: Prevent hwrng from activating during resume") > > > Reported-by: Mike Seo <mikeseohyungjin@gmail.com> > > > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219383 > > > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org> > > > --- > > > drivers/char/tpm/tpm-interface.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c > > > index 8134f002b121..3f96bc8b95df 100644 > > > --- a/drivers/char/tpm/tpm-interface.c > > > +++ b/drivers/char/tpm/tpm-interface.c > > > @@ -370,6 +370,8 @@ int tpm_pm_suspend(struct device *dev) > > > if (!chip) > > > return -ENODEV; > > > > > > + chip->flags |= TPM_CHIP_FLAG_SUSPENDED; > > > + > > > if (chip->flags & TPM_CHIP_FLAG_ALWAYS_POWERED) > > > goto suspended; > > > > > > @@ -390,8 +392,6 @@ int tpm_pm_suspend(struct device *dev) > > > } > > > > > > suspended: > > > - chip->flags |= TPM_CHIP_FLAG_SUSPENDED; > > > - > > > if (rc) > > > dev_err(dev, "Ignoring error %d while suspending\n", rc); > > > return 0; > > > -- > > > 2.47.0 > > > > > > > Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com> > > Thanks but I actually started to look at the function: > > https://elixir.bootlin.com/linux/v6.11.5/source/drivers/char/tpm/tpm-interface.c#L365 > > The absolutely safe-play way considering concurrency would be > to do tpm_try_get_ops() before checking any flags. That way > tpm_hwrng_read() is guaranteed not conflict. > > So the way I would fix this instead would be to (untested > wrote inline here): > > int tpm_pm_suspend(struct device *dev) > { > struct tpm_chip *chip = dev_get_drvdata(dev); > int rc = 0; > > if (!chip) > return -ENODEV; > > rc = tpm_try_get_ops(chip); > if (rc) { > chip->flags = |= TPM_CHIP_FLAG_SUSPENDED; > return rc; > } > > /* ... */ > > suspended: > chip->flags |= TPM_CHIP_FLAG_SUSPENDED; > tpm_put_ops(chip); > > It does not really affect performance but guarantees that > tpm_hwrng_read() is guaranteed either fully finish or > never happens given that both sides take chip->lock. > > So I'll put one more round of this and then this should be > stable and fully fixed. > > BR, Jarkko Ah, yeah better to set it while it has the mutex. That should still be 'if (!rc)' after the tpm_try_get_ops() right? (I'm assuming that is just a transcription error). Regards, Jerry
On Thu, Oct 31, 2024 at 08:02:37AM -0700, Jerry Snitselaar wrote: > On Thu, Oct 31, 2024 at 01:36:46AM +0200, Jarkko Sakkinen wrote: > > On Wed Oct 30, 2024 at 10:09 PM EET, Jerry Snitselaar wrote: > > > On Wed, Oct 30, 2024 at 12:36:47AM +0200, Jarkko Sakkinen wrote: > > > > Setting TPM_CHIP_FLAG_SUSPENDED in the end of tpm_pm_suspend() can be racy > > > > according to the bug report, as this leaves window for tpm_hwrng_read() to > > > > be called while the operation is in progress. Move setting of the flag > > > > into the beginning. > > > > > > > > Cc: stable@vger.kernel.org # v6.4+ > > > > Fixes: 99d464506255 ("tpm: Prevent hwrng from activating during resume") > > > > Reported-by: Mike Seo <mikeseohyungjin@gmail.com> > > > > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219383 > > > > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org> > > > > --- > > > > drivers/char/tpm/tpm-interface.c | 4 ++-- > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c > > > > index 8134f002b121..3f96bc8b95df 100644 > > > > --- a/drivers/char/tpm/tpm-interface.c > > > > +++ b/drivers/char/tpm/tpm-interface.c > > > > @@ -370,6 +370,8 @@ int tpm_pm_suspend(struct device *dev) > > > > if (!chip) > > > > return -ENODEV; > > > > > > > > + chip->flags |= TPM_CHIP_FLAG_SUSPENDED; > > > > + > > > > if (chip->flags & TPM_CHIP_FLAG_ALWAYS_POWERED) > > > > goto suspended; > > > > > > > > @@ -390,8 +392,6 @@ int tpm_pm_suspend(struct device *dev) > > > > } > > > > > > > > suspended: > > > > - chip->flags |= TPM_CHIP_FLAG_SUSPENDED; > > > > - > > > > if (rc) > > > > dev_err(dev, "Ignoring error %d while suspending\n", rc); > > > > return 0; > > > > -- > > > > 2.47.0 > > > > > > > > > > Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com> > > > > Thanks but I actually started to look at the function: > > > > https://elixir.bootlin.com/linux/v6.11.5/source/drivers/char/tpm/tpm-interface.c#L365 > > > > The absolutely safe-play way considering concurrency would be > > to do tpm_try_get_ops() before checking any flags. That way > > tpm_hwrng_read() is guaranteed not conflict. > > > > So the way I would fix this instead would be to (untested > > wrote inline here): > > > > int tpm_pm_suspend(struct device *dev) > > { > > struct tpm_chip *chip = dev_get_drvdata(dev); > > int rc = 0; > > > > if (!chip) > > return -ENODEV; > > > > rc = tpm_try_get_ops(chip); > > if (rc) { > > chip->flags = |= TPM_CHIP_FLAG_SUSPENDED; > > return rc; > > } > > > > /* ... */ > > > > suspended: > > chip->flags |= TPM_CHIP_FLAG_SUSPENDED; > > tpm_put_ops(chip); > > > > It does not really affect performance but guarantees that > > tpm_hwrng_read() is guaranteed either fully finish or > > never happens given that both sides take chip->lock. > > > > So I'll put one more round of this and then this should be > > stable and fully fixed. > > > > BR, Jarkko > > Ah, yeah better to set it while it has the mutex. That should still be > 'if (!rc)' after the tpm_try_get_ops() right? (I'm assuming that is just > a transcription error). > > Regards, > Jerry > It has been a while since I've looked at TPM code. Since tpm_hwrng_read doesn't check the flag with the mutex held is there a point later where it will bail out if the suspend has occurred? I'm wondering if the check for the suspend flag in tpm_hwrng_read should be after the tpm_find_get_ops in tpm_get_random. Regards, Jerry
On Thu Oct 31, 2024 at 5:28 PM EET, Jerry Snitselaar wrote: > On Thu, Oct 31, 2024 at 08:02:37AM -0700, Jerry Snitselaar wrote: > > On Thu, Oct 31, 2024 at 01:36:46AM +0200, Jarkko Sakkinen wrote: > > > On Wed Oct 30, 2024 at 10:09 PM EET, Jerry Snitselaar wrote: > > > > On Wed, Oct 30, 2024 at 12:36:47AM +0200, Jarkko Sakkinen wrote: > > > > > Setting TPM_CHIP_FLAG_SUSPENDED in the end of tpm_pm_suspend() can be racy > > > > > according to the bug report, as this leaves window for tpm_hwrng_read() to > > > > > be called while the operation is in progress. Move setting of the flag > > > > > into the beginning. > > > > > > > > > > Cc: stable@vger.kernel.org # v6.4+ > > > > > Fixes: 99d464506255 ("tpm: Prevent hwrng from activating during resume") > > > > > Reported-by: Mike Seo <mikeseohyungjin@gmail.com> > > > > > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219383 > > > > > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org> > > > > > --- > > > > > drivers/char/tpm/tpm-interface.c | 4 ++-- > > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c > > > > > index 8134f002b121..3f96bc8b95df 100644 > > > > > --- a/drivers/char/tpm/tpm-interface.c > > > > > +++ b/drivers/char/tpm/tpm-interface.c > > > > > @@ -370,6 +370,8 @@ int tpm_pm_suspend(struct device *dev) > > > > > if (!chip) > > > > > return -ENODEV; > > > > > > > > > > + chip->flags |= TPM_CHIP_FLAG_SUSPENDED; > > > > > + > > > > > if (chip->flags & TPM_CHIP_FLAG_ALWAYS_POWERED) > > > > > goto suspended; > > > > > > > > > > @@ -390,8 +392,6 @@ int tpm_pm_suspend(struct device *dev) > > > > > } > > > > > > > > > > suspended: > > > > > - chip->flags |= TPM_CHIP_FLAG_SUSPENDED; > > > > > - > > > > > if (rc) > > > > > dev_err(dev, "Ignoring error %d while suspending\n", rc); > > > > > return 0; > > > > > -- > > > > > 2.47.0 > > > > > > > > > > > > > Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com> > > > > > > Thanks but I actually started to look at the function: > > > > > > https://elixir.bootlin.com/linux/v6.11.5/source/drivers/char/tpm/tpm-interface.c#L365 > > > > > > The absolutely safe-play way considering concurrency would be > > > to do tpm_try_get_ops() before checking any flags. That way > > > tpm_hwrng_read() is guaranteed not conflict. > > > > > > So the way I would fix this instead would be to (untested > > > wrote inline here): > > > > > > int tpm_pm_suspend(struct device *dev) > > > { > > > struct tpm_chip *chip = dev_get_drvdata(dev); > > > int rc = 0; > > > > > > if (!chip) > > > return -ENODEV; > > > > > > rc = tpm_try_get_ops(chip); > > > if (rc) { > > > chip->flags = |= TPM_CHIP_FLAG_SUSPENDED; > > > return rc; > > > } > > > > > > /* ... */ > > > > > > suspended: > > > chip->flags |= TPM_CHIP_FLAG_SUSPENDED; > > > tpm_put_ops(chip); > > > > > > It does not really affect performance but guarantees that > > > tpm_hwrng_read() is guaranteed either fully finish or > > > never happens given that both sides take chip->lock. > > > > > > So I'll put one more round of this and then this should be > > > stable and fully fixed. > > > > > > BR, Jarkko > > > > Ah, yeah better to set it while it has the mutex. That should still be > > 'if (!rc)' after the tpm_try_get_ops() right? (I'm assuming that is just > > a transcription error). > > > > Regards, > > Jerry > > > > It has been a while since I've looked at TPM code. Since > tpm_hwrng_read doesn't check the flag with the mutex held is there a > point later where it will bail out if the suspend has occurred? I'm > wondering if the check for the suspend flag in tpm_hwrng_read should > be after the tpm_find_get_ops in tpm_get_random. Right, I ignored that side in v2. Yeah, I agree that in both cases it would be best that all checks are done when the lock is taken. It means open-coding tpm2_get_random() but I think it is anyway good idea (as tpm_get_random() is meant for outside callers). > Regards, > Jerry BR, Jarkko
On Thu Oct 31, 2024 at 5:02 PM EET, Jerry Snitselaar wrote: > On Thu, Oct 31, 2024 at 01:36:46AM +0200, Jarkko Sakkinen wrote: > > On Wed Oct 30, 2024 at 10:09 PM EET, Jerry Snitselaar wrote: > > > On Wed, Oct 30, 2024 at 12:36:47AM +0200, Jarkko Sakkinen wrote: > > > > Setting TPM_CHIP_FLAG_SUSPENDED in the end of tpm_pm_suspend() can be racy > > > > according to the bug report, as this leaves window for tpm_hwrng_read() to > > > > be called while the operation is in progress. Move setting of the flag > > > > into the beginning. > > > > > > > > Cc: stable@vger.kernel.org # v6.4+ > > > > Fixes: 99d464506255 ("tpm: Prevent hwrng from activating during resume") > > > > Reported-by: Mike Seo <mikeseohyungjin@gmail.com> > > > > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219383 > > > > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org> > > > > --- > > > > drivers/char/tpm/tpm-interface.c | 4 ++-- > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c > > > > index 8134f002b121..3f96bc8b95df 100644 > > > > --- a/drivers/char/tpm/tpm-interface.c > > > > +++ b/drivers/char/tpm/tpm-interface.c > > > > @@ -370,6 +370,8 @@ int tpm_pm_suspend(struct device *dev) > > > > if (!chip) > > > > return -ENODEV; > > > > > > > > + chip->flags |= TPM_CHIP_FLAG_SUSPENDED; > > > > + > > > > if (chip->flags & TPM_CHIP_FLAG_ALWAYS_POWERED) > > > > goto suspended; > > > > > > > > @@ -390,8 +392,6 @@ int tpm_pm_suspend(struct device *dev) > > > > } > > > > > > > > suspended: > > > > - chip->flags |= TPM_CHIP_FLAG_SUSPENDED; > > > > - > > > > if (rc) > > > > dev_err(dev, "Ignoring error %d while suspending\n", rc); > > > > return 0; > > > > -- > > > > 2.47.0 > > > > > > > > > > Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com> > > > > Thanks but I actually started to look at the function: > > > > https://elixir.bootlin.com/linux/v6.11.5/source/drivers/char/tpm/tpm-interface.c#L365 > > > > The absolutely safe-play way considering concurrency would be > > to do tpm_try_get_ops() before checking any flags. That way > > tpm_hwrng_read() is guaranteed not conflict. > > > > So the way I would fix this instead would be to (untested > > wrote inline here): > > > > int tpm_pm_suspend(struct device *dev) > > { > > struct tpm_chip *chip = dev_get_drvdata(dev); > > int rc = 0; > > > > if (!chip) > > return -ENODEV; > > > > rc = tpm_try_get_ops(chip); > > if (rc) { > > chip->flags = |= TPM_CHIP_FLAG_SUSPENDED; > > return rc; > > } > > > > /* ... */ > > > > suspended: > > chip->flags |= TPM_CHIP_FLAG_SUSPENDED; > > tpm_put_ops(chip); > > > > It does not really affect performance but guarantees that > > tpm_hwrng_read() is guaranteed either fully finish or > > never happens given that both sides take chip->lock. > > > > So I'll put one more round of this and then this should be > > stable and fully fixed. > > > > BR, Jarkko > > Ah, yeah better to set it while it has the mutex. That should still be > 'if (!rc)' after the tpm_try_get_ops() right? (I'm assuming that is just > a transcription error). Can you check v2 of the patch? It misses the tpm_hwrng_read() change that you suggested. I think rc is checked there correctly but it is always possible that I overlook/ignore something... So no tags for that since an update is still coming but just the parts that are already in it make sense. > > Regards, > Jerry BR, Jarkko
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c index 8134f002b121..3f96bc8b95df 100644 --- a/drivers/char/tpm/tpm-interface.c +++ b/drivers/char/tpm/tpm-interface.c @@ -370,6 +370,8 @@ int tpm_pm_suspend(struct device *dev) if (!chip) return -ENODEV; + chip->flags |= TPM_CHIP_FLAG_SUSPENDED; + if (chip->flags & TPM_CHIP_FLAG_ALWAYS_POWERED) goto suspended; @@ -390,8 +392,6 @@ int tpm_pm_suspend(struct device *dev) } suspended: - chip->flags |= TPM_CHIP_FLAG_SUSPENDED; - if (rc) dev_err(dev, "Ignoring error %d while suspending\n", rc); return 0;
Setting TPM_CHIP_FLAG_SUSPENDED in the end of tpm_pm_suspend() can be racy according to the bug report, as this leaves window for tpm_hwrng_read() to be called while the operation is in progress. Move setting of the flag into the beginning. Cc: stable@vger.kernel.org # v6.4+ Fixes: 99d464506255 ("tpm: Prevent hwrng from activating during resume") Reported-by: Mike Seo <mikeseohyungjin@gmail.com> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219383 Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org> --- drivers/char/tpm/tpm-interface.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)