[PATCH v4] tpm: Managed allocations for tpm_buf instances
Stefan Berger
stefanb at linux.ibm.com
Tue Jul 1 21:42:47 UTC 2025
On 7/1/25 10:51 AM, Jarkko Sakkinen wrote:
> From: Jarkko Sakkinen <jarkko.sakkinen at opinsys.com>
>
> Repeal and replace tpm_buf_init() and tpm_buf_init_sized() with
> tpm_buf_alloc(), which returns a buffer of memory with the struct tpm_buf
> header at the beginning of the returned buffer. This leaves 4090 bytes of
> free space for the payload.
>
> Given that kfree() is now the destructor for struct tpm_buf instances,
> tpm_buf_destroy() becomes obsolete, and can be safely wiped of too.
s/of/off/ or s/wiped of/remove,/
>
> The actual gist is that now a tpm_buf can be now declared using
s/that now a/that a/
> __free(kfree) declared in linux/slab.h:
>
> struct tpm_buf *buf __free(kfree) = NULL;
>
> /* ... */
>
> buf = tpm_buf_alloc();
>
> Doing this has two-folded benefits:
>
> 1. Yet to be discoverd memory leaks in the pre-existing code base.
-> discovered
A couple of nits below and one stray 'return rc;' that should not be
there...
> 2. Memory leaks concerning new features and other contributions.
>
> In addition, the barrier to contribute is lowered given that managing
> memory is a factor easier.
>
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen at opinsys.com>
> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
> index 524d802ede26..86b961f4027b 100644
> --- a/drivers/char/tpm/tpm2-cmd.c
> +++ b/drivers/char/tpm/tpm2-cmd.c
> @@ -165,14 +165,18 @@ struct tpm2_pcr_read_out {
> int tpm2_pcr_read(struct tpm_chip *chip, u32 pcr_idx,
> struct tpm_digest *digest, u16 *digest_size_ptr)
> {
> + struct tpm_buf *buf __free(kfree) = NULL;
> int i;
> int rc;
> - struct tpm_buf buf;
> struct tpm2_pcr_read_out *out;
> u8 pcr_select[TPM2_PCR_SELECT_MIN] = {0};
> u16 digest_size;
> u16 expected_digest_size = 0;
>
> + buf = tpm_buf_alloc();
> + if (!buf)
> + return -ENOMEM;
> +
> if (pcr_idx >= TPM2_PLATFORM_PCR)
> return -EINVAL;
>
> @@ -187,23 +191,21 @@ int tpm2_pcr_read(struct tpm_chip *chip, u32 pcr_idx,
> expected_digest_size = chip->allocated_banks[i].digest_size;
> }
>
> - rc = tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS, TPM2_CC_PCR_READ);
> - if (rc)
> - return rc;
> + tpm_buf_reset(buf, TPM2_ST_NO_SESSIONS, TPM2_CC_PCR_READ);
>
> pcr_select[pcr_idx >> 3] = 1 << (pcr_idx & 0x7);
>
> - tpm_buf_append_u32(&buf, 1);
> - tpm_buf_append_u16(&buf, digest->alg_id);
> - tpm_buf_append_u8(&buf, TPM2_PCR_SELECT_MIN);
> - tpm_buf_append(&buf, (const unsigned char *)pcr_select,
> + tpm_buf_append_u32(buf, 1);
> + tpm_buf_append_u16(buf, digest->alg_id);
> + tpm_buf_append_u8(buf, TPM2_PCR_SELECT_MIN);
> + tpm_buf_append(buf, (const unsigned char *)pcr_select,
> sizeof(pcr_select));
>
> - rc = tpm_transmit_cmd(chip, &buf, 0, "attempting to read a pcr value");
> + rc = tpm_transmit_cmd(chip, buf, 0, "attempting to read a pcr value");
> if (rc)
> goto out;
nit: -> return rc; ?
>
> - out = (struct tpm2_pcr_read_out *)&buf.data[TPM_HEADER_SIZE];
> + out = (struct tpm2_pcr_read_out *)&buf->data[TPM_HEADER_SIZE];
> digest_size = be16_to_cpu(out->digest_size);
> if (digest_size > sizeof(digest->digest) ||
> (!digest_size_ptr && digest_size != expected_digest_size)) {
> @@ -216,7 +218,6 @@ int tpm2_pcr_read(struct tpm_chip *chip, u32 pcr_idx,
>
> memcpy(digest->digest, out->digest, digest_size);
> out:
probably can remove this label
> - tpm_buf_destroy(&buf);
> return rc;
> }
>
> @@ -574,8 +569,8 @@ struct tpm2_pcr_selection {
>
> ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
> {
> + struct tpm_buf *buf __free(kfree) = NULL;
> struct tpm2_pcr_selection pcr_selection;
> - struct tpm_buf buf;
> void *marker;
> void *end;
> void *pcr_select_offset;
> @@ -587,41 +582,39 @@ ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
> int rc;
> int i = 0;
>
> - rc = tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS, TPM2_CC_GET_CAPABILITY);
> - if (rc)
> - return rc;
> + buf = tpm_buf_alloc();
> + if (!buf)
> + return -ENOMEM;
>
> - tpm_buf_append_u32(&buf, TPM2_CAP_PCRS);
> - tpm_buf_append_u32(&buf, 0);
> - tpm_buf_append_u32(&buf, 1);
> + tpm_buf_reset(buf, TPM2_ST_NO_SESSIONS, TPM2_CC_GET_CAPABILITY);
> + tpm_buf_append_u32(buf, TPM2_CAP_PCRS);
> + tpm_buf_append_u32(buf, 0);
> + tpm_buf_append_u32(buf, 1);
>
> - rc = tpm_transmit_cmd(chip, &buf, 9, "get tpm pcr allocation");
> + rc = tpm_transmit_cmd(chip, buf, 9, "get tpm pcr allocation");
> if (rc)
> - goto out;
> + return rc;
>
> nr_possible_banks = be32_to_cpup(
> - (__be32 *)&buf.data[TPM_HEADER_SIZE + 5]);
> + (__be32 *)&buf->data[TPM_HEADER_SIZE + 5]);
>
> chip->allocated_banks = kcalloc(nr_possible_banks,
> sizeof(*chip->allocated_banks),
> GFP_KERNEL);
> - if (!chip->allocated_banks) {
> - rc = -ENOMEM;
> - goto out;
> - }
> + if (!chip->allocated_banks)
> + return -ENOMEM;
>
> - marker = &buf.data[TPM_HEADER_SIZE + 9];
> + marker = &buf->data[TPM_HEADER_SIZE + 9];
>
> - rsp_len = be32_to_cpup((__be32 *)&buf.data[2]);
> - end = &buf.data[rsp_len];
> + rsp_len = be32_to_cpup((__be32 *)&buf->data[2]);
> + end = &buf->data[rsp_len];
>
> + return rc;
this doesn't look right...
> for (i = 0; i < nr_possible_banks; i++) {
> pcr_select_offset = marker +
> offsetof(struct tpm2_pcr_selection, size_of_select);
> - if (pcr_select_offset >= end) {
> - rc = -EFAULT;
> - break;
> - }
> + if (pcr_select_offset >= end)
> + return -EFAULT;
>
> memcpy(&pcr_selection, marker, sizeof(pcr_selection));
> hash_alg = be16_to_cpu(pcr_selection.hash_alg);
> @@ -633,7 +626,7 @@ ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
> diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c
> index 024be262702f..54bcd8d0621e 100644
> --- a/security/keys/trusted-keys/trusted_tpm2.c
> +++ b/security/keys/trusted-keys/trusted_tpm2.c
> @@ -241,14 +241,23 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
> struct trusted_key_payload *payload,
> struct trusted_key_options *options)
> {
> + struct tpm_buf *buf __free(kfree) = NULL;
> + struct tpm_buf *sized __free(kfree) = NULL;
Revert order of the above two lines.
> off_t offset = TPM_HEADER_SIZE;
> - struct tpm_buf buf, sized;
> int blob_len = 0;
> u32 hash;
> u32 flags;
> int i;
> int rc;
>
> + buf = tpm_buf_alloc();
> + if (!buf)
> + return -ENOMEM;
> +
> + sized = tpm_buf_alloc();
> + if (!sized)
> + return -ENOMEM;
> +
> for (i = 0; i < ARRAY_SIZE(tpm2_hash_map); i++) {> if
(options->hash == tpm2_hash_map[i].crypto_id) {
> hash = tpm2_hash_map[i].tpm_id;
> @@ -270,89 +279,76 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
> if (rc)
> goto out_put;
>
> - rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_CREATE);
> - if (rc) {
> - tpm2_end_auth_session(chip);
> - goto out_put;
> - }
> -
> - rc = tpm_buf_init_sized(&sized);
> - if (rc) {
> - tpm_buf_destroy(&buf);
> - tpm2_end_auth_session(chip);
> - goto out_put;
> - }
> -
> - tpm_buf_append_name(chip, &buf, options->keyhandle, NULL);
> - tpm_buf_append_hmac_session(chip, &buf, TPM2_SA_DECRYPT,
> + tpm_buf_reset(buf, TPM2_ST_SESSIONS, TPM2_CC_CREATE);
> + tpm_buf_reset_sized(sized);
> + tpm_buf_append_name(chip, buf, options->keyhandle, NULL);
> + tpm_buf_append_hmac_session(chip, buf, TPM2_SA_DECRYPT,
> options->keyauth, TPM_DIGEST_SIZE);
>
> /* sensitive */
> - tpm_buf_append_u16(&sized, options->blobauth_len);
> + tpm_buf_append_u16(sized, options->blobauth_len);
>
> if (options->blobauth_len)
> - tpm_buf_append(&sized, options->blobauth, options->blobauth_len);
> + tpm_buf_append(sized, options->blobauth, options->blobauth_len);
>
> - tpm_buf_append_u16(&sized, payload->key_len);
> - tpm_buf_append(&sized, payload->key, payload->key_len);
> - tpm_buf_append(&buf, sized.data, sized.length);
> + tpm_buf_append_u16(sized, payload->key_len);
> + tpm_buf_append(sized, payload->key, payload->key_len);
> + tpm_buf_append(buf, sized->data, sized->length);
>
> /* public */
> - tpm_buf_reset_sized(&sized);
> - tpm_buf_append_u16(&sized, TPM_ALG_KEYEDHASH);
> - tpm_buf_append_u16(&sized, hash);
> + tpm_buf_reset_sized(sized);
> + tpm_buf_append_u16(sized, TPM_ALG_KEYEDHASH);
> + tpm_buf_append_u16(sized, hash);
>
> /* key properties */
> flags = 0;
> flags |= options->policydigest_len ? 0 : TPM2_OA_USER_WITH_AUTH;
> flags |= payload->migratable ? 0 : (TPM2_OA_FIXED_TPM | TPM2_OA_FIXED_PARENT);
> - tpm_buf_append_u32(&sized, flags);
> + tpm_buf_append_u32(sized, flags);
>
> /* policy */
> - tpm_buf_append_u16(&sized, options->policydigest_len);
> + tpm_buf_append_u16(sized, options->policydigest_len);
> if (options->policydigest_len)
> - tpm_buf_append(&sized, options->policydigest, options->policydigest_len);
> + tpm_buf_append(sized, options->policydigest, options->policydigest_len);
>
> /* public parameters */
> - tpm_buf_append_u16(&sized, TPM_ALG_NULL);
> - tpm_buf_append_u16(&sized, 0);
> + tpm_buf_append_u16(sized, TPM_ALG_NULL);
> + tpm_buf_append_u16(sized, 0);
>
> - tpm_buf_append(&buf, sized.data, sized.length);
> + tpm_buf_append(buf, sized->data, sized->length);
>
> /* outside info */
> - tpm_buf_append_u16(&buf, 0);
> + tpm_buf_append_u16(buf, 0);
>
> /* creation PCR */
> - tpm_buf_append_u32(&buf, 0);
> + tpm_buf_append_u32(buf, 0);
>
> - if (buf.flags & TPM_BUF_OVERFLOW) {
> + if (buf->flags & TPM_BUF_OVERFLOW) {
> rc = -E2BIG;
> tpm2_end_auth_session(chip);
> goto out;
> }
>
> - tpm_buf_fill_hmac_session(chip, &buf);
> - rc = tpm_transmit_cmd(chip, &buf, 4, "sealing data");
> - rc = tpm_buf_check_hmac_response(chip, &buf, rc);
> + tpm_buf_fill_hmac_session(chip, buf);
> + rc = tpm_transmit_cmd(chip, buf, 4, "sealing data");
> + rc = tpm_buf_check_hmac_response(chip, buf, rc);
> if (rc)
> goto out;
>
> - blob_len = tpm_buf_read_u32(&buf, &offset);
> - if (blob_len > MAX_BLOB_SIZE || buf.flags & TPM_BUF_BOUNDARY_ERROR) {
> + blob_len = tpm_buf_read_u32(buf, &offset);
> + if (blob_len > MAX_BLOB_SIZE || buf->flags & TPM_BUF_BOUNDARY_ERROR) {
> rc = -E2BIG;
> goto out;
> }
> - if (buf.length - offset < blob_len) {
> + if (buf->length - offset < blob_len) {
> rc = -EFAULT;
> goto out;
> }
>
> - blob_len = tpm2_key_encode(payload, options, &buf.data[offset], blob_len);
> + blob_len = tpm2_key_encode(payload, options, &buf->data[offset],
> + blob_len);
>
> out:
> - tpm_buf_destroy(&sized);
> - tpm_buf_destroy(&buf);
> -
> if (rc > 0) {
> if (tpm2_rc_value(rc) == TPM2_RC_HASH)
> rc = -EINVAL;
More information about the Linux-security-module-archive
mailing list