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

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

On 9/26/26 4:14 AM, Jarkko Sakkinen wrote:
> On Fri, Sep 25, 2026 at 08:42:09PM +0530, Srish Srinivasan wrote:
>> 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.
> Yeah, so this is a patch that needs a few iterations I'm not against
> the change itself per se :-)
>
> I think it would make sense to declare "u8 *scratch __free(kfree)"
> while at it so that rollback does not complicate this function
> further.

Hi Jarkko,

Sounds good. I will make this change, and incorporate the other changes 
discussed,
and post a v3 for further review.


>
>>>
>>> BR, Jarkko
>>
>> Thanks,
>> Srish.
>>
> Br, Jarkko

Thanks,
Srish.