[PATCH v2 2/6] lib: Expand asn1_encode_integer() to variable size integers

[PATCH v2 2/6] lib: Expand asn1_encode_integer() to variable size integers

[Jarkko Sakkinen]

Expand asn1_encode_integer() to variable size integers, meaning that it
will get a blob in big-endian format as integer and length of the blob as
parameters. This is required in order to encode RSA public key modulus.

Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---
 include/linux/asn1_encoder.h              |   3 +-
 lib/asn1_encoder.c                        | 185 ++++++++++++----------
 security/keys/trusted-keys/trusted_tpm2.c |   4 +-
 3 files changed, 103 insertions(+), 89 deletions(-)

diff --git a/include/linux/asn1_encoder.h b/include/linux/asn1_encoder.h
index 08cd0c2ad34f..ad5fb18db9e2 100644
--- a/include/linux/asn1_encoder.h
+++ b/include/linux/asn1_encoder.h
@@ -9,9 +9,10 @@
 #include <linux/bug.h>
 
 #define asn1_oid_len(oid) (sizeof(oid)/sizeof(u32))
+
 unsigned char *
 asn1_encode_integer(unsigned char *data, const unsigned char *end_data,
-		    s64 integer);
+		    const u8 *integer, int integer_len);
 unsigned char *
 asn1_encode_oid(unsigned char *data, const unsigned char *end_data,
 		u32 oid[], int oid_len);
diff --git a/lib/asn1_encoder.c b/lib/asn1_encoder.c
index 0fd3c454a468..51a2d7010a67 100644
--- a/lib/asn1_encoder.c
+++ b/lib/asn1_encoder.c
@@ -9,12 +9,78 @@
 #include <linux/bug.h>
 #include <linux/string.h>
 #include <linux/module.h>
+#include <linux/slab.h>
+
+/**
+ * asn1_encode_length() - encode a length to follow an ASN.1 tag
+ * @data: pointer to encode at
+ * @data_len: pointer to remaining length (adjusted by routine)
+ * @len: length to encode
+ *
+ * This routine can encode lengths up to 65535 using the ASN.1 rules.
+ * It will accept a negative length and place a zero length tag
+ * instead (to keep the ASN.1 valid).  This convention allows other
+ * encoder primitives to accept negative lengths as singalling the
+ * sequence will be re-encoded when the length is known.
+ */
+static int asn1_encode_length(unsigned char **data, int *data_len, int len)
+{
+	if (*data_len < 1)
+		return -EINVAL;
+
+	if (len < 0) {
+		*((*data)++) = 0;
+		(*data_len)--;
+		return 0;
+	}
+
+	if (len <= 0x7f) {
+		*((*data)++) = len;
+		(*data_len)--;
+		return 0;
+	}
+
+	if (*data_len < 2)
+		return -EINVAL;
+
+	if (len <= 0xff) {
+		*((*data)++) = 0x81;
+		*((*data)++) = len & 0xff;
+		*data_len -= 2;
+		return 0;
+	}
+
+	if (*data_len < 3)
+		return -EINVAL;
+
+	if (len <= 0xffff) {
+		*((*data)++) = 0x82;
+		*((*data)++) = (len >> 8) & 0xff;
+		*((*data)++) = len & 0xff;
+		*data_len -= 3;
+		return 0;
+	}
+
+	if (WARN(len > 0xffffff, "ASN.1 length can't be > 0xffffff"))
+		return -EINVAL;
+
+	if (*data_len < 4)
+		return -EINVAL;
+	*((*data)++) = 0x83;
+	*((*data)++) = (len >> 16) & 0xff;
+	*((*data)++) = (len >> 8) & 0xff;
+	*((*data)++) = len & 0xff;
+	*data_len -= 4;
+
+	return 0;
+}
 
 /**
  * asn1_encode_integer() - encode positive integer to ASN.1
- * @data:	pointer to the pointer to the data
- * @end_data:	end of data pointer, points one beyond last usable byte in @data
- * @integer:	integer to be encoded
+ * @data:		pointer to the pointer to the data
+ * @end_data:		end of data pointer, points one beyond last usable byte in @data
+ * @integer:		integer to be encoded
+ * @integer_len:	length in bytes of the integer blob
  *
  * This is a simplified encoder: it only currently does
  * positive integers, but it should be simple enough to add the
@@ -22,15 +88,17 @@
  */
 unsigned char *
 asn1_encode_integer(unsigned char *data, const unsigned char *end_data,
-		    s64 integer)
+		    const u8 *integer, int integer_len)
 {
 	int data_len = end_data - data;
-	unsigned char *d = &data[2];
 	bool found = false;
+	unsigned char *d;
+	int encoded_len;
+	u8 *encoded;
+	int ret;
 	int i;
 
-	if (WARN(integer < 0,
-		 "BUG: integer encode only supports positive integers"))
+	if (WARN(!integer, "BUG: integer is null"))
 		return ERR_PTR(-EINVAL);
 
 	if (IS_ERR(data))
@@ -40,17 +108,22 @@ asn1_encode_integer(unsigned char *data, const unsigned char *end_data,
 	if (data_len < 3)
 		return ERR_PTR(-EINVAL);
 
-	/* remaining length where at d (the start of the integer encoding) */
-	data_len -= 2;
+	(*data++) = _tag(UNIV, PRIM, INT);
+	data_len--;
 
-	data[0] = _tag(UNIV, PRIM, INT);
-	if (integer == 0) {
-		*d++ = 0;
-		goto out;
+	if (!memchr_inv(integer, 0, integer_len)) {
+		data[1] = 1;
+		data[2] = 0;
+		return &data[2];
 	}
 
-	for (i = sizeof(integer); i > 0 ; i--) {
-		int byte = integer >> (8 * (i - 1));
+	encoded = kzalloc(integer_len, GFP_KERNEL);
+	if (!encoded)
+		return ERR_PTR(-ENOMEM);
+	d = encoded;
+
+	for (i = 0; i < integer_len; i++) {
+		int byte = integer[i];
 
 		if (!found && byte == 0)
 			continue;
@@ -67,21 +140,23 @@ asn1_encode_integer(unsigned char *data, const unsigned char *end_data,
 			 * have len >= 1
 			 */
 			*d++ = 0;
-			data_len--;
 		}
 
 		found = true;
-		if (data_len == 0)
-			return ERR_PTR(-EINVAL);
-
 		*d++ = byte;
-		data_len--;
 	}
 
- out:
-	data[1] = d - data - 2;
+	encoded_len = d - encoded;
 
-	return d;
+	ret = asn1_encode_length(&data, &data_len, encoded_len);
+	if (ret)  {
+		kfree(encoded);
+		return ERR_PTR(ret);
+	}
+
+	memcpy(data, encoded, encoded_len);
+	kfree(encoded);
+	return data + encoded_len;
 }
 EXPORT_SYMBOL_GPL(asn1_encode_integer);
 
@@ -176,70 +251,6 @@ asn1_encode_oid(unsigned char *data, const unsigned char *end_data,
 }
 EXPORT_SYMBOL_GPL(asn1_encode_oid);
 
-/**
- * asn1_encode_length() - encode a length to follow an ASN.1 tag
- * @data: pointer to encode at
- * @data_len: pointer to remaining length (adjusted by routine)
- * @len: length to encode
- *
- * This routine can encode lengths up to 65535 using the ASN.1 rules.
- * It will accept a negative length and place a zero length tag
- * instead (to keep the ASN.1 valid).  This convention allows other
- * encoder primitives to accept negative lengths as singalling the
- * sequence will be re-encoded when the length is known.
- */
-static int asn1_encode_length(unsigned char **data, int *data_len, int len)
-{
-	if (*data_len < 1)
-		return -EINVAL;
-
-	if (len < 0) {
-		*((*data)++) = 0;
-		(*data_len)--;
-		return 0;
-	}
-
-	if (len <= 0x7f) {
-		*((*data)++) = len;
-		(*data_len)--;
-		return 0;
-	}
-
-	if (*data_len < 2)
-		return -EINVAL;
-
-	if (len <= 0xff) {
-		*((*data)++) = 0x81;
-		*((*data)++) = len & 0xff;
-		*data_len -= 2;
-		return 0;
-	}
-
-	if (*data_len < 3)
-		return -EINVAL;
-
-	if (len <= 0xffff) {
-		*((*data)++) = 0x82;
-		*((*data)++) = (len >> 8) & 0xff;
-		*((*data)++) = len & 0xff;
-		*data_len -= 3;
-		return 0;
-	}
-
-	if (WARN(len > 0xffffff, "ASN.1 length can't be > 0xffffff"))
-		return -EINVAL;
-
-	if (*data_len < 4)
-		return -EINVAL;
-	*((*data)++) = 0x83;
-	*((*data)++) = (len >> 16) & 0xff;
-	*((*data)++) = (len >> 8) & 0xff;
-	*((*data)++) = len & 0xff;
-	*data_len -= 4;
-
-	return 0;
-}
-
 /**
  * asn1_encode_tag() - add a tag for optional or explicit value
  * @data:	pointer to place tag at
diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c
index 8b7dd73d94c1..ec59f9389a2d 100644
--- a/security/keys/trusted-keys/trusted_tpm2.c
+++ b/security/keys/trusted-keys/trusted_tpm2.c
@@ -38,6 +38,7 @@ static int tpm2_key_encode(struct trusted_key_payload *payload,
 	u8 *end_work = scratch + SCRATCH_SIZE;
 	u8 *priv, *pub;
 	u16 priv_len, pub_len;
+	u32 key_handle;
 	int ret;
 
 	priv_len = get_unaligned_be16(src) + 2;
@@ -77,7 +78,8 @@ static int tpm2_key_encode(struct trusted_key_payload *payload,
 		goto err;
 	}
 
-	work = asn1_encode_integer(work, end_work, options->keyhandle);
+	key_handle = cpu_to_be32(options->keyhandle);
+	work = asn1_encode_integer(work, end_work, (u8 *)&key_handle, 4);
 	work = asn1_encode_octet_string(work, end_work, pub, pub_len);
 	work = asn1_encode_octet_string(work, end_work, priv, priv_len);
 
-- 
2.45.1

[PATCH v2 4/6] KEYS: trusted: Move tpm2_key_decode() to the TPM driver

[Jarkko Sakkinen]

Move tpm2_key_decode() to the TPM driver and export the symbols to make
them callable from trusted keys. It can re-used for asymmetric keys.

Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---
v2:
Do not allocate blob twice. Use the one inside struct tpm2_key.
---
 drivers/char/tpm/Kconfig                      |   1 +
 drivers/char/tpm/Makefile                     |   5 +
 drivers/char/tpm/tpm2_key.c                   | 111 +++++++++++++++
 .../char/tpm}/tpm2key.asn1                    |   0
 include/crypto/tpm2_key.h                     |  33 +++++
 security/keys/trusted-keys/Makefile           |   2 -
 security/keys/trusted-keys/trusted_tpm2.c     | 127 +++---------------
 7 files changed, 167 insertions(+), 112 deletions(-)
 create mode 100644 drivers/char/tpm/tpm2_key.c
 rename {security/keys/trusted-keys => drivers/char/tpm}/tpm2key.asn1 (100%)
 create mode 100644 include/crypto/tpm2_key.h

diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
index e63a6a17793c..de2f4093c939 100644
--- a/drivers/char/tpm/Kconfig
+++ b/drivers/char/tpm/Kconfig
@@ -7,6 +7,7 @@ menuconfig TCG_TPM
 	tristate "TPM Hardware Support"
 	depends on HAS_IOMEM
 	imply SECURITYFS
+	select ASN1
 	select CRYPTO
 	select CRYPTO_HASH_INFO
 	help
diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
index 4c695b0388f3..071437058ef6 100644
--- a/drivers/char/tpm/Makefile
+++ b/drivers/char/tpm/Makefile
@@ -17,6 +17,11 @@ tpm-y += eventlog/tpm1.o
 tpm-y += eventlog/tpm2.o
 tpm-y += tpm-buf.o
 
+# TPM2 Asymmetric Key
+$(obj)/trusted_tpm2.o: $(obj)/tpm2key.asn1.h
+tpm-y += tpm2key.asn1.o
+tpm-y += tpm2_key.o
+
 tpm-$(CONFIG_TCG_TPM2_HMAC) += tpm2-sessions.o
 tpm-$(CONFIG_ACPI) += tpm_ppi.o eventlog/acpi.o
 tpm-$(CONFIG_EFI) += eventlog/efi.o
diff --git a/drivers/char/tpm/tpm2_key.c b/drivers/char/tpm/tpm2_key.c
new file mode 100644
index 000000000000..0112362e432e
--- /dev/null
+++ b/drivers/char/tpm/tpm2_key.c
@@ -0,0 +1,111 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <linux/oid_registry.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+#include <crypto/tpm2_key.h>
+#include <asm/unaligned.h>
+#include "tpm2key.asn1.h"
+
+#undef pr_fmt
+#define pr_fmt(fmt) "tpm2_key: "fmt
+
+int tpm2_key_parent(void *context, size_t hdrlen,
+		    unsigned char tag,
+		    const void *value, size_t vlen)
+{
+	struct tpm2_key *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 *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 *ctx = context;
+
+	ctx->priv = value;
+	ctx->priv_len = vlen;
+
+	return 0;
+}
+
+/**
+ * tpm_key_decode() - Decode TPM2 ASN.1 key.
+ * @src:		ASN.1 source.
+ * @src_len:		ASN.1 source length.
+ * @key:		TPM2 asymmetric key.
+ * @max_key_len:	Maximum length of the TPM2 asymmetric key.
+ *
+ * Decodes TPM2 ASN.1 key on success. Returns POSIX error code on failure.
+ */
+int tpm2_key_decode(const u8 *src, u32 src_len, struct tpm2_key *key,
+		    u32 max_key_len)
+{
+	struct tpm2_key ctx;
+	u32 blob_len;
+	int ret;
+
+	memset(&ctx, 0, sizeof(ctx));
+
+	ret = asn1_ber_decoder(&tpm2key_decoder, &ctx, src, src_len);
+	if (ret < 0)
+		return ret;
+
+	blob_len = ctx.priv_len + ctx.pub_len;
+	if (blob_len > max_key_len)
+		return -E2BIG;
+
+	ctx.blob_len = blob_len;
+	ctx.blob = kmalloc(blob_len, GFP_KERNEL);
+	if (!ctx.blob)
+		return -ENOMEM;
+
+	memcpy((void *)ctx.blob, ctx.priv, ctx.priv_len);
+	memcpy((void *)ctx.blob + ctx.priv_len, ctx.pub, ctx.pub_len);
+	ctx.priv = ctx.blob;
+	ctx.pub = ctx.blob + ctx.priv_len;
+
+	memcpy(key, &ctx, sizeof(ctx));
+	return 0;
+}
+EXPORT_SYMBOL_GPL(tpm2_key_decode);
diff --git a/security/keys/trusted-keys/tpm2key.asn1 b/drivers/char/tpm/tpm2key.asn1
similarity index 100%
rename from security/keys/trusted-keys/tpm2key.asn1
rename to drivers/char/tpm/tpm2key.asn1
diff --git a/include/crypto/tpm2_key.h b/include/crypto/tpm2_key.h
new file mode 100644
index 000000000000..acf41b2e0c92
--- /dev/null
+++ b/include/crypto/tpm2_key.h
@@ -0,0 +1,33 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+#ifndef __LINUX_TPM2_KEY_H__
+#define __LINUX_TPM2_KEY_H__
+
+#include <linux/slab.h>
+
+/*
+ * TPM2 ASN.1 key
+ */
+struct tpm2_key {
+	u32 parent;
+	const u8 *blob;
+	u32 blob_len;
+	const u8 *pub;
+	u32 pub_len;
+	const u8 *priv;
+	u32 priv_len;
+};
+
+int tpm2_key_decode(const u8 *src, u32 src_len, struct tpm2_key *key,
+		    u32 max_key_len);
+
+/**
+ * tpm2_key_free() - Release TPM2 asymmetric key resources and reset values
+ * @key:	TPM2 asymmetric key.
+ */
+static inline void tpm2_key_destroy(struct tpm2_key *key)
+{
+	kfree(key->blob);
+	memset(key, 0, sizeof(*key));
+}
+
+#endif /* __LINUX_TPM2_KEY_H__ */
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/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c
index ec59f9389a2d..f255388d32b8 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},
@@ -28,9 +27,9 @@ static struct tpm2_hash tpm2_hash_map[] = {
 
 static u32 tpm2key_oid[] = { 2, 23, 133, 10, 1, 5 };
 
-static int tpm2_key_encode(struct trusted_key_payload *payload,
-			   struct trusted_key_options *options,
-			   u8 *src, u32 len)
+static int tpm2_trusted_key_encode(struct trusted_key_payload *payload,
+				   struct trusted_key_options *options,
+				   u8 *src, u32 len)
 {
 	const int SCRATCH_SIZE = PAGE_SIZE;
 	u8 *scratch = kmalloc(SCRATCH_SIZE, GFP_KERNEL);
@@ -100,106 +99,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 -EINVAL;
-
-	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.
  *
@@ -349,7 +248,8 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
 		goto out;
 	}
 
-	blob_len = tpm2_key_encode(payload, options, &buf.data[offset], blob_len);
+	blob_len = tpm2_trusted_key_encode(payload, options, &buf.data[offset],
+					   blob_len);
 
 out:
 	tpm_buf_destroy(&sized);
@@ -389,21 +289,27 @@ 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;
+	struct tpm2_key key;
+	struct tpm_buf buf;
+	const u8 *blob, *pub;
 	int rc;
 	u32 attrs;
 
-	rc = tpm2_key_decode(payload, options, &blob);
+	rc = tpm2_key_decode(payload->blob, payload->blob_len, &key, PAGE_SIZE);
 	if (rc) {
 		/* old form */
 		blob = payload->blob;
 		payload->old_format = 1;
+	} else {
+		blob = key.blob;
 	}
 
+	if (!blob)
+		return -ENOMEM;
+
 	/* new format carries keyhandle but old format doesn't */
 	if (!options->keyhandle)
 		return -EINVAL;
@@ -467,7 +373,8 @@ static int tpm2_load_cmd(struct tpm_chip *chip,
 
 out:
 	if (blob != payload->blob)
-		kfree(blob);
+		tpm2_key_destroy(&key);
+
 	tpm_buf_destroy(&buf);
 
 	if (rc > 0)
-- 
2.45.1

[PATCH v2 5/6] tpm: tpm2_key: Extend parser to TPM_LoadableKey

[Jarkko Sakkinen]

Extend parser to TPM_LoadableKey. Add field for oid to struct tpm2_key
so that callers can differentiate different key types.

Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---
 drivers/char/tpm/tpm2_key.c               | 14 +++++++++++---
 include/crypto/tpm2_key.h                 |  2 ++
 security/keys/trusted-keys/trusted_tpm2.c |  4 ++++
 3 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/drivers/char/tpm/tpm2_key.c b/drivers/char/tpm/tpm2_key.c
index 0112362e432e..59797dc232f1 100644
--- a/drivers/char/tpm/tpm2_key.c
+++ b/drivers/char/tpm/tpm2_key.c
@@ -32,16 +32,24 @@ int tpm2_key_type(void *context, size_t hdrlen,
 		  const void *value, size_t vlen)
 {
 	enum OID oid = look_up_OID(value, vlen);
-
-	if (oid != OID_TPMSealedData) {
+	struct tpm2_key *key = context;
+
+	switch (oid) {
+	case OID_TPMSealedData:
+		pr_info("TPMSealedData\n");
+		break;
+	case OID_TPMLoadableKey:
+		pr_info("TPMLodableKey\n");
+		break;
+	default:
 		char buffer[50];
-
 		sprint_oid(value, vlen, buffer, sizeof(buffer));
 		pr_debug("OID is \"%s\" which is not TPMSealedData\n",
 			 buffer);
 		return -EINVAL;
 	}
 
+	key->oid = oid;
 	return 0;
 }
 
diff --git a/include/crypto/tpm2_key.h b/include/crypto/tpm2_key.h
index acf41b2e0c92..2d2434233000 100644
--- a/include/crypto/tpm2_key.h
+++ b/include/crypto/tpm2_key.h
@@ -2,12 +2,14 @@
 #ifndef __LINUX_TPM2_KEY_H__
 #define __LINUX_TPM2_KEY_H__
 
+#include <linux/oid_registry.h>
 #include <linux/slab.h>
 
 /*
  * TPM2 ASN.1 key
  */
 struct tpm2_key {
+	enum OID oid;
 	u32 parent;
 	const u8 *blob;
 	u32 blob_len;
diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c
index f255388d32b8..ce4c667c3ee3 100644
--- a/security/keys/trusted-keys/trusted_tpm2.c
+++ b/security/keys/trusted-keys/trusted_tpm2.c
@@ -305,6 +305,10 @@ static int tpm2_load_cmd(struct tpm_chip *chip,
 		payload->old_format = 1;
 	} else {
 		blob = key.blob;
+		if (key.oid != OID_TPMSealedData) {
+			tpm2_key_destroy(&key);
+			return -EINVAL;
+		}
 	}
 
 	if (!blob)
-- 
2.45.1

RE: [EXTERNAL] [PATCH v2 2/6] lib: Expand asn1_encode_integer() to variable size integers

[Bharat Bhushan]

> -----Original Message-----
> From: Jarkko Sakkinen <jarkko@kernel.org>
> Sent: Tuesday, May 21, 2024 8:46 AM
> To: Herbert Xu <herbert@gondor.apana.org.au>
> Cc: linux-integrity@vger.kernel.org; keyrings@vger.kernel.org;
> Andreas.Fuchs@infineon.com; James Prestwood <prestwoj@gmail.com>;
> David Woodhouse <dwmw2@infradead.org>; Eric Biggers
> <ebiggers@kernel.org>; James Bottomley
> <James.Bottomley@hansenpartnership.com>; Jarkko Sakkinen
> <jarkko@kernel.org>; David S. Miller <davem@davemloft.net>; open
> list:CRYPTO API <linux-crypto@vger.kernel.org>; open list <linux-
> kernel@vger.kernel.org>; Andrew Morton <akpm@linux-foundation.org>;
> James Bottomley <James.Bottomley@HansenPartnership.com>; Mimi Zohar
> <zohar@linux.ibm.com>; David Howells <dhowells@redhat.com>; Paul Moore
> <paul@paul-moore.com>; James Morris <jmorris@namei.org>; Serge E. Hallyn
> <serge@hallyn.com>; open list:SECURITY SUBSYSTEM <linux-security-
> module@vger.kernel.org>
> Subject: [EXTERNAL] [PATCH v2 2/6] lib: Expand asn1_encode_integer() to
> variable size integers
> 
> ----------------------------------------------------------------------
> Expand asn1_encode_integer() to variable size integers, meaning that it
> will get a blob in big-endian format as integer and length of the blob as
> parameters. This is required in order to encode RSA public key modulus.
> 
> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> ---
>  include/linux/asn1_encoder.h              |   3 +-
>  lib/asn1_encoder.c                        | 185 ++++++++++++----------
>  security/keys/trusted-keys/trusted_tpm2.c |   4 +-
>  3 files changed, 103 insertions(+), 89 deletions(-)
> 
> diff --git a/include/linux/asn1_encoder.h b/include/linux/asn1_encoder.h
> index 08cd0c2ad34f..ad5fb18db9e2 100644
> --- a/include/linux/asn1_encoder.h
> +++ b/include/linux/asn1_encoder.h
> @@ -9,9 +9,10 @@
>  #include <linux/bug.h>
> 
>  #define asn1_oid_len(oid) (sizeof(oid)/sizeof(u32))
> +
>  unsigned char *
>  asn1_encode_integer(unsigned char *data, const unsigned char *end_data,
> -		    s64 integer);
> +		    const u8 *integer, int integer_len);
>  unsigned char *
>  asn1_encode_oid(unsigned char *data, const unsigned char *end_data,
>  		u32 oid[], int oid_len);
> diff --git a/lib/asn1_encoder.c b/lib/asn1_encoder.c
> index 0fd3c454a468..51a2d7010a67 100644
> --- a/lib/asn1_encoder.c
> +++ b/lib/asn1_encoder.c
> @@ -9,12 +9,78 @@
>  #include <linux/bug.h>
>  #include <linux/string.h>
>  #include <linux/module.h>
> +#include <linux/slab.h>
> +
> +/**
> + * asn1_encode_length() - encode a length to follow an ASN.1 tag
> + * @data: pointer to encode at
> + * @data_len: pointer to remaining length (adjusted by routine)
> + * @len: length to encode
> + *
> + * This routine can encode lengths up to 65535 using the ASN.1 rules.
> + * It will accept a negative length and place a zero length tag
> + * instead (to keep the ASN.1 valid).  This convention allows other
> + * encoder primitives to accept negative lengths as singalling the
> + * sequence will be re-encoded when the length is known.
> + */
> +static int asn1_encode_length(unsigned char **data, int *data_len, int len)
> +{
> +	if (*data_len < 1)
> +		return -EINVAL;
> +
> +	if (len < 0) {
> +		*((*data)++) = 0;
> +		(*data_len)--;
> +		return 0;
> +	}
> +
> +	if (len <= 0x7f) {
> +		*((*data)++) = len;
> +		(*data_len)--;
> +		return 0;
> +	}
> +
> +	if (*data_len < 2)
> +		return -EINVAL;
> +
> +	if (len <= 0xff) {
> +		*((*data)++) = 0x81;
> +		*((*data)++) = len & 0xff;
> +		*data_len -= 2;
> +		return 0;
> +	}
> +
> +	if (*data_len < 3)
> +		return -EINVAL;
> +
> +	if (len <= 0xffff) {
> +		*((*data)++) = 0x82;
> +		*((*data)++) = (len >> 8) & 0xff;
> +		*((*data)++) = len & 0xff;
> +		*data_len -= 3;
> +		return 0;
> +	}
> +
> +	if (WARN(len > 0xffffff, "ASN.1 length can't be > 0xffffff"))
> +		return -EINVAL;
> +
> +	if (*data_len < 4)
> +		return -EINVAL;
> +	*((*data)++) = 0x83;
> +	*((*data)++) = (len >> 16) & 0xff;
> +	*((*data)++) = (len >> 8) & 0xff;
> +	*((*data)++) = len & 0xff;
> +	*data_len -= 4;
> +
> +	return 0;
> +}
> 
>  /**
>   * asn1_encode_integer() - encode positive integer to ASN.1
> - * @data:	pointer to the pointer to the data
> - * @end_data:	end of data pointer, points one beyond last usable byte in
> @data
> - * @integer:	integer to be encoded
> + * @data:		pointer to the pointer to the data
> + * @end_data:		end of data pointer, points one beyond last usable
> byte in @data
> + * @integer:		integer to be encoded
> + * @integer_len:	length in bytes of the integer blob
>   *
>   * This is a simplified encoder: it only currently does
>   * positive integers, but it should be simple enough to add the
> @@ -22,15 +88,17 @@
>   */
>  unsigned char *
>  asn1_encode_integer(unsigned char *data, const unsigned char *end_data,
> -		    s64 integer)
> +		    const u8 *integer, int integer_len)
>  {
>  	int data_len = end_data - data;
> -	unsigned char *d = &data[2];
>  	bool found = false;
> +	unsigned char *d;
> +	int encoded_len;
> +	u8 *encoded;
> +	int ret;
>  	int i;
> 
> -	if (WARN(integer < 0,
> -		 "BUG: integer encode only supports positive integers"))
> +	if (WARN(!integer, "BUG: integer is null"))
>  		return ERR_PTR(-EINVAL);
> 
>  	if (IS_ERR(data))
> @@ -40,17 +108,22 @@ asn1_encode_integer(unsigned char *data, const
> unsigned char *end_data,
>  	if (data_len < 3)
>  		return ERR_PTR(-EINVAL);
> 
> -	/* remaining length where at d (the start of the integer encoding) */
> -	data_len -= 2;
> +	(*data++) = _tag(UNIV, PRIM, INT);

Just for my clarification: 
	First index of "data" is updated here with tag and data pointer incremented.
	Next comment for continuation

> +	data_len--;
> 
> -	data[0] = _tag(UNIV, PRIM, INT);
> -	if (integer == 0) {
> -		*d++ = 0;
> -		goto out;
> +	if (!memchr_inv(integer, 0, integer_len)) {
> +		data[1] = 1;
> +		data[2] = 0;
> +		return &data[2];

Here we are effectively setting second and third index of original "data" pointer as "data" pointer was incremented earlier.
So second index of original "data" pointer is not touched. Also returning 3rd index pointer of original data pointer

Is that intentional?

Thanks
-Bharat

>  	}
> 
> -	for (i = sizeof(integer); i > 0 ; i--) {
> -		int byte = integer >> (8 * (i - 1));
> +	encoded = kzalloc(integer_len, GFP_KERNEL);
> +	if (!encoded)
> +		return ERR_PTR(-ENOMEM);
> +	d = encoded;
> +
> +	for (i = 0; i < integer_len; i++) {
> +		int byte = integer[i];
> 
>  		if (!found && byte == 0)
>  			continue;
> @@ -67,21 +140,23 @@ asn1_encode_integer(unsigned char *data, const
> unsigned char *end_data,
>  			 * have len >= 1
>  			 */
>  			*d++ = 0;
> -			data_len--;
>  		}
> 
>  		found = true;
> -		if (data_len == 0)
> -			return ERR_PTR(-EINVAL);
> -
>  		*d++ = byte;
> -		data_len--;
>  	}
> 
> - out:
> -	data[1] = d - data - 2;
> +	encoded_len = d - encoded;
> 
> -	return d;
> +	ret = asn1_encode_length(&data, &data_len, encoded_len);
> +	if (ret)  {
> +		kfree(encoded);
> +		return ERR_PTR(ret);
> +	}
> +
> +	memcpy(data, encoded, encoded_len);
> +	kfree(encoded);
> +	return data + encoded_len;
>  }
>  EXPORT_SYMBOL_GPL(asn1_encode_integer);
> 
> @@ -176,70 +251,6 @@ asn1_encode_oid(unsigned char *data, const
> unsigned char *end_data,
>  }
>  EXPORT_SYMBOL_GPL(asn1_encode_oid);
> 
> -/**
> - * asn1_encode_length() - encode a length to follow an ASN.1 tag
> - * @data: pointer to encode at
> - * @data_len: pointer to remaining length (adjusted by routine)
> - * @len: length to encode
> - *
> - * This routine can encode lengths up to 65535 using the ASN.1 rules.
> - * It will accept a negative length and place a zero length tag
> - * instead (to keep the ASN.1 valid).  This convention allows other
> - * encoder primitives to accept negative lengths as singalling the
> - * sequence will be re-encoded when the length is known.
> - */
> -static int asn1_encode_length(unsigned char **data, int *data_len, int len)
> -{
> -	if (*data_len < 1)
> -		return -EINVAL;
> -
> -	if (len < 0) {
> -		*((*data)++) = 0;
> -		(*data_len)--;
> -		return 0;
> -	}
> -
> -	if (len <= 0x7f) {
> -		*((*data)++) = len;
> -		(*data_len)--;
> -		return 0;
> -	}
> -
> -	if (*data_len < 2)
> -		return -EINVAL;
> -
> -	if (len <= 0xff) {
> -		*((*data)++) = 0x81;
> -		*((*data)++) = len & 0xff;
> -		*data_len -= 2;
> -		return 0;
> -	}
> -
> -	if (*data_len < 3)
> -		return -EINVAL;
> -
> -	if (len <= 0xffff) {
> -		*((*data)++) = 0x82;
> -		*((*data)++) = (len >> 8) & 0xff;
> -		*((*data)++) = len & 0xff;
> -		*data_len -= 3;
> -		return 0;
> -	}
> -
> -	if (WARN(len > 0xffffff, "ASN.1 length can't be > 0xffffff"))
> -		return -EINVAL;
> -
> -	if (*data_len < 4)
> -		return -EINVAL;
> -	*((*data)++) = 0x83;
> -	*((*data)++) = (len >> 16) & 0xff;
> -	*((*data)++) = (len >> 8) & 0xff;
> -	*((*data)++) = len & 0xff;
> -	*data_len -= 4;
> -
> -	return 0;
> -}
> -
>  /**
>   * asn1_encode_tag() - add a tag for optional or explicit value
>   * @data:	pointer to place tag at
> diff --git a/security/keys/trusted-keys/trusted_tpm2.c
> b/security/keys/trusted-keys/trusted_tpm2.c
> index 8b7dd73d94c1..ec59f9389a2d 100644
> --- a/security/keys/trusted-keys/trusted_tpm2.c
> +++ b/security/keys/trusted-keys/trusted_tpm2.c
> @@ -38,6 +38,7 @@ static int tpm2_key_encode(struct trusted_key_payload
> *payload,
>  	u8 *end_work = scratch + SCRATCH_SIZE;
>  	u8 *priv, *pub;
>  	u16 priv_len, pub_len;
> +	u32 key_handle;
>  	int ret;
> 
>  	priv_len = get_unaligned_be16(src) + 2;
> @@ -77,7 +78,8 @@ static int tpm2_key_encode(struct trusted_key_payload
> *payload,
>  		goto err;
>  	}
> 
> -	work = asn1_encode_integer(work, end_work, options->keyhandle);
> +	key_handle = cpu_to_be32(options->keyhandle);
> +	work = asn1_encode_integer(work, end_work, (u8 *)&key_handle, 4);
>  	work = asn1_encode_octet_string(work, end_work, pub, pub_len);
>  	work = asn1_encode_octet_string(work, end_work, priv, priv_len);
> 
> --
> 2.45.1
> 

RE: [EXTERNAL] [PATCH v2 5/6] tpm: tpm2_key: Extend parser to TPM_LoadableKey

[Bharat Bhushan]

> -----Original Message-----
> From: Jarkko Sakkinen <jarkko@kernel.org>
> Sent: Tuesday, May 21, 2024 8:47 AM
> To: Herbert Xu <herbert@gondor.apana.org.au>
> Cc: linux-integrity@vger.kernel.org; keyrings@vger.kernel.org;
> Andreas.Fuchs@infineon.com; James Prestwood <prestwoj@gmail.com>;
> David Woodhouse <dwmw2@infradead.org>; Eric Biggers
> <ebiggers@kernel.org>; James Bottomley
> <James.Bottomley@hansenpartnership.com>; Jarkko Sakkinen
> <jarkko@kernel.org>; David S. Miller <davem@davemloft.net>; open
> list:CRYPTO API <linux-crypto@vger.kernel.org>; open list <linux-
> kernel@vger.kernel.org>; Peter Huewe <peterhuewe@gmx.de>; Jason
> Gunthorpe <jgg@ziepe.ca>; James Bottomley
> <James.Bottomley@HansenPartnership.com>; Mimi Zohar
> <zohar@linux.ibm.com>; David Howells <dhowells@redhat.com>; Paul Moore
> <paul@paul-moore.com>; James Morris <jmorris@namei.org>; Serge E. Hallyn
> <serge@hallyn.com>; open list:SECURITY SUBSYSTEM <linux-security-
> module@vger.kernel.org>
> Subject: [EXTERNAL] [PATCH v2 5/6] tpm: tpm2_key: Extend parser to
> TPM_LoadableKey
> 
> ----------------------------------------------------------------------
> Extend parser to TPM_LoadableKey. Add field for oid to struct tpm2_key
> so that callers can differentiate different key types.
> 
> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> ---
>  drivers/char/tpm/tpm2_key.c               | 14 +++++++++++---
>  include/crypto/tpm2_key.h                 |  2 ++
>  security/keys/trusted-keys/trusted_tpm2.c |  4 ++++
>  3 files changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm2_key.c b/drivers/char/tpm/tpm2_key.c
> index 0112362e432e..59797dc232f1 100644
> --- a/drivers/char/tpm/tpm2_key.c
> +++ b/drivers/char/tpm/tpm2_key.c
> @@ -32,16 +32,24 @@ int tpm2_key_type(void *context, size_t hdrlen,
>  		  const void *value, size_t vlen)
>  {
>  	enum OID oid = look_up_OID(value, vlen);
> -
> -	if (oid != OID_TPMSealedData) {
> +	struct tpm2_key *key = context;
> +
> +	switch (oid) {
> +	case OID_TPMSealedData:
> +		pr_info("TPMSealedData\n");
> +		break;
> +	case OID_TPMLoadableKey:
> +		pr_info("TPMLodableKey\n");
> +		break;
> +	default:
>  		char buffer[50];
> -
>  		sprint_oid(value, vlen, buffer, sizeof(buffer));
>  		pr_debug("OID is \"%s\" which is not TPMSealedData\n",
>  			 buffer);

Maybe extend this print to say "neither TPMSealedData nor TPMLodableKey"

Thanks
-Bharat

>  		return -EINVAL;
>  	}
> 
> +	key->oid = oid;
>  	return 0;
>  }
> 
> diff --git a/include/crypto/tpm2_key.h b/include/crypto/tpm2_key.h
> index acf41b2e0c92..2d2434233000 100644
> --- a/include/crypto/tpm2_key.h
> +++ b/include/crypto/tpm2_key.h
> @@ -2,12 +2,14 @@
>  #ifndef __LINUX_TPM2_KEY_H__
>  #define __LINUX_TPM2_KEY_H__
> 
> +#include <linux/oid_registry.h>
>  #include <linux/slab.h>
> 
>  /*
>   * TPM2 ASN.1 key
>   */
>  struct tpm2_key {
> +	enum OID oid;
>  	u32 parent;
>  	const u8 *blob;
>  	u32 blob_len;
> diff --git a/security/keys/trusted-keys/trusted_tpm2.c
> b/security/keys/trusted-keys/trusted_tpm2.c
> index f255388d32b8..ce4c667c3ee3 100644
> --- a/security/keys/trusted-keys/trusted_tpm2.c
> +++ b/security/keys/trusted-keys/trusted_tpm2.c
> @@ -305,6 +305,10 @@ static int tpm2_load_cmd(struct tpm_chip *chip,
>  		payload->old_format = 1;
>  	} else {
>  		blob = key.blob;
> +		if (key.oid != OID_TPMSealedData) {
> +			tpm2_key_destroy(&key);
> +			return -EINVAL;
> +		}
>  	}
> 
>  	if (!blob)
> --
> 2.45.1
> 

Re: [EXTERNAL] [PATCH v2 2/6] lib: Expand asn1_encode_integer() to variable size integers

[Jarkko Sakkinen]

On Tue May 21, 2024 at 8:36 AM EEST, Bharat Bhushan wrote:
> > -	data_len -= 2;
> > +	(*data++) = _tag(UNIV, PRIM, INT);
>
> Just for my clarification: 
> 	First index of "data" is updated here with tag and data pointer incremented.
> 	Next comment for continuation
>
> > +	data_len--;
> > 
> > -	data[0] = _tag(UNIV, PRIM, INT);
> > -	if (integer == 0) {
> > -		*d++ = 0;
> > -		goto out;
> > +	if (!memchr_inv(integer, 0, integer_len)) {
> > +		data[1] = 1;
> > +		data[2] = 0;
> > +		return &data[2];
>
> Here we are effectively setting second and third index of original
> "data" pointer as "data" pointer was incremented earlier.
> So second index of original "data" pointer is not touched. Also
> returning 3rd index pointer of original data pointer
>
> Is that intentional?

No! I read the diff few times, and I think you have a point.

Indices should be 0 (length) and 1 (value). I.e. it forms an encoded
version of zero. The last index what it should be, i.e. return address
of the next byte after the encoded integer.

Thanks for pointing this out.

> Thanks
> -Bharat

BR, Jarkko

Re: [EXTERNAL] [PATCH v2 5/6] tpm: tpm2_key: Extend parser to TPM_LoadableKey

[Jarkko Sakkinen]

On Tue May 21, 2024 at 8:47 AM EEST, Bharat Bhushan wrote:
>
>
> > -----Original Message-----
> > From: Jarkko Sakkinen <jarkko@kernel.org>
> > Sent: Tuesday, May 21, 2024 8:47 AM
> > To: Herbert Xu <herbert@gondor.apana.org.au>
> > Cc: linux-integrity@vger.kernel.org; keyrings@vger.kernel.org;
> > Andreas.Fuchs@infineon.com; James Prestwood <prestwoj@gmail.com>;
> > David Woodhouse <dwmw2@infradead.org>; Eric Biggers
> > <ebiggers@kernel.org>; James Bottomley
> > <James.Bottomley@hansenpartnership.com>; Jarkko Sakkinen
> > <jarkko@kernel.org>; David S. Miller <davem@davemloft.net>; open
> > list:CRYPTO API <linux-crypto@vger.kernel.org>; open list <linux-
> > kernel@vger.kernel.org>; Peter Huewe <peterhuewe@gmx.de>; Jason
> > Gunthorpe <jgg@ziepe.ca>; James Bottomley
> > <James.Bottomley@HansenPartnership.com>; Mimi Zohar
> > <zohar@linux.ibm.com>; David Howells <dhowells@redhat.com>; Paul Moore
> > <paul@paul-moore.com>; James Morris <jmorris@namei.org>; Serge E. Hallyn
> > <serge@hallyn.com>; open list:SECURITY SUBSYSTEM <linux-security-
> > module@vger.kernel.org>
> > Subject: [EXTERNAL] [PATCH v2 5/6] tpm: tpm2_key: Extend parser to
> > TPM_LoadableKey
> > 
> > ----------------------------------------------------------------------
> > Extend parser to TPM_LoadableKey. Add field for oid to struct tpm2_key
> > so that callers can differentiate different key types.
> > 
> > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> > ---
> >  drivers/char/tpm/tpm2_key.c               | 14 +++++++++++---
> >  include/crypto/tpm2_key.h                 |  2 ++
> >  security/keys/trusted-keys/trusted_tpm2.c |  4 ++++
> >  3 files changed, 17 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/char/tpm/tpm2_key.c b/drivers/char/tpm/tpm2_key.c
> > index 0112362e432e..59797dc232f1 100644
> > --- a/drivers/char/tpm/tpm2_key.c
> > +++ b/drivers/char/tpm/tpm2_key.c
> > @@ -32,16 +32,24 @@ int tpm2_key_type(void *context, size_t hdrlen,
> >  		  const void *value, size_t vlen)
> >  {
> >  	enum OID oid = look_up_OID(value, vlen);
> > -
> > -	if (oid != OID_TPMSealedData) {
> > +	struct tpm2_key *key = context;
> > +
> > +	switch (oid) {
> > +	case OID_TPMSealedData:
> > +		pr_info("TPMSealedData\n");
> > +		break;
> > +	case OID_TPMLoadableKey:
> > +		pr_info("TPMLodableKey\n");

These should be pr_debug() (forgot to change).

> > +		break;
> > +	default:
> >  		char buffer[50];
> > -
> >  		sprint_oid(value, vlen, buffer, sizeof(buffer));
> >  		pr_debug("OID is \"%s\" which is not TPMSealedData\n",
> >  			 buffer);
>
> Maybe extend this print to say "neither TPMSealedData nor TPMLodableKey"

Right, I tried to apply minimal delta to patches where existing code
needs to be carved to a new form :-)

I think it could be just "OID \"%s\" is unknown"?

BR, Jarkko

Re: [PATCH v2 4/6] KEYS: trusted: Move tpm2_key_decode() to the TPM driver

[James Bottomley]

On Tue, 2024-05-21 at 06:16 +0300, Jarkko Sakkinen wrote:
[...]
> diff --git a/include/crypto/tpm2_key.h b/include/crypto/tpm2_key.h
> new file mode 100644
> index 000000000000..acf41b2e0c92
> --- /dev/null
> +++ b/include/crypto/tpm2_key.h
> @@ -0,0 +1,33 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +#ifndef __LINUX_TPM2_KEY_H__
> +#define __LINUX_TPM2_KEY_H__
> +
> +#include <linux/slab.h>
> +
> +/*
> + * TPM2 ASN.1 key
> + */
> +struct tpm2_key {
> +       u32 parent;
> +       const u8 *blob;
> +       u32 blob_len;
> +       const u8 *pub;
> +       u32 pub_len;
> +       const u8 *priv;
> +       u32 priv_len;
> +};
> +
> +int tpm2_key_decode(const u8 *src, u32 src_len, struct tpm2_key
> *key,
> +                   u32 max_key_len);

I don't think this is a good idea.  Trusted keys already have a pre-
defined max payload size (MAX_BLOB_SIZE in include/keys/trusted-type.h)
and I've already had to increase this several times because once you
get policy attached to a key, it can get pretty big (over a page). 
Exactly the same thing will happen to asymmetric keys as well, so it
does make sense that they share the same maximum (probably in a more
generic header, though).

Since the code already right sizes the allocation and all we check with
this is whether it's over a pre-defined maximum, it's way easier if
that maximum is defined in a header rather than passed in in several
places making increasing the maximum really hard because you have to
chase all the threading.

James

Re: [PATCH v2 4/6] KEYS: trusted: Move tpm2_key_decode() to the TPM driver

[Jarkko Sakkinen]

On Tue May 21, 2024 at 9:18 PM EEST, James Bottomley wrote:
> On Tue, 2024-05-21 at 06:16 +0300, Jarkko Sakkinen wrote:
> [...]
> > diff --git a/include/crypto/tpm2_key.h b/include/crypto/tpm2_key.h
> > new file mode 100644
> > index 000000000000..acf41b2e0c92
> > --- /dev/null
> > +++ b/include/crypto/tpm2_key.h
> > @@ -0,0 +1,33 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +#ifndef __LINUX_TPM2_KEY_H__
> > +#define __LINUX_TPM2_KEY_H__
> > +
> > +#include <linux/slab.h>
> > +
> > +/*
> > + * TPM2 ASN.1 key
> > + */
> > +struct tpm2_key {
> > +       u32 parent;
> > +       const u8 *blob;
> > +       u32 blob_len;
> > +       const u8 *pub;
> > +       u32 pub_len;
> > +       const u8 *priv;
> > +       u32 priv_len;
> > +};
> > +
> > +int tpm2_key_decode(const u8 *src, u32 src_len, struct tpm2_key
> > *key,
> > +                   u32 max_key_len);
>
> I don't think this is a good idea.  Trusted keys already have a pre-
> defined max payload size (MAX_BLOB_SIZE in include/keys/trusted-type.h)
> and I've already had to increase this several times because once you
> get policy attached to a key, it can get pretty big (over a page). 
> Exactly the same thing will happen to asymmetric keys as well, so it
> does make sense that they share the same maximum (probably in a more
> generic header, though).

ECDSA and RSA have different space requirements. With that solution you
actually max out space requirements given same cap for everything.

Even tpm2_key_ecdsa should use a different value than tpm2_key_rsa to
save memory.

> Since the code already right sizes the allocation and all we check with
> this is whether it's over a pre-defined maximum, it's way easier if
> that maximum is defined in a header rather than passed in in several
> places making increasing the maximum really hard because you have to
> chase all the threading.

You don't save a single byte of memory with any constant that dictates
the size requirements for multiple modules in two disjoint subsystems.

You are maximizing the use of memory.

> James

BR, Jarkko

Re: [PATCH v2 4/6] KEYS: trusted: Move tpm2_key_decode() to the TPM driver

[David Howells]

Jarkko Sakkinen <jarkko@kernel.org> wrote:

> On Tue May 21, 2024 at 9:18 PM EEST, James Bottomley wrote:
> ...
> You don't save a single byte of memory with any constant that dictates
> the size requirements for multiple modules in two disjoint subsystems.

I think James is just suggesting you replace your limit argument with a
constant not that you always allocate that amount of memory.  What the limit
should be, OTOH, is up for discussion, but PAGE_SIZE seems not unreasonable.

David

Re: [PATCH v2 4/6] KEYS: trusted: Move tpm2_key_decode() to the TPM driver

[James Bottomley]

On Tue, 2024-05-21 at 22:44 +0100, David Howells wrote:
> Jarkko Sakkinen <jarkko@kernel.org> wrote:
> 
> > On Tue May 21, 2024 at 9:18 PM EEST, James Bottomley wrote:
> > ...
> > You don't save a single byte of memory with any constant that
> > dictates the size requirements for multiple modules in two disjoint
> > subsystems.
> 
> I think James is just suggesting you replace your limit argument with
> a constant not that you always allocate that amount of memory.

Exactly.  All we use it for is the -E2BIG check to ensure user space
isn't allowed to run away with loads of kernel memory.

> What the limit should be, OTOH, is up for discussion, but PAGE_SIZE
> seems not unreasonable.

A page is fine currently (MAX_BLOB_SIZE is 512).  However, it may be
too small for some of the complex policies when they're introduced. 
I'm not bothered about what it currently is, I just want it to be able
to be increased easily when the time comes.

James

Re: [PATCH v2 4/6] KEYS: trusted: Move tpm2_key_decode() to the TPM driver

[Jarkko Sakkinen]

On Wed May 22, 2024 at 12:44 AM EEST, David Howells wrote:
> Jarkko Sakkinen <jarkko@kernel.org> wrote:
>
> > On Tue May 21, 2024 at 9:18 PM EEST, James Bottomley wrote:
> > ...
> > You don't save a single byte of memory with any constant that dictates
> > the size requirements for multiple modules in two disjoint subsystems.
>
> I think James is just suggesting you replace your limit argument with a
> constant not that you always allocate that amount of memory.  What the limit
> should be, OTOH, is up for discussion, but PAGE_SIZE seems not unreasonable.

When the decoder for ASN.1 was part of trusted keys, the check used to
be:

	if (ctx.priv_len + ctx.pub_len > MAX_BLOB_SIZE)
		return -EINVAL;

And MAX_BLOB_SIZE is only 512 bytes, which does not fit event 2048 bit
RSA key but that 512 bytes cap seems to be just fine for trusted keys.

So the new check is:

	if (blob_len > max_key_len)
		return -E2BIG;

1. Too big value is not invalid value, thus -E2BIG. It is has also
   shown to be practically useful while testing this key type.
2. tpm2_key_rsa needs up to 8192 bytes for a blob to fit 4096-bit
   RSA key. 

Just saying but there is also primary null key allocated by the driver.
And neither driver uses MAX_BLOB_SiZE. It uses value 8x MAX_BLOB_SIZE
i.e. 4096 bytes so not really following the idea suggested.

Finaly, there is three completely separate algorithms:

- KEYEDHASH (trusted_keys)
- RSA (tpm2_key_rsa)
- ECDSA (driver)§
	
With all this put together it is just common sense to have parametrized
cap value, and it would have no logic at all to treat them unified way.

For tpm2_key_rsa I will define for clarity:

#define TPM2_KEY_RSA_MAX_SIZE 8192

For tpm2_key_ecdsa you would define

#define TPM2_KEY_ECDSA_MAX_SIZE 4096

So yeah, this is how I will proceed because it is really the only
senseful way to proceed.

>
> David

BR, Jarkko

Re: [PATCH v2 4/6] KEYS: trusted: Move tpm2_key_decode() to the TPM driver

[Jarkko Sakkinen]

On Wed May 22, 2024 at 12:59 AM EEST, James Bottomley wrote:
> On Tue, 2024-05-21 at 22:44 +0100, David Howells wrote:
> > Jarkko Sakkinen <jarkko@kernel.org> wrote:
> > 
> > > On Tue May 21, 2024 at 9:18 PM EEST, James Bottomley wrote:
> > > ...
> > > You don't save a single byte of memory with any constant that
> > > dictates the size requirements for multiple modules in two disjoint
> > > subsystems.
> > 
> > I think James is just suggesting you replace your limit argument with
> > a constant not that you always allocate that amount of memory.
>
> Exactly.  All we use it for is the -E2BIG check to ensure user space
> isn't allowed to run away with loads of kernel memory.

Not true.

It did return -EINVAL. This patch changes it to -E2BIG.

>
> > What the limit should be, OTOH, is up for discussion, but PAGE_SIZE
> > seems not unreasonable.
>
> A page is fine currently (MAX_BLOB_SIZE is 512).  However, it may be
> too small for some of the complex policies when they're introduced. 
> I'm not bothered about what it currently is, I just want it to be able
> to be increased easily when the time comes.

MAX_BLOB_SIZE would be used to cap key blob, not the policy.

And you are ignoring it yourself too in the driver.


> James


BR, Jarkko

Re: [PATCH v2 4/6] KEYS: trusted: Move tpm2_key_decode() to the TPM driver

[Jarkko Sakkinen]

On Wed May 22, 2024 at 1:45 AM EEST, Jarkko Sakkinen wrote:
> On Wed May 22, 2024 at 12:59 AM EEST, James Bottomley wrote:
> > On Tue, 2024-05-21 at 22:44 +0100, David Howells wrote:
> > > Jarkko Sakkinen <jarkko@kernel.org> wrote:
> > > 
> > > > On Tue May 21, 2024 at 9:18 PM EEST, James Bottomley wrote:
> > > > ...
> > > > You don't save a single byte of memory with any constant that
> > > > dictates the size requirements for multiple modules in two disjoint
> > > > subsystems.
> > > 
> > > I think James is just suggesting you replace your limit argument with
> > > a constant not that you always allocate that amount of memory.
> >
> > Exactly.  All we use it for is the -E2BIG check to ensure user space
> > isn't allowed to run away with loads of kernel memory.
>
> Not true.
>
> It did return -EINVAL. This patch changes it to -E2BIG.
>
> >
> > > What the limit should be, OTOH, is up for discussion, but PAGE_SIZE
> > > seems not unreasonable.
> >
> > A page is fine currently (MAX_BLOB_SIZE is 512).  However, it may be
> > too small for some of the complex policies when they're introduced. 
> > I'm not bothered about what it currently is, I just want it to be able
> > to be increased easily when the time comes.
>
> MAX_BLOB_SIZE would be used to cap key blob, not the policy.
>
> And you are ignoring it yourself too in the driver.

Obviously policy is part of the key blob i.e. expected value for that.

... but that does not reduce space requirements to rsa asymmetric keys.
It increases them but I think at this point 8192 is good starting point.
And it cap can be scaled later.

Being a parameter also allows to have even kernel-command line or sysfs
parameter and stuff like that. It is robust not a bad choice.

BR, Jarkko