On Fri, Nov 01, 2024 at 02:21:56AM +0200, Jarkko Sakkinen wrote:
> Setting TPM_CHIP_FLAG_SUSPENDED in the end of tpm_pm_suspend() can be racy
> according, as this leaves window for tpm_hwrng_read() to be called while
> the operation is in progress. The recent bug report gives also evidence of
> this behaviour.
>
> Aadress this by locking the TPM chip before checking any chip->flags both
> in tpm_pm_suspend() and tpm_hwrng_read(). Move TPM_CHIP_FLAG_SUSPENDED
> check inside tpm_get_random() so that it will be always checked only when
> the lock is reserved.
>
> Cc: [email protected] # v6.4+
> Fixes: 99d464506255 ("tpm: Prevent hwrng from activating during resume")
> Reported-by: Mike Seo <[email protected]>
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219383
> Signed-off-by: Jarkko Sakkinen <[email protected]>
> ---
> v3:
> - Check TPM_CHIP_FLAG_SUSPENDED inside tpm_get_random() so that it is
> also done under the lock (suggested by Jerry Snitselaar).
> v2:
> - Addressed my own remark:
>
> https://lore.kernel.org/linux-integrity/[email protected]/
> ---
> drivers/char/tpm/tpm-chip.c | 4 ----
> drivers/char/tpm/tpm-interface.c | 32 ++++++++++++++++++++++----------
> 2 files changed, 22 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index 1ff99a7091bb..7df7abaf3e52 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -525,10 +525,6 @@ static int tpm_hwrng_read(struct hwrng *rng, void *data,
> size_t max, bool wait)
> {
> struct tpm_chip *chip = container_of(rng, struct tpm_chip, hwrng);
>
> - /* Give back zero bytes, as TPM chip has not yet fully resumed: */
> - if (chip->flags & TPM_CHIP_FLAG_SUSPENDED)
> - return 0;
> -
> return tpm_get_random(chip, data, max);
> }
>
> diff --git a/drivers/char/tpm/tpm-interface.c
> b/drivers/char/tpm/tpm-interface.c
> index 8134f002b121..b1daa0d7b341 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -370,6 +370,13 @@ int tpm_pm_suspend(struct device *dev)
> if (!chip)
> return -ENODEV;
>
> + rc = tpm_try_get_ops(chip);
> + if (rc) {
> + /* Can be safely set out of locks, as no action cannot race: */
> + chip->flags |= TPM_CHIP_FLAG_SUSPENDED;
> + goto out;
> + }
> +
> if (chip->flags & TPM_CHIP_FLAG_ALWAYS_POWERED)
> goto suspended;
>
> @@ -377,21 +384,19 @@ int tpm_pm_suspend(struct device *dev)
> !pm_suspend_via_firmware())
> goto suspended;
>
> - rc = tpm_try_get_ops(chip);
> - if (!rc) {
> - if (chip->flags & TPM_CHIP_FLAG_TPM2) {
> - tpm2_end_auth_session(chip);
> - tpm2_shutdown(chip, TPM2_SU_STATE);
> - } else {
> - rc = tpm1_pm_suspend(chip, tpm_suspend_pcr);
> - }
> -
> - tpm_put_ops(chip);
> + if (chip->flags & TPM_CHIP_FLAG_TPM2) {
> + tpm2_end_auth_session(chip);
> + tpm2_shutdown(chip, TPM2_SU_STATE);
> + goto suspended;
> }
>
> + rc = tpm1_pm_suspend(chip, tpm_suspend_pcr);
> +
I imagine the above still be wrapped in an else with the if (chip->flags &
TPM_CHIP_FLAG_TPM2)
otherwise it will call tpm1_pm_suspend for both tpm1 and tpm2 devices, yes?
So:
if (chip->flags & TPM_CHIP_FLAG_TPM2) {
tpm2_end_auth_session(chip);
tpm2_shutdown(chip, TPM2_SU_STATE);
goto suspended;
} else {
rc = tpm1_pm_suspend(chip, tpm_suspend_pcr);
}
Other than that I think it looks good.
> suspended:
> chip->flags |= TPM_CHIP_FLAG_SUSPENDED;
> + tpm_put_ops(chip);
>
> +out:
> if (rc)
> dev_err(dev, "Ignoring error %d while suspending\n", rc);
> return 0;
> @@ -440,11 +445,18 @@ int tpm_get_random(struct tpm_chip *chip, u8 *out,
> size_t max)
> if (!chip)
> return -ENODEV;
>
> + /* Give back zero bytes, as TPM chip has not yet fully resumed: */
> + if (chip->flags & TPM_CHIP_FLAG_SUSPENDED) {
> + rc = 0;
> + goto out;
> + }
> +
> if (chip->flags & TPM_CHIP_FLAG_TPM2)
> rc = tpm2_get_random(chip, out, max);
> else
> rc = tpm1_get_random(chip, out, max);
>
> +out:
> tpm_put_ops(chip);
> return rc;
> }
> --
> 2.47.0
>
