Messages in this thread | | | Subject | Re: [PATCH v2 3/4] ima: Use tpm_chip_find() and access TPM functions using it | From | Stefan Berger <> | Date | Thu, 21 Jun 2018 16:59:55 -0400 |
| |
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;
| |