Fix TPM chip hanging system before suspend/shutdown

Commit Message

Adam Alves March 6, 2024, 11:12 p.m. UTC

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

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 <[e-mail]>

Comments

Jarkko Sakkinen March 7, 2024, 7:52 p.m. UTC | #1

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 <[e-mail]>
>
> 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

Patch

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)