tpm: tpm_crb: Call acpi_put_table() on firmware bug

In crb_acpi_add(), we call acpi_get_table() to retrieve the ACPI table entry. acpi_put_table() is called on the error path to avoid a memory leak, but the current implementation does not call acpi_put_table() when the length field of struct acpi_table_header is not valid, which leads to a memory leak. Although this memory leak only occurrs when the firmware misconfigured the ACPI table, it would still be nice to have this fix.

---
 drivers/char/tpm/tpm_crb.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

index ea085b14ab7c..68fe28208331 100644
@@ -738,10 +738,14 @@ static int crb_acpi_add(struct acpi_device *device)
 
 	status = acpi_get_table(ACPI_SIG_TPM2, 1,
 				(struct acpi_table_header **) &buf);
-	if (ACPI_FAILURE(status) || buf->header.length < sizeof(*buf)) {
+	if (ACPI_FAILURE(status)) {
 		dev_err(dev, FW_BUG "failed to get TPM2 ACPI table\n");
 		return -EINVAL;
 	}
+	if (buf->header.length < sizeof(*buf)) {
+		rc = -EINVAL;
+		goto out;
+	}
 
 	/* Should the FIFO driver handle this? */
 	sm = buf->start_method;

2.34.1

1. Drop the hyphens.
2. Wouldn't it be memory corruption, and not a leak?
3. Why would ACPICA return corrupted data in this case?

BR, Jarkko

Hi Jarkko,

Thank you for your time and feedback on my previous patch.

1. Drop the hyphens.
- I have removed them from the commit message in the v2 patch below.
2. Wouldn't it be memory corruption, and not a leak?
- The validation_count field not returning to 0 causes acpi_tb_release_table() not being called, which means memory is not being unmapped. Therefore, I assume it is a memory leak.
3. Why would ACPICA return corrupted data in this case?
- It is mostly unlikely that it returns corrupted data, but it would happen when the ACPI table is misconfigured by the firmware. Although this event is rare, I thought it would still be nice to take care of the error path.

Please find the updated patch v2 attached to this email.

Best,

Joe Hattori

---
In crb_acpi_add(), we call acpi_get_table() to retrieve the ACPI table entry. acpi_put_table() is called on the error path to avoid a memory leak, but the current implementation does not call acpi_put_table() when the length field of struct acpi_table_header is not valid, which leads to a memory leak. Although this memory leak only occurrs when the firmware misconfigured the ACPI table, it would still be nice to have this fix.

---
 drivers/char/tpm/tpm_crb.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

index ea085b14ab7c..68fe28208331 100644
@@ -738,10 +738,14 @@ static int crb_acpi_add(struct acpi_device *device)
 
 	status = acpi_get_table(ACPI_SIG_TPM2, 1,
 				(struct acpi_table_header **) &buf);
-	if (ACPI_FAILURE(status) || buf->header.length < sizeof(*buf)) {
+	if (ACPI_FAILURE(status)) {
 		dev_err(dev, FW_BUG "failed to get TPM2 ACPI table\n");
 		return -EINVAL;
 	}
+	if (buf->header.length < sizeof(*buf)) {
+		rc = -EINVAL;
+		goto out;
+	}
 
 	/* Should the FIFO driver handle this? */
 	sm = buf->start_method;

2.34.1

Actually: no. And when you send a patch send them as separate entities and version them [1].

And I don't believe you because I don't live in a belief system. There is total zero evidence provided of anything.

And even if this was the case you should then submit the bug fix to ACPICA itself, not here.

Thus, NAK.

Please don't waste everyones time by inventing imaginary problems. It would be way more productive to solve some actual problems.

[1] https://www.kernel.org/doc/html/v6.9/process/submitting-patches.html

BR, Jarkko