All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 0/4] Have IMA find and use a tpm_chip until system shutdown
@ 2018-06-20 20:42 Stefan Berger
  2018-06-20 20:42 ` [PATCH v2 1/4] tpm: Implement tpm_chip_find() and tpm_chip_put() for other subsystems Stefan Berger
                   ` (3 more replies)
  0 siblings, 4 replies; 30+ messages in thread
From: Stefan Berger @ 2018-06-20 20:42 UTC (permalink / raw)
  To: linux-integrity, jarkko.sakkinen, zohar; +Cc: jgg, linux-kernel, Stefan Berger

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

v1->v2:
  - use the kref of the device via get_device()/put_device()

Stefan Berger (4):
  tpm: Implement tpm_chip_find() and tpm_chip_put() for other subsystems
  ima: Implement ima_shutdown and register it as a reboot_notifier
  ima: Use tpm_chip_find() and access TPM functions using it
  ima: Get rid of ima_used_chip and use ima_tpm_chip != NULL instead

 drivers/char/tpm/tpm-chip.c         | 37 +++++++++++++++++++++++++++++++++++++
 include/linux/tpm.h                 |  9 +++++++++
 security/integrity/ima/ima.h        |  4 +++-
 security/integrity/ima/ima_crypto.c | 14 +++++++++++---
 security/integrity/ima/ima_init.c   | 36 ++++++++++++++++++++++++++----------
 security/integrity/ima/ima_queue.c  |  9 ++++++---
 6 files changed, 92 insertions(+), 17 deletions(-)

-- 
2.13.6


^ permalink raw reply	[flat|nested] 30+ messages in thread
* [PATCH v2 1/4] tpm: Implement tpm_chip_find() and tpm_chip_put() for other subsystems
  2018-06-20 20:42 [PATCH v2 0/4] Have IMA find and use a tpm_chip until system shutdown Stefan Berger
@ 2018-06-20 20:42 ` Stefan Berger
  2018-06-20 20:50   ` Jason Gunthorpe
  2018-06-21 17:15   ` Jarkko Sakkinen
  2018-06-20 20:42 ` [PATCH v2 2/4] ima: Implement ima_shutdown and register it as a reboot_notifier Stefan Berger
                   ` (2 subsequent siblings)
  3 siblings, 2 replies; 30+ messages in thread
From: Stefan Berger @ 2018-06-20 20:42 UTC (permalink / raw)
  To: linux-integrity, jarkko.sakkinen, zohar; +Cc: jgg, linux-kernel, Stefan Berger

Implement tpm_chip_find() for other subsystems to find a TPM chip and
get a reference to that chip. Once done with using the chip, the reference
is released using tpm_chip_put().

Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
---
 drivers/char/tpm/tpm-chip.c | 37 +++++++++++++++++++++++++++++++++++++
 include/linux/tpm.h         |  9 +++++++++
 2 files changed, 46 insertions(+)

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 0a62c19937b6..eaaf41ce73d8 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -81,6 +81,43 @@ void tpm_put_ops(struct tpm_chip *chip)
 EXPORT_SYMBOL_GPL(tpm_put_ops);
 
 /**
+ * tpm_chip_put() - Release a ref to the tpm_chip
+ * @chip: Chip to put
+ */
+void tpm_chip_put(struct tpm_chip *chip)
+{
+	if (chip)
+		put_device(&chip->dev);
+}
+
+/**
+ * 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_chip_find_get() - 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..f0d1abcaf4af 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -61,6 +61,8 @@ 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);
+extern void tpm_chip_put(struct tpm_chip *chip);
 #else
 static inline int tpm_is_tpm2(struct tpm_chip *chip)
 {
@@ -96,5 +98,12 @@ static inline int tpm_unseal_trusted(struct tpm_chip *chip,
 {
 	return -ENODEV;
 }
+static inline struct tpm_chip *tpm_chip_find(void)
+{
+	return NULL;
+}
+static inline void tpm_chip_put(struct tpm_chip *chip)
+{
+}
 #endif
 #endif
-- 
2.13.6


^ permalink raw reply related	[flat|nested] 30+ messages in thread
* [PATCH v2 2/4] ima: Implement ima_shutdown and register it as a reboot_notifier
  2018-06-20 20:42 [PATCH v2 0/4] Have IMA find and use a tpm_chip until system shutdown Stefan Berger
  2018-06-20 20:42 ` [PATCH v2 1/4] tpm: Implement tpm_chip_find() and tpm_chip_put() for other subsystems Stefan Berger
@ 2018-06-20 20:42 ` Stefan Berger
  2018-06-20 20:42 ` [PATCH v2 3/4] ima: Use tpm_chip_find() and access TPM functions using it Stefan Berger
  2018-06-20 20:42 ` [PATCH v2 4/4] ima: Get rid of ima_used_chip and use ima_tpm_chip != NULL instead Stefan Berger
  3 siblings, 0 replies; 30+ messages in thread
From: Stefan Berger @ 2018-06-20 20:42 UTC (permalink / raw)
  To: linux-integrity, jarkko.sakkinen, zohar; +Cc: jgg, linux-kernel, Stefan Berger

Implement ima_shutdown so that we can release the tpm_chip before
devices are shut down. Register it as a low-priority reboot_notifier.

Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
---
 security/integrity/ima/ima_init.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c
index 29b72cd2502e..8a5258eb32b6 100644
--- a/security/integrity/ima/ima_init.c
+++ b/security/integrity/ima/ima_init.c
@@ -21,6 +21,7 @@
 #include <linux/scatterlist.h>
 #include <linux/slab.h>
 #include <linux/err.h>
+#include <linux/reboot.h>
 
 #include "ima.h"
 
@@ -104,11 +105,24 @@ void __init ima_load_x509(void)
 }
 #endif
 
+static int ima_shutdown(struct notifier_block *this, unsigned long action,
+			void *data)
+{
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block ima_reboot_notifier = {
+	.notifier_call = ima_shutdown,
+	.priority = 0,
+};
+
 int __init ima_init(void)
 {
 	u8 pcr_i[TPM_DIGEST_SIZE];
 	int rc;
 
+	register_reboot_notifier(&ima_reboot_notifier);
+
 	ima_used_chip = 0;
 	rc = tpm_pcr_read(NULL, 0, pcr_i);
 	if (rc == 0)
-- 
2.13.6


^ permalink raw reply related	[flat|nested] 30+ messages in thread
* [PATCH v2 3/4] ima: Use tpm_chip_find() and access TPM functions using it
  2018-06-20 20:42 [PATCH v2 0/4] Have IMA find and use a tpm_chip until system shutdown Stefan Berger
  2018-06-20 20:42 ` [PATCH v2 1/4] tpm: Implement tpm_chip_find() and tpm_chip_put() for other subsystems Stefan Berger
  2018-06-20 20:42 ` [PATCH v2 2/4] ima: Implement ima_shutdown and register it as a reboot_notifier Stefan Berger
@ 2018-06-20 20:42 ` Stefan Berger
  2018-06-21 20:53     ` Mimi Zohar
  2018-06-20 20:42 ` [PATCH v2 4/4] ima: Get rid of ima_used_chip and use ima_tpm_chip != NULL instead Stefan Berger
  3 siblings, 1 reply; 30+ messages in thread
From: Stefan Berger @ 2018-06-20 20:42 UTC (permalink / raw)
  To: linux-integrity, jarkko.sakkinen, zohar; +Cc: jgg, linux-kernel, Stefan Berger

Rather than accessing the TPM functions using a NULL pointer, which
causes a lookup for a suitable chip every time, get a hold of a tpm_chip
and access the TPM functions using this chip. We call the tpm_chip
ima_tpm_chip and protect it, once initialization is done, using a
rw_semaphore called ima_tpm_chip_lock.

Use ima_shutdown to release the tpm_chip.

Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
---
 security/integrity/ima/ima.h        |  3 +++
 security/integrity/ima/ima_crypto.c | 12 ++++++++++--
 security/integrity/ima/ima_init.c   | 19 ++++++++++++-------
 security/integrity/ima/ima_queue.c  |  7 +++++--
 4 files changed, 30 insertions(+), 11 deletions(-)

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 354bb5716ce3..53a88d578ca5 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -24,6 +24,7 @@
 #include <linux/hash.h>
 #include <linux/tpm.h>
 #include <linux/audit.h>
+#include <linux/rwsem.h>
 #include <crypto/hash_info.h>
 
 #include "../integrity.h"
@@ -56,6 +57,8 @@ extern int ima_policy_flag;
 extern int ima_used_chip;
 extern int ima_hash_algo;
 extern int ima_appraise;
+extern struct rw_semaphore ima_tpm_chip_lock;
+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..da7715240476 100644
--- a/security/integrity/ima/ima_crypto.c
+++ b/security/integrity/ima/ima_crypto.c
@@ -631,10 +631,18 @@ int ima_calc_buffer_hash(const void *buf, loff_t len,
 
 static void __init ima_pcrread(int idx, u8 *pcr)
 {
+	int result = 0;
+
+	down_read(&ima_tpm_chip_lock);
+
 	if (!ima_used_chip)
-		return;
+		goto out;
+
+	result = tpm_pcr_read(ima_tpm_chip, idx, pcr);
+out:
+	up_read(&ima_tpm_chip_lock);
 
-	if (tpm_pcr_read(NULL, idx, pcr) != 0)
+	if (result != 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 8a5258eb32b6..24db06c4f463 100644
--- a/security/integrity/ima/ima_init.c
+++ b/security/integrity/ima/ima_init.c
@@ -28,6 +28,8 @@
 /* name for boot aggregate entry */
 static const char *boot_aggregate_name = "boot_aggregate";
 int ima_used_chip;
+struct rw_semaphore ima_tpm_chip_lock = __RWSEM_INITIALIZER(ima_tpm_chip_lock);
+struct tpm_chip *ima_tpm_chip;
 
 /* Add the boot aggregate to the IMA measurement list and extend
  * the PCR register.
@@ -108,6 +110,13 @@ void __init ima_load_x509(void)
 static int ima_shutdown(struct notifier_block *this, unsigned long action,
 			void *data)
 {
+	down_write(&ima_tpm_chip_lock);
+	if (ima_tpm_chip) {
+		tpm_chip_put(ima_tpm_chip);
+		ima_tpm_chip = NULL;
+		ima_used_chip = 0;
+	}
+	up_write(&ima_tpm_chip_lock);
 	return NOTIFY_DONE;
 }
 
@@ -118,19 +127,15 @@ static struct notifier_block ima_reboot_notifier = {
 
 int __init ima_init(void)
 {
-	u8 pcr_i[TPM_DIGEST_SIZE];
 	int rc;
 
 	register_reboot_notifier(&ima_reboot_notifier);
 
-	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..6c9427939a28 100644
--- a/security/integrity/ima/ima_queue.c
+++ b/security/integrity/ima/ima_queue.c
@@ -142,10 +142,13 @@ static int ima_pcr_extend(const u8 *hash, int pcr)
 {
 	int result = 0;
 
+	down_read(&ima_tpm_chip_lock);
 	if (!ima_used_chip)
-		return result;
+		goto out;
 
-	result = tpm_pcr_extend(NULL, pcr, hash);
+	result = tpm_pcr_extend(ima_tpm_chip, pcr, hash);
+out:
+	up_read(&ima_tpm_chip_lock);
 	if (result != 0)
 		pr_err("Error Communicating to TPM chip, result: %d\n", result);
 	return result;
-- 
2.13.6


^ permalink raw reply related	[flat|nested] 30+ messages in thread
* [PATCH v2 4/4] ima: Get rid of ima_used_chip and use ima_tpm_chip != NULL instead
  2018-06-20 20:42 [PATCH v2 0/4] Have IMA find and use a tpm_chip until system shutdown Stefan Berger
                   ` (2 preceding siblings ...)
  2018-06-20 20:42 ` [PATCH v2 3/4] ima: Use tpm_chip_find() and access TPM functions using it Stefan Berger
@ 2018-06-20 20:42 ` Stefan Berger
  3 siblings, 0 replies; 30+ messages in thread
From: Stefan Berger @ 2018-06-20 20:42 UTC (permalink / raw)
  To: linux-integrity, jarkko.sakkinen, zohar; +Cc: jgg, linux-kernel, Stefan Berger

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 <stefanb@linux.vnet.ibm.com>
---
 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 53a88d578ca5..18491c0e9b97 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -54,7 +54,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 rw_semaphore ima_tpm_chip_lock;
diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
index da7715240476..bc6ad6b3243a 100644
--- a/security/integrity/ima/ima_crypto.c
+++ b/security/integrity/ima/ima_crypto.c
@@ -635,7 +635,7 @@ static void __init ima_pcrread(int idx, u8 *pcr)
 
 	down_read(&ima_tpm_chip_lock);
 
-	if (!ima_used_chip)
+	if (!ima_tpm_chip)
 		goto out;
 
 	result = tpm_pcr_read(ima_tpm_chip, idx, pcr);
diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c
index 24db06c4f463..84fba44c4415 100644
--- a/security/integrity/ima/ima_init.c
+++ b/security/integrity/ima/ima_init.c
@@ -27,7 +27,6 @@
 
 /* name for boot aggregate entry */
 static const char *boot_aggregate_name = "boot_aggregate";
-int ima_used_chip;
 struct rw_semaphore ima_tpm_chip_lock = __RWSEM_INITIALIZER(ima_tpm_chip_lock);
 struct tpm_chip *ima_tpm_chip;
 
@@ -67,7 +66,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";
@@ -114,7 +113,6 @@ static int ima_shutdown(struct notifier_block *this, unsigned long action,
 	if (ima_tpm_chip) {
 		tpm_chip_put(ima_tpm_chip);
 		ima_tpm_chip = NULL;
-		ima_used_chip = 0;
 	}
 	up_write(&ima_tpm_chip_lock);
 	return NOTIFY_DONE;
@@ -133,8 +131,7 @@ int __init ima_init(void)
 
 	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 6c9427939a28..591ff3d76fe6 100644
--- a/security/integrity/ima/ima_queue.c
+++ b/security/integrity/ima/ima_queue.c
@@ -143,7 +143,7 @@ static int ima_pcr_extend(const u8 *hash, int pcr)
 	int result = 0;
 
 	down_read(&ima_tpm_chip_lock);
-	if (!ima_used_chip)
+	if (!ima_tpm_chip)
 		goto out;
 
 	result = tpm_pcr_extend(ima_tpm_chip, pcr, hash);
-- 
2.13.6


^ permalink raw reply related	[flat|nested] 30+ messages in thread
* Re: [PATCH v2 1/4] tpm: Implement tpm_chip_find() and tpm_chip_put() for other subsystems
  2018-06-20 20:42 ` [PATCH v2 1/4] tpm: Implement tpm_chip_find() and tpm_chip_put() for other subsystems Stefan Berger
@ 2018-06-20 20:50   ` Jason Gunthorpe
  2018-06-20 21:13       ` Stefan Berger
  2018-06-21 17:13     ` Jarkko Sakkinen
  2018-06-21 17:15   ` Jarkko Sakkinen
  1 sibling, 2 replies; 30+ messages in thread
From: Jason Gunthorpe @ 2018-06-20 20:50 UTC (permalink / raw)
  To: Stefan Berger; +Cc: linux-integrity, jarkko.sakkinen, zohar, linux-kernel

On Wed, Jun 20, 2018 at 04:42:33PM -0400, Stefan Berger wrote:
> Implement tpm_chip_find() for other subsystems to find a TPM chip and
> get a reference to that chip. Once done with using the chip, the reference
> is released using tpm_chip_put().
> 
> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
>  drivers/char/tpm/tpm-chip.c | 37 +++++++++++++++++++++++++++++++++++++
>  include/linux/tpm.h         |  9 +++++++++
>  2 files changed, 46 insertions(+)
> 
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index 0a62c19937b6..eaaf41ce73d8 100644
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -81,6 +81,43 @@ void tpm_put_ops(struct tpm_chip *chip)
>  EXPORT_SYMBOL_GPL(tpm_put_ops);
>  
>  /**
> + * tpm_chip_put() - Release a ref to the tpm_chip
> + * @chip: Chip to put
> + */
> +void tpm_chip_put(struct tpm_chip *chip)
> +{
> +	if (chip)
> +		put_device(&chip->dev);

Rarely like to see those if's inside a put function.. Does it actually
help anything?

Jason

^ permalink raw reply	[flat|nested] 30+ messages in thread
* Re: [PATCH v2 1/4] tpm: Implement tpm_chip_find() and tpm_chip_put() for other subsystems
  2018-06-20 20:50   ` Jason Gunthorpe
@ 2018-06-20 21:13       ` Stefan Berger
  2018-06-21 17:13     ` Jarkko Sakkinen
  1 sibling, 0 replies; 30+ messages in thread
From: Stefan Berger @ 2018-06-20 21:13 UTC (permalink / raw)
  To: Jason Gunthorpe; +Cc: linux-integrity, jarkko.sakkinen, zohar, linux-kernel

On 06/20/2018 04:50 PM, Jason Gunthorpe wrote:
> On Wed, Jun 20, 2018 at 04:42:33PM -0400, Stefan Berger wrote:
>> Implement tpm_chip_find() for other subsystems to find a TPM chip and
>> get a reference to that chip. Once done with using the chip, the reference
>> is released using tpm_chip_put().
>>
>> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
>>   drivers/char/tpm/tpm-chip.c | 37 +++++++++++++++++++++++++++++++++++++
>>   include/linux/tpm.h         |  9 +++++++++
>>   2 files changed, 46 insertions(+)
>>
>> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
>> index 0a62c19937b6..eaaf41ce73d8 100644
>> +++ b/drivers/char/tpm/tpm-chip.c
>> @@ -81,6 +81,43 @@ void tpm_put_ops(struct tpm_chip *chip)
>>   EXPORT_SYMBOL_GPL(tpm_put_ops);
>>   
>>   /**
>> + * tpm_chip_put() - Release a ref to the tpm_chip
>> + * @chip: Chip to put
>> + */
>> +void tpm_chip_put(struct tpm_chip *chip)
>> +{
>> +	if (chip)
>> +		put_device(&chip->dev);
> Rarely like to see those if's inside a put function.. Does it actually
> help anything?
Following put_device() 'pattern' here which also checks using 'if (dev)'.

     Stefan


^ permalink raw reply	[flat|nested] 30+ messages in thread
* Re: [PATCH v2 1/4] tpm: Implement tpm_chip_find() and tpm_chip_put() for other subsystems
@ 2018-06-20 21:13       ` Stefan Berger
  0 siblings, 0 replies; 30+ messages in thread
From: Stefan Berger @ 2018-06-20 21:13 UTC (permalink / raw)
  To: Jason Gunthorpe; +Cc: linux-integrity, jarkko.sakkinen, zohar, linux-kernel

On 06/20/2018 04:50 PM, Jason Gunthorpe wrote:
> On Wed, Jun 20, 2018 at 04:42:33PM -0400, Stefan Berger wrote:
>> Implement tpm_chip_find() for other subsystems to find a TPM chip and
>> get a reference to that chip. Once done with using the chip, the reference
>> is released using tpm_chip_put().
>>
>> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
>>   drivers/char/tpm/tpm-chip.c | 37 +++++++++++++++++++++++++++++++++++++
>>   include/linux/tpm.h         |  9 +++++++++
>>   2 files changed, 46 insertions(+)
>>
>> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
>> index 0a62c19937b6..eaaf41ce73d8 100644
>> +++ b/drivers/char/tpm/tpm-chip.c
>> @@ -81,6 +81,43 @@ void tpm_put_ops(struct tpm_chip *chip)
>>   EXPORT_SYMBOL_GPL(tpm_put_ops);
>>   
>>   /**
>> + * tpm_chip_put() - Release a ref to the tpm_chip
>> + * @chip: Chip to put
>> + */
>> +void tpm_chip_put(struct tpm_chip *chip)
>> +{
>> +	if (chip)
>> +		put_device(&chip->dev);
> Rarely like to see those if's inside a put function.. Does it actually
> help anything?
Following put_device() 'pattern' here which also checks using 'if (dev)'.

     Stefan

^ permalink raw reply	[flat|nested] 30+ messages in thread
* Re: [PATCH v2 1/4] tpm: Implement tpm_chip_find() and tpm_chip_put() for other subsystems
  2018-06-20 20:50   ` Jason Gunthorpe
  2018-06-20 21:13       ` Stefan Berger
@ 2018-06-21 17:13     ` Jarkko Sakkinen
  1 sibling, 0 replies; 30+ messages in thread
From: Jarkko Sakkinen @ 2018-06-21 17:13 UTC (permalink / raw)
  To: Jason Gunthorpe; +Cc: Stefan Berger, linux-integrity, zohar, linux-kernel

On Wed, Jun 20, 2018 at 02:50:04PM -0600, Jason Gunthorpe wrote:
> On Wed, Jun 20, 2018 at 04:42:33PM -0400, Stefan Berger wrote:
> > Implement tpm_chip_find() for other subsystems to find a TPM chip and
> > get a reference to that chip. Once done with using the chip, the reference
> > is released using tpm_chip_put().
> > 
> > Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> >  drivers/char/tpm/tpm-chip.c | 37 +++++++++++++++++++++++++++++++++++++
> >  include/linux/tpm.h         |  9 +++++++++
> >  2 files changed, 46 insertions(+)
> > 
> > diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> > index 0a62c19937b6..eaaf41ce73d8 100644
> > +++ b/drivers/char/tpm/tpm-chip.c
> > @@ -81,6 +81,43 @@ void tpm_put_ops(struct tpm_chip *chip)
> >  EXPORT_SYMBOL_GPL(tpm_put_ops);
> >  
> >  /**
> > + * tpm_chip_put() - Release a ref to the tpm_chip
> > + * @chip: Chip to put
> > + */
> > +void tpm_chip_put(struct tpm_chip *chip)
> > +{
> > +	if (chip)
> > +		put_device(&chip->dev);
> 
> Rarely like to see those if's inside a put function.. Does it actually
> help anything?
> 
> Jason

The check only helps to hidden regressions here...

/Jarkko

^ permalink raw reply	[flat|nested] 30+ messages in thread
* Re: [PATCH v2 1/4] tpm: Implement tpm_chip_find() and tpm_chip_put() for other subsystems
  2018-06-20 20:42 ` [PATCH v2 1/4] tpm: Implement tpm_chip_find() and tpm_chip_put() for other subsystems Stefan Berger
  2018-06-20 20:50   ` Jason Gunthorpe
@ 2018-06-21 17:15   ` Jarkko Sakkinen
  2018-06-21 17:27       ` Stefan Berger
  2018-06-21 17:45       ` Stefan Berger
  1 sibling, 2 replies; 30+ messages in thread
From: Jarkko Sakkinen @ 2018-06-21 17:15 UTC (permalink / raw)
  To: Stefan Berger; +Cc: linux-integrity, zohar, jgg, linux-kernel

On Wed, Jun 20, 2018 at 04:42:33PM -0400, Stefan Berger wrote:
> Implement tpm_chip_find() for other subsystems to find a TPM chip and
> get a reference to that chip. Once done with using the chip, the reference
> is released using tpm_chip_put().
> 
> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>

You should sort this out in a way that we don't end up with duplicate
functions.

/Jarkko

^ permalink raw reply	[flat|nested] 30+ messages in thread
* Re: [PATCH v2 1/4] tpm: Implement tpm_chip_find() and tpm_chip_put() for other subsystems
  2018-06-21 17:15   ` Jarkko Sakkinen
@ 2018-06-21 17:27       ` Stefan Berger
  2018-06-21 17:45       ` Stefan Berger
  1 sibling, 0 replies; 30+ messages in thread
From: Stefan Berger @ 2018-06-21 17:27 UTC (permalink / raw)
  To: Jarkko Sakkinen; +Cc: linux-integrity, zohar, jgg, linux-kernel

On 06/21/2018 01:15 PM, Jarkko Sakkinen wrote:
> On Wed, Jun 20, 2018 at 04:42:33PM -0400, Stefan Berger wrote:
>> Implement tpm_chip_find() for other subsystems to find a TPM chip and
>> get a reference to that chip. Once done with using the chip, the reference
>> is released using tpm_chip_put().
>>
>> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> You should sort this out in a way that we don't end up with duplicate
> functions.

We cannot hold the ops_sem semaphore for a long time. So we need a 
function that gets us a reference and doesn't hold the ops semaphore and 
that's where this tpm_chip_find() comes from.

    Stefan

> /Jarkko
>


^ permalink raw reply	[flat|nested] 30+ messages in thread
* Re: [PATCH v2 1/4] tpm: Implement tpm_chip_find() and tpm_chip_put() for other subsystems
@ 2018-06-21 17:27       ` Stefan Berger
  0 siblings, 0 replies; 30+ messages in thread
From: Stefan Berger @ 2018-06-21 17:27 UTC (permalink / raw)
  To: Jarkko Sakkinen; +Cc: linux-integrity, zohar, jgg, linux-kernel

On 06/21/2018 01:15 PM, Jarkko Sakkinen wrote:
> On Wed, Jun 20, 2018 at 04:42:33PM -0400, Stefan Berger wrote:
>> Implement tpm_chip_find() for other subsystems to find a TPM chip and
>> get a reference to that chip. Once done with using the chip, the reference
>> is released using tpm_chip_put().
>>
>> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> You should sort this out in a way that we don't end up with duplicate
> functions.

We cannot hold the ops_sem semaphore for a long time. So we need a 
function that gets us a reference and doesn't hold the ops semaphore and 
that's where this tpm_chip_find() comes from.

    Stefan

> /Jarkko
>

^ permalink raw reply	[flat|nested] 30+ messages in thread
* Re: [PATCH v2 1/4] tpm: Implement tpm_chip_find() and tpm_chip_put() for other subsystems
  2018-06-21 17:15   ` Jarkko Sakkinen
@ 2018-06-21 17:45       ` Stefan Berger
  2018-06-21 17:45       ` Stefan Berger
  1 sibling, 0 replies; 30+ messages in thread
From: Stefan Berger @ 2018-06-21 17:45 UTC (permalink / raw)
  To: Jarkko Sakkinen; +Cc: linux-integrity, zohar, jgg, linux-kernel

On 06/21/2018 01:15 PM, Jarkko Sakkinen wrote:
> On Wed, Jun 20, 2018 at 04:42:33PM -0400, Stefan Berger wrote:
>> Implement tpm_chip_find() for other subsystems to find a TPM chip and
>> get a reference to that chip. Once done with using the chip, the reference
>> is released using tpm_chip_put().
>>
>> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> You should sort this out in a way that we don't end up with duplicate
> functions.

Do you want me to create a function *like* tpm_chip_find_get() that 
takes an additional parameter whether to get the ops semaphore and have 
that function called by the existing tpm_chip_find_get() and the new 
tpm_chip_find(). The latter would then not get the ops semphore. I 
didn't want to do this since one time the function returns with a lock 
held and the other time not.

   Stefan

>
> /Jarkko
>


^ permalink raw reply	[flat|nested] 30+ messages in thread
* Re: [PATCH v2 1/4] tpm: Implement tpm_chip_find() and tpm_chip_put() for other subsystems
@ 2018-06-21 17:45       ` Stefan Berger
  0 siblings, 0 replies; 30+ messages in thread
From: Stefan Berger @ 2018-06-21 17:45 UTC (permalink / raw)
  To: Jarkko Sakkinen; +Cc: linux-integrity, zohar, jgg, linux-kernel

On 06/21/2018 01:15 PM, Jarkko Sakkinen wrote:
> On Wed, Jun 20, 2018 at 04:42:33PM -0400, Stefan Berger wrote:
>> Implement tpm_chip_find() for other subsystems to find a TPM chip and
>> get a reference to that chip. Once done with using the chip, the reference
>> is released using tpm_chip_put().
>>
>> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> You should sort this out in a way that we don't end up with duplicate
> functions.

Do you want me to create a function *like* tpm_chip_find_get() that 
takes an additional parameter whether to get the ops semaphore and have 
that function called by the existing tpm_chip_find_get() and the new 
tpm_chip_find(). The latter would then not get the ops semphore. I 
didn't want to do this since one time the function returns with a lock 
held and the other time not.

   Stefan

>
> /Jarkko
>

^ permalink raw reply	[flat|nested] 30+ messages in thread
* Re: [PATCH v2 1/4] tpm: Implement tpm_chip_find() and tpm_chip_put() for other subsystems
  2018-06-21 17:45       ` Stefan Berger
  (?)
@ 2018-06-21 17:56       ` Jason Gunthorpe
  2018-06-21 18:19           ` Stefan Berger
  -1 siblings, 1 reply; 30+ messages in thread
From: Jason Gunthorpe @ 2018-06-21 17:56 UTC (permalink / raw)
  To: Stefan Berger; +Cc: Jarkko Sakkinen, linux-integrity, zohar, linux-kernel

On Thu, Jun 21, 2018 at 01:45:03PM -0400, Stefan Berger wrote:
> On 06/21/2018 01:15 PM, Jarkko Sakkinen wrote:
> >On Wed, Jun 20, 2018 at 04:42:33PM -0400, Stefan Berger wrote:
> >>Implement tpm_chip_find() for other subsystems to find a TPM chip and
> >>get a reference to that chip. Once done with using the chip, the reference
> >>is released using tpm_chip_put().
> >>
> >>Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> >You should sort this out in a way that we don't end up with duplicate
> >functions.
> 
> Do you want me to create a function *like* tpm_chip_find_get() that takes an
> additional parameter whether to get the ops semaphore and have that function
> called by the existing tpm_chip_find_get() and the new tpm_chip_find(). The
> latter would then not get the ops semphore. I didn't want to do this since
> one time the function returns with a lock held and the other time not.

Another option, and I haven't looked, is to revise the callers of
tpm_chip_find_get to not require it to hold the ops semaphore for
them.

Either by giving them an API to do it, or revising the TPM entry
points to do it.

I didn't look, but how did the ops semaphore get grabbed in your
revised patches? They do grab it, right?

Jason

^ permalink raw reply	[flat|nested] 30+ messages in thread
* Re: [PATCH v2 1/4] tpm: Implement tpm_chip_find() and tpm_chip_put() for other subsystems
  2018-06-21 17:56       ` Jason Gunthorpe
@ 2018-06-21 18:19           ` Stefan Berger
  0 siblings, 0 replies; 30+ messages in thread
From: Stefan Berger @ 2018-06-21 18:19 UTC (permalink / raw)
  To: Jason Gunthorpe; +Cc: Jarkko Sakkinen, linux-integrity, zohar, linux-kernel

On 06/21/2018 01:56 PM, Jason Gunthorpe wrote:
> On Thu, Jun 21, 2018 at 01:45:03PM -0400, Stefan Berger wrote:
>> On 06/21/2018 01:15 PM, Jarkko Sakkinen wrote:
>>> On Wed, Jun 20, 2018 at 04:42:33PM -0400, Stefan Berger wrote:
>>>> Implement tpm_chip_find() for other subsystems to find a TPM chip and
>>>> get a reference to that chip. Once done with using the chip, the reference
>>>> is released using tpm_chip_put().
>>>>
>>>> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
>>> You should sort this out in a way that we don't end up with duplicate
>>> functions.
>> Do you want me to create a function *like* tpm_chip_find_get() that takes an
>> additional parameter whether to get the ops semaphore and have that function
>> called by the existing tpm_chip_find_get() and the new tpm_chip_find(). The
>> latter would then not get the ops semphore. I didn't want to do this since
>> one time the function returns with a lock held and the other time not.
> Another option, and I haven't looked, is to revise the callers of
> tpm_chip_find_get to not require it to hold the ops semaphore for
> them.

We have tpm_chip_unregister calling tpm_del_char_device to set the ops 
to NULL once a chip is unregistered. All existing callers, if they pass 
in a tpm_chip != NULL, currently fail if the ops are NULL. (If they pass 
in tpm_chip = NULL, they shouldn't find a chip once ops are null and it 
has been removed from the IDR). I wouldn't change that since IMA will 
call in with a tpm_chip != NULL and we want to protect the ops. All 
existing code within the tpm subsystem does seem to call 
tpm_chip_find_get() with a NULL pointer, though. Also trusted keys seems 
to pass in a NULL pointer every time.

>
> Either by giving them an API to do it, or revising the TPM entry
> points to do it.
>
> I didn't look, but how did the ops semaphore get grabbed in your
> revised patches? They do grab it, right?

The revised patches do not touch the existing code much but will call 
tpm_chip_find_get() and get that semaphore every time before the ops are 
used. IMA is the only caller of tpm_chip_find() that now gets an 
additional reference to the tpm_chip and these APIs get called like this 
from IMA:

ima init: chip = tpm_chip_find()

ima::tpm: tpm_chip_find_get(chip) ... tpm_put_ops(chip)

ima::tpm: tpm_chip_find_get(chip) ... tpm_put_ops(chip)

[repeat]

ima shutdown: tpm_chip_put(chip)

     Stefan

>
> Jason
>


^ permalink raw reply	[flat|nested] 30+ messages in thread
* Re: [PATCH v2 1/4] tpm: Implement tpm_chip_find() and tpm_chip_put() for other subsystems
@ 2018-06-21 18:19           ` Stefan Berger
  0 siblings, 0 replies; 30+ messages in thread
From: Stefan Berger @ 2018-06-21 18:19 UTC (permalink / raw)
  To: Jason Gunthorpe; +Cc: Jarkko Sakkinen, linux-integrity, zohar, linux-kernel

On 06/21/2018 01:56 PM, Jason Gunthorpe wrote:
> On Thu, Jun 21, 2018 at 01:45:03PM -0400, Stefan Berger wrote:
>> On 06/21/2018 01:15 PM, Jarkko Sakkinen wrote:
>>> On Wed, Jun 20, 2018 at 04:42:33PM -0400, Stefan Berger wrote:
>>>> Implement tpm_chip_find() for other subsystems to find a TPM chip and
>>>> get a reference to that chip. Once done with using the chip, the reference
>>>> is released using tpm_chip_put().
>>>>
>>>> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
>>> You should sort this out in a way that we don't end up with duplicate
>>> functions.
>> Do you want me to create a function *like* tpm_chip_find_get() that takes an
>> additional parameter whether to get the ops semaphore and have that function
>> called by the existing tpm_chip_find_get() and the new tpm_chip_find(). The
>> latter would then not get the ops semphore. I didn't want to do this since
>> one time the function returns with a lock held and the other time not.
> Another option, and I haven't looked, is to revise the callers of
> tpm_chip_find_get to not require it to hold the ops semaphore for
> them.

We have tpm_chip_unregister calling tpm_del_char_device to set the ops 
to NULL once a chip is unregistered. All existing callers, if they pass 
in a tpm_chip != NULL, currently fail if the ops are NULL. (If they pass 
in tpm_chip = NULL, they shouldn't find a chip once ops are null and it 
has been removed from the IDR). I wouldn't change that since IMA will 
call in with a tpm_chip != NULL and we want to protect the ops. All 
existing code within the tpm subsystem does seem to call 
tpm_chip_find_get() with a NULL pointer, though. Also trusted keys seems 
to pass in a NULL pointer every time.

>
> Either by giving them an API to do it, or revising the TPM entry
> points to do it.
>
> I didn't look, but how did the ops semaphore get grabbed in your
> revised patches? They do grab it, right?

The revised patches do not touch the existing code much but will call 
tpm_chip_find_get() and get that semaphore every time before the ops are 
used. IMA is the only caller of tpm_chip_find() that now gets an 
additional reference to the tpm_chip and these APIs get called like this 
from IMA:

ima init: chip = tpm_chip_find()

ima::tpm: tpm_chip_find_get(chip) ... tpm_put_ops(chip)

ima::tpm: tpm_chip_find_get(chip) ... tpm_put_ops(chip)

[repeat]

ima shutdown: tpm_chip_put(chip)

     Stefan

>
> Jason
>

^ permalink raw reply	[flat|nested] 30+ messages in thread
* Re: [PATCH v2 1/4] tpm: Implement tpm_chip_find() and tpm_chip_put() for other subsystems
  2018-06-21 18:19           ` Stefan Berger
  (?)
@ 2018-06-21 19:06           ` Jason Gunthorpe
  2018-06-21 20:14             ` Stefan Berger
  -1 siblings, 1 reply; 30+ messages in thread
From: Jason Gunthorpe @ 2018-06-21 19:06 UTC (permalink / raw)
  To: Stefan Berger; +Cc: Jarkko Sakkinen, linux-integrity, zohar, linux-kernel

On Thu, Jun 21, 2018 at 02:19:44PM -0400, Stefan Berger wrote:
> On 06/21/2018 01:56 PM, Jason Gunthorpe wrote:
> >On Thu, Jun 21, 2018 at 01:45:03PM -0400, Stefan Berger wrote:
> >>On 06/21/2018 01:15 PM, Jarkko Sakkinen wrote:
> >>>On Wed, Jun 20, 2018 at 04:42:33PM -0400, Stefan Berger wrote:
> >>>>Implement tpm_chip_find() for other subsystems to find a TPM chip and
> >>>>get a reference to that chip. Once done with using the chip, the reference
> >>>>is released using tpm_chip_put().
> >>>>
> >>>>Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> >>>You should sort this out in a way that we don't end up with duplicate
> >>>functions.
> >>Do you want me to create a function *like* tpm_chip_find_get() that takes an
> >>additional parameter whether to get the ops semaphore and have that function
> >>called by the existing tpm_chip_find_get() and the new tpm_chip_find(). The
> >>latter would then not get the ops semphore. I didn't want to do this since
> >>one time the function returns with a lock held and the other time not.
> >Another option, and I haven't looked, is to revise the callers of
> >tpm_chip_find_get to not require it to hold the ops semaphore for
> >them.
> 
> We have tpm_chip_unregister calling tpm_del_char_device to set the ops to
> NULL once a chip is unregistered. All existing callers, if they pass in a
> tpm_chip != NULL, currently fail if the ops are NULL. (If they pass in
> tpm_chip = NULL, they shouldn't find a chip once ops are null and it has
> been removed from the IDR). I wouldn't change that since IMA will call in
> with a tpm_chip != NULL and we want to protect the ops. All existing code
> within the tpm subsystem does seem to call tpm_chip_find_get() with a NULL
> pointer, though. Also trusted keys seems to pass in a NULL pointer every
> time.
> 
> >
> >Either by giving them an API to do it, or revising the TPM entry
> >points to do it.
> >
> >I didn't look, but how did the ops semaphore get grabbed in your
> >revised patches? They do grab it, right?
> 
> The revised patches do not touch the existing code much but will call
> tpm_chip_find_get() and get that semaphore every time before the ops are
> used. IMA is the only caller of tpm_chip_find() that now gets an additional
> reference to the tpm_chip and these APIs get called like this from IMA:
> 
> ima init: chip = tpm_chip_find()
> 
> ima::tpm: tpm_chip_find_get(chip) ... tpm_put_ops(chip)
> 
> ima::tpm: tpm_chip_find_get(chip) ... tpm_put_ops(chip)
> 
> [repeat]
> 
> ima shutdown: tpm_chip_put(chip)

Maybe just change tpm_chip_find_get() into tpm_get_ops(chip) and
convert all callers?

Jason

^ permalink raw reply	[flat|nested] 30+ messages in thread
* Re: [PATCH v2 1/4] tpm: Implement tpm_chip_find() and tpm_chip_put() for other subsystems
  2018-06-21 19:06           ` Jason Gunthorpe
@ 2018-06-21 20:14             ` Stefan Berger
  2018-06-21 20:51               ` Jason Gunthorpe
  0 siblings, 1 reply; 30+ messages in thread
From: Stefan Berger @ 2018-06-21 20:14 UTC (permalink / raw)
  To: Jason Gunthorpe; +Cc: Jarkko Sakkinen, linux-integrity, zohar, linux-kernel

On 06/21/2018 03:06 PM, Jason Gunthorpe wrote:
> On Thu, Jun 21, 2018 at 02:19:44PM -0400, Stefan Berger wrote:
>> On 06/21/2018 01:56 PM, Jason Gunthorpe wrote:
>>> On Thu, Jun 21, 2018 at 01:45:03PM -0400, Stefan Berger wrote:
>>>> On 06/21/2018 01:15 PM, Jarkko Sakkinen wrote:
>>>>> On Wed, Jun 20, 2018 at 04:42:33PM -0400, Stefan Berger wrote:
>>>>>> Implement tpm_chip_find() for other subsystems to find a TPM chip and
>>>>>> get a reference to that chip. Once done with using the chip, the reference
>>>>>> is released using tpm_chip_put().
>>>>>>
>>>>>> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
>>>>> You should sort this out in a way that we don't end up with duplicate
>>>>> functions.
>>>> Do you want me to create a function *like* tpm_chip_find_get() that takes an
>>>> additional parameter whether to get the ops semaphore and have that function
>>>> called by the existing tpm_chip_find_get() and the new tpm_chip_find(). The
>>>> latter would then not get the ops semphore. I didn't want to do this since
>>>> one time the function returns with a lock held and the other time not.
>>> Another option, and I haven't looked, is to revise the callers of
>>> tpm_chip_find_get to not require it to hold the ops semaphore for
>>> them.
>> We have tpm_chip_unregister calling tpm_del_char_device to set the ops to
>> NULL once a chip is unregistered. All existing callers, if they pass in a
>> tpm_chip != NULL, currently fail if the ops are NULL. (If they pass in
>> tpm_chip = NULL, they shouldn't find a chip once ops are null and it has
>> been removed from the IDR). I wouldn't change that since IMA will call in
>> with a tpm_chip != NULL and we want to protect the ops. All existing code
>> within the tpm subsystem does seem to call tpm_chip_find_get() with a NULL
>> pointer, though. Also trusted keys seems to pass in a NULL pointer every
>> time.
>>
>>> Either by giving them an API to do it, or revising the TPM entry
>>> points to do it.
>>>
>>> I didn't look, but how did the ops semaphore get grabbed in your
>>> revised patches? They do grab it, right?
>> The revised patches do not touch the existing code much but will call
>> tpm_chip_find_get() and get that semaphore every time before the ops are
>> used. IMA is the only caller of tpm_chip_find() that now gets an additional
>> reference to the tpm_chip and these APIs get called like this from IMA:
>>
>> ima init: chip = tpm_chip_find()
>>
>> ima::tpm: tpm_chip_find_get(chip) ... tpm_put_ops(chip)
>>
>> ima::tpm: tpm_chip_find_get(chip) ... tpm_put_ops(chip)
>>
>> [repeat]
>>
>> ima shutdown: tpm_chip_put(chip)
> Maybe just change tpm_chip_find_get() into tpm_get_ops(chip) and
> convert all callers?

And then re-introduce tpm_chip_find_get() for IMA to call ?


^ permalink raw reply	[flat|nested] 30+ messages in thread
* Re: [PATCH v2 1/4] tpm: Implement tpm_chip_find() and tpm_chip_put() for other subsystems
  2018-06-21 20:14             ` Stefan Berger
@ 2018-06-21 20:51               ` Jason Gunthorpe
  0 siblings, 0 replies; 30+ messages in thread
From: Jason Gunthorpe @ 2018-06-21 20:51 UTC (permalink / raw)
  To: Stefan Berger; +Cc: Jarkko Sakkinen, linux-integrity, zohar, linux-kernel

On Thu, Jun 21, 2018 at 04:14:46PM -0400, Stefan Berger wrote:
> On 06/21/2018 03:06 PM, Jason Gunthorpe wrote:
> >On Thu, Jun 21, 2018 at 02:19:44PM -0400, Stefan Berger wrote:
> >>On 06/21/2018 01:56 PM, Jason Gunthorpe wrote:
> >>>On Thu, Jun 21, 2018 at 01:45:03PM -0400, Stefan Berger wrote:
> >>>>On 06/21/2018 01:15 PM, Jarkko Sakkinen wrote:
> >>>>>On Wed, Jun 20, 2018 at 04:42:33PM -0400, Stefan Berger wrote:
> >>>>>>Implement tpm_chip_find() for other subsystems to find a TPM chip and
> >>>>>>get a reference to that chip. Once done with using the chip, the reference
> >>>>>>is released using tpm_chip_put().
> >>>>>>
> >>>>>>Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> >>>>>You should sort this out in a way that we don't end up with duplicate
> >>>>>functions.
> >>>>Do you want me to create a function *like* tpm_chip_find_get() that takes an
> >>>>additional parameter whether to get the ops semaphore and have that function
> >>>>called by the existing tpm_chip_find_get() and the new tpm_chip_find(). The
> >>>>latter would then not get the ops semphore. I didn't want to do this since
> >>>>one time the function returns with a lock held and the other time not.
> >>>Another option, and I haven't looked, is to revise the callers of
> >>>tpm_chip_find_get to not require it to hold the ops semaphore for
> >>>them.
> >>We have tpm_chip_unregister calling tpm_del_char_device to set the ops to
> >>NULL once a chip is unregistered. All existing callers, if they pass in a
> >>tpm_chip != NULL, currently fail if the ops are NULL. (If they pass in
> >>tpm_chip = NULL, they shouldn't find a chip once ops are null and it has
> >>been removed from the IDR). I wouldn't change that since IMA will call in
> >>with a tpm_chip != NULL and we want to protect the ops. All existing code
> >>within the tpm subsystem does seem to call tpm_chip_find_get() with a NULL
> >>pointer, though. Also trusted keys seems to pass in a NULL pointer every
> >>time.
> >>
> >>>Either by giving them an API to do it, or revising the TPM entry
> >>>points to do it.
> >>>
> >>>I didn't look, but how did the ops semaphore get grabbed in your
> >>>revised patches? They do grab it, right?
> >>The revised patches do not touch the existing code much but will call
> >>tpm_chip_find_get() and get that semaphore every time before the ops are
> >>used. IMA is the only caller of tpm_chip_find() that now gets an additional
> >>reference to the tpm_chip and these APIs get called like this from IMA:
> >>
> >>ima init: chip = tpm_chip_find()
> >>
> >>ima::tpm: tpm_chip_find_get(chip) ... tpm_put_ops(chip)
> >>
> >>ima::tpm: tpm_chip_find_get(chip) ... tpm_put_ops(chip)
> >>
> >>[repeat]
> >>
> >>ima shutdown: tpm_chip_put(chip)
> >Maybe just change tpm_chip_find_get() into tpm_get_ops(chip) and
> >convert all callers?
> 
> And then re-introduce tpm_chip_find_get() for IMA to call ?

You could keep it as 'tpm_chip_find', that seems like a fine name to
me

Jason

^ permalink raw reply	[flat|nested] 30+ messages in thread
* Re: [PATCH v2 3/4] ima: Use tpm_chip_find() and access TPM functions using it
  2018-06-20 20:42 ` [PATCH v2 3/4] ima: Use tpm_chip_find() and access TPM functions using it Stefan Berger
@ 2018-06-21 20:53     ` Mimi Zohar
  0 siblings, 0 replies; 30+ messages in thread
From: Mimi Zohar @ 2018-06-21 20:53 UTC (permalink / raw)
  To: Stefan Berger, linux-integrity, jarkko.sakkinen; +Cc: jgg, linux-kernel

On Wed, 2018-06-20 at 16:42 -0400, Stefan Berger wrote:
> Rather than accessing the TPM functions using a NULL pointer, which
> causes a lookup for a suitable chip every time, get a hold of a tpm_chip
> and access the TPM functions using this chip. We call the tpm_chip
> ima_tpm_chip and protect it, once initialization is done, using a
> rw_semaphore called ima_tpm_chip_lock.
> 
> Use ima_shutdown to release the tpm_chip.
> 
> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> ---
>  security/integrity/ima/ima.h        |  3 +++
>  security/integrity/ima/ima_crypto.c | 12 ++++++++++--
>  security/integrity/ima/ima_init.c   | 19 ++++++++++++-------
>  security/integrity/ima/ima_queue.c  |  7 +++++--
>  4 files changed, 30 insertions(+), 11 deletions(-)
> 
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index 354bb5716ce3..53a88d578ca5 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -24,6 +24,7 @@
>  #include <linux/hash.h>
>  #include <linux/tpm.h>
>  #include <linux/audit.h>
> +#include <linux/rwsem.h>
>  #include <crypto/hash_info.h>
> 
>  #include "../integrity.h"
> @@ -56,6 +57,8 @@ extern int ima_policy_flag;
>  extern int ima_used_chip;
>  extern int ima_hash_algo;
>  extern int ima_appraise;
> +extern struct rw_semaphore ima_tpm_chip_lock;
> +extern struct tpm_chip *ima_tpm_chip;


ima_add_templatE_entry() synchronizes appending a measurement to the
measurement list and extending the TPM by taking a lock.  Do we really
need to introduce another lock?

Mimi

> 
>  /* 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..da7715240476 100644
> --- a/security/integrity/ima/ima_crypto.c
> +++ b/security/integrity/ima/ima_crypto.c
> @@ -631,10 +631,18 @@ int ima_calc_buffer_hash(const void *buf, loff_t len,
> 
>  static void __init ima_pcrread(int idx, u8 *pcr)
>  {
> +	int result = 0;
> +
> +	down_read(&ima_tpm_chip_lock);
> +
>  	if (!ima_used_chip)
> -		return;
> +		goto out;
> +
> +	result = tpm_pcr_read(ima_tpm_chip, idx, pcr);
> +out:
> +	up_read(&ima_tpm_chip_lock);
> 
> -	if (tpm_pcr_read(NULL, idx, pcr) != 0)
> +	if (result != 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 8a5258eb32b6..24db06c4f463 100644
> --- a/security/integrity/ima/ima_init.c
> +++ b/security/integrity/ima/ima_init.c
> @@ -28,6 +28,8 @@
>  /* name for boot aggregate entry */
>  static const char *boot_aggregate_name = "boot_aggregate";
>  int ima_used_chip;
> +struct rw_semaphore ima_tpm_chip_lock = __RWSEM_INITIALIZER(ima_tpm_chip_lock);
> +struct tpm_chip *ima_tpm_chip;
> 
>  /* Add the boot aggregate to the IMA measurement list and extend
>   * the PCR register.
> @@ -108,6 +110,13 @@ void __init ima_load_x509(void)
>  static int ima_shutdown(struct notifier_block *this, unsigned long action,
>  			void *data)
>  {
> +	down_write(&ima_tpm_chip_lock);
> +	if (ima_tpm_chip) {
> +		tpm_chip_put(ima_tpm_chip);
> +		ima_tpm_chip = NULL;
> +		ima_used_chip = 0;
> +	}
> +	up_write(&ima_tpm_chip_lock);
>  	return NOTIFY_DONE;
>  }
> 
> @@ -118,19 +127,15 @@ static struct notifier_block ima_reboot_notifier = {
> 
>  int __init ima_init(void)
>  {
> -	u8 pcr_i[TPM_DIGEST_SIZE];
>  	int rc;
> 
>  	register_reboot_notifier(&ima_reboot_notifier);
> 
> -	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..6c9427939a28 100644
> --- a/security/integrity/ima/ima_queue.c
> +++ b/security/integrity/ima/ima_queue.c
> @@ -142,10 +142,13 @@ static int ima_pcr_extend(const u8 *hash, int pcr)
>  {
>  	int result = 0;
> 
> +	down_read(&ima_tpm_chip_lock);
>  	if (!ima_used_chip)
> -		return result;
> +		goto out;
> 
> -	result = tpm_pcr_extend(NULL, pcr, hash);
> +	result = tpm_pcr_extend(ima_tpm_chip, pcr, hash);
> +out:
> +	up_read(&ima_tpm_chip_lock);
>  	if (result != 0)
>  		pr_err("Error Communicating to TPM chip, result: %d\n", result);
>  	return result;


^ permalink raw reply	[flat|nested] 30+ messages in thread
* Re: [PATCH v2 3/4] ima: Use tpm_chip_find() and access TPM functions using it
@ 2018-06-21 20:53     ` Mimi Zohar
  0 siblings, 0 replies; 30+ messages in thread
From: Mimi Zohar @ 2018-06-21 20:53 UTC (permalink / raw)
  To: Stefan Berger, linux-integrity, jarkko.sakkinen; +Cc: jgg, linux-kernel

On Wed, 2018-06-20 at 16:42 -0400, Stefan Berger wrote:
> Rather than accessing the TPM functions using a NULL pointer, which
> causes a lookup for a suitable chip every time, get a hold of a tpm_chip
> and access the TPM functions using this chip. We call the tpm_chip
> ima_tpm_chip and protect it, once initialization is done, using a
> rw_semaphore called ima_tpm_chip_lock.
> 
> Use ima_shutdown to release the tpm_chip.
> 
> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> ---
>  security/integrity/ima/ima.h        |  3 +++
>  security/integrity/ima/ima_crypto.c | 12 ++++++++++--
>  security/integrity/ima/ima_init.c   | 19 ++++++++++++-------
>  security/integrity/ima/ima_queue.c  |  7 +++++--
>  4 files changed, 30 insertions(+), 11 deletions(-)
> 
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index 354bb5716ce3..53a88d578ca5 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -24,6 +24,7 @@
>  #include <linux/hash.h>
>  #include <linux/tpm.h>
>  #include <linux/audit.h>
> +#include <linux/rwsem.h>
>  #include <crypto/hash_info.h>
> 
>  #include "../integrity.h"
> @@ -56,6 +57,8 @@ extern int ima_policy_flag;
>  extern int ima_used_chip;
>  extern int ima_hash_algo;
>  extern int ima_appraise;
> +extern struct rw_semaphore ima_tpm_chip_lock;
> +extern struct tpm_chip *ima_tpm_chip;


ima_add_templatE_entry() synchronizes appending a measurement to the
measurement list and extending the TPM by taking a lock.  Do we really
need to introduce another lock?

Mimi

> 
>  /* 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..da7715240476 100644
> --- a/security/integrity/ima/ima_crypto.c
> +++ b/security/integrity/ima/ima_crypto.c
> @@ -631,10 +631,18 @@ int ima_calc_buffer_hash(const void *buf, loff_t len,
> 
>  static void __init ima_pcrread(int idx, u8 *pcr)
>  {
> +	int result = 0;
> +
> +	down_read(&ima_tpm_chip_lock);
> +
>  	if (!ima_used_chip)
> -		return;
> +		goto out;
> +
> +	result = tpm_pcr_read(ima_tpm_chip, idx, pcr);
> +out:
> +	up_read(&ima_tpm_chip_lock);
> 
> -	if (tpm_pcr_read(NULL, idx, pcr) != 0)
> +	if (result != 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 8a5258eb32b6..24db06c4f463 100644
> --- a/security/integrity/ima/ima_init.c
> +++ b/security/integrity/ima/ima_init.c
> @@ -28,6 +28,8 @@
>  /* name for boot aggregate entry */
>  static const char *boot_aggregate_name = "boot_aggregate";
>  int ima_used_chip;
> +struct rw_semaphore ima_tpm_chip_lock = __RWSEM_INITIALIZER(ima_tpm_chip_lock);
> +struct tpm_chip *ima_tpm_chip;
> 
>  /* Add the boot aggregate to the IMA measurement list and extend
>   * the PCR register.
> @@ -108,6 +110,13 @@ void __init ima_load_x509(void)
>  static int ima_shutdown(struct notifier_block *this, unsigned long action,
>  			void *data)
>  {
> +	down_write(&ima_tpm_chip_lock);
> +	if (ima_tpm_chip) {
> +		tpm_chip_put(ima_tpm_chip);
> +		ima_tpm_chip = NULL;
> +		ima_used_chip = 0;
> +	}
> +	up_write(&ima_tpm_chip_lock);
>  	return NOTIFY_DONE;
>  }
> 
> @@ -118,19 +127,15 @@ static struct notifier_block ima_reboot_notifier = {
> 
>  int __init ima_init(void)
>  {
> -	u8 pcr_i[TPM_DIGEST_SIZE];
>  	int rc;
> 
>  	register_reboot_notifier(&ima_reboot_notifier);
> 
> -	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..6c9427939a28 100644
> --- a/security/integrity/ima/ima_queue.c
> +++ b/security/integrity/ima/ima_queue.c
> @@ -142,10 +142,13 @@ static int ima_pcr_extend(const u8 *hash, int pcr)
>  {
>  	int result = 0;
> 
> +	down_read(&ima_tpm_chip_lock);
>  	if (!ima_used_chip)
> -		return result;
> +		goto out;
> 
> -	result = tpm_pcr_extend(NULL, pcr, hash);
> +	result = tpm_pcr_extend(ima_tpm_chip, pcr, hash);
> +out:
> +	up_read(&ima_tpm_chip_lock);
>  	if (result != 0)
>  		pr_err("Error Communicating to TPM chip, result: %d\n", result);
>  	return result;

^ permalink raw reply	[flat|nested] 30+ messages in thread
* Re: [PATCH v2 3/4] ima: Use tpm_chip_find() and access TPM functions using it
  2018-06-21 20:53     ` Mimi Zohar
@ 2018-06-21 20:59       ` Stefan Berger
  -1 siblings, 0 replies; 30+ messages in thread
From: Stefan Berger @ 2018-06-21 20:59 UTC (permalink / raw)
  To: Mimi Zohar, linux-integrity, jarkko.sakkinen; +Cc: jgg, linux-kernel

On 06/21/2018 04:53 PM, Mimi Zohar wrote:
> On Wed, 2018-06-20 at 16:42 -0400, Stefan Berger wrote:
>> Rather than accessing the TPM functions using a NULL pointer, which
>> causes a lookup for a suitable chip every time, get a hold of a tpm_chip
>> and access the TPM functions using this chip. We call the tpm_chip
>> ima_tpm_chip and protect it, once initialization is done, using a
>> rw_semaphore called ima_tpm_chip_lock.
>>
>> Use ima_shutdown to release the tpm_chip.
>>
>> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
>> ---
>>   security/integrity/ima/ima.h        |  3 +++
>>   security/integrity/ima/ima_crypto.c | 12 ++++++++++--
>>   security/integrity/ima/ima_init.c   | 19 ++++++++++++-------
>>   security/integrity/ima/ima_queue.c  |  7 +++++--
>>   4 files changed, 30 insertions(+), 11 deletions(-)
>>
>> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
>> index 354bb5716ce3..53a88d578ca5 100644
>> --- a/security/integrity/ima/ima.h
>> +++ b/security/integrity/ima/ima.h
>> @@ -24,6 +24,7 @@
>>   #include <linux/hash.h>
>>   #include <linux/tpm.h>
>>   #include <linux/audit.h>
>> +#include <linux/rwsem.h>
>>   #include <crypto/hash_info.h>
>>
>>   #include "../integrity.h"
>> @@ -56,6 +57,8 @@ extern int ima_policy_flag;
>>   extern int ima_used_chip;
>>   extern int ima_hash_algo;
>>   extern int ima_appraise;
>> +extern struct rw_semaphore ima_tpm_chip_lock;
>> +extern struct tpm_chip *ima_tpm_chip;
>
> ima_add_templatE_entry() synchronizes appending a measurement to the
> measurement list and extending the TPM by taking a lock.  Do we really
> need to introduce another lock?

This lock protects the ima_tpm_chip from going from != NULL to NULL in 
the ima_shutdown function. Basically, a global pointer accessed by 
concurrent threads should be protected if its value can change. However, 
in this case ima_shutdown would be called so late that there shouldn't 
be concurrency anymore. Though, I found it better to protect it. Maybe 
someone else has an opinion?

     Stefan
>
> Mimi
>
>>   /* 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..da7715240476 100644
>> --- a/security/integrity/ima/ima_crypto.c
>> +++ b/security/integrity/ima/ima_crypto.c
>> @@ -631,10 +631,18 @@ int ima_calc_buffer_hash(const void *buf, loff_t len,
>>
>>   static void __init ima_pcrread(int idx, u8 *pcr)
>>   {
>> +	int result = 0;
>> +
>> +	down_read(&ima_tpm_chip_lock);
>> +
>>   	if (!ima_used_chip)
>> -		return;
>> +		goto out;
>> +
>> +	result = tpm_pcr_read(ima_tpm_chip, idx, pcr);
>> +out:
>> +	up_read(&ima_tpm_chip_lock);
>>
>> -	if (tpm_pcr_read(NULL, idx, pcr) != 0)
>> +	if (result != 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 8a5258eb32b6..24db06c4f463 100644
>> --- a/security/integrity/ima/ima_init.c
>> +++ b/security/integrity/ima/ima_init.c
>> @@ -28,6 +28,8 @@
>>   /* name for boot aggregate entry */
>>   static const char *boot_aggregate_name = "boot_aggregate";
>>   int ima_used_chip;
>> +struct rw_semaphore ima_tpm_chip_lock = __RWSEM_INITIALIZER(ima_tpm_chip_lock);
>> +struct tpm_chip *ima_tpm_chip;
>>
>>   /* Add the boot aggregate to the IMA measurement list and extend
>>    * the PCR register.
>> @@ -108,6 +110,13 @@ void __init ima_load_x509(void)
>>   static int ima_shutdown(struct notifier_block *this, unsigned long action,
>>   			void *data)
>>   {
>> +	down_write(&ima_tpm_chip_lock);
>> +	if (ima_tpm_chip) {
>> +		tpm_chip_put(ima_tpm_chip);
>> +		ima_tpm_chip = NULL;
>> +		ima_used_chip = 0;
>> +	}
>> +	up_write(&ima_tpm_chip_lock);
>>   	return NOTIFY_DONE;
>>   }
>>
>> @@ -118,19 +127,15 @@ static struct notifier_block ima_reboot_notifier = {
>>
>>   int __init ima_init(void)
>>   {
>> -	u8 pcr_i[TPM_DIGEST_SIZE];
>>   	int rc;
>>
>>   	register_reboot_notifier(&ima_reboot_notifier);
>>
>> -	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..6c9427939a28 100644
>> --- a/security/integrity/ima/ima_queue.c
>> +++ b/security/integrity/ima/ima_queue.c
>> @@ -142,10 +142,13 @@ static int ima_pcr_extend(const u8 *hash, int pcr)
>>   {
>>   	int result = 0;
>>
>> +	down_read(&ima_tpm_chip_lock);
>>   	if (!ima_used_chip)
>> -		return result;
>> +		goto out;
>>
>> -	result = tpm_pcr_extend(NULL, pcr, hash);
>> +	result = tpm_pcr_extend(ima_tpm_chip, pcr, hash);
>> +out:
>> +	up_read(&ima_tpm_chip_lock);
>>   	if (result != 0)
>>   		pr_err("Error Communicating to TPM chip, result: %d\n", result);
>>   	return result;



^ permalink raw reply	[flat|nested] 30+ messages in thread
* Re: [PATCH v2 3/4] ima: Use tpm_chip_find() and access TPM functions using it
@ 2018-06-21 20:59       ` Stefan Berger
  0 siblings, 0 replies; 30+ messages in thread
From: Stefan Berger @ 2018-06-21 20:59 UTC (permalink / raw)
  To: Mimi Zohar, linux-integrity, jarkko.sakkinen; +Cc: jgg, linux-kernel

On 06/21/2018 04:53 PM, Mimi Zohar wrote:
> On Wed, 2018-06-20 at 16:42 -0400, Stefan Berger wrote:
>> Rather than accessing the TPM functions using a NULL pointer, which
>> causes a lookup for a suitable chip every time, get a hold of a tpm_chip
>> and access the TPM functions using this chip. We call the tpm_chip
>> ima_tpm_chip and protect it, once initialization is done, using a
>> rw_semaphore called ima_tpm_chip_lock.
>>
>> Use ima_shutdown to release the tpm_chip.
>>
>> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
>> ---
>>   security/integrity/ima/ima.h        |  3 +++
>>   security/integrity/ima/ima_crypto.c | 12 ++++++++++--
>>   security/integrity/ima/ima_init.c   | 19 ++++++++++++-------
>>   security/integrity/ima/ima_queue.c  |  7 +++++--
>>   4 files changed, 30 insertions(+), 11 deletions(-)
>>
>> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
>> index 354bb5716ce3..53a88d578ca5 100644
>> --- a/security/integrity/ima/ima.h
>> +++ b/security/integrity/ima/ima.h
>> @@ -24,6 +24,7 @@
>>   #include <linux/hash.h>
>>   #include <linux/tpm.h>
>>   #include <linux/audit.h>
>> +#include <linux/rwsem.h>
>>   #include <crypto/hash_info.h>
>>
>>   #include "../integrity.h"
>> @@ -56,6 +57,8 @@ extern int ima_policy_flag;
>>   extern int ima_used_chip;
>>   extern int ima_hash_algo;
>>   extern int ima_appraise;
>> +extern struct rw_semaphore ima_tpm_chip_lock;
>> +extern struct tpm_chip *ima_tpm_chip;
>
> ima_add_templatE_entry() synchronizes appending a measurement to the
> measurement list and extending the TPM by taking a lock.  Do we really
> need to introduce another lock?

This lock protects the ima_tpm_chip from going from != NULL to NULL in 
the ima_shutdown function. Basically, a global pointer accessed by 
concurrent threads should be protected if its value can change. However, 
in this case ima_shutdown would be called so late that there shouldn't 
be concurrency anymore. Though, I found it better to protect it. Maybe 
someone else has an opinion?

     Stefan
>
> Mimi
>
>>   /* 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..da7715240476 100644
>> --- a/security/integrity/ima/ima_crypto.c
>> +++ b/security/integrity/ima/ima_crypto.c
>> @@ -631,10 +631,18 @@ int ima_calc_buffer_hash(const void *buf, loff_t len,
>>
>>   static void __init ima_pcrread(int idx, u8 *pcr)
>>   {
>> +	int result = 0;
>> +
>> +	down_read(&ima_tpm_chip_lock);
>> +
>>   	if (!ima_used_chip)
>> -		return;
>> +		goto out;
>> +
>> +	result = tpm_pcr_read(ima_tpm_chip, idx, pcr);
>> +out:
>> +	up_read(&ima_tpm_chip_lock);
>>
>> -	if (tpm_pcr_read(NULL, idx, pcr) != 0)
>> +	if (result != 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 8a5258eb32b6..24db06c4f463 100644
>> --- a/security/integrity/ima/ima_init.c
>> +++ b/security/integrity/ima/ima_init.c
>> @@ -28,6 +28,8 @@
>>   /* name for boot aggregate entry */
>>   static const char *boot_aggregate_name = "boot_aggregate";
>>   int ima_used_chip;
>> +struct rw_semaphore ima_tpm_chip_lock = __RWSEM_INITIALIZER(ima_tpm_chip_lock);
>> +struct tpm_chip *ima_tpm_chip;
>>
>>   /* Add the boot aggregate to the IMA measurement list and extend
>>    * the PCR register.
>> @@ -108,6 +110,13 @@ void __init ima_load_x509(void)
>>   static int ima_shutdown(struct notifier_block *this, unsigned long action,
>>   			void *data)
>>   {
>> +	down_write(&ima_tpm_chip_lock);
>> +	if (ima_tpm_chip) {
>> +		tpm_chip_put(ima_tpm_chip);
>> +		ima_tpm_chip = NULL;
>> +		ima_used_chip = 0;
>> +	}
>> +	up_write(&ima_tpm_chip_lock);
>>   	return NOTIFY_DONE;
>>   }
>>
>> @@ -118,19 +127,15 @@ static struct notifier_block ima_reboot_notifier = {
>>
>>   int __init ima_init(void)
>>   {
>> -	u8 pcr_i[TPM_DIGEST_SIZE];
>>   	int rc;
>>
>>   	register_reboot_notifier(&ima_reboot_notifier);
>>
>> -	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..6c9427939a28 100644
>> --- a/security/integrity/ima/ima_queue.c
>> +++ b/security/integrity/ima/ima_queue.c
>> @@ -142,10 +142,13 @@ static int ima_pcr_extend(const u8 *hash, int pcr)
>>   {
>>   	int result = 0;
>>
>> +	down_read(&ima_tpm_chip_lock);
>>   	if (!ima_used_chip)
>> -		return result;
>> +		goto out;
>>
>> -	result = tpm_pcr_extend(NULL, pcr, hash);
>> +	result = tpm_pcr_extend(ima_tpm_chip, pcr, hash);
>> +out:
>> +	up_read(&ima_tpm_chip_lock);
>>   	if (result != 0)
>>   		pr_err("Error Communicating to TPM chip, result: %d\n", result);
>>   	return result;

^ permalink raw reply	[flat|nested] 30+ messages in thread
* Re: [PATCH v2 3/4] ima: Use tpm_chip_find() and access TPM functions using it
  2018-06-21 20:59       ` Stefan Berger
@ 2018-06-22  3:25         ` Jason Gunthorpe
  -1 siblings, 0 replies; 30+ messages in thread
From: Jason Gunthorpe @ 2018-06-22  3:25 UTC (permalink / raw)
  To: Stefan Berger; +Cc: Mimi Zohar, linux-integrity, jarkko.sakkinen, linux-kernel

On Thu, Jun 21, 2018 at 04:59:55PM -0400, Stefan Berger wrote:
> On 06/21/2018 04:53 PM, Mimi Zohar wrote:
> >On Wed, 2018-06-20 at 16:42 -0400, Stefan Berger wrote:
> >>Rather than accessing the TPM functions using a NULL pointer, which
> >>causes a lookup for a suitable chip every time, get a hold of a tpm_chip
> >>and access the TPM functions using this chip. We call the tpm_chip
> >>ima_tpm_chip and protect it, once initialization is done, using a
> >>rw_semaphore called ima_tpm_chip_lock.
> >>
> >>Use ima_shutdown to release the tpm_chip.
> >>
> >>Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> >>  security/integrity/ima/ima.h        |  3 +++
> >>  security/integrity/ima/ima_crypto.c | 12 ++++++++++--
> >>  security/integrity/ima/ima_init.c   | 19 ++++++++++++-------
> >>  security/integrity/ima/ima_queue.c  |  7 +++++--
> >>  4 files changed, 30 insertions(+), 11 deletions(-)
> >>
> >>diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> >>index 354bb5716ce3..53a88d578ca5 100644
> >>+++ b/security/integrity/ima/ima.h
> >>@@ -24,6 +24,7 @@
> >>  #include <linux/hash.h>
> >>  #include <linux/tpm.h>
> >>  #include <linux/audit.h>
> >>+#include <linux/rwsem.h>
> >>  #include <crypto/hash_info.h>
> >>
> >>  #include "../integrity.h"
> >>@@ -56,6 +57,8 @@ extern int ima_policy_flag;
> >>  extern int ima_used_chip;
> >>  extern int ima_hash_algo;
> >>  extern int ima_appraise;
> >>+extern struct rw_semaphore ima_tpm_chip_lock;
> >>+extern struct tpm_chip *ima_tpm_chip;
> >
> >ima_add_templatE_entry() synchronizes appending a measurement to the
> >measurement list and extending the TPM by taking a lock.  Do we really
> >need to introduce another lock?
> 
> This lock protects the ima_tpm_chip from going from != NULL to NULL in the
> ima_shutdown function. Basically, a global pointer accessed by concurrent
> threads should be protected if its value can change. However, in this case
> ima_shutdown would be called so late that there shouldn't be concurrency
> anymore. Though, I found it better to protect it. Maybe someone else has an
> opinion?

Why have a shutdown block? There is no harm in holding a kref if the
machine is shutting down.

Jason

^ permalink raw reply	[flat|nested] 30+ messages in thread
* Re: [PATCH v2 3/4] ima: Use tpm_chip_find() and access TPM functions using it
@ 2018-06-22  3:25         ` Jason Gunthorpe
  0 siblings, 0 replies; 30+ messages in thread
From: Jason Gunthorpe @ 2018-06-22  3:25 UTC (permalink / raw)
  To: Stefan Berger; +Cc: Mimi Zohar, linux-integrity, jarkko.sakkinen, linux-kernel

On Thu, Jun 21, 2018 at 04:59:55PM -0400, Stefan Berger wrote:
> On 06/21/2018 04:53 PM, Mimi Zohar wrote:
> >On Wed, 2018-06-20 at 16:42 -0400, Stefan Berger wrote:
> >>Rather than accessing the TPM functions using a NULL pointer, which
> >>causes a lookup for a suitable chip every time, get a hold of a tpm_chip
> >>and access the TPM functions using this chip. We call the tpm_chip
> >>ima_tpm_chip and protect it, once initialization is done, using a
> >>rw_semaphore called ima_tpm_chip_lock.
> >>
> >>Use ima_shutdown to release the tpm_chip.
> >>
> >>Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> >>  security/integrity/ima/ima.h        |  3 +++
> >>  security/integrity/ima/ima_crypto.c | 12 ++++++++++--
> >>  security/integrity/ima/ima_init.c   | 19 ++++++++++++-------
> >>  security/integrity/ima/ima_queue.c  |  7 +++++--
> >>  4 files changed, 30 insertions(+), 11 deletions(-)
> >>
> >>diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> >>index 354bb5716ce3..53a88d578ca5 100644
> >>+++ b/security/integrity/ima/ima.h
> >>@@ -24,6 +24,7 @@
> >>  #include <linux/hash.h>
> >>  #include <linux/tpm.h>
> >>  #include <linux/audit.h>
> >>+#include <linux/rwsem.h>
> >>  #include <crypto/hash_info.h>
> >>
> >>  #include "../integrity.h"
> >>@@ -56,6 +57,8 @@ extern int ima_policy_flag;
> >>  extern int ima_used_chip;
> >>  extern int ima_hash_algo;
> >>  extern int ima_appraise;
> >>+extern struct rw_semaphore ima_tpm_chip_lock;
> >>+extern struct tpm_chip *ima_tpm_chip;
> >
> >ima_add_templatE_entry() synchronizes appending a measurement to the
> >measurement list and extending the TPM by taking a lock.  Do we really
> >need to introduce another lock?
> 
> This lock protects the ima_tpm_chip from going from != NULL to NULL in the
> ima_shutdown function. Basically, a global pointer accessed by concurrent
> threads should be protected if its value can change. However, in this case
> ima_shutdown would be called so late that there shouldn't be concurrency
> anymore. Though, I found it better to protect it. Maybe someone else has an
> opinion?

Why have a shutdown block? There is no harm in holding a kref if the
machine is shutting down.

Jason

^ permalink raw reply	[flat|nested] 30+ messages in thread
* Re: [PATCH v2 3/4] ima: Use tpm_chip_find() and access TPM functions using it
  2018-06-22  3:25         ` Jason Gunthorpe
@ 2018-06-22 11:40           ` Stefan Berger
  -1 siblings, 0 replies; 30+ messages in thread
From: Stefan Berger @ 2018-06-22 11:40 UTC (permalink / raw)
  To: Jason Gunthorpe
  Cc: Mimi Zohar, linux-integrity, jarkko.sakkinen, linux-kernel

On 06/21/2018 11:25 PM, Jason Gunthorpe wrote:
> On Thu, Jun 21, 2018 at 04:59:55PM -0400, Stefan Berger wrote:
>> On 06/21/2018 04:53 PM, Mimi Zohar wrote:
>>> On Wed, 2018-06-20 at 16:42 -0400, Stefan Berger wrote:
>>>> Rather than accessing the TPM functions using a NULL pointer, which
>>>> causes a lookup for a suitable chip every time, get a hold of a tpm_chip
>>>> and access the TPM functions using this chip. We call the tpm_chip
>>>> ima_tpm_chip and protect it, once initialization is done, using a
>>>> rw_semaphore called ima_tpm_chip_lock.
>>>>
>>>> Use ima_shutdown to release the tpm_chip.
>>>>
>>>> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
>>>>   security/integrity/ima/ima.h        |  3 +++
>>>>   security/integrity/ima/ima_crypto.c | 12 ++++++++++--
>>>>   security/integrity/ima/ima_init.c   | 19 ++++++++++++-------
>>>>   security/integrity/ima/ima_queue.c  |  7 +++++--
>>>>   4 files changed, 30 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
>>>> index 354bb5716ce3..53a88d578ca5 100644
>>>> +++ b/security/integrity/ima/ima.h
>>>> @@ -24,6 +24,7 @@
>>>>   #include <linux/hash.h>
>>>>   #include <linux/tpm.h>
>>>>   #include <linux/audit.h>
>>>> +#include <linux/rwsem.h>
>>>>   #include <crypto/hash_info.h>
>>>>
>>>>   #include "../integrity.h"
>>>> @@ -56,6 +57,8 @@ extern int ima_policy_flag;
>>>>   extern int ima_used_chip;
>>>>   extern int ima_hash_algo;
>>>>   extern int ima_appraise;
>>>> +extern struct rw_semaphore ima_tpm_chip_lock;
>>>> +extern struct tpm_chip *ima_tpm_chip;
>>> ima_add_templatE_entry() synchronizes appending a measurement to the
>>> measurement list and extending the TPM by taking a lock.  Do we really
>>> need to introduce another lock?
>> This lock protects the ima_tpm_chip from going from != NULL to NULL in the
>> ima_shutdown function. Basically, a global pointer accessed by concurrent
>> threads should be protected if its value can change. However, in this case
>> ima_shutdown would be called so late that there shouldn't be concurrency
>> anymore. Though, I found it better to protect it. Maybe someone else has an
>> opinion?
> Why have a shutdown block? There is no harm in holding a kref if the
> machine is shutting down.

Looking around at other drivers' usage of the reboot notifier, I find 
other drivers as well that use spinlocks or mutexes during the shutdown. 
Besides that, we do have the shutdown block already when device_shutdown 
calls tpm_class_shutdown() and we get the ops_sem.

     Stefan
>
> Jason
>


^ permalink raw reply	[flat|nested] 30+ messages in thread
* Re: [PATCH v2 3/4] ima: Use tpm_chip_find() and access TPM functions using it
@ 2018-06-22 11:40           ` Stefan Berger
  0 siblings, 0 replies; 30+ messages in thread
From: Stefan Berger @ 2018-06-22 11:40 UTC (permalink / raw)
  To: Jason Gunthorpe
  Cc: Mimi Zohar, linux-integrity, jarkko.sakkinen, linux-kernel

On 06/21/2018 11:25 PM, Jason Gunthorpe wrote:
> On Thu, Jun 21, 2018 at 04:59:55PM -0400, Stefan Berger wrote:
>> On 06/21/2018 04:53 PM, Mimi Zohar wrote:
>>> On Wed, 2018-06-20 at 16:42 -0400, Stefan Berger wrote:
>>>> Rather than accessing the TPM functions using a NULL pointer, which
>>>> causes a lookup for a suitable chip every time, get a hold of a tpm_chip
>>>> and access the TPM functions using this chip. We call the tpm_chip
>>>> ima_tpm_chip and protect it, once initialization is done, using a
>>>> rw_semaphore called ima_tpm_chip_lock.
>>>>
>>>> Use ima_shutdown to release the tpm_chip.
>>>>
>>>> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
>>>>   security/integrity/ima/ima.h        |  3 +++
>>>>   security/integrity/ima/ima_crypto.c | 12 ++++++++++--
>>>>   security/integrity/ima/ima_init.c   | 19 ++++++++++++-------
>>>>   security/integrity/ima/ima_queue.c  |  7 +++++--
>>>>   4 files changed, 30 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
>>>> index 354bb5716ce3..53a88d578ca5 100644
>>>> +++ b/security/integrity/ima/ima.h
>>>> @@ -24,6 +24,7 @@
>>>>   #include <linux/hash.h>
>>>>   #include <linux/tpm.h>
>>>>   #include <linux/audit.h>
>>>> +#include <linux/rwsem.h>
>>>>   #include <crypto/hash_info.h>
>>>>
>>>>   #include "../integrity.h"
>>>> @@ -56,6 +57,8 @@ extern int ima_policy_flag;
>>>>   extern int ima_used_chip;
>>>>   extern int ima_hash_algo;
>>>>   extern int ima_appraise;
>>>> +extern struct rw_semaphore ima_tpm_chip_lock;
>>>> +extern struct tpm_chip *ima_tpm_chip;
>>> ima_add_templatE_entry() synchronizes appending a measurement to the
>>> measurement list and extending the TPM by taking a lock.  Do we really
>>> need to introduce another lock?
>> This lock protects the ima_tpm_chip from going from != NULL to NULL in the
>> ima_shutdown function. Basically, a global pointer accessed by concurrent
>> threads should be protected if its value can change. However, in this case
>> ima_shutdown would be called so late that there shouldn't be concurrency
>> anymore. Though, I found it better to protect it. Maybe someone else has an
>> opinion?
> Why have a shutdown block? There is no harm in holding a kref if the
> machine is shutting down.

Looking around at other drivers' usage of the reboot notifier, I find 
other drivers as well that use spinlocks or mutexes during the shutdown. 
Besides that, we do have the shutdown block already when device_shutdown 
calls tpm_class_shutdown() and we get the ops_sem.

     Stefan
>
> Jason
>

^ permalink raw reply	[flat|nested] 30+ messages in thread
* Re: [PATCH v2 3/4] ima: Use tpm_chip_find() and access TPM functions using it
  2018-06-22 11:40           ` Stefan Berger
@ 2018-06-22 14:19             ` Jason Gunthorpe
  -1 siblings, 0 replies; 30+ messages in thread
From: Jason Gunthorpe @ 2018-06-22 14:19 UTC (permalink / raw)
  To: Stefan Berger; +Cc: Mimi Zohar, linux-integrity, jarkko.sakkinen, linux-kernel

On Fri, Jun 22, 2018 at 07:40:37AM -0400, Stefan Berger wrote:
> On 06/21/2018 11:25 PM, Jason Gunthorpe wrote:
> >On Thu, Jun 21, 2018 at 04:59:55PM -0400, Stefan Berger wrote:
> >>On 06/21/2018 04:53 PM, Mimi Zohar wrote:
> >>>On Wed, 2018-06-20 at 16:42 -0400, Stefan Berger wrote:
> >>>>Rather than accessing the TPM functions using a NULL pointer, which
> >>>>causes a lookup for a suitable chip every time, get a hold of a tpm_chip
> >>>>and access the TPM functions using this chip. We call the tpm_chip
> >>>>ima_tpm_chip and protect it, once initialization is done, using a
> >>>>rw_semaphore called ima_tpm_chip_lock.
> >>>>
> >>>>Use ima_shutdown to release the tpm_chip.
> >>>>
> >>>>Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> >>>>  security/integrity/ima/ima.h        |  3 +++
> >>>>  security/integrity/ima/ima_crypto.c | 12 ++++++++++--
> >>>>  security/integrity/ima/ima_init.c   | 19 ++++++++++++-------
> >>>>  security/integrity/ima/ima_queue.c  |  7 +++++--
> >>>>  4 files changed, 30 insertions(+), 11 deletions(-)
> >>>>
> >>>>diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> >>>>index 354bb5716ce3..53a88d578ca5 100644
> >>>>+++ b/security/integrity/ima/ima.h
> >>>>@@ -24,6 +24,7 @@
> >>>>  #include <linux/hash.h>
> >>>>  #include <linux/tpm.h>
> >>>>  #include <linux/audit.h>
> >>>>+#include <linux/rwsem.h>
> >>>>  #include <crypto/hash_info.h>
> >>>>
> >>>>  #include "../integrity.h"
> >>>>@@ -56,6 +57,8 @@ extern int ima_policy_flag;
> >>>>  extern int ima_used_chip;
> >>>>  extern int ima_hash_algo;
> >>>>  extern int ima_appraise;
> >>>>+extern struct rw_semaphore ima_tpm_chip_lock;
> >>>>+extern struct tpm_chip *ima_tpm_chip;
> >>>ima_add_templatE_entry() synchronizes appending a measurement to the
> >>>measurement list and extending the TPM by taking a lock.  Do we really
> >>>need to introduce another lock?
> >>This lock protects the ima_tpm_chip from going from != NULL to NULL in the
> >>ima_shutdown function. Basically, a global pointer accessed by concurrent
> >>threads should be protected if its value can change. However, in this case
> >>ima_shutdown would be called so late that there shouldn't be concurrency
> >>anymore. Though, I found it better to protect it. Maybe someone else has an
> >>opinion?
> >Why have a shutdown block? There is no harm in holding a kref if the
> >machine is shutting down.
> 
> Looking around at other drivers' usage of the reboot notifier, I find other
> drivers as well that use spinlocks or mutexes during the shutdown. Besides
> that, we do have the shutdown block already when device_shutdown calls
> tpm_class_shutdown() and we get the ops_sem.

But the shutdown handler in TPM an actual purpose, we are doing
something to the persistent state in the TPM itself during shutdown.

I can't see why IMA needs a shutdown handler. You shouldn't add one
'just because'

Jason

^ permalink raw reply	[flat|nested] 30+ messages in thread
* Re: [PATCH v2 3/4] ima: Use tpm_chip_find() and access TPM functions using it
@ 2018-06-22 14:19             ` Jason Gunthorpe
  0 siblings, 0 replies; 30+ messages in thread
From: Jason Gunthorpe @ 2018-06-22 14:19 UTC (permalink / raw)
  To: Stefan Berger; +Cc: Mimi Zohar, linux-integrity, jarkko.sakkinen, linux-kernel

On Fri, Jun 22, 2018 at 07:40:37AM -0400, Stefan Berger wrote:
> On 06/21/2018 11:25 PM, Jason Gunthorpe wrote:
> >On Thu, Jun 21, 2018 at 04:59:55PM -0400, Stefan Berger wrote:
> >>On 06/21/2018 04:53 PM, Mimi Zohar wrote:
> >>>On Wed, 2018-06-20 at 16:42 -0400, Stefan Berger wrote:
> >>>>Rather than accessing the TPM functions using a NULL pointer, which
> >>>>causes a lookup for a suitable chip every time, get a hold of a tpm_chip
> >>>>and access the TPM functions using this chip. We call the tpm_chip
> >>>>ima_tpm_chip and protect it, once initialization is done, using a
> >>>>rw_semaphore called ima_tpm_chip_lock.
> >>>>
> >>>>Use ima_shutdown to release the tpm_chip.
> >>>>
> >>>>Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> >>>>  security/integrity/ima/ima.h        |  3 +++
> >>>>  security/integrity/ima/ima_crypto.c | 12 ++++++++++--
> >>>>  security/integrity/ima/ima_init.c   | 19 ++++++++++++-------
> >>>>  security/integrity/ima/ima_queue.c  |  7 +++++--
> >>>>  4 files changed, 30 insertions(+), 11 deletions(-)
> >>>>
> >>>>diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> >>>>index 354bb5716ce3..53a88d578ca5 100644
> >>>>+++ b/security/integrity/ima/ima.h
> >>>>@@ -24,6 +24,7 @@
> >>>>  #include <linux/hash.h>
> >>>>  #include <linux/tpm.h>
> >>>>  #include <linux/audit.h>
> >>>>+#include <linux/rwsem.h>
> >>>>  #include <crypto/hash_info.h>
> >>>>
> >>>>  #include "../integrity.h"
> >>>>@@ -56,6 +57,8 @@ extern int ima_policy_flag;
> >>>>  extern int ima_used_chip;
> >>>>  extern int ima_hash_algo;
> >>>>  extern int ima_appraise;
> >>>>+extern struct rw_semaphore ima_tpm_chip_lock;
> >>>>+extern struct tpm_chip *ima_tpm_chip;
> >>>ima_add_templatE_entry() synchronizes appending a measurement to the
> >>>measurement list and extending the TPM by taking a lock.  Do we really
> >>>need to introduce another lock?
> >>This lock protects the ima_tpm_chip from going from != NULL to NULL in the
> >>ima_shutdown function. Basically, a global pointer accessed by concurrent
> >>threads should be protected if its value can change. However, in this case
> >>ima_shutdown would be called so late that there shouldn't be concurrency
> >>anymore. Though, I found it better to protect it. Maybe someone else has an
> >>opinion?
> >Why have a shutdown block? There is no harm in holding a kref if the
> >machine is shutting down.
> 
> Looking around at other drivers' usage of the reboot notifier, I find other
> drivers as well that use spinlocks or mutexes during the shutdown. Besides
> that, we do have the shutdown block already when device_shutdown calls
> tpm_class_shutdown() and we get the ops_sem.

But the shutdown handler in TPM an actual purpose, we are doing
something to the persistent state in the TPM itself during shutdown.

I can't see why IMA needs a shutdown handler. You shouldn't add one
'just because'

Jason

^ permalink raw reply	[flat|nested] 30+ messages in thread
end of thread, other threads:[~2018-06-22 14:20 UTC | newest]

Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-06-20 20:42 [PATCH v2 0/4] Have IMA find and use a tpm_chip until system shutdown Stefan Berger
2018-06-20 20:42 ` [PATCH v2 1/4] tpm: Implement tpm_chip_find() and tpm_chip_put() for other subsystems Stefan Berger
2018-06-20 20:50   ` Jason Gunthorpe
2018-06-20 21:13     ` Stefan Berger
2018-06-20 21:13       ` Stefan Berger
2018-06-21 17:13     ` Jarkko Sakkinen
2018-06-21 17:15   ` Jarkko Sakkinen
2018-06-21 17:27     ` Stefan Berger
2018-06-21 17:27       ` Stefan Berger
2018-06-21 17:45     ` Stefan Berger
2018-06-21 17:45       ` Stefan Berger
2018-06-21 17:56       ` Jason Gunthorpe
2018-06-21 18:19         ` Stefan Berger
2018-06-21 18:19           ` Stefan Berger
2018-06-21 19:06           ` Jason Gunthorpe
2018-06-21 20:14             ` Stefan Berger
2018-06-21 20:51               ` Jason Gunthorpe
2018-06-20 20:42 ` [PATCH v2 2/4] ima: Implement ima_shutdown and register it as a reboot_notifier Stefan Berger
2018-06-20 20:42 ` [PATCH v2 3/4] ima: Use tpm_chip_find() and access TPM functions using it Stefan Berger
2018-06-21 20:53   ` Mimi Zohar
2018-06-21 20:53     ` Mimi Zohar
2018-06-21 20:59     ` Stefan Berger
2018-06-21 20:59       ` Stefan Berger
2018-06-22  3:25       ` Jason Gunthorpe
2018-06-22  3:25         ` Jason Gunthorpe
2018-06-22 11:40         ` Stefan Berger
2018-06-22 11:40           ` Stefan Berger
2018-06-22 14:19           ` Jason Gunthorpe
2018-06-22 14:19             ` Jason Gunthorpe
2018-06-20 20:42 ` [PATCH v2 4/4] ima: Get rid of ima_used_chip and use ima_tpm_chip != NULL instead Stefan Berger

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.