From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751500AbdFFIwp (ORCPT ); Tue, 6 Jun 2017 04:52:45 -0400 Received: from lhrrgout.huawei.com ([194.213.3.17]:28059 "EHLO lhrrgout.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751476AbdFFIwn (ORCPT ); Tue, 6 Jun 2017 04:52:43 -0400 Subject: Re: [Linux-ima-devel] [PATCH 5/7] ima: add securityfs interface to save a measurements list with kexec header To: Mimi Zohar , References: <20170516125347.10574-1-roberto.sassu@huawei.com> <20170516125347.10574-6-roberto.sassu@huawei.com> <1496642698.2998.104.camel@linux.vnet.ibm.com> CC: , From: Roberto Sassu Message-ID: Date: Tue, 6 Jun 2017 10:49:37 +0200 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.7.1 MIME-Version: 1.0 In-Reply-To: <1496642698.2998.104.camel@linux.vnet.ibm.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [10.220.96.113] X-CFilter-Loop: Reflected X-Mirapoint-Virus-RAPID-Raw: score=unknown(0), refid=str=0001.0A020202.59366CAD.00F5,ss=1,re=0.000,recu=0.000,reip=0.000,cl=1,cld=1,fgs=0, ip=0.0.0.0, so=2013-06-18 04:22:30, dmn=2013-03-21 17:37:32 X-Mirapoint-Loop-Id: 08efd0990a412bd164691fe12ee048dc Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 6/5/2017 8:04 AM, Mimi Zohar wrote: > On Tue, 2017-05-16 at 14:53 +0200, Roberto Sassu wrote: >> Through the new interface binary_kexec_runtime_measurements, it will be >> possible to read the same content returned by binary_runtime_measurements, >> with the kexec header prepended. >> >> The new interface has been added for testing ima_restore_measurement_list() >> which, at the moment, works only on PPC systems. The interface for reading >> the binary list with the kexec header will be provided in a separate patch. >> >> The patch reuses ima_measurements_start() and ima_measurements_next() >> to send the measurements list to userspace. Their behavior changes >> depending on the current dentry. >> >> To provide the correct information in the kexec header, >> ima_measurements_start() has to iterate over the whole list and calculate >> the number of entries and the total size. It is not possible to read >> the value of the global variable binary_runtime_size and ima_htable.len >> without taking ima_extend_list_mutex, because there might have been a list >> add between the two read operations. > > Please correct me if I'm wrong. Your code walks the measurement list > calculating the total number of measurements and the memory size > needed to store in the kexec header. Can't there be additional > measurements between the time these values - total number of > measurements and memory needed - were calculated and actually saving > the measurements? How would that be any different than the problem > you're trying to solve? In both cases, the number of measurements > might be less than the actual number of measurements. > > As long as you query the number of measurements before getting the > memory needed, unless you're trying to verify a TPM quote, having > fewer measurements shouldn't be a problem for testing. The problem is that the total number of entries and the required memory size might be inconsistent without taking ima_extend_list_mutex. ima_measurements_start() could read the entries counter before it is incremented by ima_add_digest_entry() and the required memory size after it is updated. If this happens, the parser returns an error because ENFORCE_BUFEND is set for the last entry and there would be still data to read (the new entry added to the list). Roberto > >> >> Signed-off-by: Roberto Sassu >> --- >> security/integrity/ima/Kconfig | 8 ++++++ >> security/integrity/ima/ima.h | 2 ++ >> security/integrity/ima/ima_fs.c | 42 ++++++++++++++++++++++++++++--- >> security/integrity/ima/ima_kexec.c | 2 +- >> security/integrity/ima/ima_template.c | 2 +- >> security/integrity/ima/ima_template_lib.c | 19 ++++++++++++++ >> security/integrity/ima/ima_template_lib.h | 2 ++ >> 7 files changed, 71 insertions(+), 6 deletions(-) >> >> diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig >> index 370eb2f..0f60c04 100644 >> --- a/security/integrity/ima/Kconfig >> +++ b/security/integrity/ima/Kconfig >> @@ -39,6 +39,14 @@ config IMA_KEXEC >> Depending on the IMA policy, the measurement list can grow to >> be very large. >> >> +config IMA_KEXEC_TESTING >> + bool "Enable securityfs interfaces to save/restore measurement list" >> + depends on IMA >> + default n >> + help >> + Use binary_kexec_runtime_measurements to save the binary list >> + with the kexec header; use restore_kexec_list to restore a list. >> + >> config IMA_MEASURE_PCR_IDX >> int >> depends on IMA >> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h >> index 10ef9c8..416497b 100644 >> --- a/security/integrity/ima/ima.h >> +++ b/security/integrity/ima/ima.h >> @@ -49,6 +49,8 @@ enum tpm_pcrs { TPM_PCR0 = 0, TPM_PCR8 = 8 }; >> #define IMA_TEMPLATE_IMA_NAME "ima" >> #define IMA_TEMPLATE_IMA_FMT "d|n" >> >> +#define IMA_KEXEC_HDR_VERSION 1 >> + >> /* current content of the policy */ >> extern int ima_policy_flag; >> >> diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c >> index ca303e5..a93f941 100644 >> --- a/security/integrity/ima/ima_fs.c >> +++ b/security/integrity/ima/ima_fs.c >> @@ -25,6 +25,7 @@ >> #include >> >> #include "ima.h" >> +#include "ima_template_lib.h" >> >> static DEFINE_MUTEX(ima_write_mutex); >> >> @@ -75,28 +76,52 @@ static const struct file_operations ima_measurements_count_ops = { >> .llseek = generic_file_llseek, >> }; >> >> +static struct dentry *binary_kexec_runtime_measurements; >> + >> /* returns pointer to hlist_node */ >> static void *ima_measurements_start(struct seq_file *m, loff_t *pos) >> { >> loff_t l = *pos; >> struct ima_queue_entry *qe; >> + struct ima_queue_entry *qe_found = NULL; >> + unsigned long size = 0, count = 0; >> + bool khdr = m->file->f_path.dentry == binary_kexec_runtime_measurements; >> >> /* we need a lock since pos could point beyond last element */ >> rcu_read_lock(); >> list_for_each_entry_rcu(qe, &ima_measurements, later) { >> - if (!l--) { >> - rcu_read_unlock(); >> - return qe; >> + if (!l) { >> + qe_found = qe_found ? qe_found : qe; > > What is this? > >> + >> + if (!khdr) >> + break; > > Does this test need to be in the loop? > > Mimi > >> + >> + if (*pos) >> + break; >> + >> + size += ima_get_template_entry_size(qe->entry); >> + count++; >> + m->private = qe; >> + continue; >> } >> + l--; >> } >> rcu_read_unlock(); >> - return NULL; >> + >> + if (khdr && size) >> + ima_show_kexec_hdr(m, count, size); >> + >> + return qe_found; >> } >> >> static void *ima_measurements_next(struct seq_file *m, void *v, loff_t *pos) >> { >> + bool khdr = m->file->f_path.dentry == binary_kexec_runtime_measurements; >> struct ima_queue_entry *qe = v; >> >> + if (khdr && qe == m->private) >> + return NULL; >> + >> /* lock protects when reading beyond last element >> * against concurrent list-extension >> */ >> @@ -490,8 +515,17 @@ int __init ima_fs_init(void) >> if (IS_ERR(ima_policy)) >> goto out; >> >> +#ifdef CONFIG_IMA_KEXEC_TESTING >> + binary_kexec_runtime_measurements = >> + securityfs_create_file("binary_kexec_runtime_measurements", >> + S_IRUSR | S_IRGRP, ima_dir, NULL, >> + &ima_measurements_ops); >> + if (IS_ERR(binary_kexec_runtime_measurements)) >> + goto out; >> +#endif >> return 0; >> out: >> + securityfs_remove(binary_kexec_runtime_measurements); >> securityfs_remove(violations); >> securityfs_remove(runtime_measurements_count); >> securityfs_remove(ascii_runtime_measurements); >> diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c >> index e473eee..b0b8ed2 100644 >> --- a/security/integrity/ima/ima_kexec.c >> +++ b/security/integrity/ima/ima_kexec.c >> @@ -36,7 +36,7 @@ static int ima_dump_measurement_list(unsigned long *buffer_size, void **buffer, >> file.count = sizeof(khdr); /* reserved space */ >> >> memset(&khdr, 0, sizeof(khdr)); >> - khdr.version = 1; >> + khdr.version = IMA_KEXEC_HDR_VERSION; >> list_for_each_entry_rcu(qe, &ima_measurements, later) { >> if (file.count < file.size) { >> khdr.count++; >> diff --git a/security/integrity/ima/ima_template.c b/security/integrity/ima/ima_template.c >> index 7412d02..f86456c 100644 >> --- a/security/integrity/ima/ima_template.c >> +++ b/security/integrity/ima/ima_template.c >> @@ -347,7 +347,7 @@ int ima_restore_measurement_list(loff_t size, void *buf) >> khdr->buffer_size = le64_to_cpu(khdr->buffer_size); >> } >> >> - if (khdr->version != 1) { >> + if (khdr->version != IMA_KEXEC_HDR_VERSION) { >> pr_err("attempting to restore a incompatible measurement list"); >> return -EINVAL; >> } >> diff --git a/security/integrity/ima/ima_template_lib.c b/security/integrity/ima/ima_template_lib.c >> index 28af43f..de2b064 100644 >> --- a/security/integrity/ima/ima_template_lib.c >> +++ b/security/integrity/ima/ima_template_lib.c >> @@ -159,6 +159,25 @@ void ima_show_template_sig(struct seq_file *m, enum ima_show_type show, >> ima_show_template_field_data(m, show, DATA_FMT_HEX, field_data); >> } >> >> +void ima_show_kexec_hdr(struct seq_file *m, unsigned long count, >> + unsigned long size) >> +{ >> + struct ima_kexec_hdr khdr; >> + >> + memset(&khdr, 0, sizeof(khdr)); >> + khdr.version = IMA_KEXEC_HDR_VERSION; >> + khdr.count = count; >> + khdr.buffer_size = sizeof(khdr) + size; >> + >> + if (ima_canonical_fmt) { >> + khdr.version = cpu_to_le16(khdr.version); >> + khdr.count = cpu_to_le64(khdr.count); >> + khdr.buffer_size = cpu_to_le64(khdr.buffer_size); >> + } >> + >> + ima_putc(m, &khdr, sizeof(khdr)); >> +} >> + >> /** >> * ima_parse_buf() - Parses lengths and data from an input buffer >> * @bufstartp: Buffer start address. >> diff --git a/security/integrity/ima/ima_template_lib.h b/security/integrity/ima/ima_template_lib.h >> index 6a3d8b8..069e4ba 100644 >> --- a/security/integrity/ima/ima_template_lib.h >> +++ b/security/integrity/ima/ima_template_lib.h >> @@ -29,6 +29,8 @@ void ima_show_template_string(struct seq_file *m, enum ima_show_type show, >> struct ima_field_data *field_data); >> void ima_show_template_sig(struct seq_file *m, enum ima_show_type show, >> struct ima_field_data *field_data); >> +void ima_show_kexec_hdr(struct seq_file *m, unsigned long count, >> + unsigned long size); >> int ima_parse_buf(void *bufstartp, void *bufendp, void **bufcurp, >> int maxfields, struct ima_field_data *fields, int *curfields, >> unsigned long *len_mask, int enforce_mask, char *bufname); > -- HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063 Managing Director: Bo PENG, Qiuen PENG, Shengli WANG