On Fri, Sep 19, 2025 at 06:32:43PM +0300, Jarkko Sakkinen wrote:
On Fri, Sep 19, 2025 at 03:35:43PM +0200, Stefano Garzarella wrote:
On Fri, Sep 19, 2025 at 02:24:47PM +0300, Jarkko Sakkinen wrote:
> From: Jarkko Sakkinen <jarkko.sakki...@opinsys.com>
>
> Drop 'tpm_buf_init', 'tpm_buf_init_sized' and 'tpm_buf_free'. Refine
> 'struct tpm_buf' to hold capacity in order to enable stack allocation and
> sizes other than page size.
>
> The updated 'struct tpm_buf' can be allocated either from stack or heap.
>
> The contract is the following:
>
> 1. 'tpm_buf_reset' and 'tpm_buf_reset_size' expect that on the first run
> the passed buffer is zeroed by the caller (e.g. via memset or kzalloc).
> 2. The same buffer can be reused. On the second and subsequent resets the
> aforementioned functions verify that 'buf_size' has the same value, and
> emits warning if not.
>
> As a consequence 'struct tpm_buf' instance can be easily wrapped into
> managed allocation:
>
> struct tpm_buf *buf __free(kfree) buf = kzalloc(PAGE_SIZE,
> GFP_KERNEL);
>
> Reviewed-by: Stefan Berger <stef...@linux.ibm.com>
> Signed-off-by: Jarkko Sakkinen <jarkko.sakki...@opinsys.com>
> ---
> 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-986dc90d7...@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 | 68 ++----
> drivers/char/tpm/tpm-sysfs.c | 19 +-
> drivers/char/tpm/tpm1-cmd.c | 143 ++++++------
> drivers/char/tpm/tpm2-cmd.c | 270 ++++++++++------------
> drivers/char/tpm/tpm2-sessions.c | 118 +++++-----
> drivers/char/tpm/tpm2-space.c | 42 ++--
> drivers/char/tpm/tpm_vtpm_proxy.c | 29 +--
> include/linux/tpm.h | 17 +-
> security/keys/trusted-keys/trusted_tpm1.c | 40 ++--
> security/keys/trusted-keys/trusted_tpm2.c | 153 ++++++------
> 10 files changed, 390 insertions(+), 509 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm-buf.c b/drivers/char/tpm/tpm-buf.c
> index dc882fc9fa9e..19774bc5786f 100644
> --- a/drivers/char/tpm/tpm-buf.c
> +++ b/drivers/char/tpm/tpm-buf.c
> @@ -7,83 +7,57 @@
> #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)
> -{
> - buf->data = (u8 *)__get_free_page(GFP_KERNEL);
> - if (!buf->data)
> - return -ENOMEM;
> -
> - tpm_buf_reset(buf, tag, ordinal);
> - return 0;
> -}
> -EXPORT_SYMBOL_GPL(tpm_buf_init);
> -
> /**
> * tpm_buf_reset() - Initialize a TPM command
> * @buf: A &tpm_buf
> + * @buf_size: Size of the buffer.
> * @tag: TPM_TAG_RQU_COMMAND, TPM2_ST_NO_SESSIONS or TPM2_ST_SESSIONS
> * @ordinal: A command ordinal
> + *
> + * 1. Expects that on the first run the passed buffer is zeroed by the
caller.
> + * 2. Old buffer can be reused. On the second and subsequent resets
@buf_size is
> + * verified to be equal to the previous value.
> */
> -void tpm_buf_reset(struct tpm_buf *buf, u16 tag, u32 ordinal)
> +void tpm_buf_reset(struct tpm_buf *buf, u16 buf_size, u16 tag, u32 ordinal)
> {
> struct tpm_header *head = (struct tpm_header *)buf->data;
>
> + WARN_ON(buf->capacity != 0 && buf_size != (buf->capacity + sizeof(*buf)));
> 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);
> - buf->handles = 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
> - */
> -int tpm_buf_init_sized(struct tpm_buf *buf)
> -{
> - buf->data = (u8 *)__get_free_page(GFP_KERNEL);
> - if (!buf->data)
> - return -ENOMEM;
> -
> - tpm_buf_reset_sized(buf);
> - return 0;
> -}
> -EXPORT_SYMBOL_GPL(tpm_buf_init_sized);
> -
> /**
> * tpm_buf_reset_sized() - Initialize a sized buffer
> * @buf: A &tpm_buf
> + * @buf_size: Size of the buffer.
> + *
> + * 1. Expects that on the first run the passed buffer is zeroed by the
caller.
> + * 2. Old buffer can be reused. On the second and subsequent resets
@buf_size is
> + * verified to be equal to the previous value.
> */
> -void tpm_buf_reset_sized(struct tpm_buf *buf)
> +void tpm_buf_reset_sized(struct tpm_buf *buf, u16 buf_size)
> {
> + WARN_ON(buf->capacity != 0 && buf_size != (buf->capacity + sizeof(*buf)));
> +
> 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_sized);
>
> -void tpm_buf_destroy(struct tpm_buf *buf)
> -{
> - free_page((unsigned long)buf->data);
> -}
> -EXPORT_SYMBOL_GPL(tpm_buf_destroy);
> -
> /**
> * tpm_buf_length() - Return the number of bytes consumed by the data
> * @buf: A &tpm_buf
*
* Return: The number of bytes consumed by the buffer
*/
u32 tpm_buf_length(struct tpm_buf *buf)
{
return buf->length;
}
EXPORT_SYMBOL_GPL(tpm_buf_length);
Should we update the return type (u16) on this function?
Yes we should. Thanks for catching this.
> @@ -108,7 +82,7 @@ void tpm_buf_append(struct tpm_buf *buf, const u8
*new_data, u16 new_length)
> if (buf->flags & TPM_BUF_OVERFLOW)
> return;
>
> - if ((buf->length + new_length) > PAGE_SIZE) {
> + if ((buf->length + new_length) > buf->capacity) {
IIUC all of these are u16, so there could be an overflow when we do
`buf->length + new_length` ?
Should we cast to u32 or just change the expression in something like
`new_length > (buf->capacity - buf->length)`
Thanks, these really appropriate comments.
I think best measure here would have hard constant cap of PAGE_SIZE
for buf_size. That's how the specs limit anyhow. I'll extend the
invariant.
Oh I see, that makes sense, thanks :-)
Stefano
