[PATCH 0/3] Load keys from TPM2 NV Index on IMA keyring

[PATCH 0/3] Load keys from TPM2 NV Index on IMA keyring

[Patrick Uiterwijk]

The system's signature chain of trust is rooted in hardware and pivots to
the keys baked into the kernel. IMA maintains this signature chain of trust
by requiring any key being added to the IMA trusted keyring to be signed by
a key on the builtin (or secondary) keyrings. This prevents a local key,
needed for signing policies or other files, from being loaded on the IMA
keyring, without requiring a custom built kernel (or injecting a key and
resigning the kernel image).

Allow users to load their own public key stored in a specific TPM2 NV
Index, requiring the absence of the Platform Create and Platform Write
attributes on the NV Index, to be loaded on the IMA keyring.

To test this with the TPM2-software tools with a DER-encoded imacert.der:
  tpm2_nvdefine -C o -s 945 0x184b520
  tpm2_nvwrite -C o -i imacert.der 0x184b520

Or with the IBM TSS tools:
  tssnvdefinespace -ha 0x184b520 -hi o -sz 945 +at ow +at or
  tssnvwrite -hia o -ha 0x184b520 -if imacert.der

Then after a reboot, the ima keyring should contain the certificate.

Note that if this feature is enabled, users should make sure an NV Index
is created with accurate attributes to prevent any other users from
writing or deleting the NV Index. Without this precaution, any user who
has access to the TPM would be able to write a key to the NV Index and have
that key loaded on the IMA trusted keyring.

A distro who wants to enable this feature, for example, should ensure that
the installer defines the NV Index in all cases, and only fills it if a key
was provided by the user.

It is strongly adviced that any NV Index created for this purpose has at
least the policy_delete and policywrite attributes set, together with a
non-empty policy. Those flags make sure that the policy (which would be up
to them to define) is required to be satisfied to delete or write the index.

Patrick Uiterwijk (3):
  tpm: Add support for reading a TPM NV Index
  integrity: Allow specifying flags in integrity_load_cert
  integrity: Load keys from TPM NV onto IMA keyring

 drivers/char/tpm/tpm-interface.c              |  30 ++++
 drivers/char/tpm/tpm.h                        |   5 +
 drivers/char/tpm/tpm2-cmd.c                   | 163 ++++++++++++++++++
 include/linux/tpm.h                           |  65 +++++++
 security/integrity/digsig.c                   |  11 +-
 security/integrity/ima/Kconfig                |  22 +++
 security/integrity/ima/ima_init.c             |  53 ++++++
 security/integrity/integrity.h                |   6 +-
 .../platform_certs/platform_keyring.c         |   2 +-
 9 files changed, 349 insertions(+), 8 deletions(-)

-- 
2.29.2

[PATCH 1/3] tpm: Add support for reading a TPM NV Index

[Patrick Uiterwijk]

Add support to read contents from a TPM2 Non-Volatile Index location,
allowing the kernel to retrieve contents and attributes of NV indexes.

Signed-off-by: Patrick Uiterwijk <patrick@puiterwijk.org>
---
 drivers/char/tpm/tpm-interface.c |  30 ++++++
 drivers/char/tpm/tpm.h           |   5 +
 drivers/char/tpm/tpm2-cmd.c      | 163 +++++++++++++++++++++++++++++++
 include/linux/tpm.h              |  65 ++++++++++++
 4 files changed, 263 insertions(+)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 1621ce818705..9d81c11181d4 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -342,6 +342,36 @@ int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
 }
 EXPORT_SYMBOL_GPL(tpm_pcr_extend);
 
+/**
+ * tpm_nv_read - Read an NV Index from the TPM
+ * @chip:	A &struct tpm_chip instance, %NULL for the default chip
+ * @nv_idx:	The NV Index to be retrieved
+ * @attr_out:	A place to store returned attributes if a TPM 2 was used
+ * @out:	A pointer where to store the return buffer
+ *
+ * Return: number of bytes read or a negative error value
+ */
+int tpm_nv_read(struct tpm_chip *chip, u32 nv_idx, u32 *attr_out, void **out)
+{
+	int rc;
+
+	chip = tpm_find_get_ops(chip);
+	if (!chip)
+		return -ENODEV;
+
+	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
+		rc = tpm2_nv_read(chip, nv_idx, attr_out, out);
+		goto out;
+	}
+
+	rc = -ENODEV;
+
+out:
+	tpm_put_ops(chip);
+	return rc;
+}
+EXPORT_SYMBOL_GPL(tpm_nv_read);
+
 /**
  * tpm_send - send a TPM command
  * @chip:	a &struct tpm_chip instance, %NULL for the default chip
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 947d1db0a5cc..d4dfc5148adb 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -56,9 +56,12 @@ enum tpm_addr {
 #define TPM_ERR_DEACTIVATED     0x6
 #define TPM_ERR_DISABLED        0x7
 #define TPM_ERR_INVALID_POSTINIT 38
+#define TPM_ERR_INVALID_HANDLE_1 0x18b
 
 #define TPM_TAG_RQU_COMMAND 193
 
+#define TPM2_HR_NV_INDEX 0x1000000
+
 /* TPM2 specific constants. */
 #define TPM2_SPACE_BUFFER_SIZE		16384 /* 16 kB */
 
@@ -224,6 +227,8 @@ int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t max);
 ssize_t tpm2_get_tpm_pt(struct tpm_chip *chip, u32 property_id,
 			u32 *value, const char *desc);
 
+int tpm2_nv_read(struct tpm_chip *chip, u32 nvindex, u32 *attr_out, void **dest);
+int tpm2_nv_readpublic(struct tpm_chip *chip, u32 nvindex, struct tpm2_nv_public *info);
 ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip);
 int tpm2_auto_startup(struct tpm_chip *chip);
 void tpm2_shutdown(struct tpm_chip *chip, u16 shutdown_type);
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index eff1f12d981a..ba1026123464 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -269,6 +269,169 @@ int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
 	return rc;
 }
 
+struct tpm2_buffer_out {
+	__be16	size;
+	u8	data[];
+} __packed;
+
+struct tpm2_nv_public_out {
+	__be32	nvIndex;
+	__be16	nameAlg;
+	__be32	attributes;
+	__be16	authPolicySize;
+	u8	data[];
+} __packed;
+
+int tpm2_nv_readpublic(struct tpm_chip *chip, u32 nvindex, struct tpm2_nv_public *info)
+{
+	struct tpm_buf buf;
+	int rc;
+	u16 recd;
+	u32 resp_header_length;
+	struct tpm2_buffer_out *out;
+	struct tpm2_nv_public_out *publicout;
+	u32 nvhandle;
+	u16 auth_policy_size;
+
+	if ((nvindex & ~TPM2_HR_NV_INDEX) > 0x00FFFFFF)
+		return -EINVAL;
+
+	/* HR_NV_INDEX = TPM_HT_NV_INDEX << HR_SHIFT */
+	nvhandle = TPM2_HR_NV_INDEX | nvindex;
+
+	rc = tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS, TPM2_CC_NV_READPUBLIC);
+	if (rc)
+		return rc;
+	tpm_buf_append_u32(&buf, nvhandle);
+	rc = tpm_transmit_cmd(chip, &buf, 0, NULL);
+	if (rc) {
+		if (rc != TPM_ERR_DISABLED && rc != TPM_ERR_DEACTIVATED
+		    && rc != TPM2_RC_TESTING && rc != TPM_ERR_INVALID_HANDLE_1)
+			dev_err(&chip->dev, "A TPM error (%d) occurred attempting to read an NV Index public\n", rc);
+		if (rc == TPM_ERR_INVALID_HANDLE_1)
+			rc = -ENOENT;
+		else if (rc > 0)
+			rc = -EIO;
+		goto out;
+	}
+	resp_header_length = tpm_buf_response_header_length(&buf, 0);
+	out = (struct tpm2_buffer_out *)&buf.data[resp_header_length];
+	publicout = (struct tpm2_nv_public_out *)&out->data;
+	recd = be16_to_cpu(out->size);
+
+	info->nv_index = be32_to_cpu(publicout->nvIndex);
+	info->name_alg = be16_to_cpu(publicout->nameAlg);
+	info->attributes = be32_to_cpu(publicout->attributes);
+
+	/* Determine the size of the authPolicy, so we can skip over that to grab the data size */
+	auth_policy_size = be16_to_cpu(publicout->authPolicySize);
+
+	info->data_size = be16_to_cpu((publicout->data[auth_policy_size]) | (publicout->data[auth_policy_size+1] << 8));
+
+out:
+	tpm_buf_destroy(&buf);
+	return rc;
+}
+
+int tpm2_nv_read(struct tpm_chip *chip, u32 nvindex, u32 *attr_out, void **dest)
+{
+	struct tpm_buf buf;
+	int rc;
+	struct tpm2_buffer_out *out;
+	u16 recd;
+	u16 copied;
+	u32 nvhandle;
+	u32 resp_header_length;
+	struct tpm2_null_auth_area auth_area;
+	u16 size;
+	struct tpm2_nv_public public;
+
+	copied = 0;
+
+	if ((nvindex & ~TPM2_HR_NV_INDEX) > 0x00FFFFFF)
+		return -EINVAL;
+
+	/* HR_NV_INDEX = TPM_HT_NV_INDEX << HR_SHIFT */
+	nvhandle = TPM2_HR_NV_INDEX | nvindex;
+
+	/* Determine the size of the NV Index contents */
+	rc = tpm2_nv_readpublic(chip, nvindex, &public);
+	if (rc < 0)
+		return rc;
+	if (attr_out != NULL)
+		*attr_out = public.attributes;
+	size = public.data_size;
+	*dest = kzalloc(size, GFP_KERNEL);
+	if (!*dest) {
+		rc = -ENOMEM;
+		goto out;
+	}
+
+	/* Retrieve the actual NV Index contents */
+	rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_NV_READ);
+	if (rc)
+		goto out_free;
+
+	while (copied < size) {
+		tpm_buf_reset(&buf, TPM2_ST_SESSIONS, TPM2_CC_NV_READ);
+
+		tpm_buf_append_u32(&buf, TPM2_RH_OWNER);
+		tpm_buf_append_u32(&buf, nvhandle);
+
+		auth_area.handle = cpu_to_be32(TPM2_RS_PW);
+		auth_area.nonce_size = 0;
+		auth_area.attributes = 0;
+		auth_area.auth_size = 0;
+
+		tpm_buf_append_u32(&buf, sizeof(struct tpm2_null_auth_area));
+		tpm_buf_append(&buf, (const unsigned char *)&auth_area,
+			       sizeof(auth_area));
+
+		/* Size to request: at most 512 bytes at a time */
+		tpm_buf_append_u16(&buf, min_t(u16, 512, size-copied));
+		/* Offset: start at where we ended up */
+		tpm_buf_append_u16(&buf, copied);
+
+		rc = tpm_transmit_cmd(chip, &buf, 0, "attempting to read NV index");
+		if (rc) {
+			if (rc > 0)
+				rc = -EIO;
+			goto out_free;
+		}
+		resp_header_length = tpm_buf_response_header_length(&buf, 0);
+		out = (struct tpm2_buffer_out *)&buf.data[resp_header_length];
+		recd = be16_to_cpu(out->size);
+
+		if (recd == 0) {
+			rc = -EIO;
+			goto out_free;
+		}
+		if (recd > size-copied) {
+			rc = -EIO;
+			goto out_free;
+		}
+
+		memcpy(*dest + copied, out->data, recd);
+		copied += recd;
+	};
+
+out_free:
+	if ((rc < 0) || (copied != size)) {
+		kvfree(*dest);
+		*dest = NULL;
+	}
+
+out:
+	tpm_buf_destroy(&buf);
+
+	if (rc < 0)
+		return rc;
+	else if (copied != size)
+		return -EIO;
+	else
+		return size;
+}
+
 struct tpm2_get_random_out {
 	__be16 size;
 	u8 buffer[TPM_MAX_RNG_DATA];
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index 8f4ff39f51e7..b812236b9955 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -53,6 +53,40 @@ struct tpm_bank_info {
 	u16 crypto_id;
 };
 
+enum tpm_nv_public_attrs {
+	TPM2_ATTR_NV_PPWRITE = 0x00000001,
+	TPM2_ATTR_NV_OWNERWRITE = 0x00000002,
+	TPM2_ATTR_NV_AUTHWRITE = 0x00000004,
+	TPM2_ATTR_NV_POLICYWRITE = 0x00000008,
+	/* Bits 4-7 TPM_NT */
+	/* Bits 8-9 reserved */
+	TPM2_ATTR_NV_POLICY_DELETE = 0x00000400,
+	TPM2_ATTR_NV_WRITELOCKED = 0x00000800,
+	TPM2_ATTR_NV_WRITEALL = 0x00001000,
+	TPM2_ATTR_NV_WRITE_DEFINE = 0x00002000,
+	TPM2_ATTR_NV_WRITE_STCLEAR = 0x00004000,
+	TPM2_ATTR_NV_GLOBALLOCK = 0x00008000,
+	TPM2_ATTR_NV_PPREAD = 0x00010000,
+	TPM2_ATTR_NV_OWNERREAD = 0x00020000,
+	TPM2_ATTR_NV_AUTHREAD = 0x00040000,
+	TPM2_ATTR_NV_POLICYREAD = 0x00080000,
+	/* Bits 20-24 reserved */
+	TPM2_ATTR_NV_NO_DA = 0x02000000,
+	TPM2_ATTR_NV_ORDERLY = 0x04000000,
+	TPM2_ATTR_NV_CLEAR_STCLEAR = 0x08000000,
+	TPM2_ATTR_NV_READLOCKED = 0x10000000,
+	TPM2_ATTR_NV_WRITTEN = 0x20000000,
+	TPM2_ATTR_NV_PLATFORMCREATE = 0x40000000,
+	TPM2_ATTR_NV_READ_STCLEAR = 0x80000000,
+};
+
+struct tpm2_nv_public {
+	u32 nv_index;
+	u16 name_alg;
+	u32 attributes;
+	u16 data_size;
+};
+
 enum TPM_OPS_FLAGS {
 	TPM_OPS_AUTO_STARTUP = BIT(0),
 };
@@ -189,6 +223,10 @@ enum tpm2_structures {
 	TPM2_ST_SESSIONS	= 0x8002,
 };
 
+enum tpm2_root_handles {
+	TPM2_RH_OWNER		= 0x40000001,
+};
+
 /* Indicates from what layer of the software stack the error comes from */
 #define TSS2_RC_LAYER_SHIFT	 16
 #define TSS2_RESMGR_TPM_RC_LAYER (11 << TSS2_RC_LAYER_SHIFT)
@@ -223,6 +261,7 @@ enum tpm2_command_codes {
 	TPM2_CC_CONTEXT_LOAD	        = 0x0161,
 	TPM2_CC_CONTEXT_SAVE	        = 0x0162,
 	TPM2_CC_FLUSH_CONTEXT	        = 0x0165,
+	TPM2_CC_NV_READPUBLIC		= 0x0169,
 	TPM2_CC_VERIFY_SIGNATURE        = 0x0177,
 	TPM2_CC_GET_CAPABILITY	        = 0x017A,
 	TPM2_CC_GET_RANDOM	        = 0x017B,
@@ -389,6 +428,21 @@ static inline void tpm_buf_append_u32(struct tpm_buf *buf, const u32 value)
 	tpm_buf_append(buf, (u8 *) &value2, 4);
 }
 
+static inline u32 tpm_buf_response_header_length(struct tpm_buf *buf, bool has_shielded_locations)
+{
+	u32 header_length = TPM_HEADER_SIZE;
+
+	/* Possibly a handle for a Shielded Location */
+	if (has_shielded_locations)
+		header_length += 4;
+
+	/* Possibly the 32-bit parameter area size */
+	if (tpm_buf_tag(buf) == TPM2_ST_SESSIONS)
+		header_length += 4;
+
+	return header_length;
+}
+
 static inline u32 tpm2_rc_value(u32 rc)
 {
 	return (rc & BIT(7)) ? rc & 0xff : rc;
@@ -401,6 +455,7 @@ extern int tpm_pcr_read(struct tpm_chip *chip, u32 pcr_idx,
 			struct tpm_digest *digest);
 extern int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
 			  struct tpm_digest *digests);
+extern int tpm_nv_read(struct tpm_chip *chip, u32 nv_idx, u32 *attrs_out, void **out);
 extern int tpm_send(struct tpm_chip *chip, void *cmd, size_t buflen);
 extern int tpm_get_random(struct tpm_chip *chip, u8 *data, size_t max);
 extern struct tpm_chip *tpm_default_chip(void);
@@ -423,6 +478,16 @@ static inline int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
 	return -ENODEV;
 }
 
+static inline int tpm2_nv_readpublic(struct tpm_chip *chip, u32 nvindex, struct tpm2_nv_public *info)
+{
+	return -ENODEV;
+}
+
+static inline int tpm_nv_read(struct tpm_chip *chip, u32 nv_idx, u8 *out, size_t max)
+{
+	return -ENODEV;
+}
+
 static inline int tpm_send(struct tpm_chip *chip, void *cmd, size_t buflen)
 {
 	return -ENODEV;
-- 
2.29.2

Re: [PATCH 1/3] tpm: Add support for reading a TPM NV Index

[Stefan Berger]

On 2/25/21 3:32 PM, Patrick Uiterwijk wrote:
> Add support to read contents from a TPM2 Non-Volatile Index location,
> allowing the kernel to retrieve contents and attributes of NV indexes.
>
> Signed-off-by: Patrick Uiterwijk <patrick@puiterwijk.org>
> ---
>   drivers/char/tpm/tpm-interface.c |  30 ++++++
>   drivers/char/tpm/tpm.h           |   5 +
>   drivers/char/tpm/tpm2-cmd.c      | 163 +++++++++++++++++++++++++++++++
>   include/linux/tpm.h              |  65 ++++++++++++
>   4 files changed, 263 insertions(+)
>
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index 1621ce818705..9d81c11181d4 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -342,6 +342,36 @@ int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
>   }
>   EXPORT_SYMBOL_GPL(tpm_pcr_extend);
>   
> +/**
> + * tpm_nv_read - Read an NV Index from the TPM
> + * @chip:	A &struct tpm_chip instance, %NULL for the default chip
> + * @nv_idx:	The NV Index to be retrieved
> + * @attr_out:	A place to store returned attributes if a TPM 2 was used
> + * @out:	A pointer where to store the return buffer
> + *
> + * Return: number of bytes read or a negative error value
> + */
> +int tpm_nv_read(struct tpm_chip *chip, u32 nv_idx, u32 *attr_out, void **out)
> +{
> +	int rc;
> +
> +	chip = tpm_find_get_ops(chip);
> +	if (!chip)
> +		return -ENODEV;
> +
> +	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
> +		rc = tpm2_nv_read(chip, nv_idx, attr_out, out);
> +		goto out;
> +	}
> +
> +	rc = -ENODEV;
> +
> +out:
> +	tpm_put_ops(chip);
> +	return rc;
> +}
> +EXPORT_SYMBOL_GPL(tpm_nv_read);
> +
>   /**
>    * tpm_send - send a TPM command
>    * @chip:	a &struct tpm_chip instance, %NULL for the default chip
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index 947d1db0a5cc..d4dfc5148adb 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -56,9 +56,12 @@ enum tpm_addr {
>   #define TPM_ERR_DEACTIVATED     0x6
>   #define TPM_ERR_DISABLED        0x7
>   #define TPM_ERR_INVALID_POSTINIT 38
> +#define TPM_ERR_INVALID_HANDLE_1 0x18b
>   
>   #define TPM_TAG_RQU_COMMAND 193
>   
> +#define TPM2_HR_NV_INDEX 0x1000000
> +
>   /* TPM2 specific constants. */
>   #define TPM2_SPACE_BUFFER_SIZE		16384 /* 16 kB */
>   
> @@ -224,6 +227,8 @@ int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t max);
>   ssize_t tpm2_get_tpm_pt(struct tpm_chip *chip, u32 property_id,
>   			u32 *value, const char *desc);
>   
> +int tpm2_nv_read(struct tpm_chip *chip, u32 nvindex, u32 *attr_out, void **dest);
> +int tpm2_nv_readpublic(struct tpm_chip *chip, u32 nvindex, struct tpm2_nv_public *info);
>   ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip);
>   int tpm2_auto_startup(struct tpm_chip *chip);
>   void tpm2_shutdown(struct tpm_chip *chip, u16 shutdown_type);
> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
> index eff1f12d981a..ba1026123464 100644
> --- a/drivers/char/tpm/tpm2-cmd.c
> +++ b/drivers/char/tpm/tpm2-cmd.c
> @@ -269,6 +269,169 @@ int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
>   	return rc;
>   }
>   
> +struct tpm2_buffer_out {
> +	__be16	size;
> +	u8	data[];
> +} __packed;
> +
> +struct tpm2_nv_public_out {
> +	__be32	nvIndex;
> +	__be16	nameAlg;
> +	__be32	attributes;
> +	__be16	authPolicySize;
> +	u8	data[];
> +} __packed;
> +
> +int tpm2_nv_readpublic(struct tpm_chip *chip, u32 nvindex, struct tpm2_nv_public *info)
> +{
> +	struct tpm_buf buf;
> +	int rc;
> +	u16 recd;
> +	u32 resp_header_length;
> +	struct tpm2_buffer_out *out;
> +	struct tpm2_nv_public_out *publicout;
> +	u32 nvhandle;
> +	u16 auth_policy_size;
> +
> +	if ((nvindex & ~TPM2_HR_NV_INDEX) > 0x00FFFFFF)
> +		return -EINVAL;
> +
> +	/* HR_NV_INDEX = TPM_HT_NV_INDEX << HR_SHIFT */
> +	nvhandle = TPM2_HR_NV_INDEX | nvindex;
> +
> +	rc = tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS, TPM2_CC_NV_READPUBLIC);
> +	if (rc)
> +		return rc;
> +	tpm_buf_append_u32(&buf, nvhandle);
> +	rc = tpm_transmit_cmd(chip, &buf, 0, NULL);
> +	if (rc) {
> +		if (rc != TPM_ERR_DISABLED && rc != TPM_ERR_DEACTIVATED

These are TPM 1.2 error codes. I don't think TPM 2 returns those, does 
it? Ah, you took that from tpm_transmit_cmd... I think you can remove those.


> +		    && rc != TPM2_RC_TESTING && rc != TPM_ERR_INVALID_HANDLE_1)
> +			dev_err(&chip->dev, "A TPM error (%d) occurred attempting to read an NV Index public\n", rc);
> +		if (rc == TPM_ERR_INVALID_HANDLE_1)
> +			rc = -ENOENT;
> +		else if (rc > 0)
> +			rc = -EIO;
> +		goto out;
> +	}
> +	resp_header_length = tpm_buf_response_header_length(&buf, 0);
> +	out = (struct tpm2_buffer_out *)&buf.data[resp_header_length];
> +	publicout = (struct tpm2_nv_public_out *)&out->data;
> +	recd = be16_to_cpu(out->size);
> +
> +	info->nv_index = be32_to_cpu(publicout->nvIndex);
> +	info->name_alg = be16_to_cpu(publicout->nameAlg);
> +	info->attributes = be32_to_cpu(publicout->attributes);
> +
> +	/* Determine the size of the authPolicy, so we can skip over that to grab the data size */
> +	auth_policy_size = be16_to_cpu(publicout->authPolicySize);
> +
> +	info->data_size = be16_to_cpu((publicout->data[auth_policy_size]) | (publicout->data[auth_policy_size+1] << 8));


I don't think this is correct. The way you read the 2 bytes they are 
already in native format then due to the shifting. So be16_to_cpu should 
return the wrong result on little endian machines.


> +
> +out:
> +	tpm_buf_destroy(&buf);
> +	return rc;
> +}
> +
> +int tpm2_nv_read(struct tpm_chip *chip, u32 nvindex, u32 *attr_out, void **dest)
> +{
> +	struct tpm_buf buf;
> +	int rc;
> +	struct tpm2_buffer_out *out;
> +	u16 recd;
> +	u16 copied;
> +	u32 nvhandle;
> +	u32 resp_header_length;
> +	struct tpm2_null_auth_area auth_area;
> +	u16 size;
> +	struct tpm2_nv_public public;
> +
> +	copied = 0;
> +
> +	if ((nvindex & ~TPM2_HR_NV_INDEX) > 0x00FFFFFF)
> +		return -EINVAL;
> +
> +	/* HR_NV_INDEX = TPM_HT_NV_INDEX << HR_SHIFT */
> +	nvhandle = TPM2_HR_NV_INDEX | nvindex;
> +
> +	/* Determine the size of the NV Index contents */
> +	rc = tpm2_nv_readpublic(chip, nvindex, &public);
> +	if (rc < 0)
> +		return rc;
> +	if (attr_out != NULL)
> +		*attr_out = public.attributes;
> +	size = public.data_size;
> +	*dest = kzalloc(size, GFP_KERNEL);
> +	if (!*dest) {
> +		rc = -ENOMEM;
> +		goto out;
> +	}
> +
> +	/* Retrieve the actual NV Index contents */
> +	rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_NV_READ);
> +	if (rc)
> +		goto out_free;
> +
> +	while (copied < size) {
> +		tpm_buf_reset(&buf, TPM2_ST_SESSIONS, TPM2_CC_NV_READ);
> +
> +		tpm_buf_append_u32(&buf, TPM2_RH_OWNER);
> +		tpm_buf_append_u32(&buf, nvhandle);
> +
> +		auth_area.handle = cpu_to_be32(TPM2_RS_PW);
> +		auth_area.nonce_size = 0;
> +		auth_area.attributes = 0;
> +		auth_area.auth_size = 0;
> +
> +		tpm_buf_append_u32(&buf, sizeof(struct tpm2_null_auth_area));
> +		tpm_buf_append(&buf, (const unsigned char *)&auth_area,
> +			       sizeof(auth_area));
> +
> +		/* Size to request: at most 512 bytes at a time */
> +		tpm_buf_append_u16(&buf, min_t(u16, 512, size-copied));
> +		/* Offset: start at where we ended up */
> +		tpm_buf_append_u16(&buf, copied);
> +
> +		rc = tpm_transmit_cmd(chip, &buf, 0, "attempting to read NV index");
> +		if (rc) {
> +			if (rc > 0)
> +				rc = -EIO;
> +			goto out_free;
> +		}
> +		resp_header_length = tpm_buf_response_header_length(&buf, 0);
> +		out = (struct tpm2_buffer_out *)&buf.data[resp_header_length];
> +		recd = be16_to_cpu(out->size);
> +
> +		if (recd == 0) {
> +			rc = -EIO;
> +			goto out_free;
> +		}
> +		if (recd > size-copied) {
> +			rc = -EIO;
> +			goto out_free;
> +		}

You could add this to the above.

if (recd == 0 || recd > size - copied) ...

This address the case where the TPM is actually returning more bytes 
than requested.

> +
> +		memcpy(*dest + copied, out->data, recd);
> +		copied += recd;
> +	};
> +
> +out_free:
> +	if ((rc < 0) || (copied != size)) {
> +		kvfree(*dest);


kfree ?


> +		*dest = NULL;
> +	}
> +
> +out:
> +	tpm_buf_destroy(&buf);
> +
> +	if (rc < 0)
> +		return rc;
> +	else if (copied != size)
> +		return -EIO;
> +	else
> +		return size;
> +}
> +
>   struct tpm2_get_random_out {
>   	__be16 size;
>   	u8 buffer[TPM_MAX_RNG_DATA];
> diff --git a/include/linux/tpm.h b/include/linux/tpm.h
> index 8f4ff39f51e7..b812236b9955 100644
> --- a/include/linux/tpm.h
> +++ b/include/linux/tpm.h
> @@ -53,6 +53,40 @@ struct tpm_bank_info {
>   	u16 crypto_id;
>   };
>   
> +enum tpm_nv_public_attrs {
> +	TPM2_ATTR_NV_PPWRITE = 0x00000001,
> +	TPM2_ATTR_NV_OWNERWRITE = 0x00000002,
> +	TPM2_ATTR_NV_AUTHWRITE = 0x00000004,
> +	TPM2_ATTR_NV_POLICYWRITE = 0x00000008,
> +	/* Bits 4-7 TPM_NT */
> +	/* Bits 8-9 reserved */
> +	TPM2_ATTR_NV_POLICY_DELETE = 0x00000400,
> +	TPM2_ATTR_NV_WRITELOCKED = 0x00000800,
> +	TPM2_ATTR_NV_WRITEALL = 0x00001000,
> +	TPM2_ATTR_NV_WRITE_DEFINE = 0x00002000,
> +	TPM2_ATTR_NV_WRITE_STCLEAR = 0x00004000,
> +	TPM2_ATTR_NV_GLOBALLOCK = 0x00008000,
> +	TPM2_ATTR_NV_PPREAD = 0x00010000,
> +	TPM2_ATTR_NV_OWNERREAD = 0x00020000,
> +	TPM2_ATTR_NV_AUTHREAD = 0x00040000,
> +	TPM2_ATTR_NV_POLICYREAD = 0x00080000,
> +	/* Bits 20-24 reserved */
> +	TPM2_ATTR_NV_NO_DA = 0x02000000,
> +	TPM2_ATTR_NV_ORDERLY = 0x04000000,
> +	TPM2_ATTR_NV_CLEAR_STCLEAR = 0x08000000,
> +	TPM2_ATTR_NV_READLOCKED = 0x10000000,
> +	TPM2_ATTR_NV_WRITTEN = 0x20000000,
> +	TPM2_ATTR_NV_PLATFORMCREATE = 0x40000000,
> +	TPM2_ATTR_NV_READ_STCLEAR = 0x80000000,
> +};
> +
> +struct tpm2_nv_public {
> +	u32 nv_index;
> +	u16 name_alg;
> +	u32 attributes;
> +	u16 data_size;
> +};
> +
>   enum TPM_OPS_FLAGS {
>   	TPM_OPS_AUTO_STARTUP = BIT(0),
>   };
> @@ -189,6 +223,10 @@ enum tpm2_structures {
>   	TPM2_ST_SESSIONS	= 0x8002,
>   };
>   
> +enum tpm2_root_handles {
> +	TPM2_RH_OWNER		= 0x40000001,
> +};
> +
>   /* Indicates from what layer of the software stack the error comes from */
>   #define TSS2_RC_LAYER_SHIFT	 16
>   #define TSS2_RESMGR_TPM_RC_LAYER (11 << TSS2_RC_LAYER_SHIFT)
> @@ -223,6 +261,7 @@ enum tpm2_command_codes {
>   	TPM2_CC_CONTEXT_LOAD	        = 0x0161,
>   	TPM2_CC_CONTEXT_SAVE	        = 0x0162,
>   	TPM2_CC_FLUSH_CONTEXT	        = 0x0165,
> +	TPM2_CC_NV_READPUBLIC		= 0x0169,
>   	TPM2_CC_VERIFY_SIGNATURE        = 0x0177,
>   	TPM2_CC_GET_CAPABILITY	        = 0x017A,
>   	TPM2_CC_GET_RANDOM	        = 0x017B,
> @@ -389,6 +428,21 @@ static inline void tpm_buf_append_u32(struct tpm_buf *buf, const u32 value)
>   	tpm_buf_append(buf, (u8 *) &value2, 4);
>   }
>   
> +static inline u32 tpm_buf_response_header_length(struct tpm_buf *buf, bool has_shielded_locations)
> +{
> +	u32 header_length = TPM_HEADER_SIZE;
> +
> +	/* Possibly a handle for a Shielded Location */
> +	if (has_shielded_locations)
> +		header_length += 4;
> +
> +	/* Possibly the 32-bit parameter area size */
> +	if (tpm_buf_tag(buf) == TPM2_ST_SESSIONS)
> +		header_length += 4;
> +
> +	return header_length;
> +}
> +
>   static inline u32 tpm2_rc_value(u32 rc)
>   {
>   	return (rc & BIT(7)) ? rc & 0xff : rc;
> @@ -401,6 +455,7 @@ extern int tpm_pcr_read(struct tpm_chip *chip, u32 pcr_idx,
>   			struct tpm_digest *digest);
>   extern int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
>   			  struct tpm_digest *digests);
> +extern int tpm_nv_read(struct tpm_chip *chip, u32 nv_idx, u32 *attrs_out, void **out);
>   extern int tpm_send(struct tpm_chip *chip, void *cmd, size_t buflen);
>   extern int tpm_get_random(struct tpm_chip *chip, u8 *data, size_t max);
>   extern struct tpm_chip *tpm_default_chip(void);
> @@ -423,6 +478,16 @@ static inline int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
>   	return -ENODEV;
>   }
>   
> +static inline int tpm2_nv_readpublic(struct tpm_chip *chip, u32 nvindex, struct tpm2_nv_public *info)
> +{
> +	return -ENODEV;
> +}
> +
> +static inline int tpm_nv_read(struct tpm_chip *chip, u32 nv_idx, u8 *out, size_t max)
> +{


tpm2_nv_read


> +	return -ENODEV;
> +}
> +
>   static inline int tpm_send(struct tpm_chip *chip, void *cmd, size_t buflen)
>   {
>   	return -ENODEV;

Re: [PATCH 1/3] tpm: Add support for reading a TPM NV Index

[Jarkko Sakkinen]

On Thu, Feb 25, 2021 at 09:32:27PM +0100, Patrick Uiterwijk wrote:
> Add support to read contents from a TPM2 Non-Volatile Index location,
> allowing the kernel to retrieve contents and attributes of NV indexes.
> 
> Signed-off-by: Patrick Uiterwijk <patrick@puiterwijk.org>

How many call sites?

/Jarkko

[PATCH 2/3] integrity: Allow specifying flags in integrity_load_cert

[Patrick Uiterwijk]

Allows passing flags for key_create_or_update via
integrity_load_cert.

Signed-off-by: Patrick Uiterwijk <patrick@puiterwijk.org>
---
 security/integrity/digsig.c                          | 11 ++++++-----
 security/integrity/integrity.h                       |  6 ++++--
 security/integrity/platform_certs/platform_keyring.c |  2 +-
 3 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
index 250fb0836156..93203c767b57 100644
--- a/security/integrity/digsig.c
+++ b/security/integrity/digsig.c
@@ -144,7 +144,7 @@ int __init integrity_init_keyring(const unsigned int id)
 }
 
 static int __init integrity_add_key(const unsigned int id, const void *data,
-				    off_t size, key_perm_t perm)
+				    off_t size, key_perm_t perm, unsigned long flags)
 {
 	key_ref_t key;
 	int rc = 0;
@@ -154,7 +154,7 @@ static int __init integrity_add_key(const unsigned int id, const void *data,
 
 	key = key_create_or_update(make_key_ref(keyring[id], 1), "asymmetric",
 				   NULL, data, size, perm,
-				   KEY_ALLOC_NOT_IN_QUOTA);
+				   flags | KEY_ALLOC_NOT_IN_QUOTA);
 	if (IS_ERR(key)) {
 		rc = PTR_ERR(key);
 		pr_err("Problem loading X.509 certificate %d\n", rc);
@@ -186,18 +186,19 @@ int __init integrity_load_x509(const unsigned int id, const char *path)
 	perm = (KEY_POS_ALL & ~KEY_POS_SETATTR) | KEY_USR_VIEW | KEY_USR_READ;
 
 	pr_info("Loading X.509 certificate: %s\n", path);
-	rc = integrity_add_key(id, (const void *)data, size, perm);
+	rc = integrity_add_key(id, (const void *)data, size, perm, 0);
 
 	vfree(data);
 	return rc;
 }
 
 int __init integrity_load_cert(const unsigned int id, const char *source,
-			       const void *data, size_t len, key_perm_t perm)
+			       const void *data, size_t len, key_perm_t perm,
+			       unsigned long flags)
 {
 	if (!data)
 		return -EINVAL;
 
 	pr_info("Loading X.509 certificate: %s\n", source);
-	return integrity_add_key(id, data, len, perm);
+	return integrity_add_key(id, data, len, perm, flags);
 }
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index 547425c20e11..1194ff71a1c1 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -166,7 +166,8 @@ int integrity_modsig_verify(unsigned int id, const struct modsig *modsig);
 int __init integrity_init_keyring(const unsigned int id);
 int __init integrity_load_x509(const unsigned int id, const char *path);
 int __init integrity_load_cert(const unsigned int id, const char *source,
-			       const void *data, size_t len, key_perm_t perm);
+			       const void *data, size_t len, key_perm_t perm,
+			       unsigned long flags);
 #else
 
 static inline int integrity_digsig_verify(const unsigned int id,
@@ -190,7 +191,8 @@ static inline int integrity_init_keyring(const unsigned int id)
 static inline int __init integrity_load_cert(const unsigned int id,
 					     const char *source,
 					     const void *data, size_t len,
-					     key_perm_t perm)
+					     key_perm_t perm,
+					     unsigned long flags)
 {
 	return 0;
 }
diff --git a/security/integrity/platform_certs/platform_keyring.c b/security/integrity/platform_certs/platform_keyring.c
index bcafd7387729..131462c826b5 100644
--- a/security/integrity/platform_certs/platform_keyring.c
+++ b/security/integrity/platform_certs/platform_keyring.c
@@ -32,7 +32,7 @@ void __init add_to_platform_keyring(const char *source, const void *data,
 	perm = (KEY_POS_ALL & ~KEY_POS_SETATTR) | KEY_USR_VIEW;
 
 	rc = integrity_load_cert(INTEGRITY_KEYRING_PLATFORM, source, data, len,
-				 perm);
+				 perm, 0);
 	if (rc)
 		pr_info("Error adding keys to platform keyring %s\n", source);
 }
-- 
2.29.2

Re: [PATCH 2/3] integrity: Allow specifying flags in integrity_load_cert

[Stefan Berger]

On 2/25/21 3:32 PM, Patrick Uiterwijk wrote:
> Allows passing flags for key_create_or_update via
> integrity_load_cert.
>
> Signed-off-by: Patrick Uiterwijk <patrick@puiterwijk.org>


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


> ---
>   security/integrity/digsig.c                          | 11 ++++++-----
>   security/integrity/integrity.h                       |  6 ++++--
>   security/integrity/platform_certs/platform_keyring.c |  2 +-
>   3 files changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
> index 250fb0836156..93203c767b57 100644
> --- a/security/integrity/digsig.c
> +++ b/security/integrity/digsig.c
> @@ -144,7 +144,7 @@ int __init integrity_init_keyring(const unsigned int id)
>   }
>   
>   static int __init integrity_add_key(const unsigned int id, const void *data,
> -				    off_t size, key_perm_t perm)
> +				    off_t size, key_perm_t perm, unsigned long flags)
>   {
>   	key_ref_t key;
>   	int rc = 0;
> @@ -154,7 +154,7 @@ static int __init integrity_add_key(const unsigned int id, const void *data,
>   
>   	key = key_create_or_update(make_key_ref(keyring[id], 1), "asymmetric",
>   				   NULL, data, size, perm,
> -				   KEY_ALLOC_NOT_IN_QUOTA);
> +				   flags | KEY_ALLOC_NOT_IN_QUOTA);
>   	if (IS_ERR(key)) {
>   		rc = PTR_ERR(key);
>   		pr_err("Problem loading X.509 certificate %d\n", rc);
> @@ -186,18 +186,19 @@ int __init integrity_load_x509(const unsigned int id, const char *path)
>   	perm = (KEY_POS_ALL & ~KEY_POS_SETATTR) | KEY_USR_VIEW | KEY_USR_READ;
>   
>   	pr_info("Loading X.509 certificate: %s\n", path);
> -	rc = integrity_add_key(id, (const void *)data, size, perm);
> +	rc = integrity_add_key(id, (const void *)data, size, perm, 0);
>   
>   	vfree(data);
>   	return rc;
>   }
>   
>   int __init integrity_load_cert(const unsigned int id, const char *source,
> -			       const void *data, size_t len, key_perm_t perm)
> +			       const void *data, size_t len, key_perm_t perm,
> +			       unsigned long flags)
>   {
>   	if (!data)
>   		return -EINVAL;
>   
>   	pr_info("Loading X.509 certificate: %s\n", source);
> -	return integrity_add_key(id, data, len, perm);
> +	return integrity_add_key(id, data, len, perm, flags);
>   }
> diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
> index 547425c20e11..1194ff71a1c1 100644
> --- a/security/integrity/integrity.h
> +++ b/security/integrity/integrity.h
> @@ -166,7 +166,8 @@ int integrity_modsig_verify(unsigned int id, const struct modsig *modsig);
>   int __init integrity_init_keyring(const unsigned int id);
>   int __init integrity_load_x509(const unsigned int id, const char *path);
>   int __init integrity_load_cert(const unsigned int id, const char *source,
> -			       const void *data, size_t len, key_perm_t perm);
> +			       const void *data, size_t len, key_perm_t perm,
> +			       unsigned long flags);
>   #else
>   
>   static inline int integrity_digsig_verify(const unsigned int id,
> @@ -190,7 +191,8 @@ static inline int integrity_init_keyring(const unsigned int id)
>   static inline int __init integrity_load_cert(const unsigned int id,
>   					     const char *source,
>   					     const void *data, size_t len,
> -					     key_perm_t perm)
> +					     key_perm_t perm,
> +					     unsigned long flags)
>   {
>   	return 0;
>   }
> diff --git a/security/integrity/platform_certs/platform_keyring.c b/security/integrity/platform_certs/platform_keyring.c
> index bcafd7387729..131462c826b5 100644
> --- a/security/integrity/platform_certs/platform_keyring.c
> +++ b/security/integrity/platform_certs/platform_keyring.c
> @@ -32,7 +32,7 @@ void __init add_to_platform_keyring(const char *source, const void *data,
>   	perm = (KEY_POS_ALL & ~KEY_POS_SETATTR) | KEY_USR_VIEW;
>   
>   	rc = integrity_load_cert(INTEGRITY_KEYRING_PLATFORM, source, data, len,
> -				 perm);
> +				 perm, 0);
>   	if (rc)
>   		pr_info("Error adding keys to platform keyring %s\n", source);
>   }

[PATCH 3/3] integrity: Load keys from TPM NV onto IMA keyring

[Patrick Uiterwijk]

Allows users to enroll their own public key stored in a specific TPM2
NV Index, requiring the absence of the Platform Create and Platform
Write attributes on the NV Index, to be loaded on the IMA keyring.

Provides a method for users to load keys without the need to recompile
the kernel or change the kernel binary, which would require a resign of
the kernel image.

Signed-off-by: Patrick Uiterwijk <patrick@puiterwijk.org>
---
 security/integrity/ima/Kconfig    | 22 +++++++++++++
 security/integrity/ima/ima_init.c | 53 +++++++++++++++++++++++++++++++
 2 files changed, 75 insertions(+)

diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
index 12e9250c1bec..28424b930c81 100644
--- a/security/integrity/ima/Kconfig
+++ b/security/integrity/ima/Kconfig
@@ -291,6 +291,28 @@ config IMA_BLACKLIST_KEYRING
 	   the search is successful the requested operation is rejected and
 	   an error is returned to the caller.
 
+config IMA_LOAD_CERT_NVINDEX
+	bool "Load certificate from TPM nvindex into '.ima' trusted keyring"
+	depends on IMA_TRUSTED_KEYRING && TCG_TPM
+	default n
+	help
+	   File signature verification is based on the public keys
+	   loaded on the .ima trusted keyring. These public keys are
+	   X509 certificates signed by a trusted key on the
+	   .system keyring.  This option enables X509 certificate
+	   loading by the kernel onto the '.ima' trusted keyring
+	   from a TPM nvindex, bypassing the builtin keyring check.
+
+config IMA_LOAD_CERT_NVINDEX_INDEX
+	hex "The TPM NV Index to load into the '.ima' trusted keyring"
+	depends on IMA_LOAD_CERT_NVINDEX
+	default 0x184b520
+	help
+	   Defines the index of the NV Index that gets loaded into the
+	   '.ima' keyring.
+	   The default is the "0x18" prefix for a non-TCG specified NV Index,
+	   suffixed with ASCII for "KR" (keyring) and then 0
+
 config IMA_LOAD_X509
 	bool "Load X509 certificate onto the '.ima' trusted keyring"
 	depends on IMA_TRUSTED_KEYRING
diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c
index 6e8742916d1d..ea0949e8df12 100644
--- a/security/integrity/ima/ima_init.c
+++ b/security/integrity/ima/ima_init.c
@@ -112,6 +112,55 @@ void __init ima_load_x509(void)
 }
 #endif
 
+#ifndef CONFIG_IMA_LOAD_CERT_NVINDEX
+int __init ima_load_key_nvindex(void)
+{
+	return 0;
+}
+#else
+int __init ima_load_key_nvindex(void)
+{
+	void *cert_buffer;
+	int rc;
+	key_perm_t perm;
+	u32 nvindex_attributes = 0;
+
+	rc = tpm_nv_read(tpm_default_chip(),
+				CONFIG_IMA_LOAD_CERT_NVINDEX_INDEX,
+				&nvindex_attributes, &cert_buffer);
+	if (rc < 0) {
+		if (rc == -ENODEV)  /* No TPM2 */
+			rc = 0;
+		if (rc == -ENOENT)  /* No certificate in NV Index */
+			rc = 0;
+		goto out;
+	}
+
+	pr_info("Loading IMA key from TPM NV Index 0x%x", CONFIG_IMA_LOAD_CERT_NVINDEX_INDEX);
+
+	if (nvindex_attributes & TPM2_ATTR_NV_PLATFORMCREATE) {
+		pr_err("NV Index has the Platform Create attribute");
+		rc = -EACCES;
+		goto out_free;
+	}
+	if (nvindex_attributes & TPM2_ATTR_NV_PPWRITE) {
+		pr_err("NV Index has the Platform Write attribute");
+		rc = -EACCES;
+		goto out_free;
+	}
+
+	perm = (KEY_POS_ALL & ~KEY_POS_SETATTR) | KEY_USR_VIEW | KEY_USR_READ;
+	rc = integrity_load_cert(INTEGRITY_KEYRING_IMA, "TPM NV Index",
+				 cert_buffer, rc, perm,
+				 KEY_ALLOC_BYPASS_RESTRICTION);
+
+out_free:
+	kvfree(cert_buffer);
+out:
+	return rc;
+}
+#endif
+
 int __init ima_init(void)
 {
 	int rc;
@@ -124,6 +173,10 @@ int __init ima_init(void)
 	if (rc)
 		return rc;
 
+	rc = ima_load_key_nvindex();
+	if (rc)
+		pr_info("Failed to load IMA key from TPM NV Index (%d)", rc);
+
 	rc = ima_init_crypto();
 	if (rc)
 		return rc;
-- 
2.29.2

Re: [PATCH 3/3] integrity: Load keys from TPM NV onto IMA keyring

[Stefan Berger]

On 2/25/21 3:32 PM, Patrick Uiterwijk wrote:
> Allows users to enroll their own public key stored in a specific TPM2
> NV Index, requiring the absence of the Platform Create and Platform
> Write attributes on the NV Index, to be loaded on the IMA keyring.
>
> Provides a method for users to load keys without the need to recompile
> the kernel or change the kernel binary, which would require a resign of
> the kernel image.
>
> Signed-off-by: Patrick Uiterwijk <patrick@puiterwijk.org>
> ---
>   security/integrity/ima/Kconfig    | 22 +++++++++++++
>   security/integrity/ima/ima_init.c | 53 +++++++++++++++++++++++++++++++
>   2 files changed, 75 insertions(+)
>
> diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
> index 12e9250c1bec..28424b930c81 100644
> --- a/security/integrity/ima/Kconfig
> +++ b/security/integrity/ima/Kconfig
> @@ -291,6 +291,28 @@ config IMA_BLACKLIST_KEYRING
>   	   the search is successful the requested operation is rejected and
>   	   an error is returned to the caller.
>   
> +config IMA_LOAD_CERT_NVINDEX
> +	bool "Load certificate from TPM nvindex into '.ima' trusted keyring"
> +	depends on IMA_TRUSTED_KEYRING && TCG_TPM
> +	default n
> +	help
> +	   File signature verification is based on the public keys
> +	   loaded on the .ima trusted keyring. These public keys are
> +	   X509 certificates signed by a trusted key on the
> +	   .system keyring.  This option enables X509 certificate
> +	   loading by the kernel onto the '.ima' trusted keyring
> +	   from a TPM nvindex, bypassing the builtin keyring check.
> +
> +config IMA_LOAD_CERT_NVINDEX_INDEX
> +	hex "The TPM NV Index to load into the '.ima' trusted keyring"
> +	depends on IMA_LOAD_CERT_NVINDEX
> +	default 0x184b520
> +	help
> +	   Defines the index of the NV Index that gets loaded into the
> +	   '.ima' keyring.
> +	   The default is the "0x18" prefix for a non-TCG specified NV Index,
> +	   suffixed with ASCII for "KR" (keyring) and then 0
> +
>   config IMA_LOAD_X509
>   	bool "Load X509 certificate onto the '.ima' trusted keyring"
>   	depends on IMA_TRUSTED_KEYRING
> diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c
> index 6e8742916d1d..ea0949e8df12 100644
> --- a/security/integrity/ima/ima_init.c
> +++ b/security/integrity/ima/ima_init.c
> @@ -112,6 +112,55 @@ void __init ima_load_x509(void)
>   }
>   #endif
>   
> +#ifndef CONFIG_IMA_LOAD_CERT_NVINDEX
> +int __init ima_load_key_nvindex(void)
> +{
> +	return 0;
> +}
> +#else
> +int __init ima_load_key_nvindex(void)
> +{
> +	void *cert_buffer;
> +	int rc;
> +	key_perm_t perm;
> +	u32 nvindex_attributes = 0;
> +
> +	rc = tpm_nv_read(tpm_default_chip(),


You should do chip = tpm_default_chip() so that later on you can do 
put_device(&chip->dev).


> +				CONFIG_IMA_LOAD_CERT_NVINDEX_INDEX,
> +				&nvindex_attributes, &cert_buffer);
> +	if (rc < 0) {
> +		if (rc == -ENODEV)  /* No TPM2 */
> +			rc = 0;
> +		if (rc == -ENOENT)  /* No certificate in NV Index */
> +			rc = 0;
> +		goto out;
> +	}
> +
> +	pr_info("Loading IMA key from TPM NV Index 0x%x", CONFIG_IMA_LOAD_CERT_NVINDEX_INDEX);
> +
> +	if (nvindex_attributes & TPM2_ATTR_NV_PLATFORMCREATE) {
> +		pr_err("NV Index has the Platform Create attribute");
> +		rc = -EACCES;
> +		goto out_free;
> +	}
> +	if (nvindex_attributes & TPM2_ATTR_NV_PPWRITE) {
> +		pr_err("NV Index has the Platform Write attribute");
> +		rc = -EACCES;
> +		goto out_free;
> +	}
> +
> +	perm = (KEY_POS_ALL & ~KEY_POS_SETATTR) | KEY_USR_VIEW | KEY_USR_READ;
> +	rc = integrity_load_cert(INTEGRITY_KEYRING_IMA, "TPM NV Index",
> +				 cert_buffer, rc, perm,
> +				 KEY_ALLOC_BYPASS_RESTRICTION);
> +
> +out_free:
> +	kvfree(cert_buffer);


kfree?


> +out:
> +	return rc;
> +}
> +#endif
> +
>   int __init ima_init(void)
>   {
>   	int rc;
> @@ -124,6 +173,10 @@ int __init ima_init(void)
>   	if (rc)
>   		return rc;
>   
> +	rc = ima_load_key_nvindex();
> +	if (rc)
> +		pr_info("Failed to load IMA key from TPM NV Index (%d)", rc);
> +
>   	rc = ima_init_crypto();
>   	if (rc)
>   		return rc;

Re: [PATCH 3/3] integrity: Load keys from TPM NV onto IMA keyring

[Stefan Berger]

On 2/26/21 4:47 PM, Stefan Berger wrote:
> On 2/25/21 3:32 PM, Patrick Uiterwijk wrote:
>> Allows users to enroll their own public key stored in a specific TPM2
>> NV Index, requiring the absence of the Platform Create and Platform
>> Write attributes on the NV Index, to be loaded on the IMA keyring.
>>
>> Provides a method for users to load keys without the need to recompile
>> the kernel or change the kernel binary, which would require a resign of
>> the kernel image.
>>
>> Signed-off-by: Patrick Uiterwijk <patrick@puiterwijk.org>
>> ---
>>   security/integrity/ima/Kconfig    | 22 +++++++++++++
>>   security/integrity/ima/ima_init.c | 53 +++++++++++++++++++++++++++++++
>>   2 files changed, 75 insertions(+)
>>
>> diff --git a/security/integrity/ima/Kconfig 
>> b/security/integrity/ima/Kconfig
>> index 12e9250c1bec..28424b930c81 100644
>> --- a/security/integrity/ima/Kconfig
>> +++ b/security/integrity/ima/Kconfig
>> @@ -291,6 +291,28 @@ config IMA_BLACKLIST_KEYRING
>>          the search is successful the requested operation is rejected 
>> and
>>          an error is returned to the caller.
>>   +config IMA_LOAD_CERT_NVINDEX
>> +    bool "Load certificate from TPM nvindex into '.ima' trusted 
>> keyring"
>> +    depends on IMA_TRUSTED_KEYRING && TCG_TPM
>> +    default n
>> +    help
>> +       File signature verification is based on the public keys
>> +       loaded on the .ima trusted keyring. These public keys are
>> +       X509 certificates signed by a trusted key on the
>> +       .system keyring.  This option enables X509 certificate
>> +       loading by the kernel onto the '.ima' trusted keyring
>> +       from a TPM nvindex, bypassing the builtin keyring check.
>> +
>> +config IMA_LOAD_CERT_NVINDEX_INDEX
>> +    hex "The TPM NV Index to load into the '.ima' trusted keyring"
>> +    depends on IMA_LOAD_CERT_NVINDEX
>> +    default 0x184b520
>> +    help
>> +       Defines the index of the NV Index that gets loaded into the
>> +       '.ima' keyring.
>> +       The default is the "0x18" prefix for a non-TCG specified NV 
>> Index,
>> +       suffixed with ASCII for "KR" (keyring) and then 0
>> +
>>   config IMA_LOAD_X509
>>       bool "Load X509 certificate onto the '.ima' trusted keyring"
>>       depends on IMA_TRUSTED_KEYRING
>> diff --git a/security/integrity/ima/ima_init.c 
>> b/security/integrity/ima/ima_init.c
>> index 6e8742916d1d..ea0949e8df12 100644
>> --- a/security/integrity/ima/ima_init.c
>> +++ b/security/integrity/ima/ima_init.c
>> @@ -112,6 +112,55 @@ void __init ima_load_x509(void)
>>   }
>>   #endif
>>   +#ifndef CONFIG_IMA_LOAD_CERT_NVINDEX
>> +int __init ima_load_key_nvindex(void)
>> +{
>> +    return 0;
>> +}
>> +#else
>> +int __init ima_load_key_nvindex(void)
>> +{
>> +    void *cert_buffer;
>> +    int rc;
>> +    key_perm_t perm;
>> +    u32 nvindex_attributes = 0;
>> +
>> +    rc = tpm_nv_read(tpm_default_chip(),
>
>
> You should do chip = tpm_default_chip() so that later on you can do 
> put_device(&chip->dev).


... or just use ima_tpm_chip if != NULL: 
https://elixir.bootlin.com/linux/latest/source/security/integrity/ima/ima_init.c#L23



>
>
>> + CONFIG_IMA_LOAD_CERT_NVINDEX_INDEX,
>> +                &nvindex_attributes, &cert_buffer);
>> +    if (rc < 0) {
>> +        if (rc == -ENODEV)  /* No TPM2 */
>> +            rc = 0;
>> +        if (rc == -ENOENT)  /* No certificate in NV Index */
>> +            rc = 0;
>> +        goto out;
>> +    }
>> +
>> +    pr_info("Loading IMA key from TPM NV Index 0x%x", 
>> CONFIG_IMA_LOAD_CERT_NVINDEX_INDEX);
>> +
>> +    if (nvindex_attributes & TPM2_ATTR_NV_PLATFORMCREATE) {
>> +        pr_err("NV Index has the Platform Create attribute");
>> +        rc = -EACCES;
>> +        goto out_free;
>> +    }
>> +    if (nvindex_attributes & TPM2_ATTR_NV_PPWRITE) {
>> +        pr_err("NV Index has the Platform Write attribute");
>> +        rc = -EACCES;
>> +        goto out_free;
>> +    }
>> +
>> +    perm = (KEY_POS_ALL & ~KEY_POS_SETATTR) | KEY_USR_VIEW | 
>> KEY_USR_READ;
>> +    rc = integrity_load_cert(INTEGRITY_KEYRING_IMA, "TPM NV Index",
>> +                 cert_buffer, rc, perm,
>> +                 KEY_ALLOC_BYPASS_RESTRICTION);
>> +
>> +out_free:
>> +    kvfree(cert_buffer);
>
>
> kfree?
>
>
>> +out:
>> +    return rc;
>> +}
>> +#endif
>> +
>>   int __init ima_init(void)
>>   {
>>       int rc;
>> @@ -124,6 +173,10 @@ int __init ima_init(void)
>>       if (rc)
>>           return rc;
>>   +    rc = ima_load_key_nvindex();
>> +    if (rc)
>> +        pr_info("Failed to load IMA key from TPM NV Index (%d)", rc);
>> +
>>       rc = ima_init_crypto();
>>       if (rc)
>>           return rc;

Re: [PATCH 0/3] Load keys from TPM2 NV Index on IMA keyring

[James Bottomley]

On Thu, 2021-02-25 at 21:32 +0100, Patrick Uiterwijk wrote:
> The system's signature chain of trust is rooted in hardware and
> pivots to the keys baked into the kernel. IMA maintains this
> signature chain of trust by requiring any key being added to the IMA
> trusted keyring to be signed by a key on the builtin (or secondary)
> keyrings. This prevents a local key, needed for signing policies or
> other files, from being loaded on the IMA keyring, without requiring
> a custom built kernel (or injecting a key and resigning the kernel
> image).
> 
> Allow users to load their own public key stored in a specific TPM2 NV
> Index, requiring the absence of the Platform Create and Platform
> Write attributes on the NV Index, to be loaded on the IMA keyring.
> 
> To test this with the TPM2-software tools with a DER-encoded
> imacert.der:
>   tpm2_nvdefine -C o -s 945 0x184b520
>   tpm2_nvwrite -C o -i imacert.der 0x184b520
> 
> Or with the IBM TSS tools:
>   tssnvdefinespace -ha 0x184b520 -hi o -sz 945 +at ow +at or
>   tssnvwrite -hia o -ha 0x184b520 -if imacert.der
> 
> Then after a reboot, the ima keyring should contain the certificate.
> 
> Note that if this feature is enabled, users should make sure an NV
> Index is created with accurate attributes to prevent any other users
> from writing or deleting the NV Index. Without this precaution, any
> user who has access to the TPM would be able to write a key to the NV
> Index and have that key loaded on the IMA trusted keyring.
> 
> A distro who wants to enable this feature, for example, should ensure
> that the installer defines the NV Index in all cases, and only fills
> it if a key was provided by the user.

This has some problematic security implications:  any member of the tpm
group (which is pretty much all users if you use the TPM for user space
secrets or other operations) can read and write NV indexes.  What does
a distro do if the index is occupied on install (because it could be
some malicious entity who's put their cert in the index)?

> It is strongly adviced that any NV Index created for this purpose has
> at least the policy_delete and policywrite attributes set, together
> with a non-empty policy. Those flags make sure that the policy (which
> would be up to them to define) is required to be satisfied to delete
> or write the index.

This isn't necessarily good enough.  Unless the index has
PlatformCreate set, then any member of the tpm group can delete it with
TPM2_NV_UndefineIndex.  Creating stuff with TPM_NV_PLATFORMCREATE
attributes is possible, but whoever does must know the platform policy
or auth, so how would any distro get that if it's non standard (and if
it is standard then any tpm user can delete the index with
TPM2_NV_UndefineSpaceSpecial).

The bottom line is I don't see how this could safely be used by a
distribution in any standard manner, so why not simply pass the cert in
on the command line instead?  At least any random user can't then
compromise the process.

James

Re: [PATCH 0/3] Load keys from TPM2 NV Index on IMA keyring

[Ken Goldman]

[-- Attachment #1: Type: text/plain, Size: 3133 bytes --]


On 2/25/2021 4:50 PM, James Bottomley wrote:
> On Thu, 2021-02-25 at 21:32 +0100, Patrick Uiterwijk wrote:
>>
>> Note that if this feature is enabled, users should make sure an NV
>> Index is created with accurate attributes to prevent any other users
>> from writing or deleting the NV Index. Without this precaution, any
>> user who has access to the TPM would be able to write a key to the NV
>> Index and have that key loaded on the IMA trusted keyring.
>>
>> A distro who wants to enable this feature, for example, should ensure
>> that the installer defines the NV Index in all cases, and only fills
>> it if a key was provided by the user.
> 
> This has some problematic security implications:  any member of the tpm
> group (which is pretty much all users if you use the TPM for user space
> secrets or other operations) can read and write NV indexes.  What does
> a distro do if the index is occupied on install (because it could be
> some malicious entity who's put their cert in the index)?
> 
>> It is strongly adviced that any NV Index created for this purpose has
>> at least the policy_delete and policywrite attributes set, together
>> with a non-empty policy. Those flags make sure that the policy (which
>> would be up to them to define) is required to be satisfied to delete
>> or write the index.
> 
> This isn't necessarily good enough.  Unless the index has
> PlatformCreate set, then any member of the tpm group can delete it with
> TPM2_NV_UndefineIndex.  Creating stuff with TPM_NV_PLATFORMCREATE
> attributes is possible, but whoever does must know the platform policy
> or auth, so how would any distro get that if it's non standard (and if
> it is standard then any tpm user can delete the index with
> TPM2_NV_UndefineSpaceSpecial).
> 
> The bottom line is I don't see how this could safely be used by a
> distribution in any standard manner, so why not simply pass the cert in
> on the command line instead?  At least any random user can't then
> compromise the process.

Some ideas on this:

1 - Create the index such that it can be deleted by the platform (pre-OS,
physical presence) but not post-OS.

E.g, create with TPMA_NV_POLICY_DELETE in the owner hierarchy, and with a zero
length policy.  TPM2_Clear will delete it but the owner cannot.

2 - Permit reads with no authorization.

3 - Permit the first write (when the OS is first installed) with no authorization.

E.g., write if written is clear.

If worried about a malicious first write, remove this step and use #5.

4 - Permit writes when a password is supplied locally, the password supplied
during first install.

5 - For remote update, permit a write when given a signed authorization by an
update serve, e.g. a tang server.

E.g., the index policy says "accepts writes signed by this key".  The key
signs an authorization to write a specific new value to NV.
Also:

It is not enough for the application to read the index data.  The application
must read the index metadata (the Name in TCG jargon) to verify that the
index has the correct authorization metadata.



[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4490 bytes --]