Re: [PATCH v4] tpm: Managed allocations for tpm_buf instances

[Jarkko Sakkinen <jarkko@kernel.org>]

On Tue, Jul 01, 2025 at 05:42:47PM -0400, Stefan Berger wrote:
> 
> 
> On 7/1/25 10:51 AM, Jarkko Sakkinen wrote:
> > From: Jarkko Sakkinen <jarkko.sakkinen@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@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

yup, i agree

> 
> > -	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...

thanks for catching this!

> 
> 
> >   	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;


Thanks for the remarks.

BR, Jarkko