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

[Jarkko Sakkinen <jarkko@kernel.org>]

On Wed, Sep 30, 2026 at 11:22:02AM +0530, Srish Srinivasan wrote:
> 
> 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.

sure, np :-)

Br, Jarkko