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 <[email protected]> > > > --- > > > 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 <[email protected]> > > > Thanks for the review and for helping improve the patch, Jarkko.
sure, np :-) Br, Jarkko
