[RFC] [PATCH] tpm: Clean up TPM space after command failure

[RFC] [PATCH] tpm: Clean up TPM space after command failure

[Jonathan McDowell]

We've been seeing a problem where TPM commands time out, which if
they're the last command before the TPM device is closed causes a leak
of transient handles. They can be seen and cleaned up (with a flush
context) if the /dev/tpm0 device is used instead of /dev/tpmrm0, but it
seems like we should be doing this automatically on the transmit error
path. Patch below adds a tpm2_flush_space on error to avoid this.

Does this seem reasonable? The other query is whether tpm2_del_space
should cleanup the contexts as well, rather than just the sessions.

(Obviously in an ideal world we wouldn't see the timeouts at all, and
I'm still trying to work on getting to the bottom of these, which are
generally infrequent, but happening enough across our fleet that we were
able to observe this handle leak.)

From: Jonathan McDowell <noodles@meta.com>

tpm_dev_transmit prepares the TPM space before attempting command
transmission. However if the command fails no rollback of this
preparation is done. This can result in transient handles being leaked
if the device is subsequently closed with no further commands performed.

Fix this by flushing the space in the event of command transmission
failure.

Signed-off-by: Jonathan McDowell <noodles@meta.com>

---
 drivers/char/tpm/tpm-dev-common.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/char/tpm/tpm-dev-common.c b/drivers/char/tpm/tpm-dev-common.c
index 30b4c288c1bb..c3fbbf4d3db7 100644
--- a/drivers/char/tpm/tpm-dev-common.c
+++ b/drivers/char/tpm/tpm-dev-common.c
@@ -47,6 +47,8 @@ static ssize_t tpm_dev_transmit(struct tpm_chip *chip, struct tpm_space *space,
 
 	if (!ret)
 		ret = tpm2_commit_space(chip, space, buf, &len);
+	else
+		tpm2_flush_space(chip);
 
 out_rc:
 	return ret ? ret : len;
-- 
2.46.0

Re: [RFC] [PATCH] tpm: Clean up TPM space after command failure

[Jarkko Sakkinen]

On Wed Aug 14, 2024 at 7:19 PM EEST, Jonathan McDowell wrote:
> We've been seeing a problem where TPM commands time out, which if
> they're the last command before the TPM device is closed causes a leak
> of transient handles. They can be seen and cleaned up (with a flush
> context) if the /dev/tpm0 device is used instead of /dev/tpmrm0, but it
> seems like we should be doing this automatically on the transmit error
> path. Patch below adds a tpm2_flush_space on error to avoid this.
>
> Does this seem reasonable? The other query is whether tpm2_del_space
> should cleanup the contexts as well, rather than just the sessions.
>
> (Obviously in an ideal world we wouldn't see the timeouts at all, and
> I'm still trying to work on getting to the bottom of these, which are
> generally infrequent, but happening enough across our fleet that we were
> able to observe this handle leak.)

Seems reasonable without this story ;-) I get that this is here because
of query np.

>
> From: Jonathan McDowell <noodles@meta.com>
>
> tpm_dev_transmit prepares the TPM space before attempting command
> transmission. However if the command fails no rollback of this
> preparation is done. This can result in transient handles being leaked
> if the device is subsequently closed with no further commands performed.
>
> Fix this by flushing the space in the event of command transmission
> failure.
>
> Signed-off-by: Jonathan McDowell <noodles@meta.com>

I would consider fixes tag for this even! I think it can be
classified as a minor bug. I implemented this feature together
with James Bottomley so would be nice to get some feedback
also from him (as a sanity check)>

>
> ---

Just as a tip: if you put stuff here like supplemental commets the won't
get included when the commit is finally applied (also a popular place
for change log).

>  drivers/char/tpm/tpm-dev-common.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/char/tpm/tpm-dev-common.c b/drivers/char/tpm/tpm-dev-common.c
> index 30b4c288c1bb..c3fbbf4d3db7 100644
> --- a/drivers/char/tpm/tpm-dev-common.c
> +++ b/drivers/char/tpm/tpm-dev-common.c
> @@ -47,6 +47,8 @@ static ssize_t tpm_dev_transmit(struct tpm_chip *chip, struct tpm_space *space,
>  
>  	if (!ret)
>  		ret = tpm2_commit_space(chip, space, buf, &len);
> +	else
> +		tpm2_flush_space(chip);
>  
>  out_rc:
>  	return ret ? ret : len;

Can you send v2 with fixes tag and James as cc.

Since you have such a long cover letter you could possibly:

git format-patch -1 -v2 --cover-letter

Then just move that text in front to 00/01.

BR, Jarkko

[PATCH v2] tpm: Clean up TPM space after command failure

[Jonathan McDowell]

From: Jonathan McDowell <noodles@meta.com>

tpm_dev_transmit prepares the TPM space before attempting command
transmission. However if the command fails no rollback of this
preparation is done. This can result in transient handles being leaked
if the device is subsequently closed with no further commands performed.

Fix this by flushing the space in the event of command transmission
failure.

Fixes: 745b361e989a ("tpm: infrastructure for TPM spaces")
Signed-off-by: Jonathan McDowell <noodles@meta.com>
---
v2:
 - Add 'Fixes:'
 - Cc James as one of the original authors
 - Add space sanity check in tpm2_flush_space

 drivers/char/tpm/tpm-dev-common.c | 2 ++
 drivers/char/tpm/tpm2-space.c     | 3 +++
 2 files changed, 5 insertions(+)

diff --git a/drivers/char/tpm/tpm-dev-common.c b/drivers/char/tpm/tpm-dev-common.c
index 30b4c288c1bb..c3fbbf4d3db7 100644
--- a/drivers/char/tpm/tpm-dev-common.c
+++ b/drivers/char/tpm/tpm-dev-common.c
@@ -47,6 +47,8 @@ static ssize_t tpm_dev_transmit(struct tpm_chip *chip, struct tpm_space *space,
 
 	if (!ret)
 		ret = tpm2_commit_space(chip, space, buf, &len);
+	else
+		tpm2_flush_space(chip);
 
 out_rc:
 	return ret ? ret : len;
diff --git a/drivers/char/tpm/tpm2-space.c b/drivers/char/tpm/tpm2-space.c
index 4892d491da8d..25a66870c165 100644
--- a/drivers/char/tpm/tpm2-space.c
+++ b/drivers/char/tpm/tpm2-space.c
@@ -169,6 +169,9 @@ void tpm2_flush_space(struct tpm_chip *chip)
 	struct tpm_space *space = &chip->work_space;
 	int i;
 
+	if (!space)
+		return;
+
 	for (i = 0; i < ARRAY_SIZE(space->context_tbl); i++)
 		if (space->context_tbl[i] && ~space->context_tbl[i])
 			tpm2_flush_context(chip, space->context_tbl[i]);
-- 
2.46.0

Re: [PATCH v2] tpm: Clean up TPM space after command failure

[Jarkko Sakkinen]

On Fri Aug 16, 2024 at 2:55 PM EEST, Jonathan McDowell wrote:
> From: Jonathan McDowell <noodles@meta.com>
>
> tpm_dev_transmit prepares the TPM space before attempting command
> transmission. However if the command fails no rollback of this
> preparation is done. This can result in transient handles being leaked
> if the device is subsequently closed with no further commands performed.
>
> Fix this by flushing the space in the event of command transmission
> failure.
>
> Fixes: 745b361e989a ("tpm: infrastructure for TPM spaces")
> Signed-off-by: Jonathan McDowell <noodles@meta.com>
> ---
> v2:
>  - Add 'Fixes:'
>  - Cc James as one of the original authors
>  - Add space sanity check in tpm2_flush_space
>
>  drivers/char/tpm/tpm-dev-common.c | 2 ++
>  drivers/char/tpm/tpm2-space.c     | 3 +++
>  2 files changed, 5 insertions(+)
>
> diff --git a/drivers/char/tpm/tpm-dev-common.c b/drivers/char/tpm/tpm-dev-common.c
> index 30b4c288c1bb..c3fbbf4d3db7 100644
> --- a/drivers/char/tpm/tpm-dev-common.c
> +++ b/drivers/char/tpm/tpm-dev-common.c
> @@ -47,6 +47,8 @@ static ssize_t tpm_dev_transmit(struct tpm_chip *chip, struct tpm_space *space,
>  
>  	if (!ret)
>  		ret = tpm2_commit_space(chip, space, buf, &len);
> +	else
> +		tpm2_flush_space(chip);
>  
>  out_rc:
>  	return ret ? ret : len;
> diff --git a/drivers/char/tpm/tpm2-space.c b/drivers/char/tpm/tpm2-space.c
> index 4892d491da8d..25a66870c165 100644
> --- a/drivers/char/tpm/tpm2-space.c
> +++ b/drivers/char/tpm/tpm2-space.c
> @@ -169,6 +169,9 @@ void tpm2_flush_space(struct tpm_chip *chip)
>  	struct tpm_space *space = &chip->work_space;
>  	int i;
>  
> +	if (!space)
> +		return;
> +
>  	for (i = 0; i < ARRAY_SIZE(space->context_tbl); i++)
>  		if (space->context_tbl[i] && ~space->context_tbl[i])
>  			tpm2_flush_context(chip, space->context_tbl[i]);

I'll pick this for 6.12: I think it is a legit fix but not worth of
disturbing the in-progress release cycle (which is going almost rc4
already):

Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>

Thank you.

BR, Jarkko