tpmdd-devel.lists.sourceforge.net archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/5] Updated API for TPM 2.0 PCR extend
@ 2017-05-05 14:21 Roberto Sassu
  2017-05-05 14:21 ` [PATCH v2 1/5] tpm: introduce tpm_pcr_algorithms() Roberto Sassu
                   ` (4 more replies)
  0 siblings, 5 replies; 31+ messages in thread
From: Roberto Sassu @ 2017-05-05 14:21 UTC (permalink / raw)
  To: tpmdd-devel
  Cc: linux-ima-devel, linux-security-module, keyrings, linux-kernel,
	Roberto Sassu

The first version of the patch set can be retrieved at the URL:

https://sourceforge.net/p/tpmdd/mailman/message/35756302/

The patches in this set should be applied on top of the patch set
'tpm_pcr_extend() code split', which can be retrieved at the URL:

https://sourceforge.net/p/tpmdd/mailman/message/35820107/

This patch set updates the TPM driver API for extending Platform
Configuration Registers (PCRs). These are special TPM registers which
cannot be written directly, but can only be updated through the extend
operation, by passing a digest as input:

PCR_value_new = hash_func(PCR_value_old | digest)

While TPM 1.2 can only use SHA1 as hash function, TPM 2.0 can support
multiple algorithms. In the second case, PCR values extended with the same
algorithm are stored in a location called bank.

The primary use of the PCR extend operation is to protect the integrity
of measurements (e.g. of kernel, initial ram disk, application binaries),
which can be used by remote verifiers to determine if the software
running on a platform can be trusted to produce the expected outputs.
An example of software performing such measurements is Integrity
Measurement Architecture (IMA), which implements a set of hooks called
each time a subset of system calls, e.g. execve() or open(), is executed.

When IMA performs a measurement, it extends a PCR with the digest of a
measurement event log. The extend operation guarantees that modifications
of the measurements list can always be detected. Since PCRs cannot be
reverted to a previous value, it won't be possible for an attacker to hide
his actions by removing one of the log entries, because remote verifiers
would obtain a different value by replicating the extend operation with
the event log digests.

Currently, PCRs can only be extended from the kernel with a SHA1 digest,
through tpm_pcr_extend(). Remaining banks of a TPM 2.0 are extended with
the SHA1 digest padded with zeros. In order to take advantage of stronger
algorithms, IMA must be able to pass to the TPM driver interface digests
of different lengths. The second requirement comes from the TCG consortium,
which recommends to extend all banks, to prevent attackers from misusing
them. The third requirement is that TPM users should be able to obtain
from the driver interface TPM algorithm IDs, in order to produce an event
log with the format defined by TCG.

This patch set:

1) introduces tpm_pcr_algorithms(), to obtain the IDs of the algorithms
   supported by the TPM (TPM2_ALG_SHA1 is returned for TPM 1.2)

2) introduces tpm_pcr_algo_to_crypto() and tpm_pcr_algo_from_crypto() to
   convert TPM IDs to crypto IDs (in order to calculate the digest of
   an event log with the crypto subsystem), and vice-versa

3) modifies the parameters of tpm_pcr_extend(), to pass multiple digests

4) modifies the callers of tpm_pcr_extend(), to pass the correct arguments:
   - pcrlock() in security/keys/trusted.c
   - ima_pcr_extend() in security/integrity/ima/ima_queue.c

Given this definition of the tpm2_digest structure:

struct tpm2_digest {
	u16 alg_id;
	u8 digest[SHA512_DIGEST_SIZE];
} __packed;

these are the two methods to extend PCRs:

1) by passing only one tpm2_digest structure containing a SHA1 digest
   (as it is done in the patches 4/5 and 5/5); in this case, the SHA1
   digest is padded with zeros (current behavior)

2) by calling tpm_pcr_algorithms() to obtain the algorithms supported by
   the TPM, and by calling tpm_pcr_extend() with as many tpm2_digest
   structures as the number of algorithms retrieved in the first step


API Usage Examples

In the following examples, an application extends PCR 16 with the digest
of an event (e.g. record of a software measurement), with the methods
described above.

void app_calc_event_digest(struct crypto_shash *tfm, char *event,
                           u8 *digest)
{
    SHASH_DESC_ON_STACK(shash, tfm);

    shash->tfm = tfm;
    shash->flags = 0;

    crypto_shash_init(shash);
    crypto_shash_update(shash, event, strlen(event));
    crypto_shash_final(shash, digest);
}

void app_pcr_extend_method_1(void)
{
    char *event = "application event";
    struct tpm2_digest digestarg = {.alg_id = TPM2_ALG_SHA1};

    /* calculate event digest with current hash algorithm */
    struct crypto_shash *tfm = crypto_alloc_shash("sha1", 0, 0);

    app_calc_event_digest(tfm, event, digestarg.digest);

    /* extend all PCR banks with SHA1 digest*/
    tpm_pcr_extend(TPM_ANY_NUM, 16, 1, &digestarg);
}

void app_pcr_extend_method_2(void)
{
    /* declare static arrays, limit is known */
    enum tpm2_algorithms algo_array[TPM_ACTIVE_BANKS_MAX];
    struct tpm2_digest digest_array[TPM_ACTIVE_BANKS_MAX];
    int i, num_algo;

    /* obtain algorithms supported by the TPM */
    num_algo = tpm_pcr_algorithms(TPM_ANY_NUM, ARRAY_SIZE(algo_array),
                                  algo_array);

    for (i = 0; i < num_algo; i++) {
        char *event = "application event";

        /* convert TPM ID to crypto ID to calculate the digest */
        unsigned int crypto_id = tpm_pcr_algo_to_crypto(algo_array[i]);

        /* calculate event digest with current hash algorithm */
        const char *algo_name = hash_algo_name[crypto_id];
        struct crypto_shash *tfm = crypto_alloc_shash(algo_name, 0, 0);

        app_calc_event_digest(tfm, event, digest_array[i].digest);
        digest_array[i].alg_id = algo_array[i];
    }

    /* extend all PCR banks with calculated digests */
    tpm_pcr_extend(TPM_ANY_NUM, 16, num_algo, digest_array);
}

Changelog:

v2

- removed tpm2_digests_all_banks(); input check is now done by
  tpm_pcr_check_input(), called by tpm_pcr_extend(), also for TPM 1.2
- fixed return values of tpm2_pcr_algo_to_crypto() and
  tpm2_pcr_algo_from_crypto() if TPM is not supported
- tpm_pcr_algorithms() returns supported algorithms also for TPM 1.2
- removed tpm_pcr_extend_digests()
- modified parameters of tpm_pcr_extend()
- modified callers of tpm_pcr_extend()

Roberto Sassu (5):
  tpm: introduce tpm_pcr_algorithms()
  tpm: introduce tpm_pcr_algo_to_crypto() and tpm_pcr_algo_from_crypto()
  tpm: pass multiple digests to tpm_pcr_extend()
  keys, trusted: modify arguments of tpm_pcr_extend()
  ima: modify arguments of tpm_pcr_extend()

 drivers/char/tpm/tpm-interface.c   | 173 +++++++++++++++++++++++++++++++++++--
 drivers/char/tpm/tpm.h             |  19 +---
 drivers/char/tpm/tpm2-cmd.c        |  42 +++------
 include/linux/tpm.h                |  43 ++++++++-
 security/integrity/ima/ima_queue.c |   4 +-
 security/keys/trusted.c            |   6 +-
 6 files changed, 227 insertions(+), 60 deletions(-)

-- 
2.9.3


^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH v2 1/5] tpm: introduce tpm_pcr_algorithms()
  2017-05-05 14:21 [PATCH v2 0/5] Updated API for TPM 2.0 PCR extend Roberto Sassu
@ 2017-05-05 14:21 ` Roberto Sassu
  2017-05-15 10:36   ` Jarkko Sakkinen
  2017-05-05 14:21 ` [PATCH v2 2/5] tpm: introduce tpm_pcr_algo_to_crypto() and tpm_pcr_algo_from_crypto() Roberto Sassu
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 31+ messages in thread
From: Roberto Sassu @ 2017-05-05 14:21 UTC (permalink / raw)
  To: tpmdd-devel
  Cc: linux-ima-devel, linux-security-module, keyrings, linux-kernel,
	Roberto Sassu

This function allows TPM users to know which algorithms the TPM supports.
It stores the algorithms in a static array of 'enum tpm2_algorithms',
allocated by the caller. If the array is not large enough, the function
returns an error. Otherwise, it returns the number of algorithms written
to the array. If the TPM version is 1.2, the function writes TPM2_ALG_SHA1
to first element of the array.

Writing the algorithm also for TPM 1.2 has the advantage that callers
can use the API, tpm_pcr_algorithms() and tpm_pcr_extend(), regardless
of the TPM version.

A minor change added to this patch was to make available the size of
the active_banks array, member of the tpm_chip structure, outside
the TPM driver. The array size (TPM_ACTIVE_BANKS_MAX) has been exported
to include/linux/tpm.h.

With this information, callers of tpm_pcr_algorithms() can provide
a static array with enough room for all the algorithms, instead
of receiving the pointer of a dynamic array that they have to free later.

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
v2

- tpm_pcr_algorithms() returns supported algorithms also for TPM 1.2

 drivers/char/tpm/tpm-interface.c | 46 ++++++++++++++++++++++++++++++++++++++++
 drivers/char/tpm/tpm.h           | 13 +-----------
 include/linux/tpm.h              | 19 +++++++++++++++++
 3 files changed, 66 insertions(+), 12 deletions(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 4ed08ab..b90de3d 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -911,6 +911,52 @@ int tpm_pcr_extend(u32 chip_num, int pcr_idx, const u8 *hash)
 EXPORT_SYMBOL_GPL(tpm_pcr_extend);
 
 /**
+ * tpm_pcr_algorithms - get TPM IDs of active PCR banks algorithms
+ * @chip_num:	tpm idx # or ANY
+ * @count:	# of items in algorithms
+ * @algorithms:	array of TPM IDs
+ *
+ * Returns < 0 on error, and the number of active PCR banks on success.
+ *
+ * TPM 1.2 has always one SHA1 bank.
+ */
+int tpm_pcr_algorithms(u32 chip_num, int count,
+		       enum tpm2_algorithms *algorithms)
+{
+	struct tpm_chip *chip;
+	int i;
+
+	if (count <= 0 || algorithms == NULL)
+		return -EINVAL;
+
+	chip = tpm_chip_find_get(chip_num);
+	if (chip == NULL)
+		return -ENODEV;
+
+	if (!(chip->flags & TPM_CHIP_FLAG_TPM2)) {
+		algorithms[0] = TPM2_ALG_SHA1;
+		tpm_put_ops(chip);
+		return 1;
+	}
+
+	for (i = 0; i < ARRAY_SIZE(chip->active_banks) &&
+	     chip->active_banks[i] != TPM2_ALG_ERROR; i++) {
+		if (i >= count) {
+			dev_dbg(&chip->dev, "%s: insufficient array items %d\n",
+				__func__, count);
+			tpm_put_ops(chip);
+			return -EINVAL;
+		}
+
+		algorithms[i] = chip->active_banks[i];
+	}
+
+	tpm_put_ops(chip);
+	return i;
+}
+EXPORT_SYMBOL_GPL(tpm_pcr_algorithms);
+
+/**
  * tpm_do_selftest - have the TPM continue its selftest and wait until it
  *                   can receive further commands
  * @chip: TPM chip to use
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index e81d8c7..b22bc25 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -98,17 +98,6 @@ enum tpm2_return_codes {
 	TPM2_RC_REFERENCE_H0	= 0x0910,
 };
 
-enum tpm2_algorithms {
-	TPM2_ALG_ERROR		= 0x0000,
-	TPM2_ALG_SHA1		= 0x0004,
-	TPM2_ALG_KEYEDHASH	= 0x0008,
-	TPM2_ALG_SHA256		= 0x000B,
-	TPM2_ALG_SHA384		= 0x000C,
-	TPM2_ALG_SHA512		= 0x000D,
-	TPM2_ALG_NULL		= 0x0010,
-	TPM2_ALG_SM3_256	= 0x0012,
-};
-
 enum tpm2_command_codes {
 	TPM2_CC_FIRST		= 0x011F,
 	TPM2_CC_SELF_TEST	= 0x0143,
@@ -219,7 +208,7 @@ struct tpm_chip {
 	const struct attribute_group *groups[3];
 	unsigned int groups_cnt;
 
-	u16 active_banks[7];
+	u16 active_banks[TPM_ACTIVE_BANKS_MAX];
 #ifdef CONFIG_ACPI
 	acpi_handle acpi_dev_handle;
 	char ppi_version[TPM_PPI_VERSION_LEN + 1];
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index 5a090f5..b0d0061 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -23,6 +23,7 @@
 #define __LINUX_TPM_H__
 
 #define TPM_DIGEST_SIZE 20	/* Max TPM v1.2 PCR size */
+#define TPM_ACTIVE_BANKS_MAX 7	/* Max num of active banks for TPM 2.0 */
 
 /*
  * Chip num is this value or a valid tpm idx
@@ -37,6 +38,17 @@ enum TPM_OPS_FLAGS {
 	TPM_OPS_AUTO_STARTUP = BIT(0),
 };
 
+enum tpm2_algorithms {
+	TPM2_ALG_ERROR		= 0x0000,
+	TPM2_ALG_SHA1		= 0x0004,
+	TPM2_ALG_KEYEDHASH	= 0x0008,
+	TPM2_ALG_SHA256		= 0x000B,
+	TPM2_ALG_SHA384		= 0x000C,
+	TPM2_ALG_SHA512		= 0x000D,
+	TPM2_ALG_NULL		= 0x0010,
+	TPM2_ALG_SM3_256	= 0x0012,
+};
+
 struct tpm_class_ops {
 	unsigned int flags;
 	const u8 req_complete_mask;
@@ -57,6 +69,8 @@ struct tpm_class_ops {
 extern int tpm_is_tpm2(u32 chip_num);
 extern int tpm_pcr_read(u32 chip_num, int pcr_idx, u8 *res_buf);
 extern int tpm_pcr_extend(u32 chip_num, int pcr_idx, const u8 *hash);
+extern int tpm_pcr_algorithms(u32 chip_num, int count,
+			      enum tpm2_algorithms *algorithms);
 extern int tpm_send(u32 chip_num, void *cmd, size_t buflen);
 extern int tpm_get_random(u32 chip_num, u8 *data, size_t max);
 extern int tpm_seal_trusted(u32 chip_num,
@@ -76,6 +90,11 @@ static inline int tpm_pcr_read(u32 chip_num, int pcr_idx, u8 *res_buf) {
 static inline int tpm_pcr_extend(u32 chip_num, int pcr_idx, const u8 *hash) {
 	return -ENODEV;
 }
+static inline int tpm_pcr_algorithms(u32 chip_num, int count,
+				     enum tpm2_algorithms *algorithms)
+{
+	return -ENODEV;
+}
 static inline int tpm_send(u32 chip_num, void *cmd, size_t buflen) {
 	return -ENODEV;
 }
-- 
2.9.3


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v2 2/5] tpm: introduce tpm_pcr_algo_to_crypto() and tpm_pcr_algo_from_crypto()
  2017-05-05 14:21 [PATCH v2 0/5] Updated API for TPM 2.0 PCR extend Roberto Sassu
  2017-05-05 14:21 ` [PATCH v2 1/5] tpm: introduce tpm_pcr_algorithms() Roberto Sassu
@ 2017-05-05 14:21 ` Roberto Sassu
       [not found]   ` <20170505142152.29795-3-roberto.sassu-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
  2017-05-05 14:21 ` [PATCH v2 3/5] tpm: pass multiple digests to tpm_pcr_extend() Roberto Sassu
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 31+ messages in thread
From: Roberto Sassu @ 2017-05-05 14:21 UTC (permalink / raw)
  To: tpmdd-devel
  Cc: linux-ima-devel, linux-security-module, keyrings, linux-kernel,
	Roberto Sassu

tpm_pcr_algorithms() returns to its callers the IDs of the hash algorithms
supported by the TPM. This patch introduces tpm_pcr_algo_to_crypto(),
so that the callers can use the crypto subsystem to calculate the digest
to be passed to tpm_pcr_extend().

tpm_pcr_algo_from_crypto(), implemented for completeness, is instead used
by tpm2_seal_trusted() to perform the opposite conversion.

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
v2

- fixed return values of tpm2_pcr_algo_to_crypto() and
  tpm2_pcr_algo_from_crypto() if TPM support is disabled in the kernel

 drivers/char/tpm/tpm-interface.c | 51 ++++++++++++++++++++++++++++++++++++++++
 drivers/char/tpm/tpm2-cmd.c      | 42 +++++++++------------------------
 include/linux/tpm.h              | 13 ++++++++++
 3 files changed, 75 insertions(+), 31 deletions(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index b90de3d..aac703e 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -956,6 +956,57 @@ int tpm_pcr_algorithms(u32 chip_num, int count,
 }
 EXPORT_SYMBOL_GPL(tpm_pcr_algorithms);
 
+struct tpm2_hash {
+	unsigned int crypto_id;
+	unsigned int tpm_id;
+};
+
+static struct tpm2_hash tpm2_hash_map[] = {
+	{HASH_ALGO_SHA1, TPM2_ALG_SHA1},
+	{HASH_ALGO_SHA256, TPM2_ALG_SHA256},
+	{HASH_ALGO_SHA384, TPM2_ALG_SHA384},
+	{HASH_ALGO_SHA512, TPM2_ALG_SHA512},
+	{HASH_ALGO_SM3_256, TPM2_ALG_SM3_256},
+};
+
+/**
+ * tpm_pcr_algo_to_crypto() - convert from TPM ID to crypto ID
+ * @tpm_id:	TPM ID
+ *
+ * Return: crypto ID
+ */
+enum hash_algo tpm_pcr_algo_to_crypto(enum tpm2_algorithms tpm_id)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(tpm2_hash_map); i++) {
+		if (tpm_id == tpm2_hash_map[i].tpm_id)
+			return tpm2_hash_map[i].crypto_id;
+	}
+
+	return HASH_ALGO__LAST;
+}
+EXPORT_SYMBOL_GPL(tpm_pcr_algo_to_crypto);
+
+/**
+ * tpm_pcr_algo_from_crypto() - convert from crypto ID to TPM ID
+ * @crypto_id:	crypto ID
+ *
+ * Return: TPM ID
+ */
+enum tpm2_algorithms tpm_pcr_algo_from_crypto(enum hash_algo crypto_id)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(tpm2_hash_map); i++) {
+		if (crypto_id == tpm2_hash_map[i].crypto_id)
+			return tpm2_hash_map[i].tpm_id;
+	}
+
+	return TPM2_ALG_ERROR;
+}
+EXPORT_SYMBOL_GPL(tpm_pcr_algo_from_crypto);
+
 /**
  * tpm_do_selftest - have the TPM continue its selftest and wait until it
  *                   can receive further commands
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index 3ee6883..828a688 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -92,19 +92,6 @@ struct tpm2_cmd {
 	union tpm2_cmd_params	params;
 } __packed;
 
-struct tpm2_hash {
-	unsigned int crypto_id;
-	unsigned int tpm_id;
-};
-
-static struct tpm2_hash tpm2_hash_map[] = {
-	{HASH_ALGO_SHA1, TPM2_ALG_SHA1},
-	{HASH_ALGO_SHA256, TPM2_ALG_SHA256},
-	{HASH_ALGO_SHA384, TPM2_ALG_SHA384},
-	{HASH_ALGO_SHA512, TPM2_ALG_SHA512},
-	{HASH_ALGO_SM3_256, TPM2_ALG_SM3_256},
-};
-
 /*
  * Array with one entry per ordinal defining the maximum amount
  * of time the chip could take to return the result. The values
@@ -301,7 +288,6 @@ int tpm2_pcr_extend(struct tpm_chip *chip, int pcr_idx, u32 count,
 	struct tpm2_null_auth_area auth_area;
 	int rc;
 	int i;
-	int j;
 
 	if (count > ARRAY_SIZE(chip->active_banks))
 		return -EINVAL;
@@ -323,14 +309,15 @@ int tpm2_pcr_extend(struct tpm_chip *chip, int pcr_idx, u32 count,
 	tpm_buf_append_u32(&buf, count);
 
 	for (i = 0; i < count; i++) {
-		for (j = 0; j < ARRAY_SIZE(tpm2_hash_map); j++) {
-			if (digests[i].alg_id != tpm2_hash_map[j].tpm_id)
-				continue;
-			tpm_buf_append_u16(&buf, digests[i].alg_id);
-			tpm_buf_append(&buf, (const unsigned char
-					      *)&digests[i].digest,
-			       hash_digest_size[tpm2_hash_map[j].crypto_id]);
-		}
+		enum tpm2_algorithms tpm_id = digests[i].alg_id;
+		enum hash_algo crypto_id = tpm_pcr_algo_to_crypto(tpm_id);
+
+		if (crypto_id == HASH_ALGO__LAST)
+			continue;
+
+		tpm_buf_append_u16(&buf, digests[i].alg_id);
+		tpm_buf_append(&buf, (const unsigned char *)&digests[i].digest,
+			       hash_digest_size[crypto_id]);
 	}
 
 	rc = tpm_transmit_cmd(chip, NULL, buf.data, PAGE_SIZE, 0, 0,
@@ -493,17 +480,10 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
 	unsigned int blob_len;
 	struct tpm_buf buf;
 	u32 hash, rlength;
-	int i;
 	int rc;
 
-	for (i = 0; i < ARRAY_SIZE(tpm2_hash_map); i++) {
-		if (options->hash == tpm2_hash_map[i].crypto_id) {
-			hash = tpm2_hash_map[i].tpm_id;
-			break;
-		}
-	}
-
-	if (i == ARRAY_SIZE(tpm2_hash_map))
+	hash = tpm_pcr_algo_from_crypto(options->hash);
+	if (hash == TPM2_ALG_ERROR)
 		return -EINVAL;
 
 	rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_CREATE);
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index b0d0061..9ecd12c 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -22,6 +22,8 @@
 #ifndef __LINUX_TPM_H__
 #define __LINUX_TPM_H__
 
+#include <crypto/hash_info.h>
+
 #define TPM_DIGEST_SIZE 20	/* Max TPM v1.2 PCR size */
 #define TPM_ACTIVE_BANKS_MAX 7	/* Max num of active banks for TPM 2.0 */
 
@@ -71,6 +73,8 @@ extern int tpm_pcr_read(u32 chip_num, int pcr_idx, u8 *res_buf);
 extern int tpm_pcr_extend(u32 chip_num, int pcr_idx, const u8 *hash);
 extern int tpm_pcr_algorithms(u32 chip_num, int count,
 			      enum tpm2_algorithms *algorithms);
+extern enum hash_algo tpm_pcr_algo_to_crypto(enum tpm2_algorithms tpm_id);
+extern enum tpm2_algorithms tpm_pcr_algo_from_crypto(enum hash_algo crypto_id);
 extern int tpm_send(u32 chip_num, void *cmd, size_t buflen);
 extern int tpm_get_random(u32 chip_num, u8 *data, size_t max);
 extern int tpm_seal_trusted(u32 chip_num,
@@ -95,6 +99,15 @@ static inline int tpm_pcr_algorithms(u32 chip_num, int count,
 {
 	return -ENODEV;
 }
+static inline enum hash_algo tpm_pcr_algo_to_crypto(enum tpm2_algorithms tpm_id)
+{
+	return HASH_ALGO__LAST;
+}
+static inline enum tpm2_algorithms tpm_pcr_algo_from_crypto(
+						enum hash_algo crypto_id)
+{
+	return TPM2_ALG_ERROR;
+}
 static inline int tpm_send(u32 chip_num, void *cmd, size_t buflen) {
 	return -ENODEV;
 }
-- 
2.9.3


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v2 3/5] tpm: pass multiple digests to tpm_pcr_extend()
  2017-05-05 14:21 [PATCH v2 0/5] Updated API for TPM 2.0 PCR extend Roberto Sassu
  2017-05-05 14:21 ` [PATCH v2 1/5] tpm: introduce tpm_pcr_algorithms() Roberto Sassu
  2017-05-05 14:21 ` [PATCH v2 2/5] tpm: introduce tpm_pcr_algo_to_crypto() and tpm_pcr_algo_from_crypto() Roberto Sassu
@ 2017-05-05 14:21 ` Roberto Sassu
       [not found]   ` <20170505142152.29795-4-roberto.sassu-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
  2017-05-05 14:21 ` [PATCH v2 4/5] keys, trusted: modify arguments of tpm_pcr_extend() Roberto Sassu
  2017-05-05 14:21 ` [PATCH v2 5/5] ima: " Roberto Sassu
  4 siblings, 1 reply; 31+ messages in thread
From: Roberto Sassu @ 2017-05-05 14:21 UTC (permalink / raw)
  To: tpmdd-devel
  Cc: linux-ima-devel, linux-security-module, keyrings, linux-kernel,
	Roberto Sassu

The tpm_pcr_extend() definition has been modified to take an array of
tpm2_digest structures, and the size of the array as arguments.

The function now checks if callers provided a digests for each active
PCR bank (or a SHA1 digest for TPM 1.2), to follow the recomendation from
the TCG specifications. See commit c1f92b4b04ad ("tpm: enhance TPM 2.0
PCR extend to support multiple banks"). All banks should be extended
because unused banks could be used by an attacker to hide the true
integrity status of the platform.

The only allowed exception to the rule above is to pass a SHA1 digest.
It has been introduced to maintain compatibility with applications that
expect to interact with a TPM 1.2, and provide only a SHA1 digest.
In this case, the behavior of tpm_pcr_extend() is unchanged and
remaining PCR banks are extended with that digest, padded with zeros.

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
v2

- tpm_pcr_extend() arguments checked by tpm_pcr_check_input()
- modified parameters of tpm_pcr_extend()

 drivers/char/tpm/tpm-interface.c | 76 +++++++++++++++++++++++++++++++++++++---
 drivers/char/tpm/tpm.h           |  6 ----
 include/linux/tpm.h              | 11 ++++--
 3 files changed, 80 insertions(+), 13 deletions(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index aac703e..4b08b02 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -867,6 +867,55 @@ static int tpm1_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash,
 }
 
 /**
+ * tpm_pcr_check_input - check digests argument
+ *
+ * Return values:
+ *       1: input correct
+ *       0: fill digests with SHA1 digest padded with zeros
+ * -EINVAL: input incorrect
+ */
+static int tpm_pcr_check_input(struct tpm_chip *chip, int count,
+			       struct tpm2_digest *digests)
+{
+	bool sha1_only;
+	int found = 0, not_found = 0;
+	int i, j;
+
+	if (count <= 0 || digests == NULL)
+		return -EINVAL;
+
+	sha1_only = (count == 1 && digests[0].alg_id == TPM2_ALG_SHA1);
+
+	if (!(chip->flags & TPM_CHIP_FLAG_TPM2))
+		return sha1_only ? 1 : -EINVAL;
+
+	if (sha1_only)
+		return 0;
+
+	for (i = 0; i < ARRAY_SIZE(chip->active_banks) &&
+	     chip->active_banks[i] != TPM2_ALG_ERROR; i++) {
+		for (j = 0; j < count; j++) {
+			if (digests[j].alg_id == chip->active_banks[i]) {
+				found++;
+				break;
+			}
+		}
+
+		if (j == count) {
+			dev_dbg(&chip->dev, "%s: missing algorithm 0x%X\n",
+				__func__, chip->active_banks[i]);
+			not_found++;
+		}
+	}
+
+	if (not_found == 0 && found != count)
+		dev_dbg(&chip->dev,
+			"%s: duplicate or unsupported algorithm\n", __func__);
+
+	return (not_found == 0 && found == count) ? 1 : -EINVAL;
+}
+
+/**
  * tpm_pcr_extend - extend pcr value with hash
  * @chip_num:	tpm idx # or AN&
  * @pcr_idx:	pcr idx to extend
@@ -876,29 +925,46 @@ static int tpm1_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash,
  * isn't, protect against the chip disappearing, by incrementing
  * the module usage count.
  */
-int tpm_pcr_extend(u32 chip_num, int pcr_idx, const u8 *hash)
+int tpm_pcr_extend(u32 chip_num, int pcr_idx, int count,
+		   struct tpm2_digest *digests)
 {
 	int rc;
 	struct tpm_chip *chip;
 	struct tpm2_digest digest_list[ARRAY_SIZE(chip->active_banks)];
-	u32 count = 0;
+	struct tpm2_digest *digests_ptr = digests;
+	u32 filled_count = 0;
+	u8 *hash;
 	int i;
 
 	chip = tpm_chip_find_get(chip_num);
 	if (chip == NULL)
 		return -ENODEV;
 
-	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
+	rc = tpm_pcr_check_input(chip, count, digests);
+	if (rc < 0) {
+		dev_dbg(&chip->dev, "%s: invalid arguments\n", __func__);
+		tpm_put_ops(chip);
+		return rc;
+	}
+
+	hash = digests[0].digest;
+
+	if (!rc) {
 		memset(digest_list, 0, sizeof(digest_list));
 
 		for (i = 0; i < ARRAY_SIZE(chip->active_banks) &&
 			    chip->active_banks[i] != TPM2_ALG_ERROR; i++) {
 			digest_list[i].alg_id = chip->active_banks[i];
 			memcpy(digest_list[i].digest, hash, TPM_DIGEST_SIZE);
-			count++;
+			filled_count++;
 		}
 
-		rc = tpm2_pcr_extend(chip, pcr_idx, count, digest_list);
+		digests_ptr = digest_list;
+		count = filled_count;
+	}
+
+	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
+		rc = tpm2_pcr_extend(chip, pcr_idx, count, digests_ptr);
 		tpm_put_ops(chip);
 		return rc;
 	}
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index b22bc25..6d775c4 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -34,7 +34,6 @@
 #include <linux/acpi.h>
 #include <linux/cdev.h>
 #include <linux/highmem.h>
-#include <crypto/hash_info.h>
 
 enum tpm_const {
 	TPM_MINOR = 224,	/* officially assigned */
@@ -405,11 +404,6 @@ struct tpm_cmd_t {
 	tpm_cmd_params	params;
 } __packed;
 
-struct tpm2_digest {
-	u16 alg_id;
-	u8 digest[SHA512_DIGEST_SIZE];
-} __packed;
-
 /* A string buffer type for constructing TPM commands. This is based on the
  * ideas of string buffer code in security/keys/trusted.h but is heap based
  * in order to keep the stack usage minimal.
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index 9ecd12c..5c5a600 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -36,6 +36,11 @@ struct tpm_chip;
 struct trusted_key_payload;
 struct trusted_key_options;
 
+struct tpm2_digest {
+	u16 alg_id;
+	u8 digest[SHA512_DIGEST_SIZE];
+} __packed;
+
 enum TPM_OPS_FLAGS {
 	TPM_OPS_AUTO_STARTUP = BIT(0),
 };
@@ -70,7 +75,8 @@ struct tpm_class_ops {
 
 extern int tpm_is_tpm2(u32 chip_num);
 extern int tpm_pcr_read(u32 chip_num, int pcr_idx, u8 *res_buf);
-extern int tpm_pcr_extend(u32 chip_num, int pcr_idx, const u8 *hash);
+extern int tpm_pcr_extend(u32 chip_num, int pcr_idx, int count,
+			  struct tpm2_digest *digests);
 extern int tpm_pcr_algorithms(u32 chip_num, int count,
 			      enum tpm2_algorithms *algorithms);
 extern enum hash_algo tpm_pcr_algo_to_crypto(enum tpm2_algorithms tpm_id);
@@ -91,7 +97,8 @@ static inline int tpm_is_tpm2(u32 chip_num)
 static inline int tpm_pcr_read(u32 chip_num, int pcr_idx, u8 *res_buf) {
 	return -ENODEV;
 }
-static inline int tpm_pcr_extend(u32 chip_num, int pcr_idx, const u8 *hash) {
+static inline int tpm_pcr_extend(u32 chip_num, int pcr_idx, int count,
+				 struct tpm2_digest *digests) {
 	return -ENODEV;
 }
 static inline int tpm_pcr_algorithms(u32 chip_num, int count,
-- 
2.9.3


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v2 4/5] keys, trusted: modify arguments of tpm_pcr_extend()
  2017-05-05 14:21 [PATCH v2 0/5] Updated API for TPM 2.0 PCR extend Roberto Sassu
                   ` (2 preceding siblings ...)
  2017-05-05 14:21 ` [PATCH v2 3/5] tpm: pass multiple digests to tpm_pcr_extend() Roberto Sassu
@ 2017-05-05 14:21 ` Roberto Sassu
  2017-05-30  3:35   ` [Linux-ima-devel] " Mimi Zohar
  2017-05-05 14:21 ` [PATCH v2 5/5] ima: " Roberto Sassu
  4 siblings, 1 reply; 31+ messages in thread
From: Roberto Sassu @ 2017-05-05 14:21 UTC (permalink / raw)
  To: tpmdd-devel
  Cc: linux-ima-devel, linux-security-module, keyrings, linux-kernel,
	Roberto Sassu

pcrlock() has been modified to pass the correct arguments
to tpm_pcr_extend(): the pointer of a tpm2_digest structure containing
a random value generated by tpm_get_random() and the size of the array (1).

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 security/keys/trusted.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/security/keys/trusted.c b/security/keys/trusted.c
index 2ae31c5..3eb89e6 100644
--- a/security/keys/trusted.c
+++ b/security/keys/trusted.c
@@ -377,15 +377,15 @@ static int trusted_tpm_send(const u32 chip_num, unsigned char *cmd,
  */
 static int pcrlock(const int pcrnum)
 {
-	unsigned char hash[SHA1_DIGEST_SIZE];
+	struct tpm2_digest digestarg = {.alg_id = TPM2_ALG_SHA1};
 	int ret;
 
 	if (!capable(CAP_SYS_ADMIN))
 		return -EPERM;
-	ret = tpm_get_random(TPM_ANY_NUM, hash, SHA1_DIGEST_SIZE);
+	ret = tpm_get_random(TPM_ANY_NUM, digestarg.digest, SHA1_DIGEST_SIZE);
 	if (ret != SHA1_DIGEST_SIZE)
 		return ret;
-	return tpm_pcr_extend(TPM_ANY_NUM, pcrnum, hash) ? -EINVAL : 0;
+	return tpm_pcr_extend(TPM_ANY_NUM, pcrnum, 1, &digestarg) ? -EINVAL : 0;
 }
 
 /*
-- 
2.9.3

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v2 5/5] ima: modify arguments of tpm_pcr_extend()
  2017-05-05 14:21 [PATCH v2 0/5] Updated API for TPM 2.0 PCR extend Roberto Sassu
                   ` (3 preceding siblings ...)
  2017-05-05 14:21 ` [PATCH v2 4/5] keys, trusted: modify arguments of tpm_pcr_extend() Roberto Sassu
@ 2017-05-05 14:21 ` Roberto Sassu
  4 siblings, 0 replies; 31+ messages in thread
From: Roberto Sassu @ 2017-05-05 14:21 UTC (permalink / raw)
  To: tpmdd-devel
  Cc: linux-ima-devel, linux-security-module, keyrings, linux-kernel,
	Roberto Sassu

ima_pcr_extend() has been modified to pass the correct arguments
to tpm_pcr_extend(): the pointer of a tpm2_digest structure containing
the template digest and the size of the array (1).

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 security/integrity/ima/ima_queue.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c
index d9aa5ab..f628968 100644
--- a/security/integrity/ima/ima_queue.c
+++ b/security/integrity/ima/ima_queue.c
@@ -140,12 +140,14 @@ unsigned long ima_get_binary_runtime_size(void)
 
 static int ima_pcr_extend(const u8 *hash, int pcr)
 {
+	struct tpm2_digest digestarg = {.alg_id = TPM2_ALG_SHA1};
 	int result = 0;
 
 	if (!ima_used_chip)
 		return result;
 
-	result = tpm_pcr_extend(TPM_ANY_NUM, pcr, hash);
+	memcpy(digestarg.digest, hash, IMA_DIGEST_SIZE);
+	result = tpm_pcr_extend(TPM_ANY_NUM, pcr, 1, &digestarg);
 	if (result != 0)
 		pr_err("Error Communicating to TPM chip, result: %d\n", result);
 	return result;
-- 
2.9.3


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* Re: [PATCH v2 1/5] tpm: introduce tpm_pcr_algorithms()
  2017-05-05 14:21 ` [PATCH v2 1/5] tpm: introduce tpm_pcr_algorithms() Roberto Sassu
@ 2017-05-15 10:36   ` Jarkko Sakkinen
       [not found]     ` <20170515103623.sumyo2vyldezual2-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
  0 siblings, 1 reply; 31+ messages in thread
From: Jarkko Sakkinen @ 2017-05-15 10:36 UTC (permalink / raw)
  To: Roberto Sassu
  Cc: tpmdd-devel, linux-ima-devel, linux-security-module, keyrings,
	linux-kernel

On Fri, May 05, 2017 at 04:21:48PM +0200, Roberto Sassu wrote:
> This function allows TPM users to know which algorithms the TPM supports.
> It stores the algorithms in a static array of 'enum tpm2_algorithms',
> allocated by the caller. If the array is not large enough, the function
> returns an error. Otherwise, it returns the number of algorithms written
> to the array. If the TPM version is 1.2, the function writes TPM2_ALG_SHA1
> to first element of the array.
> 
> Writing the algorithm also for TPM 1.2 has the advantage that callers
> can use the API, tpm_pcr_algorithms() and tpm_pcr_extend(), regardless
> of the TPM version.
> 
> A minor change added to this patch was to make available the size of
> the active_banks array, member of the tpm_chip structure, outside
> the TPM driver. The array size (TPM_ACTIVE_BANKS_MAX) has been exported
> to include/linux/tpm.h.
> 
> With this information, callers of tpm_pcr_algorithms() can provide
> a static array with enough room for all the algorithms, instead
> of receiving the pointer of a dynamic array that they have to free later.
> 
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
> v2
> 
> - tpm_pcr_algorithms() returns supported algorithms also for TPM 1.2
> 
>  drivers/char/tpm/tpm-interface.c | 46 ++++++++++++++++++++++++++++++++++++++++
>  drivers/char/tpm/tpm.h           | 13 +-----------
>  include/linux/tpm.h              | 19 +++++++++++++++++
>  3 files changed, 66 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index 4ed08ab..b90de3d 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -911,6 +911,52 @@ int tpm_pcr_extend(u32 chip_num, int pcr_idx, const u8 *hash)
>  EXPORT_SYMBOL_GPL(tpm_pcr_extend);
>  
>  /**
> + * tpm_pcr_algorithms - get TPM IDs of active PCR banks algorithms

The grammar is incorrect here I believe. Should rather be

  "algorithms of the active PCR banks"

And there is no such thing as "TPM ID".

> + * @chip_num:	tpm idx # or ANY
> + * @count:	# of items in algorithms
> + * @algorithms:	array of TPM IDs
> + *
> + * Returns < 0 on error, and the number of active PCR banks on success.
> + *
> + * TPM 1.2 has always one SHA1 bank.
> + */
> +int tpm_pcr_algorithms(u32 chip_num, int count,
> +		       enum tpm2_algorithms *algorithms)
                       unsigned int

In addition the function name is not too greatg,

Your syntax for return value is not correct. In addition after
describing the return value there should not be anything. You should
study

https://www.kernel.org/doc/Documentation/kernel-doc-nano-HOWTO.txt

Better name for the function would be tpm_get_pcr_algorithms().

> +{
> +	struct tpm_chip *chip;
> +	int i;
> +
> +	if (count <= 0 || algorithms == NULL)
> +		return -EINVAL;

Is there a legal case where caller would pass these values? Now it
looks like that there is.

'count' should unsigned int and zero should be a legal value for
count.

That said I think the whole design is wrong as you could calculate
array for algs only one time and pass a const reference to it on
request.

/Jarkko

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v2 2/5] tpm: introduce tpm_pcr_algo_to_crypto() and tpm_pcr_algo_from_crypto()
       [not found]   ` <20170505142152.29795-3-roberto.sassu-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
@ 2017-05-15 11:16     ` Jarkko Sakkinen
       [not found]       ` <20170515111629.urjvbhqzohv4vakc-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
  0 siblings, 1 reply; 31+ messages in thread
From: Jarkko Sakkinen @ 2017-05-15 11:16 UTC (permalink / raw)
  To: Roberto Sassu
  Cc: linux-ima-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
	linux-security-module-u79uwXL29TY76Z2rM5mHXA,
	tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
	keyrings-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA

On Fri, May 05, 2017 at 04:21:49PM +0200, Roberto Sassu wrote:
> tpm_pcr_algorithms() returns to its callers the IDs of the hash algorithms
> supported by the TPM. This patch introduces tpm_pcr_algo_to_crypto(),
> so that the callers can use the crypto subsystem to calculate the digest
> to be passed to tpm_pcr_extend().
> 
> tpm_pcr_algo_from_crypto(), implemented for completeness, is instead used

What do you mean by completeness? Please, never add unused stuff.

> by tpm2_seal_trusted() to perform the opposite conversion.
> 
> Signed-off-by: Roberto Sassu <roberto.sassu-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> ---
> v2
> 
> - fixed return values of tpm2_pcr_algo_to_crypto() and
>   tpm2_pcr_algo_from_crypto() if TPM support is disabled in the kernel

Change Log only to the cover letter.

>  drivers/char/tpm/tpm-interface.c | 51 ++++++++++++++++++++++++++++++++++++++++
>  drivers/char/tpm/tpm2-cmd.c      | 42 +++++++++------------------------
>  include/linux/tpm.h              | 13 ++++++++++
>  3 files changed, 75 insertions(+), 31 deletions(-)

This commit is just deadly wrong in so many ways.

I would suggest to make extend always just take crypto ID in so you
don't have to add these bizarre conversion functions.

This probably covers all the patches but just "unsigned int" as type
for parameters and local variables.

I think I'll pass rest of the patches for now because just reviewing the
two first took too much time and wait for an updated version.

/Jarkko

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v2 1/5] tpm: introduce tpm_pcr_algorithms()
       [not found]     ` <20170515103623.sumyo2vyldezual2-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
@ 2017-05-15 13:18       ` Roberto Sassu
       [not found]         ` <d2974dd2-e30d-e8f0-a60a-2147f3290670-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
  2017-05-20 13:00         ` Jarkko Sakkinen
  0 siblings, 2 replies; 31+ messages in thread
From: Roberto Sassu @ 2017-05-15 13:18 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: linux-ima-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
	linux-security-module-u79uwXL29TY76Z2rM5mHXA,
	tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
	keyrings-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA



On 5/15/2017 12:36 PM, Jarkko Sakkinen wrote:
> On Fri, May 05, 2017 at 04:21:48PM +0200, Roberto Sassu wrote:
>> This function allows TPM users to know which algorithms the TPM supports.
>> It stores the algorithms in a static array of 'enum tpm2_algorithms',
>> allocated by the caller. If the array is not large enough, the function
>> returns an error. Otherwise, it returns the number of algorithms written
>> to the array. If the TPM version is 1.2, the function writes TPM2_ALG_SHA1
>> to first element of the array.
>>
>> Writing the algorithm also for TPM 1.2 has the advantage that callers
>> can use the API, tpm_pcr_algorithms() and tpm_pcr_extend(), regardless
>> of the TPM version.
>>
>> A minor change added to this patch was to make available the size of
>> the active_banks array, member of the tpm_chip structure, outside
>> the TPM driver. The array size (TPM_ACTIVE_BANKS_MAX) has been exported
>> to include/linux/tpm.h.
>>
>> With this information, callers of tpm_pcr_algorithms() can provide
>> a static array with enough room for all the algorithms, instead
>> of receiving the pointer of a dynamic array that they have to free later.
>>
>> Signed-off-by: Roberto Sassu <roberto.sassu-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
>> ---
>> v2
>>
>> - tpm_pcr_algorithms() returns supported algorithms also for TPM 1.2
>>
>>  drivers/char/tpm/tpm-interface.c | 46 ++++++++++++++++++++++++++++++++++++++++
>>  drivers/char/tpm/tpm.h           | 13 +-----------
>>  include/linux/tpm.h              | 19 +++++++++++++++++
>>  3 files changed, 66 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
>> index 4ed08ab..b90de3d 100644
>> --- a/drivers/char/tpm/tpm-interface.c
>> +++ b/drivers/char/tpm/tpm-interface.c
>> @@ -911,6 +911,52 @@ int tpm_pcr_extend(u32 chip_num, int pcr_idx, const u8 *hash)
>>  EXPORT_SYMBOL_GPL(tpm_pcr_extend);
>>
>>  /**
>> + * tpm_pcr_algorithms - get TPM IDs of active PCR banks algorithms
>
> The grammar is incorrect here I believe. Should rather be
>
>   "algorithms of the active PCR banks"
>
> And there is no such thing as "TPM ID".
>
>> + * @chip_num:	tpm idx # or ANY
>> + * @count:	# of items in algorithms
>> + * @algorithms:	array of TPM IDs
>> + *
>> + * Returns < 0 on error, and the number of active PCR banks on success.
>> + *
>> + * TPM 1.2 has always one SHA1 bank.
>> + */
>> +int tpm_pcr_algorithms(u32 chip_num, int count,
>> +		       enum tpm2_algorithms *algorithms)
>                        unsigned int
>
> In addition the function name is not too greatg,
>
> Your syntax for return value is not correct. In addition after
> describing the return value there should not be anything. You should
> study
>
> https://www.kernel.org/doc/Documentation/kernel-doc-nano-HOWTO.txt
>
> Better name for the function would be tpm_get_pcr_algorithms().
>
>> +{
>> +	struct tpm_chip *chip;
>> +	int i;
>> +
>> +	if (count <= 0 || algorithms == NULL)
>> +		return -EINVAL;
>
> Is there a legal case where caller would pass these values? Now it
> looks like that there is.
>
> 'count' should unsigned int and zero should be a legal value for
> count.

I wanted to avoid that a negative value returned by tpm_pcr_algorithms()
is passed to tpm_pcr_extend() as unsigned int.


> That said I think the whole design is wrong as you could calculate
> array for algs only one time and pass a const reference to it on
> request.

Ok. If I understood it correctly, you are saying to pass a const
reference of chip->active_banks. Then, I should also:

- add a new field (e.g. active_banks_num) in the tpm_chip structure
   to store the number of algorithms supported by the TPM (unless,
   I should determine it every time tpm_pcr_algorithms() is called)
- initialize chip->active_banks and chip->active_banks_num
   respectively to TPM2_ALG_SHA1 and 1 in tpm1_auto_startup()

Roberto

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v2 2/5] tpm: introduce tpm_pcr_algo_to_crypto() and tpm_pcr_algo_from_crypto()
       [not found]       ` <20170515111629.urjvbhqzohv4vakc-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
@ 2017-05-15 14:22         ` Roberto Sassu
  2017-05-20 13:22           ` Jarkko Sakkinen
  0 siblings, 1 reply; 31+ messages in thread
From: Roberto Sassu @ 2017-05-15 14:22 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: linux-ima-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
	linux-security-module-u79uwXL29TY76Z2rM5mHXA,
	tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
	keyrings-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA

On 5/15/2017 1:16 PM, Jarkko Sakkinen wrote:
> On Fri, May 05, 2017 at 04:21:49PM +0200, Roberto Sassu wrote:
>> tpm_pcr_algorithms() returns to its callers the IDs of the hash algorithms
>> supported by the TPM. This patch introduces tpm_pcr_algo_to_crypto(),
>> so that the callers can use the crypto subsystem to calculate the digest
>> to be passed to tpm_pcr_extend().
>>
>> tpm_pcr_algo_from_crypto(), implemented for completeness, is instead used
>
> What do you mean by completeness? Please, never add unused stuff.
>
>> by tpm2_seal_trusted() to perform the opposite conversion.
>>
>> Signed-off-by: Roberto Sassu <roberto.sassu-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
>> ---
>> v2
>>
>> - fixed return values of tpm2_pcr_algo_to_crypto() and
>>   tpm2_pcr_algo_from_crypto() if TPM support is disabled in the kernel
>
> Change Log only to the cover letter.
>
>>  drivers/char/tpm/tpm-interface.c | 51 ++++++++++++++++++++++++++++++++++++++++
>>  drivers/char/tpm/tpm2-cmd.c      | 42 +++++++++------------------------
>>  include/linux/tpm.h              | 13 ++++++++++
>>  3 files changed, 75 insertions(+), 31 deletions(-)
>
> This commit is just deadly wrong in so many ways.
>
> I would suggest to make extend always just take crypto ID in so you
> don't have to add these bizarre conversion functions.

The reason of this choice (as I explained in the cover letter)
is that TPM users might want to produce an event log with
the TCG format (which includes the TPM algorithm ID). Also,
TPM IDs should be preferred because, with them, TPM users
can calculate a digest directly with the TPM.

Taking crypto IDs means relying on the fact that there
is always a mapping between TPM IDs and crypto IDs.
Otherwise, tpm_pcr_algorithms() cannot return the algorithms
to its callers and PCRs cannot be extended. If TPM IDs are used,
TPM users have two alternatives: calculate the digest with
the TPM, or pass a SHA1 digest to tpm_pcr_extend(), so that
it can be padded to extend remanining PCR banks.

However, this second option will work only when the TPM driver
determines the size of an algorithm without relying on the crypto
subsystem. At the moment, if a TPM ID is not mapped to a crypto ID,
tpm2_pcr_extend() ignores the digest.

Roberto

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v2 1/5] tpm: introduce tpm_pcr_algorithms()
       [not found]         ` <d2974dd2-e30d-e8f0-a60a-2147f3290670-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
@ 2017-05-17  8:42           ` Roberto Sassu
  2017-05-20 13:18             ` Jarkko Sakkinen
  0 siblings, 1 reply; 31+ messages in thread
From: Roberto Sassu @ 2017-05-17  8:42 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: linux-ima-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
	linux-security-module-u79uwXL29TY76Z2rM5mHXA,
	tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
	keyrings-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA

On 5/15/2017 3:18 PM, Roberto Sassu wrote:
>
>
> On 5/15/2017 12:36 PM, Jarkko Sakkinen wrote:
>> On Fri, May 05, 2017 at 04:21:48PM +0200, Roberto Sassu wrote:
>>> This function allows TPM users to know which algorithms the TPM
>>> supports.
>>> It stores the algorithms in a static array of 'enum tpm2_algorithms',
>>> allocated by the caller. If the array is not large enough, the function
>>> returns an error. Otherwise, it returns the number of algorithms written
>>> to the array. If the TPM version is 1.2, the function writes
>>> TPM2_ALG_SHA1
>>> to first element of the array.
>>>
>>> Writing the algorithm also for TPM 1.2 has the advantage that callers
>>> can use the API, tpm_pcr_algorithms() and tpm_pcr_extend(), regardless
>>> of the TPM version.
>>>
>>> A minor change added to this patch was to make available the size of
>>> the active_banks array, member of the tpm_chip structure, outside
>>> the TPM driver. The array size (TPM_ACTIVE_BANKS_MAX) has been exported
>>> to include/linux/tpm.h.
>>>
>>> With this information, callers of tpm_pcr_algorithms() can provide
>>> a static array with enough room for all the algorithms, instead
>>> of receiving the pointer of a dynamic array that they have to free
>>> later.
>>>
>>> Signed-off-by: Roberto Sassu <roberto.sassu-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
>>> ---
>>> v2
>>>
>>> - tpm_pcr_algorithms() returns supported algorithms also for TPM 1.2
>>>
>>>  drivers/char/tpm/tpm-interface.c | 46
>>> ++++++++++++++++++++++++++++++++++++++++
>>>  drivers/char/tpm/tpm.h           | 13 +-----------
>>>  include/linux/tpm.h              | 19 +++++++++++++++++
>>>  3 files changed, 66 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/char/tpm/tpm-interface.c
>>> b/drivers/char/tpm/tpm-interface.c
>>> index 4ed08ab..b90de3d 100644
>>> --- a/drivers/char/tpm/tpm-interface.c
>>> +++ b/drivers/char/tpm/tpm-interface.c
>>> @@ -911,6 +911,52 @@ int tpm_pcr_extend(u32 chip_num, int pcr_idx,
>>> const u8 *hash)
>>>  EXPORT_SYMBOL_GPL(tpm_pcr_extend);
>>>
>>>  /**
>>> + * tpm_pcr_algorithms - get TPM IDs of active PCR banks algorithms
>>
>> The grammar is incorrect here I believe. Should rather be
>>
>>   "algorithms of the active PCR banks"
>>
>> And there is no such thing as "TPM ID".
>>
>>> + * @chip_num:    tpm idx # or ANY
>>> + * @count:    # of items in algorithms
>>> + * @algorithms:    array of TPM IDs
>>> + *
>>> + * Returns < 0 on error, and the number of active PCR banks on success.
>>> + *
>>> + * TPM 1.2 has always one SHA1 bank.
>>> + */
>>> +int tpm_pcr_algorithms(u32 chip_num, int count,
>>> +               enum tpm2_algorithms *algorithms)
>>                        unsigned int
>>
>> In addition the function name is not too greatg,
>>
>> Your syntax for return value is not correct. In addition after
>> describing the return value there should not be anything. You should
>> study
>>
>> https://www.kernel.org/doc/Documentation/kernel-doc-nano-HOWTO.txt
>>
>> Better name for the function would be tpm_get_pcr_algorithms().
>>
>>> +{
>>> +    struct tpm_chip *chip;
>>> +    int i;
>>> +
>>> +    if (count <= 0 || algorithms == NULL)
>>> +        return -EINVAL;
>>
>> Is there a legal case where caller would pass these values? Now it
>> looks like that there is.
>>
>> 'count' should unsigned int and zero should be a legal value for
>> count.
>
> I wanted to avoid that a negative value returned by tpm_pcr_algorithms()
> is passed to tpm_pcr_extend() as unsigned int.
>
>
>> That said I think the whole design is wrong as you could calculate
>> array for algs only one time and pass a const reference to it on
>> request.
>
> Ok. If I understood it correctly, you are saying to pass a const
> reference of chip->active_banks. Then, I should also:

This is not a viable option. chip could be freed and the reference
becomes invalid, without increasing the reference count.

Did you think about something different?

Thanks

Roberto

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v2 1/5] tpm: introduce tpm_pcr_algorithms()
  2017-05-15 13:18       ` Roberto Sassu
       [not found]         ` <d2974dd2-e30d-e8f0-a60a-2147f3290670-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
@ 2017-05-20 13:00         ` Jarkko Sakkinen
  1 sibling, 0 replies; 31+ messages in thread
From: Jarkko Sakkinen @ 2017-05-20 13:00 UTC (permalink / raw)
  To: Roberto Sassu
  Cc: tpmdd-devel, linux-ima-devel, linux-security-module, keyrings,
	linux-kernel

On Mon, May 15, 2017 at 03:18:41PM +0200, Roberto Sassu wrote:
> 
> 
> On 5/15/2017 12:36 PM, Jarkko Sakkinen wrote:
> > On Fri, May 05, 2017 at 04:21:48PM +0200, Roberto Sassu wrote:
> > > This function allows TPM users to know which algorithms the TPM supports.
> > > It stores the algorithms in a static array of 'enum tpm2_algorithms',
> > > allocated by the caller. If the array is not large enough, the function
> > > returns an error. Otherwise, it returns the number of algorithms written
> > > to the array. If the TPM version is 1.2, the function writes TPM2_ALG_SHA1
> > > to first element of the array.
> > > 
> > > Writing the algorithm also for TPM 1.2 has the advantage that callers
> > > can use the API, tpm_pcr_algorithms() and tpm_pcr_extend(), regardless
> > > of the TPM version.
> > > 
> > > A minor change added to this patch was to make available the size of
> > > the active_banks array, member of the tpm_chip structure, outside
> > > the TPM driver. The array size (TPM_ACTIVE_BANKS_MAX) has been exported
> > > to include/linux/tpm.h.
> > > 
> > > With this information, callers of tpm_pcr_algorithms() can provide
> > > a static array with enough room for all the algorithms, instead
> > > of receiving the pointer of a dynamic array that they have to free later.
> > > 
> > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > ---
> > > v2
> > > 
> > > - tpm_pcr_algorithms() returns supported algorithms also for TPM 1.2
> > > 
> > >  drivers/char/tpm/tpm-interface.c | 46 ++++++++++++++++++++++++++++++++++++++++
> > >  drivers/char/tpm/tpm.h           | 13 +-----------
> > >  include/linux/tpm.h              | 19 +++++++++++++++++
> > >  3 files changed, 66 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> > > index 4ed08ab..b90de3d 100644
> > > --- a/drivers/char/tpm/tpm-interface.c
> > > +++ b/drivers/char/tpm/tpm-interface.c
> > > @@ -911,6 +911,52 @@ int tpm_pcr_extend(u32 chip_num, int pcr_idx, const u8 *hash)
> > >  EXPORT_SYMBOL_GPL(tpm_pcr_extend);
> > > 
> > >  /**
> > > + * tpm_pcr_algorithms - get TPM IDs of active PCR banks algorithms
> > 
> > The grammar is incorrect here I believe. Should rather be
> > 
> >   "algorithms of the active PCR banks"
> > 
> > And there is no such thing as "TPM ID".
> > 
> > > + * @chip_num:	tpm idx # or ANY
> > > + * @count:	# of items in algorithms
> > > + * @algorithms:	array of TPM IDs
> > > + *
> > > + * Returns < 0 on error, and the number of active PCR banks on success.
> > > + *
> > > + * TPM 1.2 has always one SHA1 bank.
> > > + */
> > > +int tpm_pcr_algorithms(u32 chip_num, int count,
> > > +		       enum tpm2_algorithms *algorithms)
> >                        unsigned int
> > 
> > In addition the function name is not too greatg,
> > 
> > Your syntax for return value is not correct. In addition after
> > describing the return value there should not be anything. You should
> > study
> > 
> > https://www.kernel.org/doc/Documentation/kernel-doc-nano-HOWTO.txt
> > 
> > Better name for the function would be tpm_get_pcr_algorithms().
> > 
> > > +{
> > > +	struct tpm_chip *chip;
> > > +	int i;
> > > +
> > > +	if (count <= 0 || algorithms == NULL)
> > > +		return -EINVAL;
> > 
> > Is there a legal case where caller would pass these values? Now it
> > looks like that there is.
> > 
> > 'count' should unsigned int and zero should be a legal value for
> > count.
> 
> I wanted to avoid that a negative value returned by tpm_pcr_algorithms()
> is passed to tpm_pcr_extend() as unsigned int.

Why would you pass a negative value? I'm sorry but I have to admit tht I'm
seriously not computing your argument.

> > That said I think the whole design is wrong as you could calculate
> > array for algs only one time and pass a const reference to it on
> > request.
> 
> Ok. If I understood it correctly, you are saying to pass a const
> reference of chip->active_banks. Then, I should also:
> 
> - add a new field (e.g. active_banks_num) in the tpm_chip structure
>   to store the number of algorithms supported by the TPM (unless,
>   I should determine it every time tpm_pcr_algorithms() is called)
> - initialize chip->active_banks and chip->active_banks_num
>   respectively to TPM2_ALG_SHA1 and 1 in tpm1_auto_startup()
> 
> Roberto

Something along those lines.

Doing precalcuation makes the code more stable as you can only fall to
an error conditio when the driver is initialized.

/Jarkko

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v2 1/5] tpm: introduce tpm_pcr_algorithms()
  2017-05-17  8:42           ` Roberto Sassu
@ 2017-05-20 13:18             ` Jarkko Sakkinen
       [not found]               ` <20170520131846.e3niqiknlrttbdf4-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
  0 siblings, 1 reply; 31+ messages in thread
From: Jarkko Sakkinen @ 2017-05-20 13:18 UTC (permalink / raw)
  To: Roberto Sassu
  Cc: tpmdd-devel, linux-ima-devel, linux-security-module, keyrings,
	linux-kernel

On Wed, May 17, 2017 at 10:42:35AM +0200, Roberto Sassu wrote:
> On 5/15/2017 3:18 PM, Roberto Sassu wrote:
> > 
> > 
> > On 5/15/2017 12:36 PM, Jarkko Sakkinen wrote:
> > > On Fri, May 05, 2017 at 04:21:48PM +0200, Roberto Sassu wrote:
> > > > This function allows TPM users to know which algorithms the TPM
> > > > supports.
> > > > It stores the algorithms in a static array of 'enum tpm2_algorithms',
> > > > allocated by the caller. If the array is not large enough, the function
> > > > returns an error. Otherwise, it returns the number of algorithms written
> > > > to the array. If the TPM version is 1.2, the function writes
> > > > TPM2_ALG_SHA1
> > > > to first element of the array.
> > > > 
> > > > Writing the algorithm also for TPM 1.2 has the advantage that callers
> > > > can use the API, tpm_pcr_algorithms() and tpm_pcr_extend(), regardless
> > > > of the TPM version.
> > > > 
> > > > A minor change added to this patch was to make available the size of
> > > > the active_banks array, member of the tpm_chip structure, outside
> > > > the TPM driver. The array size (TPM_ACTIVE_BANKS_MAX) has been exported
> > > > to include/linux/tpm.h.
> > > > 
> > > > With this information, callers of tpm_pcr_algorithms() can provide
> > > > a static array with enough room for all the algorithms, instead
> > > > of receiving the pointer of a dynamic array that they have to free
> > > > later.
> > > > 
> > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > > ---
> > > > v2
> > > > 
> > > > - tpm_pcr_algorithms() returns supported algorithms also for TPM 1.2
> > > > 
> > > >  drivers/char/tpm/tpm-interface.c | 46
> > > > ++++++++++++++++++++++++++++++++++++++++
> > > >  drivers/char/tpm/tpm.h           | 13 +-----------
> > > >  include/linux/tpm.h              | 19 +++++++++++++++++
> > > >  3 files changed, 66 insertions(+), 12 deletions(-)
> > > > 
> > > > diff --git a/drivers/char/tpm/tpm-interface.c
> > > > b/drivers/char/tpm/tpm-interface.c
> > > > index 4ed08ab..b90de3d 100644
> > > > --- a/drivers/char/tpm/tpm-interface.c
> > > > +++ b/drivers/char/tpm/tpm-interface.c
> > > > @@ -911,6 +911,52 @@ int tpm_pcr_extend(u32 chip_num, int pcr_idx,
> > > > const u8 *hash)
> > > >  EXPORT_SYMBOL_GPL(tpm_pcr_extend);
> > > > 
> > > >  /**
> > > > + * tpm_pcr_algorithms - get TPM IDs of active PCR banks algorithms
> > > 
> > > The grammar is incorrect here I believe. Should rather be
> > > 
> > >   "algorithms of the active PCR banks"
> > > 
> > > And there is no such thing as "TPM ID".
> > > 
> > > > + * @chip_num:    tpm idx # or ANY
> > > > + * @count:    # of items in algorithms
> > > > + * @algorithms:    array of TPM IDs
> > > > + *
> > > > + * Returns < 0 on error, and the number of active PCR banks on success.
> > > > + *
> > > > + * TPM 1.2 has always one SHA1 bank.
> > > > + */
> > > > +int tpm_pcr_algorithms(u32 chip_num, int count,
> > > > +               enum tpm2_algorithms *algorithms)
> > >                        unsigned int
> > > 
> > > In addition the function name is not too greatg,
> > > 
> > > Your syntax for return value is not correct. In addition after
> > > describing the return value there should not be anything. You should
> > > study
> > > 
> > > https://www.kernel.org/doc/Documentation/kernel-doc-nano-HOWTO.txt
> > > 
> > > Better name for the function would be tpm_get_pcr_algorithms().
> > > 
> > > > +{
> > > > +    struct tpm_chip *chip;
> > > > +    int i;
> > > > +
> > > > +    if (count <= 0 || algorithms == NULL)
> > > > +        return -EINVAL;
> > > 
> > > Is there a legal case where caller would pass these values? Now it
> > > looks like that there is.
> > > 
> > > 'count' should unsigned int and zero should be a legal value for
> > > count.
> > 
> > I wanted to avoid that a negative value returned by tpm_pcr_algorithms()
> > is passed to tpm_pcr_extend() as unsigned int.
> > 
> > 
> > > That said I think the whole design is wrong as you could calculate
> > > array for algs only one time and pass a const reference to it on
> > > request.
> > 
> > Ok. If I understood it correctly, you are saying to pass a const
> > reference of chip->active_banks. Then, I should also:
> 
> This is not a viable option. chip could be freed and the reference
> becomes invalid, without increasing the reference count.
> 
> Did you think about something different?
> 
> Thanks
> 
> Roberto

Two alternatives come in mind.

1. add tpm_put to linclude/linux/tpm.h
2. take put_ops away from the implementation
3. add documentation that tpm_put needs to be called

or acceptable alternative would memcpy in the implementation

In any case, you should probably drop completely 'count' and have
function for getting the number of active banks.

/Jarkko

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v2 2/5] tpm: introduce tpm_pcr_algo_to_crypto() and tpm_pcr_algo_from_crypto()
  2017-05-15 14:22         ` Roberto Sassu
@ 2017-05-20 13:22           ` Jarkko Sakkinen
       [not found]             ` <20170520132217.t7n7l2pjn7i63hbm-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
  0 siblings, 1 reply; 31+ messages in thread
From: Jarkko Sakkinen @ 2017-05-20 13:22 UTC (permalink / raw)
  To: Roberto Sassu
  Cc: tpmdd-devel, linux-ima-devel, linux-security-module, keyrings,
	linux-kernel

On Mon, May 15, 2017 at 04:22:22PM +0200, Roberto Sassu wrote:
> On 5/15/2017 1:16 PM, Jarkko Sakkinen wrote:
> > On Fri, May 05, 2017 at 04:21:49PM +0200, Roberto Sassu wrote:
> > > tpm_pcr_algorithms() returns to its callers the IDs of the hash algorithms
> > > supported by the TPM. This patch introduces tpm_pcr_algo_to_crypto(),
> > > so that the callers can use the crypto subsystem to calculate the digest
> > > to be passed to tpm_pcr_extend().
> > > 
> > > tpm_pcr_algo_from_crypto(), implemented for completeness, is instead used
> > 
> > What do you mean by completeness? Please, never add unused stuff.
> > 
> > > by tpm2_seal_trusted() to perform the opposite conversion.
> > > 
> > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > ---
> > > v2
> > > 
> > > - fixed return values of tpm2_pcr_algo_to_crypto() and
> > >   tpm2_pcr_algo_from_crypto() if TPM support is disabled in the kernel
> > 
> > Change Log only to the cover letter.
> > 
> > >  drivers/char/tpm/tpm-interface.c | 51 ++++++++++++++++++++++++++++++++++++++++
> > >  drivers/char/tpm/tpm2-cmd.c      | 42 +++++++++------------------------
> > >  include/linux/tpm.h              | 13 ++++++++++
> > >  3 files changed, 75 insertions(+), 31 deletions(-)
> > 
> > This commit is just deadly wrong in so many ways.
> > 
> > I would suggest to make extend always just take crypto ID in so you
> > don't have to add these bizarre conversion functions.
> 
> The reason of this choice (as I explained in the cover letter)
> is that TPM users might want to produce an event log with
> the TCG format (which includes the TPM algorithm ID). Also,
> TPM IDs should be preferred because, with them, TPM users
> can calculate a digest directly with the TPM.
> 
> Taking crypto IDs means relying on the fact that there
> is always a mapping between TPM IDs and crypto IDs.
> Otherwise, tpm_pcr_algorithms() cannot return the algorithms
> to its callers and PCRs cannot be extended. If TPM IDs are used,
> TPM users have two alternatives: calculate the digest with
> the TPM, or pass a SHA1 digest to tpm_pcr_extend(), so that
> it can be padded to extend remanining PCR banks.
> 
> However, this second option will work only when the TPM driver
> determines the size of an algorithm without relying on the crypto
> subsystem. At the moment, if a TPM ID is not mapped to a crypto ID,
> tpm2_pcr_extend() ignores the digest.
> 
> Roberto

What if tpm_pcr_algorithms would convert TPM IDs to crypto IDs?

Externally other subsystems would have to then deal with only crypto
IDs.

That would favor "memcpy" (or it would become a for loop) for 1/5.

/Jarkko

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v2 2/5] tpm: introduce tpm_pcr_algo_to_crypto() and tpm_pcr_algo_from_crypto()
       [not found]             ` <20170520132217.t7n7l2pjn7i63hbm-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
@ 2017-05-22  7:21               ` Roberto Sassu
  2017-05-24 17:33                 ` Jarkko Sakkinen
  0 siblings, 1 reply; 31+ messages in thread
From: Roberto Sassu @ 2017-05-22  7:21 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: linux-ima-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
	linux-security-module-u79uwXL29TY76Z2rM5mHXA,
	tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
	keyrings-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA

On 5/20/2017 3:22 PM, Jarkko Sakkinen wrote:
> On Mon, May 15, 2017 at 04:22:22PM +0200, Roberto Sassu wrote:
>> On 5/15/2017 1:16 PM, Jarkko Sakkinen wrote:
>>> On Fri, May 05, 2017 at 04:21:49PM +0200, Roberto Sassu wrote:
>>>> tpm_pcr_algorithms() returns to its callers the IDs of the hash algorithms
>>>> supported by the TPM. This patch introduces tpm_pcr_algo_to_crypto(),
>>>> so that the callers can use the crypto subsystem to calculate the digest
>>>> to be passed to tpm_pcr_extend().
>>>>
>>>> tpm_pcr_algo_from_crypto(), implemented for completeness, is instead used
>>>
>>> What do you mean by completeness? Please, never add unused stuff.
>>>
>>>> by tpm2_seal_trusted() to perform the opposite conversion.
>>>>
>>>> Signed-off-by: Roberto Sassu <roberto.sassu-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
>>>> ---
>>>> v2
>>>>
>>>> - fixed return values of tpm2_pcr_algo_to_crypto() and
>>>>   tpm2_pcr_algo_from_crypto() if TPM support is disabled in the kernel
>>>
>>> Change Log only to the cover letter.
>>>
>>>>  drivers/char/tpm/tpm-interface.c | 51 ++++++++++++++++++++++++++++++++++++++++
>>>>  drivers/char/tpm/tpm2-cmd.c      | 42 +++++++++------------------------
>>>>  include/linux/tpm.h              | 13 ++++++++++
>>>>  3 files changed, 75 insertions(+), 31 deletions(-)
>>>
>>> This commit is just deadly wrong in so many ways.
>>>
>>> I would suggest to make extend always just take crypto ID in so you
>>> don't have to add these bizarre conversion functions.
>>
>> The reason of this choice (as I explained in the cover letter)
>> is that TPM users might want to produce an event log with
>> the TCG format (which includes the TPM algorithm ID). Also,
>> TPM IDs should be preferred because, with them, TPM users
>> can calculate a digest directly with the TPM.
>>
>> Taking crypto IDs means relying on the fact that there
>> is always a mapping between TPM IDs and crypto IDs.
>> Otherwise, tpm_pcr_algorithms() cannot return the algorithms
>> to its callers and PCRs cannot be extended. If TPM IDs are used,
>> TPM users have two alternatives: calculate the digest with
>> the TPM, or pass a SHA1 digest to tpm_pcr_extend(), so that
>> it can be padded to extend remanining PCR banks.
>>
>> However, this second option will work only when the TPM driver
>> determines the size of an algorithm without relying on the crypto
>> subsystem. At the moment, if a TPM ID is not mapped to a crypto ID,
>> tpm2_pcr_extend() ignores the digest.
>>
>> Roberto
>
> What if tpm_pcr_algorithms would convert TPM IDs to crypto IDs?
>
> Externally other subsystems would have to then deal with only crypto
> IDs.

Then, other subsystems cannot:

- use the TPM to calculate a digest
- create an event log with the TCG format

Roberto

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v2 1/5] tpm: introduce tpm_pcr_algorithms()
       [not found]               ` <20170520131846.e3niqiknlrttbdf4-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
@ 2017-05-22  9:07                 ` Roberto Sassu
  2017-05-24 17:35                   ` Jarkko Sakkinen
  0 siblings, 1 reply; 31+ messages in thread
From: Roberto Sassu @ 2017-05-22  9:07 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: linux-ima-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
	linux-security-module-u79uwXL29TY76Z2rM5mHXA,
	tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
	keyrings-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA

On 5/20/2017 3:18 PM, Jarkko Sakkinen wrote:
> On Wed, May 17, 2017 at 10:42:35AM +0200, Roberto Sassu wrote:
>> On 5/15/2017 3:18 PM, Roberto Sassu wrote:
>>>
>>>
>>> On 5/15/2017 12:36 PM, Jarkko Sakkinen wrote:
>>>> On Fri, May 05, 2017 at 04:21:48PM +0200, Roberto Sassu wrote:
>>>>> This function allows TPM users to know which algorithms the TPM
>>>>> supports.
>>>>> It stores the algorithms in a static array of 'enum tpm2_algorithms',
>>>>> allocated by the caller. If the array is not large enough, the function
>>>>> returns an error. Otherwise, it returns the number of algorithms written
>>>>> to the array. If the TPM version is 1.2, the function writes
>>>>> TPM2_ALG_SHA1
>>>>> to first element of the array.
>>>>>
>>>>> Writing the algorithm also for TPM 1.2 has the advantage that callers
>>>>> can use the API, tpm_pcr_algorithms() and tpm_pcr_extend(), regardless
>>>>> of the TPM version.
>>>>>
>>>>> A minor change added to this patch was to make available the size of
>>>>> the active_banks array, member of the tpm_chip structure, outside
>>>>> the TPM driver. The array size (TPM_ACTIVE_BANKS_MAX) has been exported
>>>>> to include/linux/tpm.h.
>>>>>
>>>>> With this information, callers of tpm_pcr_algorithms() can provide
>>>>> a static array with enough room for all the algorithms, instead
>>>>> of receiving the pointer of a dynamic array that they have to free
>>>>> later.
>>>>>
>>>>> Signed-off-by: Roberto Sassu <roberto.sassu-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
>>>>> ---
>>>>> v2
>>>>>
>>>>> - tpm_pcr_algorithms() returns supported algorithms also for TPM 1.2
>>>>>
>>>>>  drivers/char/tpm/tpm-interface.c | 46
>>>>> ++++++++++++++++++++++++++++++++++++++++
>>>>>  drivers/char/tpm/tpm.h           | 13 +-----------
>>>>>  include/linux/tpm.h              | 19 +++++++++++++++++
>>>>>  3 files changed, 66 insertions(+), 12 deletions(-)
>>>>>
>>>>> diff --git a/drivers/char/tpm/tpm-interface.c
>>>>> b/drivers/char/tpm/tpm-interface.c
>>>>> index 4ed08ab..b90de3d 100644
>>>>> --- a/drivers/char/tpm/tpm-interface.c
>>>>> +++ b/drivers/char/tpm/tpm-interface.c
>>>>> @@ -911,6 +911,52 @@ int tpm_pcr_extend(u32 chip_num, int pcr_idx,
>>>>> const u8 *hash)
>>>>>  EXPORT_SYMBOL_GPL(tpm_pcr_extend);
>>>>>
>>>>>  /**
>>>>> + * tpm_pcr_algorithms - get TPM IDs of active PCR banks algorithms
>>>>
>>>> The grammar is incorrect here I believe. Should rather be
>>>>
>>>>   "algorithms of the active PCR banks"
>>>>
>>>> And there is no such thing as "TPM ID".
>>>>
>>>>> + * @chip_num:    tpm idx # or ANY
>>>>> + * @count:    # of items in algorithms
>>>>> + * @algorithms:    array of TPM IDs
>>>>> + *
>>>>> + * Returns < 0 on error, and the number of active PCR banks on success.
>>>>> + *
>>>>> + * TPM 1.2 has always one SHA1 bank.
>>>>> + */
>>>>> +int tpm_pcr_algorithms(u32 chip_num, int count,
>>>>> +               enum tpm2_algorithms *algorithms)
>>>>                        unsigned int
>>>>
>>>> In addition the function name is not too greatg,
>>>>
>>>> Your syntax for return value is not correct. In addition after
>>>> describing the return value there should not be anything. You should
>>>> study
>>>>
>>>> https://www.kernel.org/doc/Documentation/kernel-doc-nano-HOWTO.txt
>>>>
>>>> Better name for the function would be tpm_get_pcr_algorithms().
>>>>
>>>>> +{
>>>>> +    struct tpm_chip *chip;
>>>>> +    int i;
>>>>> +
>>>>> +    if (count <= 0 || algorithms == NULL)
>>>>> +        return -EINVAL;
>>>>
>>>> Is there a legal case where caller would pass these values? Now it
>>>> looks like that there is.
>>>>
>>>> 'count' should unsigned int and zero should be a legal value for
>>>> count.
>>>
>>> I wanted to avoid that a negative value returned by tpm_pcr_algorithms()
>>> is passed to tpm_pcr_extend() as unsigned int.
>>>
>>>
>>>> That said I think the whole design is wrong as you could calculate
>>>> array for algs only one time and pass a const reference to it on
>>>> request.
>>>
>>> Ok. If I understood it correctly, you are saying to pass a const
>>> reference of chip->active_banks. Then, I should also:
>>
>> This is not a viable option. chip could be freed and the reference
>> becomes invalid, without increasing the reference count.
>>
>> Did you think about something different?
>>
>> Thanks
>>
>> Roberto
>
> Two alternatives come in mind.
>
> 1. add tpm_put to linclude/linux/tpm.h
> 2. take put_ops away from the implementation
> 3. add documentation that tpm_put needs to be called
>
> or acceptable alternative would memcpy in the implementation
>
> In any case, you should probably drop completely 'count' and have
> function for getting the number of active banks.

Having a variable number, makes things more complicated.
In the IMA patch set for multiple template digests,
the array of algorithms is stored inside a structure
called digest descriptor. The array size must be fixed,
to avoid VLAIS (which is not supported by clang).

Since the size of tpm_chip->active_banks is a small number,
we can drop count, and assume that the size of the array
passed to tpm_pcr_algorithms() always matches the size
of the active_banks array.

Roberto

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v2 2/5] tpm: introduce tpm_pcr_algo_to_crypto() and tpm_pcr_algo_from_crypto()
  2017-05-22  7:21               ` Roberto Sassu
@ 2017-05-24 17:33                 ` Jarkko Sakkinen
  2017-05-24 20:25                   ` Jarkko Sakkinen
  0 siblings, 1 reply; 31+ messages in thread
From: Jarkko Sakkinen @ 2017-05-24 17:33 UTC (permalink / raw)
  To: Roberto Sassu
  Cc: tpmdd-devel, linux-ima-devel, linux-security-module, keyrings,
	linux-kernel

On Mon, May 22, 2017 at 09:21:28AM +0200, Roberto Sassu wrote:
> On 5/20/2017 3:22 PM, Jarkko Sakkinen wrote:
> > On Mon, May 15, 2017 at 04:22:22PM +0200, Roberto Sassu wrote:
> > > On 5/15/2017 1:16 PM, Jarkko Sakkinen wrote:
> > > > On Fri, May 05, 2017 at 04:21:49PM +0200, Roberto Sassu wrote:
> > > > > tpm_pcr_algorithms() returns to its callers the IDs of the hash algorithms
> > > > > supported by the TPM. This patch introduces tpm_pcr_algo_to_crypto(),
> > > > > so that the callers can use the crypto subsystem to calculate the digest
> > > > > to be passed to tpm_pcr_extend().
> > > > > 
> > > > > tpm_pcr_algo_from_crypto(), implemented for completeness, is instead used
> > > > 
> > > > What do you mean by completeness? Please, never add unused stuff.
> > > > 
> > > > > by tpm2_seal_trusted() to perform the opposite conversion.
> > > > > 
> > > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > > > ---
> > > > > v2
> > > > > 
> > > > > - fixed return values of tpm2_pcr_algo_to_crypto() and
> > > > >   tpm2_pcr_algo_from_crypto() if TPM support is disabled in the kernel
> > > > 
> > > > Change Log only to the cover letter.
> > > > 
> > > > >  drivers/char/tpm/tpm-interface.c | 51 ++++++++++++++++++++++++++++++++++++++++
> > > > >  drivers/char/tpm/tpm2-cmd.c      | 42 +++++++++------------------------
> > > > >  include/linux/tpm.h              | 13 ++++++++++
> > > > >  3 files changed, 75 insertions(+), 31 deletions(-)
> > > > 
> > > > This commit is just deadly wrong in so many ways.
> > > > 
> > > > I would suggest to make extend always just take crypto ID in so you
> > > > don't have to add these bizarre conversion functions.
> > > 
> > > The reason of this choice (as I explained in the cover letter)
> > > is that TPM users might want to produce an event log with
> > > the TCG format (which includes the TPM algorithm ID). Also,
> > > TPM IDs should be preferred because, with them, TPM users
> > > can calculate a digest directly with the TPM.
> > > 
> > > Taking crypto IDs means relying on the fact that there
> > > is always a mapping between TPM IDs and crypto IDs.
> > > Otherwise, tpm_pcr_algorithms() cannot return the algorithms
> > > to its callers and PCRs cannot be extended. If TPM IDs are used,
> > > TPM users have two alternatives: calculate the digest with
> > > the TPM, or pass a SHA1 digest to tpm_pcr_extend(), so that
> > > it can be padded to extend remanining PCR banks.
> > > 
> > > However, this second option will work only when the TPM driver
> > > determines the size of an algorithm without relying on the crypto
> > > subsystem. At the moment, if a TPM ID is not mapped to a crypto ID,
> > > tpm2_pcr_extend() ignores the digest.
> > > 
> > > Roberto
> > 
> > What if tpm_pcr_algorithms would convert TPM IDs to crypto IDs?
> > 
> > Externally other subsystems would have to then deal with only crypto
> > IDs.
> 
> Then, other subsystems cannot:
> 
> - use the TPM to calculate a digest
> - create an event log with the TCG format
> 
> Roberto

Can you open this up a bit? What is the use case that you canno
implement?

/Jarkko

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v2 1/5] tpm: introduce tpm_pcr_algorithms()
  2017-05-22  9:07                 ` Roberto Sassu
@ 2017-05-24 17:35                   ` Jarkko Sakkinen
       [not found]                     ` <20170524173521.yiiohct7brkcozyk-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
  0 siblings, 1 reply; 31+ messages in thread
From: Jarkko Sakkinen @ 2017-05-24 17:35 UTC (permalink / raw)
  To: Roberto Sassu
  Cc: tpmdd-devel, linux-ima-devel, linux-security-module, keyrings,
	linux-kernel

On Mon, May 22, 2017 at 11:07:54AM +0200, Roberto Sassu wrote:
> On 5/20/2017 3:18 PM, Jarkko Sakkinen wrote:
> > On Wed, May 17, 2017 at 10:42:35AM +0200, Roberto Sassu wrote:
> > > On 5/15/2017 3:18 PM, Roberto Sassu wrote:
> > > > 
> > > > 
> > > > On 5/15/2017 12:36 PM, Jarkko Sakkinen wrote:
> > > > > On Fri, May 05, 2017 at 04:21:48PM +0200, Roberto Sassu wrote:
> > > > > > This function allows TPM users to know which algorithms the TPM
> > > > > > supports.
> > > > > > It stores the algorithms in a static array of 'enum tpm2_algorithms',
> > > > > > allocated by the caller. If the array is not large enough, the function
> > > > > > returns an error. Otherwise, it returns the number of algorithms written
> > > > > > to the array. If the TPM version is 1.2, the function writes
> > > > > > TPM2_ALG_SHA1
> > > > > > to first element of the array.
> > > > > > 
> > > > > > Writing the algorithm also for TPM 1.2 has the advantage that callers
> > > > > > can use the API, tpm_pcr_algorithms() and tpm_pcr_extend(), regardless
> > > > > > of the TPM version.
> > > > > > 
> > > > > > A minor change added to this patch was to make available the size of
> > > > > > the active_banks array, member of the tpm_chip structure, outside
> > > > > > the TPM driver. The array size (TPM_ACTIVE_BANKS_MAX) has been exported
> > > > > > to include/linux/tpm.h.
> > > > > > 
> > > > > > With this information, callers of tpm_pcr_algorithms() can provide
> > > > > > a static array with enough room for all the algorithms, instead
> > > > > > of receiving the pointer of a dynamic array that they have to free
> > > > > > later.
> > > > > > 
> > > > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > > > > ---
> > > > > > v2
> > > > > > 
> > > > > > - tpm_pcr_algorithms() returns supported algorithms also for TPM 1.2
> > > > > > 
> > > > > >  drivers/char/tpm/tpm-interface.c | 46
> > > > > > ++++++++++++++++++++++++++++++++++++++++
> > > > > >  drivers/char/tpm/tpm.h           | 13 +-----------
> > > > > >  include/linux/tpm.h              | 19 +++++++++++++++++
> > > > > >  3 files changed, 66 insertions(+), 12 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/char/tpm/tpm-interface.c
> > > > > > b/drivers/char/tpm/tpm-interface.c
> > > > > > index 4ed08ab..b90de3d 100644
> > > > > > --- a/drivers/char/tpm/tpm-interface.c
> > > > > > +++ b/drivers/char/tpm/tpm-interface.c
> > > > > > @@ -911,6 +911,52 @@ int tpm_pcr_extend(u32 chip_num, int pcr_idx,
> > > > > > const u8 *hash)
> > > > > >  EXPORT_SYMBOL_GPL(tpm_pcr_extend);
> > > > > > 
> > > > > >  /**
> > > > > > + * tpm_pcr_algorithms - get TPM IDs of active PCR banks algorithms
> > > > > 
> > > > > The grammar is incorrect here I believe. Should rather be
> > > > > 
> > > > >   "algorithms of the active PCR banks"
> > > > > 
> > > > > And there is no such thing as "TPM ID".
> > > > > 
> > > > > > + * @chip_num:    tpm idx # or ANY
> > > > > > + * @count:    # of items in algorithms
> > > > > > + * @algorithms:    array of TPM IDs
> > > > > > + *
> > > > > > + * Returns < 0 on error, and the number of active PCR banks on success.
> > > > > > + *
> > > > > > + * TPM 1.2 has always one SHA1 bank.
> > > > > > + */
> > > > > > +int tpm_pcr_algorithms(u32 chip_num, int count,
> > > > > > +               enum tpm2_algorithms *algorithms)
> > > > >                        unsigned int
> > > > > 
> > > > > In addition the function name is not too greatg,
> > > > > 
> > > > > Your syntax for return value is not correct. In addition after
> > > > > describing the return value there should not be anything. You should
> > > > > study
> > > > > 
> > > > > https://www.kernel.org/doc/Documentation/kernel-doc-nano-HOWTO.txt
> > > > > 
> > > > > Better name for the function would be tpm_get_pcr_algorithms().
> > > > > 
> > > > > > +{
> > > > > > +    struct tpm_chip *chip;
> > > > > > +    int i;
> > > > > > +
> > > > > > +    if (count <= 0 || algorithms == NULL)
> > > > > > +        return -EINVAL;
> > > > > 
> > > > > Is there a legal case where caller would pass these values? Now it
> > > > > looks like that there is.
> > > > > 
> > > > > 'count' should unsigned int and zero should be a legal value for
> > > > > count.
> > > > 
> > > > I wanted to avoid that a negative value returned by tpm_pcr_algorithms()
> > > > is passed to tpm_pcr_extend() as unsigned int.
> > > > 
> > > > 
> > > > > That said I think the whole design is wrong as you could calculate
> > > > > array for algs only one time and pass a const reference to it on
> > > > > request.
> > > > 
> > > > Ok. If I understood it correctly, you are saying to pass a const
> > > > reference of chip->active_banks. Then, I should also:
> > > 
> > > This is not a viable option. chip could be freed and the reference
> > > becomes invalid, without increasing the reference count.
> > > 
> > > Did you think about something different?
> > > 
> > > Thanks
> > > 
> > > Roberto
> > 
> > Two alternatives come in mind.
> > 
> > 1. add tpm_put to linclude/linux/tpm.h
> > 2. take put_ops away from the implementation
> > 3. add documentation that tpm_put needs to be called
> > 
> > or acceptable alternative would memcpy in the implementation
> > 
> > In any case, you should probably drop completely 'count' and have
> > function for getting the number of active banks.
> 
> Having a variable number, makes things more complicated.
> In the IMA patch set for multiple template digests,
> the array of algorithms is stored inside a structure
> called digest descriptor. The array size must be fixed,
> to avoid VLAIS (which is not supported by clang).

What is VLAI?

You can always allocate it from heap.

> Since the size of tpm_chip->active_banks is a small number,
> we can drop count, and assume that the size of the array
> passed to tpm_pcr_algorithms() always matches the size
> of the active_banks array.
> 
> Roberto

I don't understand what you mean.


/Jarkko

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v2 2/5] tpm: introduce tpm_pcr_algo_to_crypto() and tpm_pcr_algo_from_crypto()
  2017-05-24 17:33                 ` Jarkko Sakkinen
@ 2017-05-24 20:25                   ` Jarkko Sakkinen
  2017-05-30 10:24                     ` Roberto Sassu
  0 siblings, 1 reply; 31+ messages in thread
From: Jarkko Sakkinen @ 2017-05-24 20:25 UTC (permalink / raw)
  To: Roberto Sassu
  Cc: tpmdd-devel, linux-ima-devel, linux-security-module, keyrings,
	linux-kernel

On Wed, May 24, 2017 at 10:33:54AM -0700, Jarkko Sakkinen wrote:
> On Mon, May 22, 2017 at 09:21:28AM +0200, Roberto Sassu wrote:
> > On 5/20/2017 3:22 PM, Jarkko Sakkinen wrote:
> > > On Mon, May 15, 2017 at 04:22:22PM +0200, Roberto Sassu wrote:
> > > > On 5/15/2017 1:16 PM, Jarkko Sakkinen wrote:
> > > > > On Fri, May 05, 2017 at 04:21:49PM +0200, Roberto Sassu wrote:
> > > > > > tpm_pcr_algorithms() returns to its callers the IDs of the hash algorithms
> > > > > > supported by the TPM. This patch introduces tpm_pcr_algo_to_crypto(),
> > > > > > so that the callers can use the crypto subsystem to calculate the digest
> > > > > > to be passed to tpm_pcr_extend().
> > > > > > 
> > > > > > tpm_pcr_algo_from_crypto(), implemented for completeness, is instead used
> > > > > 
> > > > > What do you mean by completeness? Please, never add unused stuff.
> > > > > 
> > > > > > by tpm2_seal_trusted() to perform the opposite conversion.
> > > > > > 
> > > > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > > > > ---
> > > > > > v2
> > > > > > 
> > > > > > - fixed return values of tpm2_pcr_algo_to_crypto() and
> > > > > >   tpm2_pcr_algo_from_crypto() if TPM support is disabled in the kernel
> > > > > 
> > > > > Change Log only to the cover letter.
> > > > > 
> > > > > >  drivers/char/tpm/tpm-interface.c | 51 ++++++++++++++++++++++++++++++++++++++++
> > > > > >  drivers/char/tpm/tpm2-cmd.c      | 42 +++++++++------------------------
> > > > > >  include/linux/tpm.h              | 13 ++++++++++
> > > > > >  3 files changed, 75 insertions(+), 31 deletions(-)
> > > > > 
> > > > > This commit is just deadly wrong in so many ways.
> > > > > 
> > > > > I would suggest to make extend always just take crypto ID in so you
> > > > > don't have to add these bizarre conversion functions.
> > > > 
> > > > The reason of this choice (as I explained in the cover letter)
> > > > is that TPM users might want to produce an event log with
> > > > the TCG format (which includes the TPM algorithm ID). Also,
> > > > TPM IDs should be preferred because, with them, TPM users
> > > > can calculate a digest directly with the TPM.
> > > > 
> > > > Taking crypto IDs means relying on the fact that there
> > > > is always a mapping between TPM IDs and crypto IDs.
> > > > Otherwise, tpm_pcr_algorithms() cannot return the algorithms
> > > > to its callers and PCRs cannot be extended. If TPM IDs are used,
> > > > TPM users have two alternatives: calculate the digest with
> > > > the TPM, or pass a SHA1 digest to tpm_pcr_extend(), so that
> > > > it can be padded to extend remanining PCR banks.
> > > > 
> > > > However, this second option will work only when the TPM driver
> > > > determines the size of an algorithm without relying on the crypto
> > > > subsystem. At the moment, if a TPM ID is not mapped to a crypto ID,
> > > > tpm2_pcr_extend() ignores the digest.
> > > > 
> > > > Roberto
> > > 
> > > What if tpm_pcr_algorithms would convert TPM IDs to crypto IDs?
> > > 
> > > Externally other subsystems would have to then deal with only crypto
> > > IDs.
> > 
> > Then, other subsystems cannot:
> > 
> > - use the TPM to calculate a digest
> > - create an event log with the TCG format
> > 
> > Roberto
> 
> Can you open this up a bit? What is the use case that you canno
> implement?
> 
> /Jarkko

The most important question is: are this used *right now*. If not, it is
a definitive NAK. We don't want any future-proof code to the kernel.
Always do the lowest common denominator in terms of generality and we
will refactor from there when need arises.

/Jarkko

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [Linux-ima-devel] [PATCH v2 3/5] tpm: pass multiple digests to tpm_pcr_extend()
       [not found]   ` <20170505142152.29795-4-roberto.sassu-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
@ 2017-05-30  3:29     ` Mimi Zohar
  2017-05-30  7:28       ` Roberto Sassu
  0 siblings, 1 reply; 31+ messages in thread
From: Mimi Zohar @ 2017-05-30  3:29 UTC (permalink / raw)
  To: Roberto Sassu, tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f
  Cc: linux-ima-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
	linux-security-module-u79uwXL29TY76Z2rM5mHXA,
	keyrings-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA

On Fri, 2017-05-05 at 16:21 +0200, Roberto Sassu wrote:
> The tpm_pcr_extend() definition has been modified to take an array of
> tpm2_digest structures, and the size of the array as arguments.
> 
> The function now checks if callers provided a digests for each active
> PCR bank (or a SHA1 digest for TPM 1.2), to follow the recomendation from
> the TCG specifications. See commit c1f92b4b04ad ("tpm: enhance TPM 2.0
> PCR extend to support multiple banks"). All banks should be extended
> because unused banks could be used by an attacker to hide the true
> integrity status of the platform.
> 
> The only allowed exception to the rule above is to pass a SHA1 digest.
> It has been introduced to maintain compatibility with applications that
> expect to interact with a TPM 1.2, and provide only a SHA1 digest.
> In this case, the behavior of tpm_pcr_extend() is unchanged and
> remaining PCR banks are extended with that digest, padded with zeros.
> 
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
> v2
> 
> - tpm_pcr_extend() arguments checked by tpm_pcr_check_input()
> - modified parameters of tpm_pcr_extend()
> 
>  drivers/char/tpm/tpm-interface.c | 76 +++++++++++++++++++++++++++++++++++++---
>  drivers/char/tpm/tpm.h           |  6 ----
>  include/linux/tpm.h              | 11 ++++--
>  3 files changed, 80 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index aac703e..4b08b02 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -867,6 +867,55 @@ static int tpm1_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash,
>  }
> 
>  /**
> + * tpm_pcr_check_input - check digests argument
> + *
> + * Return values:
> + *       1: input correct
> + *       0: fill digests with SHA1 digest padded with zeros
> + * -EINVAL: input incorrect
> + */
> +static int tpm_pcr_check_input(struct tpm_chip *chip, int count,
> +			       struct tpm2_digest *digests)
> +{
> +	bool sha1_only;
> +	int found = 0, not_found = 0;
> +	int i, j;
> +
> +	if (count <= 0 || digests == NULL)
> +		return -EINVAL;
> +
> +	sha1_only = (count == 1 && digests[0].alg_id == TPM2_ALG_SHA1);
> +
> +	if (!(chip->flags & TPM_CHIP_FLAG_TPM2))
> +		return sha1_only ? 1 : -EINVAL;
> +
> +	if (sha1_only)
> +		return 0;
> +
> +	for (i = 0; i < ARRAY_SIZE(chip->active_banks) &&
> +	     chip->active_banks[i] != TPM2_ALG_ERROR; i++) {
> +		for (j = 0; j < count; j++) {
> +			if (digests[j].alg_id == chip->active_banks[i]) {
> +				found++;
> +				break;
> +			}
> +		}
> +
> +		if (j == count) {
> +			dev_dbg(&chip->dev, "%s: missing algorithm 0x%X\n",
> +				__func__, chip->active_banks[i]);
> +			not_found++;
> +		}
> +	}
> +
> +	if (not_found == 0 && found != count)
> +		dev_dbg(&chip->dev,
> +			"%s: duplicate or unsupported algorithm\n", __func__);
> +
> +	return (not_found == 0 && found == count) ? 1 : -EINVAL;
> +}
> +
> +/**
>   * tpm_pcr_extend - extend pcr value with hash
>   * @chip_num:	tpm idx # or AN&
>   * @pcr_idx:	pcr idx to extend
> @@ -876,29 +925,46 @@ static int tpm1_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash,
>   * isn't, protect against the chip disappearing, by incrementing
>   * the module usage count.
>   */
> -int tpm_pcr_extend(u32 chip_num, int pcr_idx, const u8 *hash)
> +int tpm_pcr_extend(u32 chip_num, int pcr_idx, int count,
> +		   struct tpm2_digest *digests)
>  {
>  	int rc;
>  	struct tpm_chip *chip;
>  	struct tpm2_digest digest_list[ARRAY_SIZE(chip->active_banks)];
> -	u32 count = 0;
> +	struct tpm2_digest *digests_ptr = digests;
> +	u32 filled_count = 0;
> +	u8 *hash;
>  	int i;
> 
>  	chip = tpm_chip_find_get(chip_num);
>  	if (chip == NULL)
>  		return -ENODEV;
> 
> -	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
> +	rc = tpm_pcr_check_input(chip, count, digests);
> +	if (rc < 0) {
> +		dev_dbg(&chip->dev, "%s: invalid arguments\n", __func__);
> +		tpm_put_ops(chip);

This rejects the TPM extend, if ANY of the algorithms are unknown.
Suppose that the standards were updated, TPM vendors add support for
the new algorithm, but the kernel has not been updated to reflect the
new algorithms supported.  As the measurement hash already been added
to the IMA measurement list, verifying the measurement list against a
TPM quote will fail, not just for the unknown algorithm, but for all
algorithms.  Something is very broken with this approach.

Mimi

> +		return rc;
> +	}
> +
> +	hash = digests[0].digest;
> +
> +	if (!rc) {
>  		memset(digest_list, 0, sizeof(digest_list));
> 
>  		for (i = 0; i < ARRAY_SIZE(chip->active_banks) &&
>  			    chip->active_banks[i] != TPM2_ALG_ERROR; i++) {
>  			digest_list[i].alg_id = chip->active_banks[i];
>  			memcpy(digest_list[i].digest, hash, TPM_DIGEST_SIZE);
> -			count++;
> +			filled_count++;
>  		}
> 
> -		rc = tpm2_pcr_extend(chip, pcr_idx, count, digest_list);
> +		digests_ptr = digest_list;
> +		count = filled_count;
> +	}
> +
> +	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
> +		rc = tpm2_pcr_extend(chip, pcr_idx, count, digests_ptr);
>  		tpm_put_ops(chip);
>  		return rc;
>  	}
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index b22bc25..6d775c4 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -34,7 +34,6 @@
>  #include <linux/acpi.h>
>  #include <linux/cdev.h>
>  #include <linux/highmem.h>
> -#include <crypto/hash_info.h>
> 
>  enum tpm_const {
>  	TPM_MINOR = 224,	/* officially assigned */
> @@ -405,11 +404,6 @@ struct tpm_cmd_t {
>  	tpm_cmd_params	params;
>  } __packed;
> 
> -struct tpm2_digest {
> -	u16 alg_id;
> -	u8 digest[SHA512_DIGEST_SIZE];
> -} __packed;
> -
>  /* A string buffer type for constructing TPM commands. This is based on the
>   * ideas of string buffer code in security/keys/trusted.h but is heap based
>   * in order to keep the stack usage minimal.
> diff --git a/include/linux/tpm.h b/include/linux/tpm.h
> index 9ecd12c..5c5a600 100644
> --- a/include/linux/tpm.h
> +++ b/include/linux/tpm.h
> @@ -36,6 +36,11 @@ struct tpm_chip;
>  struct trusted_key_payload;
>  struct trusted_key_options;
> 
> +struct tpm2_digest {
> +	u16 alg_id;
> +	u8 digest[SHA512_DIGEST_SIZE];
> +} __packed;
> +
>  enum TPM_OPS_FLAGS {
>  	TPM_OPS_AUTO_STARTUP = BIT(0),
>  };
> @@ -70,7 +75,8 @@ struct tpm_class_ops {
> 
>  extern int tpm_is_tpm2(u32 chip_num);
>  extern int tpm_pcr_read(u32 chip_num, int pcr_idx, u8 *res_buf);
> -extern int tpm_pcr_extend(u32 chip_num, int pcr_idx, const u8 *hash);
> +extern int tpm_pcr_extend(u32 chip_num, int pcr_idx, int count,
> +			  struct tpm2_digest *digests);
>  extern int tpm_pcr_algorithms(u32 chip_num, int count,
>  			      enum tpm2_algorithms *algorithms);
>  extern enum hash_algo tpm_pcr_algo_to_crypto(enum tpm2_algorithms tpm_id);
> @@ -91,7 +97,8 @@ static inline int tpm_is_tpm2(u32 chip_num)
>  static inline int tpm_pcr_read(u32 chip_num, int pcr_idx, u8 *res_buf) {
>  	return -ENODEV;
>  }
> -static inline int tpm_pcr_extend(u32 chip_num, int pcr_idx, const u8 *hash) {
> +static inline int tpm_pcr_extend(u32 chip_num, int pcr_idx, int count,
> +				 struct tpm2_digest *digests) {
>  	return -ENODEV;
>  }
>  static inline int tpm_pcr_algorithms(u32 chip_num, int count,


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [Linux-ima-devel] [PATCH v2 4/5] keys,     trusted: modify arguments of tpm_pcr_extend()
  2017-05-05 14:21 ` [PATCH v2 4/5] keys, trusted: modify arguments of tpm_pcr_extend() Roberto Sassu
@ 2017-05-30  3:35   ` Mimi Zohar
       [not found]     ` <1496115337.3841.485.camel-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
  0 siblings, 1 reply; 31+ messages in thread
From: Mimi Zohar @ 2017-05-30  3:35 UTC (permalink / raw)
  To: Roberto Sassu, tpmdd-devel
  Cc: linux-ima-devel, linux-security-module, keyrings, linux-kernel

On Fri, 2017-05-05 at 16:21 +0200, Roberto Sassu wrote:
> pcrlock() has been modified to pass the correct arguments
> to tpm_pcr_extend(): the pointer of a tpm2_digest structure containing
> a random value generated by tpm_get_random() and the size of the array (1).

If the number of arguments is wrong, that means the patch that
introduced the change is not bi-sect safe.  (This comment is
applicable to patch 5/5 too.)

Mimi

> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
>  security/keys/trusted.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/security/keys/trusted.c b/security/keys/trusted.c
> index 2ae31c5..3eb89e6 100644
> --- a/security/keys/trusted.c
> +++ b/security/keys/trusted.c
> @@ -377,15 +377,15 @@ static int trusted_tpm_send(const u32 chip_num, unsigned char *cmd,
>   */
>  static int pcrlock(const int pcrnum)
>  {
> -	unsigned char hash[SHA1_DIGEST_SIZE];
> +	struct tpm2_digest digestarg = {.alg_id = TPM2_ALG_SHA1};
>  	int ret;
> 
>  	if (!capable(CAP_SYS_ADMIN))
>  		return -EPERM;
> -	ret = tpm_get_random(TPM_ANY_NUM, hash, SHA1_DIGEST_SIZE);
> +	ret = tpm_get_random(TPM_ANY_NUM, digestarg.digest, SHA1_DIGEST_SIZE);
>  	if (ret != SHA1_DIGEST_SIZE)
>  		return ret;
> -	return tpm_pcr_extend(TPM_ANY_NUM, pcrnum, hash) ? -EINVAL : 0;
> +	return tpm_pcr_extend(TPM_ANY_NUM, pcrnum, 1, &digestarg) ? -EINVAL : 0;
>  }
> 
>  /*


^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [Linux-ima-devel] [PATCH v2 3/5] tpm: pass multiple digests to tpm_pcr_extend()
  2017-05-30  3:29     ` [Linux-ima-devel] " Mimi Zohar
@ 2017-05-30  7:28       ` Roberto Sassu
  2017-05-30 11:25         ` Mimi Zohar
  0 siblings, 1 reply; 31+ messages in thread
From: Roberto Sassu @ 2017-05-30  7:28 UTC (permalink / raw)
  To: Mimi Zohar, tpmdd-devel
  Cc: linux-ima-devel, linux-security-module, keyrings, linux-kernel

On 5/30/2017 5:29 AM, Mimi Zohar wrote:
> On Fri, 2017-05-05 at 16:21 +0200, Roberto Sassu wrote:
>> The tpm_pcr_extend() definition has been modified to take an array of
>> tpm2_digest structures, and the size of the array as arguments.
>>
>> The function now checks if callers provided a digests for each active
>> PCR bank (or a SHA1 digest for TPM 1.2), to follow the recomendation from
>> the TCG specifications. See commit c1f92b4b04ad ("tpm: enhance TPM 2.0
>> PCR extend to support multiple banks"). All banks should be extended
>> because unused banks could be used by an attacker to hide the true
>> integrity status of the platform.
>>
>> The only allowed exception to the rule above is to pass a SHA1 digest.
>> It has been introduced to maintain compatibility with applications that
>> expect to interact with a TPM 1.2, and provide only a SHA1 digest.
>> In this case, the behavior of tpm_pcr_extend() is unchanged and
>> remaining PCR banks are extended with that digest, padded with zeros.
>>
>> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
>> ---
>> v2
>>
>> - tpm_pcr_extend() arguments checked by tpm_pcr_check_input()
>> - modified parameters of tpm_pcr_extend()
>>
>>  drivers/char/tpm/tpm-interface.c | 76 +++++++++++++++++++++++++++++++++++++---
>>  drivers/char/tpm/tpm.h           |  6 ----
>>  include/linux/tpm.h              | 11 ++++--
>>  3 files changed, 80 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
>> index aac703e..4b08b02 100644
>> --- a/drivers/char/tpm/tpm-interface.c
>> +++ b/drivers/char/tpm/tpm-interface.c
>> @@ -867,6 +867,55 @@ static int tpm1_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash,
>>  }
>>
>>  /**
>> + * tpm_pcr_check_input - check digests argument
>> + *
>> + * Return values:
>> + *       1: input correct
>> + *       0: fill digests with SHA1 digest padded with zeros
>> + * -EINVAL: input incorrect
>> + */
>> +static int tpm_pcr_check_input(struct tpm_chip *chip, int count,
>> +			       struct tpm2_digest *digests)
>> +{
>> +	bool sha1_only;
>> +	int found = 0, not_found = 0;
>> +	int i, j;
>> +
>> +	if (count <= 0 || digests == NULL)
>> +		return -EINVAL;
>> +
>> +	sha1_only = (count == 1 && digests[0].alg_id == TPM2_ALG_SHA1);
>> +
>> +	if (!(chip->flags & TPM_CHIP_FLAG_TPM2))
>> +		return sha1_only ? 1 : -EINVAL;
>> +
>> +	if (sha1_only)
>> +		return 0;
>> +
>> +	for (i = 0; i < ARRAY_SIZE(chip->active_banks) &&
>> +	     chip->active_banks[i] != TPM2_ALG_ERROR; i++) {
>> +		for (j = 0; j < count; j++) {
>> +			if (digests[j].alg_id == chip->active_banks[i]) {
>> +				found++;
>> +				break;
>> +			}
>> +		}
>> +
>> +		if (j == count) {
>> +			dev_dbg(&chip->dev, "%s: missing algorithm 0x%X\n",
>> +				__func__, chip->active_banks[i]);
>> +			not_found++;
>> +		}
>> +	}
>> +
>> +	if (not_found == 0 && found != count)
>> +		dev_dbg(&chip->dev,
>> +			"%s: duplicate or unsupported algorithm\n", __func__);
>> +
>> +	return (not_found == 0 && found == count) ? 1 : -EINVAL;
>> +}
>> +
>> +/**
>>   * tpm_pcr_extend - extend pcr value with hash
>>   * @chip_num:	tpm idx # or AN&
>>   * @pcr_idx:	pcr idx to extend
>> @@ -876,29 +925,46 @@ static int tpm1_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash,
>>   * isn't, protect against the chip disappearing, by incrementing
>>   * the module usage count.
>>   */
>> -int tpm_pcr_extend(u32 chip_num, int pcr_idx, const u8 *hash)
>> +int tpm_pcr_extend(u32 chip_num, int pcr_idx, int count,
>> +		   struct tpm2_digest *digests)
>>  {
>>  	int rc;
>>  	struct tpm_chip *chip;
>>  	struct tpm2_digest digest_list[ARRAY_SIZE(chip->active_banks)];
>> -	u32 count = 0;
>> +	struct tpm2_digest *digests_ptr = digests;
>> +	u32 filled_count = 0;
>> +	u8 *hash;
>>  	int i;
>>
>>  	chip = tpm_chip_find_get(chip_num);
>>  	if (chip == NULL)
>>  		return -ENODEV;
>>
>> -	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
>> +	rc = tpm_pcr_check_input(chip, count, digests);
>> +	if (rc < 0) {
>> +		dev_dbg(&chip->dev, "%s: invalid arguments\n", __func__);
>> +		tpm_put_ops(chip);
>
> This rejects the TPM extend, if ANY of the algorithms are unknown.
> Suppose that the standards were updated, TPM vendors add support for
> the new algorithm, but the kernel has not been updated to reflect the
> new algorithms supported.  As the measurement hash already been added
> to the IMA measurement list, verifying the measurement list against a
> TPM quote will fail, not just for the unknown algorithm, but for all
> algorithms.  Something is very broken with this approach.

The alternative is to extend remaining banks with a digest,
for example the first passed by the caller. I will modify
the patch, if everyone agrees on that.

Roberto


>
> Mimi
>
>> +		return rc;
>> +	}
>> +
>> +	hash = digests[0].digest;
>> +
>> +	if (!rc) {
>>  		memset(digest_list, 0, sizeof(digest_list));
>>
>>  		for (i = 0; i < ARRAY_SIZE(chip->active_banks) &&
>>  			    chip->active_banks[i] != TPM2_ALG_ERROR; i++) {
>>  			digest_list[i].alg_id = chip->active_banks[i];
>>  			memcpy(digest_list[i].digest, hash, TPM_DIGEST_SIZE);
>> -			count++;
>> +			filled_count++;
>>  		}
>>
>> -		rc = tpm2_pcr_extend(chip, pcr_idx, count, digest_list);
>> +		digests_ptr = digest_list;
>> +		count = filled_count;
>> +	}
>> +
>> +	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
>> +		rc = tpm2_pcr_extend(chip, pcr_idx, count, digests_ptr);
>>  		tpm_put_ops(chip);
>>  		return rc;
>>  	}
>> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
>> index b22bc25..6d775c4 100644
>> --- a/drivers/char/tpm/tpm.h
>> +++ b/drivers/char/tpm/tpm.h
>> @@ -34,7 +34,6 @@
>>  #include <linux/acpi.h>
>>  #include <linux/cdev.h>
>>  #include <linux/highmem.h>
>> -#include <crypto/hash_info.h>
>>
>>  enum tpm_const {
>>  	TPM_MINOR = 224,	/* officially assigned */
>> @@ -405,11 +404,6 @@ struct tpm_cmd_t {
>>  	tpm_cmd_params	params;
>>  } __packed;
>>
>> -struct tpm2_digest {
>> -	u16 alg_id;
>> -	u8 digest[SHA512_DIGEST_SIZE];
>> -} __packed;
>> -
>>  /* A string buffer type for constructing TPM commands. This is based on the
>>   * ideas of string buffer code in security/keys/trusted.h but is heap based
>>   * in order to keep the stack usage minimal.
>> diff --git a/include/linux/tpm.h b/include/linux/tpm.h
>> index 9ecd12c..5c5a600 100644
>> --- a/include/linux/tpm.h
>> +++ b/include/linux/tpm.h
>> @@ -36,6 +36,11 @@ struct tpm_chip;
>>  struct trusted_key_payload;
>>  struct trusted_key_options;
>>
>> +struct tpm2_digest {
>> +	u16 alg_id;
>> +	u8 digest[SHA512_DIGEST_SIZE];
>> +} __packed;
>> +
>>  enum TPM_OPS_FLAGS {
>>  	TPM_OPS_AUTO_STARTUP = BIT(0),
>>  };
>> @@ -70,7 +75,8 @@ struct tpm_class_ops {
>>
>>  extern int tpm_is_tpm2(u32 chip_num);
>>  extern int tpm_pcr_read(u32 chip_num, int pcr_idx, u8 *res_buf);
>> -extern int tpm_pcr_extend(u32 chip_num, int pcr_idx, const u8 *hash);
>> +extern int tpm_pcr_extend(u32 chip_num, int pcr_idx, int count,
>> +			  struct tpm2_digest *digests);
>>  extern int tpm_pcr_algorithms(u32 chip_num, int count,
>>  			      enum tpm2_algorithms *algorithms);
>>  extern enum hash_algo tpm_pcr_algo_to_crypto(enum tpm2_algorithms tpm_id);
>> @@ -91,7 +97,8 @@ static inline int tpm_is_tpm2(u32 chip_num)
>>  static inline int tpm_pcr_read(u32 chip_num, int pcr_idx, u8 *res_buf) {
>>  	return -ENODEV;
>>  }
>> -static inline int tpm_pcr_extend(u32 chip_num, int pcr_idx, const u8 *hash) {
>> +static inline int tpm_pcr_extend(u32 chip_num, int pcr_idx, int count,
>> +				 struct tpm2_digest *digests) {
>>  	return -ENODEV;
>>  }
>>  static inline int tpm_pcr_algorithms(u32 chip_num, int count,
>

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [Linux-ima-devel] [PATCH v2 4/5] keys, trusted: modify arguments of tpm_pcr_extend()
       [not found]     ` <1496115337.3841.485.camel-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
@ 2017-05-30  7:36       ` Roberto Sassu
  2017-05-30 12:06         ` Mimi Zohar
  0 siblings, 1 reply; 31+ messages in thread
From: Roberto Sassu @ 2017-05-30  7:36 UTC (permalink / raw)
  To: Mimi Zohar, tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f
  Cc: linux-ima-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
	linux-security-module-u79uwXL29TY76Z2rM5mHXA,
	keyrings-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA

On 5/30/2017 5:35 AM, Mimi Zohar wrote:
> On Fri, 2017-05-05 at 16:21 +0200, Roberto Sassu wrote:
>> pcrlock() has been modified to pass the correct arguments
>> to tpm_pcr_extend(): the pointer of a tpm2_digest structure containing
>> a random value generated by tpm_get_random() and the size of the array (1).
>
> If the number of arguments is wrong, that means the patch that
> introduced the change is not bi-sect safe.  (This comment is
> applicable to patch 5/5 too.)

Jarkko (the TPM driver maintainer) asked me to not introduce
a new function to pass multiple digests, but to modify
the parameters of tpm_pcr_extend().

Roberto


>
> Mimi
>
>> Signed-off-by: Roberto Sassu <roberto.sassu-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
>> ---
>>  security/keys/trusted.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/security/keys/trusted.c b/security/keys/trusted.c
>> index 2ae31c5..3eb89e6 100644
>> --- a/security/keys/trusted.c
>> +++ b/security/keys/trusted.c
>> @@ -377,15 +377,15 @@ static int trusted_tpm_send(const u32 chip_num, unsigned char *cmd,
>>   */
>>  static int pcrlock(const int pcrnum)
>>  {
>> -	unsigned char hash[SHA1_DIGEST_SIZE];
>> +	struct tpm2_digest digestarg = {.alg_id = TPM2_ALG_SHA1};
>>  	int ret;
>>
>>  	if (!capable(CAP_SYS_ADMIN))
>>  		return -EPERM;
>> -	ret = tpm_get_random(TPM_ANY_NUM, hash, SHA1_DIGEST_SIZE);
>> +	ret = tpm_get_random(TPM_ANY_NUM, digestarg.digest, SHA1_DIGEST_SIZE);
>>  	if (ret != SHA1_DIGEST_SIZE)
>>  		return ret;
>> -	return tpm_pcr_extend(TPM_ANY_NUM, pcrnum, hash) ? -EINVAL : 0;
>> +	return tpm_pcr_extend(TPM_ANY_NUM, pcrnum, 1, &digestarg) ? -EINVAL : 0;
>>  }
>>
>>  /*
>

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v2 1/5] tpm: introduce tpm_pcr_algorithms()
       [not found]                     ` <20170524173521.yiiohct7brkcozyk-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
@ 2017-05-30  8:44                       ` Roberto Sassu
       [not found]                         ` <1cf4fbb1-5683-ceb4-2d5b-8ade25a91e83-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
  0 siblings, 1 reply; 31+ messages in thread
From: Roberto Sassu @ 2017-05-30  8:44 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: linux-ima-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
	linux-security-module-u79uwXL29TY76Z2rM5mHXA,
	tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
	keyrings-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA

On 5/24/2017 7:35 PM, Jarkko Sakkinen wrote:
> On Mon, May 22, 2017 at 11:07:54AM +0200, Roberto Sassu wrote:
>> On 5/20/2017 3:18 PM, Jarkko Sakkinen wrote:
>>> On Wed, May 17, 2017 at 10:42:35AM +0200, Roberto Sassu wrote:
>>>> On 5/15/2017 3:18 PM, Roberto Sassu wrote:
>>>>>
>>>>>
>>>>> On 5/15/2017 12:36 PM, Jarkko Sakkinen wrote:
>>>>>> On Fri, May 05, 2017 at 04:21:48PM +0200, Roberto Sassu wrote:
>>>>>>> This function allows TPM users to know which algorithms the TPM
>>>>>>> supports.
>>>>>>> It stores the algorithms in a static array of 'enum tpm2_algorithms',
>>>>>>> allocated by the caller. If the array is not large enough, the function
>>>>>>> returns an error. Otherwise, it returns the number of algorithms written
>>>>>>> to the array. If the TPM version is 1.2, the function writes
>>>>>>> TPM2_ALG_SHA1
>>>>>>> to first element of the array.
>>>>>>>
>>>>>>> Writing the algorithm also for TPM 1.2 has the advantage that callers
>>>>>>> can use the API, tpm_pcr_algorithms() and tpm_pcr_extend(), regardless
>>>>>>> of the TPM version.
>>>>>>>
>>>>>>> A minor change added to this patch was to make available the size of
>>>>>>> the active_banks array, member of the tpm_chip structure, outside
>>>>>>> the TPM driver. The array size (TPM_ACTIVE_BANKS_MAX) has been exported
>>>>>>> to include/linux/tpm.h.
>>>>>>>
>>>>>>> With this information, callers of tpm_pcr_algorithms() can provide
>>>>>>> a static array with enough room for all the algorithms, instead
>>>>>>> of receiving the pointer of a dynamic array that they have to free
>>>>>>> later.
>>>>>>>
>>>>>>> Signed-off-by: Roberto Sassu <roberto.sassu-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
>>>>>>> ---
>>>>>>> v2
>>>>>>>
>>>>>>> - tpm_pcr_algorithms() returns supported algorithms also for TPM 1.2
>>>>>>>
>>>>>>>  drivers/char/tpm/tpm-interface.c | 46
>>>>>>> ++++++++++++++++++++++++++++++++++++++++
>>>>>>>  drivers/char/tpm/tpm.h           | 13 +-----------
>>>>>>>  include/linux/tpm.h              | 19 +++++++++++++++++
>>>>>>>  3 files changed, 66 insertions(+), 12 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/char/tpm/tpm-interface.c
>>>>>>> b/drivers/char/tpm/tpm-interface.c
>>>>>>> index 4ed08ab..b90de3d 100644
>>>>>>> --- a/drivers/char/tpm/tpm-interface.c
>>>>>>> +++ b/drivers/char/tpm/tpm-interface.c
>>>>>>> @@ -911,6 +911,52 @@ int tpm_pcr_extend(u32 chip_num, int pcr_idx,
>>>>>>> const u8 *hash)
>>>>>>>  EXPORT_SYMBOL_GPL(tpm_pcr_extend);
>>>>>>>
>>>>>>>  /**
>>>>>>> + * tpm_pcr_algorithms - get TPM IDs of active PCR banks algorithms
>>>>>>
>>>>>> The grammar is incorrect here I believe. Should rather be
>>>>>>
>>>>>>   "algorithms of the active PCR banks"
>>>>>>
>>>>>> And there is no such thing as "TPM ID".
>>>>>>
>>>>>>> + * @chip_num:    tpm idx # or ANY
>>>>>>> + * @count:    # of items in algorithms
>>>>>>> + * @algorithms:    array of TPM IDs
>>>>>>> + *
>>>>>>> + * Returns < 0 on error, and the number of active PCR banks on success.
>>>>>>> + *
>>>>>>> + * TPM 1.2 has always one SHA1 bank.
>>>>>>> + */
>>>>>>> +int tpm_pcr_algorithms(u32 chip_num, int count,
>>>>>>> +               enum tpm2_algorithms *algorithms)
>>>>>>                        unsigned int
>>>>>>
>>>>>> In addition the function name is not too greatg,
>>>>>>
>>>>>> Your syntax for return value is not correct. In addition after
>>>>>> describing the return value there should not be anything. You should
>>>>>> study
>>>>>>
>>>>>> https://www.kernel.org/doc/Documentation/kernel-doc-nano-HOWTO.txt
>>>>>>
>>>>>> Better name for the function would be tpm_get_pcr_algorithms().
>>>>>>
>>>>>>> +{
>>>>>>> +    struct tpm_chip *chip;
>>>>>>> +    int i;
>>>>>>> +
>>>>>>> +    if (count <= 0 || algorithms == NULL)
>>>>>>> +        return -EINVAL;
>>>>>>
>>>>>> Is there a legal case where caller would pass these values? Now it
>>>>>> looks like that there is.
>>>>>>
>>>>>> 'count' should unsigned int and zero should be a legal value for
>>>>>> count.
>>>>>
>>>>> I wanted to avoid that a negative value returned by tpm_pcr_algorithms()
>>>>> is passed to tpm_pcr_extend() as unsigned int.
>>>>>
>>>>>
>>>>>> That said I think the whole design is wrong as you could calculate
>>>>>> array for algs only one time and pass a const reference to it on
>>>>>> request.
>>>>>
>>>>> Ok. If I understood it correctly, you are saying to pass a const
>>>>> reference of chip->active_banks. Then, I should also:
>>>>
>>>> This is not a viable option. chip could be freed and the reference
>>>> becomes invalid, without increasing the reference count.
>>>>
>>>> Did you think about something different?
>>>>
>>>> Thanks
>>>>
>>>> Roberto
>>>
>>> Two alternatives come in mind.
>>>
>>> 1. add tpm_put to linclude/linux/tpm.h
>>> 2. take put_ops away from the implementation
>>> 3. add documentation that tpm_put needs to be called
>>>
>>> or acceptable alternative would memcpy in the implementation
>>>
>>> In any case, you should probably drop completely 'count' and have
>>> function for getting the number of active banks.
>>
>> Having a variable number, makes things more complicated.
>> In the IMA patch set for multiple template digests,
>> the array of algorithms is stored inside a structure
>> called digest descriptor. The array size must be fixed,
>> to avoid VLAIS (which is not supported by clang).
>
> What is VLAI?

Variable-length arrays in structures.


> You can always allocate it from heap.

The algorithms to be used by IMA are set in a __setup function.


>> Since the size of tpm_chip->active_banks is a small number,
>> we can drop count, and assume that the size of the array
>> passed to tpm_pcr_algorithms() always matches the size
>> of the active_banks array.
>>
>> Roberto
>
> I don't understand what you mean.

I meant that callers of tpm_pcr_algorithms() can always pass
an array whose size is the same of that of tpm_chip->active_banks.
Since the array size is known, passing count is not necessary.

Roberto

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v2 2/5] tpm: introduce tpm_pcr_algo_to_crypto() and tpm_pcr_algo_from_crypto()
  2017-05-24 20:25                   ` Jarkko Sakkinen
@ 2017-05-30 10:24                     ` Roberto Sassu
  2017-05-31 11:40                       ` Jarkko Sakkinen
  0 siblings, 1 reply; 31+ messages in thread
From: Roberto Sassu @ 2017-05-30 10:24 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: tpmdd-devel, linux-ima-devel, linux-security-module, keyrings,
	linux-kernel

On 5/24/2017 10:25 PM, Jarkko Sakkinen wrote:
> On Wed, May 24, 2017 at 10:33:54AM -0700, Jarkko Sakkinen wrote:
>> On Mon, May 22, 2017 at 09:21:28AM +0200, Roberto Sassu wrote:
>>> On 5/20/2017 3:22 PM, Jarkko Sakkinen wrote:
>>>> On Mon, May 15, 2017 at 04:22:22PM +0200, Roberto Sassu wrote:
>>>>> On 5/15/2017 1:16 PM, Jarkko Sakkinen wrote:
>>>>>> On Fri, May 05, 2017 at 04:21:49PM +0200, Roberto Sassu wrote:
>>>>>>> tpm_pcr_algorithms() returns to its callers the IDs of the hash algorithms
>>>>>>> supported by the TPM. This patch introduces tpm_pcr_algo_to_crypto(),
>>>>>>> so that the callers can use the crypto subsystem to calculate the digest
>>>>>>> to be passed to tpm_pcr_extend().
>>>>>>>
>>>>>>> tpm_pcr_algo_from_crypto(), implemented for completeness, is instead used
>>>>>>
>>>>>> What do you mean by completeness? Please, never add unused stuff.
>>>>>>
>>>>>>> by tpm2_seal_trusted() to perform the opposite conversion.
>>>>>>>
>>>>>>> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
>>>>>>> ---
>>>>>>> v2
>>>>>>>
>>>>>>> - fixed return values of tpm2_pcr_algo_to_crypto() and
>>>>>>>   tpm2_pcr_algo_from_crypto() if TPM support is disabled in the kernel
>>>>>>
>>>>>> Change Log only to the cover letter.
>>>>>>
>>>>>>>  drivers/char/tpm/tpm-interface.c | 51 ++++++++++++++++++++++++++++++++++++++++
>>>>>>>  drivers/char/tpm/tpm2-cmd.c      | 42 +++++++++------------------------
>>>>>>>  include/linux/tpm.h              | 13 ++++++++++
>>>>>>>  3 files changed, 75 insertions(+), 31 deletions(-)
>>>>>>
>>>>>> This commit is just deadly wrong in so many ways.
>>>>>>
>>>>>> I would suggest to make extend always just take crypto ID in so you
>>>>>> don't have to add these bizarre conversion functions.
>>>>>
>>>>> The reason of this choice (as I explained in the cover letter)
>>>>> is that TPM users might want to produce an event log with
>>>>> the TCG format (which includes the TPM algorithm ID). Also,
>>>>> TPM IDs should be preferred because, with them, TPM users
>>>>> can calculate a digest directly with the TPM.
>>>>>
>>>>> Taking crypto IDs means relying on the fact that there
>>>>> is always a mapping between TPM IDs and crypto IDs.
>>>>> Otherwise, tpm_pcr_algorithms() cannot return the algorithms
>>>>> to its callers and PCRs cannot be extended. If TPM IDs are used,
>>>>> TPM users have two alternatives: calculate the digest with
>>>>> the TPM, or pass a SHA1 digest to tpm_pcr_extend(), so that
>>>>> it can be padded to extend remanining PCR banks.
>>>>>
>>>>> However, this second option will work only when the TPM driver
>>>>> determines the size of an algorithm without relying on the crypto
>>>>> subsystem. At the moment, if a TPM ID is not mapped to a crypto ID,
>>>>> tpm2_pcr_extend() ignores the digest.
>>>>>
>>>>> Roberto
>>>>
>>>> What if tpm_pcr_algorithms would convert TPM IDs to crypto IDs?
>>>>
>>>> Externally other subsystems would have to then deal with only crypto
>>>> IDs.
>>>
>>> Then, other subsystems cannot:
>>>
>>> - use the TPM to calculate a digest
>>> - create an event log with the TCG format
>>>
>>> Roberto
>>
>> Can you open this up a bit? What is the use case that you canno
>> implement?
>>
>> /Jarkko
>
> The most important question is: are this used *right now*. If not, it is
> a definitive NAK. We don't want any future-proof code to the kernel.
> Always do the lowest common denominator in terms of generality and we
> will refactor from there when need arises.

This is still being discussed.

If we use crypto IDs, as you suggested, what it should be passed
to tpm_pcr_extend()? Or, probably to a new function (Mimi said that
changing the parameters of an existing function is not bisect-safe).

Roberto

-- 
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Bo PENG, Qiuen PENG, Shengli WANG

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [Linux-ima-devel] [PATCH v2 3/5] tpm: pass multiple digests to tpm_pcr_extend()
  2017-05-30  7:28       ` Roberto Sassu
@ 2017-05-30 11:25         ` Mimi Zohar
       [not found]           ` <1496143547.3841.517.camel-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
  0 siblings, 1 reply; 31+ messages in thread
From: Mimi Zohar @ 2017-05-30 11:25 UTC (permalink / raw)
  To: Roberto Sassu, tpmdd-devel
  Cc: linux-ima-devel, linux-security-module, keyrings, linux-kernel

On Tue, 2017-05-30 at 09:28 +0200, Roberto Sassu wrote:
> On 5/30/2017 5:29 AM, Mimi Zohar wrote:
> > On Fri, 2017-05-05 at 16:21 +0200, Roberto Sassu wrote:


> >> @@ -876,29 +925,46 @@ static int tpm1_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash,
> >>   * isn't, protect against the chip disappearing, by incrementing
> >>   * the module usage count.
> >>   */
> >> -int tpm_pcr_extend(u32 chip_num, int pcr_idx, const u8 *hash)
> >> +int tpm_pcr_extend(u32 chip_num, int pcr_idx, int count,
> >> +		   struct tpm2_digest *digests)
> >>  {
> >>  	int rc;
> >>  	struct tpm_chip *chip;
> >>  	struct tpm2_digest digest_list[ARRAY_SIZE(chip->active_banks)];
> >> -	u32 count = 0;
> >> +	struct tpm2_digest *digests_ptr = digests;
> >> +	u32 filled_count = 0;
> >> +	u8 *hash;
> >>  	int i;
> >>
> >>  	chip = tpm_chip_find_get(chip_num);
> >>  	if (chip == NULL)
> >>  		return -ENODEV;
> >>
> >> -	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
> >> +	rc = tpm_pcr_check_input(chip, count, digests);
> >> +	if (rc < 0) {
> >> +		dev_dbg(&chip->dev, "%s: invalid arguments\n", __func__);
> >> +		tpm_put_ops(chip);
> >
> > This rejects the TPM extend, if ANY of the algorithms are unknown.
> > Suppose that the standards were updated, TPM vendors add support for
> > the new algorithm, but the kernel has not been updated to reflect the
> > new algorithms supported.  As the measurement hash already been added
> > to the IMA measurement list, verifying the measurement list against a
> > TPM quote will fail, not just for the unknown algorithm, but for all
> > algorithms.  Something is very broken with this approach.
> 
> The alternative is to extend remaining banks with a digest,
> for example the first passed by the caller. I will modify
> the patch, if everyone agrees on that.

The solution you're proposing is similar to the original solution of
extending the TPM with a padded/truncated SHA1 hash, but this time it
might not be a padded/truncated SHA1 hash, but a different algorithm.
So the attestation server will then need to know which hash algorithm
was used to extend each of the TPM banks - a padded/truncated digest
value or the real digest value.

The only issue, from the kernel's perspective, will be determining the
algorithm's digest size as the kernel has no knowledge of it.  On TPM
registration/initialization, if you're not already querying the TPM
for the algorithm digest sizes, you will need to do so.

Mimi


^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [Linux-ima-devel] [PATCH v2 4/5] keys, trusted: modify arguments of tpm_pcr_extend()
  2017-05-30  7:36       ` Roberto Sassu
@ 2017-05-30 12:06         ` Mimi Zohar
       [not found]           ` <1496145961.3841.537.camel-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
  0 siblings, 1 reply; 31+ messages in thread
From: Mimi Zohar @ 2017-05-30 12:06 UTC (permalink / raw)
  To: Roberto Sassu, tpmdd-devel
  Cc: linux-ima-devel, linux-security-module, keyrings, linux-kernel

On Tue, 2017-05-30 at 09:36 +0200, Roberto Sassu wrote:
> On 5/30/2017 5:35 AM, Mimi Zohar wrote:
> > On Fri, 2017-05-05 at 16:21 +0200, Roberto Sassu wrote:
> >> pcrlock() has been modified to pass the correct arguments
> >> to tpm_pcr_extend(): the pointer of a tpm2_digest structure containing
> >> a random value generated by tpm_get_random() and the size of the array (1).
> >
> > If the number of arguments is wrong, that means the patch that
> > introduced the change is not bi-sect safe.  (This comment is
> > applicable to patch 5/5 too.)
> 
> Jarkko (the TPM driver maintainer) asked me to not introduce
> a new function to pass multiple digests, but to modify
> the parameters of tpm_pcr_extend().

Since struct tpm2_digest is a static size, shouldn't we be able to
compute the number of digests?

Mimi


^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [Linux-ima-devel] [PATCH v2 3/5] tpm: pass multiple digests to tpm_pcr_extend()
       [not found]           ` <1496143547.3841.517.camel-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
@ 2017-05-30 13:25             ` Roberto Sassu
  0 siblings, 0 replies; 31+ messages in thread
From: Roberto Sassu @ 2017-05-30 13:25 UTC (permalink / raw)
  To: Mimi Zohar, tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f
  Cc: linux-ima-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
	linux-security-module-u79uwXL29TY76Z2rM5mHXA,
	keyrings-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA

On 5/30/2017 1:25 PM, Mimi Zohar wrote:
> On Tue, 2017-05-30 at 09:28 +0200, Roberto Sassu wrote:
>> On 5/30/2017 5:29 AM, Mimi Zohar wrote:
>>> On Fri, 2017-05-05 at 16:21 +0200, Roberto Sassu wrote:
>
>
>>>> @@ -876,29 +925,46 @@ static int tpm1_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash,
>>>>   * isn't, protect against the chip disappearing, by incrementing
>>>>   * the module usage count.
>>>>   */
>>>> -int tpm_pcr_extend(u32 chip_num, int pcr_idx, const u8 *hash)
>>>> +int tpm_pcr_extend(u32 chip_num, int pcr_idx, int count,
>>>> +		   struct tpm2_digest *digests)
>>>>  {
>>>>  	int rc;
>>>>  	struct tpm_chip *chip;
>>>>  	struct tpm2_digest digest_list[ARRAY_SIZE(chip->active_banks)];
>>>> -	u32 count = 0;
>>>> +	struct tpm2_digest *digests_ptr = digests;
>>>> +	u32 filled_count = 0;
>>>> +	u8 *hash;
>>>>  	int i;
>>>>
>>>>  	chip = tpm_chip_find_get(chip_num);
>>>>  	if (chip == NULL)
>>>>  		return -ENODEV;
>>>>
>>>> -	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
>>>> +	rc = tpm_pcr_check_input(chip, count, digests);
>>>> +	if (rc < 0) {
>>>> +		dev_dbg(&chip->dev, "%s: invalid arguments\n", __func__);
>>>> +		tpm_put_ops(chip);
>>>
>>> This rejects the TPM extend, if ANY of the algorithms are unknown.
>>> Suppose that the standards were updated, TPM vendors add support for
>>> the new algorithm, but the kernel has not been updated to reflect the
>>> new algorithms supported.  As the measurement hash already been added
>>> to the IMA measurement list, verifying the measurement list against a
>>> TPM quote will fail, not just for the unknown algorithm, but for all
>>> algorithms.  Something is very broken with this approach.
>>
>> The alternative is to extend remaining banks with a digest,
>> for example the first passed by the caller. I will modify
>> the patch, if everyone agrees on that.
>
> The solution you're proposing is similar to the original solution of
> extending the TPM with a padded/truncated SHA1 hash, but this time it
> might not be a padded/truncated SHA1 hash, but a different algorithm.
> So the attestation server will then need to know which hash algorithm
> was used to extend each of the TPM banks - a padded/truncated digest
> value or the real digest value.

The convention would be that the first digest in the event log
is truncated/padded, to extend banks for which no digest is
provided. This convention would apply to all callers of
tpm_pcr_extend().

Roberto


> The only issue, from the kernel's perspective, will be determining the
> algorithm's digest size as the kernel has no knowledge of it.  On TPM
> registration/initialization, if you're not already querying the TPM
> for the algorithm digest sizes, you will need to do so.
>
> Mimi
>

-- 
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Bo PENG, Qiuen PENG, Shengli WANG

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [Linux-ima-devel] [PATCH v2 4/5] keys, trusted: modify arguments of tpm_pcr_extend()
       [not found]           ` <1496145961.3841.537.camel-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
@ 2017-05-30 13:41             ` Roberto Sassu
  0 siblings, 0 replies; 31+ messages in thread
From: Roberto Sassu @ 2017-05-30 13:41 UTC (permalink / raw)
  To: Mimi Zohar, tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f
  Cc: linux-ima-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
	linux-security-module-u79uwXL29TY76Z2rM5mHXA,
	keyrings-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA

On 5/30/2017 2:06 PM, Mimi Zohar wrote:
> On Tue, 2017-05-30 at 09:36 +0200, Roberto Sassu wrote:
>> On 5/30/2017 5:35 AM, Mimi Zohar wrote:
>>> On Fri, 2017-05-05 at 16:21 +0200, Roberto Sassu wrote:
>>>> pcrlock() has been modified to pass the correct arguments
>>>> to tpm_pcr_extend(): the pointer of a tpm2_digest structure containing
>>>> a random value generated by tpm_get_random() and the size of the array (1).
>>>
>>> If the number of arguments is wrong, that means the patch that
>>> introduced the change is not bi-sect safe.  (This comment is
>>> applicable to patch 5/5 too.)
>>
>> Jarkko (the TPM driver maintainer) asked me to not introduce
>> a new function to pass multiple digests, but to modify
>> the parameters of tpm_pcr_extend().
>
> Since struct tpm2_digest is a static size, shouldn't we be able to
> compute the number of digests?

The length of 'hash' is not passed to tpm_pcr_extend().

The only way to avoid changing the number of parameters would
be to pass a buffer whose format is the same of the event log
format defined by TCG:

<algo count> <algo1 ID> <digest1> ... <algoN ID> <digestN>

Roberto

-- 
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Bo PENG, Qiuen PENG, Shengli WANG

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v2 1/5] tpm: introduce tpm_pcr_algorithms()
       [not found]                         ` <1cf4fbb1-5683-ceb4-2d5b-8ade25a91e83-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
@ 2017-05-31 11:39                           ` Jarkko Sakkinen
  0 siblings, 0 replies; 31+ messages in thread
From: Jarkko Sakkinen @ 2017-05-31 11:39 UTC (permalink / raw)
  To: Roberto Sassu
  Cc: linux-ima-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
	linux-security-module-u79uwXL29TY76Z2rM5mHXA,
	tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
	keyrings-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA

On Tue, May 30, 2017 at 10:44:50AM +0200, Roberto Sassu wrote:
> On 5/24/2017 7:35 PM, Jarkko Sakkinen wrote:
> > On Mon, May 22, 2017 at 11:07:54AM +0200, Roberto Sassu wrote:
> > > On 5/20/2017 3:18 PM, Jarkko Sakkinen wrote:
> > > > On Wed, May 17, 2017 at 10:42:35AM +0200, Roberto Sassu wrote:
> > > > > On 5/15/2017 3:18 PM, Roberto Sassu wrote:
> > > > > > 
> > > > > > 
> > > > > > On 5/15/2017 12:36 PM, Jarkko Sakkinen wrote:
> > > > > > > On Fri, May 05, 2017 at 04:21:48PM +0200, Roberto Sassu wrote:
> > > > > > > > This function allows TPM users to know which algorithms the TPM
> > > > > > > > supports.
> > > > > > > > It stores the algorithms in a static array of 'enum tpm2_algorithms',
> > > > > > > > allocated by the caller. If the array is not large enough, the function
> > > > > > > > returns an error. Otherwise, it returns the number of algorithms written
> > > > > > > > to the array. If the TPM version is 1.2, the function writes
> > > > > > > > TPM2_ALG_SHA1
> > > > > > > > to first element of the array.
> > > > > > > > 
> > > > > > > > Writing the algorithm also for TPM 1.2 has the advantage that callers
> > > > > > > > can use the API, tpm_pcr_algorithms() and tpm_pcr_extend(), regardless
> > > > > > > > of the TPM version.
> > > > > > > > 
> > > > > > > > A minor change added to this patch was to make available the size of
> > > > > > > > the active_banks array, member of the tpm_chip structure, outside
> > > > > > > > the TPM driver. The array size (TPM_ACTIVE_BANKS_MAX) has been exported
> > > > > > > > to include/linux/tpm.h.
> > > > > > > > 
> > > > > > > > With this information, callers of tpm_pcr_algorithms() can provide
> > > > > > > > a static array with enough room for all the algorithms, instead
> > > > > > > > of receiving the pointer of a dynamic array that they have to free
> > > > > > > > later.
> > > > > > > > 
> > > > > > > > Signed-off-by: Roberto Sassu <roberto.sassu-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> > > > > > > > ---
> > > > > > > > v2
> > > > > > > > 
> > > > > > > > - tpm_pcr_algorithms() returns supported algorithms also for TPM 1.2
> > > > > > > > 
> > > > > > > >  drivers/char/tpm/tpm-interface.c | 46
> > > > > > > > ++++++++++++++++++++++++++++++++++++++++
> > > > > > > >  drivers/char/tpm/tpm.h           | 13 +-----------
> > > > > > > >  include/linux/tpm.h              | 19 +++++++++++++++++
> > > > > > > >  3 files changed, 66 insertions(+), 12 deletions(-)
> > > > > > > > 
> > > > > > > > diff --git a/drivers/char/tpm/tpm-interface.c
> > > > > > > > b/drivers/char/tpm/tpm-interface.c
> > > > > > > > index 4ed08ab..b90de3d 100644
> > > > > > > > --- a/drivers/char/tpm/tpm-interface.c
> > > > > > > > +++ b/drivers/char/tpm/tpm-interface.c
> > > > > > > > @@ -911,6 +911,52 @@ int tpm_pcr_extend(u32 chip_num, int pcr_idx,
> > > > > > > > const u8 *hash)
> > > > > > > >  EXPORT_SYMBOL_GPL(tpm_pcr_extend);
> > > > > > > > 
> > > > > > > >  /**
> > > > > > > > + * tpm_pcr_algorithms - get TPM IDs of active PCR banks algorithms
> > > > > > > 
> > > > > > > The grammar is incorrect here I believe. Should rather be
> > > > > > > 
> > > > > > >   "algorithms of the active PCR banks"
> > > > > > > 
> > > > > > > And there is no such thing as "TPM ID".
> > > > > > > 
> > > > > > > > + * @chip_num:    tpm idx # or ANY
> > > > > > > > + * @count:    # of items in algorithms
> > > > > > > > + * @algorithms:    array of TPM IDs
> > > > > > > > + *
> > > > > > > > + * Returns < 0 on error, and the number of active PCR banks on success.
> > > > > > > > + *
> > > > > > > > + * TPM 1.2 has always one SHA1 bank.
> > > > > > > > + */
> > > > > > > > +int tpm_pcr_algorithms(u32 chip_num, int count,
> > > > > > > > +               enum tpm2_algorithms *algorithms)
> > > > > > >                        unsigned int
> > > > > > > 
> > > > > > > In addition the function name is not too greatg,
> > > > > > > 
> > > > > > > Your syntax for return value is not correct. In addition after
> > > > > > > describing the return value there should not be anything. You should
> > > > > > > study
> > > > > > > 
> > > > > > > https://www.kernel.org/doc/Documentation/kernel-doc-nano-HOWTO.txt
> > > > > > > 
> > > > > > > Better name for the function would be tpm_get_pcr_algorithms().
> > > > > > > 
> > > > > > > > +{
> > > > > > > > +    struct tpm_chip *chip;
> > > > > > > > +    int i;
> > > > > > > > +
> > > > > > > > +    if (count <= 0 || algorithms == NULL)
> > > > > > > > +        return -EINVAL;
> > > > > > > 
> > > > > > > Is there a legal case where caller would pass these values? Now it
> > > > > > > looks like that there is.
> > > > > > > 
> > > > > > > 'count' should unsigned int and zero should be a legal value for
> > > > > > > count.
> > > > > > 
> > > > > > I wanted to avoid that a negative value returned by tpm_pcr_algorithms()
> > > > > > is passed to tpm_pcr_extend() as unsigned int.
> > > > > > 
> > > > > > 
> > > > > > > That said I think the whole design is wrong as you could calculate
> > > > > > > array for algs only one time and pass a const reference to it on
> > > > > > > request.
> > > > > > 
> > > > > > Ok. If I understood it correctly, you are saying to pass a const
> > > > > > reference of chip->active_banks. Then, I should also:
> > > > > 
> > > > > This is not a viable option. chip could be freed and the reference
> > > > > becomes invalid, without increasing the reference count.
> > > > > 
> > > > > Did you think about something different?
> > > > > 
> > > > > Thanks
> > > > > 
> > > > > Roberto
> > > > 
> > > > Two alternatives come in mind.
> > > > 
> > > > 1. add tpm_put to linclude/linux/tpm.h
> > > > 2. take put_ops away from the implementation
> > > > 3. add documentation that tpm_put needs to be called
> > > > 
> > > > or acceptable alternative would memcpy in the implementation
> > > > 
> > > > In any case, you should probably drop completely 'count' and have
> > > > function for getting the number of active banks.
> > > 
> > > Having a variable number, makes things more complicated.
> > > In the IMA patch set for multiple template digests,
> > > the array of algorithms is stored inside a structure
> > > called digest descriptor. The array size must be fixed,
> > > to avoid VLAIS (which is not supported by clang).
> > 
> > What is VLAI?
> 
> Variable-length arrays in structures.
> 
> 
> > You can always allocate it from heap.
> 
> The algorithms to be used by IMA are set in a __setup function.
> 
> 
> > > Since the size of tpm_chip->active_banks is a small number,
> > > we can drop count, and assume that the size of the array
> > > passed to tpm_pcr_algorithms() always matches the size
> > > of the active_banks array.
> > > 
> > > Roberto
> > 
> > I don't understand what you mean.
> 
> I meant that callers of tpm_pcr_algorithms() can always pass
> an array whose size is the same of that of tpm_chip->active_banks.
> Since the array size is known, passing count is not necessary.
> 
> Roberto

OK, got it.

/Jarkko

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v2 2/5] tpm: introduce tpm_pcr_algo_to_crypto() and tpm_pcr_algo_from_crypto()
  2017-05-30 10:24                     ` Roberto Sassu
@ 2017-05-31 11:40                       ` Jarkko Sakkinen
  0 siblings, 0 replies; 31+ messages in thread
From: Jarkko Sakkinen @ 2017-05-31 11:40 UTC (permalink / raw)
  To: Roberto Sassu
  Cc: tpmdd-devel, linux-ima-devel, linux-security-module, keyrings,
	linux-kernel

On Tue, May 30, 2017 at 12:24:21PM +0200, Roberto Sassu wrote:
> On 5/24/2017 10:25 PM, Jarkko Sakkinen wrote:
> > On Wed, May 24, 2017 at 10:33:54AM -0700, Jarkko Sakkinen wrote:
> > > On Mon, May 22, 2017 at 09:21:28AM +0200, Roberto Sassu wrote:
> > > > On 5/20/2017 3:22 PM, Jarkko Sakkinen wrote:
> > > > > On Mon, May 15, 2017 at 04:22:22PM +0200, Roberto Sassu wrote:
> > > > > > On 5/15/2017 1:16 PM, Jarkko Sakkinen wrote:
> > > > > > > On Fri, May 05, 2017 at 04:21:49PM +0200, Roberto Sassu wrote:
> > > > > > > > tpm_pcr_algorithms() returns to its callers the IDs of the hash algorithms
> > > > > > > > supported by the TPM. This patch introduces tpm_pcr_algo_to_crypto(),
> > > > > > > > so that the callers can use the crypto subsystem to calculate the digest
> > > > > > > > to be passed to tpm_pcr_extend().
> > > > > > > > 
> > > > > > > > tpm_pcr_algo_from_crypto(), implemented for completeness, is instead used
> > > > > > > 
> > > > > > > What do you mean by completeness? Please, never add unused stuff.
> > > > > > > 
> > > > > > > > by tpm2_seal_trusted() to perform the opposite conversion.
> > > > > > > > 
> > > > > > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > > > > > > ---
> > > > > > > > v2
> > > > > > > > 
> > > > > > > > - fixed return values of tpm2_pcr_algo_to_crypto() and
> > > > > > > >   tpm2_pcr_algo_from_crypto() if TPM support is disabled in the kernel
> > > > > > > 
> > > > > > > Change Log only to the cover letter.
> > > > > > > 
> > > > > > > >  drivers/char/tpm/tpm-interface.c | 51 ++++++++++++++++++++++++++++++++++++++++
> > > > > > > >  drivers/char/tpm/tpm2-cmd.c      | 42 +++++++++------------------------
> > > > > > > >  include/linux/tpm.h              | 13 ++++++++++
> > > > > > > >  3 files changed, 75 insertions(+), 31 deletions(-)
> > > > > > > 
> > > > > > > This commit is just deadly wrong in so many ways.
> > > > > > > 
> > > > > > > I would suggest to make extend always just take crypto ID in so you
> > > > > > > don't have to add these bizarre conversion functions.
> > > > > > 
> > > > > > The reason of this choice (as I explained in the cover letter)
> > > > > > is that TPM users might want to produce an event log with
> > > > > > the TCG format (which includes the TPM algorithm ID). Also,
> > > > > > TPM IDs should be preferred because, with them, TPM users
> > > > > > can calculate a digest directly with the TPM.
> > > > > > 
> > > > > > Taking crypto IDs means relying on the fact that there
> > > > > > is always a mapping between TPM IDs and crypto IDs.
> > > > > > Otherwise, tpm_pcr_algorithms() cannot return the algorithms
> > > > > > to its callers and PCRs cannot be extended. If TPM IDs are used,
> > > > > > TPM users have two alternatives: calculate the digest with
> > > > > > the TPM, or pass a SHA1 digest to tpm_pcr_extend(), so that
> > > > > > it can be padded to extend remanining PCR banks.
> > > > > > 
> > > > > > However, this second option will work only when the TPM driver
> > > > > > determines the size of an algorithm without relying on the crypto
> > > > > > subsystem. At the moment, if a TPM ID is not mapped to a crypto ID,
> > > > > > tpm2_pcr_extend() ignores the digest.
> > > > > > 
> > > > > > Roberto
> > > > > 
> > > > > What if tpm_pcr_algorithms would convert TPM IDs to crypto IDs?
> > > > > 
> > > > > Externally other subsystems would have to then deal with only crypto
> > > > > IDs.
> > > > 
> > > > Then, other subsystems cannot:
> > > > 
> > > > - use the TPM to calculate a digest
> > > > - create an event log with the TCG format
> > > > 
> > > > Roberto
> > > 
> > > Can you open this up a bit? What is the use case that you canno
> > > implement?
> > > 
> > > /Jarkko
> > 
> > The most important question is: are this used *right now*. If not, it is
> > a definitive NAK. We don't want any future-proof code to the kernel.
> > Always do the lowest common denominator in terms of generality and we
> > will refactor from there when need arises.
> 
> This is still being discussed.
> 
> If we use crypto IDs, as you suggested, what it should be passed
> to tpm_pcr_extend()? Or, probably to a new function (Mimi said that
> changing the parameters of an existing function is not bisect-safe).
> 
> Roberto
> 
> -- 
> HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
> Managing Director: Bo PENG, Qiuen PENG, Shengli WANG

Parameters should be changed when it makes sense to do so. It makes
sense in this case.

/Jarkko

^ permalink raw reply	[flat|nested] 31+ messages in thread

end of thread, other threads:[~2017-05-31 11:40 UTC | newest]

Thread overview: 31+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-05-05 14:21 [PATCH v2 0/5] Updated API for TPM 2.0 PCR extend Roberto Sassu
2017-05-05 14:21 ` [PATCH v2 1/5] tpm: introduce tpm_pcr_algorithms() Roberto Sassu
2017-05-15 10:36   ` Jarkko Sakkinen
     [not found]     ` <20170515103623.sumyo2vyldezual2-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
2017-05-15 13:18       ` Roberto Sassu
     [not found]         ` <d2974dd2-e30d-e8f0-a60a-2147f3290670-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
2017-05-17  8:42           ` Roberto Sassu
2017-05-20 13:18             ` Jarkko Sakkinen
     [not found]               ` <20170520131846.e3niqiknlrttbdf4-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
2017-05-22  9:07                 ` Roberto Sassu
2017-05-24 17:35                   ` Jarkko Sakkinen
     [not found]                     ` <20170524173521.yiiohct7brkcozyk-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
2017-05-30  8:44                       ` Roberto Sassu
     [not found]                         ` <1cf4fbb1-5683-ceb4-2d5b-8ade25a91e83-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
2017-05-31 11:39                           ` Jarkko Sakkinen
2017-05-20 13:00         ` Jarkko Sakkinen
2017-05-05 14:21 ` [PATCH v2 2/5] tpm: introduce tpm_pcr_algo_to_crypto() and tpm_pcr_algo_from_crypto() Roberto Sassu
     [not found]   ` <20170505142152.29795-3-roberto.sassu-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
2017-05-15 11:16     ` Jarkko Sakkinen
     [not found]       ` <20170515111629.urjvbhqzohv4vakc-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
2017-05-15 14:22         ` Roberto Sassu
2017-05-20 13:22           ` Jarkko Sakkinen
     [not found]             ` <20170520132217.t7n7l2pjn7i63hbm-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
2017-05-22  7:21               ` Roberto Sassu
2017-05-24 17:33                 ` Jarkko Sakkinen
2017-05-24 20:25                   ` Jarkko Sakkinen
2017-05-30 10:24                     ` Roberto Sassu
2017-05-31 11:40                       ` Jarkko Sakkinen
2017-05-05 14:21 ` [PATCH v2 3/5] tpm: pass multiple digests to tpm_pcr_extend() Roberto Sassu
     [not found]   ` <20170505142152.29795-4-roberto.sassu-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
2017-05-30  3:29     ` [Linux-ima-devel] " Mimi Zohar
2017-05-30  7:28       ` Roberto Sassu
2017-05-30 11:25         ` Mimi Zohar
     [not found]           ` <1496143547.3841.517.camel-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
2017-05-30 13:25             ` Roberto Sassu
2017-05-05 14:21 ` [PATCH v2 4/5] keys, trusted: modify arguments of tpm_pcr_extend() Roberto Sassu
2017-05-30  3:35   ` [Linux-ima-devel] " Mimi Zohar
     [not found]     ` <1496115337.3841.485.camel-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
2017-05-30  7:36       ` Roberto Sassu
2017-05-30 12:06         ` Mimi Zohar
     [not found]           ` <1496145961.3841.537.camel-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
2017-05-30 13:41             ` Roberto Sassu
2017-05-05 14:21 ` [PATCH v2 5/5] ima: " Roberto Sassu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).