On Fri, Nov 11, 2022 at 03:16:29PM -0800, Evan Green wrote:
> diff --git a/security/keys/trusted-keys/tpm2key.asn1 b/security/keys/trusted-keys/tpm2key.asn1
> index f57f869ad60068..608f8d9ca95fa8 100644
> --- a/security/keys/trusted-keys/tpm2key.asn1
> +++ b/security/keys/trusted-keys/tpm2key.asn1
> @@ -7,5 +7,18 @@ TPMKey ::= SEQUENCE {
> emptyAuth [0] EXPLICIT BOOLEAN OPTIONAL,
> parent INTEGER ({tpm2_key_parent}),
> pubkey OCTET STRING ({tpm2_key_pub}),
> - privkey OCTET STRING ({tpm2_key_priv})
> + privkey OCTET STRING ({tpm2_key_priv}),
> + ---
> + --- A TPM2B_CREATION_DATA struct as returned from the TPM2_Create command.
> + ---
> + creationData [1] EXPLICIT OCTET STRING OPTIONAL ({tpm2_key_creation_data}),
> + ---
> + --- A TPM2B_DIGEST of the creationHash as returned from the TPM2_Create
> + --- command.
> + ---
> + creationHash [2] EXPLICIT OCTET STRING OPTIONAL ({tpm2_key_creation_hash}),
> + ---
> + --- A TPMT_TK_CREATION ticket as returned from the TPM2_Create command.
> + ---
> + creationTk [3] EXPLICIT OCTET STRING OPTIONAL ({tpm2_key_creation_tk})
> }
The commit that added this file claimed:
"The benefit of the ASN.1 format is that it's a standard and thus the
exported key can be used by userspace tools (openssl_tpm2_engine,
openconnect and tpm2-tss-engine"
Are these new fields in compliance with whatever standard that was referring to?
Or was that just referring to ASN.1 itself?
> +/* Helper function to advance past a __be16 length + buffer safely */
> +static const u8 *get_sized_section(const u8 *src, const u8 *end, u16 *len)
> +{
> + u32 length;
> +
> + if (src + sizeof(u16) > end)
> + return NULL;
'end - src < sizeof(u16)', so the pointer isn't advanced past the end.
> +
> + /* Include the size field in the returned section length. */
> + length = get_unaligned_be16(src) + sizeof(u16);
> + *len = length;
> + if (*len != length)
> + return NULL;
> +
> + src += *len;
> + if (src > end)
> + return NULL;
> +
> + return src;
Similarly:
if (end - src < *len)
return NULL;
return src + *len;
> + /*
> + * The creation ticket (TPMT_TK_CREATION) consists of a 2 byte
> + * tag, 4 byte handle, and then a TPM2B_DIGEST, which is a 2
> + * byte length followed by data.
> + */
> + if (src + 8 > end)
end - src < 8
And actually it really should be 6 instead of 8, to match the code below.
get_sized_section() already validates that there are at least 2 more bytes.
> + return -EINVAL;
> +
> + creation_tk = src;
> + src = get_sized_section(src + 6, end, &creation_tk_len);
> + if (!src)
> + return -EINVAL;
> +
> + creation_tk_len += 6;
> +
> + } else {
> + creation_data_len = 0;
> + creation_data = NULL;
> + }
>
> if (!scratch)
> return -ENOMEM;
> @@ -63,26 +125,81 @@ static int tpm2_key_encode(struct trusted_key_payload *payload,
> }
>
> /*
> - * Assume both octet strings will encode to a 2 byte definite length
> + * Assume each octet string will encode to a 2 byte definite length.
> + * Each optional octet string consumes one extra byte.
> *
> - * Note: For a well behaved TPM, this warning should never
> - * trigger, so if it does there's something nefarious going on
> + * Note: For a well behaved TPM, this warning should never trigger, so
> + * if it does there's something nefarious going on
> */
> - if (WARN(work - scratch + pub_len + priv_len + 14 > SCRATCH_SIZE,
> - "BUG: scratch buffer is too small"))
> - return -EINVAL;
> + if (WARN(work - scratch + pub_len + priv_len + creation_data_len +
> + creation_hash_len + creation_tk_len + (7 * 5) + 3 >
> + SCRATCH_SIZE,
> + "BUG: scratch buffer is too small")) {
> + rc = -EINVAL;
> + goto err;
> + }
This appears to be fixing a memory leak in the error case.
The same memory leak also still appears above in:
if (WARN(IS_ERR(w), "BUG: Boolean failed to encode"))
return PTR_ERR(w);
Maybe both should be fixed in a separate patch.
> + work2 = asn1_encode_octet_string(scratch2,
> + end_work2,
> + creation_data,
> + creation_data_len);
> +
> + work = asn1_encode_tag(work,
> + end_work,
> + 1,
> + scratch2,
> + work2 - scratch2);
There's no helper function to do these two steps together?
> +
> - if (WARN(IS_ERR(work1), "BUG: ASN.1 encoder failed"))
> - return PTR_ERR(work1);
> + if (WARN(IS_ERR(work1), "BUG: ASN.1 encoder failed")) {
> + rc = PTR_ERR(work1);
> + goto err;
> + }
>
> return work1 - payload->blob;
> +err:
> + kfree(scratch);
> + return rc;
Is this another memory leak fix that is unrelated to the functionality added by
this patch?
Also, isn't 'scratch' still being leaked in the success case?
> static int tpm2_key_decode(struct trusted_key_payload *payload,
> - struct trusted_key_options *options,
> - u8 **buf)
> + struct trusted_key_options *options)
> {
> + u64 data_len;
> int ret;
> struct tpm2_key_context ctx;
> - u8 *blob;
> + u8 *blob, *buf;
>
> memset(&ctx, 0, sizeof(ctx));
>
> @@ -108,21 +231,57 @@ static int tpm2_key_decode(struct trusted_key_payload *payload,
> if (ret < 0)
> return ret;
>
> - if (ctx.priv_len + ctx.pub_len > MAX_BLOB_SIZE)
> + data_len = ctx.priv_len + ctx.pub_len + ctx.creation_data_len +
> + ctx.creation_hash_len + ctx.creation_tk_len;
It's unclear why 'data_len' is a u64, given that the value assigned to it always
fits in a u32. Perhaps you intended to do the additions with 64-bit numbers so
that they can't overflow.
But shouldn't the lengths already be bounded by size of the ASN.1 blob before
even reaching here, anyway?
> +
> + if (data_len > MAX_BLOB_SIZE)
> return -EINVAL;
>
> - blob = kmalloc(ctx.priv_len + ctx.pub_len + 4, GFP_KERNEL);
> - if (!blob)
> + buf = kmalloc(data_len + 4, GFP_KERNEL);
> + if (!buf)
> return -ENOMEM;
It's unclear what the '+ 4' is for.
> @@ -229,6 +424,7 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
> struct trusted_key_options *options)
> {
> int blob_len = 0;
> + unsigned int offset;
> struct tpm_buf buf;
> u32 hash;
> u32 flags;
> @@ -317,13 +513,14 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
> rc = -E2BIG;
> goto out;
> }
> - if (tpm_buf_length(&buf) < TPM_HEADER_SIZE + 4 + blob_len) {
> + offset = TPM_HEADER_SIZE + 4;
> + if (tpm_buf_length(&buf) < offset + blob_len) {
> rc = -EFAULT;
> goto out;
> }
>
> blob_len = tpm2_key_encode(payload, options,
> - &buf.data[TPM_HEADER_SIZE + 4],
> + &buf.data[offset],
> blob_len);
This hunk of the patch doesn't seem to serve any purpose.
- Eric
