This series of patches converts IMA's usage of the tpm_chip to find a TPM
chip initially and use it until the machine is shut down. To do this we need
to introduce a kref for the tpm_chip that IMA and all other users of a
tpm_chip hold onto until they don't need the TPM chip anymore.
Stefan
v2->v3:
- renaming tpm_chip_find_get() to tpm_get_ops()
- IMA does not lock access to ima_tpm_chip anymore
- IMA does not have a reboot notifier to release the chip anymore
v1->v2:
- use the kref of the device via get_device()/put_device()
Stefan Berger (4):
tpm: rename tpm_chip_find_get() to tpm_get_ops()
tpm: Implement tpm_chip_find() for other subsystems to find a TPM chip
ima: Use tpm_chip_find() and call TPM functions with a tpm_chip
ima: Get rid of ima_used_chip and use ima_tpm_chip != NULL instead
drivers/char/tpm/tpm-chip.c | 31 +++++++++++++++++++++++++++++--
drivers/char/tpm/tpm-interface.c | 14 +++++++-------
drivers/char/tpm/tpm.h | 2 +-
include/linux/tpm.h | 5 +++++
security/integrity/ima/ima.h | 2 +-
security/integrity/ima/ima_crypto.c | 4 ++--
security/integrity/ima/ima_init.c | 16 +++++-----------
security/integrity/ima/ima_queue.c | 4 ++--
8 files changed, 52 insertions(+), 26 deletions(-)
--
2.13.6
Get rid of ima_used_chip and use ima_tpm_chip variable instead for
determining whether to use the TPM chip.
Signed-off-by: Stefan Berger <[email protected]>
---
security/integrity/ima/ima.h | 1 -
security/integrity/ima/ima_crypto.c | 2 +-
security/integrity/ima/ima_init.c | 7 ++-----
security/integrity/ima/ima_queue.c | 2 +-
4 files changed, 4 insertions(+), 8 deletions(-)
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 35409461a3f2..2ab1affffa36 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -53,7 +53,6 @@ enum tpm_pcrs { TPM_PCR0 = 0, TPM_PCR8 = 8 };
extern int ima_policy_flag;
/* set during initialization */
-extern int ima_used_chip;
extern int ima_hash_algo;
extern int ima_appraise;
extern struct tpm_chip *ima_tpm_chip;
diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
index 88082f35adb2..7e7e7e7c250a 100644
--- a/security/integrity/ima/ima_crypto.c
+++ b/security/integrity/ima/ima_crypto.c
@@ -631,7 +631,7 @@ int ima_calc_buffer_hash(const void *buf, loff_t len,
static void __init ima_pcrread(int idx, u8 *pcr)
{
- if (!ima_used_chip)
+ if (!ima_tpm_chip)
return;
if (tpm_pcr_read(ima_tpm_chip, idx, pcr) != 0)
diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c
index ae3a853e0049..4c63e1e030f5 100644
--- a/security/integrity/ima/ima_init.c
+++ b/security/integrity/ima/ima_init.c
@@ -26,7 +26,6 @@
/* name for boot aggregate entry */
static const char *boot_aggregate_name = "boot_aggregate";
-int ima_used_chip;
struct tpm_chip *ima_tpm_chip;
/* Add the boot aggregate to the IMA measurement list and extend
@@ -65,7 +64,7 @@ static int __init ima_add_boot_aggregate(void)
iint->ima_hash->algo = HASH_ALGO_SHA1;
iint->ima_hash->length = SHA1_DIGEST_SIZE;
- if (ima_used_chip) {
+ if (ima_tpm_chip) {
result = ima_calc_boot_aggregate(&hash.hdr);
if (result < 0) {
audit_cause = "hashing_error";
@@ -110,9 +109,7 @@ int __init ima_init(void)
int rc;
ima_tpm_chip = tpm_chip_find();
-
- ima_used_chip = (ima_tpm_chip != NULL);
- if (!ima_used_chip)
+ if (!ima_tpm_chip)
pr_info("No TPM chip found, activating TPM-bypass!\n");
rc = integrity_init_keyring(INTEGRITY_KEYRING_IMA);
diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c
index c6303fa19a49..b186819bd5aa 100644
--- a/security/integrity/ima/ima_queue.c
+++ b/security/integrity/ima/ima_queue.c
@@ -142,7 +142,7 @@ static int ima_pcr_extend(const u8 *hash, int pcr)
{
int result = 0;
- if (!ima_used_chip)
+ if (!ima_tpm_chip)
return result;
result = tpm_pcr_extend(ima_tpm_chip, pcr, hash);
--
2.13.6
Implement tpm_chip_find() for other subsystems to find a TPM chip and
get a reference to that chip.
Signed-off-by: Stefan Berger <[email protected]>
---
drivers/char/tpm/tpm-chip.c | 27 +++++++++++++++++++++++++++
include/linux/tpm.h | 5 +++++
2 files changed, 32 insertions(+)
diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 4e83695af068..2520555b1e17 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -81,6 +81,33 @@ void tpm_put_ops(struct tpm_chip *chip)
EXPORT_SYMBOL_GPL(tpm_put_ops);
/**
+ * tpm_chip_find() - find a TPM chip and get a reference to it
+ */
+struct tpm_chip *tpm_chip_find(void)
+{
+ struct tpm_chip *chip, *res = NULL;
+ int chip_num = 0;
+ int chip_prev;
+
+ mutex_lock(&idr_lock);
+
+ do {
+ chip_prev = chip_num;
+ chip = idr_get_next(&dev_nums_idr, &chip_num);
+ if (chip) {
+ get_device(&chip->dev);
+ res = chip;
+ break;
+ }
+ } while (chip_prev != chip_num);
+
+ mutex_unlock(&idr_lock);
+
+ return res;
+}
+EXPORT_SYMBOL_GPL(tpm_chip_find);
+
+/**
* tpm_get_ops() - find and reserve a TPM chip
* @chip: a &struct tpm_chip instance, %NULL for the default chip
*
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index 06639fb6ab85..a9c65f5670b6 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -61,6 +61,7 @@ extern int tpm_seal_trusted(struct tpm_chip *chip,
extern int tpm_unseal_trusted(struct tpm_chip *chip,
struct trusted_key_payload *payload,
struct trusted_key_options *options);
+extern struct tpm_chip *tpm_chip_find(void);
#else
static inline int tpm_is_tpm2(struct tpm_chip *chip)
{
@@ -96,5 +97,9 @@ static inline int tpm_unseal_trusted(struct tpm_chip *chip,
{
return -ENODEV;
}
+static inline struct tpm_chip *tpm_chip_find(void)
+{
+ return NULL;
+}
#endif
#endif
--
2.13.6
Rather than accessing the TPM functions by passing a NULL pointer for
the tpm_chip, which causes a lookup for a suitable chip every time, get a
hold of a tpm_chip and access the TPM functions using it.
Signed-off-by: Stefan Berger <[email protected]>
---
security/integrity/ima/ima.h | 1 +
security/integrity/ima/ima_crypto.c | 2 +-
security/integrity/ima/ima_init.c | 11 ++++-------
security/integrity/ima/ima_queue.c | 2 +-
4 files changed, 7 insertions(+), 9 deletions(-)
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 354bb5716ce3..35409461a3f2 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -56,6 +56,7 @@ extern int ima_policy_flag;
extern int ima_used_chip;
extern int ima_hash_algo;
extern int ima_appraise;
+extern struct tpm_chip *ima_tpm_chip;
/* IMA event related data */
struct ima_event_data {
diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
index 4e085a17124f..88082f35adb2 100644
--- a/security/integrity/ima/ima_crypto.c
+++ b/security/integrity/ima/ima_crypto.c
@@ -634,7 +634,7 @@ static void __init ima_pcrread(int idx, u8 *pcr)
if (!ima_used_chip)
return;
- if (tpm_pcr_read(NULL, idx, pcr) != 0)
+ if (tpm_pcr_read(ima_tpm_chip, 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 29b72cd2502e..ae3a853e0049 100644
--- a/security/integrity/ima/ima_init.c
+++ b/security/integrity/ima/ima_init.c
@@ -27,6 +27,7 @@
/* name for boot aggregate entry */
static const char *boot_aggregate_name = "boot_aggregate";
int ima_used_chip;
+struct tpm_chip *ima_tpm_chip;
/* Add the boot aggregate to the IMA measurement list and extend
* the PCR register.
@@ -106,17 +107,13 @@ void __init ima_load_x509(void)
int __init ima_init(void)
{
- u8 pcr_i[TPM_DIGEST_SIZE];
int rc;
- ima_used_chip = 0;
- rc = tpm_pcr_read(NULL, 0, pcr_i);
- if (rc == 0)
- ima_used_chip = 1;
+ ima_tpm_chip = tpm_chip_find();
+ ima_used_chip = (ima_tpm_chip != NULL);
if (!ima_used_chip)
- pr_info("No TPM chip found, activating TPM-bypass! (rc=%d)\n",
- rc);
+ pr_info("No TPM chip found, activating TPM-bypass!\n");
rc = integrity_init_keyring(INTEGRITY_KEYRING_IMA);
if (rc)
diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c
index 418f35e38015..c6303fa19a49 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(NULL, pcr, hash);
+ result = tpm_pcr_extend(ima_tpm_chip, pcr, hash);
if (result != 0)
pr_err("Error Communicating to TPM chip, result: %d\n", result);
return result;
--
2.13.6
Rename tpm_chip_find_get() to tpm_get_ops(). This now matches
the name of its counter part tpm_put_ops() better.
Signed-off-by: Stefan Berger <[email protected]>
---
drivers/char/tpm/tpm-chip.c | 4 ++--
drivers/char/tpm/tpm-interface.c | 14 +++++++-------
drivers/char/tpm/tpm.h | 2 +-
3 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 0a62c19937b6..4e83695af068 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -81,7 +81,7 @@ void tpm_put_ops(struct tpm_chip *chip)
EXPORT_SYMBOL_GPL(tpm_put_ops);
/**
- * tpm_chip_find_get() - find and reserve a TPM chip
+ * tpm_get_ops() - find and reserve a TPM chip
* @chip: a &struct tpm_chip instance, %NULL for the default chip
*
* Finds a TPM chip and reserves its class device and operations. The chip must
@@ -92,7 +92,7 @@ EXPORT_SYMBOL_GPL(tpm_put_ops);
* %NULL if a chip is not found.
* %NULL if the chip is not available.
*/
-struct tpm_chip *tpm_chip_find_get(struct tpm_chip *chip)
+struct tpm_chip *tpm_get_ops(struct tpm_chip *chip)
{
struct tpm_chip *res = NULL;
int chip_num = 0;
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index c43a9e28995e..fde731df838b 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -929,7 +929,7 @@ int tpm_is_tpm2(struct tpm_chip *chip)
{
int rc;
- chip = tpm_chip_find_get(chip);
+ chip = tpm_get_ops(chip);
if (!chip)
return -ENODEV;
@@ -953,7 +953,7 @@ int tpm_pcr_read(struct tpm_chip *chip, int pcr_idx, u8 *res_buf)
{
int rc;
- chip = tpm_chip_find_get(chip);
+ chip = tpm_get_ops(chip);
if (!chip)
return -ENODEV;
if (chip->flags & TPM_CHIP_FLAG_TPM2)
@@ -1012,7 +1012,7 @@ int tpm_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash)
u32 count = 0;
int i;
- chip = tpm_chip_find_get(chip);
+ chip = tpm_get_ops(chip);
if (!chip)
return -ENODEV;
@@ -1141,7 +1141,7 @@ int tpm_send(struct tpm_chip *chip, void *cmd, size_t buflen)
{
int rc;
- chip = tpm_chip_find_get(chip);
+ chip = tpm_get_ops(chip);
if (!chip)
return -ENODEV;
@@ -1261,7 +1261,7 @@ int tpm_get_random(struct tpm_chip *chip, u8 *out, size_t max)
if (!out || !num_bytes || max > TPM_MAX_RNG_DATA)
return -EINVAL;
- chip = tpm_chip_find_get(chip);
+ chip = tpm_get_ops(chip);
if (!chip)
return -ENODEV;
@@ -1323,7 +1323,7 @@ int tpm_seal_trusted(struct tpm_chip *chip, struct trusted_key_payload *payload,
{
int rc;
- chip = tpm_chip_find_get(chip);
+ chip = tpm_get_ops(chip);
if (!chip || !(chip->flags & TPM_CHIP_FLAG_TPM2))
return -ENODEV;
@@ -1351,7 +1351,7 @@ int tpm_unseal_trusted(struct tpm_chip *chip,
{
int rc;
- chip = tpm_chip_find_get(chip);
+ chip = tpm_get_ops(chip);
if (!chip || !(chip->flags & TPM_CHIP_FLAG_TPM2))
return -ENODEV;
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 7f2d0f489e9c..9d2ad67778b5 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -535,7 +535,7 @@ static inline void tpm_msleep(unsigned int delay_msec)
delay_msec * 1000);
};
-struct tpm_chip *tpm_chip_find_get(struct tpm_chip *chip);
+struct tpm_chip *tpm_get_ops(struct tpm_chip *chip);
__must_check int tpm_try_get_ops(struct tpm_chip *chip);
void tpm_put_ops(struct tpm_chip *chip);
--
2.13.6
On Fri, Jun 22, 2018 at 12:46:10PM -0400, Stefan Berger wrote:
> Rename tpm_chip_find_get() to tpm_get_ops(). This now matches
> the name of its counter part tpm_put_ops() better.
>
> Signed-off-by: Stefan Berger <[email protected]>
> drivers/char/tpm/tpm-chip.c | 4 ++--
> drivers/char/tpm/tpm-interface.c | 14 +++++++-------
> drivers/char/tpm/tpm.h | 2 +-
> 3 files changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index 0a62c19937b6..4e83695af068 100644
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -81,7 +81,7 @@ void tpm_put_ops(struct tpm_chip *chip)
> EXPORT_SYMBOL_GPL(tpm_put_ops);
>
> /**
> - * tpm_chip_find_get() - find and reserve a TPM chip
> + * tpm_get_ops() - find and reserve a TPM chip
> * @chip: a &struct tpm_chip instance, %NULL for the default chip
> *
> * Finds a TPM chip and reserves its class device and operations. The chip must
> @@ -92,7 +92,7 @@ EXPORT_SYMBOL_GPL(tpm_put_ops);
> * %NULL if a chip is not found.
> * %NULL if the chip is not available.
> */
> -struct tpm_chip *tpm_chip_find_get(struct tpm_chip *chip)
> +struct tpm_chip *tpm_get_ops(struct tpm_chip *chip)
> {
It is sort of weird that this returns a tpm_chip, but I think if you
revise the comment it would be OK.
This function is for internal use only, it supports existing TPM
callers by accepting NULL, but those callers should be converted to
pass in a chip directly.
Jason
On Fri, Jun 22, 2018 at 12:46:11PM -0400, Stefan Berger wrote:
> Implement tpm_chip_find() for other subsystems to find a TPM chip and
> get a reference to that chip.
>
> Signed-off-by: Stefan Berger <[email protected]>
> drivers/char/tpm/tpm-chip.c | 27 +++++++++++++++++++++++++++
> include/linux/tpm.h | 5 +++++
> 2 files changed, 32 insertions(+)
>
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index 4e83695af068..2520555b1e17 100644
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -81,6 +81,33 @@ void tpm_put_ops(struct tpm_chip *chip)
> EXPORT_SYMBOL_GPL(tpm_put_ops);
>
> /**
> + * tpm_chip_find() - find a TPM chip and get a reference to it
> + */
> +struct tpm_chip *tpm_chip_find(void)
I feel like this should be called 'tpm_default_chip()'
> +{
> + struct tpm_chip *chip, *res = NULL;
> + int chip_num = 0;
> + int chip_prev;
> +
> + mutex_lock(&idr_lock);
> +
> + do {
> + chip_prev = chip_num;
> + chip = idr_get_next(&dev_nums_idr, &chip_num);
> + if (chip) {
> + get_device(&chip->dev);
> + res = chip;
> + break;
> + }
> + } while (chip_prev != chip_num);
> +
> + mutex_unlock(&idr_lock);
And what was tpm_chip_find_get should just call this function..
Jason
On 06/22/2018 04:43 PM, Jason Gunthorpe wrote:
> On Fri, Jun 22, 2018 at 12:46:11PM -0400, Stefan Berger wrote:
>> Implement tpm_chip_find() for other subsystems to find a TPM chip and
>> get a reference to that chip.
>>
>> Signed-off-by: Stefan Berger <[email protected]>
>> drivers/char/tpm/tpm-chip.c | 27 +++++++++++++++++++++++++++
>> include/linux/tpm.h | 5 +++++
>> 2 files changed, 32 insertions(+)
>>
>> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
>> index 4e83695af068..2520555b1e17 100644
>> +++ b/drivers/char/tpm/tpm-chip.c
>> @@ -81,6 +81,33 @@ void tpm_put_ops(struct tpm_chip *chip)
>> EXPORT_SYMBOL_GPL(tpm_put_ops);
>>
>> /**
>> + * tpm_chip_find() - find a TPM chip and get a reference to it
>> + */
>> +struct tpm_chip *tpm_chip_find(void)
> I feel like this should be called 'tpm_default_chip()'
>
>> +{
>> + struct tpm_chip *chip, *res = NULL;
>> + int chip_num = 0;
>> + int chip_prev;
>> +
>> + mutex_lock(&idr_lock);
>> +
>> + do {
>> + chip_prev = chip_num;
>> + chip = idr_get_next(&dev_nums_idr, &chip_num);
>> + if (chip) {
>> + get_device(&chip->dev);
>> + res = chip;
>> + break;
>> + }
>> + } while (chip_prev != chip_num);
>> +
>> + mutex_unlock(&idr_lock);
> And what was tpm_chip_find_get should just call this function..
And then after that each time tpm_get_ops() ?
>
> Jason
>
On Fri, Jun 22, 2018 at 04:45:47PM -0400, Stefan Berger wrote:
> On 06/22/2018 04:43 PM, Jason Gunthorpe wrote:
> >On Fri, Jun 22, 2018 at 12:46:11PM -0400, Stefan Berger wrote:
> >>Implement tpm_chip_find() for other subsystems to find a TPM chip and
> >>get a reference to that chip.
> >>
> >>Signed-off-by: Stefan Berger <[email protected]>
> >> drivers/char/tpm/tpm-chip.c | 27 +++++++++++++++++++++++++++
> >> include/linux/tpm.h | 5 +++++
> >> 2 files changed, 32 insertions(+)
> >>
> >>diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> >>index 4e83695af068..2520555b1e17 100644
> >>+++ b/drivers/char/tpm/tpm-chip.c
> >>@@ -81,6 +81,33 @@ void tpm_put_ops(struct tpm_chip *chip)
> >> EXPORT_SYMBOL_GPL(tpm_put_ops);
> >> /**
> >>+ * tpm_chip_find() - find a TPM chip and get a reference to it
> >>+ */
> >>+struct tpm_chip *tpm_chip_find(void)
> >I feel like this should be called 'tpm_default_chip()'
> >
> >>+{
> >>+ struct tpm_chip *chip, *res = NULL;
> >>+ int chip_num = 0;
> >>+ int chip_prev;
> >>+
> >>+ mutex_lock(&idr_lock);
> >>+
> >>+ do {
> >>+ chip_prev = chip_num;
> >>+ chip = idr_get_next(&dev_nums_idr, &chip_num);
> >>+ if (chip) {
> >>+ get_device(&chip->dev);
> >>+ res = chip;
> >>+ break;
> >>+ }
> >>+ } while (chip_prev != chip_num);
> >>+
> >>+ mutex_unlock(&idr_lock);
> >And what was tpm_chip_find_get should just call this function..
> And then after that each time tpm_get_ops() ?
It is best to keep the tpm_chip_find_get/tpm_put_ops for internal use
as it also manages the kref lifetime of chip in a subtle way, it
relies on the ops lock not the kref to keep the memory valid when it
has a NULL parameter..
Something like this:
struct tpm_chip *tpm_find_get_ops(struct tpm_chip *chip)
{
int rc;
if (chip) {
if (!tpm_try_get_ops(chip))
return NULL;
return chip;
}
chip = tpm_default_chip();
rc = tpm_try_get_ops(chip));
put_device(&chip->dev);
if (rc)
return NULL;
return chip;
}
Jason