| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <f4af7347-a9dd-1683-5c2c-faeb45750e2e@linux.vnet.ibm.com>
Date: Thu, 21 Jun 2018 16:59:55 -0400
From: Stefan Berger <stefanb@...ux.vnet.ibm.com>
To: Mimi Zohar <zohar@...ux.vnet.ibm.com>,
linux-integrity@...r.kernel.org, jarkko.sakkinen@...ux.intel.com
Cc: jgg@...pe.ca, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 3/4] ima: Use tpm_chip_find() and access TPM functions
using it
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@...ux.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;
Powered by blists - more mailing lists