* [PATCH] tpm2: fix TIS locality timeout problems
@ 2020-05-27 0:25 James Bottomley
2020-05-27 1:01 ` Mario.Limonciello
` (2 more replies)
0 siblings, 3 replies; 4+ messages in thread
From: James Bottomley @ 2020-05-27 0:25 UTC (permalink / raw)
To: linux-integrity
Cc: Jarkko Sakkinen, Jason Gunthorpe, Mario Limonciello, Alex Guzman
It has been reported that some TIS based TPMs are giving unexpected
errors when using the O_NONBLOCK path. The problem is that some TPMs
don't like it when you get and then relinquish a locality (as the
tpm_try_get_ops/tpm_put_ops pair does) without sending a command.
This currently happens all the time in the O_NONBLOCK write path. We
can fix this by moving the tpm_try_get_ops further down the code to
after the O_NONBLOCK determination is made. This is safe because the
priv->buffer_mutex still protects the priv state being modified.
Fixes: d23d12484307 ("tpm: fix invalid locking in NONBLOCKING mode")
Reported-by: Mario Limonciello <Mario.Limonciello@dell.com>
Tested-by: Alex Guzman <alex@guzman.io>
Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
---
drivers/char/tpm/tpm-dev-common.c | 19 +++++++++----------
1 file changed, 9 insertions(+), 10 deletions(-)
diff --git a/drivers/char/tpm/tpm-dev-common.c b/drivers/char/tpm/tpm-dev-common.c
index 87f449340202..1784530b8387 100644
--- a/drivers/char/tpm/tpm-dev-common.c
+++ b/drivers/char/tpm/tpm-dev-common.c
@@ -189,15 +189,6 @@ ssize_t tpm_common_write(struct file *file, const char __user *buf,
goto out;
}
- /* atomic tpm command send and result receive. We only hold the ops
- * lock during this period so that the tpm can be unregistered even if
- * the char dev is held open.
- */
- if (tpm_try_get_ops(priv->chip)) {
- ret = -EPIPE;
- goto out;
- }
-
priv->response_length = 0;
priv->response_read = false;
*off = 0;
@@ -211,11 +202,19 @@ ssize_t tpm_common_write(struct file *file, const char __user *buf,
if (file->f_flags & O_NONBLOCK) {
priv->command_enqueued = true;
queue_work(tpm_dev_wq, &priv->async_work);
- tpm_put_ops(priv->chip);
mutex_unlock(&priv->buffer_mutex);
return size;
}
+ /* atomic tpm command send and result receive. We only hold the ops
+ * lock during this period so that the tpm can be unregistered even if
+ * the char dev is held open.
+ */
+ if (tpm_try_get_ops(priv->chip)) {
+ ret = -EPIPE;
+ goto out;
+ }
+
ret = tpm_dev_transmit(priv->chip, priv->space, priv->data_buffer,
sizeof(priv->data_buffer));
tpm_put_ops(priv->chip);
--
2.26.2
^ permalink raw reply related [flat|nested] 4+ messages in thread
* RE: [PATCH] tpm2: fix TIS locality timeout problems
2020-05-27 0:25 [PATCH] tpm2: fix TIS locality timeout problems James Bottomley
@ 2020-05-27 1:01 ` Mario.Limonciello
2020-05-27 15:58 ` Jerry Snitselaar
2020-05-28 0:38 ` Jarkko Sakkinen
2 siblings, 0 replies; 4+ messages in thread
From: Mario.Limonciello @ 2020-05-27 1:01 UTC (permalink / raw)
To: James.Bottomley, linux-integrity; +Cc: jarkko.sakkinen, jgg, alex
> -----Original Message-----
> From: James Bottomley <James.Bottomley@HansenPartnership.com>
> Sent: Tuesday, May 26, 2020 7:25 PM
> To: linux-integrity@vger.kernel.org
> Cc: Jarkko Sakkinen; Jason Gunthorpe; Limonciello, Mario; Alex Guzman
> Subject: [PATCH] tpm2: fix TIS locality timeout problems
>
>
> [EXTERNAL EMAIL]
>
> It has been reported that some TIS based TPMs are giving unexpected
> errors when using the O_NONBLOCK path. The problem is that some TPMs
> don't like it when you get and then relinquish a locality (as the
> tpm_try_get_ops/tpm_put_ops pair does) without sending a command.
> This currently happens all the time in the O_NONBLOCK write path. We
> can fix this by moving the tpm_try_get_ops further down the code to
> after the O_NONBLOCK determination is made. This is safe because the
> priv->buffer_mutex still protects the priv state being modified.
>
> Fixes: d23d12484307 ("tpm: fix invalid locking in NONBLOCKING mode")
> Reported-by: Mario Limonciello <Mario.Limonciello@dell.com>
> Tested-by: Alex Guzman <alex@guzman.io>
> Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
A few other notes for right here:
* You should probably mention this buglink:
BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=206275
* And add another Reported-by: For Alex probably makes sense , as he is
the original reporter to the bug. I just helped to bring it here.
* This should CC stable (as this regression managed to hit the stable
kernels too).
> ---
> drivers/char/tpm/tpm-dev-common.c | 19 +++++++++----------
> 1 file changed, 9 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm-dev-common.c b/drivers/char/tpm/tpm-dev-
> common.c
> index 87f449340202..1784530b8387 100644
> --- a/drivers/char/tpm/tpm-dev-common.c
> +++ b/drivers/char/tpm/tpm-dev-common.c
> @@ -189,15 +189,6 @@ ssize_t tpm_common_write(struct file *file, const char
> __user *buf,
> goto out;
> }
>
> - /* atomic tpm command send and result receive. We only hold the ops
> - * lock during this period so that the tpm can be unregistered even if
> - * the char dev is held open.
> - */
> - if (tpm_try_get_ops(priv->chip)) {
> - ret = -EPIPE;
> - goto out;
> - }
> -
> priv->response_length = 0;
> priv->response_read = false;
> *off = 0;
> @@ -211,11 +202,19 @@ ssize_t tpm_common_write(struct file *file, const char
> __user *buf,
> if (file->f_flags & O_NONBLOCK) {
> priv->command_enqueued = true;
> queue_work(tpm_dev_wq, &priv->async_work);
> - tpm_put_ops(priv->chip);
> mutex_unlock(&priv->buffer_mutex);
> return size;
> }
>
> + /* atomic tpm command send and result receive. We only hold the ops
> + * lock during this period so that the tpm can be unregistered even if
> + * the char dev is held open.
> + */
> + if (tpm_try_get_ops(priv->chip)) {
> + ret = -EPIPE;
> + goto out;
> + }
> +
> ret = tpm_dev_transmit(priv->chip, priv->space, priv->data_buffer,
> sizeof(priv->data_buffer));
> tpm_put_ops(priv->chip);
> --
> 2.26.2
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] tpm2: fix TIS locality timeout problems
2020-05-27 0:25 [PATCH] tpm2: fix TIS locality timeout problems James Bottomley
2020-05-27 1:01 ` Mario.Limonciello
@ 2020-05-27 15:58 ` Jerry Snitselaar
2020-05-28 0:38 ` Jarkko Sakkinen
2 siblings, 0 replies; 4+ messages in thread
From: Jerry Snitselaar @ 2020-05-27 15:58 UTC (permalink / raw)
To: James Bottomley
Cc: linux-integrity, Jarkko Sakkinen, Jason Gunthorpe,
Mario Limonciello, Alex Guzman
On Tue May 26 20, James Bottomley wrote:
>It has been reported that some TIS based TPMs are giving unexpected
>errors when using the O_NONBLOCK path. The problem is that some TPMs
>don't like it when you get and then relinquish a locality (as the
>tpm_try_get_ops/tpm_put_ops pair does) without sending a command.
>This currently happens all the time in the O_NONBLOCK write path. We
>can fix this by moving the tpm_try_get_ops further down the code to
>after the O_NONBLOCK determination is made. This is safe because the
>priv->buffer_mutex still protects the priv state being modified.
>
>Fixes: d23d12484307 ("tpm: fix invalid locking in NONBLOCKING mode")
>Reported-by: Mario Limonciello <Mario.Limonciello@dell.com>
>Tested-by: Alex Guzman <alex@guzman.io>
>Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] tpm2: fix TIS locality timeout problems
2020-05-27 0:25 [PATCH] tpm2: fix TIS locality timeout problems James Bottomley
2020-05-27 1:01 ` Mario.Limonciello
2020-05-27 15:58 ` Jerry Snitselaar
@ 2020-05-28 0:38 ` Jarkko Sakkinen
2 siblings, 0 replies; 4+ messages in thread
From: Jarkko Sakkinen @ 2020-05-28 0:38 UTC (permalink / raw)
To: James Bottomley
Cc: linux-integrity, Jason Gunthorpe, Mario Limonciello, Alex Guzman
On Tue, May 26, 2020 at 05:25:14PM -0700, James Bottomley wrote:
> It has been reported that some TIS based TPMs are giving unexpected
> errors when using the O_NONBLOCK path. The problem is that some TPMs
> don't like it when you get and then relinquish a locality (as the
> tpm_try_get_ops/tpm_put_ops pair does) without sending a command.
> This currently happens all the time in the O_NONBLOCK write path. We
> can fix this by moving the tpm_try_get_ops further down the code to
> after the O_NONBLOCK determination is made. This is safe because the
> priv->buffer_mutex still protects the priv state being modified.
>
> Fixes: d23d12484307 ("tpm: fix invalid locking in NONBLOCKING mode")
> Reported-by: Mario Limonciello <Mario.Limonciello@dell.com>
> Tested-by: Alex Guzman <alex@guzman.io>
> Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
Subsystem tag is wrong, function name is missing '()' after and
sometimes there are spaces after ".". Please fix the commit
message and I can look at the code when it is clean.
/Jarkko
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-05-28 0:38 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-27 0:25 [PATCH] tpm2: fix TIS locality timeout problems James Bottomley
2020-05-27 1:01 ` Mario.Limonciello
2020-05-27 15:58 ` Jerry Snitselaar
2020-05-28 0:38 ` Jarkko Sakkinen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).