Re: [PATCH] tpm: Reject reads outside the response buffer

[Jarkko Sakkinen <jarkko@kernel.org>]

On Sat, Jul 11, 2026 at 11:58:14AM -0700, Linus Torvalds wrote:
> On Sat, 11 Jul 2026 at 10:09, Jarkko Sakkinen <jarkko@kernel.org> 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