[v2] tpm: tpm_crb: Call acpi_put_table() on firmware bug

Commit Message

Joe Hattori June 5, 2024, 7:33 a.m. UTC

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.

Signed-off-by: Joe Hattori <[e-mail]>
---
 drivers/char/tpm/tpm_crb.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Comments

Jarkko Sakkinen June 5, 2024, 8:04 a.m. UTC | #1

On Wed Jun 5, 2024 at 10:33 AM EEST, Joe Hattori wrote:
> 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.

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

Patch

diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index ea085b14ab7c..68fe28208331 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -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;