On Sat, 11 Jul 2026 at 10:09, Jarkko Sakkinen <[email protected]> wrote:
>
> Please don't delegate the work for me that you should doing i.e.
> looking at your reproducer and describing the scenario/sequence
> that would lead to infeasible consequence.
I don't understand this reply.
The original email had all the relevant information: how to trigger
the condition, what the bug is, and what's going on.
The bug seems straightforward: the tpm code appears to blindly use a
user-supplied 'loff_t' that is simply not checked. The email you
responded to seems very clear on the issue:
Sequential read() keeps *off in range, but the fops use the legacy .read
callback and neither tpm_open() nor tpmrm_open() calls nonseekable_open(),
so FMODE_PREAD stays set and pread(2) passes an arbitrary offset straight
into *off.
So what's the problem here?
Now, honestly, I think this tpm code is badly written to begin with,
and the patch doesn't fix that fundamental issue.
The code simply shouldn't change response_length by how much have been
read, it should just use the fixed size of the buffer and compare it
to the offset.
And once the buffer has been fully used, we clean it up.
Something ENTIRELY UNTESTED like the attached patch. But I do want to
point out that this patch is
(a) UNTESTED
(b) bigger and more invasive than the simple "just check the offset"
That said, I really don't like how the original patch by Jaewon
depends on TPM_BUFSIZE when the real limit is that "response_length",
but that's a direct result of the tpm code being disgustign and
changing response_length as a response to partial reads.
So the attached patch tries to fix that fundamental mistake, and in
the process I think it makes the code more readable, but whatever. I
probably think that primarily because I wrote the patch.
Comments? But this "dismiss a patch based on bogus reasons" seems like
a huge mistake.
Again. The attached patch is UNTESTED.
Linus
drivers/char/tpm/tpm-dev-common.c | 67 ++++++++++++++++++++++-----------------
1 file changed, 38 insertions(+), 29 deletions(-)
diff --git a/drivers/char/tpm/tpm-dev-common.c b/drivers/char/tpm/tpm-dev-common.c
index f942c0c8e402..f1c17fdea74d 100644
--- a/drivers/char/tpm/tpm-dev-common.c
+++ b/drivers/char/tpm/tpm-dev-common.c
@@ -127,44 +127,53 @@ void tpm_common_open(struct file *file, struct tpm_chip *chip,
file->private_data = priv;
}
+static inline ssize_t tpm_common_read_cleanup(struct file_priv *priv, loff_t *off, ssize_t ret)
+{
+ *off = 0;
+ timer_delete_sync(&priv->user_read_timer);
+ flush_work(&priv->timeout_work);
+ return ret;
+}
+
ssize_t tpm_common_read(struct file *file, char __user *buf,
size_t size, loff_t *off)
{
struct file_priv *priv = file->private_data;
- ssize_t ret_size = 0;
- int rc;
+ ssize_t response_length;
+ u64 offset;
- mutex_lock(&priv->buffer_mutex);
+ if (!size)
+ return 0;
- if (priv->response_length) {
- priv->response_read = true;
+ guard(mutex)(&priv->buffer_mutex);
+ response_length = priv->response_length;
- ret_size = min_t(ssize_t, size, priv->response_length);
- if (ret_size <= 0) {
- priv->response_length = 0;
- goto out;
- }
+ // Error or nothing to read?
+ if (response_length <= 0)
+ return tpm_common_read_cleanup(priv, off, response_length);
- rc = copy_to_user(buf, priv->data_buffer + *off, ret_size);
- if (rc) {
- memset(priv->data_buffer, 0, TPM_BUFSIZE);
- priv->response_length = 0;
- ret_size = -EFAULT;
- } else {
- memset(priv->data_buffer + *off, 0, ret_size);
- priv->response_length -= ret_size;
- *off += ret_size;
- }
- }
+ offset = *off;
-out:
- if (!priv->response_length) {
- *off = 0;
- timer_delete_sync(&priv->user_read_timer);
- flush_work(&priv->timeout_work);
- }
- mutex_unlock(&priv->buffer_mutex);
- return ret_size;
+ // Have we already read everything? Return EOF
+ if (offset >= response_length)
+ return tpm_common_read_cleanup(priv, off, 0);
+
+ // Do the potentially partial read
+ size = min_t(size_t, size, response_length - offset);
+ size -= copy_to_user(buf, priv->data_buffer + offset, size);
+ if (!size)
+ return -EFAULT;
+ offset += size;
+
+ // Are we all done with the response?
+ if (offset >= response_length)
+ return tpm_common_read_cleanup(priv, off, size);
+
+ // We still have some data left, update the offset
+ // and be ready to read the rest later - don't do
+ // the cleanup.
+ *off = offset;
+ return size;
}
ssize_t tpm_common_write(struct file *file, const char __user *buf,
