Re: [PATCH v3] keys/trusted/tpm2: Validate TPM2_Create object sizes separately

[Srish Srinivasan <ssrish@linux.ibm.com>]

On 9/30/26 2:45 AM, Jarkko Sakkinen wrote:
> On Sun, Sep 27, 2026 at 12:12:03AM +0530, Srish Srinivasan wrote:
>> TPM2_Create returns outPrivate, outPublic, creationData, creationHash and
>> creationTicket in its response parameter area. However, only outPrivate and
>> outPublic are included in the trusted key blob. The size of the blob is
>> therefore not determined by the size of the complete response parameter
>> area.
>>
>> tpm2_seal_trusted() currently compares the size of the complete response
>> parameter area against MAX_BLOB_SIZE. This can reject a valid response
>> when the remaining response outputs cause the entire response parameter
>> area to exceed MAX_BLOB_SIZE, even though the outPrivate and outPublic
>> TPM2B structures consumed by tpm2_key_encode() remain small enough to be
>> encoded in the key blob.
>>
>> This is observed when creating larger trusted keys using the swtpm TPM 2.0
>> emulator backed by libtpms.
>>
>> For example, requesting a 113-byte key succeeds, 114 fails.
>>
>> ~$ keyctl add trusted trusted_key1 "new 113 keyhandle=0x81000001" @u
>> 520504613
>> ~$ keyctl add trusted trusted_key2 "new 114 keyhandle=0x81000001" @u
>> add_key: Argument list too long
>> ~$
>>
>> Remove the MAX_BLOB_SIZE check on the complete response parameter area.
>> Instead, use the response length passed to tpm2_key_encode() to validate
>> that the outPrivate and outPublic TPM2B structures are fully contained
>> in the response before accessing them.
>>
>> Previously, a response parameter area larger than MAX_BLOB_SIZE was
>> rejected with -E2BIG before ASN.1 encoding. With this change, if the
>> resulting encoded blob does not fit in payload->blob, the error returned by
>> asn1_encode_sequence() is propagated instead.
>>
>> Also, use scope-based cleanup to simplify resource management.
>>
>> Signed-off-by: Srish Srinivasan <ssrish@linux.ibm.com>
>> ---
>> Changelog:
>>
>>   v3:
>>    - Simplify the TPM2B_PRIVATE and TPM2B_PUBLIC bounds checks
>>    - Use appropriate error codes for failure returns
>>    - Use scope-based cleanup to simplify resource management
>>
>>   v2:
>>    - Exclude a comment pointed out by Jarkko
>>
>>   security/keys/trusted-keys/trusted_tpm2.c | 42 +++++++++++++----------
>>   1 file changed, 24 insertions(+), 18 deletions(-)
>>
>> diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c
>> index 906700c3d7f0..920f45c7864b 100644
>> --- a/security/keys/trusted-keys/trusted_tpm2.c
>> +++ b/security/keys/trusted-keys/trusted_tpm2.c
>> @@ -25,24 +25,36 @@ static int tpm2_key_encode(struct trusted_key_payload *payload,
>>   {
>>   	struct trusted_key_tpm *private = options->private;
>>   	const int SCRATCH_SIZE = PAGE_SIZE;
>> -	u8 *scratch = kmalloc(SCRATCH_SIZE, GFP_KERNEL);
>> -	u8 *work = scratch, *work1;
>> -	u8 *end_work = scratch + SCRATCH_SIZE;
>> +	u8 *scratch __free(kfree) = NULL;
>> +	u8 *work, *work1;
>> +	u8 *end_work;
>>   	u8 *priv, *pub;
>> -	u16 priv_len, pub_len;
>> +	u32 priv_len, pub_len;
>>   	int ret;
>>   
>> +	if (len < 4)
>> +		return -EINVAL;
>> +
>>   	priv_len = get_unaligned_be16(src) + 2;
>> -	priv = src;
>> +	if (priv_len + 2 > len)
>> +		return -EIO;
>>   
>> +	priv = src;
>>   	src += priv_len;
>>   
>>   	pub_len = get_unaligned_be16(src) + 2;
>> +	if (pub_len + priv_len > len)
>> +		return -EIO;
>> +
>>   	pub = src;
>>   
>> +	scratch = kmalloc(SCRATCH_SIZE, GFP_KERNEL);
>>   	if (!scratch)
>>   		return -ENOMEM;
>>   
>> +	work = scratch;
>> +	end_work = scratch + SCRATCH_SIZE;
>> +
>>   	work = asn1_encode_oid(work, end_work, tpm2key_oid,
>>   			       asn1_oid_len(tpm2key_oid));
>>   
>> @@ -50,10 +62,9 @@ static int tpm2_key_encode(struct trusted_key_payload *payload,
>>   		unsigned char bool[3], *w = bool;
>>   		/* tag 0 is emptyAuth */
>>   		w = asn1_encode_boolean(w, w + sizeof(bool), true);
>> -		if (WARN(IS_ERR(w), "BUG: Boolean failed to encode")) {
>> -			ret = PTR_ERR(w);
>> -			goto err;
>> -		}
>> +		if (WARN(IS_ERR(w), "BUG: Boolean failed to encode"))
>> +			return PTR_ERR(w);
>> +
>>   		work = asn1_encode_tag(work, end_work, 0, bool, w - bool);
>>   	}
>>   
>> @@ -65,8 +76,7 @@ static int tpm2_key_encode(struct trusted_key_payload *payload,
>>   	 */
>>   	if (WARN(work - scratch + pub_len + priv_len + 14 > SCRATCH_SIZE,
>>   		 "BUG: scratch buffer is too small")) {
>> -		ret = -EINVAL;
>> -		goto err;
>> +		return -EINVAL;
>>   	}
>>   
>>   	work = asn1_encode_integer(work, end_work, private->keyhandle);
>> @@ -79,15 +89,10 @@ static int tpm2_key_encode(struct trusted_key_payload *payload,
>>   	if (IS_ERR(work1)) {
>>   		ret = PTR_ERR(work1);
>>   		pr_err("BUG: ASN.1 encoder failed with %d\n", ret);
>> -		goto err;
>> +		return ret;
>>   	}
>>   
>> -	kfree(scratch);
>>   	return work1 - payload->blob;
>> -
>> -err:
>> -	kfree(scratch);
>> -	return ret;
>>   }
>>   
>>   struct tpm2_key_context {
>> @@ -340,10 +345,11 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
>>   		goto out;
>>   
>>   	blob_len = tpm_buf_read_u32(buf, &offset);
>> -	if (blob_len > MAX_BLOB_SIZE || buf->flags & TPM_BUF_INVALID) {
>> +	if (buf->flags & TPM_BUF_INVALID) {
>>   		rc = -E2BIG;
>>   		goto out;
>>   	}
>> +
>>   	if (buf->length - offset < blob_len) {
>>   		rc = -EFAULT;
>>   		goto out;
>> -- 
>> 2.53.0
>>
> Now we can say that it leaves the tree to cleaner state than it was
> before applying this patch. The first version, despite doing the right
> thing was simply too convoluted.
>
> Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>


Thanks for the review and for helping improve the patch, Jarkko.


> Br, Jarkko

Thanks,
Srish.