Re: [PATCH v2 3/4] ima: Use tpm_chip_find() and access TPM functions using it

[Stefan Berger <stefanb@xxxxxxxxxxxxxxxxxx>]

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@xxxxxxxxxxxxxxxxxx>
> > ---
> >   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;