[PATCH] tpm: Reject reads outside the response buffer

[PATCH] tpm: Reject reads outside the response buffer

[Jaewon Yang]

tpm_common_read() clamps the transfer length to priv->response_length but
does not validate the file offset (*off) before using it to index the
fixed TPM_BUFSIZE-byte priv->data_buffer:

	ret_size = min_t(ssize_t, size, priv->response_length);
	copy_to_user(buf, priv->data_buffer + *off, ret_size);
	memset(priv->data_buffer + *off, 0, ret_size);

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. An out-of-range offset then accesses memory beyond data_buffer,
causing an out-of-bounds read through copy_to_user() and, when the copy
succeeds, an out-of-bounds zero-write through memset().

Reject any read whose offset and length leave the response buffer.

Fixes: 9488585b21be ("tpm: add support for partial reads")
Cc: stable@vger.kernel.org
Signed-off-by: Jaewon Yang <yong010301@gmail.com>
---
Notes for reviewers (not part of the commit):

Reproduced on a KASAN 6.12 build with a swtpm TPM2 device. After a command
leaves a response pending, pread(fd, buf, 16, 0x1400) triggers two
slab-out-of-bounds reports, one for the copy_to_user() read and one for the
memset() write; on that x86-64 build the faulting access was 962 bytes past
a 4344-byte struct tpmrm_priv served from kmalloc-8k. With this patch,
out-of-range preads (offset past the buffer, at the end, or an in-range
offset whose length crosses the end) return -EINVAL with no KASAN report,
while sequential partial reads still return the full response and a normal
read after a rejected pread still works.

Reaching it needs a process that can open the TPM device and send a command.
Access depends on the device-node permissions; the upstream tpm2-tss udev
rules set tpmrm devices to mode 0660 with group tss. My reproduction ran as
root, so I have not shown non-root reach on a specific distribution or built
a privilege-escalation chain.

I searched public archives on 2026-07-10 and found no matching report, which
does not rule out a private, very recent, or unindexed one. Found through
AI-assisted source review; the code path and reproduction were verified by
hand. A reproducer and full logs are available on request.

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

diff --git a/drivers/char/tpm/tpm-dev-common.c b/drivers/char/tpm/tpm-dev-common.c
index f942c0c8e..dbf049028 100644
--- a/drivers/char/tpm/tpm-dev-common.c
+++ b/drivers/char/tpm/tpm-dev-common.c
@@ -145,6 +145,16 @@ ssize_t tpm_common_read(struct file *file, char __user *buf,
 			goto out;
 		}
 
+		/*
+		 * Reject reads whose offset and length fall outside the fixed
+		 * response buffer.
+		 */
+		if (*off < 0 || *off >= TPM_BUFSIZE ||
+		    ret_size > TPM_BUFSIZE - *off) {
+			ret_size = -EINVAL;
+			goto out;
+		}
+
 		rc = copy_to_user(buf, priv->data_buffer + *off, ret_size);
 		if (rc) {
 			memset(priv->data_buffer, 0, TPM_BUFSIZE);
-- 
2.43.0

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

[Jarkko Sakkinen]

On Fri, Jul 10, 2026 at 06:02:17PM +0900, Jaewon Yang wrote:
> tpm_common_read() clamps the transfer length to priv->response_length but
> does not validate the file offset (*off) before using it to index the
> fixed TPM_BUFSIZE-byte priv->data_buffer:
> 
> 	ret_size = min_t(ssize_t, size, priv->response_length);
> 	copy_to_user(buf, priv->data_buffer + *off, ret_size);
> 	memset(priv->data_buffer + *off, 0, ret_size);
> 
> 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. An out-of-range offset then accesses memory beyond data_buffer,
> causing an out-of-bounds read through copy_to_user() and, when the copy
> succeeds, an out-of-bounds zero-write through memset().
> 
> Reject any read whose offset and length leave the response buffer.
> 
> Fixes: 9488585b21be ("tpm: add support for partial reads")
> Cc: stable@vger.kernel.org
> Signed-off-by: Jaewon Yang <yong010301@gmail.com>
> ---
> Notes for reviewers (not part of the commit):
> 
> Reproduced on a KASAN 6.12 build with a swtpm TPM2 device. After a command
> leaves a response pending, pread(fd, buf, 16, 0x1400) triggers two
> slab-out-of-bounds reports, one for the copy_to_user() read and one for the
> memset() write; on that x86-64 build the faulting access was 962 bytes past
> a 4344-byte struct tpmrm_priv served from kmalloc-8k. With this patch,
> out-of-range preads (offset past the buffer, at the end, or an in-range
> offset whose length crosses the end) return -EINVAL with no KASAN report,
> while sequential partial reads still return the full response and a normal
> read after a rejected pread still works.
> 
> Reaching it needs a process that can open the TPM device and send a command.
> Access depends on the device-node permissions; the upstream tpm2-tss udev
> rules set tpmrm devices to mode 0660 with group tss. My reproduction ran as
> root, so I have not shown non-root reach on a specific distribution or built
> a privilege-escalation chain.
> 
> I searched public archives on 2026-07-10 and found no matching report, which
> does not rule out a private, very recent, or unindexed one. Found through
> AI-assisted source review; the code path and reproduction were verified by
> hand. A reproducer and full logs are available on request.

I will choose to skip reading most of the text as already commit
message had zero relevant information.

Thus, NAK.

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.


> 
>  drivers/char/tpm/tpm-dev-common.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/char/tpm/tpm-dev-common.c b/drivers/char/tpm/tpm-dev-common.c
> index f942c0c8e..dbf049028 100644
> --- a/drivers/char/tpm/tpm-dev-common.c
> +++ b/drivers/char/tpm/tpm-dev-common.c
> @@ -145,6 +145,16 @@ ssize_t tpm_common_read(struct file *file, char __user *buf,
>  			goto out;
>  		}
>  
> +		/*
> +		 * Reject reads whose offset and length fall outside the fixed
> +		 * response buffer.
> +		 */
> +		if (*off < 0 || *off >= TPM_BUFSIZE ||
> +		    ret_size > TPM_BUFSIZE - *off) {
> +			ret_size = -EINVAL;
> +			goto out;
> +		}
> +
>  		rc = copy_to_user(buf, priv->data_buffer + *off, ret_size);
>  		if (rc) {
>  			memset(priv->data_buffer, 0, TPM_BUFSIZE);
> -- 
> 2.43.0
> 

BR, Jarkko

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

[Linus Torvalds]

[-- Attachment #1: Type: text/plain, Size: 1982 bytes --]

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

[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 2568 bytes --]

 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,

[PATCH v2] tpm: Make the TPM character devices non-seekable

[Jaewon Yang]

The TPM character devices expose a sequential command/response
interface, but their open handlers leave FMODE_PREAD and FMODE_PWRITE
enabled.

After a command leaves a response pending, pread(fd, buf, 16, 0x1400)
passes 0x1400 as *off to tpm_common_read(). The transfer length is
bounded by response_length, but the offset is used unchecked when
forming data_buffer + *off. A sufficiently large offset therefore
causes an out-of-bounds heap read through copy_to_user() and, if the
copy succeeds, an out-of-bounds zero-write through the following
memset().

Positional I/O does not provide coherent semantics for this interface.
An arbitrary pread offset cannot represent how much of a response has
been consumed sequentially. The write callback always stores a command
at the start of data_buffer, while pwrite() does not update file->f_pos
and can leave the sequential read cursor stale.

Call nonseekable_open() from both open handlers. This removes
FMODE_PREAD and FMODE_PWRITE, causing positional reads and writes to
fail with -ESPIPE before reaching the TPM callbacks, and explicitly
marks the files non-seekable. Normal read() and write() continue to use
the existing sequential f_pos cursor, leaving the response state
machine unchanged.

Tested on Linux 6.12 with KASAN and a swtpm TPM2 device:

- sequential partial reads returned the complete response;
- pread() and preadv() with offset 0x1400 returned -ESPIPE;
- pwrite() and pwritev() with offset zero returned -ESPIPE;
- the pending response remained intact after the rejected operations;
- a subsequent normal command/response cycle completed normally; and
- no KASAN report was produced.

Fixes: 9488585b21be ("tpm: add support for partial reads")
Link: https://lore.kernel.org/all/20260710090217.191289-1-yong010301@gmail.com/
Cc: stable@vger.kernel.org
Signed-off-by: Jaewon Yang <yong010301@gmail.com>
---
Changes in v2:
- replace the response-buffer bounds check with nonseekable_open();
- reject positional read and write at open time;
- preserve the existing sequential read/write state machine.

The alternative response_length rework proposed during review was tested
and not taken: a read-until-EOF loop hangs because cleanup resets *off
without clearing response_length. It also treats an arbitrary positional
offset as the consumption cursor; for example,

    pread(fd, &c, 1, 99)

on a 100-byte response can discard bytes 0 through 98 without returning
them.

 drivers/char/tpm/tpm-dev.c   | 2 +-
 drivers/char/tpm/tpmrm-dev.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/char/tpm/tpm-dev.c b/drivers/char/tpm/tpm-dev.c
index 2779a8738..74488f0a7 100644
--- a/drivers/char/tpm/tpm-dev.c
+++ b/drivers/char/tpm/tpm-dev.c
@@ -36,7 +36,7 @@ static int tpm_open(struct inode *inode, struct file *file)
 
 	tpm_common_open(file, chip, priv, NULL);
 
-	return 0;
+	return nonseekable_open(inode, file);
 
  out:
 	clear_bit(0, &chip->is_open);
diff --git a/drivers/char/tpm/tpmrm-dev.c b/drivers/char/tpm/tpmrm-dev.c
index f48d4d9e1..19e8f2779 100644
--- a/drivers/char/tpm/tpmrm-dev.c
+++ b/drivers/char/tpm/tpmrm-dev.c
@@ -29,7 +29,7 @@ static int tpmrm_open(struct inode *inode, struct file *file)
 
 	tpm_common_open(file, chip, &priv->priv, &priv->space);
 
-	return 0;
+	return nonseekable_open(inode, file);
 }
 
 static int tpmrm_release(struct inode *inode, struct file *file)
-- 
2.43.0

Re: [PATCH v2] tpm: Make the TPM character devices non-seekable

[Linus Torvalds]

On Sun, 12 Jul 2026 at 10:12, Jaewon Yang <yong010301@gmail.com> wrote:
>
> Call nonseekable_open() from both open handlers.

Ack, this seems to be the simplest approach. I'll just commit it as-is,

                 Linus

Re: [PATCH v2] tpm: Make the TPM character devices non-seekable

[Jarkko Sakkinen]

On Mon, Jul 13, 2026 at 02:11:47AM +0900, Jaewon Yang wrote:
> The TPM character devices expose a sequential command/response
> interface, but their open handlers leave FMODE_PREAD and FMODE_PWRITE
> enabled.
> 
> After a command leaves a response pending, pread(fd, buf, 16, 0x1400)
> passes 0x1400 as *off to tpm_common_read(). The transfer length is
> bounded by response_length, but the offset is used unchecked when
> forming data_buffer + *off. A sufficiently large offset therefore
> causes an out-of-bounds heap read through copy_to_user() and, if the
> copy succeeds, an out-of-bounds zero-write through the following
> memset().
> 
> Positional I/O does not provide coherent semantics for this interface.
> An arbitrary pread offset cannot represent how much of a response has
> been consumed sequentially. The write callback always stores a command
> at the start of data_buffer, while pwrite() does not update file->f_pos
> and can leave the sequential read cursor stale.
> 
> Call nonseekable_open() from both open handlers. This removes
> FMODE_PREAD and FMODE_PWRITE, causing positional reads and writes to
> fail with -ESPIPE before reaching the TPM callbacks, and explicitly
> marks the files non-seekable. Normal read() and write() continue to use
> the existing sequential f_pos cursor, leaving the response state
> machine unchanged.
> 
> Tested on Linux 6.12 with KASAN and a swtpm TPM2 device:
> 
> - sequential partial reads returned the complete response;
> - pread() and preadv() with offset 0x1400 returned -ESPIPE;
> - pwrite() and pwritev() with offset zero returned -ESPIPE;
> - the pending response remained intact after the rejected operations;
> - a subsequent normal command/response cycle completed normally; and
> - no KASAN report was produced.
> 
> Fixes: 9488585b21be ("tpm: add support for partial reads")
> Link: https://lore.kernel.org/all/20260710090217.191289-1-yong010301@gmail.com/
> Cc: stable@vger.kernel.org
> Signed-off-by: Jaewon Yang <yong010301@gmail.com>
> ---
> Changes in v2:
> - replace the response-buffer bounds check with nonseekable_open();
> - reject positional read and write at open time;
> - preserve the existing sequential read/write state machine.
> 
> The alternative response_length rework proposed during review was tested
> and not taken: a read-until-EOF loop hangs because cleanup resets *off
> without clearing response_length. It also treats an arbitrary positional
> offset as the consumption cursor; for example,
> 
>     pread(fd, &c, 1, 99)
> 
> on a 100-byte response can discard bytes 0 through 98 without returning
> them.
> 
>  drivers/char/tpm/tpm-dev.c   | 2 +-
>  drivers/char/tpm/tpmrm-dev.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-dev.c b/drivers/char/tpm/tpm-dev.c
> index 2779a8738..74488f0a7 100644
> --- a/drivers/char/tpm/tpm-dev.c
> +++ b/drivers/char/tpm/tpm-dev.c
> @@ -36,7 +36,7 @@ static int tpm_open(struct inode *inode, struct file *file)
>  
>  	tpm_common_open(file, chip, priv, NULL);
>  
> -	return 0;
> +	return nonseekable_open(inode, file);
>  
>   out:
>  	clear_bit(0, &chip->is_open);
> diff --git a/drivers/char/tpm/tpmrm-dev.c b/drivers/char/tpm/tpmrm-dev.c
> index f48d4d9e1..19e8f2779 100644
> --- a/drivers/char/tpm/tpmrm-dev.c
> +++ b/drivers/char/tpm/tpmrm-dev.c
> @@ -29,7 +29,7 @@ static int tpmrm_open(struct inode *inode, struct file *file)
>  
>  	tpm_common_open(file, chip, &priv->priv, &priv->space);
>  
> -	return 0;
> +	return nonseekable_open(inode, file);
>  }
>  
>  static int tpmrm_release(struct inode *inode, struct file *file)
> -- 
> 2.43.0
> 

LGTM


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

BR, Jarkko

Re: [PATCH v2] tpm: Make the TPM character devices non-seekable

[Jarkko Sakkinen]

oN Sat, Jul 18, 2026 at 09:13:30PM +0300, Jarkko Sakkinen wrote:
> On Mon, Jul 13, 2026 at 02:11:47AM +0900, Jaewon Yang wrote:
> > The TPM character devices expose a sequential command/response
> > interface, but their open handlers leave FMODE_PREAD and FMODE_PWRITE
> > enabled.
> > 
> > After a command leaves a response pending, pread(fd, buf, 16, 0x1400)
> > passes 0x1400 as *off to tpm_common_read(). The transfer length is
> > bounded by response_length, but the offset is used unchecked when
> > forming data_buffer + *off. A sufficiently large offset therefore
> > causes an out-of-bounds heap read through copy_to_user() and, if the
> > copy succeeds, an out-of-bounds zero-write through the following
> > memset().
> > 
> > Positional I/O does not provide coherent semantics for this interface.
> > An arbitrary pread offset cannot represent how much of a response has
> > been consumed sequentially. The write callback always stores a command
> > at the start of data_buffer, while pwrite() does not update file->f_pos
> > and can leave the sequential read cursor stale.
> > 
> > Call nonseekable_open() from both open handlers. This removes
> > FMODE_PREAD and FMODE_PWRITE, causing positional reads and writes to
> > fail with -ESPIPE before reaching the TPM callbacks, and explicitly
> > marks the files non-seekable. Normal read() and write() continue to use
> > the existing sequential f_pos cursor, leaving the response state
> > machine unchanged.
> > 
> > Tested on Linux 6.12 with KASAN and a swtpm TPM2 device:
> > 
> > - sequential partial reads returned the complete response;
> > - pread() and preadv() with offset 0x1400 returned -ESPIPE;
> > - pwrite() and pwritev() with offset zero returned -ESPIPE;
> > - the pending response remained intact after the rejected operations;
> > - a subsequent normal command/response cycle completed normally; and
> > - no KASAN report was produced.
> > 
> > Fixes: 9488585b21be ("tpm: add support for partial reads")
> > Link: https://lore.kernel.org/all/20260710090217.191289-1-yong010301@gmail.com/
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Jaewon Yang <yong010301@gmail.com>
> > ---
> > Changes in v2:
> > - replace the response-buffer bounds check with nonseekable_open();
> > - reject positional read and write at open time;
> > - preserve the existing sequential read/write state machine.
> > 
> > The alternative response_length rework proposed during review was tested
> > and not taken: a read-until-EOF loop hangs because cleanup resets *off
> > without clearing response_length. It also treats an arbitrary positional
> > offset as the consumption cursor; for example,
> > 
> >     pread(fd, &c, 1, 99)
> > 
> > on a 100-byte response can discard bytes 0 through 98 without returning
> > them.
> > 
> >  drivers/char/tpm/tpm-dev.c   | 2 +-
> >  drivers/char/tpm/tpmrm-dev.c | 2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/char/tpm/tpm-dev.c b/drivers/char/tpm/tpm-dev.c
> > index 2779a8738..74488f0a7 100644
> > --- a/drivers/char/tpm/tpm-dev.c
> > +++ b/drivers/char/tpm/tpm-dev.c
> > @@ -36,7 +36,7 @@ static int tpm_open(struct inode *inode, struct file *file)
> >  
> >  	tpm_common_open(file, chip, priv, NULL);
> >  
> > -	return 0;
> > +	return nonseekable_open(inode, file);
> >  
> >   out:
> >  	clear_bit(0, &chip->is_open);
> > diff --git a/drivers/char/tpm/tpmrm-dev.c b/drivers/char/tpm/tpmrm-dev.c
> > index f48d4d9e1..19e8f2779 100644
> > --- a/drivers/char/tpm/tpmrm-dev.c
> > +++ b/drivers/char/tpm/tpmrm-dev.c
> > @@ -29,7 +29,7 @@ static int tpmrm_open(struct inode *inode, struct file *file)
> >  
> >  	tpm_common_open(file, chip, &priv->priv, &priv->space);
> >  
> > -	return 0;
> > +	return nonseekable_open(inode, file);
> >  }
> >  
> >  static int tpmrm_release(struct inode *inode, struct file *file)
> > -- 
> > 2.43.0
> > 
> 
> LGTM
> 
> 
> Acked-by: Jarkko Sakkinen <jarkko@kernel.org>

[A slow response time due on holiday until end of this Month.]

BR, Jarkko

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

[Jarkko Sakkinen]

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