Re: [PATCH] Fix TPM chip hanging system before suspend/shutdown

["Jarkko Sakkinen" <jarkko@kernel.org>]

On Thu Mar 7, 2024 at 1:12 AM EET, Adam Alves wrote:
> Some buggy firmwares might require the TPM device to be in default
> locality (Locality 0) before suspend or shutdown. Failing to do so
> would leave the system in a hanged state before sleep or power off
> (after “reboot: Power down” message). Such is the case for the ASUSTeK
> COMPUTER INC. TUF GAMING B460M-PLUS board, I believe this might be the
> case for several other boards based on the bugs I have found on the
> internet while trying to find out how to fix my specific issue. Most
> forums suggest the user to disable the TPM device on firmware BIOS in
> order to work around this specific issue, which disables several nice
> security features provided by TPM, such as secure boot attestation,
> automatic decryption and hardware random generator.



>
> My PC would hang on almost every shutdown/suspend until I started
> testing this patch and so far in the past week I haven’t experienced
> any problems anymore.
>
> I suspect that the root cause on my specific board is that after the
> ACPI command to put the device to S3 or S5, some firmware
> application/driver will try to use the TPM chip expecting it to be in
> Locality 0 as expected by TCG PC Client Platform Firmware Profile
> Version 1.06 Revision 52 (3.1.1 – Pre-OS Environment) and then when it
> fails to do so it simply halts the whole system.
>
> This issue might be related to the following bug:
> https://bugzilla.kernel.org/show_bug.cgi?id=217890

You can put Link: <url> for this before signed-off-by's.

>
> Enable a user to configure the kernel
> through “tpm.locality_on_suspend=1” boot parameter so that the locality
> is set before suspend/shutdown in order to diagnose whether or not the
> board is one of the buggy ones that require this workaround. Since this
> bug is related to the board/platform instead of the specific TPM chip,
> call dmi_check_system on the tpm_init function so that this setting is
> automatically enabled for boards specified in code (ASUS TUF GAMING
> B460M-PLUS already included) – automatic configuration only works in
> case CONFIG_DMI is set though, since dmi_check_system is a non-op when
> CONFIG_DMI is not set.
>
> In case “tpm.locality_on_suspend=0” (the default) don't change any
> behavior thus preserving current functionality of any other board
> except ASUSTeK COMPUTER INC. TUF GAMING B460M-PLUS and possibly future
> boards as we successfully diagnose other boards with the same issue
> fixed by using “tpm.locality_on_suspend=1”.
>
> Signed-off-by: Adam Alves <adamoa@gmail.com>
>
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index 42b1062e33cd..8fdf7a137a94 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -139,6 +139,9 @@ void tpm_chip_stop(struct tpm_chip *chip)
>  {
>   tpm_go_idle(chip);
>   tpm_relinquish_locality(chip);
> + // If locality is to be preserved, we need to make sure it is Locality 0.
> + if (chip->flags & TPM_CHIP_PRESERVE_LOCALITY)
> + tpm_request_locality(chip);
>   tpm_clk_disable(chip);
>  }
>  EXPORT_SYMBOL_GPL(tpm_chip_stop);
> @@ -291,6 +294,9 @@ int tpm_class_shutdown(struct device *dev)
>  {
>   struct tpm_chip *chip = container_of(dev, struct tpm_chip, dev);
>
> + if (tpm_locality_on_suspend)
> + chip->flags |= TPM_CHIP_PRESERVE_LOCALITY;
> +
>   down_write(&chip->ops_sem);
>   if (chip->flags & TPM_CHIP_FLAG_TPM2) {
>   if (!tpm_chip_start(chip)) {
> @@ -668,6 +674,9 @@ EXPORT_SYMBOL_GPL(tpm_chip_register);
>   */
>  void tpm_chip_unregister(struct tpm_chip *chip)
>  {
> + if (tpm_locality_on_suspend)
> + chip->flags |= TPM_CHIP_PRESERVE_LOCALITY;
> +
>   tpm_del_legacy_sysfs(chip);
>   if (tpm_is_hwrng_enabled(chip))
>   hwrng_unregister(&chip->hwrng);
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index 66b16d26eecc..8aeea2dee0a8 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -26,6 +26,7 @@
>  #include <linux/suspend.h>
>  #include <linux/freezer.h>
>  #include <linux/tpm_eventlog.h>
> +#include <linux/dmi.h>
>
>  #include "tpm.h"
>
> @@ -382,6 +383,34 @@ int tpm_auto_startup(struct tpm_chip *chip)
>   return rc;
>  }
>
> +/*
> + * Bug workaround - some boards expect the TPM to be on ready
> + * state before suspend/shutdown, otherwise suspend and shutdown might
> + * hang the system, so we need to adjust suspend code for handling this.
> + */
> +bool tpm_locality_on_suspend;
> +module_param_named(locality_on_suspend, tpm_locality_on_suspend, bool, 0644);
> +MODULE_PARM_DESC(locality_on_suspend, "The firmware expects TPM to be
> at locality 0 before suspend/shutdown.");
> +
> +static int __init tpm_set_locality_on_suspend(const struct
> dmi_system_id *system_id)
> +{
> + pr_info("Board %s: TPM locality preserved before
> suspend/shutdown.\n", system_id->ident);
> + tpm_locality_on_suspend = true;
> +
> + return 0;
> +}
> +
> +static const struct dmi_system_id tpm_board_quirks[] __initconst = {
> + {
> + .ident = "TUF GAMING B460M-PLUS",
> + .matches = {
> + DMI_MATCH(DMI_BOARD_VENDOR, "ASUSTeK COMPUTER INC."),
> + DMI_MATCH(DMI_BOARD_NAME, "TUF GAMING B460M-PLUS"),
> + },
> + .callback = tpm_set_locality_on_suspend,
> + },
> +};
> +
>  /*
>   * We are about to suspend. Save the TPM state
>   * so that it can be restored.
> @@ -394,6 +423,9 @@ int tpm_pm_suspend(struct device *dev)
>   if (!chip)
>   return -ENODEV;
>
> + if (tpm_locality_on_suspend)
> + chip->flags |= TPM_CHIP_PRESERVE_LOCALITY;
> +
>   if (chip->flags & TPM_CHIP_FLAG_ALWAYS_POWERED)
>   goto suspended;
>
> @@ -431,7 +463,7 @@ int tpm_pm_resume(struct device *dev)
>   if (chip == NULL)
>   return -ENODEV;
>
> - chip->flags &= ~TPM_CHIP_FLAG_SUSPENDED;
> + chip->flags &= ~(TPM_CHIP_PRESERVE_LOCALITY | TPM_CHIP_FLAG_SUSPENDED);
>
>   /*
>   * Guarantee that SUSPENDED is written last, so that hwrng does not
> @@ -476,6 +508,8 @@ static int __init tpm_init(void)
>  {
>   int rc;
>
> + dmi_check_system(tpm_board_quirks);
> +
>   rc = class_register(&tpm_class);
>   if (rc) {
>   pr_err("couldn't create tpm class\n");
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index 61445f1dc46d..f2657b611b81 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -236,6 +236,7 @@ extern dev_t tpm_devt;
>  extern const struct file_operations tpm_fops;
>  extern const struct file_operations tpmrm_fops;
>  extern struct idr dev_nums_idr;
> +extern bool tpm_locality_on_suspend;
>
>  ssize_t tpm_transmit(struct tpm_chip *chip, u8 *buf, size_t bufsiz);
>  int tpm_get_timeouts(struct tpm_chip *);
> diff --git a/include/linux/tpm.h b/include/linux/tpm.h
> index 4ee9d13749ad..7717f484ac25 100644
> --- a/include/linux/tpm.h
> +++ b/include/linux/tpm.h
> @@ -284,6 +284,7 @@ enum tpm_chip_flags {
>   TPM_CHIP_FLAG_FIRMWARE_UPGRADE = BIT(7),
>   TPM_CHIP_FLAG_SUSPENDED = BIT(8),
>   TPM_CHIP_FLAG_HWRNG_DISABLED = BIT(9),
> + TPM_CHIP_PRESERVE_LOCALITY = BIT(10),
>  };
>
>  #define to_tpm_chip(d) container_of(d, struct tpm_chip, dev)

Did you use git format-patch to generate this? It does not look to be in
the mbox format. E.g. diffstat is missing completely.

I neither think that indentation is according to the guidelines at all.

So what you need to do next is to read [1] through and send v2 if with
those learnings [2]. When you use checkpatch.pl, invoke it as:

scripts/checkpatch.pl --strict <patch file>

[1] https://www.kernel.org/doc/html/latest/process/submitting-patches.html
[2] git format-patch -1 -v2

BR, Jarkko