Message-ID: <vtfrdnkts3hqwvretrqrx445cef5wjourgdmay6rzrrwmfc3hu@nbs5cf2oz3b3>
Date: Wed, 24 Sep 2025 12:10:31 +0200
From: Stefano Garzarella <sgarzare@...hat.com>
To: Jarkko Sakkinen <jarkko@...nel.org>
Cc: linux-integrity@...r.kernel.org, 
	Jarkko Sakkinen <jarkko.sakkinen@...nsys.com>, Stefan Berger <stefanb@...ux.ibm.com>, 
	Peter Huewe <peterhuewe@....de>, Jason Gunthorpe <jgg@...pe.ca>, 
	David Howells <dhowells@...hat.com>, Paul Moore <paul@...l-moore.com>, 
	James Morris <jmorris@...ei.org>, "Serge E. Hallyn" <serge@...lyn.com>, 
	James Bottomley <James.Bottomley@...senpartnership.com>, Mimi Zohar <zohar@...ux.ibm.com>, 
	open list <linux-kernel@...r.kernel.org>, "open list:KEYS/KEYRINGS" <keyrings@...r.kernel.org>, 
	"open list:SECURITY SUBSYSTEM" <linux-security-module@...r.kernel.org>
Subject: Re: [PATCH v10 1/4] tpm: Make TPM buffer allocations more robust

On Tue, Sep 23, 2025 at 05:30:20PM +0300, Jarkko Sakkinen wrote:
>On Mon, Sep 22, 2025 at 10:44:34AM +0200, Stefano Garzarella wrote:
>> On Sun, Sep 21, 2025 at 05:08:01AM +0300, Jarkko Sakkinen wrote:
>> > From: Jarkko Sakkinen <jarkko.sakkinen@...nsys.com>
>> >
>> > Create more ergonomic primitives to work with TPM buffers, where
>> > 'tpm_buf_init*' initialize the memory and 'tpm_buf_reset*' (re)set a buffer
>> > for a particular use and purpose. The new primitives are ubiquitos when
>> > it comes to heap and stack usage.
>> >
>> > Given that the kzalloc is decoupled, a managed allocation can be done
>> > trivially:
>> >
>> > 	struct tpm_buf *buf __free(kfree) buf = kzalloc(TPM_BUF_MAX_SIZE,
>> > 							GFP_KERNEL);
>> >
>> > This effectively zeros out the odds having any memory leaks with TPM
>> > buffers. The new structures can be later used to widen the use of stack
>> > over heap in the subsystem in the critical code paths..
>> >
>> > Reviewed-by: Stefan Berger <stefanb@...ux.ibm.com>
>> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@...nsys.com>
>> > ---
>> > v10:
>> > - No changes.
>> > v9:
>> > - Rename pre-existing TPM_BUFSIZE as TPM_BUF_MAX_SIZE and redeclare
>> >  it in include/linux/tpm.h, and use this constant instead of
>> >  PAGE_SIZE in tpm_buf_init*. Define TPM_BUF_MIN_SIZE to be a
>> >  sane placeholder value for stack allocations.
>> > - Fix and restructure invariant for the sake of clarity.
>> > - memset buffers to zero in tpm_buf_init* in order to guarantee a legit
>> >  initial state. This does not cause any regressions but once tpm_buf
>> >  is allocaed from stack at some site, this will zero out chance of
>> >  corrupted state.
>> > v8:
>> > - Decouple memory init i.e. bring tpm_init* back but with new fresh
>> >  form.
>> > - Cap buf_size to page size and also make checks in tpm_buf_append
>> >  safe:
>> >  https://lore.kernel.org/linux-integrity/hwsx2t2tkbos3g7k2syemxbvc6sfrsbviwm64ubcdl6ms7ljvo@toetomkhheht/
>> > - Fix trusted_tpm_send() and simplify the flow.
>> > v7:
>> > - Additional function comments and invariant was unfortunately left to
>> >  my staging area so here's the addition (does not affect functionality).
>> > v6:
>> > - Removed two empty lines as requested by Stefan:
>> >  https://lore.kernel.org/linux-integrity/be1c5bef-7c97-4173-b417-986dc90d779c@linux.ibm.com/
>> > - Add 'capacity' field as this makes easy to stretch tpm_buf into stack
>> >  allocation.
>> > v5:
>> > - I tested this version also with TPM 1.2 by booting up and checking
>> >  that sysfs attributes work.
>> > - Fixed the length check against capacity (James) with TPM_BUF_CAPACITY.
>> > - Fixed declaration style: do it at the site (Jason).
>> > - Improved commit message (Stefan).
>> > - Removed "out" label from tpm2_pcr_read() (Stefan).
>> > - Removed spurious "return rc;" from tpm2_get_pcr_allocation() (Stefan).
>> > v4:
>> > - Wrote a more a descriptive short summary and improved description.
>> > - Fixed the error in documentation: there is 4090 bytes of space left
>> >  for the payload - not 4088 bytes.
>> > - Turned tpm_buf_alloc() into inline function.
>> > v3:
>> > - Removed the cleanup class and moved on using __free(kfree) instead.
>> > - Removed `buf_size` (James).
>> > - I'm open for the idea of splitting still (Jason) but I'll hold
>> >  at least this revision just to check that my core idea here
>> >  is correct.
>> > v2:
>> > - Implement also memory allocation using the cleanup class.
>> > - Rewrote the commit message.
>> > - Implemented CLASS_TPM_BUF() helper macro.
>> > ---
>> > drivers/char/tpm/tpm-buf.c                | 137 +++++++----
>> > drivers/char/tpm/tpm-dev-common.c         |   4 +-
>> > drivers/char/tpm/tpm-dev.h                |   2 +-
>> > drivers/char/tpm/tpm-interface.c          |   4 +-
>> > drivers/char/tpm/tpm-sysfs.c              |  20 +-
>> > drivers/char/tpm/tpm.h                    |   3 +-
>> > drivers/char/tpm/tpm1-cmd.c               | 149 ++++++------
>> > drivers/char/tpm/tpm2-cmd.c               | 282 ++++++++++------------
>> > drivers/char/tpm/tpm2-sessions.c          | 121 +++++-----
>> > drivers/char/tpm/tpm2-space.c             |  44 ++--
>> > drivers/char/tpm/tpm_tis_i2c.c            |   4 +-
>> > drivers/char/tpm/tpm_vtpm_proxy.c         |  32 ++-
>> > include/linux/tpm.h                       |  28 ++-
>> > security/keys/trusted-keys/trusted_tpm1.c |  34 ++-
>> > security/keys/trusted-keys/trusted_tpm2.c | 156 ++++++------
>> > 15 files changed, 493 insertions(+), 527 deletions(-)
>> >
>> > diff --git a/drivers/char/tpm/tpm-buf.c b/drivers/char/tpm/tpm-buf.c
>> > index dc882fc9fa9e..82dce0350a41 100644
>> > --- a/drivers/char/tpm/tpm-buf.c
>> > +++ b/drivers/char/tpm/tpm-buf.c
>> > @@ -7,82 +7,107 @@
>> > #include <linux/module.h>
>> > #include <linux/tpm.h>
>> >
>> > -/**
>> > - * tpm_buf_init() - Allocate and initialize a TPM command
>> > - * @buf:	A &tpm_buf
>> > - * @tag:	TPM_TAG_RQU_COMMAND, TPM2_ST_NO_SESSIONS or TPM2_ST_SESSIONS
>> > - * @ordinal:	A command ordinal
>> > - *
>> > - * Return: 0 or -ENOMEM
>> > - */
>> > -int tpm_buf_init(struct tpm_buf *buf, u16 tag, u32 ordinal)
>> > +static void __tpm_buf_size_invariant(struct tpm_buf *buf, u16 buf_size)
>> > {
>> > -	buf->data = (u8 *)__get_free_page(GFP_KERNEL);
>> > -	if (!buf->data)
>> > -		return -ENOMEM;
>> > -
>> > -	tpm_buf_reset(buf, tag, ordinal);
>> > -	return 0;
>> > +	if (!buf->capacity) {
>> > +		if (buf_size > TPM_BUF_MAX_SIZE) {
>> > +			WARN(1, "%s: size overflow: %u\n", __func__, buf_size);
>>
>> IIUC here we will always print something like:
>>     "__tpm_buf_size_invariant: size overflow: XXX"
>>
>> So, should we just remove `__func__` or maybe use a macro to print the name
>> of the caller?
>
>It also prints a stack trace.

Oh, I see, thanks!

>
>>
>> > +			buf->flags |= TPM_BUF_ERROR;
>> > +		}
>> > +	} else {
>> > +		if (buf_size != buf->capacity + sizeof(*buf)) {
>> > +			WARN(1, "%s: size mismatch: %u != %u\n", __func__, buf_size,
>> > +			     buf->capacity + sizeof(*buf));
>> > +			buf->flags |= TPM_BUF_ERROR;
>> > +		}
>> > +	}
>> > }
>> > -EXPORT_SYMBOL_GPL(tpm_buf_init);
>> >
>> > -/**
>> > - * tpm_buf_reset() - Initialize a TPM command
>> > - * @buf:	A &tpm_buf
>> > - * @tag:	TPM_TAG_RQU_COMMAND, TPM2_ST_NO_SESSIONS or TPM2_ST_SESSIONS
>> > - * @ordinal:	A command ordinal
>> > - */
>> > -void tpm_buf_reset(struct tpm_buf *buf, u16 tag, u32 ordinal)
>> > +static void __tpm_buf_reset(struct tpm_buf *buf, u16 buf_size, u16 tag, u32 ordinal)
>> > {
>> > 	struct tpm_header *head = (struct tpm_header *)buf->data;
>> >
>> > +	__tpm_buf_size_invariant(buf, buf_size);
>> > +
>> > +	if (buf->flags & TPM_BUF_ERROR)
>> > +		return;
>> > +
>> > 	WARN_ON(tag != TPM_TAG_RQU_COMMAND && tag != TPM2_ST_NO_SESSIONS &&
>> > 		tag != TPM2_ST_SESSIONS && tag != 0);
>> >
>> > 	buf->flags = 0;
>> > 	buf->length = sizeof(*head);
>> > +	buf->capacity = buf_size - sizeof(*buf);
>> > +	buf->handles = 0;
>> > 	head->tag = cpu_to_be16(tag);
>> > 	head->length = cpu_to_be32(sizeof(*head));
>> > 	head->ordinal = cpu_to_be32(ordinal);
>> > +}
>> > +
>> > +static void __tpm_buf_reset_sized(struct tpm_buf *buf, u16 buf_size)
>> > +{
>> > +	__tpm_buf_size_invariant(buf, buf_size);
>> > +
>> > +	if (buf->flags & TPM_BUF_ERROR)
>> > +		return;
>> > +
>> > +	buf->flags = TPM_BUF_TPM2B;
>> > +	buf->length = 2;
>> > +	buf->capacity = buf_size - sizeof(*buf);
>> > 	buf->handles = 0;
>> > +	buf->data[0] = 0;
>> > +	buf->data[1] = 0;
>> > }
>> > -EXPORT_SYMBOL_GPL(tpm_buf_reset);
>> >
>> > /**
>> > - * tpm_buf_init_sized() - Allocate and initialize a sized (TPM2B) buffer
>> > - * @buf:	A @tpm_buf
>> > - *
>> > - * Return: 0 or -ENOMEM
>> > + * tpm_buf_init() - Initialize a TPM command
>> > + * @buf:	A &tpm_buf
>> > + * @buf_size:	Size of the buffer.
>> >  */
>> > -int tpm_buf_init_sized(struct tpm_buf *buf)
>> > +void tpm_buf_init(struct tpm_buf *buf, u16 buf_size)
>> > {
>> > -	buf->data = (u8 *)__get_free_page(GFP_KERNEL);
>> > -	if (!buf->data)
>> > -		return -ENOMEM;
>> > +	memset(buf, 0, buf_size);
>> > +	__tpm_buf_reset(buf, buf_size, TPM_TAG_RQU_COMMAND, 0);
>> > +}
>> > +EXPORT_SYMBOL_GPL(tpm_buf_init);
>> >
>> > -	tpm_buf_reset_sized(buf);
>> > -	return 0;
>> > +/**
>> > + * tpm_buf_init_sized() - Initialize a sized buffer
>> > + * @buf:	A &tpm_buf
>> > + * @buf_size:	Size of the buffer.
>> > + */
>> > +void tpm_buf_init_sized(struct tpm_buf *buf, u16 buf_size)
>> > +{
>> > +	memset(buf, 0, buf_size);
>> > +	__tpm_buf_reset_sized(buf, buf_size);
>> > }
>> > EXPORT_SYMBOL_GPL(tpm_buf_init_sized);
>> >
>> > /**
>> > - * tpm_buf_reset_sized() - Initialize a sized buffer
>> > + * tpm_buf_reset() - Re-initialize a TPM command
>> >  * @buf:	A &tpm_buf
>> > + * @tag:	TPM_TAG_RQU_COMMAND, TPM2_ST_NO_SESSIONS or TPM2_ST_SESSIONS
>> > + * @ordinal:	A command ordinal
>> >  */
>> > -void tpm_buf_reset_sized(struct tpm_buf *buf)
>> > +void tpm_buf_reset(struct tpm_buf *buf, u16 tag, u32 ordinal)
>> > {
>> > -	buf->flags = TPM_BUF_TPM2B;
>> > -	buf->length = 2;
>> > -	buf->data[0] = 0;
>> > -	buf->data[1] = 0;
>> > +	u16 buf_size = buf->capacity + sizeof(*buf);
>> > +
>> > +	__tpm_buf_reset(buf, buf_size, tag, ordinal);
>> > }
>> > -EXPORT_SYMBOL_GPL(tpm_buf_reset_sized);
>> > +EXPORT_SYMBOL_GPL(tpm_buf_reset);
>> >
>> > -void tpm_buf_destroy(struct tpm_buf *buf)
>> > +/**
>> > + * tpm_buf_reset_sized() - Re-initialize a sized buffer
>> > + * @buf:	A &tpm_buf
>> > + */
>> > +void tpm_buf_reset_sized(struct tpm_buf *buf)
>> > {
>> > -	free_page((unsigned long)buf->data);
>> > +	u16 buf_size = buf->capacity + sizeof(*buf);
>> > +
>> > +	__tpm_buf_reset_sized(buf, buf_size);
>> > }
>> > -EXPORT_SYMBOL_GPL(tpm_buf_destroy);
>> > +EXPORT_SYMBOL_GPL(tpm_buf_reset_sized);
>> >
>> > /**
>> >  * tpm_buf_length() - Return the number of bytes consumed by the data
>> > @@ -92,6 +117,9 @@ EXPORT_SYMBOL_GPL(tpm_buf_destroy);
>> >  */
>> > u32 tpm_buf_length(struct tpm_buf *buf)
>> > {
>> > +	if (buf->flags & TPM_BUF_ERROR)
>> > +		return 0;
>> > +
>> > 	return buf->length;
>> > }
>> > EXPORT_SYMBOL_GPL(tpm_buf_length);
>> > @@ -104,13 +132,14 @@ EXPORT_SYMBOL_GPL(tpm_buf_length);
>> >  */
>> > void tpm_buf_append(struct tpm_buf *buf, const u8 *new_data, u16 new_length)
>> > {
>> > -	/* Return silently if overflow has already happened. */
>> > -	if (buf->flags & TPM_BUF_OVERFLOW)
>> > +	u32 total_length = (u32)buf->length + (u32)new_length;
>> > +
>> > +	if (buf->flags & TPM_BUF_ERROR)
>> > 		return;
>> >
>> > -	if ((buf->length + new_length) > PAGE_SIZE) {
>> > +	if (total_length > (u32)buf->capacity) {
>> > 		WARN(1, "tpm_buf: write overflow\n");
>> > -		buf->flags |= TPM_BUF_OVERFLOW;
>> > +		buf->flags |= TPM_BUF_ERROR;
>> > 		return;
>> > 	}
>> >
>> > @@ -157,8 +186,12 @@ EXPORT_SYMBOL_GPL(tpm_buf_append_u32);
>> >  */
>> > void tpm_buf_append_handle(struct tpm_chip *chip, struct tpm_buf *buf, u32 handle)
>> > {
>> > +	if (buf->flags & TPM_BUF_ERROR)
>> > +		return;
>> > +
>> > 	if (buf->flags & TPM_BUF_TPM2B) {
>> > -		dev_err(&chip->dev, "Invalid buffer type (TPM2B)\n");
>> > +		dev_err(&chip->dev, "%s: invalid for buffer type: TPM2B\n", __func__);
>> > +		buf->flags |= TPM_BUF_ERROR;
>> > 		return;
>> > 	}
>> >
>> > @@ -178,13 +211,13 @@ static void tpm_buf_read(struct tpm_buf *buf, off_t *offset, size_t count, void
>> > 	off_t next_offset;
>> >
>> > 	/* Return silently if overflow has already happened. */
>> > -	if (buf->flags & TPM_BUF_BOUNDARY_ERROR)
>> > +	if (buf->flags & TPM_BUF_ERROR)
>> > 		return;
>> >
>> > 	next_offset = *offset + count;
>> > 	if (next_offset > buf->length) {
>> > 		WARN(1, "tpm_buf: read out of boundary\n");
>> > -		buf->flags |= TPM_BUF_BOUNDARY_ERROR;
>> > +		buf->flags |= TPM_BUF_ERROR;
>> > 		return;
>> > 	}
>> >
>> > @@ -242,5 +275,3 @@ u32 tpm_buf_read_u32(struct tpm_buf *buf, off_t *offset)
>> > 	return be32_to_cpu(value);
>> > }
>> > EXPORT_SYMBOL_GPL(tpm_buf_read_u32);
>> > -
>> > -
>> > diff --git a/drivers/char/tpm/tpm-dev-common.c b/drivers/char/tpm/tpm-dev-common.c
>> > index f2a5e09257dd..4f5893555fb7 100644
>> > --- a/drivers/char/tpm/tpm-dev-common.c
>> > +++ b/drivers/char/tpm/tpm-dev-common.c
>> > @@ -147,7 +147,7 @@ ssize_t tpm_common_read(struct file *file, char __user *buf,
>> >
>> > 		rc = copy_to_user(buf, priv->data_buffer + *off, 		ret_size);
>> > 		if (rc) {
>> > -			memset(priv->data_buffer, 0, TPM_BUFSIZE);
>> > +			memset(priv->data_buffer, 0, TPM_BUF_MAX_SIZE);
>> > 			priv->response_length = 0;
>> > 			ret_size = -EFAULT;
>> > 		} else {
>> > @@ -173,7 +173,7 @@ ssize_t tpm_common_write(struct file *file, const char __user *buf,
>> > 	struct file_priv *priv = file->private_data;
>> > 	int ret = 0;
>> >
>> > -	if (size > TPM_BUFSIZE)
>> > +	if (size > TPM_BUF_MAX_SIZE)
>> > 		return -E2BIG;
>> >
>> > 	mutex_lock(&priv->buffer_mutex);
>> > diff --git a/drivers/char/tpm/tpm-dev.h b/drivers/char/tpm/tpm-dev.h
>> > index f3742bcc73e3..700e3d9d8b64 100644
>> > --- a/drivers/char/tpm/tpm-dev.h
>> > +++ b/drivers/char/tpm/tpm-dev.h
>> > @@ -18,7 +18,7 @@ struct file_priv {
>> > 	bool response_read;
>> > 	bool command_enqueued;
>> >
>> > -	u8 data_buffer[TPM_BUFSIZE];
>> > +	u8 data_buffer[TPM_BUF_MAX_SIZE];
>> > };
>> >
>> > void tpm_common_open(struct file *file, struct tpm_chip *chip,
>> > diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
>> > index c9f173001d0e..b0d5098fb92b 100644
>> > --- a/drivers/char/tpm/tpm-interface.c
>> > +++ b/drivers/char/tpm/tpm-interface.c
>> > @@ -100,8 +100,8 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip, void *buf, size_t bufsiz)
>> > 	if (bufsiz < TPM_HEADER_SIZE)
>> > 		return -EINVAL;
>> >
>> > -	if (bufsiz > TPM_BUFSIZE)
>> > -		bufsiz = TPM_BUFSIZE;
>> > +	if (bufsiz > TPM_BUF_MAX_SIZE)
>> > +		bufsiz = TPM_BUF_MAX_SIZE;
>> >
>> > 	count = be32_to_cpu(header->length);
>> > 	ordinal = be32_to_cpu(header->ordinal);
>> > diff --git a/drivers/char/tpm/tpm-sysfs.c b/drivers/char/tpm/tpm-sysfs.c
>> > index 94231f052ea7..4213a8285ed0 100644
>> > --- a/drivers/char/tpm/tpm-sysfs.c
>> > +++ b/drivers/char/tpm/tpm-sysfs.c
>> > @@ -32,28 +32,30 @@ struct tpm_readpubek_out {
>> > static ssize_t pubek_show(struct device *dev, struct device_attribute *attr,
>> > 			  char *buf)
>> > {
>> > -	struct tpm_buf tpm_buf;
>> > 	struct tpm_readpubek_out *out;
>> > 	int i;
>> > 	char *str = buf;
>> > 	struct tpm_chip *chip = to_tpm_chip(dev);
>> > 	char anti_replay[20];
>> >
>> > +	struct tpm_buf *tpm_buf __free(kfree) = kzalloc(PAGE_SIZE, GFP_KERNEL);
>>
>> We're using PAGE_SIZE instead of TPM_BUF_MAX_SIZE to reduce the pressure on
>> the allocator, right?
>>
>> I was wondering if we could create an inline function or a macro that calls
>> kzalloc() and tpm_buf_init().
>> Just because we do it often, it's not a strong opinion, just something that
>> came to mind while doing the review.
>> I mean something like this (untested):
>>
>> struct tpm_buf *tpm_buf_alloc_max(void) {
>> 	struct tpm_buf *buf = kzalloc(PAGE_SIZE, GFP_KERNEL);
>> 	if (!buf)
>> 		return NULL;
>>
>> 	tpm_buf_init(buf, TPM_BUF_MAX_SIZE);
>> 	return buf;
>> }
>
>With or without this, about same about of boiler plate is introduced to
>the call site. It's easier to understand the code later on when kzalloc
>alloc is clearly at site. Finally I think for most uses the driver could
>favor stack over heap (but that requires prepare the tpm_buf like this).

Yep, I see.

Stefano

Powered by blists - more mailing lists