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

[Jarkko Sakkinen <jarkko@kernel.org>]

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.

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

Br, Jarkko