On Sat, Jul 11, 2026 at 11:58:14AM -0700, Linus Torvalds wrote:
> 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
I hear you. I was probably too quick draw with this one.
> 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,
BR, Jarkko
