[PATCH v7 2/5] KEYS: trusted: Change -EINVAL to -E2BIG

[PATCH v7 2/5] KEYS: trusted: Change -EINVAL to -E2BIG

[Jarkko Sakkinen]

Report -E2BIG instead of -EINVAL when too large size for the key blob is
requested.

Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---
 security/keys/trusted-keys/trusted_tpm2.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c
index 8b7dd73d94c1..06c8fa7b21ae 100644
--- a/security/keys/trusted-keys/trusted_tpm2.c
+++ b/security/keys/trusted-keys/trusted_tpm2.c
@@ -122,7 +122,7 @@ static int tpm2_key_decode(struct trusted_key_payload *payload,
 		return ret;
 
 	if (ctx.priv_len + ctx.pub_len > MAX_BLOB_SIZE)
-		return -EINVAL;
+		return -E2BIG;
 
 	blob = kmalloc(ctx.priv_len + ctx.pub_len + 4, GFP_KERNEL);
 	if (!blob)
-- 
2.45.1

Re: [PATCH v7 2/5] KEYS: trusted: Change -EINVAL to -E2BIG

[Stefan Berger]

On 5/28/24 17:08, Jarkko Sakkinen wrote:
> Report -E2BIG instead of -EINVAL when too large size for the key blob is
> requested.
> 
> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>

Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>

> ---
>   security/keys/trusted-keys/trusted_tpm2.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c
> index 8b7dd73d94c1..06c8fa7b21ae 100644
> --- a/security/keys/trusted-keys/trusted_tpm2.c
> +++ b/security/keys/trusted-keys/trusted_tpm2.c
> @@ -122,7 +122,7 @@ static int tpm2_key_decode(struct trusted_key_payload *payload,
>   		return ret;
>   
>   	if (ctx.priv_len + ctx.pub_len > MAX_BLOB_SIZE)
> -		return -EINVAL;
> +		return -E2BIG;
>   
>   	blob = kmalloc(ctx.priv_len + ctx.pub_len + 4, GFP_KERNEL);
>   	if (!blob)

Re: [PATCH v7 2/5] KEYS: trusted: Change -EINVAL to -E2BIG

[Jarkko Sakkinen]

On Wed May 29, 2024 at 4:50 AM EEST, Stefan Berger wrote:
>
>
> On 5/28/24 17:08, Jarkko Sakkinen wrote:
> > Report -E2BIG instead of -EINVAL when too large size for the key blob is
> > requested.
> > 
> > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
>
> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>

Thank you.

Hmm... I'd like to add even:

Cc: stable@vger.kernel.org # v5.13+
Fixes: f2219745250f ("security: keys: trusted: use ASN.1 TPM2 key format for the blobs")

It turned out to be useful error message and would be useful also for
stable kernels. So if no decent counter-arguments, I'll just pick it
to my master branch.

BR, Jarkko

[PATCH v7 3/5] crypto: tpm2_key: Introduce a TPM2 key type

[Jarkko Sakkinen]

TPM2 ASN.1 format is required for trusted keys and asymmetric keys. Move it
to crypto in order to make it available for both. Implement validation with
coverage of all TPMT_PUBLIC shared fields. Key type specific fields must be
covered by the different subsystems using this.

A Kconfig option CRYPTO_TPM2_KEY can be used to select the feature, which
depends only crypto subsystem itself and ASN.1 parser.

Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---
v6:
* Relocate to crypto. Validate the shared part and provide
  accessor functions. Use a fixed buffer size.
v2:
* Do not allocate blob twice. Use the one inside struct tpm2_key.
---
 crypto/Kconfig                            |   7 ++
 crypto/Makefile                           |   6 +
 crypto/tpm2_key.asn1                      |  11 ++
 crypto/tpm2_key.c                         | 134 ++++++++++++++++++++
 include/crypto/tpm2_key.h                 |  46 +++++++
 security/keys/trusted-keys/Kconfig        |   2 +-
 security/keys/trusted-keys/Makefile       |   2 -
 security/keys/trusted-keys/tpm2key.asn1   |  11 --
 security/keys/trusted-keys/trusted_tpm2.c | 141 +++++-----------------
 9 files changed, 235 insertions(+), 125 deletions(-)
 create mode 100644 crypto/tpm2_key.asn1
 create mode 100644 crypto/tpm2_key.c
 create mode 100644 include/crypto/tpm2_key.h
 delete mode 100644 security/keys/trusted-keys/tpm2key.asn1

diff --git a/crypto/Kconfig b/crypto/Kconfig
index 5688d42a59c2..c8989bc71f57 100644
--- a/crypto/Kconfig
+++ b/crypto/Kconfig
@@ -5,6 +5,13 @@
 config XOR_BLOCKS
 	tristate
 
+config CRYPTO_TPM2_KEY
+	bool
+	depends on CRYPTO
+	select ASN1
+	select OID_REGISTRY
+	default n
+
 #
 # async_tx api: hardware offloaded memory transfer/transform support
 #
diff --git a/crypto/Makefile b/crypto/Makefile
index edbbaa3ffef5..d932fdb72319 100644
--- a/crypto/Makefile
+++ b/crypto/Makefile
@@ -216,3 +216,9 @@ obj-$(CONFIG_CRYPTO_SIMD) += crypto_simd.o
 # Key derivation function
 #
 obj-$(CONFIG_CRYPTO_KDF800108_CTR) += kdf_sp800108.o
+
+ifdef CONFIG_CRYPTO_TPM2_KEY
+$(obj)/tpm2_key.asn1.o: $(obj)/tpm2_key.asn1.h $(obj)/tpm2_key.asn1.c
+$(obj)/tpm2_key.o: $(obj)/tpm2_key.asn1.h
+obj-y += tpm2_key.o tpm2_key.asn1.o
+endif
diff --git a/crypto/tpm2_key.asn1 b/crypto/tpm2_key.asn1
new file mode 100644
index 000000000000..b235d02ab78e
--- /dev/null
+++ b/crypto/tpm2_key.asn1
@@ -0,0 +1,11 @@
+---
+--- ASN.1 for TPM 2.0 keys
+---
+
+TPMKey ::= SEQUENCE {
+	type		OBJECT IDENTIFIER ({tpm2_key_get_type}),
+	emptyAuth	[0] EXPLICIT BOOLEAN OPTIONAL,
+	parent		INTEGER ({tpm2_key_get_parent}),
+	pubkey		OCTET STRING ({tpm2_get_public}),
+	privkey		OCTET STRING ({tpm2_get_private})
+	}
diff --git a/crypto/tpm2_key.c b/crypto/tpm2_key.c
new file mode 100644
index 000000000000..78f55478d046
--- /dev/null
+++ b/crypto/tpm2_key.c
@@ -0,0 +1,134 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <crypto/tpm2_key.h>
+#include <linux/oid_registry.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+#include <asm/unaligned.h>
+#include "tpm2_key.asn1.h"
+
+#undef pr_fmt
+#define pr_fmt(fmt) "tpm2_key: "fmt
+
+struct tpm2_key_decoder_context {
+	u32 parent;
+	const u8 *pub;
+	u32 pub_len;
+	const u8 *priv;
+	u32 priv_len;
+	enum OID oid;
+};
+
+int tpm2_key_get_parent(void *context, size_t hdrlen,
+			unsigned char tag,
+			const void *value, size_t vlen)
+{
+	struct tpm2_key_decoder_context *decoder = context;
+	const u8 *v = value;
+	int i;
+
+	decoder->parent = 0;
+	for (i = 0; i < vlen; i++) {
+		decoder->parent <<= 8;
+		decoder->parent |= v[i];
+	}
+
+	return 0;
+}
+
+int tpm2_key_get_type(void *context, size_t hdrlen,
+		      unsigned char tag,
+		      const void *value, size_t vlen)
+{
+	struct tpm2_key_decoder_context *decoder = context;
+
+	decoder->oid = look_up_OID(value, vlen);
+	return 0;
+}
+
+static inline bool tpm2_key_is_valid(const void *value, size_t vlen)
+{
+	if (vlen < 2 || vlen > TPM2_KEY_BYTES_MAX)
+		return false;
+
+	if (get_unaligned_be16(value) != vlen - 2)
+		return false;
+
+	return true;
+}
+
+int tpm2_get_public(void *context, size_t hdrlen, unsigned char tag,
+		    const void *value, size_t vlen)
+{
+	struct tpm2_key_decoder_context *decoder = context;
+
+	if (!tpm2_key_is_valid(value, vlen))
+		return -EBADMSG;
+
+	if (sizeof(struct tpm2_key_desc) > vlen - 2)
+		return -EBADMSG;
+
+	decoder->pub = value;
+	decoder->pub_len = vlen;
+	return 0;
+}
+
+int tpm2_get_private(void *context, size_t hdrlen, unsigned char tag,
+		     const void *value, size_t vlen)
+{
+	struct tpm2_key_decoder_context *decoder = context;
+
+	if (!tpm2_key_is_valid(value, vlen))
+		return -EBADMSG;
+
+	decoder->priv = value;
+	decoder->priv_len = vlen;
+	return 0;
+}
+
+/**
+ * tpm_key_decode() - Decode TPM2 ASN.1 key
+ * @src:	ASN.1 source.
+ * @src_len:	ASN.1 source length.
+ *
+ * Decodes the TPM2 ASN.1 key and validates that the public key data has all
+ * the shared fields of TPMT_PUBLIC. This is full coverage of the memory that
+ * can be validated before doing any key type specific validation.
+ *
+ * Return:
+ * - TPM2 ASN.1 key on success.
+ * - -EBADMSG when decoding fails.
+ * - -ENOMEM when OOM while allocating struct tpm2_key.
+ */
+struct tpm2_key *tpm2_key_decode(const u8 *src, u32 src_len)
+{
+	struct tpm2_key_decoder_context decoder;
+	struct tpm2_key *key;
+	u8 *data;
+	int ret;
+
+	memset(&decoder, 0, sizeof(decoder));
+	ret = asn1_ber_decoder(&tpm2_key_decoder, &decoder, src, src_len);
+	if (ret < 0) {
+		if (ret != -EBADMSG)
+			pr_info("Decoder error %d\n", ret);
+
+		return ERR_PTR(-EBADMSG);
+	}
+
+	key = kzalloc(sizeof(*key), GFP_KERNEL);
+	if (!key)
+		return ERR_PTR(-ENOMEM);
+
+	data = &key->data[0];
+	memcpy(&data[0], decoder.priv, decoder.priv_len);
+	memcpy(&data[decoder.priv_len], decoder.pub, decoder.pub_len);
+
+	key->oid = decoder.oid;
+	key->priv_len = decoder.priv_len;
+	key->pub_len = decoder.pub_len;
+	key->parent = decoder.parent;
+	key->desc = (struct tpm2_key_desc *)&data[decoder.priv_len + 2];
+	return key;
+}
+EXPORT_SYMBOL_GPL(tpm2_key_decode);
diff --git a/include/crypto/tpm2_key.h b/include/crypto/tpm2_key.h
new file mode 100644
index 000000000000..74debaf707bf
--- /dev/null
+++ b/include/crypto/tpm2_key.h
@@ -0,0 +1,46 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+#ifndef __LINUX_TPM2_KEY_H__
+#define __LINUX_TPM2_KEY_H__
+
+#include <linux/oid_registry.h>
+#include <linux/slab.h>
+
+#define TPM2_KEY_BYTES_MAX 1024
+
+/*  TPM2 Structures 12.2.4: TPMT_PUBLIC */
+struct tpm2_key_desc {
+	__be16 type;
+	__be16 name_alg;
+	__be32 object_attributes;
+	__be16 policy_size;
+} __packed;
+
+/* Decoded TPM2 ASN.1 key. */
+struct tpm2_key {
+	u8 data[2 * TPM2_KEY_BYTES_MAX];
+	struct tpm2_key_desc *desc;
+	u16 priv_len;
+	u16 pub_len;
+	u32 parent;
+	enum OID oid;
+	char oid_str[64];
+};
+
+struct tpm2_key *tpm2_key_decode(const u8 *src, u32 src_len);
+
+static inline const void *tpm2_key_data(const struct tpm2_key *key)
+{
+	return &key->data[0];
+}
+
+static inline u16 tpm2_key_type(const struct tpm2_key *key)
+{
+	return be16_to_cpu(key->desc->type);
+}
+
+static inline int tpm2_key_policy_size(const struct tpm2_key *key)
+{
+	return be16_to_cpu(key->desc->policy_size);
+}
+
+#endif /* __LINUX_TPM2_KEY_H__ */
diff --git a/security/keys/trusted-keys/Kconfig b/security/keys/trusted-keys/Kconfig
index 1fb8aa001995..00d9489384ac 100644
--- a/security/keys/trusted-keys/Kconfig
+++ b/security/keys/trusted-keys/Kconfig
@@ -9,9 +9,9 @@ config TRUSTED_KEYS_TPM
 	select CRYPTO_HMAC
 	select CRYPTO_SHA1
 	select CRYPTO_HASH_INFO
+	select CRYPTO_TPM2_KEY
 	select ASN1_ENCODER
 	select OID_REGISTRY
-	select ASN1
 	select HAVE_TRUSTED_KEYS
 	help
 	  Enable use of the Trusted Platform Module (TPM) as trusted key
diff --git a/security/keys/trusted-keys/Makefile b/security/keys/trusted-keys/Makefile
index f0f3b27f688b..2674d5c10fc9 100644
--- a/security/keys/trusted-keys/Makefile
+++ b/security/keys/trusted-keys/Makefile
@@ -7,9 +7,7 @@ obj-$(CONFIG_TRUSTED_KEYS) += trusted.o
 trusted-y += trusted_core.o
 trusted-$(CONFIG_TRUSTED_KEYS_TPM) += trusted_tpm1.o
 
-$(obj)/trusted_tpm2.o: $(obj)/tpm2key.asn1.h
 trusted-$(CONFIG_TRUSTED_KEYS_TPM) += trusted_tpm2.o
-trusted-$(CONFIG_TRUSTED_KEYS_TPM) += tpm2key.asn1.o
 
 trusted-$(CONFIG_TRUSTED_KEYS_TEE) += trusted_tee.o
 
diff --git a/security/keys/trusted-keys/tpm2key.asn1 b/security/keys/trusted-keys/tpm2key.asn1
deleted file mode 100644
index f57f869ad600..000000000000
--- a/security/keys/trusted-keys/tpm2key.asn1
+++ /dev/null
@@ -1,11 +0,0 @@
----
---- ASN.1 for TPM 2.0 keys
----
-
-TPMKey ::= SEQUENCE {
-	type		OBJECT IDENTIFIER ({tpm2_key_type}),
-	emptyAuth	[0] EXPLICIT BOOLEAN OPTIONAL,
-	parent		INTEGER ({tpm2_key_parent}),
-	pubkey		OCTET STRING ({tpm2_key_pub}),
-	privkey		OCTET STRING ({tpm2_key_priv})
-	}
diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c
index 06c8fa7b21ae..b9e505e99e8c 100644
--- a/security/keys/trusted-keys/trusted_tpm2.c
+++ b/security/keys/trusted-keys/trusted_tpm2.c
@@ -13,11 +13,10 @@
 
 #include <keys/trusted-type.h>
 #include <keys/trusted_tpm.h>
+#include <crypto/tpm2_key.h>
 
 #include <asm/unaligned.h>
 
-#include "tpm2key.asn1.h"
-
 static struct tpm2_hash tpm2_hash_map[] = {
 	{HASH_ALGO_SHA1, TPM_ALG_SHA1},
 	{HASH_ALGO_SHA256, TPM_ALG_SHA256},
@@ -98,106 +97,6 @@ static int tpm2_key_encode(struct trusted_key_payload *payload,
 	return ret;
 }
 
-struct tpm2_key_context {
-	u32 parent;
-	const u8 *pub;
-	u32 pub_len;
-	const u8 *priv;
-	u32 priv_len;
-};
-
-static int tpm2_key_decode(struct trusted_key_payload *payload,
-			   struct trusted_key_options *options,
-			   u8 **buf)
-{
-	int ret;
-	struct tpm2_key_context ctx;
-	u8 *blob;
-
-	memset(&ctx, 0, sizeof(ctx));
-
-	ret = asn1_ber_decoder(&tpm2key_decoder, &ctx, payload->blob,
-			       payload->blob_len);
-	if (ret < 0)
-		return ret;
-
-	if (ctx.priv_len + ctx.pub_len > MAX_BLOB_SIZE)
-		return -E2BIG;
-
-	blob = kmalloc(ctx.priv_len + ctx.pub_len + 4, GFP_KERNEL);
-	if (!blob)
-		return -ENOMEM;
-
-	*buf = blob;
-	options->keyhandle = ctx.parent;
-
-	memcpy(blob, ctx.priv, ctx.priv_len);
-	blob += ctx.priv_len;
-
-	memcpy(blob, ctx.pub, ctx.pub_len);
-
-	return 0;
-}
-
-int tpm2_key_parent(void *context, size_t hdrlen,
-		  unsigned char tag,
-		  const void *value, size_t vlen)
-{
-	struct tpm2_key_context *ctx = context;
-	const u8 *v = value;
-	int i;
-
-	ctx->parent = 0;
-	for (i = 0; i < vlen; i++) {
-		ctx->parent <<= 8;
-		ctx->parent |= v[i];
-	}
-
-	return 0;
-}
-
-int tpm2_key_type(void *context, size_t hdrlen,
-		unsigned char tag,
-		const void *value, size_t vlen)
-{
-	enum OID oid = look_up_OID(value, vlen);
-
-	if (oid != OID_TPMSealedData) {
-		char buffer[50];
-
-		sprint_oid(value, vlen, buffer, sizeof(buffer));
-		pr_debug("OID is \"%s\" which is not TPMSealedData\n",
-			 buffer);
-		return -EINVAL;
-	}
-
-	return 0;
-}
-
-int tpm2_key_pub(void *context, size_t hdrlen,
-	       unsigned char tag,
-	       const void *value, size_t vlen)
-{
-	struct tpm2_key_context *ctx = context;
-
-	ctx->pub = value;
-	ctx->pub_len = vlen;
-
-	return 0;
-}
-
-int tpm2_key_priv(void *context, size_t hdrlen,
-		unsigned char tag,
-		const void *value, size_t vlen)
-{
-	struct tpm2_key_context *ctx = context;
-
-	ctx->priv = value;
-	ctx->priv_len = vlen;
-
-	return 0;
-}
-
 /**
  * tpm2_buf_append_auth() - append TPMS_AUTH_COMMAND to the buffer.
  *
@@ -387,22 +286,43 @@ static int tpm2_load_cmd(struct tpm_chip *chip,
 			 struct trusted_key_options *options,
 			 u32 *blob_handle)
 {
-	struct tpm_buf buf;
 	unsigned int private_len;
 	unsigned int public_len;
 	unsigned int blob_len;
-	u8 *blob, *pub;
-	int rc;
+	struct tpm2_key *key;
+	const u8 *blob, *pub;
+	struct tpm_buf buf;
 	u32 attrs;
+	int rc;
 
-	rc = tpm2_key_decode(payload, options, &blob);
-	if (rc) {
-		/* old form */
+	key = tpm2_key_decode(payload->blob, payload->blob_len);
+	if (IS_ERR(key)) {
+		/* Get the error code and reset the pointer to the key: */
+		rc = PTR_ERR(key);
+		key = NULL;
+
+		if (rc == -ENOMEM)
+			return -ENOMEM;
+
+		/* A sanity check, as only -EBADMSG or -ENOMEM are expected: */
+		if (rc != -EBADMSG)
+			pr_err("tpm2_key_decode(): spurious error code %d\n", rc);
+
+		/* Fallback to the legacy format: */
 		blob = payload->blob;
 		payload->old_format = 1;
+	} else {
+		blob = tpm2_key_data(key);
+		if (key->oid != OID_TPMSealedData) {
+			kfree(key);
+			return -EBADMSG;
+		}
 	}
 
-	/* new format carries keyhandle but old format doesn't */
+	/*
+	 * Must be non-zero here, either extracted from the ASN.1 for the new
+	 * format or specified on the command line for the old.
+	 */
 	if (!options->keyhandle)
 		return -EINVAL;
 
@@ -464,8 +384,7 @@ static int tpm2_load_cmd(struct tpm_chip *chip,
 			(__be32 *) &buf.data[TPM_HEADER_SIZE]);
 
 out:
-	if (blob != payload->blob)
-		kfree(blob);
+	kfree(key);
 	tpm_buf_destroy(&buf);
 
 	if (rc > 0)
-- 
2.45.1

Re: [PATCH v7 3/5] crypto: tpm2_key: Introduce a TPM2 key type

[Stefan Berger]

On 5/28/24 17:08, Jarkko Sakkinen wrote:
> TPM2 ASN.1 format is required for trusted keys and asymmetric keys. Move it
> to crypto in order to make it available for both. Implement validation with
> coverage of all TPMT_PUBLIC shared fields. Key type specific fields must be
> covered by the different subsystems using this.
> 
> A Kconfig option CRYPTO_TPM2_KEY can be used to select the feature, which
> depends only crypto subsystem itself and ASN.1 parser.
> 
> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> ---
> v6:
> * Relocate to crypto. Validate the shared part and provide
>    accessor functions. Use a fixed buffer size.
> v2:
> * Do not allocate blob twice. Use the one inside struct tpm2_key.
> ---
>   crypto/Kconfig                            |   7 ++
>   crypto/Makefile                           |   6 +
>   crypto/tpm2_key.asn1                      |  11 ++
>   crypto/tpm2_key.c                         | 134 ++++++++++++++++++++
>   include/crypto/tpm2_key.h                 |  46 +++++++
>   security/keys/trusted-keys/Kconfig        |   2 +-
>   security/keys/trusted-keys/Makefile       |   2 -
>   security/keys/trusted-keys/tpm2key.asn1   |  11 --
>   security/keys/trusted-keys/trusted_tpm2.c | 141 +++++-----------------
>   9 files changed, 235 insertions(+), 125 deletions(-)
>   create mode 100644 crypto/tpm2_key.asn1
>   create mode 100644 crypto/tpm2_key.c
>   create mode 100644 include/crypto/tpm2_key.h
>   delete mode 100644 security/keys/trusted-keys/tpm2key.asn1
> 
> diff --git a/crypto/Kconfig b/crypto/Kconfig
> index 5688d42a59c2..c8989bc71f57 100644
> --- a/crypto/Kconfig
> +++ b/crypto/Kconfig
> @@ -5,6 +5,13 @@
>   config XOR_BLOCKS
>   	tristate
>   
> +config CRYPTO_TPM2_KEY
> +	bool
> +	depends on CRYPTO
> +	select ASN1
> +	select OID_REGISTRY
> +	default n
> +
>   #
>   # async_tx api: hardware offloaded memory transfer/transform support
>   #
> diff --git a/crypto/Makefile b/crypto/Makefile
> index edbbaa3ffef5..d932fdb72319 100644
> --- a/crypto/Makefile
> +++ b/crypto/Makefile
> @@ -216,3 +216,9 @@ obj-$(CONFIG_CRYPTO_SIMD) += crypto_simd.o
>   # Key derivation function
>   #
>   obj-$(CONFIG_CRYPTO_KDF800108_CTR) += kdf_sp800108.o
> +
> +ifdef CONFIG_CRYPTO_TPM2_KEY
> +$(obj)/tpm2_key.asn1.o: $(obj)/tpm2_key.asn1.h $(obj)/tpm2_key.asn1.c
> +$(obj)/tpm2_key.o: $(obj)/tpm2_key.asn1.h
> +obj-y += tpm2_key.o tpm2_key.asn1.o
> +endif
> diff --git a/crypto/tpm2_key.asn1 b/crypto/tpm2_key.asn1
> new file mode 100644
> index 000000000000..b235d02ab78e
> --- /dev/null
> +++ b/crypto/tpm2_key.asn1
> @@ -0,0 +1,11 @@
> +---
> +--- ASN.1 for TPM 2.0 keys
> +---
> +
> +TPMKey ::= SEQUENCE {
> +	type		OBJECT IDENTIFIER ({tpm2_key_get_type}),
> +	emptyAuth	[0] EXPLICIT BOOLEAN OPTIONAL,
> +	parent		INTEGER ({tpm2_key_get_parent}),
> +	pubkey		OCTET STRING ({tpm2_get_public}),
> +	privkey		OCTET STRING ({tpm2_get_private})
> +	}
> diff --git a/crypto/tpm2_key.c b/crypto/tpm2_key.c
> new file mode 100644
> index 000000000000..78f55478d046
> --- /dev/null
> +++ b/crypto/tpm2_key.c
> @@ -0,0 +1,134 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include <crypto/tpm2_key.h>
> +#include <linux/oid_registry.h>
> +#include <linux/slab.h>
> +#include <linux/types.h>
> +#include <asm/unaligned.h>
> +#include "tpm2_key.asn1.h"
> +
> +#undef pr_fmt
> +#define pr_fmt(fmt) "tpm2_key: "fmt
> +
> +struct tpm2_key_decoder_context {
> +	u32 parent;
> +	const u8 *pub;
> +	u32 pub_len;
> +	const u8 *priv;
> +	u32 priv_len;
> +	enum OID oid;
> +};
> +
> +int tpm2_key_get_parent(void *context, size_t hdrlen,
> +			unsigned char tag,
> +			const void *value, size_t vlen)
> +{
> +	struct tpm2_key_decoder_context *decoder = context;
> +	const u8 *v = value;
> +	int i;
> +
> +	decoder->parent = 0;
> +	for (i = 0; i < vlen; i++) {
> +		decoder->parent <<= 8;
> +		decoder->parent |= v[i];
> +	}
> +
> +	return 0;
> +}
> +
> +int tpm2_key_get_type(void *context, size_t hdrlen,
> +		      unsigned char tag,
> +		      const void *value, size_t vlen)
> +{
> +	struct tpm2_key_decoder_context *decoder = context;
> +
> +	decoder->oid = look_up_OID(value, vlen);
> +	return 0;
> +}
> +
> +static inline bool tpm2_key_is_valid(const void *value, size_t vlen)
> +{
> +	if (vlen < 2 || vlen > TPM2_KEY_BYTES_MAX)
> +		return false;
> +
> +	if (get_unaligned_be16(value) != vlen - 2)
> +		return false;
> +
> +	return true;
> +}
> +
> +int tpm2_get_public(void *context, size_t hdrlen, unsigned char tag,
> +		    const void *value, size_t vlen)
> +{
> +	struct tpm2_key_decoder_context *decoder = context;
> +
> +	if (!tpm2_key_is_valid(value, vlen))
> +		return -EBADMSG;
> +
> +	if (sizeof(struct tpm2_key_desc) > vlen - 2)
> +		return -EBADMSG;
> +
> +	decoder->pub = value;
> +	decoder->pub_len = vlen;
> +	return 0;
> +}
> +
> +int tpm2_get_private(void *context, size_t hdrlen, unsigned char tag,
> +		     const void *value, size_t vlen)
> +{
> +	struct tpm2_key_decoder_context *decoder = context;
> +
> +	if (!tpm2_key_is_valid(value, vlen))
> +		return -EBADMSG;
> +
> +	decoder->priv = value;
> +	decoder->priv_len = vlen;
> +	return 0;
> +}
> +
> +/**
> + * tpm_key_decode() - Decode TPM2 ASN.1 key
> + * @src:	ASN.1 source.
> + * @src_len:	ASN.1 source length.
> + *
> + * Decodes the TPM2 ASN.1 key and validates that the public key data has all
> + * the shared fields of TPMT_PUBLIC. This is full coverage of the memory that
> + * can be validated before doing any key type specific validation.

I am not sure what the last sentence means.

> + *
> + * Return:
> + * - TPM2 ASN.1 key on success.
> + * - -EBADMSG when decoding fails.
> + * - -ENOMEM when OOM while allocating struct tpm2_key.
> + */
> +struct tpm2_key *tpm2_key_decode(const u8 *src, u32 src_len)
> +{
> +	struct tpm2_key_decoder_context decoder;
> +	struct tpm2_key *key;
> +	u8 *data;
> +	int ret;
> +
> +	memset(&decoder, 0, sizeof(decoder));
> +	ret = asn1_ber_decoder(&tpm2_key_decoder, &decoder, src, src_len);
> +	if (ret < 0) {
> +		if (ret != -EBADMSG)
> +			pr_info("Decoder error %d\n", ret);
> +
> +		return ERR_PTR(-EBADMSG);
> +	}
> +
> +	key = kzalloc(sizeof(*key), GFP_KERNEL);
> +	if (!key)
> +		return ERR_PTR(-ENOMEM);
> +
> +	data = &key->data[0];
> +	memcpy(&data[0], decoder.priv, decoder.priv_len);
> +	memcpy(&data[decoder.priv_len], decoder.pub, decoder.pub_len);
> +
> +	key->oid = decoder.oid;
> +	key->priv_len = decoder.priv_len;
> +	key->pub_len = decoder.pub_len;
> +	key->parent = decoder.parent;
> +	key->desc = (struct tpm2_key_desc *)&data[decoder.priv_len + 2];
> +	return key;
> +}
> +EXPORT_SYMBOL_GPL(tpm2_key_decode);
> diff --git a/include/crypto/tpm2_key.h b/include/crypto/tpm2_key.h
> new file mode 100644
> index 000000000000..74debaf707bf
> --- /dev/null
> +++ b/include/crypto/tpm2_key.h
> @@ -0,0 +1,46 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +#ifndef __LINUX_TPM2_KEY_H__
> +#define __LINUX_TPM2_KEY_H__
> +
> +#include <linux/oid_registry.h>
> +#include <linux/slab.h>
> +
> +#define TPM2_KEY_BYTES_MAX 1024
> +
> +/*  TPM2 Structures 12.2.4: TPMT_PUBLIC */
> +struct tpm2_key_desc {
> +	__be16 type;
> +	__be16 name_alg;
> +	__be32 object_attributes;
> +	__be16 policy_size;
> +} __packed;
> +
> +/* Decoded TPM2 ASN.1 key. */
> +struct tpm2_key {
> +	u8 data[2 * TPM2_KEY_BYTES_MAX];
> +	struct tpm2_key_desc *desc;
> +	u16 priv_len;
> +	u16 pub_len;
> +	u32 parent;
> +	enum OID oid;
> +	char oid_str[64];
> +};
> +
> +struct tpm2_key *tpm2_key_decode(const u8 *src, u32 src_len);
> +
> +static inline const void *tpm2_key_data(const struct tpm2_key *key)
> +{
> +	return &key->data[0];
> +}
> +
> +static inline u16 tpm2_key_type(const struct tpm2_key *key)
> +{
> +	return be16_to_cpu(key->desc->type);
> +}
> +
> +static inline int tpm2_key_policy_size(const struct tpm2_key *key)
> +{
> +	return be16_to_cpu(key->desc->policy_size);
> +}
> +
> +#endif /* __LINUX_TPM2_KEY_H__ */
> diff --git a/security/keys/trusted-keys/Kconfig b/security/keys/trusted-keys/Kconfig
> index 1fb8aa001995..00d9489384ac 100644
> --- a/security/keys/trusted-keys/Kconfig
> +++ b/security/keys/trusted-keys/Kconfig
> @@ -9,9 +9,9 @@ config TRUSTED_KEYS_TPM
>   	select CRYPTO_HMAC
>   	select CRYPTO_SHA1
>   	select CRYPTO_HASH_INFO
> +	select CRYPTO_TPM2_KEY
>   	select ASN1_ENCODER
>   	select OID_REGISTRY
> -	select ASN1
>   	select HAVE_TRUSTED_KEYS
>   	help
>   	  Enable use of the Trusted Platform Module (TPM) as trusted key
> diff --git a/security/keys/trusted-keys/Makefile b/security/keys/trusted-keys/Makefile
> index f0f3b27f688b..2674d5c10fc9 100644
> --- a/security/keys/trusted-keys/Makefile
> +++ b/security/keys/trusted-keys/Makefile
> @@ -7,9 +7,7 @@ obj-$(CONFIG_TRUSTED_KEYS) += trusted.o
>   trusted-y += trusted_core.o
>   trusted-$(CONFIG_TRUSTED_KEYS_TPM) += trusted_tpm1.o
>   
> -$(obj)/trusted_tpm2.o: $(obj)/tpm2key.asn1.h
>   trusted-$(CONFIG_TRUSTED_KEYS_TPM) += trusted_tpm2.o
> -trusted-$(CONFIG_TRUSTED_KEYS_TPM) += tpm2key.asn1.o
>   
>   trusted-$(CONFIG_TRUSTED_KEYS_TEE) += trusted_tee.o
>   
> diff --git a/security/keys/trusted-keys/tpm2key.asn1 b/security/keys/trusted-keys/tpm2key.asn1
> deleted file mode 100644
> index f57f869ad600..000000000000
> --- a/security/keys/trusted-keys/tpm2key.asn1
> +++ /dev/null
> @@ -1,11 +0,0 @@
> ----
> ---- ASN.1 for TPM 2.0 keys
> ----
> -
> -TPMKey ::= SEQUENCE {
> -	type		OBJECT IDENTIFIER ({tpm2_key_type}),
> -	emptyAuth	[0] EXPLICIT BOOLEAN OPTIONAL,
> -	parent		INTEGER ({tpm2_key_parent}),
> -	pubkey		OCTET STRING ({tpm2_key_pub}),
> -	privkey		OCTET STRING ({tpm2_key_priv})
> -	}
> diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c
> index 06c8fa7b21ae..b9e505e99e8c 100644
> --- a/security/keys/trusted-keys/trusted_tpm2.c
> +++ b/security/keys/trusted-keys/trusted_tpm2.c
> @@ -13,11 +13,10 @@
>   
>   #include <keys/trusted-type.h>
>   #include <keys/trusted_tpm.h>
> +#include <crypto/tpm2_key.h>
>   
>   #include <asm/unaligned.h>
>   
> -#include "tpm2key.asn1.h"
> -
>   static struct tpm2_hash tpm2_hash_map[] = {
>   	{HASH_ALGO_SHA1, TPM_ALG_SHA1},
>   	{HASH_ALGO_SHA256, TPM_ALG_SHA256},
> @@ -98,106 +97,6 @@ static int tpm2_key_encode(struct trusted_key_payload *payload,
>   	return ret;
>   }
>   
> -struct tpm2_key_context {
> -	u32 parent;
> -	const u8 *pub;
> -	u32 pub_len;
> -	const u8 *priv;
> -	u32 priv_len;
> -};
> -
> -static int tpm2_key_decode(struct trusted_key_payload *payload,
> -			   struct trusted_key_options *options,
> -			   u8 **buf)
> -{
> -	int ret;
> -	struct tpm2_key_context ctx;
> -	u8 *blob;
> -
> -	memset(&ctx, 0, sizeof(ctx));
> -
> -	ret = asn1_ber_decoder(&tpm2key_decoder, &ctx, payload->blob,
> -			       payload->blob_len);
> -	if (ret < 0)
> -		return ret;
> -
> -	if (ctx.priv_len + ctx.pub_len > MAX_BLOB_SIZE)
> -		return -E2BIG;
> -
> -	blob = kmalloc(ctx.priv_len + ctx.pub_len + 4, GFP_KERNEL);
> -	if (!blob)
> -		return -ENOMEM;
> -
> -	*buf = blob;
> -	options->keyhandle = ctx.parent;
> -
> -	memcpy(blob, ctx.priv, ctx.priv_len);
> -	blob += ctx.priv_len;
> -
> -	memcpy(blob, ctx.pub, ctx.pub_len);
> -
> -	return 0;
> -}
> -
> -int tpm2_key_parent(void *context, size_t hdrlen,
> -		  unsigned char tag,
> -		  const void *value, size_t vlen)
> -{
> -	struct tpm2_key_context *ctx = context;
> -	const u8 *v = value;
> -	int i;
> -
> -	ctx->parent = 0;
> -	for (i = 0; i < vlen; i++) {
> -		ctx->parent <<= 8;
> -		ctx->parent |= v[i];
> -	}
> -
> -	return 0;
> -}
> -
> -int tpm2_key_type(void *context, size_t hdrlen,
> -		unsigned char tag,
> -		const void *value, size_t vlen)
> -{
> -	enum OID oid = look_up_OID(value, vlen);
> -
> -	if (oid != OID_TPMSealedData) {
> -		char buffer[50];
> -
> -		sprint_oid(value, vlen, buffer, sizeof(buffer));
> -		pr_debug("OID is \"%s\" which is not TPMSealedData\n",
> -			 buffer);
> -		return -EINVAL;
> -	}
> -
> -	return 0;
> -}
> -
> -int tpm2_key_pub(void *context, size_t hdrlen,
> -	       unsigned char tag,
> -	       const void *value, size_t vlen)
> -{
> -	struct tpm2_key_context *ctx = context;
> -
> -	ctx->pub = value;
> -	ctx->pub_len = vlen;
> -
> -	return 0;
> -}
> -
> -int tpm2_key_priv(void *context, size_t hdrlen,
> -		unsigned char tag,
> -		const void *value, size_t vlen)
> -{
> -	struct tpm2_key_context *ctx = context;
> -
> -	ctx->priv = value;
> -	ctx->priv_len = vlen;
> -
> -	return 0;
> -}
> -
>   /**
>    * tpm2_buf_append_auth() - append TPMS_AUTH_COMMAND to the buffer.
>    *
> @@ -387,22 +286,43 @@ static int tpm2_load_cmd(struct tpm_chip *chip,
>   			 struct trusted_key_options *options,
>   			 u32 *blob_handle)
>   {
> -	struct tpm_buf buf;
>   	unsigned int private_len;
>   	unsigned int public_len;
>   	unsigned int blob_len;
> -	u8 *blob, *pub;
> -	int rc;
> +	struct tpm2_key *key;
> +	const u8 *blob, *pub;
> +	struct tpm_buf buf;
>   	u32 attrs;
> +	int rc;
>   
> -	rc = tpm2_key_decode(payload, options, &blob);
> -	if (rc) {
> -		/* old form */
> +	key = tpm2_key_decode(payload->blob, payload->blob_len);
> +	if (IS_ERR(key)) {
> +		/* Get the error code and reset the pointer to the key: */
> +		rc = PTR_ERR(key);
> +		key = NULL;
> +
> +		if (rc == -ENOMEM)
> +			return -ENOMEM;
> +
> +		/* A sanity check, as only -EBADMSG or -ENOMEM are expected: */
> +		if (rc != -EBADMSG)
> +			pr_err("tpm2_key_decode(): spurious error code %d\n", rc);

tpm2_key_decode seems simple enough that it only returns key, -ENOMEM or 
EBADMSG.

> +
> +		/* Fallback to the legacy format: */
>   		blob = payload->blob;
>   		payload->old_format = 1;
> +	} else {
> +		blob = tpm2_key_data(key);
> +		if (key->oid != OID_TPMSealedData) {
> +			kfree(key);
> +			return -EBADMSG;
> +		}
>   	}
>   
> -	/* new format carries keyhandle but old format doesn't */
> +	/*
> +	 * Must be non-zero here, either extracted from the ASN.1 for the new
> +	 * format or specified on the command line for the old.

sentence seems incomplete: ... for the old one.  OR  ... for the old format.

> +	 */
>   	if (!options->keyhandle)
>   		return -EINVAL;
>   
> @@ -464,8 +384,7 @@ static int tpm2_load_cmd(struct tpm_chip *chip,
>   			(__be32 *) &buf.data[TPM_HEADER_SIZE]);
>   
>   out:
> -	if (blob != payload->blob)
> -		kfree(blob);
> +	kfree(key);
>   	tpm_buf_destroy(&buf);
>   
>   	if (rc > 0)

Re: [PATCH v7 3/5] crypto: tpm2_key: Introduce a TPM2 key type

[Jarkko Sakkinen]

On Fri May 31, 2024 at 3:35 AM EEST, Stefan Berger wrote:
>
>
> On 5/28/24 17:08, Jarkko Sakkinen wrote:
> > TPM2 ASN.1 format is required for trusted keys and asymmetric keys. Move it
> > to crypto in order to make it available for both. Implement validation with
> > coverage of all TPMT_PUBLIC shared fields. Key type specific fields must be
> > covered by the different subsystems using this.
> > 
> > A Kconfig option CRYPTO_TPM2_KEY can be used to select the feature, which
> > depends only crypto subsystem itself and ASN.1 parser.
> > 
> > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> > ---
> > v6:
> > * Relocate to crypto. Validate the shared part and provide
> >    accessor functions. Use a fixed buffer size.
> > v2:
> > * Do not allocate blob twice. Use the one inside struct tpm2_key.
> > ---
> >   crypto/Kconfig                            |   7 ++
> >   crypto/Makefile                           |   6 +
> >   crypto/tpm2_key.asn1                      |  11 ++
> >   crypto/tpm2_key.c                         | 134 ++++++++++++++++++++
> >   include/crypto/tpm2_key.h                 |  46 +++++++
> >   security/keys/trusted-keys/Kconfig        |   2 +-
> >   security/keys/trusted-keys/Makefile       |   2 -
> >   security/keys/trusted-keys/tpm2key.asn1   |  11 --
> >   security/keys/trusted-keys/trusted_tpm2.c | 141 +++++-----------------
> >   9 files changed, 235 insertions(+), 125 deletions(-)
> >   create mode 100644 crypto/tpm2_key.asn1
> >   create mode 100644 crypto/tpm2_key.c
> >   create mode 100644 include/crypto/tpm2_key.h
> >   delete mode 100644 security/keys/trusted-keys/tpm2key.asn1
> > 
> > diff --git a/crypto/Kconfig b/crypto/Kconfig
> > index 5688d42a59c2..c8989bc71f57 100644
> > --- a/crypto/Kconfig
> > +++ b/crypto/Kconfig
> > @@ -5,6 +5,13 @@
> >   config XOR_BLOCKS
> >   	tristate
> >   
> > +config CRYPTO_TPM2_KEY
> > +	bool
> > +	depends on CRYPTO
> > +	select ASN1
> > +	select OID_REGISTRY
> > +	default n
> > +
> >   #
> >   # async_tx api: hardware offloaded memory transfer/transform support
> >   #
> > diff --git a/crypto/Makefile b/crypto/Makefile
> > index edbbaa3ffef5..d932fdb72319 100644
> > --- a/crypto/Makefile
> > +++ b/crypto/Makefile
> > @@ -216,3 +216,9 @@ obj-$(CONFIG_CRYPTO_SIMD) += crypto_simd.o
> >   # Key derivation function
> >   #
> >   obj-$(CONFIG_CRYPTO_KDF800108_CTR) += kdf_sp800108.o
> > +
> > +ifdef CONFIG_CRYPTO_TPM2_KEY
> > +$(obj)/tpm2_key.asn1.o: $(obj)/tpm2_key.asn1.h $(obj)/tpm2_key.asn1.c
> > +$(obj)/tpm2_key.o: $(obj)/tpm2_key.asn1.h
> > +obj-y += tpm2_key.o tpm2_key.asn1.o
> > +endif
> > diff --git a/crypto/tpm2_key.asn1 b/crypto/tpm2_key.asn1
> > new file mode 100644
> > index 000000000000..b235d02ab78e
> > --- /dev/null
> > +++ b/crypto/tpm2_key.asn1
> > @@ -0,0 +1,11 @@
> > +---
> > +--- ASN.1 for TPM 2.0 keys
> > +---
> > +
> > +TPMKey ::= SEQUENCE {
> > +	type		OBJECT IDENTIFIER ({tpm2_key_get_type}),
> > +	emptyAuth	[0] EXPLICIT BOOLEAN OPTIONAL,
> > +	parent		INTEGER ({tpm2_key_get_parent}),
> > +	pubkey		OCTET STRING ({tpm2_get_public}),
> > +	privkey		OCTET STRING ({tpm2_get_private})
> > +	}
> > diff --git a/crypto/tpm2_key.c b/crypto/tpm2_key.c
> > new file mode 100644
> > index 000000000000..78f55478d046
> > --- /dev/null
> > +++ b/crypto/tpm2_key.c
> > @@ -0,0 +1,134 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +
> > +#include <crypto/tpm2_key.h>
> > +#include <linux/oid_registry.h>
> > +#include <linux/slab.h>
> > +#include <linux/types.h>
> > +#include <asm/unaligned.h>
> > +#include "tpm2_key.asn1.h"
> > +
> > +#undef pr_fmt
> > +#define pr_fmt(fmt) "tpm2_key: "fmt
> > +
> > +struct tpm2_key_decoder_context {
> > +	u32 parent;
> > +	const u8 *pub;
> > +	u32 pub_len;
> > +	const u8 *priv;
> > +	u32 priv_len;
> > +	enum OID oid;
> > +};
> > +
> > +int tpm2_key_get_parent(void *context, size_t hdrlen,
> > +			unsigned char tag,
> > +			const void *value, size_t vlen)
> > +{
> > +	struct tpm2_key_decoder_context *decoder = context;
> > +	const u8 *v = value;
> > +	int i;
> > +
> > +	decoder->parent = 0;
> > +	for (i = 0; i < vlen; i++) {
> > +		decoder->parent <<= 8;
> > +		decoder->parent |= v[i];
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +int tpm2_key_get_type(void *context, size_t hdrlen,
> > +		      unsigned char tag,
> > +		      const void *value, size_t vlen)
> > +{
> > +	struct tpm2_key_decoder_context *decoder = context;
> > +
> > +	decoder->oid = look_up_OID(value, vlen);
> > +	return 0;
> > +}
> > +
> > +static inline bool tpm2_key_is_valid(const void *value, size_t vlen)
> > +{
> > +	if (vlen < 2 || vlen > TPM2_KEY_BYTES_MAX)
> > +		return false;
> > +
> > +	if (get_unaligned_be16(value) != vlen - 2)
> > +		return false;
> > +
> > +	return true;
> > +}
> > +
> > +int tpm2_get_public(void *context, size_t hdrlen, unsigned char tag,
> > +		    const void *value, size_t vlen)
> > +{
> > +	struct tpm2_key_decoder_context *decoder = context;
> > +
> > +	if (!tpm2_key_is_valid(value, vlen))
> > +		return -EBADMSG;
> > +
> > +	if (sizeof(struct tpm2_key_desc) > vlen - 2)
> > +		return -EBADMSG;
> > +
> > +	decoder->pub = value;
> > +	decoder->pub_len = vlen;
> > +	return 0;
> > +}
> > +
> > +int tpm2_get_private(void *context, size_t hdrlen, unsigned char tag,
> > +		     const void *value, size_t vlen)
> > +{
> > +	struct tpm2_key_decoder_context *decoder = context;
> > +
> > +	if (!tpm2_key_is_valid(value, vlen))
> > +		return -EBADMSG;
> > +
> > +	decoder->priv = value;
> > +	decoder->priv_len = vlen;
> > +	return 0;
> > +}
> > +
> > +/**
> > + * tpm_key_decode() - Decode TPM2 ASN.1 key
> > + * @src:	ASN.1 source.
> > + * @src_len:	ASN.1 source length.
> > + *
> > + * Decodes the TPM2 ASN.1 key and validates that the public key data has all
> > + * the shared fields of TPMT_PUBLIC. This is full coverage of the memory that
> > + * can be validated before doing any key type specific validation.
>
> I am not sure what the last sentence means.

I think the whole paragraph should be rewritten.

So what it does is that it takes the private and public parts and
concanates them together so maybe just write:

"Load TPMT_PUBLIC and TPMT_PRIVATE from ASN.1 file, and concatenate the
blobs together as a single blob, as this is expected format for the TPM2
commands. In addition, validate TPMT_PUBLIC fields so that they make
sense for trusted and asymmetric keys."

What you think of this?


>
> > + *
> > + * Return:
> > + * - TPM2 ASN.1 key on success.
> > + * - -EBADMSG when decoding fails.
> > + * - -ENOMEM when OOM while allocating struct tpm2_key.
> > + */
> > +struct tpm2_key *tpm2_key_decode(const u8 *src, u32 src_len)
> > +{
> > +	struct tpm2_key_decoder_context decoder;
> > +	struct tpm2_key *key;
> > +	u8 *data;
> > +	int ret;
> > +
> > +	memset(&decoder, 0, sizeof(decoder));
> > +	ret = asn1_ber_decoder(&tpm2_key_decoder, &decoder, src, src_len);
> > +	if (ret < 0) {
> > +		if (ret != -EBADMSG)
> > +			pr_info("Decoder error %d\n", ret);
> > +
> > +		return ERR_PTR(-EBADMSG);
> > +	}
> > +
> > +	key = kzalloc(sizeof(*key), GFP_KERNEL);
> > +	if (!key)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	data = &key->data[0];
> > +	memcpy(&data[0], decoder.priv, decoder.priv_len);
> > +	memcpy(&data[decoder.priv_len], decoder.pub, decoder.pub_len);
> > +
> > +	key->oid = decoder.oid;
> > +	key->priv_len = decoder.priv_len;
> > +	key->pub_len = decoder.pub_len;
> > +	key->parent = decoder.parent;
> > +	key->desc = (struct tpm2_key_desc *)&data[decoder.priv_len + 2];
> > +	return key;
> > +}
> > +EXPORT_SYMBOL_GPL(tpm2_key_decode);
> > diff --git a/include/crypto/tpm2_key.h b/include/crypto/tpm2_key.h
> > new file mode 100644
> > index 000000000000..74debaf707bf
> > --- /dev/null
> > +++ b/include/crypto/tpm2_key.h
> > @@ -0,0 +1,46 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +#ifndef __LINUX_TPM2_KEY_H__
> > +#define __LINUX_TPM2_KEY_H__
> > +
> > +#include <linux/oid_registry.h>
> > +#include <linux/slab.h>
> > +
> > +#define TPM2_KEY_BYTES_MAX 1024
> > +
> > +/*  TPM2 Structures 12.2.4: TPMT_PUBLIC */
> > +struct tpm2_key_desc {
> > +	__be16 type;
> > +	__be16 name_alg;
> > +	__be32 object_attributes;
> > +	__be16 policy_size;
> > +} __packed;
> > +
> > +/* Decoded TPM2 ASN.1 key. */
> > +struct tpm2_key {
> > +	u8 data[2 * TPM2_KEY_BYTES_MAX];
> > +	struct tpm2_key_desc *desc;
> > +	u16 priv_len;
> > +	u16 pub_len;
> > +	u32 parent;
> > +	enum OID oid;
> > +	char oid_str[64];
> > +};
> > +
> > +struct tpm2_key *tpm2_key_decode(const u8 *src, u32 src_len);
> > +
> > +static inline const void *tpm2_key_data(const struct tpm2_key *key)
> > +{
> > +	return &key->data[0];
> > +}
> > +
> > +static inline u16 tpm2_key_type(const struct tpm2_key *key)
> > +{
> > +	return be16_to_cpu(key->desc->type);
> > +}
> > +
> > +static inline int tpm2_key_policy_size(const struct tpm2_key *key)
> > +{
> > +	return be16_to_cpu(key->desc->policy_size);
> > +}
> > +
> > +#endif /* __LINUX_TPM2_KEY_H__ */
> > diff --git a/security/keys/trusted-keys/Kconfig b/security/keys/trusted-keys/Kconfig
> > index 1fb8aa001995..00d9489384ac 100644
> > --- a/security/keys/trusted-keys/Kconfig
> > +++ b/security/keys/trusted-keys/Kconfig
> > @@ -9,9 +9,9 @@ config TRUSTED_KEYS_TPM
> >   	select CRYPTO_HMAC
> >   	select CRYPTO_SHA1
> >   	select CRYPTO_HASH_INFO
> > +	select CRYPTO_TPM2_KEY
> >   	select ASN1_ENCODER
> >   	select OID_REGISTRY
> > -	select ASN1
> >   	select HAVE_TRUSTED_KEYS
> >   	help
> >   	  Enable use of the Trusted Platform Module (TPM) as trusted key
> > diff --git a/security/keys/trusted-keys/Makefile b/security/keys/trusted-keys/Makefile
> > index f0f3b27f688b..2674d5c10fc9 100644
> > --- a/security/keys/trusted-keys/Makefile
> > +++ b/security/keys/trusted-keys/Makefile
> > @@ -7,9 +7,7 @@ obj-$(CONFIG_TRUSTED_KEYS) += trusted.o
> >   trusted-y += trusted_core.o
> >   trusted-$(CONFIG_TRUSTED_KEYS_TPM) += trusted_tpm1.o
> >   
> > -$(obj)/trusted_tpm2.o: $(obj)/tpm2key.asn1.h
> >   trusted-$(CONFIG_TRUSTED_KEYS_TPM) += trusted_tpm2.o
> > -trusted-$(CONFIG_TRUSTED_KEYS_TPM) += tpm2key.asn1.o
> >   
> >   trusted-$(CONFIG_TRUSTED_KEYS_TEE) += trusted_tee.o
> >   
> > diff --git a/security/keys/trusted-keys/tpm2key.asn1 b/security/keys/trusted-keys/tpm2key.asn1
> > deleted file mode 100644
> > index f57f869ad600..000000000000
> > --- a/security/keys/trusted-keys/tpm2key.asn1
> > +++ /dev/null
> > @@ -1,11 +0,0 @@
> > ----
> > ---- ASN.1 for TPM 2.0 keys
> > ----
> > -
> > -TPMKey ::= SEQUENCE {
> > -	type		OBJECT IDENTIFIER ({tpm2_key_type}),
> > -	emptyAuth	[0] EXPLICIT BOOLEAN OPTIONAL,
> > -	parent		INTEGER ({tpm2_key_parent}),
> > -	pubkey		OCTET STRING ({tpm2_key_pub}),
> > -	privkey		OCTET STRING ({tpm2_key_priv})
> > -	}
> > diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c
> > index 06c8fa7b21ae..b9e505e99e8c 100644
> > --- a/security/keys/trusted-keys/trusted_tpm2.c
> > +++ b/security/keys/trusted-keys/trusted_tpm2.c
> > @@ -13,11 +13,10 @@
> >   
> >   #include <keys/trusted-type.h>
> >   #include <keys/trusted_tpm.h>
> > +#include <crypto/tpm2_key.h>
> >   
> >   #include <asm/unaligned.h>
> >   
> > -#include "tpm2key.asn1.h"
> > -
> >   static struct tpm2_hash tpm2_hash_map[] = {
> >   	{HASH_ALGO_SHA1, TPM_ALG_SHA1},
> >   	{HASH_ALGO_SHA256, TPM_ALG_SHA256},
> > @@ -98,106 +97,6 @@ static int tpm2_key_encode(struct trusted_key_payload *payload,
> >   	return ret;
> >   }
> >   
> > -struct tpm2_key_context {
> > -	u32 parent;
> > -	const u8 *pub;
> > -	u32 pub_len;
> > -	const u8 *priv;
> > -	u32 priv_len;
> > -};
> > -
> > -static int tpm2_key_decode(struct trusted_key_payload *payload,
> > -			   struct trusted_key_options *options,
> > -			   u8 **buf)
> > -{
> > -	int ret;
> > -	struct tpm2_key_context ctx;
> > -	u8 *blob;
> > -
> > -	memset(&ctx, 0, sizeof(ctx));
> > -
> > -	ret = asn1_ber_decoder(&tpm2key_decoder, &ctx, payload->blob,
> > -			       payload->blob_len);
> > -	if (ret < 0)
> > -		return ret;
> > -
> > -	if (ctx.priv_len + ctx.pub_len > MAX_BLOB_SIZE)
> > -		return -E2BIG;
> > -
> > -	blob = kmalloc(ctx.priv_len + ctx.pub_len + 4, GFP_KERNEL);
> > -	if (!blob)
> > -		return -ENOMEM;
> > -
> > -	*buf = blob;
> > -	options->keyhandle = ctx.parent;
> > -
> > -	memcpy(blob, ctx.priv, ctx.priv_len);
> > -	blob += ctx.priv_len;
> > -
> > -	memcpy(blob, ctx.pub, ctx.pub_len);
> > -
> > -	return 0;
> > -}
> > -
> > -int tpm2_key_parent(void *context, size_t hdrlen,
> > -		  unsigned char tag,
> > -		  const void *value, size_t vlen)
> > -{
> > -	struct tpm2_key_context *ctx = context;
> > -	const u8 *v = value;
> > -	int i;
> > -
> > -	ctx->parent = 0;
> > -	for (i = 0; i < vlen; i++) {
> > -		ctx->parent <<= 8;
> > -		ctx->parent |= v[i];
> > -	}
> > -
> > -	return 0;
> > -}
> > -
> > -int tpm2_key_type(void *context, size_t hdrlen,
> > -		unsigned char tag,
> > -		const void *value, size_t vlen)
> > -{
> > -	enum OID oid = look_up_OID(value, vlen);
> > -
> > -	if (oid != OID_TPMSealedData) {
> > -		char buffer[50];
> > -
> > -		sprint_oid(value, vlen, buffer, sizeof(buffer));
> > -		pr_debug("OID is \"%s\" which is not TPMSealedData\n",
> > -			 buffer);
> > -		return -EINVAL;
> > -	}
> > -
> > -	return 0;
> > -}
> > -
> > -int tpm2_key_pub(void *context, size_t hdrlen,
> > -	       unsigned char tag,
> > -	       const void *value, size_t vlen)
> > -{
> > -	struct tpm2_key_context *ctx = context;
> > -
> > -	ctx->pub = value;
> > -	ctx->pub_len = vlen;
> > -
> > -	return 0;
> > -}
> > -
> > -int tpm2_key_priv(void *context, size_t hdrlen,
> > -		unsigned char tag,
> > -		const void *value, size_t vlen)
> > -{
> > -	struct tpm2_key_context *ctx = context;
> > -
> > -	ctx->priv = value;
> > -	ctx->priv_len = vlen;
> > -
> > -	return 0;
> > -}
> > -
> >   /**
> >    * tpm2_buf_append_auth() - append TPMS_AUTH_COMMAND to the buffer.
> >    *
> > @@ -387,22 +286,43 @@ static int tpm2_load_cmd(struct tpm_chip *chip,
> >   			 struct trusted_key_options *options,
> >   			 u32 *blob_handle)
> >   {
> > -	struct tpm_buf buf;
> >   	unsigned int private_len;
> >   	unsigned int public_len;
> >   	unsigned int blob_len;
> > -	u8 *blob, *pub;
> > -	int rc;
> > +	struct tpm2_key *key;
> > +	const u8 *blob, *pub;
> > +	struct tpm_buf buf;
> >   	u32 attrs;
> > +	int rc;
> >   
> > -	rc = tpm2_key_decode(payload, options, &blob);
> > -	if (rc) {
> > -		/* old form */
> > +	key = tpm2_key_decode(payload->blob, payload->blob_len);
> > +	if (IS_ERR(key)) {
> > +		/* Get the error code and reset the pointer to the key: */
> > +		rc = PTR_ERR(key);
> > +		key = NULL;
> > +
> > +		if (rc == -ENOMEM)
> > +			return -ENOMEM;
> > +
> > +		/* A sanity check, as only -EBADMSG or -ENOMEM are expected: */
> > +		if (rc != -EBADMSG)
> > +			pr_err("tpm2_key_decode(): spurious error code %d\n", rc);
>
> tpm2_key_decode seems simple enough that it only returns key, -ENOMEM or 
> EBADMSG.

So what is your suggestion here?

The reasoning here is that asymmetric keys use -EBADMSG not only as
error but also iterator, when probing which can load a specific key.

>
> > +
> > +		/* Fallback to the legacy format: */
> >   		blob = payload->blob;
> >   		payload->old_format = 1;
> > +	} else {
> > +		blob = tpm2_key_data(key);
> > +		if (key->oid != OID_TPMSealedData) {
> > +			kfree(key);
> > +			return -EBADMSG;
> > +		}
> >   	}
> >   
> > -	/* new format carries keyhandle but old format doesn't */
> > +	/*
> > +	 * Must be non-zero here, either extracted from the ASN.1 for the new
> > +	 * format or specified on the command line for the old.
>
> sentence seems incomplete: ... for the old one.  OR  ... for the old format.

Yep, I think it is a plain mistake.

>
> > +	 */
> >   	if (!options->keyhandle)
> >   		return -EINVAL;
> >   
> > @@ -464,8 +384,7 @@ static int tpm2_load_cmd(struct tpm_chip *chip,
> >   			(__be32 *) &buf.data[TPM_HEADER_SIZE]);
> >   
> >   out:
> > -	if (blob != payload->blob)
> > -		kfree(blob);
> > +	kfree(key);
> >   	tpm_buf_destroy(&buf);
> >   
> >   	if (rc > 0)

Thanks for the feedback.

BR, Jarkko

Re: [PATCH v7 3/5] crypto: tpm2_key: Introduce a TPM2 key type

[Stefan Berger]

On 6/4/24 13:23, Jarkko Sakkinen wrote:
> On Fri May 31, 2024 at 3:35 AM EEST, Stefan Berger wrote:
>>

>>>    
>>> -	rc = tpm2_key_decode(payload, options, &blob);
>>> -	if (rc) {
>>> -		/* old form */
>>> +	key = tpm2_key_decode(payload->blob, payload->blob_len);
>>> +	if (IS_ERR(key)) {
>>> +		/* Get the error code and reset the pointer to the key: */
>>> +		rc = PTR_ERR(key);
>>> +		key = NULL;
>>> +
>>> +		if (rc == -ENOMEM)
>>> +			return -ENOMEM;
>>> +
>>> +		/* A sanity check, as only -EBADMSG or -ENOMEM are expected: */
>>> +		if (rc != -EBADMSG)
>>> +			pr_err("tpm2_key_decode(): spurious error code %d\n", rc);
>>
>> tpm2_key_decode seems simple enough that it only returns key, -ENOMEM or
>> EBADMSG.
> 
> So what is your suggestion here?

You can remove the check resuling in pr_err().

> 
> The reasoning here is that asymmetric keys use -EBADMSG not only as
> error but also iterator, when probing which can load a specific key.
> 

Re: [PATCH v7 3/5] crypto: tpm2_key: Introduce a TPM2 key type

[Jarkko Sakkinen]

On Tue Jun 4, 2024 at 9:41 PM EEST, Stefan Berger wrote:
>
>
> On 6/4/24 13:23, Jarkko Sakkinen wrote:
> > On Fri May 31, 2024 at 3:35 AM EEST, Stefan Berger wrote:
> >>
>
> >>>    
> >>> -	rc = tpm2_key_decode(payload, options, &blob);
> >>> -	if (rc) {
> >>> -		/* old form */
> >>> +	key = tpm2_key_decode(payload->blob, payload->blob_len);
> >>> +	if (IS_ERR(key)) {
> >>> +		/* Get the error code and reset the pointer to the key: */
> >>> +		rc = PTR_ERR(key);
> >>> +		key = NULL;
> >>> +
> >>> +		if (rc == -ENOMEM)
> >>> +			return -ENOMEM;
> >>> +
> >>> +		/* A sanity check, as only -EBADMSG or -ENOMEM are expected: */
> >>> +		if (rc != -EBADMSG)
> >>> +			pr_err("tpm2_key_decode(): spurious error code %d\n", rc);
> >>
> >> tpm2_key_decode seems simple enough that it only returns key, -ENOMEM or
> >> EBADMSG.
> > 
> > So what is your suggestion here?
>
> You can remove the check resuling in pr_err().

OK, I think so too. Just had to (sanity) check.

>
> > 
> > The reasoning here is that asymmetric keys use -EBADMSG not only as
> > error but also iterator, when probing which can load a specific key.
> > 

BR, Jarkko