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

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

Hi Jarkko,
Thanks for taking a look.

On 9/25/26 4:51 PM, Jarkko Sakkinen wrote:
> On Fri, Sep 18, 2026 at 12:18:25PM +0530, Srish Srinivasan wrote:
>> On 9/18/26 7:40 AM, Jarkko Sakkinen wrote:
>>> On Thu, Sep 10, 2026 at 03:32:22PM +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 an 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.
>>>>
>>>> Signed-off-by: Srish Srinivasan <ssrish@linux.ibm.com>
>>>> ---
>>>> Changelog:
>>>>
>>>>    v2:
>>>>     - Exclude a comment pointed out by Jarkko
>>>>
>>>>    security/keys/trusted-keys/trusted_tpm2.c | 35 +++++++++++++++++------
>>>>    1 file changed, 27 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c
>>>> index 01f18bb37047..cbec4f591952 100644
>>>> --- a/security/keys/trusted-keys/trusted_tpm2.c
>>>> +++ b/security/keys/trusted-keys/trusted_tpm2.c
>>>> @@ -24,24 +24,42 @@ static int tpm2_key_encode(struct trusted_key_payload *payload,
>>>>    			   u8 *src, u32 len)
>>>>    {
>>>>    	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;
>>>> +	u8 *work, *work1;
>>>> +	u8 *end_work;
>>>>    	u8 *priv, *pub;
>>>> -	u16 priv_len, pub_len;
>>>> +	u32 priv_len, pub_len;
>>>>    	int ret;
>>>> -	priv_len = get_unaligned_be16(src) + 2;
>>>> +	if (len < sizeof(__be16))
>>>> +		return -EFAULT;
>>>> +
>>>> +	priv_len = get_unaligned_be16(src);
>>>> +	if (priv_len > len - sizeof(__be16))
>>>> +		return -EFAULT;
>>>> +
>>>> +	priv_len += sizeof(__be16);
>>>>    	priv = src;
>>>> +	if (len - priv_len < sizeof(__be16))
>>>> +		return -EFAULT;
>>> This caused for me some head scrathing tbh.
>>>
>>> See:
>>>
>>> 1. len >= 2
>>> 2. priv_len <= len - 2
>>> 3. 2 - priv_len + priv_len <= 2 - priv_len + len - 2
>>> 4. 2 <= len - priv_len
>>>
>>> Is this check required?
>>
>> Hi Jarkko,
>> Thanks for taking a look.
>>
>> The preceding check,
>>
>>
>>      if (priv_len > len - sizeof(__be16))
>>          return -EFAULT;
>>
>>
>> ensures that the complete TPM2B_PRIVATE, including its size field,
>> fits within len. After this check, priv_len is incremented by 2 and
>> therefore represents the complete TPM2B_PRIVATE length.
>>
>>
>> The check you pointed out,
>>
>>
>>      if (len - priv_len < sizeof(__be16))
>>          return -EFAULT;
>>
>>
>> then ensures that at least 2 bytes remain for reading the following
>> TPM2B_PUBLIC size field.
> OK, maybe I got something wrong in my deduction but I'm not
> still entirely sure whether this is leanest way to do these
> checks.


Sure, I will simplify this. Would the following be better?


     if (len < 4)
       return -EINVAL;

     priv_len = get_unaligned_be16(src) + 2;
     if (priv_len + 2 > len)
       return -EIO;

     priv = src;
     src += priv_len;

     pub_len = get_unaligned_be16(src) + 2;
     if (priv_len + pub_len > len)
       return -EIO;

     pub = src;


The first check ensures that len can contain both TPM2B size fields.
The second verifies that the complete TPM2B_PRIVATE leaves room for the
TPM2B_PUBLIC size field. The final check ensures that both the complete
TPM2B_PRIVATE and TPM2B_PUBLIC structures fit within len.

>
> I'll iterate some other remarks so that this can move forward.
>
> I was not either entirely sure why this was done:
>
> -	u16 priv_len, pub_len;
> +	u32 priv_len, pub_len;


Yes, so the change from u16 to u32 was because the values stored in 
priv_len and
pub_len include both the 16-bit value read from the response and the 
two-byte TPM2B
size field.


>
> Why wrong len causes -EFAULT and not -EINVAL? EFAULT is for
> memory failures. Probably two that follow should return -EIO.


Agreed, I will fix this.


>
> One more that came to mind:
>
> -	priv_len = get_unaligned_be16(src) + 2;
> +	if (len < sizeof(__be16))
> +		return -EFAULT;
> +
> +	priv_len = get_unaligned_be16(src);
> +	if (priv_len > len - sizeof(__be16))
> +		return -EFAULT;
> +
> +	priv_len += sizeof(__be16);
>
> Removing the line above is not necessary and neither is the last
> the last statement if you just change your check accordingly.


Yes, that's right. And I have made this change in the diff
that I have pasted above.

Please let me know if this looks better now.

And, thanks for the feedback.

>
>
> BR, Jarkko


Thanks,
Srish.