| Message ID | 20241101002157.645874-1-jarkko@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] tpm: Lock TPM chip in tpm_pm_suspend() first | expand |
On Fri Nov 1, 2024 at 2:21 AM EET, 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: 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> A basic smoke test in QEMU: # rtcwake -m mem -s 15 rtcwake -m mem -s 15 rtcwake: assuming RTC uses UTC ... rtcwake: wakeup from "mem" using /dev/rtc0 at Fri Nov 1 02:21:06 2024 PM: suspend entry (deep) Filesystems sync: 0.017 seconds Freezing user space processes Freezing user space processes completed (elapsed 0.004 seconds) OOM killer disabled. Freezing remaining freezable tasks Freezing remaining freezable tasks completed (elapsed 0.004 seconds) printk: Suspending console(s) (use no_console_suspend to debug) ata2.00: Check power mode failed (err_mask=0x1) ACPI: PM: Preparing to enter system sleep state S3 ACPI: PM: Saving platform NVS memory Disabling non-boot CPUs ... ACPI: PM: Low-level resume complete ACPI: PM: Restoring platform NVS memory ACPI: PM: Waking up from system sleep state S3 pci 0000:00:01.0: PIIX3: Enabling Passive Release virtio_blk virtio1: 1/0/0 default/read/poll queues OOM killer enabled. Restarting tasks ... done. random: crng reseeded on system resumption PM: suspend exit # ata2: found unknown device (class 0) # dmesg|tail -20 dmesg|tail -20 [ 28.199150] Freezing user space processes [ 28.205393] Freezing user space processes completed (elapsed 0.004 seconds) [ 28.206780] OOM killer disabled. [ 28.207858] Freezing remaining freezable tasks [ 28.213224] Freezing remaining freezable tasks completed (elapsed 0.004 seconds) [ 28.214591] printk: Suspending console(s) (use no_console_suspend to debug) [ 28.222203] ata2.00: Check power mode failed (err_mask=0x1) [ 28.240808] ACPI: PM: Preparing to enter system sleep state S3 [ 28.241218] ACPI: PM: Saving platform NVS memory [ 28.241390] Disabling non-boot CPUs ... [ 28.243011] ACPI: PM: Low-level resume complete [ 28.243273] ACPI: PM: Restoring platform NVS memory [ 28.246191] ACPI: PM: Waking up from system sleep state S3 [ 28.250415] pci 0000:00:01.0: PIIX3: Enabling Passive Release [ 28.256539] virtio_blk virtio1: 1/0/0 default/read/poll queues [ 28.280715] OOM killer enabled. [ 28.281766] Restarting tasks ... done. [ 28.287096] random: crng reseeded on system resumption [ 28.288181] PM: suspend exit [ 28.410073] ata2: found unknown device (class 0) Testing done with https://codeberg.org/jarkko/linux-tpmdd-test cmake -Bbuild -Dbuildroot_defconfig=busybox_x86_64_defconfig && make -Cbuild buildroot-prepare make -Cbuild/buildroot/build pushd build/buildroot/build images/run-qemu.sh & socat - UNIX-CONNECT:images/serial.sock BR, Jarkko
On Fri Nov 1, 2024 at 2:25 AM EET, Jarkko Sakkinen wrote: > cmake -Bbuild -Dbuildroot_defconfig=busybox_x86_64_defconfig && make -Cbuild buildroot-prepare > make -Cbuild/buildroot/build > pushd build/buildroot/build > images/run-qemu.sh & > socat - UNIX-CONNECT:images/serial.sock and export LINUX_OVERRIDE_SRCDIR=/home/jarkko/work/kernel.org/jarkko/linux-tpmdd <offtopic> I wondered what was the thing anyway with those "kernel patches for Github/lab" discussed in LWN while ago. If you know how to propeerly use BuildRoot, CI compatibility is and old thing. This has been fully tested to run also inside CI (and has run-tests.sh based on TCL's utility expect). How I understood that article was lack of knowledge of the tools available. Hope none of those kernel patches never landed tbh... </offtopic> BR, Jarkko
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: 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> > --- > 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/D59JAI6RR2CD.G5E5T4ZCZ49W@kernel.org/ > --- > 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 >
On Fri Nov 1, 2024 at 10:23 PM EET, Jerry Snitselaar wrote: > 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: 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> > > --- > > 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/D59JAI6RR2CD.G5E5T4ZCZ49W@kernel.org/ > > --- > > 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. It should be fine because after tpm2_shutdown() is called there is "goto suspended;". This is IMHO more readable as it matches the structure of previous exits before it. In future if this needs to be improved it will easier to move the logic to a helper function (e.g. __tpm_pm_suspend()) where gotos are substituted with return-statements. BR, Jarkko
On Fri, Nov 01, 2024 at 11:07:15PM +0200, Jarkko Sakkinen wrote: > On Fri Nov 1, 2024 at 10:23 PM EET, Jerry Snitselaar wrote: > > 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: 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> > > > --- > > > 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/D59JAI6RR2CD.G5E5T4ZCZ49W@kernel.org/ > > > --- > > > 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. > > It should be fine because after tpm2_shutdown() is called there is "goto > suspended;". This is IMHO more readable as it matches the structure of > previous exits before it. In future if this needs to be improved it will > easier to move the logic to a helper function (e.g. __tpm_pm_suspend()) > where gotos are substituted with return-statements. > > BR, Jarkko > Heh, yep. Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>
On Fri Nov 1, 2024 at 11:09 PM EET, Jerry Snitselaar wrote: > On Fri, Nov 01, 2024 at 11:07:15PM +0200, Jarkko Sakkinen wrote: > > On Fri Nov 1, 2024 at 10:23 PM EET, Jerry Snitselaar wrote: > > > 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: 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> > > > > --- > > > > 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/D59JAI6RR2CD.G5E5T4ZCZ49W@kernel.org/ > > > > --- > > > > 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. > > > > It should be fine because after tpm2_shutdown() is called there is "goto > > suspended;". This is IMHO more readable as it matches the structure of > > previous exits before it. In future if this needs to be improved it will > > easier to move the logic to a helper function (e.g. __tpm_pm_suspend()) > > where gotos are substituted with return-statements. > > > > BR, Jarkko > > > > Heh, yep. > > Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com> Thanks! BR, Jarkko
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); + 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; }
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: 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> --- 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/D59JAI6RR2CD.G5E5T4ZCZ49W@kernel.org/ --- drivers/char/tpm/tpm-chip.c | 4 ---- drivers/char/tpm/tpm-interface.c | 32 ++++++++++++++++++++++---------- 2 files changed, 22 insertions(+), 14 deletions(-)