Re: [Linux-ima-devel] [PATCH 5/7] ima: add securityfs interface to save a measurements list with kexec header
From: Roberto Sassu
Date: Tue Jun 06 2017 - 04:53:12 EST
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@xxxxxxxxxx>
---
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);
--
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Bo PENG, Qiuen PENG, Shengli WANG