Re: [PATCH 2/2] keys/trusted_keys: move TPM-specific fields into trusted_tpm_options
Srish Srinivasan <[email protected]>
| Newsgroups | org.kernel.vger.keyrings,org.kernel.vger.linux-integrity,org.kernel.vger.linux-kernel,org.kernel.vger.linux-security-module |
|---|---|
| Message-ID | <[email protected]> |
Hi Jarkko, thanks for taking a look. And, apologies for the delayed response. On 1/25/26 10:30 PM, Jarkko Sakkinen wrote: > On Fri, Jan 23, 2026 at 10:25:04PM +0530, Srish Srinivasan wrote: >> The trusted_key_options struct contains TPM-specific fields (keyhandle, >> keyauth, blobauth_len, blobauth, pcrinfo_len, pcrinfo, pcrlock, hash, >> policydigest_len, policydigest, and policyhandle). This leads to the >> accumulation of backend-specific fields in the generic options structure. >> >> Define trusted_tpm_options structure and move the TPM-specific fields >> there. Store a pointer to trusted_tpm_options in trusted_key_options's >> private. >> >> No functional change intended. >> >> Signed-off-by: Srish Srinivasan <[email protected]> >> Reviewed-by: Stefan Berger <[email protected]> >> --- >> include/keys/trusted-type.h | 11 --- >> include/keys/trusted_tpm.h | 14 +++ >> security/keys/trusted-keys/trusted_tpm1.c | 103 ++++++++++++++-------- >> security/keys/trusted-keys/trusted_tpm2.c | 62 ++++++++----- >> 4 files changed, 121 insertions(+), 69 deletions(-) >> >> diff --git a/include/keys/trusted-type.h b/include/keys/trusted-type.h >> index 03527162613f..b80f250305b8 100644 >> --- a/include/keys/trusted-type.h >> +++ b/include/keys/trusted-type.h >> @@ -39,17 +39,6 @@ struct trusted_key_payload { >> >> struct trusted_key_options { >> uint16_t keytype; >> - uint32_t keyhandle; >> - unsigned char keyauth[TPM_DIGEST_SIZE]; >> - uint32_t blobauth_len; >> - unsigned char blobauth[TPM_DIGEST_SIZE]; >> - uint32_t pcrinfo_len; >> - unsigned char pcrinfo[MAX_PCRINFO_SIZE]; >> - int pcrlock; >> - uint32_t hash; >> - uint32_t policydigest_len; >> - unsigned char policydigest[MAX_DIGEST_SIZE]; >> - uint32_t policyhandle; >> void *private; >> }; >> >> diff --git a/include/keys/trusted_tpm.h b/include/keys/trusted_tpm.h >> index 0fadc6a4f166..355ebd36cbfd 100644 >> --- a/include/keys/trusted_tpm.h >> +++ b/include/keys/trusted_tpm.h >> @@ -7,6 +7,20 @@ >> >> extern struct trusted_key_ops trusted_key_tpm_ops; >> >> +struct trusted_tpm_options { >> + uint32_t keyhandle; >> + unsigned char keyauth[TPM_DIGEST_SIZE]; >> + uint32_t blobauth_len; >> + unsigned char blobauth[TPM_DIGEST_SIZE]; >> + uint32_t pcrinfo_len; >> + unsigned char pcrinfo[MAX_PCRINFO_SIZE]; >> + int pcrlock; >> + uint32_t hash; >> + uint32_t policydigest_len; >> + unsigned char policydigest[MAX_DIGEST_SIZE]; >> + uint32_t policyhandle; >> +}; >> + >> int tpm2_seal_trusted(struct tpm_chip *chip, >> struct trusted_key_payload *payload, >> struct trusted_key_options *options); >> diff --git a/security/keys/trusted-keys/trusted_tpm1.c b/security/keys/trusted-keys/trusted_tpm1.c >> index 636acb66a4f6..0ab0234ebe37 100644 >> --- a/security/keys/trusted-keys/trusted_tpm1.c >> +++ b/security/keys/trusted-keys/trusted_tpm1.c >> @@ -50,12 +50,14 @@ enum { >> #if TPM_DEBUG >> static inline void dump_options(struct trusted_key_options *o) >> { >> + struct trusted_tpm_options *tpm_opts = o->private; > > TPM context is obvious i.e., actually private would be a better name. Noted. Will make the change. > >> + >> pr_info("sealing key type %d\n", o->keytype); >> - pr_info("sealing key handle %0X\n", o->keyhandle); >> - pr_info("pcrlock %d\n", o->pcrlock); >> - pr_info("pcrinfo %d\n", o->pcrinfo_len); >> + pr_info("sealing key handle %0X\n", tpm_opts->keyhandle); >> + pr_info("pcrlock %d\n", tpm_opts->pcrlock); >> + pr_info("pcrinfo %d\n", tpm_opts->pcrinfo_len); >> print_hex_dump(KERN_INFO, "pcrinfo ", DUMP_PREFIX_NONE, >> - 16, 1, o->pcrinfo, o->pcrinfo_len, 0); >> + 16, 1, tpm_opts->pcrinfo, tpm_opts->pcrinfo_len, 0); >> } > Should be replaced with pr_debug() and KERN_DEBUG as precursory patch > (and remove TPM_DEBUG). Will fix this, and make it a preparatory clean-up patch. > >> >> static inline void dump_sess(struct osapsess *s) >> @@ -624,6 +626,7 @@ static int tpm_unseal(struct tpm_buf *tb, >> static int key_seal(struct trusted_key_payload *p, >> struct trusted_key_options *o) >> { >> + struct trusted_tpm_options *tpm_opts; >> struct tpm_buf tb; >> int ret; >> >> @@ -634,9 +637,12 @@ static int key_seal(struct trusted_key_payload *p, >> /* include migratable flag at end of sealed key */ >> p->key[p->key_len] = p->migratable; >> >> - ret = tpm_seal(&tb, o->keytype, o->keyhandle, o->keyauth, >> + tpm_opts = o->private; > Not sure why this is not done in the declaration. Will fix this. > >> + >> + ret = tpm_seal(&tb, o->keytype, tpm_opts->keyhandle, tpm_opts->keyauth, >> p->key, p->key_len + 1, p->blob, &p->blob_len, >> - o->blobauth, o->pcrinfo, o->pcrinfo_len); >> + tpm_opts->blobauth, tpm_opts->pcrinfo, >> + tpm_opts->pcrinfo_len); >> if (ret < 0) >> pr_info("srkseal failed (%d)\n", ret); >> >> @@ -650,6 +656,7 @@ static int key_seal(struct trusted_key_payload *p, >> static int key_unseal(struct trusted_key_payload *p, >> struct trusted_key_options *o) >> { >> + struct trusted_tpm_options *tpm_opts; >> struct tpm_buf tb; >> int ret; >> >> @@ -657,8 +664,10 @@ static int key_unseal(struct trusted_key_payload *p, >> if (ret) >> return ret; >> >> - ret = tpm_unseal(&tb, o->keyhandle, o->keyauth, p->blob, p->blob_len, >> - o->blobauth, p->key, &p->key_len); >> + tpm_opts = o->private; >> + >> + ret = tpm_unseal(&tb, tpm_opts->keyhandle, tpm_opts->keyauth, p->blob, >> + p->blob_len, tpm_opts->blobauth, p->key, &p->key_len); >> if (ret < 0) >> pr_info("srkunseal failed (%d)\n", ret); >> else >> @@ -695,6 +704,7 @@ static const match_table_t key_tokens = { >> static int getoptions(char *c, struct trusted_key_payload *pay, >> struct trusted_key_options *opt) >> { >> + struct trusted_tpm_options *tpm_opts; >> substring_t args[MAX_OPT_ARGS]; >> char *p = c; >> int token; >> @@ -710,7 +720,9 @@ static int getoptions(char *c, struct trusted_key_payload *pay, >> if (tpm2 < 0) >> return tpm2; >> >> - opt->hash = tpm2 ? HASH_ALGO_SHA256 : HASH_ALGO_SHA1; >> + tpm_opts = opt->private; >> + > I'd remove this empty line. Will fix this. > >> + tpm_opts->hash = tpm2 ? HASH_ALGO_SHA256 : HASH_ALGO_SHA1; >> >> if (!c) >> return 0; >> @@ -724,11 +736,11 @@ static int getoptions(char *c, struct trusted_key_payload *pay, >> >> switch (token) { >> case Opt_pcrinfo: >> - opt->pcrinfo_len = strlen(args[0].from) / 2; >> - if (opt->pcrinfo_len > MAX_PCRINFO_SIZE) >> + tpm_opts->pcrinfo_len = strlen(args[0].from) / 2; >> + if (tpm_opts->pcrinfo_len > MAX_PCRINFO_SIZE) >> return -EINVAL; >> - res = hex2bin(opt->pcrinfo, args[0].from, >> - opt->pcrinfo_len); >> + res = hex2bin(tpm_opts->pcrinfo, args[0].from, >> + tpm_opts->pcrinfo_len); >> if (res < 0) >> return -EINVAL; >> break; >> @@ -737,12 +749,12 @@ static int getoptions(char *c, struct trusted_key_payload *pay, >> if (res < 0) >> return -EINVAL; >> opt->keytype = SEAL_keytype; >> - opt->keyhandle = handle; >> + tpm_opts->keyhandle = handle; >> break; >> case Opt_keyauth: >> if (strlen(args[0].from) != 2 * SHA1_DIGEST_SIZE) >> return -EINVAL; >> - res = hex2bin(opt->keyauth, args[0].from, >> + res = hex2bin(tpm_opts->keyauth, args[0].from, >> SHA1_DIGEST_SIZE); >> if (res < 0) >> return -EINVAL; >> @@ -753,21 +765,23 @@ static int getoptions(char *c, struct trusted_key_payload *pay, >> * hex strings. TPM 2.0 authorizations are simple >> * passwords (although it can take a hash as well) >> */ >> - opt->blobauth_len = strlen(args[0].from); >> + tpm_opts->blobauth_len = strlen(args[0].from); >> >> - if (opt->blobauth_len == 2 * TPM_DIGEST_SIZE) { >> - res = hex2bin(opt->blobauth, args[0].from, >> + if (tpm_opts->blobauth_len == 2 * TPM_DIGEST_SIZE) { >> + res = hex2bin(tpm_opts->blobauth, args[0].from, >> TPM_DIGEST_SIZE); >> if (res < 0) >> return -EINVAL; >> >> - opt->blobauth_len = TPM_DIGEST_SIZE; >> + tpm_opts->blobauth_len = TPM_DIGEST_SIZE; >> break; >> } >> >> - if (tpm2 && opt->blobauth_len <= sizeof(opt->blobauth)) { >> - memcpy(opt->blobauth, args[0].from, >> - opt->blobauth_len); >> + if (tpm2 && >> + tpm_opts->blobauth_len <= >> + sizeof(tpm_opts->blobauth)) { >> + memcpy(tpm_opts->blobauth, args[0].from, >> + tpm_opts->blobauth_len); >> break; >> } >> >> @@ -785,14 +799,14 @@ static int getoptions(char *c, struct trusted_key_payload *pay, >> res = kstrtoul(args[0].from, 10, &lock); >> if (res < 0) >> return -EINVAL; >> - opt->pcrlock = lock; >> + tpm_opts->pcrlock = lock; >> break; >> case Opt_hash: >> if (test_bit(Opt_policydigest, &token_mask)) >> return -EINVAL; >> for (i = 0; i < HASH_ALGO__LAST; i++) { >> if (!strcmp(args[0].from, hash_algo_name[i])) { >> - opt->hash = i; >> + tpm_opts->hash = i; >> break; >> } >> } >> @@ -804,14 +818,14 @@ static int getoptions(char *c, struct trusted_key_payload *pay, >> } >> break; >> case Opt_policydigest: >> - digest_len = hash_digest_size[opt->hash]; >> + digest_len = hash_digest_size[tpm_opts->hash]; >> if (!tpm2 || strlen(args[0].from) != (2 * digest_len)) >> return -EINVAL; >> - res = hex2bin(opt->policydigest, args[0].from, >> + res = hex2bin(tpm_opts->policydigest, args[0].from, >> digest_len); >> if (res < 0) >> return -EINVAL; >> - opt->policydigest_len = digest_len; >> + tpm_opts->policydigest_len = digest_len; >> break; >> case Opt_policyhandle: >> if (!tpm2) >> @@ -819,7 +833,7 @@ static int getoptions(char *c, struct trusted_key_payload *pay, >> res = kstrtoul(args[0].from, 16, &handle); >> if (res < 0) >> return -EINVAL; >> - opt->policyhandle = handle; >> + tpm_opts->policyhandle = handle; >> break; >> default: >> return -EINVAL; >> @@ -830,6 +844,7 @@ static int getoptions(char *c, struct trusted_key_payload *pay, >> >> static struct trusted_key_options *trusted_options_alloc(void) >> { >> + struct trusted_tpm_options *tpm_opts; >> struct trusted_key_options *options; >> int tpm2; >> >> @@ -842,14 +857,23 @@ static struct trusted_key_options *trusted_options_alloc(void) >> /* set any non-zero defaults */ >> options->keytype = SRK_keytype; >> >> - if (!tpm2) >> - options->keyhandle = SRKHANDLE; >> + tpm_opts = kzalloc(sizeof(*tpm_opts), GFP_KERNEL); >> + if (!tpm_opts) { >> + kfree_sensitive(options); >> + options = NULL; >> + } else { >> + if (!tpm2) >> + tpm_opts->keyhandle = SRKHANDLE; >> + >> + options->private = tpm_opts; >> + } >> } >> return options; >> } >> >> static int trusted_tpm_seal(struct trusted_key_payload *p, char *datablob) >> { >> + struct trusted_tpm_options *tpm_opts = NULL; >> struct trusted_key_options *options = NULL; >> int ret = 0; >> int tpm2; >> @@ -867,7 +891,9 @@ static int trusted_tpm_seal(struct trusted_key_payload *p, char *datablob) >> goto out; >> dump_options(options); >> >> - if (!options->keyhandle && !tpm2) { >> + tpm_opts = options->private; >> + >> + if (!tpm_opts->keyhandle && !tpm2) { >> ret = -EINVAL; >> goto out; >> } >> @@ -881,20 +907,22 @@ static int trusted_tpm_seal(struct trusted_key_payload *p, char *datablob) >> goto out; >> } >> >> - if (options->pcrlock) { >> - ret = pcrlock(options->pcrlock); >> + if (tpm_opts->pcrlock) { >> + ret = pcrlock(tpm_opts->pcrlock); >> if (ret < 0) { >> pr_info("pcrlock failed (%d)\n", ret); >> goto out; >> } >> } >> out: >> + kfree_sensitive(options->private); >> kfree_sensitive(options); >> return ret; >> } >> >> static int trusted_tpm_unseal(struct trusted_key_payload *p, char *datablob) >> { >> + struct trusted_tpm_options *tpm_opts = NULL; >> struct trusted_key_options *options = NULL; >> int ret = 0; >> int tpm2; >> @@ -912,7 +940,9 @@ static int trusted_tpm_unseal(struct trusted_key_payload *p, char *datablob) >> goto out; >> dump_options(options); >> >> - if (!options->keyhandle && !tpm2) { >> + tpm_opts = options->private; >> + >> + if (!tpm_opts->keyhandle && !tpm2) { >> ret = -EINVAL; >> goto out; >> } >> @@ -924,14 +954,15 @@ static int trusted_tpm_unseal(struct trusted_key_payload *p, char *datablob) >> if (ret < 0) >> pr_info("key_unseal failed (%d)\n", ret); >> >> - if (options->pcrlock) { >> - ret = pcrlock(options->pcrlock); >> + if (tpm_opts->pcrlock) { >> + ret = pcrlock(tpm_opts->pcrlock); >> if (ret < 0) { >> pr_info("pcrlock failed (%d)\n", ret); >> goto out; >> } >> } >> out: >> + kfree_sensitive(options->private); >> kfree_sensitive(options); >> return ret; >> } >> diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c >> index 6340823f8b53..568c4af9010c 100644 >> --- a/security/keys/trusted-keys/trusted_tpm2.c >> +++ b/security/keys/trusted-keys/trusted_tpm2.c >> @@ -24,6 +24,7 @@ static int tpm2_key_encode(struct trusted_key_payload *payload, >> struct trusted_key_options *options, >> u8 *src, u32 len) >> { >> + struct trusted_tpm_options *tpm_opts; >> const int SCRATCH_SIZE = PAGE_SIZE; >> u8 *scratch = kmalloc(SCRATCH_SIZE, GFP_KERNEL); >> u8 *work = scratch, *work1; >> @@ -46,7 +47,9 @@ static int tpm2_key_encode(struct trusted_key_payload *payload, >> work = asn1_encode_oid(work, end_work, tpm2key_oid, >> asn1_oid_len(tpm2key_oid)); >> >> - if (options->blobauth_len == 0) { >> + tpm_opts = options->private; >> + >> + if (tpm_opts->blobauth_len == 0) { >> unsigned char bool[3], *w = bool; >> /* tag 0 is emptyAuth */ >> w = asn1_encode_boolean(w, w + sizeof(bool), true); >> @@ -69,7 +72,7 @@ static int tpm2_key_encode(struct trusted_key_payload *payload, >> goto err; >> } >> >> - work = asn1_encode_integer(work, end_work, options->keyhandle); >> + work = asn1_encode_integer(work, end_work, tpm_opts->keyhandle); >> work = asn1_encode_octet_string(work, end_work, pub, pub_len); >> work = asn1_encode_octet_string(work, end_work, priv, priv_len); >> >> @@ -102,6 +105,7 @@ static int tpm2_key_decode(struct trusted_key_payload *payload, >> struct trusted_key_options *options, >> u8 **buf) >> { >> + struct trusted_tpm_options *tpm_opts; >> int ret; >> struct tpm2_key_context ctx; >> u8 *blob; >> @@ -120,8 +124,10 @@ static int tpm2_key_decode(struct trusted_key_payload *payload, >> if (!blob) >> return -ENOMEM; >> >> + tpm_opts = options->private; >> + >> *buf = blob; >> - options->keyhandle = ctx.parent; >> + tpm_opts->keyhandle = ctx.parent; >> >> memcpy(blob, ctx.priv, ctx.priv_len); >> blob += ctx.priv_len; >> @@ -233,6 +239,7 @@ int tpm2_seal_trusted(struct tpm_chip *chip, >> struct trusted_key_payload *payload, >> struct trusted_key_options *options) >> { >> + struct trusted_tpm_options *tpm_opts; >> off_t offset = TPM_HEADER_SIZE; >> struct tpm_buf buf, sized; >> int blob_len = 0; >> @@ -240,11 +247,13 @@ int tpm2_seal_trusted(struct tpm_chip *chip, >> u32 flags; >> int rc; >> >> - hash = tpm2_find_hash_alg(options->hash); >> + tpm_opts = options->private; >> + >> + hash = tpm2_find_hash_alg(tpm_opts->hash); >> if (hash < 0) >> return hash; >> >> - if (!options->keyhandle) >> + if (!tpm_opts->keyhandle) >> return -EINVAL; >> >> rc = tpm_try_get_ops(chip); >> @@ -268,18 +277,19 @@ int tpm2_seal_trusted(struct tpm_chip *chip, >> goto out_put; >> } >> >> - rc = tpm_buf_append_name(chip, &buf, options->keyhandle, NULL); >> + rc = tpm_buf_append_name(chip, &buf, tpm_opts->keyhandle, NULL); >> if (rc) >> goto out; >> >> tpm_buf_append_hmac_session(chip, &buf, TPM2_SA_DECRYPT, >> - options->keyauth, TPM_DIGEST_SIZE); >> + tpm_opts->keyauth, TPM_DIGEST_SIZE); >> >> /* sensitive */ >> - tpm_buf_append_u16(&sized, options->blobauth_len); >> + tpm_buf_append_u16(&sized, tpm_opts->blobauth_len); >> >> - if (options->blobauth_len) >> - tpm_buf_append(&sized, options->blobauth, options->blobauth_len); >> + if (tpm_opts->blobauth_len) >> + tpm_buf_append(&sized, tpm_opts->blobauth, >> + tpm_opts->blobauth_len); >> >> tpm_buf_append_u16(&sized, payload->key_len); >> tpm_buf_append(&sized, payload->key, payload->key_len); >> @@ -292,14 +302,15 @@ int tpm2_seal_trusted(struct tpm_chip *chip, >> >> /* key properties */ >> flags = 0; >> - flags |= options->policydigest_len ? 0 : TPM2_OA_USER_WITH_AUTH; >> + flags |= tpm_opts->policydigest_len ? 0 : TPM2_OA_USER_WITH_AUTH; >> flags |= payload->migratable ? 0 : (TPM2_OA_FIXED_TPM | TPM2_OA_FIXED_PARENT); >> tpm_buf_append_u32(&sized, flags); >> >> /* policy */ >> - tpm_buf_append_u16(&sized, options->policydigest_len); >> - if (options->policydigest_len) >> - tpm_buf_append(&sized, options->policydigest, options->policydigest_len); >> + tpm_buf_append_u16(&sized, tpm_opts->policydigest_len); >> + if (tpm_opts->policydigest_len) >> + tpm_buf_append(&sized, tpm_opts->policydigest, >> + tpm_opts->policydigest_len); >> >> /* public parameters */ >> tpm_buf_append_u16(&sized, TPM_ALG_NULL); >> @@ -373,6 +384,7 @@ static int tpm2_load_cmd(struct tpm_chip *chip, >> u32 *blob_handle) >> { >> u8 *blob_ref __free(kfree) = NULL; >> + struct trusted_tpm_options *tpm_opts; >> struct tpm_buf buf; >> unsigned int private_len; >> unsigned int public_len; >> @@ -391,8 +403,10 @@ static int tpm2_load_cmd(struct tpm_chip *chip, >> blob_ref = blob; >> } >> >> + tpm_opts = options->private; >> + >> /* new format carries keyhandle but old format doesn't */ >> - if (!options->keyhandle) >> + if (!tpm_opts->keyhandle) >> return -EINVAL; >> >> /* must be big enough for at least the two be16 size counts */ >> @@ -433,11 +447,11 @@ static int tpm2_load_cmd(struct tpm_chip *chip, >> return rc; >> } >> >> - rc = tpm_buf_append_name(chip, &buf, options->keyhandle, NULL); >> + rc = tpm_buf_append_name(chip, &buf, tpm_opts->keyhandle, NULL); >> if (rc) >> goto out; >> >> - tpm_buf_append_hmac_session(chip, &buf, 0, options->keyauth, >> + tpm_buf_append_hmac_session(chip, &buf, 0, tpm_opts->keyauth, >> TPM_DIGEST_SIZE); >> >> tpm_buf_append(&buf, blob, blob_len); >> @@ -481,6 +495,7 @@ static int tpm2_unseal_cmd(struct tpm_chip *chip, >> struct trusted_key_options *options, >> u32 blob_handle) >> { >> + struct trusted_tpm_options *tpm_opts; >> struct tpm_header *head; >> struct tpm_buf buf; >> u16 data_len; >> @@ -502,10 +517,12 @@ static int tpm2_unseal_cmd(struct tpm_chip *chip, >> if (rc) >> goto out; >> >> - if (!options->policyhandle) { >> + tpm_opts = options->private; >> + >> + if (!tpm_opts->policyhandle) { >> tpm_buf_append_hmac_session(chip, &buf, TPM2_SA_ENCRYPT, >> - options->blobauth, >> - options->blobauth_len); >> + tpm_opts->blobauth, >> + tpm_opts->blobauth_len); >> } else { >> /* >> * FIXME: The policy session was generated outside the >> @@ -518,9 +535,10 @@ static int tpm2_unseal_cmd(struct tpm_chip *chip, >> * could repeat our actions with the exfiltrated >> * password. >> */ >> - tpm2_buf_append_auth(&buf, options->policyhandle, >> + tpm2_buf_append_auth(&buf, tpm_opts->policyhandle, >> NULL /* nonce */, 0, 0, >> - options->blobauth, options->blobauth_len); >> + tpm_opts->blobauth, >> + tpm_opts->blobauth_len); >> if (tpm2_chip_auth(chip)) { >> tpm_buf_append_hmac_session(chip, &buf, TPM2_SA_ENCRYPT, NULL, 0); >> } else { >> -- >> 2.43.0 >> > BR, Jarkko I will shortly send out v2 with the changes. Thanks, Srish.