| Message ID | 20200414115020.99288-1-tianjia.zhang@linux.alibaba.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ima: optimize ima_pcr_extend function by asynchronous | expand |
[Cc'ing Ken Goldman, Monty Wiseman, and Dave Safford] On Tue, 2020-04-14 at 19:50 +0800, Tianjia Zhang wrote: > Because ima_pcr_extend() to operate the TPM chip, this process is > very time-consuming, for IMA, this is a blocking action, especially > when the TPM is in self test state, this process will block for up > to ten seconds. > > Because the return result of ima_pcr_extend() is of no concern to IMA, > it only affects the audit of IMA, so this patch use async_schedule() > to asynchronously perform the ima_pcr_extend() operation and do an > audit operation at the end. > > In a vtpm scenario, I added the measure policy of BPRM and MMAP to > compare the efficiency before and after applying the patch. The results > show that the overall startup efficiency of conventional processes can > be increased by 5% to 10%. I believe this efficiency increase It will > be more obvious on real hardware tpm. Yes, we're fully aware that extending the TPM PCR takes a long time. That is the reason for a lot of Nayna Jain's and my work on improving the TPM performance. At one point, I implemented queueing the measurements without waiting for the measurements to extend the TPM. The performance was absolutely amazing, but not waiting for the TPM extend to complete violates the trusted boot principle of measuring and extending the TPM PCR before use. Secondly, the IMA measurement list order and the order in which the measurements extend the TPM is really important in order to be able to validate the IMA measurement list against the TPM PCR quote. One solution that we've considered is batching the measurements, so that the TPM PCR is extended with the hash of the batched measurements, instead of each measurement. The IMA measurement list would continue to contain the individual measurements, but would also need to indicate start/stop of the batched measurement group. None of this is trivial. Mimi
I wonder if there's a different issue? I just ran selftest with fullTest = yes in two different TPM vendors. One took 230 msec, the other 320 msec. I've never seen anything near 10 seconds. Note that this is worse than the worst case because it's forcing a full retest. The TPM typically starts its self test immediately at power up and could be complete by the time the OS starts to boot. When I run selftest with fullTest = no, I get 30 msec, probably because it's not doing anything. On 4/14/2020 7:50 AM, Tianjia Zhang wrote: > Because ima_pcr_extend() to operate the TPM chip, this process is > very time-consuming, for IMA, this is a blocking action, especially > when the TPM is in self test state, this process will block for up > to ten seconds.
On 2020/4/15 2:07, Ken Goldman wrote: > I wonder if there's a different issue? I just ran selftest with > fullTest = yes in two different TPM vendors. > > One took 230 msec, the other 320 msec. > > I've never seen anything near 10 seconds. > > Note that this is worse than the worst case because it's forcing a full > retest. The TPM typically starts its self test immediately at power up > and could be complete by the time the OS starts to boot. > > When I run selftest with fullTest = no, I get 30 msec, probably > because it's not doing anything. > > On 4/14/2020 7:50 AM, Tianjia Zhang wrote: >> Because ima_pcr_extend() to operate the TPM chip, this process is >> very time-consuming, for IMA, this is a blocking action, especially >> when the TPM is in self test state, this process will block for up >> to ten seconds. > Ten seconds is an extreme scenario, and I haven't seen this worst case, but the TPM driver will fail to return in this scenario. Thanks and best, Tianjia
diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c index 8753212ddb18..12cbf69c2442 100644 --- a/security/integrity/ima/ima_queue.c +++ b/security/integrity/ima/ima_queue.c @@ -17,6 +17,7 @@ #include <linux/rculist.h> #include <linux/slab.h> +#include <linux/async.h> #include "ima.h" #define AUDIT_CAUSE_LEN_MAX 32 @@ -151,6 +152,42 @@ static int ima_pcr_extend(const u8 *hash, int pcr) return result; } +struct ima_pcr_extend_async_ctx { + struct ima_template_entry *entry; + int violation; + const char *op; + struct inode *inode; + const unsigned char *filename; + const char *audit_cause; + int audit_info; + int result; +}; + +static void ima_pcr_extend_async(void *data, async_cookie_t cookie) +{ + struct ima_pcr_extend_async_ctx *ctx = data; + u8 digest[TPM_DIGEST_SIZE]; + char tpm_audit_cause[AUDIT_CAUSE_LEN_MAX]; + int result; + + if (ctx->violation) /* invalidate pcr */ + memset(digest, 0xff, sizeof(digest)); + else + memcpy(digest, ctx->entry->digest, sizeof(digest)); + + result = ima_pcr_extend(digest, ctx->entry->pcr); + if (result != 0) { + snprintf(tpm_audit_cause, AUDIT_CAUSE_LEN_MAX, "TPM_error(%d)", + result); + ctx->audit_cause = tpm_audit_cause; + ctx->audit_info = 0; + } + integrity_audit_msg(AUDIT_INTEGRITY_PCR, ctx->inode, ctx->filename, + ctx->op, ctx->audit_cause, ctx->result, ctx->audit_info); + + kfree(ctx); +} + /* * Add template entry to the measurement list and hash table, and * extend the pcr. @@ -163,20 +200,16 @@ int ima_add_template_entry(struct ima_template_entry *entry, int violation, const char *op, struct inode *inode, const unsigned char *filename) { - u8 digest[TPM_DIGEST_SIZE]; const char *audit_cause = "hash_added"; - char tpm_audit_cause[AUDIT_CAUSE_LEN_MAX]; int audit_info = 1; - int result = 0, tpmresult = 0; + int result = 0; + struct ima_pcr_extend_async_ctx *ctx; mutex_lock(&ima_extend_list_mutex); - if (!violation) { - memcpy(digest, entry->digest, sizeof(digest)); - if (ima_lookup_digest_entry(digest, entry->pcr)) { - audit_cause = "hash_exists"; - result = -EEXIST; - goto out; - } + if (!violation && ima_lookup_digest_entry(entry->digest, entry->pcr)) { + audit_cause = "hash_exists"; + result = -EEXIST; + goto out; } result = ima_add_digest_entry(entry, 1); @@ -186,20 +219,25 @@ int ima_add_template_entry(struct ima_template_entry *entry, int violation, goto out; } - if (violation) /* invalidate pcr */ - memset(digest, 0xff, sizeof(digest)); - - tpmresult = ima_pcr_extend(digest, entry->pcr); - if (tpmresult != 0) { - snprintf(tpm_audit_cause, AUDIT_CAUSE_LEN_MAX, "TPM_error(%d)", - tpmresult); - audit_cause = tpm_audit_cause; - audit_info = 0; + ctx = kmalloc(sizeof(*ctx), GFP_KERNEL); + if (!ctx) { + audit_cause = "ENOMEM"; + result = -ENOMEM; + goto out; } + + ctx->entry = entry; + ctx->violation = violation; + ctx->op = op; + ctx->inode = inode; + ctx->filename = filename; + ctx->audit_cause = audit_cause; + ctx->audit_info = audit_info; + ctx->result = result; + async_schedule(ima_pcr_extend_async, ctx); + out: mutex_unlock(&ima_extend_list_mutex); - integrity_audit_msg(AUDIT_INTEGRITY_PCR, inode, filename, - op, audit_cause, result, audit_info); return result; }
Because ima_pcr_extend() to operate the TPM chip, this process is very time-consuming, for IMA, this is a blocking action, especially when the TPM is in self test state, this process will block for up to ten seconds. Because the return result of ima_pcr_extend() is of no concern to IMA, it only affects the audit of IMA, so this patch use async_schedule() to asynchronously perform the ima_pcr_extend() operation and do an audit operation at the end. In a vtpm scenario, I added the measure policy of BPRM and MMAP to compare the efficiency before and after applying the patch. The results show that the overall startup efficiency of conventional processes can be increased by 5% to 10%. I believe this efficiency increase It will be more obvious on real hardware tpm. Signed-off-by: Tianjia Zhang <tianjia.zhang@linux.alibaba.com> --- security/integrity/ima/ima_queue.c | 80 ++++++++++++++++++++++-------- 1 file changed, 59 insertions(+), 21 deletions(-)