Dear Adam,
Thank you very much for analyzing the problem and finding a fix. I have
some small nits and questions. The current state is alright. Should you
send another version, that you could address the nits.
Am 07.03.24 um 23:49 schrieb Adam Alves:
Some buggy firmware 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).
Out of curiosity, would reboot work?
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 some bugs over 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 security features provided by TPM. >
The root cause might be 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.
Enable a user to configure the kernel through
“tpm.locality_on_suspend=1” boot parameter so that the locality is set
I’d use `` from Markdown.
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.
Could you please document the TPM for completeness?
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”.
Is the exception useful, if a user explicitly requests to disable the
behavior? I was thinking for a case, where a newer system firmware has a
fix (and the DMI check would need to be extended).
For completeness, I’d mention/paste the new log message.
Link: https://bugzilla.kernel.org/show_bug.cgi?id=217890
Signed-off-by: Adam Alves <adamoa@xxxxxxxxx>
Should this be backported to the stable series?
---
v1->v2: fix formatting issues and simplified tpm_chip_stop code.
drivers/char/tpm/tpm-chip.c | 12 +++++++++++
drivers/char/tpm/tpm-interface.c | 37 ++++++++++++++++++++++++++++++++
drivers/char/tpm/tpm.h | 1 +
include/linux/tpm.h | 1 +
4 files changed, 51 insertions(+)
diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 42b1062e33cd..a183e1355289 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -137,6 +137,12 @@ EXPORT_SYMBOL_GPL(tpm_chip_start);
*/
void tpm_chip_stop(struct tpm_chip *chip)
{
+ if (chip->flags & TPM_CHIP_FLAG_PRESERVE_LOCALITY) {
+ if (chip->locality != 0)
+ tpm_request_locality(chip);
+ return;
+ }
+
tpm_go_idle(chip);
tpm_relinquish_locality(chip);
tpm_clk_disable(chip);
@@ -291,6 +297,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_FLAG_PRESERVE_LOCALITY;
+
down_write(&chip->ops_sem);
if (chip->flags & TPM_CHIP_FLAG_TPM2) {
if (!tpm_chip_start(chip)) {
@@ -668,6 +677,9 @@ EXPORT_SYMBOL_GPL(tpm_chip_register);
*/
void tpm_chip_unregister(struct tpm_chip *chip)
{
+ if (tpm_locality_on_suspend)
+ chip->flags |= TPM_CHIP_FLAG_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..7f770ea98402 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,36 @@ int tpm_auto_startup(struct tpm_chip *chip)
return rc;
}
+/*
+ * Bug workaround - some boards expect the TPM to be on Locality 0
+ * before suspend/shutdown, halting the system otherwise before
+ * suspend and shutdown. Change suspend behavior for these cases.
+ */
+bool tpm_locality_on_suspend;
+module_param_named(locality_on_suspend, tpm_locality_on_suspend, bool, 0644);
+MODULE_PARM_DESC(locality_on_suspend,
+ "Put TPM at locality 0 before suspend/shutdown.");
I was wondering, if there is a name, that would make clear, that it is
not only during suspend. But I couldn’t come up with one.
+
+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 +425,9 @@ int tpm_pm_suspend(struct device *dev)
if (!chip)
return -ENODEV;
+ if (tpm_locality_on_suspend)
+ chip->flags |= TPM_CHIP_FLAG_PRESERVE_LOCALITY;
+
if (chip->flags & TPM_CHIP_FLAG_ALWAYS_POWERED)
goto suspended;
@@ -431,6 +465,7 @@ int tpm_pm_resume(struct device *dev)
if (chip == NULL)
return -ENODEV;
+ chip->flags &= ~TPM_CHIP_FLAG_PRESERVE_LOCALITY;
chip->flags &= ~TPM_CHIP_FLAG_SUSPENDED;
/*
@@ -476,6 +511,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..1fbb33f386d1 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_FLAG_PRESERVE_LOCALITY = BIT(10),
};
#define to_tpm_chip(d) container_of(d, struct tpm_chip, dev)
Reviewed-by: Paul Menzel <pmenzel@xxxxxxxxxxxxx>
Kind regards,
Paul
