2017-10-23 12:38:34

by Jarkko Sakkinen

Subject: [PATCH] tpm: remove chip_num parameter from in-kernel API

The reasoning is simple and obvious. Since every call site passes the
value TPM_ANY_NUM (0xFFFF) the parameter does not have right to exist.
Refined the documentation of the corresponding functions.

Signed-off-by: Jarkko Sakkinen <[email protected]>
---
drivers/char/hw_random/tpm-rng.c | 2 +-
drivers/char/tpm/tpm-chip.c | 38 ++++++++--------
drivers/char/tpm/tpm-interface.c | 87 ++++++++++++++++++-------------------
drivers/char/tpm/tpm.h | 2 +-
include/linux/tpm.h | 43 ++++++++----------
security/integrity/ima/ima_crypto.c | 2 +-
security/integrity/ima/ima_init.c | 2 +-
security/integrity/ima/ima_queue.c | 2 +-
security/keys/trusted.c | 35 ++++++++-------
9 files changed, 101 insertions(+), 112 deletions(-)

diff --git a/drivers/char/hw_random/tpm-rng.c b/drivers/char/hw_random/tpm-rng.c
index d6d448266f07..8823efcddab8 100644
--- a/drivers/char/hw_random/tpm-rng.c
+++ b/drivers/char/hw_random/tpm-rng.c
@@ -25,7 +25,7 @@

static int tpm_rng_read(struct hwrng *rng, void *data, size_t max, bool wait)
{
- return tpm_get_random(TPM_ANY_NUM, data, max);
+ return tpm_get_random(data, max);
}

static struct hwrng tpm_rng = {
diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index a114e8f7fb90..ec351111643b 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -81,34 +81,32 @@ void tpm_put_ops(struct tpm_chip *chip)
EXPORT_SYMBOL_GPL(tpm_put_ops);

/**
- * tpm_chip_find_get() - return tpm_chip for a given chip number
- * @chip_num: id to find
+ * tpm_chip_find_get() - reserved the first available TPM chip
*
- * The return'd chip has been tpm_try_get_ops'd and must be released via
- * tpm_put_ops
+ * Finds the first available TPM chip and reserves its class device and
+ * operations.
+ *
+ * Return: a reserved &struct tpm_chip instance
*/
-struct tpm_chip *tpm_chip_find_get(int chip_num)
+struct tpm_chip *tpm_chip_find_get(void)
{
- struct tpm_chip *chip, *res = NULL;
+ struct tpm_chip *chip;
+ struct tpm_chip *res = NULL;
int chip_prev;
+ int chip_num;

mutex_lock(&idr_lock);

- if (chip_num == TPM_ANY_NUM) {
- chip_num = 0;
- do {
- chip_prev = chip_num;
- chip = idr_get_next(&dev_nums_idr, &chip_num);
- if (chip && !tpm_try_get_ops(chip)) {
- res = chip;
- break;
- }
- } while (chip_prev != chip_num);
- } else {
- chip = idr_find(&dev_nums_idr, chip_num);
- if (chip && !tpm_try_get_ops(chip))
+ chip_num = 0;
+
+ do {
+ chip_prev = chip_num;
+ chip = idr_get_next(&dev_nums_idr, &chip_num);
+ if (chip && !tpm_try_get_ops(chip)) {
res = chip;
- }
+ break;
+ }
+ } while (chip_prev != chip_num);

mutex_unlock(&idr_lock);

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index d8e2e5bca903..b3907d3556ce 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -802,18 +802,19 @@ int tpm_pcr_read_dev(struct tpm_chip *chip, int pcr_idx, u8 *res_buf)
}

/**
- * tpm_is_tpm2 - is the chip a TPM2 chip?
- * @chip_num: tpm idx # or ANY
+ * tpm_is_tpm2 - do we a have a TPM2 chip?
*
- * Returns < 0 on error, and 1 or 0 on success depending whether the chip
- * is a TPM2 chip.
+ * Return:
+ * 1 if we have a TPM2 chip.
+ * 0 if we don't have a TPM2 chip.
+ * A negative number for system errors (errno).
*/
-int tpm_is_tpm2(u32 chip_num)
+int tpm_is_tpm2(void)
{
struct tpm_chip *chip;
int rc;

- chip = tpm_chip_find_get(chip_num);
+ chip = tpm_chip_find_get();
if (chip == NULL)
return -ENODEV;

@@ -826,22 +827,18 @@ int tpm_is_tpm2(u32 chip_num)
EXPORT_SYMBOL_GPL(tpm_is_tpm2);

/**
- * tpm_pcr_read - read a pcr value
- * @chip_num: tpm idx # or ANY
- * @pcr_idx: pcr idx to retrieve
- * @res_buf: TPM_PCR value
- * size of res_buf is 20 bytes (or NULL if you don't care)
+ * tpm_pcr_read - read a PCR value from SHA1 bank
+ * @pcr_idx: the PCR to be retrieved
+ * @res_buf: the value of the PCR
*
- * The TPM driver should be built-in, but for whatever reason it
- * isn't, protect against the chip disappearing, by incrementing
- * the module usage count.
+ * Return: same as with tpm_transmit_cmd()
*/
-int tpm_pcr_read(u32 chip_num, int pcr_idx, u8 *res_buf)
+int tpm_pcr_read(int pcr_idx, u8 *res_buf)
{
struct tpm_chip *chip;
int rc;

- chip = tpm_chip_find_get(chip_num);
+ chip = tpm_chip_find_get();
if (chip == NULL)
return -ENODEV;
if (chip->flags & TPM_CHIP_FLAG_TPM2)
@@ -882,16 +879,17 @@ static int tpm1_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash,
}

/**
- * tpm_pcr_extend - extend pcr value with hash
- * @chip_num: tpm idx # or AN&
- * @pcr_idx: pcr idx to extend
- * @hash: hash value used to extend pcr value
+ * tpm_pcr_extend - extend a PCR value in SHA1 bank.
+ * @pcr_idx: the PCR to be retrieved
+ * @hash: the hash value used to extend the PCR value
+ *
+ * Note: with TPM 2.0 extends also those banks with a known digest size to the
+ * cryto subsystem in order to prevent malicious use of those PCR banks. In the
+ * future we should dynamically determine digest sizes.
*
- * The TPM driver should be built-in, but for whatever reason it
- * isn't, protect against the chip disappearing, by incrementing
- * the module usage count.
+ * Return: same as with tpm_transmit_cmd()
*/
-int tpm_pcr_extend(u32 chip_num, int pcr_idx, const u8 *hash)
+int tpm_pcr_extend(int pcr_idx, const u8 *hash)
{
int rc;
struct tpm_chip *chip;
@@ -899,7 +897,7 @@ int tpm_pcr_extend(u32 chip_num, int pcr_idx, const u8 *hash)
u32 count = 0;
int i;

- chip = tpm_chip_find_get(chip_num);
+ chip = tpm_chip_find_get();
if (chip == NULL)
return -ENODEV;

@@ -1012,12 +1010,12 @@ int tpm1_auto_startup(struct tpm_chip *chip)
return rc;
}

-int tpm_send(u32 chip_num, void *cmd, size_t buflen)
+int tpm_send(void *cmd, size_t buflen)
{
struct tpm_chip *chip;
int rc;

- chip = tpm_chip_find_get(chip_num);
+ chip = tpm_chip_find_get();
if (chip == NULL)
return -ENODEV;

@@ -1120,14 +1118,13 @@ static const struct tpm_input_header tpm_getrandom_header = {
};

/**
- * tpm_get_random() - Get random bytes from the tpm's RNG
- * @chip_num: A specific chip number for the request or TPM_ANY_NUM
+ * tpm_get_random() - acquire random bytes
* @out: destination buffer for the random bytes
* @max: the max number of bytes to write to @out
*
- * Returns < 0 on error and the number of bytes read on success
+ * Return: same as with tpm_transmit_cmd()
*/
-int tpm_get_random(u32 chip_num, u8 *out, size_t max)
+int tpm_get_random(u8 *out, size_t max)
{
struct tpm_chip *chip;
struct tpm_cmd_t tpm_cmd;
@@ -1138,7 +1135,7 @@ int tpm_get_random(u32 chip_num, u8 *out, size_t max)
if (!out || !num_bytes || max > TPM_MAX_RNG_DATA)
return -EINVAL;

- chip = tpm_chip_find_get(chip_num);
+ chip = tpm_chip_find_get();
if (chip == NULL)
return -ENODEV;

@@ -1181,21 +1178,22 @@ int tpm_get_random(u32 chip_num, u8 *out, size_t max)
EXPORT_SYMBOL_GPL(tpm_get_random);

/**
- * tpm_seal_trusted() - seal a trusted key
- * @chip_num: A specific chip number for the request or TPM_ANY_NUM
+ * tpm_seal_trusted() - seal a trusted key payload
* @options: authentication values and other options
* @payload: the key data in clear and encrypted form
*
- * Returns < 0 on error and 0 on success. At the moment, only TPM 2.0 chips
- * are supported.
+ * Note: at the moment, only TPM 2.0 chip are supported. TPM 1.x implementation
+ * is still located in the keyring subsystem.
+ *
+ * Return: same as with tpm_transmit_cmd()
*/
-int tpm_seal_trusted(u32 chip_num, struct trusted_key_payload *payload,
+int tpm_seal_trusted(struct trusted_key_payload *payload,
struct trusted_key_options *options)
{
struct tpm_chip *chip;
int rc;

- chip = tpm_chip_find_get(chip_num);
+ chip = tpm_chip_find_get();
if (chip == NULL || !(chip->flags & TPM_CHIP_FLAG_TPM2))
return -ENODEV;

@@ -1207,21 +1205,22 @@ int tpm_seal_trusted(u32 chip_num, struct trusted_key_payload *payload,
EXPORT_SYMBOL_GPL(tpm_seal_trusted);

/**
- * tpm_unseal_trusted() - unseal a trusted key
- * @chip_num: A specific chip number for the request or TPM_ANY_NUM
+ * tpm_unseal_trusted() - unseal a trusted key payload
* @options: authentication values and other options
* @payload: the key data in clear and encrypted form
*
- * Returns < 0 on error and 0 on success. At the moment, only TPM 2.0 chips
- * are supported.
+ * Note: at the moment, only TPM 2.0 chip are supported. TPM 1.x implementation
+ * is still located in the keyring subsystem.
+ *
+ * Return: same as with tpm_transmit_cmd()
*/
-int tpm_unseal_trusted(u32 chip_num, struct trusted_key_payload *payload,
+int tpm_unseal_trusted(struct trusted_key_payload *payload,
struct trusted_key_options *options)
{
struct tpm_chip *chip;
int rc;

- chip = tpm_chip_find_get(chip_num);
+ chip = tpm_chip_find_get();
if (chip == NULL || !(chip->flags & TPM_CHIP_FLAG_TPM2))
return -ENODEV;

diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index c1866cc02e30..269c32bb3af0 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -516,7 +516,7 @@ static inline void tpm_msleep(unsigned int delay_msec)
delay_msec * 1000);
};

-struct tpm_chip *tpm_chip_find_get(int chip_num);
+struct tpm_chip *tpm_chip_find_get(void);
__must_check int tpm_try_get_ops(struct tpm_chip *chip);
void tpm_put_ops(struct tpm_chip *chip);

diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index 5a090f5ab335..54cd6d903d31 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -24,11 +24,6 @@

#define TPM_DIGEST_SIZE 20 /* Max TPM v1.2 PCR size */

-/*
- * Chip num is this value or a valid tpm idx
- */
-#define TPM_ANY_NUM 0xFFFF
-
struct tpm_chip;
struct trusted_key_payload;
struct trusted_key_options;
@@ -53,44 +48,42 @@ struct tpm_class_ops {
};

#if defined(CONFIG_TCG_TPM) || defined(CONFIG_TCG_TPM_MODULE)
-
-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_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,
- struct trusted_key_payload *payload,
+extern int tpm_is_tpm2(void);
+extern int tpm_pcr_read(int pcr_idx, u8 *res_buf);
+extern int tpm_pcr_extend(int pcr_idx, const u8 *hash);
+extern int tpm_send(void *cmd, size_t buflen);
+extern int tpm_get_random(u8 *data, size_t max);
+extern int tpm_seal_trusted(struct trusted_key_payload *payload,
struct trusted_key_options *options);
-extern int tpm_unseal_trusted(u32 chip_num,
- struct trusted_key_payload *payload,
+extern int tpm_unseal_trusted(struct trusted_key_payload *payload,
struct trusted_key_options *options);
#else
-static inline int tpm_is_tpm2(u32 chip_num)
+static inline int tpm_is_tpm2(void)
{
return -ENODEV;
}
-static inline int tpm_pcr_read(u32 chip_num, int pcr_idx, u8 *res_buf) {
+static inline int tpm_pcr_read(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(int pcr_idx, const u8 *hash)
+{
return -ENODEV;
}
-static inline int tpm_send(u32 chip_num, void *cmd, size_t buflen) {
+static inline int tpm_send(void *cmd, size_t buflen)
+{
return -ENODEV;
}
-static inline int tpm_get_random(u32 chip_num, u8 *data, size_t max) {
+static inline int tpm_get_random(u8 *data, size_t max)
+{
return -ENODEV;
}
-
-static inline int tpm_seal_trusted(u32 chip_num,
- struct trusted_key_payload *payload,
+static inline int tpm_seal_trusted(struct trusted_key_payload *payload,
struct trusted_key_options *options)
{
return -ENODEV;
}
-static inline int tpm_unseal_trusted(u32 chip_num,
- struct trusted_key_payload *payload,
+static inline int tpm_unseal_trusted(struct trusted_key_payload *payload,
struct trusted_key_options *options)
{
return -ENODEV;
diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
index 802d5d20f36f..b5828bafab26 100644
--- a/security/integrity/ima/ima_crypto.c
+++ b/security/integrity/ima/ima_crypto.c
@@ -644,7 +644,7 @@ static void __init ima_pcrread(int idx, u8 *pcr)
if (!ima_used_chip)
return;

- if (tpm_pcr_read(TPM_ANY_NUM, idx, pcr) != 0)
+ if (tpm_pcr_read(idx, pcr) != 0)
pr_err("Error Communicating to TPM chip\n");
}

diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c
index 2967d497a665..21be72f604cd 100644
--- a/security/integrity/ima/ima_init.c
+++ b/security/integrity/ima/ima_init.c
@@ -110,7 +110,7 @@ int __init ima_init(void)
int rc;

ima_used_chip = 0;
- rc = tpm_pcr_read(TPM_ANY_NUM, 0, pcr_i);
+ rc = tpm_pcr_read(0, pcr_i);
if (rc == 0)
ima_used_chip = 1;

diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c
index a02a86d51102..d33966ff210d 100644
--- a/security/integrity/ima/ima_queue.c
+++ b/security/integrity/ima/ima_queue.c
@@ -145,7 +145,7 @@ static int ima_pcr_extend(const u8 *hash, int pcr)
if (!ima_used_chip)
return result;

- result = tpm_pcr_extend(TPM_ANY_NUM, pcr, hash);
+ result = tpm_pcr_extend(pcr, hash);
if (result != 0)
pr_err("Error Communicating to TPM chip, result: %d\n", result);
return result;
diff --git a/security/keys/trusted.c b/security/keys/trusted.c
index ddfaebf60fc8..f912b5bffdad 100644
--- a/security/keys/trusted.c
+++ b/security/keys/trusted.c
@@ -355,13 +355,12 @@ static int TSS_checkhmac2(unsigned char *buffer,
* For key specific tpm requests, we will generate and send our
* own TPM command packets using the drivers send function.
*/
-static int trusted_tpm_send(const u32 chip_num, unsigned char *cmd,
- size_t buflen)
+static int trusted_tpm_send(unsigned char *cmd, size_t buflen)
{
int rc;

dump_tpm_buf(cmd);
- rc = tpm_send(chip_num, cmd, buflen);
+ rc = tpm_send(cmd, buflen);
dump_tpm_buf(cmd);
if (rc > 0)
/* Can't return positive return codes values to keyctl */
@@ -382,10 +381,10 @@ static int pcrlock(const int pcrnum)

if (!capable(CAP_SYS_ADMIN))
return -EPERM;
- ret = tpm_get_random(TPM_ANY_NUM, hash, SHA1_DIGEST_SIZE);
+ ret = tpm_get_random(hash, 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(pcrnum, hash) ? -EINVAL : 0;
}

/*
@@ -398,7 +397,7 @@ static int osap(struct tpm_buf *tb, struct osapsess *s,
unsigned char ononce[TPM_NONCE_SIZE];
int ret;

- ret = tpm_get_random(TPM_ANY_NUM, ononce, TPM_NONCE_SIZE);
+ ret = tpm_get_random(ononce, TPM_NONCE_SIZE);
if (ret != TPM_NONCE_SIZE)
return ret;

@@ -410,7 +409,7 @@ static int osap(struct tpm_buf *tb, struct osapsess *s,
store32(tb, handle);
storebytes(tb, ononce, TPM_NONCE_SIZE);

- ret = trusted_tpm_send(TPM_ANY_NUM, tb->data, MAX_BUF_SIZE);
+ ret = trusted_tpm_send(tb->data, MAX_BUF_SIZE);
if (ret < 0)
return ret;

@@ -434,7 +433,7 @@ static int oiap(struct tpm_buf *tb, uint32_t *handle, unsigned char *nonce)
store16(tb, TPM_TAG_RQU_COMMAND);
store32(tb, TPM_OIAP_SIZE);
store32(tb, TPM_ORD_OIAP);
- ret = trusted_tpm_send(TPM_ANY_NUM, tb->data, MAX_BUF_SIZE);
+ ret = trusted_tpm_send(tb->data, MAX_BUF_SIZE);
if (ret < 0)
return ret;

@@ -493,7 +492,7 @@ static int tpm_seal(struct tpm_buf *tb, uint16_t keytype,
if (ret < 0)
goto out;

- ret = tpm_get_random(TPM_ANY_NUM, td->nonceodd, TPM_NONCE_SIZE);
+ ret = tpm_get_random(td->nonceodd, TPM_NONCE_SIZE);
if (ret != TPM_NONCE_SIZE)
goto out;
ordinal = htonl(TPM_ORD_SEAL);
@@ -542,7 +541,7 @@ static int tpm_seal(struct tpm_buf *tb, uint16_t keytype,
store8(tb, cont);
storebytes(tb, td->pubauth, SHA1_DIGEST_SIZE);

- ret = trusted_tpm_send(TPM_ANY_NUM, tb->data, MAX_BUF_SIZE);
+ ret = trusted_tpm_send(tb->data, MAX_BUF_SIZE);
if (ret < 0)
goto out;

@@ -603,7 +602,7 @@ static int tpm_unseal(struct tpm_buf *tb,

ordinal = htonl(TPM_ORD_UNSEAL);
keyhndl = htonl(SRKHANDLE);
- ret = tpm_get_random(TPM_ANY_NUM, nonceodd, TPM_NONCE_SIZE);
+ ret = tpm_get_random(nonceodd, TPM_NONCE_SIZE);
if (ret != TPM_NONCE_SIZE) {
pr_info("trusted_key: tpm_get_random failed (%d)\n", ret);
return ret;
@@ -635,7 +634,7 @@ static int tpm_unseal(struct tpm_buf *tb,
store8(tb, cont);
storebytes(tb, authdata2, SHA1_DIGEST_SIZE);

- ret = trusted_tpm_send(TPM_ANY_NUM, tb->data, MAX_BUF_SIZE);
+ ret = trusted_tpm_send(tb->data, MAX_BUF_SIZE);
if (ret < 0) {
pr_info("trusted_key: authhmac failed (%d)\n", ret);
return ret;
@@ -748,7 +747,7 @@ static int getoptions(char *c, struct trusted_key_payload *pay,
int i;
int tpm2;

- tpm2 = tpm_is_tpm2(TPM_ANY_NUM);
+ tpm2 = tpm_is_tpm2();
if (tpm2 < 0)
return tpm2;

@@ -917,7 +916,7 @@ static struct trusted_key_options *trusted_options_alloc(void)
struct trusted_key_options *options;
int tpm2;

- tpm2 = tpm_is_tpm2(TPM_ANY_NUM);
+ tpm2 = tpm_is_tpm2();
if (tpm2 < 0)
return NULL;

@@ -967,7 +966,7 @@ static int trusted_instantiate(struct key *key,
size_t key_len;
int tpm2;

- tpm2 = tpm_is_tpm2(TPM_ANY_NUM);
+ tpm2 = tpm_is_tpm2();
if (tpm2 < 0)
return tpm2;

@@ -1008,7 +1007,7 @@ static int trusted_instantiate(struct key *key,
switch (key_cmd) {
case Opt_load:
if (tpm2)
- ret = tpm_unseal_trusted(TPM_ANY_NUM, payload, options);
+ ret = tpm_unseal_trusted(payload, options);
else
ret = key_unseal(payload, options);
dump_payload(payload);
@@ -1018,13 +1017,13 @@ static int trusted_instantiate(struct key *key,
break;
case Opt_new:
key_len = payload->key_len;
- ret = tpm_get_random(TPM_ANY_NUM, payload->key, key_len);
+ ret = tpm_get_random(payload->key, key_len);
if (ret != key_len) {
pr_info("trusted_key: key_create failed (%d)\n", ret);
goto out;
}
if (tpm2)
- ret = tpm_seal_trusted(TPM_ANY_NUM, payload, options);
+ ret = tpm_seal_trusted(payload, options);
else
ret = key_seal(payload, options);
if (ret < 0)
--
2.14.1

2017-10-23 14:07:31

by Stefan Berger

Subject: Re: [tpmdd-devel] [PATCH] tpm: remove chip_num parameter from in-kernel API

On 10/23/2017 08:38 AM, Jarkko Sakkinen wrote:
> The reasoning is simple and obvious. Since every call site passes the
> value TPM_ANY_NUM (0xFFFF) the parameter does not have right to exist.
> Refined the documentation of the corresponding functions.
>
> Signed-off-by: Jarkko Sakkinen <[email protected]>
> ---
> drivers/char/hw_random/tpm-rng.c | 2 +-
> drivers/char/tpm/tpm-chip.c | 38 ++++++++--------
> drivers/char/tpm/tpm-interface.c | 87 ++++++++++++++++++-------------------
> drivers/char/tpm/tpm.h | 2 +-
> include/linux/tpm.h | 43 ++++++++----------
> security/integrity/ima/ima_crypto.c | 2 +-
> security/integrity/ima/ima_init.c | 2 +-
> security/integrity/ima/ima_queue.c | 2 +-
> security/keys/trusted.c | 35 ++++++++-------
> 9 files changed, 101 insertions(+), 112 deletions(-)
>
> diff --git a/drivers/char/hw_random/tpm-rng.c b/drivers/char/hw_random/tpm-rng.c
> index d6d448266f07..8823efcddab8 100644
> --- a/drivers/char/hw_random/tpm-rng.c
> +++ b/drivers/char/hw_random/tpm-rng.c
> @@ -25,7 +25,7 @@
>
> static int tpm_rng_read(struct hwrng *rng, void *data, size_t max, bool wait)
> {
> - return tpm_get_random(TPM_ANY_NUM, data, max);
> + return tpm_get_random(data, max);
> }
>
> static struct hwrng tpm_rng = {
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index a114e8f7fb90..ec351111643b 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -81,34 +81,32 @@ void tpm_put_ops(struct tpm_chip *chip)
> EXPORT_SYMBOL_GPL(tpm_put_ops);
>
> /**
> - * tpm_chip_find_get() - return tpm_chip for a given chip number
> - * @chip_num: id to find
> + * tpm_chip_find_get() - reserved the first available TPM chip
> *
> - * The return'd chip has been tpm_try_get_ops'd and must be released via
> - * tpm_put_ops
> + * Finds the first available TPM chip and reserves its class device and
> + * operations.
> + *
> + * Return: a reserved &struct tpm_chip instance
> */
> -struct tpm_chip *tpm_chip_find_get(int chip_num)
> +struct tpm_chip *tpm_chip_find_get(void)
> {
> - struct tpm_chip *chip, *res = NULL;
> + struct tpm_chip *chip;
> + struct tpm_chip *res = NULL;
> int chip_prev;
> + int chip_num;
>
> mutex_lock(&idr_lock);
>
> - if (chip_num == TPM_ANY_NUM) {
> - chip_num = 0;
> - do {
> - chip_prev = chip_num;
> - chip = idr_get_next(&dev_nums_idr, &chip_num);
> - if (chip && !tpm_try_get_ops(chip)) {
> - res = chip;
> - break;
> - }
> - } while (chip_prev != chip_num);
> - } else {
> - chip = idr_find(&dev_nums_idr, chip_num);
> - if (chip && !tpm_try_get_ops(chip))
> + chip_num = 0;
> +
> + do {
> + chip_prev = chip_num;
> + chip = idr_get_next(&dev_nums_idr, &chip_num);
> + if (chip && !tpm_try_get_ops(chip)) {
> res = chip;
> - }
> + break;
> + }
> + } while (chip_prev != chip_num);
>
> mutex_unlock(&idr_lock);


Here you are keeping the loop, which I think is good and I would like
you to keep it...


> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index d8e2e5bca903..b3907d3556ce 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -802,18 +802,19 @@ int tpm_pcr_read_dev(struct tpm_chip *chip, int pcr_idx, u8 *res_buf)
> }
>
> /**
> - * tpm_is_tpm2 - is the chip a TPM2 chip?
> - * @chip_num: tpm idx # or ANY
> + * tpm_is_tpm2 - do we a have a TPM2 chip?
> *
> - * Returns < 0 on error, and 1 or 0 on success depending whether the chip
> - * is a TPM2 chip.
> + * Return:
> + * 1 if we have a TPM2 chip.
> + * 0 if we don't have a TPM2 chip.
> + * A negative number for system errors (errno).
> */
> -int tpm_is_tpm2(u32 chip_num)
> +int tpm_is_tpm2(void)
> {
> struct tpm_chip *chip;
> int rc;
>
> - chip = tpm_chip_find_get(chip_num);
> + chip = tpm_chip_find_get();
> if (chip == NULL)
> return -ENODEV;
>
> @@ -826,22 +827,18 @@ int tpm_is_tpm2(u32 chip_num)
> EXPORT_SYMBOL_GPL(tpm_is_tpm2);
>
> /**
> - * tpm_pcr_read - read a pcr value
> - * @chip_num: tpm idx # or ANY
> - * @pcr_idx: pcr idx to retrieve
> - * @res_buf: TPM_PCR value
> - * size of res_buf is 20 bytes (or NULL if you don't care)
> + * tpm_pcr_read - read a PCR value from SHA1 bank
> + * @pcr_idx: the PCR to be retrieved
> + * @res_buf: the value of the PCR
> *
> - * The TPM driver should be built-in, but for whatever reason it
> - * isn't, protect against the chip disappearing, by incrementing
> - * the module usage count.
> + * Return: same as with tpm_transmit_cmd()
> */
> -int tpm_pcr_read(u32 chip_num, int pcr_idx, u8 *res_buf)
> +int tpm_pcr_read(int pcr_idx, u8 *res_buf)
> {
> struct tpm_chip *chip;
> int rc;
>
> - chip = tpm_chip_find_get(chip_num);
> + chip = tpm_chip_find_get();
> if (chip == NULL)
> return -ENODEV;
> if (chip->flags & TPM_CHIP_FLAG_TPM2)
> @@ -882,16 +879,17 @@ static int tpm1_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash,
> }
>
> /**
> - * tpm_pcr_extend - extend pcr value with hash
> - * @chip_num: tpm idx # or AN&
> - * @pcr_idx: pcr idx to extend
> - * @hash: hash value used to extend pcr value
> + * tpm_pcr_extend - extend a PCR value in SHA1 bank.
> + * @pcr_idx: the PCR to be retrieved
> + * @hash: the hash value used to extend the PCR value
> + *
> + * Note: with TPM 2.0 extends also those banks with a known digest size to the
> + * cryto subsystem in order to prevent malicious use of those PCR banks. In the
> + * future we should dynamically determine digest sizes.
> *
> - * The TPM driver should be built-in, but for whatever reason it
> - * isn't, protect against the chip disappearing, by incrementing
> - * the module usage count.
> + * Return: same as with tpm_transmit_cmd()
> */
> -int tpm_pcr_extend(u32 chip_num, int pcr_idx, const u8 *hash)
> +int tpm_pcr_extend(int pcr_idx, const u8 *hash)
> {


I think every kernel internal TPM driver API should be called with the
tpm_chip as a parameter. This is in foresight of namespacing of IMA
where we want to provide the flexibility of passing a dedicated vTPM to
each namespace and IMA would use the chip as a parameter to all of these
functions to talk to the right tpm_vtpm_proxy instance. From that
perspective this patch goes into the wrong direction.

Stefan

> int rc;
> struct tpm_chip *chip;
> @@ -899,7 +897,7 @@ int tpm_pcr_extend(u32 chip_num, int pcr_idx, const u8 *hash)
> u32 count = 0;
> int i;
>
> - chip = tpm_chip_find_get(chip_num);
> + chip = tpm_chip_find_get();
> if (chip == NULL)
> return -ENODEV;
>
> @@ -1012,12 +1010,12 @@ int tpm1_auto_startup(struct tpm_chip *chip)
> return rc;
> }
>
> -int tpm_send(u32 chip_num, void *cmd, size_t buflen)
> +int tpm_send(void *cmd, size_t buflen)
> {
> struct tpm_chip *chip;
> int rc;
>
> - chip = tpm_chip_find_get(chip_num);
> + chip = tpm_chip_find_get();
> if (chip == NULL)
> return -ENODEV;
>
> @@ -1120,14 +1118,13 @@ static const struct tpm_input_header tpm_getrandom_header = {
> };
>
> /**
> - * tpm_get_random() - Get random bytes from the tpm's RNG
> - * @chip_num: A specific chip number for the request or TPM_ANY_NUM
> + * tpm_get_random() - acquire random bytes
> * @out: destination buffer for the random bytes
> * @max: the max number of bytes to write to @out
> *
> - * Returns < 0 on error and the number of bytes read on success
> + * Return: same as with tpm_transmit_cmd()
> */
> -int tpm_get_random(u32 chip_num, u8 *out, size_t max)
> +int tpm_get_random(u8 *out, size_t max)
> {
> struct tpm_chip *chip;
> struct tpm_cmd_t tpm_cmd;
> @@ -1138,7 +1135,7 @@ int tpm_get_random(u32 chip_num, u8 *out, size_t max)
> if (!out || !num_bytes || max > TPM_MAX_RNG_DATA)
> return -EINVAL;
>
> - chip = tpm_chip_find_get(chip_num);
> + chip = tpm_chip_find_get();
> if (chip == NULL)
> return -ENODEV;
>
> @@ -1181,21 +1178,22 @@ int tpm_get_random(u32 chip_num, u8 *out, size_t max)
> EXPORT_SYMBOL_GPL(tpm_get_random);
>
> /**
> - * tpm_seal_trusted() - seal a trusted key
> - * @chip_num: A specific chip number for the request or TPM_ANY_NUM
> + * tpm_seal_trusted() - seal a trusted key payload
> * @options: authentication values and other options
> * @payload: the key data in clear and encrypted form
> *
> - * Returns < 0 on error and 0 on success. At the moment, only TPM 2.0 chips
> - * are supported.
> + * Note: at the moment, only TPM 2.0 chip are supported. TPM 1.x implementation
> + * is still located in the keyring subsystem.
> + *
> + * Return: same as with tpm_transmit_cmd()
> */
> -int tpm_seal_trusted(u32 chip_num, struct trusted_key_payload *payload,
> +int tpm_seal_trusted(struct trusted_key_payload *payload,
> struct trusted_key_options *options)
> {
> struct tpm_chip *chip;
> int rc;
>
> - chip = tpm_chip_find_get(chip_num);
> + chip = tpm_chip_find_get();
> if (chip == NULL || !(chip->flags & TPM_CHIP_FLAG_TPM2))
> return -ENODEV;
>
> @@ -1207,21 +1205,22 @@ int tpm_seal_trusted(u32 chip_num, struct trusted_key_payload *payload,
> EXPORT_SYMBOL_GPL(tpm_seal_trusted);
>
> /**
> - * tpm_unseal_trusted() - unseal a trusted key
> - * @chip_num: A specific chip number for the request or TPM_ANY_NUM
> + * tpm_unseal_trusted() - unseal a trusted key payload
> * @options: authentication values and other options
> * @payload: the key data in clear and encrypted form
> *
> - * Returns < 0 on error and 0 on success. At the moment, only TPM 2.0 chips
> - * are supported.
> + * Note: at the moment, only TPM 2.0 chip are supported. TPM 1.x implementation
> + * is still located in the keyring subsystem.
> + *
> + * Return: same as with tpm_transmit_cmd()
> */
> -int tpm_unseal_trusted(u32 chip_num, struct trusted_key_payload *payload,
> +int tpm_unseal_trusted(struct trusted_key_payload *payload,
> struct trusted_key_options *options)
> {
> struct tpm_chip *chip;
> int rc;
>
> - chip = tpm_chip_find_get(chip_num);
> + chip = tpm_chip_find_get();
> if (chip == NULL || !(chip->flags & TPM_CHIP_FLAG_TPM2))
> return -ENODEV;
>
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index c1866cc02e30..269c32bb3af0 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -516,7 +516,7 @@ static inline void tpm_msleep(unsigned int delay_msec)
> delay_msec * 1000);
> };
>
> -struct tpm_chip *tpm_chip_find_get(int chip_num);
> +struct tpm_chip *tpm_chip_find_get(void);
> __must_check int tpm_try_get_ops(struct tpm_chip *chip);
> void tpm_put_ops(struct tpm_chip *chip);
>
> diff --git a/include/linux/tpm.h b/include/linux/tpm.h
> index 5a090f5ab335..54cd6d903d31 100644
> --- a/include/linux/tpm.h
> +++ b/include/linux/tpm.h
> @@ -24,11 +24,6 @@
>
> #define TPM_DIGEST_SIZE 20 /* Max TPM v1.2 PCR size */
>
> -/*
> - * Chip num is this value or a valid tpm idx
> - */
> -#define TPM_ANY_NUM 0xFFFF
> -
> struct tpm_chip;
> struct trusted_key_payload;
> struct trusted_key_options;
> @@ -53,44 +48,42 @@ struct tpm_class_ops {
> };
>
> #if defined(CONFIG_TCG_TPM) || defined(CONFIG_TCG_TPM_MODULE)
> -
> -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_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,
> - struct trusted_key_payload *payload,
> +extern int tpm_is_tpm2(void);
> +extern int tpm_pcr_read(int pcr_idx, u8 *res_buf);
> +extern int tpm_pcr_extend(int pcr_idx, const u8 *hash);
> +extern int tpm_send(void *cmd, size_t buflen);
> +extern int tpm_get_random(u8 *data, size_t max);
> +extern int tpm_seal_trusted(struct trusted_key_payload *payload,
> struct trusted_key_options *options);
> -extern int tpm_unseal_trusted(u32 chip_num,
> - struct trusted_key_payload *payload,
> +extern int tpm_unseal_trusted(struct trusted_key_payload *payload,
> struct trusted_key_options *options);
> #else
> -static inline int tpm_is_tpm2(u32 chip_num)
> +static inline int tpm_is_tpm2(void)
> {
> return -ENODEV;
> }
> -static inline int tpm_pcr_read(u32 chip_num, int pcr_idx, u8 *res_buf) {
> +static inline int tpm_pcr_read(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(int pcr_idx, const u8 *hash)
> +{
> return -ENODEV;
> }
> -static inline int tpm_send(u32 chip_num, void *cmd, size_t buflen) {
> +static inline int tpm_send(void *cmd, size_t buflen)
> +{
> return -ENODEV;
> }
> -static inline int tpm_get_random(u32 chip_num, u8 *data, size_t max) {
> +static inline int tpm_get_random(u8 *data, size_t max)
> +{
> return -ENODEV;
> }
> -
> -static inline int tpm_seal_trusted(u32 chip_num,
> - struct trusted_key_payload *payload,
> +static inline int tpm_seal_trusted(struct trusted_key_payload *payload,
> struct trusted_key_options *options)
> {
> return -ENODEV;
> }
> -static inline int tpm_unseal_trusted(u32 chip_num,
> - struct trusted_key_payload *payload,
> +static inline int tpm_unseal_trusted(struct trusted_key_payload *payload,
> struct trusted_key_options *options)
> {
> return -ENODEV;
> diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
> index 802d5d20f36f..b5828bafab26 100644
> --- a/security/integrity/ima/ima_crypto.c
> +++ b/security/integrity/ima/ima_crypto.c
> @@ -644,7 +644,7 @@ static void __init ima_pcrread(int idx, u8 *pcr)
> if (!ima_used_chip)
> return;
>
> - if (tpm_pcr_read(TPM_ANY_NUM, idx, pcr) != 0)
> + if (tpm_pcr_read(idx, pcr) != 0)
> pr_err("Error Communicating to TPM chip\n");
> }
>
> diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c
> index 2967d497a665..21be72f604cd 100644
> --- a/security/integrity/ima/ima_init.c
> +++ b/security/integrity/ima/ima_init.c
> @@ -110,7 +110,7 @@ int __init ima_init(void)
> int rc;
>
> ima_used_chip = 0;
> - rc = tpm_pcr_read(TPM_ANY_NUM, 0, pcr_i);
> + rc = tpm_pcr_read(0, pcr_i);
> if (rc == 0)
> ima_used_chip = 1;
>
> diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c
> index a02a86d51102..d33966ff210d 100644
> --- a/security/integrity/ima/ima_queue.c
> +++ b/security/integrity/ima/ima_queue.c
> @@ -145,7 +145,7 @@ static int ima_pcr_extend(const u8 *hash, int pcr)
> if (!ima_used_chip)
> return result;
>
> - result = tpm_pcr_extend(TPM_ANY_NUM, pcr, hash);
> + result = tpm_pcr_extend(pcr, hash);
> if (result != 0)
> pr_err("Error Communicating to TPM chip, result: %d\n", result);
> return result;
> diff --git a/security/keys/trusted.c b/security/keys/trusted.c
> index ddfaebf60fc8..f912b5bffdad 100644
> --- a/security/keys/trusted.c
> +++ b/security/keys/trusted.c
> @@ -355,13 +355,12 @@ static int TSS_checkhmac2(unsigned char *buffer,
> * For key specific tpm requests, we will generate and send our
> * own TPM command packets using the drivers send function.
> */
> -static int trusted_tpm_send(const u32 chip_num, unsigned char *cmd,
> - size_t buflen)
> +static int trusted_tpm_send(unsigned char *cmd, size_t buflen)
> {
> int rc;
>
> dump_tpm_buf(cmd);
> - rc = tpm_send(chip_num, cmd, buflen);
> + rc = tpm_send(cmd, buflen);
> dump_tpm_buf(cmd);
> if (rc > 0)
> /* Can't return positive return codes values to keyctl */
> @@ -382,10 +381,10 @@ static int pcrlock(const int pcrnum)
>
> if (!capable(CAP_SYS_ADMIN))
> return -EPERM;
> - ret = tpm_get_random(TPM_ANY_NUM, hash, SHA1_DIGEST_SIZE);
> + ret = tpm_get_random(hash, 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(pcrnum, hash) ? -EINVAL : 0;
> }
>
> /*
> @@ -398,7 +397,7 @@ static int osap(struct tpm_buf *tb, struct osapsess *s,
> unsigned char ononce[TPM_NONCE_SIZE];
> int ret;
>
> - ret = tpm_get_random(TPM_ANY_NUM, ononce, TPM_NONCE_SIZE);
> + ret = tpm_get_random(ononce, TPM_NONCE_SIZE);
> if (ret != TPM_NONCE_SIZE)
> return ret;
>
> @@ -410,7 +409,7 @@ static int osap(struct tpm_buf *tb, struct osapsess *s,
> store32(tb, handle);
> storebytes(tb, ononce, TPM_NONCE_SIZE);
>
> - ret = trusted_tpm_send(TPM_ANY_NUM, tb->data, MAX_BUF_SIZE);
> + ret = trusted_tpm_send(tb->data, MAX_BUF_SIZE);
> if (ret < 0)
> return ret;
>
> @@ -434,7 +433,7 @@ static int oiap(struct tpm_buf *tb, uint32_t *handle, unsigned char *nonce)
> store16(tb, TPM_TAG_RQU_COMMAND);
> store32(tb, TPM_OIAP_SIZE);
> store32(tb, TPM_ORD_OIAP);
> - ret = trusted_tpm_send(TPM_ANY_NUM, tb->data, MAX_BUF_SIZE);
> + ret = trusted_tpm_send(tb->data, MAX_BUF_SIZE);
> if (ret < 0)
> return ret;
>
> @@ -493,7 +492,7 @@ static int tpm_seal(struct tpm_buf *tb, uint16_t keytype,
> if (ret < 0)
> goto out;
>
> - ret = tpm_get_random(TPM_ANY_NUM, td->nonceodd, TPM_NONCE_SIZE);
> + ret = tpm_get_random(td->nonceodd, TPM_NONCE_SIZE);
> if (ret != TPM_NONCE_SIZE)
> goto out;
> ordinal = htonl(TPM_ORD_SEAL);
> @@ -542,7 +541,7 @@ static int tpm_seal(struct tpm_buf *tb, uint16_t keytype,
> store8(tb, cont);
> storebytes(tb, td->pubauth, SHA1_DIGEST_SIZE);
>
> - ret = trusted_tpm_send(TPM_ANY_NUM, tb->data, MAX_BUF_SIZE);
> + ret = trusted_tpm_send(tb->data, MAX_BUF_SIZE);
> if (ret < 0)
> goto out;
>
> @@ -603,7 +602,7 @@ static int tpm_unseal(struct tpm_buf *tb,
>
> ordinal = htonl(TPM_ORD_UNSEAL);
> keyhndl = htonl(SRKHANDLE);
> - ret = tpm_get_random(TPM_ANY_NUM, nonceodd, TPM_NONCE_SIZE);
> + ret = tpm_get_random(nonceodd, TPM_NONCE_SIZE);
> if (ret != TPM_NONCE_SIZE) {
> pr_info("trusted_key: tpm_get_random failed (%d)\n", ret);
> return ret;
> @@ -635,7 +634,7 @@ static int tpm_unseal(struct tpm_buf *tb,
> store8(tb, cont);
> storebytes(tb, authdata2, SHA1_DIGEST_SIZE);
>
> - ret = trusted_tpm_send(TPM_ANY_NUM, tb->data, MAX_BUF_SIZE);
> + ret = trusted_tpm_send(tb->data, MAX_BUF_SIZE);
> if (ret < 0) {
> pr_info("trusted_key: authhmac failed (%d)\n", ret);
> return ret;
> @@ -748,7 +747,7 @@ static int getoptions(char *c, struct trusted_key_payload *pay,
> int i;
> int tpm2;
>
> - tpm2 = tpm_is_tpm2(TPM_ANY_NUM);
> + tpm2 = tpm_is_tpm2();
> if (tpm2 < 0)
> return tpm2;
>
> @@ -917,7 +916,7 @@ static struct trusted_key_options *trusted_options_alloc(void)
> struct trusted_key_options *options;
> int tpm2;
>
> - tpm2 = tpm_is_tpm2(TPM_ANY_NUM);
> + tpm2 = tpm_is_tpm2();
> if (tpm2 < 0)
> return NULL;
>
> @@ -967,7 +966,7 @@ static int trusted_instantiate(struct key *key,
> size_t key_len;
> int tpm2;
>
> - tpm2 = tpm_is_tpm2(TPM_ANY_NUM);
> + tpm2 = tpm_is_tpm2();
> if (tpm2 < 0)
> return tpm2;
>
> @@ -1008,7 +1007,7 @@ static int trusted_instantiate(struct key *key,
> switch (key_cmd) {
> case Opt_load:
> if (tpm2)
> - ret = tpm_unseal_trusted(TPM_ANY_NUM, payload, options);
> + ret = tpm_unseal_trusted(payload, options);
> else
> ret = key_unseal(payload, options);
> dump_payload(payload);
> @@ -1018,13 +1017,13 @@ static int trusted_instantiate(struct key *key,
> break;
> case Opt_new:
> key_len = payload->key_len;
> - ret = tpm_get_random(TPM_ANY_NUM, payload->key, key_len);
> + ret = tpm_get_random(payload->key, key_len);
> if (ret != key_len) {
> pr_info("trusted_key: key_create failed (%d)\n", ret);
> goto out;
> }
> if (tpm2)
> - ret = tpm_seal_trusted(TPM_ANY_NUM, payload, options);
> + ret = tpm_seal_trusted(payload, options);
> else
> ret = key_seal(payload, options);
> if (ret < 0)

2017-10-23 16:31:39

by Jason Gunthorpe

Subject: Re: [tpmdd-devel] [PATCH] tpm: remove chip_num parameter from in-kernel API

On Mon, Oct 23, 2017 at 10:07:31AM -0400, Stefan Berger wrote:

> >-int tpm_pcr_extend(u32 chip_num, int pcr_idx, const u8 *hash)
> >+int tpm_pcr_extend(int pcr_idx, const u8 *hash)
> > {
>
>
> I think every kernel internal TPM driver API should be called with the
> tpm_chip as a parameter. This is in foresight of namespacing of IMA where we
> want to provide the flexibility of passing a dedicated vTPM to each
> namespace and IMA would use the chip as a parameter to all of these
> functions to talk to the right tpm_vtpm_proxy instance. From that
> perspective this patch goes into the wrong direction.

Yes, we should ultimately try and get to there.. Someday the
tpm_chip_find_get() will need to become namespace aware.

But this patch is along the right path, eliminating the chip_num is
the right thing to do..

> >- tpm2 = tpm_is_tpm2(TPM_ANY_NUM);
> >+ tpm2 = tpm_is_tpm2();
> > if (tpm2 < 0)
> > return tpm2;
> >
> >@@ -1008,7 +1007,7 @@ static int trusted_instantiate(struct key *key,
> > switch (key_cmd) {
> > case Opt_load:
> > if (tpm2)
> >- ret = tpm_unseal_trusted(TPM_ANY_NUM, payload, options);
> >+ ret = tpm_unseal_trusted(payload, options);

Sequences like this are sketchy.

It should be

struct tpm_chip *tpm;

tpm = tpm_chip_find_get()
tpm2 = tpm_is_tpm2(tpm);

[..]

if (tpm2)
ret = tpm_unseal_trusted(tpm, payload, options);

[..]

tpm_put_chip(tpm);

As hot plug could alter the 'tpm' between the two tpm calls.

Jason

2017-10-24 14:04:15

by Jarkko Sakkinen

Subject: Re: [tpmdd-devel] [PATCH] tpm: remove chip_num parameter from in-kernel API

On Mon, Oct 23, 2017 at 10:07:31AM -0400, Stefan Berger wrote:
> I think every kernel internal TPM driver API should be called with the
> tpm_chip as a parameter. This is in foresight of namespacing of IMA where we
> want to provide the flexibility of passing a dedicated vTPM to each
> namespace and IMA would use the chip as a parameter to all of these
> functions to talk to the right tpm_vtpm_proxy instance. From that
> perspective this patch goes into the wrong direction.
>
> Stefan

The goal of this patch is to kernel code that never gets executed. It
removes a load of completely dead code. It is the only thing that this
commit does. Why do you think this is "going into wrong direction" if it
only removes dead code and refines the documentation up to date?

After the dead code has been removed it makes sense to propose a better
mechanism. Maybe the one that you are speaking about. But you need to
remove the cruft first.

/Jarkko

2017-10-24 15:44:40

by Jarkko Sakkinen

Subject: Re: [tpmdd-devel] [PATCH] tpm: remove chip_num parameter from in-kernel API

On Mon, Oct 23, 2017 at 10:31:39AM -0600, Jason Gunthorpe wrote:
> On Mon, Oct 23, 2017 at 10:07:31AM -0400, Stefan Berger wrote:
>
> > >-int tpm_pcr_extend(u32 chip_num, int pcr_idx, const u8 *hash)
> > >+int tpm_pcr_extend(int pcr_idx, const u8 *hash)
> > > {
> >
> >
> > I think every kernel internal TPM driver API should be called with the
> > tpm_chip as a parameter. This is in foresight of namespacing of IMA where we
> > want to provide the flexibility of passing a dedicated vTPM to each
> > namespace and IMA would use the chip as a parameter to all of these
> > functions to talk to the right tpm_vtpm_proxy instance. From that
> > perspective this patch goes into the wrong direction.
>
> Yes, we should ultimately try and get to there.. Someday the
> tpm_chip_find_get() will need to become namespace aware.
>
> But this patch is along the right path, eliminating the chip_num is
> the right thing to do..
>
> > >- tpm2 = tpm_is_tpm2(TPM_ANY_NUM);
> > >+ tpm2 = tpm_is_tpm2();
> > > if (tpm2 < 0)
> > > return tpm2;
> > >
> > >@@ -1008,7 +1007,7 @@ static int trusted_instantiate(struct key *key,
> > > switch (key_cmd) {
> > > case Opt_load:
> > > if (tpm2)
> > >- ret = tpm_unseal_trusted(TPM_ANY_NUM, payload, options);
> > >+ ret = tpm_unseal_trusted(payload, options);
>
> Sequences like this are sketchy.
>
> It should be
>
> struct tpm_chip *tpm;
>
> tpm = tpm_chip_find_get()
> tpm2 = tpm_is_tpm2(tpm);
>
> [..]
>
> if (tpm2)
> ret = tpm_unseal_trusted(tpm, payload, options);
>
> [..]
>
> tpm_put_chip(tpm);
>
> As hot plug could alter the 'tpm' between the two tpm calls.
>
> Jason

This patch just removes bunch of dead code. It does not change existing
semantics. What you are saying should be done after the dead code has
been removed. This commit is first step to that direction.

/Jarkko

2017-10-24 15:51:15

by PrasannaKumar Muralidharan

Subject: Re: [PATCH] tpm: remove chip_num parameter from in-kernel API

On 24 October 2017 at 21:14, Jarkko Sakkinen
<jarkko.sakkinen-VuQAYsv1563Yd54FQh9/[email protected]> wrote:
> On Mon, Oct 23, 2017 at 10:31:39AM -0600, Jason Gunthorpe wrote:
>> On Mon, Oct 23, 2017 at 10:07:31AM -0400, Stefan Berger wrote:
>>
>> > >-int tpm_pcr_extend(u32 chip_num, int pcr_idx, const u8 *hash)
>> > >+int tpm_pcr_extend(int pcr_idx, const u8 *hash)
>> > > {
>> >
>> >
>> > I think every kernel internal TPM driver API should be called with the
>> > tpm_chip as a parameter. This is in foresight of namespacing of IMA where we
>> > want to provide the flexibility of passing a dedicated vTPM to each
>> > namespace and IMA would use the chip as a parameter to all of these
>> > functions to talk to the right tpm_vtpm_proxy instance. From that
>> > perspective this patch goes into the wrong direction.
>>
>> Yes, we should ultimately try and get to there.. Someday the
>> tpm_chip_find_get() will need to become namespace aware.
>>
>> But this patch is along the right path, eliminating the chip_num is
>> the right thing to do..
>>
>> > >- tpm2 = tpm_is_tpm2(TPM_ANY_NUM);
>> > >+ tpm2 = tpm_is_tpm2();
>> > > if (tpm2 < 0)
>> > > return tpm2;
>> > >
>> > >@@ -1008,7 +1007,7 @@ static int trusted_instantiate(struct key *key,
>> > > switch (key_cmd) {
>> > > case Opt_load:
>> > > if (tpm2)
>> > >- ret = tpm_unseal_trusted(TPM_ANY_NUM, payload, options);
>> > >+ ret = tpm_unseal_trusted(payload, options);
>>
>> Sequences like this are sketchy.
>>
>> It should be
>>
>> struct tpm_chip *tpm;
>>
>> tpm = tpm_chip_find_get()
>> tpm2 = tpm_is_tpm2(tpm);
>>
>> [..]
>>
>> if (tpm2)
>> ret = tpm_unseal_trusted(tpm, payload, options);
>>
>> [..]
>>
>> tpm_put_chip(tpm);
>>
>> As hot plug could alter the 'tpm' between the two tpm calls.
>>
>> Jason
>
> This patch just removes bunch of dead code. It does not change existing
> semantics. What you are saying should be done after the dead code has
> been removed. This commit is first step to that direction.
>
> /Jarkko

Please check the RFC [1]. It does use chip id. The rfc has issues and
has to be fixed but still there could be users of the API.

1. https://www.spinics.net/lists/linux-crypto/msg28282.html

Regards,
PrasannaKumar

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

2017-10-24 15:55:37

by Jason Gunthorpe

Subject: Re: [tpmdd-devel] [PATCH] tpm: remove chip_num parameter from in-kernel API

On Tue, Oct 24, 2017 at 09:21:15PM +0530, PrasannaKumar Muralidharan wrote:

> Please check the RFC [1]. It does use chip id. The rfc has issues and
> has to be fixed but still there could be users of the API.
>
> 1. https://www.spinics.net/lists/linux-crypto/msg28282.html

That patch isn't safe at all. You need to store a kref to th chip in
the hwrng, not parse a string.

Jason

2017-10-24 16:07:35

by PrasannaKumar Muralidharan

Subject: Re: [tpmdd-devel] [PATCH] tpm: remove chip_num parameter from in-kernel API

Hi Jason,

On 24 October 2017 at 21:25, Jason Gunthorpe
<[email protected]> wrote:
> On Tue, Oct 24, 2017 at 09:21:15PM +0530, PrasannaKumar Muralidharan wrote:
>
>> Please check the RFC [1]. It does use chip id. The rfc has issues and
>> has to be fixed but still there could be users of the API.
>>
>> 1. https://www.spinics.net/lists/linux-crypto/msg28282.html
>
> That patch isn't safe at all. You need to store a kref to th chip in
> the hwrng, not parse a string.

The drivers/char/hw_random/tpm-rng.c module does not store the chip
reference so I guess the usage is safe. The RFC is just a sample use
case of the API.

Regards,
PrasannaKumar

2017-10-24 16:11:18

by Jason Gunthorpe

Subject: Re: [tpmdd-devel] [PATCH] tpm: remove chip_num parameter from in-kernel API

On Tue, Oct 24, 2017 at 09:37:33PM +0530, PrasannaKumar Muralidharan wrote:
> Hi Jason,
>
> On 24 October 2017 at 21:25, Jason Gunthorpe
> <[email protected]> wrote:
> > On Tue, Oct 24, 2017 at 09:21:15PM +0530, PrasannaKumar Muralidharan wrote:
> >
> >> Please check the RFC [1]. It does use chip id. The rfc has issues and
> >> has to be fixed but still there could be users of the API.
> >>
> >> 1. https://www.spinics.net/lists/linux-crypto/msg28282.html
> >
> > That patch isn't safe at all. You need to store a kref to th chip in
> > the hwrng, not parse a string.
>
> The drivers/char/hw_random/tpm-rng.c module does not store the chip
> reference so I guess the usage is safe.

It is using the default TPM, it is always safe to use the default tpm.

> The RFC is just a sample use case of the API.

Well, a wrong example not to be emulated, and I think, further shows
how Jarkko's direction is the right one.

Jason

2017-10-24 16:14:30

by PrasannaKumar Muralidharan

Subject: Re: [tpmdd-devel] [PATCH] tpm: remove chip_num parameter from in-kernel API

On 24 October 2017 at 21:41, Jason Gunthorpe
<[email protected]> wrote:
> On Tue, Oct 24, 2017 at 09:37:33PM +0530, PrasannaKumar Muralidharan wrote:
>> Hi Jason,
>>
>> On 24 October 2017 at 21:25, Jason Gunthorpe
>> <[email protected]> wrote:
>> > On Tue, Oct 24, 2017 at 09:21:15PM +0530, PrasannaKumar Muralidharan wrote:
>> >
>> >> Please check the RFC [1]. It does use chip id. The rfc has issues and
>> >> has to be fixed but still there could be users of the API.
>> >>
>> >> 1. https://www.spinics.net/lists/linux-crypto/msg28282.html
>> >
>> > That patch isn't safe at all. You need to store a kref to th chip in
>> > the hwrng, not parse a string.
>>
>> The drivers/char/hw_random/tpm-rng.c module does not store the chip
>> reference so I guess the usage is safe.
>
> It is using the default TPM, it is always safe to use the default tpm.
>
>> The RFC is just a sample use case of the API.
>
> Well, a wrong example not to be emulated, and I think, further shows
> how Jarkko's direction is the right one.

I am wondering why it is wrong. Isn't the chip id valid till it is
unregistered? If so the rfc is correct. Please explain, may be I am
missing something.

Thanks,
PrasannaKumar

2017-10-24 16:23:59

by Jarkko Sakkinen

Subject: Re: [tpmdd-devel] [PATCH] tpm: remove chip_num parameter from in-kernel API

On Tue, Oct 24, 2017 at 09:21:15PM +0530, PrasannaKumar Muralidharan wrote:
> On 24 October 2017 at 21:14, Jarkko Sakkinen
> <[email protected]> wrote:
> > On Mon, Oct 23, 2017 at 10:31:39AM -0600, Jason Gunthorpe wrote:
> >> On Mon, Oct 23, 2017 at 10:07:31AM -0400, Stefan Berger wrote:
> >>
> >> > >-int tpm_pcr_extend(u32 chip_num, int pcr_idx, const u8 *hash)
> >> > >+int tpm_pcr_extend(int pcr_idx, const u8 *hash)
> >> > > {
> >> >
> >> >
> >> > I think every kernel internal TPM driver API should be called with the
> >> > tpm_chip as a parameter. This is in foresight of namespacing of IMA where we
> >> > want to provide the flexibility of passing a dedicated vTPM to each
> >> > namespace and IMA would use the chip as a parameter to all of these
> >> > functions to talk to the right tpm_vtpm_proxy instance. From that
> >> > perspective this patch goes into the wrong direction.
> >>
> >> Yes, we should ultimately try and get to there.. Someday the
> >> tpm_chip_find_get() will need to become namespace aware.
> >>
> >> But this patch is along the right path, eliminating the chip_num is
> >> the right thing to do..
> >>
> >> > >- tpm2 = tpm_is_tpm2(TPM_ANY_NUM);
> >> > >+ tpm2 = tpm_is_tpm2();
> >> > > if (tpm2 < 0)
> >> > > return tpm2;
> >> > >
> >> > >@@ -1008,7 +1007,7 @@ static int trusted_instantiate(struct key *key,
> >> > > switch (key_cmd) {
> >> > > case Opt_load:
> >> > > if (tpm2)
> >> > >- ret = tpm_unseal_trusted(TPM_ANY_NUM, payload, options);
> >> > >+ ret = tpm_unseal_trusted(payload, options);
> >>
> >> Sequences like this are sketchy.
> >>
> >> It should be
> >>
> >> struct tpm_chip *tpm;
> >>
> >> tpm = tpm_chip_find_get()
> >> tpm2 = tpm_is_tpm2(tpm);
> >>
> >> [..]
> >>
> >> if (tpm2)
> >> ret = tpm_unseal_trusted(tpm, payload, options);
> >>
> >> [..]
> >>
> >> tpm_put_chip(tpm);
> >>
> >> As hot plug could alter the 'tpm' between the two tpm calls.
> >>
> >> Jason
> >
> > This patch just removes bunch of dead code. It does not change existing
> > semantics. What you are saying should be done after the dead code has
> > been removed. This commit is first step to that direction.
> >
> > /Jarkko
>
> Please check the RFC [1]. It does use chip id. The rfc has issues and
> has to be fixed but still there could be users of the API.
>
> 1. https://www.spinics.net/lists/linux-crypto/msg28282.html
>
> Regards,
> PrasannaKumar

1. Every user in the kernel is using TPM_ANY_NUM, which means there are
no other users.
2. Moving struct tpm_rng to the TPM client is architecturally
uacceptable.
3. Using zero deos not give you any better guarantees on anything than
just using TPM_ANY_NUM.

Why this patch is not CC'd to linux-integrity? It modifies the TPM
driver. And in the worst way.

Implementing the ideas that Jason explained is the senseful way to
get stable access. modules.dep makes sure that the modules are loaded
in the correct order.

/Jarkko

2017-10-24 16:35:20

by PrasannaKumar Muralidharan

Subject: Re: [tpmdd-devel] [PATCH] tpm: remove chip_num parameter from in-kernel API

On 24 October 2017 at 21:53, Jarkko Sakkinen
<[email protected]> wrote:
> On Tue, Oct 24, 2017 at 09:21:15PM +0530, PrasannaKumar Muralidharan wrote:
>> On 24 October 2017 at 21:14, Jarkko Sakkinen
>> <[email protected]> wrote:
>> > On Mon, Oct 23, 2017 at 10:31:39AM -0600, Jason Gunthorpe wrote:
>> >> On Mon, Oct 23, 2017 at 10:07:31AM -0400, Stefan Berger wrote:
>> >>
>> >> > >-int tpm_pcr_extend(u32 chip_num, int pcr_idx, const u8 *hash)
>> >> > >+int tpm_pcr_extend(int pcr_idx, const u8 *hash)
>> >> > > {
>> >> >
>> >> >
>> >> > I think every kernel internal TPM driver API should be called with the
>> >> > tpm_chip as a parameter. This is in foresight of namespacing of IMA where we
>> >> > want to provide the flexibility of passing a dedicated vTPM to each
>> >> > namespace and IMA would use the chip as a parameter to all of these
>> >> > functions to talk to the right tpm_vtpm_proxy instance. From that
>> >> > perspective this patch goes into the wrong direction.
>> >>
>> >> Yes, we should ultimately try and get to there.. Someday the
>> >> tpm_chip_find_get() will need to become namespace aware.
>> >>
>> >> But this patch is along the right path, eliminating the chip_num is
>> >> the right thing to do..
>> >>
>> >> > >- tpm2 = tpm_is_tpm2(TPM_ANY_NUM);
>> >> > >+ tpm2 = tpm_is_tpm2();
>> >> > > if (tpm2 < 0)
>> >> > > return tpm2;
>> >> > >
>> >> > >@@ -1008,7 +1007,7 @@ static int trusted_instantiate(struct key *key,
>> >> > > switch (key_cmd) {
>> >> > > case Opt_load:
>> >> > > if (tpm2)
>> >> > >- ret = tpm_unseal_trusted(TPM_ANY_NUM, payload, options);
>> >> > >+ ret = tpm_unseal_trusted(payload, options);
>> >>
>> >> Sequences like this are sketchy.
>> >>
>> >> It should be
>> >>
>> >> struct tpm_chip *tpm;
>> >>
>> >> tpm = tpm_chip_find_get()
>> >> tpm2 = tpm_is_tpm2(tpm);
>> >>
>> >> [..]
>> >>
>> >> if (tpm2)
>> >> ret = tpm_unseal_trusted(tpm, payload, options);
>> >>
>> >> [..]
>> >>
>> >> tpm_put_chip(tpm);
>> >>
>> >> As hot plug could alter the 'tpm' between the two tpm calls.
>> >>
>> >> Jason
>> >
>> > This patch just removes bunch of dead code. It does not change existing
>> > semantics. What you are saying should be done after the dead code has
>> > been removed. This commit is first step to that direction.
>> >
>> > /Jarkko
>>
>> Please check the RFC [1]. It does use chip id. The rfc has issues and
>> has to be fixed but still there could be users of the API.
>>
>> 1. https://www.spinics.net/lists/linux-crypto/msg28282.html
>>
>> Regards,
>> PrasannaKumar
>
> 1. Every user in the kernel is using TPM_ANY_NUM, which means there are
> no other users.

Completely agree that there is no in kernel users yet.

> 2. Moving struct tpm_rng to the TPM client is architecturally
> uacceptable.

As there was no response to the patch there is no way to know whether
it is acceptable or not.

> 3. Using zero deos not give you any better guarantees on anything than
> just using TPM_ANY_NUM.

Chip id is used, not zero.

> Why this patch is not CC'd to linux-integrity? It modifies the TPM
> driver. And in the worst way.

TPM list is moderated and the moderator has not approved it yet.
get_maintainer script did not say about linux-integrity mailing list.

It could be doing things in worst way but it is not known until some
one says. If no one tells it is the case I don't think it is possible
to fix. Which is what happened.

> Implementing the ideas that Jason explained is the senseful way to
> get stable access. modules.dep makes sure that the modules are loaded
> in the correct order.

If that is sensible then it is the way to go.

There must be a reason to believe what is sensible and what is not.
Looks like this RFC has helped in judging that.

Regards,
PrasannaKumar

2017-10-24 17:02:00

by Dmitry Torokhov

Subject: Re: [tpmdd-devel] [PATCH] tpm: remove chip_num parameter from in-kernel API

On Tue, Oct 24, 2017 at 9:11 AM, Jason Gunthorpe
<[email protected]> wrote:
> On Tue, Oct 24, 2017 at 09:37:33PM +0530, PrasannaKumar Muralidharan wrote:
>> Hi Jason,
>>
>> On 24 October 2017 at 21:25, Jason Gunthorpe
>> <[email protected]> wrote:
>> > On Tue, Oct 24, 2017 at 09:21:15PM +0530, PrasannaKumar Muralidharan wrote:
>> >
>> >> Please check the RFC [1]. It does use chip id. The rfc has issues and
>> >> has to be fixed but still there could be users of the API.
>> >>
>> >> 1. https://www.spinics.net/lists/linux-crypto/msg28282.html
>> >
>> > That patch isn't safe at all. You need to store a kref to th chip in
>> > the hwrng, not parse a string.
>>
>> The drivers/char/hw_random/tpm-rng.c module does not store the chip
>> reference so I guess the usage is safe.
>
> It is using the default TPM, it is always safe to use the default tpm.

tpm-rng is abomination that should be kicked out as soon as possible.
It wrecks havoc with the power management (TPM chip drivers may go
into suspend state, but tpm_rng does not do any power management and
happily forwards requests to suspended hardware) and may be available
when there is no TPM at all yet (the drivers have not been probed yet,
or have gotten a deferral, etc).

TPM core should register HWRNGs when chips are ready.

Thanks.

--
Dmitry

2017-10-24 17:38:08

by Jason Gunthorpe

Subject: Re: [tpmdd-devel] [PATCH] tpm: remove chip_num parameter from in-kernel API

On Tue, Oct 24, 2017 at 10:02:00AM -0700, Dmitry Torokhov wrote:
> tpm-rng is abomination that should be kicked out as soon as possible.
> It wrecks havoc with the power management (TPM chip drivers may go
> into suspend state, but tpm_rng does not do any power management and
> happily forwards requests to suspended hardware) and may be available
> when there is no TPM at all yet (the drivers have not been probed yet,
> or have gotten a deferral, etc).

Makes sense

> TPM core should register HWRNGs when chips are ready.

The main thing I've wanted from the TPM RNG is
'add_early_randomness'..

We can certainly provide a TPM interface to hwrng, it seems
reasonable.

Excep that we already have a user api in /dev/tpm to access the
tpm RNG, is the duplication a problem?

Jason

2017-10-24 17:44:33

by PrasannaKumar Muralidharan

Subject: Re: [tpmdd-devel] [PATCH] tpm: remove chip_num parameter from in-kernel API

On 24 October 2017 at 23:07, Jason Gunthorpe
<[email protected]> wrote:
> On Tue, Oct 24, 2017 at 10:02:00AM -0700, Dmitry Torokhov wrote:
>> tpm-rng is abomination that should be kicked out as soon as possible.
>> It wrecks havoc with the power management (TPM chip drivers may go
>> into suspend state, but tpm_rng does not do any power management and
>> happily forwards requests to suspended hardware) and may be available
>> when there is no TPM at all yet (the drivers have not been probed yet,
>> or have gotten a deferral, etc).
>
> Makes sense
>
>> TPM core should register HWRNGs when chips are ready.
>
> The main thing I've wanted from the TPM RNG is
> 'add_early_randomness'..
>
> We can certainly provide a TPM interface to hwrng, it seems
> reasonable.
>
> Excep that we already have a user api in /dev/tpm to access the
> tpm RNG, is the duplication a problem?

I tried to do that via the rfc we discussed previously. It may not be
the right way but I wanted to start the discussion via the rfc.

Thanks,
PrasannaKumar

2017-10-24 17:46:45

by Jason Gunthorpe

Subject: Re: [tpmdd-devel] [PATCH] tpm: remove chip_num parameter from in-kernel API

On Tue, Oct 24, 2017 at 09:44:30PM +0530, PrasannaKumar Muralidharan wrote:

> I am wondering why it is wrong. Isn't the chip id valid till it is
> unregistered? If so the rfc is correct. Please explain, may be I am
> missing something.

The lifetime is a bit complicated, but the general rule in the kernel
for things like this it to use pointers, not ids, and certainly not
string ids.

For that patch it could just use container_of to get the chip..

Jason

2017-10-24 17:56:52

by PrasannaKumar Muralidharan

Subject: Re: [tpmdd-devel] [PATCH] tpm: remove chip_num parameter from in-kernel API

Hi Jason,

On 24 October 2017 at 23:16, Jason Gunthorpe
<[email protected]> wrote:
> On Tue, Oct 24, 2017 at 09:44:30PM +0530, PrasannaKumar Muralidharan wrote:
>
>> I am wondering why it is wrong. Isn't the chip id valid till it is
>> unregistered? If so the rfc is correct. Please explain, may be I am
>> missing something.
>
> The lifetime is a bit complicated, but the general rule in the kernel
> for things like this it to use pointers, not ids, and certainly not
> string ids.
>
> For that patch it could just use container_of to get the chip..
>
> Jason

hwrng requires a unique name for every device. In that patch
"tpm-rng-<chip_num>" is used. chip_num is nothing but dev->dev_num.
This way more than 1 tpm chip can be used as rng provider.
tpm_get_random uses chip_num as its parameter. This is why
chip->dev_num was used.

Is that reasoning correct?

Please feel free to correct me if I am wrong.

Thanks,
PrasannaKumar

2017-10-24 18:04:44

by Dmitry Torokhov

Subject: Re: [PATCH] tpm: remove chip_num parameter from in-kernel API

On Tue, Oct 24, 2017 at 11:37:57AM -0600, Jason Gunthorpe wrote:
> On Tue, Oct 24, 2017 at 10:02:00AM -0700, Dmitry Torokhov wrote:
> > tpm-rng is abomination that should be kicked out as soon as possible.
> > It wrecks havoc with the power management (TPM chip drivers may go
> > into suspend state, but tpm_rng does not do any power management and
> > happily forwards requests to suspended hardware) and may be available
> > when there is no TPM at all yet (the drivers have not been probed yet,
> > or have gotten a deferral, etc).
>
> Makes sense
>
> > TPM core should register HWRNGs when chips are ready.
>
> The main thing I've wanted from the TPM RNG is
> 'add_early_randomness'..

I see... However you can't add any randomness if hardware is not quite
there yet.

>
> We can certainly provide a TPM interface to hwrng, it seems
> reasonable.
>
> Excep that we already have a user api in /dev/tpm to access the
> tpm RNG, is the duplication a problem?

If we already have userspace API we have to maintain this, even if it is
duplicate, there is no way around it. I'd expect most of the users will
use unified random API, while some TPM-oriented users may still go via
/dev/tpm to use the same API as all other their requests.

Thanks.

--
Dmitry

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

2017-10-24 18:15:12

by Jarkko Sakkinen

Subject: Re: [tpmdd-devel] [PATCH] tpm: remove chip_num parameter from in-kernel API

On Tue, Oct 24, 2017 at 10:02:00AM -0700, Dmitry Torokhov wrote:
> On Tue, Oct 24, 2017 at 9:11 AM, Jason Gunthorpe
> <[email protected]> wrote:
> > On Tue, Oct 24, 2017 at 09:37:33PM +0530, PrasannaKumar Muralidharan wrote:
> >> Hi Jason,
> >>
> >> On 24 October 2017 at 21:25, Jason Gunthorpe
> >> <[email protected]> wrote:
> >> > On Tue, Oct 24, 2017 at 09:21:15PM +0530, PrasannaKumar Muralidharan wrote:
> >> >
> >> >> Please check the RFC [1]. It does use chip id. The rfc has issues and
> >> >> has to be fixed but still there could be users of the API.
> >> >>
> >> >> 1. https://www.spinics.net/lists/linux-crypto/msg28282.html
> >> >
> >> > That patch isn't safe at all. You need to store a kref to th chip in
> >> > the hwrng, not parse a string.
> >>
> >> The drivers/char/hw_random/tpm-rng.c module does not store the chip
> >> reference so I guess the usage is safe.
> >
> > It is using the default TPM, it is always safe to use the default tpm.
>
> tpm-rng is abomination that should be kicked out as soon as possible.
> It wrecks havoc with the power management (TPM chip drivers may go
> into suspend state, but tpm_rng does not do any power management and
> happily forwards requests to suspended hardware) and may be available
> when there is no TPM at all yet (the drivers have not been probed yet,
> or have gotten a deferral, etc).
>
> TPM core should register HWRNGs when chips are ready.
>
> Thanks.
>
> --
> Dmitry

I'm fine to review a two patch set where:

1. Patch 1 removes the existing TPM rng driver
2. Patch 2 makes the TPM driver as rng producer

Unrelate to patch that I'm proposing now but this sounds sensible.

/Jarkko

2017-10-24 18:22:35

by Jarkko Sakkinen

Subject: Re: [PATCH] tpm: remove chip_num parameter from in-kernel API

On Tue, Oct 24, 2017 at 10:05:20PM +0530, PrasannaKumar Muralidharan wrote:
> > 1. Every user in the kernel is using TPM_ANY_NUM, which means there are
> > no other users.
>
> Completely agree that there is no in kernel users yet.

And should never be. It's a bogus parameter that makes no sense.

> > 2. Moving struct tpm_rng to the TPM client is architecturally
> > uacceptable.
>
> As there was no response to the patch there is no way to know whether
> it is acceptable or not.

I like the idea of removing the tpm rng driver as discussed in other
emails in this thread.

> > 3. Using zero deos not give you any better guarantees on anything than
> > just using TPM_ANY_NUM.
>
> Chip id is used, not zero.

Sorry I misread the patch first time. Anyway it's not any kind of ID to
be trusted.

> > Why this patch is not CC'd to linux-integrity? It modifies the TPM
> > driver. And in the worst way.
>
> TPM list is moderated and the moderator has not approved it yet.
> get_maintainer script did not say about linux-integrity mailing list.
>
> It could be doing things in worst way but it is not known until some
> one says. If no one tells it is the case I don't think it is possible
> to fix. Which is what happened.

Understood. We've moved to linux-integrity-u79uwXL29TaiAVqoAR/[email protected] MAINTAINERS
update is in the queue for the next kernel release.

> > Implementing the ideas that Jason explained is the senseful way to
> > get stable access. modules.dep makes sure that the modules are loaded
> > in the correct order.
>
> If that is sensible then it is the way to go.
>
> There must be a reason to believe what is sensible and what is not.
> Looks like this RFC has helped in judging that.
>
> Regards,
> PrasannaKumar

Would you be interested to work on patch set that would remove the
existing tpm rng driver and make the TPM driver the customer? It's not
that far away from the work you've been doing already.

/Jarkko

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

2017-10-24 18:40:47

by Peter Huewe

Subject: Re: [PATCH] tpm: remove chip_num parameter from in-kernel API

Am 24. Oktober 2017 20:15:12 MESZ schrieb Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/[email protected]>:
>On Tue, Oct 24, 2017 at 10:02:00AM -0700, Dmitry Torokhov wrote:
>> On Tue, Oct 24, 2017 at 9:11 AM, Jason Gunthorpe
>> <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> wrote:
>> > On Tue, Oct 24, 2017 at 09:37:33PM +0530, PrasannaKumar
>Muralidharan wrote:
>> >> Hi Jason,
>> >>
>> >> On 24 October 2017 at 21:25, Jason Gunthorpe
>> >> <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> wrote:
>> >> > On Tue, Oct 24, 2017 at 09:21:15PM +0530, PrasannaKumar
>Muralidharan wrote:
>> >> >
>> >> >> Please check the RFC [1]. It does use chip id. The rfc has
>issues and
>> >> >> has to be fixed but still there could be users of the API.
>> >> >>
>> >> >> 1. https://www.spinics.net/lists/linux-crypto/msg28282.html
>> >> >
>> >> > That patch isn't safe at all. You need to store a kref to th
>chip in
>> >> > the hwrng, not parse a string.
>> >>
>> >> The drivers/char/hw_random/tpm-rng.c module does not store the
>chip
>> >> reference so I guess the usage is safe.
>> >
>> > It is using the default TPM, it is always safe to use the default
>tpm.
>>
>> tpm-rng is abomination that should be kicked out as soon as possible.
>> It wrecks havoc with the power management (TPM chip drivers may go
>> into suspend state, but tpm_rng does not do any power management and
>> happily forwards requests to suspended hardware) and may be available
>> when there is no TPM at all yet (the drivers have not been probed
>yet,
>> or have gotten a deferral, etc).
>>
>> TPM core should register HWRNGs when chips are ready.
>>
>> Thanks.
>>
>> --
>> Dmitry
>
>I'm fine to review a two patch set where:
>
>1. Patch 1 removes the existing TPM rng driver
>2. Patch 2 makes the TPM driver as rng producer

Yes, but tpm must be kept a hwrng source.
This is imho an important use case.

>
>Unrelate to patch that I'm proposing now but this sounds sensible.
>
>/Jarkko

--
Sent from my mobile

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

2017-10-24 18:52:08

by Jason Gunthorpe

Subject: Re: [PATCH] tpm: remove chip_num parameter from in-kernel API

On Mon, Oct 23, 2017 at 02:38:14PM +0200, Jarkko Sakkinen wrote:
> The reasoning is simple and obvious. Since every call site passes the
> value TPM_ANY_NUM (0xFFFF) the parameter does not have right to exist.
> Refined the documentation of the corresponding functions.

I like this patch, but how about a slightly different take, make this
change instead:

-struct tpm_chip *tpm_chip_find_get(int chip_num)
+struct tpm_chip *tpm_chip_find_get(struct tpm_chip *chip);

Where chip == NULL means the default TPM.

And then at all call sites swap TPM_ANY_NUM to NULL and instead of
flowing an 'int chip_num' to tpm_chip_find_get, flow the 'struct
tpm_chip *' directly.

This gets us much closer to the desired API with about the same amount
of churn as this patch has.

Jason

2017-10-24 22:27:47

by Jarkko Sakkinen

Subject: Re: [PATCH] tpm: remove chip_num parameter from in-kernel API

On Tue, Oct 24, 2017 at 12:52:08PM -0600, Jason Gunthorpe wrote:
> On Mon, Oct 23, 2017 at 02:38:14PM +0200, Jarkko Sakkinen wrote:
> > The reasoning is simple and obvious. Since every call site passes the
> > value TPM_ANY_NUM (0xFFFF) the parameter does not have right to exist.
> > Refined the documentation of the corresponding functions.
>
> I like this patch, but how about a slightly different take, make this
> change instead:
>
> -struct tpm_chip *tpm_chip_find_get(int chip_num)
> +struct tpm_chip *tpm_chip_find_get(struct tpm_chip *chip);
>
> Where chip == NULL means the default TPM.
>
> And then at all call sites swap TPM_ANY_NUM to NULL and instead of
> flowing an 'int chip_num' to tpm_chip_find_get, flow the 'struct
> tpm_chip *' directly.
>
> This gets us much closer to the desired API with about the same amount
> of churn as this patch has.
>
> Jason

I can do that. I will rework this as a patch set that includes this
proposal. Gives us more clean position to continue. Thank you.

/Jarkko

2017-10-25 14:51:16

by PrasannaKumar Muralidharan

Subject: Re: [PATCH] tpm: remove chip_num parameter from in-kernel API

Hi Jarkko,

On 24 October 2017 at 23:52, Jarkko Sakkinen
<jarkko.sakkinen-VuQAYsv1563Yd54FQh9/[email protected]> wrote:
> On Tue, Oct 24, 2017 at 10:05:20PM +0530, PrasannaKumar Muralidharan wrote:
>> > 1. Every user in the kernel is using TPM_ANY_NUM, which means there are
>> > no other users.
>>
>> Completely agree that there is no in kernel users yet.
>
> And should never be. It's a bogus parameter that makes no sense.

I understood that after seeing latest patch that uses struct tpm_chip.
Sorry for the noise.

>> > 2. Moving struct tpm_rng to the TPM client is architecturally
>> > uacceptable.
>>
>> As there was no response to the patch there is no way to know whether
>> it is acceptable or not.
>
> I like the idea of removing the tpm rng driver as discussed in other
> emails in this thread.

Thank you.

>> > 3. Using zero deos not give you any better guarantees on anything than
>> > just using TPM_ANY_NUM.
>>
>> Chip id is used, not zero.
>
> Sorry I misread the patch first time. Anyway it's not any kind of ID to
> be trusted.

Okay.

>> > Why this patch is not CC'd to linux-integrity? It modifies the TPM
>> > driver. And in the worst way.
>>
>> TPM list is moderated and the moderator has not approved it yet.
>> get_maintainer script did not say about linux-integrity mailing list.
>>
>> It could be doing things in worst way but it is not known until some
>> one says. If no one tells it is the case I don't think it is possible
>> to fix. Which is what happened.
>
> Understood. We've moved to linux-integrity-u79uwXL29TaiAVqoAR/[email protected] MAINTAINERS
> update is in the queue for the next kernel release.

Sorry I never knew this.

>> > Implementing the ideas that Jason explained is the senseful way to
>> > get stable access. modules.dep makes sure that the modules are loaded
>> > in the correct order.
>>
>> If that is sensible then it is the way to go.
>>
>> There must be a reason to believe what is sensible and what is not.
>> Looks like this RFC has helped in judging that.
>>
>> Regards,
>> PrasannaKumar
>
> Would you be interested to work on patch set that would remove the
> existing tpm rng driver and make the TPM driver the customer? It's not
> that far away from the work you've been doing already.
>
> /Jarkko

I am late to the party. Jason has sent a patch doing that by the time
I read this email.

Thanks and regards,
PrasannaKumar

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

2017-10-25 19:11:23

by Jarkko Sakkinen

Subject: Re: [tpmdd-devel] [PATCH] tpm: remove chip_num parameter from in-kernel API

On Wed, Oct 25, 2017 at 08:21:16PM +0530, PrasannaKumar Muralidharan wrote:
> >> > 2. Moving struct tpm_rng to the TPM client is architecturally
> >> > uacceptable.
> >>
> >> As there was no response to the patch there is no way to know whether
> >> it is acceptable or not.
> >
> > I like the idea of removing the tpm rng driver as discussed in other
> > emails in this thread.
>
> Thank you.

No, thank you.

I didn't first understand the big idea and only looked at the code
change per se. I apologize for that.

The problem that you went to solve was real and it led to a properly
implemented solution. You were not late from the party. Jason's code
change is derivative work of your code change. That's why his code
change has also your signed-off-by.

Thanks for doing awesome work :-)

/Jarkko

2017-10-26 16:23:30

by PrasannaKumar Muralidharan

Subject: Re: [tpmdd-devel] [PATCH] tpm: remove chip_num parameter from in-kernel API

On 26 October 2017 at 00:41, Jarkko Sakkinen
<[email protected]> wrote:
> On Wed, Oct 25, 2017 at 08:21:16PM +0530, PrasannaKumar Muralidharan wrote:
>> >> > 2. Moving struct tpm_rng to the TPM client is architecturally
>> >> > uacceptable.
>> >>
>> >> As there was no response to the patch there is no way to know whether
>> >> it is acceptable or not.
>> >
>> > I like the idea of removing the tpm rng driver as discussed in other
>> > emails in this thread.
>>
>> Thank you.
>
> No, thank you.
>
> I didn't first understand the big idea and only looked at the code
> change per se. I apologize for that.

No need for that. I missed mentioning the reason for the patch and it
is not obvious from code change. Its my fault.

> The problem that you went to solve was real and it led to a properly
> implemented solution. You were not late from the party. Jason's code
> change is derivative work of your code change. That's why his code
> change has also your signed-off-by.
>
> Thanks for doing awesome work :-)

Its really nice to hear such words :-) :-D.

>
> /Jarkko

Thanks and regards,
PrasannaKumar