| Message ID | 20170516125347.10574-6-roberto.sassu@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> > --- > 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 <linux/vmalloc.h> > > #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); -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <roberto.sassu@huawei.com> >> --- >> 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 <linux/vmalloc.h> >> >> #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); >
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. > >> >> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> >> --- >> 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 <linux/vmalloc.h> >> >> #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? ima_measurements_start() should return the list entry at position *pos. The line above prevents qe_found from being updated when the loop continues until the last list entry. > >> + >> + if (!khdr) >> + break; > > Does this test need to be in the loop? Yes. Otherwise, ima_measurements_start() would iterate over the whole list when it is not necessary. Roberto > > 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); >
On Tue, 2017-06-06 at 10:49 +0200, Roberto Sassu wrote: > 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). I don't see this as being any different than what happens when the kernel saves the measurement list. Originally, the memory size was defined at kexec load, but only populated at kexec execute. There was plenty of time between the kexec load and execute for additional measurement records to be added. The upstreamed version defines the buffer size and populates it at kexec load. However kexec load itself generates additional measurements, so it has to reserve more memory than what is returned by ima_get_binary_runtime_size(). (Refer to ima_add_kexec_buffer.) When restoring the buffer, it processes the number of records as stored in the header, making sure it doesn't go beyond the buffer size. Mimi -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2017-06-06 at 11:13 +0200, Roberto Sassu wrote: > >> /* 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? > > ima_measurements_start() should return the list entry at position *pos. > The line above prevents qe_found from being updated when the loop > continues until the last list entry. Wouldn't a simple if/then be more appropriate here? > > > > >> + > >> + if (!khdr) > >> + break; > > > > Does this test need to be in the loop? > > Yes. Otherwise, ima_measurements_start() would iterate over the whole > list when it is not necessary. Oh, for displaying the measurement list you need to set qe_found before returning. thanks, Mimi -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 6/6/2017 12:56 PM, Mimi Zohar wrote: > On Tue, 2017-06-06 at 10:49 +0200, Roberto Sassu wrote: >> 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). > > I don't see this as being any different than what happens when the > kernel saves the measurement list. Originally, the memory size was > defined at kexec load, but only populated at kexec execute. There was > plenty of time between the kexec load and execute for additional > measurement records to be added. > > The upstreamed version defines the buffer size and populates it at > kexec load. However kexec load itself generates additional > measurements, so it has to reserve more memory than what is returned > by ima_get_binary_runtime_size(). (Refer to ima_add_kexec_buffer.) ima_dump_measurement_list() determines the total number of entries and the required memory size (which are written to the kexec header) after iterating over the whole list. Are new entries added to the kexec buffer after ima_dump_measurement_list() is called? Roberto > > When restoring the buffer, it processes the number of records as > stored in the header, making sure it doesn't go beyond the buffer > size. > > Mimi >
On Tue, 2017-06-06 at 14:45 +0200, Roberto Sassu wrote: > On 6/6/2017 12:56 PM, Mimi Zohar wrote: > > On Tue, 2017-06-06 at 10:49 +0200, Roberto Sassu wrote: > >> 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). > > > > I don't see this as being any different than what happens when the > > kernel saves the measurement list. Originally, the memory size was > > defined at kexec load, but only populated at kexec execute. There was > > plenty of time between the kexec load and execute for additional > > measurement records to be added. > > > > The upstreamed version defines the buffer size and populates it at > > kexec load. However kexec load itself generates additional > > measurements, so it has to reserve more memory than what is returned > > by ima_get_binary_runtime_size(). (Refer to ima_add_kexec_buffer.) > > ima_dump_measurement_list() determines the total number of entries and > the required memory size (which are written to the kexec header) after > iterating over the whole list. Are new entries added to the kexec buffer > after ima_dump_measurement_list() is called? The upstreamed version allocates the segment in kexec load and then fills the buffer. However, in between getting the current memory size needed and filling the buffer, additional measurements can be added. Thus the segment size needs to be larger than the current memory size. The header reflects the number of measurements and the actual buffer size, not the segment size. When restoring the measurement list, however, we rely on the number of measurements and use the buffer size as a reference to prevent accessing memory beyond the buffer. The buffer size does not need to be exact. ** Note ** This upstream version, which doesn't update the measurement list in kexec execute, requires all files to be pre-measured, before calling kexec load. Otherwise, the measurement list will not validate against the quoted TPM PCRs. Mimi -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 6/6/2017 3:23 PM, Mimi Zohar wrote: > On Tue, 2017-06-06 at 14:45 +0200, Roberto Sassu wrote: >> On 6/6/2017 12:56 PM, Mimi Zohar wrote: >>> On Tue, 2017-06-06 at 10:49 +0200, Roberto Sassu wrote: >>>> 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). >>> >>> I don't see this as being any different than what happens when the >>> kernel saves the measurement list. Originally, the memory size was >>> defined at kexec load, but only populated at kexec execute. There was >>> plenty of time between the kexec load and execute for additional >>> measurement records to be added. >>> >>> The upstreamed version defines the buffer size and populates it at >>> kexec load. However kexec load itself generates additional >>> measurements, so it has to reserve more memory than what is returned >>> by ima_get_binary_runtime_size(). (Refer to ima_add_kexec_buffer.) >> >> ima_dump_measurement_list() determines the total number of entries and >> the required memory size (which are written to the kexec header) after >> iterating over the whole list. Are new entries added to the kexec buffer >> after ima_dump_measurement_list() is called? > > The upstreamed version allocates the segment in kexec load and then > fills the buffer. However, in between getting the current memory size > needed and filling the buffer, additional measurements can be added. > Thus the segment size needs to be larger than the current memory > size. > > The header reflects the number of measurements and the actual buffer > size, not the segment size. When restoring the measurement list, > however, we rely on the number of measurements and use the buffer size > as a reference to prevent accessing memory beyond the buffer. The > buffer size does not need to be exact. In this case, I have to modify patch 2/7 and remove ENFORCE_BUFEND from the enforcing mask. Otherwise, ima_restore_measurement_list() would return an error when parsing the last entry and buffer size in the kexec header is greater than the exact size required to store the measurements list. Should I just send the modified patch with the [RESEND] tag or should I send the whole patch set with an incremented version number? Also, since patches 4-6 are for testing, I would prefer to skip them for now and push a new version of the patch set for the Crypto Agile format first. Thanks Roberto
On Tue, 2017-06-13 at 09:27 +0200, Roberto Sassu wrote: > On 6/6/2017 3:23 PM, Mimi Zohar wrote: > > On Tue, 2017-06-06 at 14:45 +0200, Roberto Sassu wrote: > >> On 6/6/2017 12:56 PM, Mimi Zohar wrote: > >>> On Tue, 2017-06-06 at 10:49 +0200, Roberto Sassu wrote: > >>>> 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). > >>> > >>> I don't see this as being any different than what happens when the > >>> kernel saves the measurement list. Originally, the memory size was > >>> defined at kexec load, but only populated at kexec execute. There was > >>> plenty of time between the kexec load and execute for additional > >>> measurement records to be added. > >>> > >>> The upstreamed version defines the buffer size and populates it at > >>> kexec load. However kexec load itself generates additional > >>> measurements, so it has to reserve more memory than what is returned > >>> by ima_get_binary_runtime_size(). (Refer to ima_add_kexec_buffer.) > >> > >> ima_dump_measurement_list() determines the total number of entries and > >> the required memory size (which are written to the kexec header) after > >> iterating over the whole list. Are new entries added to the kexec buffer > >> after ima_dump_measurement_list() is called? > > > > The upstreamed version allocates the segment in kexec load and then > > fills the buffer. However, in between getting the current memory size > > needed and filling the buffer, additional measurements can be added. > > Thus the segment size needs to be larger than the current memory > > size. > > > > The header reflects the number of measurements and the actual buffer > > size, not the segment size. When restoring the measurement list, > > however, we rely on the number of measurements and use the buffer size > > as a reference to prevent accessing memory beyond the buffer. The > > buffer size does not need to be exact. > > In this case, I have to modify patch 2/7 and remove ENFORCE_BUFEND > from the enforcing mask. Otherwise, ima_restore_measurement_list() > would return an error when parsing the last entry and buffer size > in the kexec header is greater than the exact size required to > store the measurements list. Or the testing patches could relax the ENFORCE_BUFEND requirement. Without the ENFORCE_BUFEND requirement, I'm not sure this patch (5/7) will be needed. > Should I just send the modified patch with the [RESEND] tag > or should I send the whole patch set with an incremented version > number? The testing patches could be upstreamed separately, if you prefer. > Also, since patches 4-6 are for testing, I would prefer to skip > them for now and push a new version of the patch set for the > Crypto Agile format first? That's fine. I've pushed patches 1 - 3, and 7 to the next-4.12- ima_parse_buf branch for a day or so, before staging them in the next branch to be upstreamed. The patches can be found here https://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux- integrity.git/next-4.12-ima_parse_buf. Mimi -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 6/13/2017 2:09 PM, Mimi Zohar wrote: > On Tue, 2017-06-13 at 09:27 +0200, Roberto Sassu wrote: >> On 6/6/2017 3:23 PM, Mimi Zohar wrote: >>> On Tue, 2017-06-06 at 14:45 +0200, Roberto Sassu wrote: >>>> On 6/6/2017 12:56 PM, Mimi Zohar wrote: >>>>> On Tue, 2017-06-06 at 10:49 +0200, Roberto Sassu wrote: >>>>>> 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). >>>>> >>>>> I don't see this as being any different than what happens when the >>>>> kernel saves the measurement list. Originally, the memory size was >>>>> defined at kexec load, but only populated at kexec execute. There was >>>>> plenty of time between the kexec load and execute for additional >>>>> measurement records to be added. >>>>> >>>>> The upstreamed version defines the buffer size and populates it at >>>>> kexec load. However kexec load itself generates additional >>>>> measurements, so it has to reserve more memory than what is returned >>>>> by ima_get_binary_runtime_size(). (Refer to ima_add_kexec_buffer.) >>>> >>>> ima_dump_measurement_list() determines the total number of entries and >>>> the required memory size (which are written to the kexec header) after >>>> iterating over the whole list. Are new entries added to the kexec buffer >>>> after ima_dump_measurement_list() is called? >>> >>> The upstreamed version allocates the segment in kexec load and then >>> fills the buffer. However, in between getting the current memory size >>> needed and filling the buffer, additional measurements can be added. >>> Thus the segment size needs to be larger than the current memory >>> size. >>> >>> The header reflects the number of measurements and the actual buffer >>> size, not the segment size. When restoring the measurement list, >>> however, we rely on the number of measurements and use the buffer size >>> as a reference to prevent accessing memory beyond the buffer. The >>> buffer size does not need to be exact. >> >> In this case, I have to modify patch 2/7 and remove ENFORCE_BUFEND >> from the enforcing mask. Otherwise, ima_restore_measurement_list() >> would return an error when parsing the last entry and buffer size >> in the kexec header is greater than the exact size required to >> store the measurements list. > > Or the testing patches could relax the ENFORCE_BUFEND requirement. > Without the ENFORCE_BUFEND requirement, I'm not sure this patch (5/7) > will be needed. The ENFORCE_BUFEND bit is set in patch 2/7: --- + enforce_mask |= (count == khdr->count) ? ENFORCE_BUFEND : 0; --- If this requirement is too strict for restoring measurements, then patch 2/7 should be modified. Regarding this patch, I agree that it is not strictly necessary. With an userspace tool, it would be possible to prepend the kexec header to the binary list after parsing the entries. >> Should I just send the modified patch with the [RESEND] tag >> or should I send the whole patch set with an incremented version >> number? > > The testing patches could be upstreamed separately, if you prefer. Ok. Thanks Roberto >> Also, since patches 4-6 are for testing, I would prefer to skip >> them for now and push a new version of the patch set for the >> Crypto Agile format first? > > That's fine. I've pushed patches 1 - 3, and 7 to the next-4.12- > ima_parse_buf branch for a day or so, before staging them in the next > branch to be upstreamed. > > The patches can be found here > https://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux- > integrity.git/next-4.12-ima_parse_buf. > > Mimi >
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 <linux/vmalloc.h> #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; + + if (!khdr) + break; + + 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);
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. Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> --- 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(-)