Re: [PATCH v6 5/8] tpm: Store the length of the tpm_buf data separately.

[Stefan Berger <stefanb@xxxxxxxxxxxxx>]

On 11/23/23 21:02, Jarkko Sakkinen wrote:
> TPM2B buffers, or sized buffers, have a two byte header, which contains the
> length of the payload as a 16-bit big-endian number, without counting in
> the space taken by the header. This differs from encoding in the TPM header
> where the length includes also the bytes taken by the header.
>
> Unbound the length of a tpm_buf from the value stored to the TPM command
> header. A separate encoding and decoding step so that different buffer
> types can be supported, with variant header format and length encoding.
>
> Signed-off-by: Jarkko Sakkinen <jarkko@xxxxxxxxxx>

Reviewed-by: Stefan Berger <stefanb@xxxxxxxxxxxxx>

> ---
> v3 [2023-11-21]: Removed spurious memset() (albeit not harmful). Expand
> tag invariant in tpm_buf_reset() to be allowed to be zero.
> v2 [2023-11-21]: Squashed together with the following patch, as the API
> of tpm_buf_init() is no longer changed.
> ---
>   drivers/char/tpm/tpm-buf.c                | 48 +++++++++++++++++------
>   drivers/char/tpm/tpm-interface.c          |  1 +
>   include/keys/trusted_tpm.h                |  2 -
>   include/linux/tpm.h                       |  6 +--
>   security/keys/trusted-keys/trusted_tpm1.c |  9 +++--
>   5 files changed, 46 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm-buf.c b/drivers/char/tpm/tpm-buf.c
> index 96cee41d5b9c..3f39893f3bb1 100644
> --- a/drivers/char/tpm/tpm-buf.c
> +++ b/drivers/char/tpm/tpm-buf.c
> @@ -3,25 +3,44 @@
>    * Handling of TPM command and other buffers.
>    */
>   
> +#include <linux/tpm_command.h>
>   #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;
>   
> -	buf->flags = 0;
>   	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
> + * @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)
>   {
>   	struct tpm_header *head = (struct tpm_header *)buf->data;
>   
> +	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);
>   	head->tag = cpu_to_be16(tag);
>   	head->length = cpu_to_be32(sizeof(*head));
>   	head->ordinal = cpu_to_be32(ordinal);
> @@ -34,33 +53,40 @@ void tpm_buf_destroy(struct tpm_buf *buf)
>   }
>   EXPORT_SYMBOL_GPL(tpm_buf_destroy);
>   
> +/**
> + * tpm_buf_length() - Return the number of bytes consumed by the data
> + *
> + * Return: The number of bytes consumed by the buffer
> + */
>   u32 tpm_buf_length(struct tpm_buf *buf)
>   {
> -	struct tpm_header *head = (struct tpm_header *)buf->data;
> -
> -	return be32_to_cpu(head->length);
> +	return buf->length;
>   }
>   EXPORT_SYMBOL_GPL(tpm_buf_length);
>   
> -void tpm_buf_append(struct tpm_buf *buf,
> -		    const unsigned char *new_data,
> -		    unsigned int new_len)
> +/**
> + * tpm_buf_append() - Append data to an initialized buffer
> + * @buf:	A &tpm_buf
> + * @new_data:	A data blob
> + * @new_length:	Size of the appended data
> + */
> +void tpm_buf_append(struct tpm_buf *buf, const u8 *new_data, u16 new_length)
>   {
>   	struct tpm_header *head = (struct tpm_header *)buf->data;
> -	u32 len = tpm_buf_length(buf);
>   
>   	/* Return silently if overflow has already happened. */
>   	if (buf->flags & TPM_BUF_OVERFLOW)
>   		return;
>   
> -	if ((len + new_len) > PAGE_SIZE) {
> +	if ((buf->length + new_length) > PAGE_SIZE) {
>   		WARN(1, "tpm_buf: overflow\n");
>   		buf->flags |= TPM_BUF_OVERFLOW;
>   		return;
>   	}
>   
> -	memcpy(&buf->data[len], new_data, new_len);
> -	head->length = cpu_to_be32(len + new_len);
> +	memcpy(&buf->data[buf->length], new_data, new_length);
> +	buf->length += new_length;
> +	head->length = cpu_to_be32(buf->length);
>   }
>   EXPORT_SYMBOL_GPL(tpm_buf_append);
>   
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index 163ae247bff2..ea75f2776c2f 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -232,6 +232,7 @@ ssize_t tpm_transmit_cmd(struct tpm_chip *chip, struct tpm_buf *buf,
>   	if (len < min_rsp_body_length + TPM_HEADER_SIZE)
>   		return -EFAULT;
>   
> +	buf->length = len;
>   	return 0;
>   }
>   EXPORT_SYMBOL_GPL(tpm_transmit_cmd);
> diff --git a/include/keys/trusted_tpm.h b/include/keys/trusted_tpm.h
> index 7769b726863a..a088b33fd0e3 100644
> --- a/include/keys/trusted_tpm.h
> +++ b/include/keys/trusted_tpm.h
> @@ -6,8 +6,6 @@
>   #include <linux/tpm_command.h>
>   
>   /* implementation specific TPM constants */
> -#define MAX_BUF_SIZE			1024
> -#define TPM_GETRANDOM_SIZE		14
>   #define TPM_SIZE_OFFSET			2
>   #define TPM_RETURN_OFFSET		6
>   #define TPM_DATA_OFFSET			10
> diff --git a/include/linux/tpm.h b/include/linux/tpm.h
> index 0a8c1351adc2..1d7b39b5c383 100644
> --- a/include/linux/tpm.h
> +++ b/include/linux/tpm.h
> @@ -306,7 +306,8 @@ enum tpm_buf_flags {
>    * A string buffer type for constructing TPM commands.
>    */
>   struct tpm_buf {
> -	unsigned int flags;
> +	u32 flags;
> +	u32 length;
>   	u8 *data;
>   };
>   
> @@ -329,8 +330,7 @@ int tpm_buf_init(struct tpm_buf *buf, u16 tag, u32 ordinal);
>   void tpm_buf_reset(struct tpm_buf *buf, u16 tag, u32 ordinal);
>   void tpm_buf_destroy(struct tpm_buf *buf);
>   u32 tpm_buf_length(struct tpm_buf *buf);
> -void tpm_buf_append(struct tpm_buf *buf, const unsigned char *new_data,
> -		    unsigned int new_len);
> +void tpm_buf_append(struct tpm_buf *buf, const u8 *new_data, u16 new_length);
>   void tpm_buf_append_u8(struct tpm_buf *buf, const u8 value);
>   void tpm_buf_append_u16(struct tpm_buf *buf, const u16 value);
>   void tpm_buf_append_u32(struct tpm_buf *buf, const u32 value);
> diff --git a/security/keys/trusted-keys/trusted_tpm1.c b/security/keys/trusted-keys/trusted_tpm1.c
> index 37bce84eef99..89c9798d1800 100644
> --- a/security/keys/trusted-keys/trusted_tpm1.c
> +++ b/security/keys/trusted-keys/trusted_tpm1.c
> @@ -367,6 +367,7 @@ int trusted_tpm_send(unsigned char *cmd, size_t buflen)
>   		return rc;
>   
>   	buf.flags = 0;
> +	buf.length = buflen;
>   	buf.data = cmd;
>   	dump_tpm_buf(cmd);
>   	rc = tpm_transmit_cmd(chip, &buf, 4, "sending data");
> @@ -417,7 +418,7 @@ static int osap(struct tpm_buf *tb, struct osapsess *s,
>   	tpm_buf_append_u32(tb, handle);
>   	tpm_buf_append(tb, ononce, TPM_NONCE_SIZE);
>   
> -	ret = trusted_tpm_send(tb->data, MAX_BUF_SIZE);
> +	ret = trusted_tpm_send(tb->data, tb->length);
>   	if (ret < 0)
>   		return ret;
>   
> @@ -441,7 +442,7 @@ int oiap(struct tpm_buf *tb, uint32_t *handle, unsigned char *nonce)
>   		return -ENODEV;
>   
>   	tpm_buf_reset(tb, TPM_TAG_RQU_COMMAND, TPM_ORD_OIAP);
> -	ret = trusted_tpm_send(tb->data, MAX_BUF_SIZE);
> +	ret = trusted_tpm_send(tb->data, tb->length);
>   	if (ret < 0)
>   		return ret;
>   
> @@ -553,7 +554,7 @@ static int tpm_seal(struct tpm_buf *tb, uint16_t keytype,
>   	tpm_buf_append_u8(tb, cont);
>   	tpm_buf_append(tb, td->pubauth, SHA1_DIGEST_SIZE);
>   
> -	ret = trusted_tpm_send(tb->data, MAX_BUF_SIZE);
> +	ret = trusted_tpm_send(tb->data, tb->length);
>   	if (ret < 0)
>   		goto out;
>   
> @@ -644,7 +645,7 @@ static int tpm_unseal(struct tpm_buf *tb,
>   	tpm_buf_append_u8(tb, cont);
>   	tpm_buf_append(tb, authdata2, SHA1_DIGEST_SIZE);
>   
> -	ret = trusted_tpm_send(tb->data, MAX_BUF_SIZE);
> +	ret = trusted_tpm_send(tb->data, tb->length);
>   	if (ret < 0) {
>   		pr_info("authhmac failed (%d)\n", ret);
>   		return ret;