[PATCH 0/7] ima: carry the measurement list across kexec

[PATCH 0/7] ima: carry the measurement list across kexec

[Mimi Zohar]

The TPM PCRs are only reset on a hard reboot.  In order to validate a
TPM's quote after a soft reboot (eg. kexec -e), the IMA measurement list
of the running kernel must be saved and then restored on the subsequent
boot.

The existing securityfs binary_runtime_measurements file conveniently
provides a serialized format of the IMA measurement list. This patch
set serializes the measurement list in this format and restores it.

This patch set pre-req's Thiago Bauermann's "kexec_file: Add buffer
hand-over for the next kernel" patch set* for actually carrying the
serialized measurement list across the kexec.

Mimi

*https://lists.infradead.org/pipermail/kexec/2016-June/016157.html

Mimi Zohar (6):
  ima: on soft reboot, restore the measurement list
  ima: permit duplicate measurement list entries
  ima: maintain memory size needed for serializing the measurement list
  ima: serialize the binary_runtime_measurements
  ima: store the builtin/custom template definitions in a list
  ima: support restoring multiple template formats

Thiago Jung Bauermann (1):
  ima: on soft reboot, save the measurement list

 include/linux/ima.h                   |  15 ++
 kernel/kexec_file.c                   |   3 +
 security/integrity/ima/Kconfig        |  12 ++
 security/integrity/ima/Makefile       |   1 +
 security/integrity/ima/ima.h          |  14 ++
 security/integrity/ima/ima_fs.c       |   2 +-
 security/integrity/ima/ima_init.c     |   2 +
 security/integrity/ima/ima_kexec.c    | 189 ++++++++++++++++++++++++
 security/integrity/ima/ima_main.c     |   1 +
 security/integrity/ima/ima_queue.c    |  72 +++++++++-
 security/integrity/ima/ima_template.c | 262 ++++++++++++++++++++++++++++++++--
 11 files changed, 556 insertions(+), 17 deletions(-)
 create mode 100644 security/integrity/ima/ima_kexec.c

-- 
2.1.0

[PATCH 1/7] ima: on soft reboot, restore the measurement list

[Mimi Zohar]

The TPM PCRs are only reset on a hard reboot.  In order to validate a
TPM's quote after a soft reboot (eg. kexec -e), the IMA measurement list
of the running kernel must be saved and restored on boot.  This patch
restores the measurement list.

Changelog:
- call ima_load_kexec_buffer() (Thiago)

Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
---
 security/integrity/ima/Makefile       |   1 +
 security/integrity/ima/ima.h          |  10 ++
 security/integrity/ima/ima_init.c     |   2 +
 security/integrity/ima/ima_kexec.c    |  55 +++++++++++
 security/integrity/ima/ima_queue.c    |  10 ++
 security/integrity/ima/ima_template.c | 171 ++++++++++++++++++++++++++++++++++
 6 files changed, 249 insertions(+)
 create mode 100644 security/integrity/ima/ima_kexec.c

diff --git a/security/integrity/ima/Makefile b/security/integrity/ima/Makefile
index c34599f..c0ce7b1 100644
--- a/security/integrity/ima/Makefile
+++ b/security/integrity/ima/Makefile
@@ -8,4 +8,5 @@ obj-$(CONFIG_IMA) += ima.o
 ima-y := ima_fs.o ima_queue.o ima_init.o ima_main.o ima_crypto.o ima_api.o \
 	 ima_policy.o ima_template.o ima_template_lib.o ima_buffer.o
 ima-$(CONFIG_IMA_APPRAISE) += ima_appraise.o
+ima-$(CONFIG_KEXEC_FILE) += ima_kexec.o
 obj-$(CONFIG_IMA_BLACKLIST_KEYRING) += ima_mok.o
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index b5728da..84e8d36 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -102,6 +102,13 @@ struct ima_queue_entry {
 };
 extern struct list_head ima_measurements;	/* list of all measurements */
 
+/* Some details preceding the binary serialized measurement list */
+struct ima_kexec_hdr {
+	unsigned short version;
+	unsigned long buffer_size;
+	unsigned long count;
+} __packed;
+
 /* Internal IMA function definitions */
 int ima_init(void);
 int ima_fs_init(void);
@@ -122,6 +129,9 @@ int ima_init_crypto(void);
 void ima_putc(struct seq_file *m, void *data, int datalen);
 void ima_print_digest(struct seq_file *m, u8 *digest, u32 size);
 struct ima_template_desc *ima_template_desc_current(void);
+void ima_load_kexec_buffer(void);
+int ima_restore_measurement_entry(struct ima_template_entry *entry);
+int ima_restore_measurement_list(loff_t bufsize, void *buf);
 int ima_init_template(void);
 
 /*
diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c
index 32912bd..3ba0ca4 100644
--- a/security/integrity/ima/ima_init.c
+++ b/security/integrity/ima/ima_init.c
@@ -128,6 +128,8 @@ int __init ima_init(void)
 	if (rc != 0)
 		return rc;
 
+	ima_load_kexec_buffer();
+
 	rc = ima_add_boot_aggregate();	/* boot aggregate must be first entry */
 	if (rc != 0)
 		return rc;
diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
new file mode 100644
index 0000000..6a046ad
--- /dev/null
+++ b/security/integrity/ima/ima_kexec.c
@@ -0,0 +1,55 @@
+/*
+ * Copyright (C) 2016 IBM Corporation
+ *
+ * Authors:
+ * Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
+ * Mimi Zohar <zohar@linux.vnet.ibm.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+#include <linux/fcntl.h>
+#include <linux/slab.h>
+#include <linux/module.h>
+#include <linux/seq_file.h>
+#include <linux/rculist.h>
+#include <linux/rcupdate.h>
+#include <linux/parser.h>
+#include <linux/vmalloc.h>
+#include <linux/kexec.h>
+#include <linux/reboot.h>
+
+#include "ima.h"
+
+/*
+ * Restore the measurement list from the previous kernel.
+ */
+void ima_load_kexec_buffer(void)
+{
+	void *kexec_buffer = NULL;
+	size_t kexec_buffer_size = 0;
+	int rc;
+
+	rc = kexec_get_handover_buffer(&kexec_buffer, &kexec_buffer_size);
+	switch (rc) {
+	case 0:
+		rc = ima_restore_measurement_list(kexec_buffer_size,
+						  kexec_buffer);
+		if (rc != 0)
+			pr_err("Failed to restore the measurement list: %d\n",
+				rc);
+
+		kexec_free_handover_buffer();
+		break;
+	case -ENOTSUPP:
+		pr_debug("Restoring the measurement list not supported\n");
+		break;
+	case -ENOENT:
+		pr_debug("No measurement list to restore\n");
+		break;
+	default:
+		pr_debug("Error restoring the measurement list: %d\n", rc);
+	}
+}
diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c
index 32f6ac0..4b1bb77 100644
--- a/security/integrity/ima/ima_queue.c
+++ b/security/integrity/ima/ima_queue.c
@@ -149,3 +149,13 @@ out:
 			    op, audit_cause, result, audit_info);
 	return result;
 }
+
+int ima_restore_measurement_entry(struct ima_template_entry *entry)
+{
+	int result = 0;
+
+	mutex_lock(&ima_extend_list_mutex);
+	result = ima_add_digest_entry(entry);
+	mutex_unlock(&ima_extend_list_mutex);
+	return result;
+}
diff --git a/security/integrity/ima/ima_template.c b/security/integrity/ima/ima_template.c
index febd12e..c6510f0 100644
--- a/security/integrity/ima/ima_template.c
+++ b/security/integrity/ima/ima_template.c
@@ -205,3 +205,174 @@ int __init ima_init_template(void)
 
 	return result;
 }
+
+static int ima_restore_template_data(struct ima_template_desc *template_desc,
+				     void *template_data,
+				     int template_data_size,
+				     struct ima_template_entry **entry)
+{
+	struct binary_field_data {
+		u32 len;
+		u8 data[0];
+	} __packed;
+
+	struct binary_field_data *field_data;
+	int offset = 0;
+	int ret = 0;
+	int i;
+
+	*entry = kzalloc(sizeof(**entry) +
+		    template_desc->num_fields * sizeof(struct ima_field_data),
+		    GFP_NOFS);
+	if (!*entry)
+		return -ENOMEM;
+
+	(*entry)->template_desc = template_desc;
+	for (i = 0; i < template_desc->num_fields; i++) {
+		field_data = template_data + offset;
+
+		/* Each field of the template data is prefixed with a length. */
+		if (offset > (template_data_size - sizeof(field_data->len))) {
+			pr_err("Restoring the template field failed\n");
+			ret = -EINVAL;
+			break;
+		}
+		offset += sizeof(field_data->len);
+
+		if (offset > (template_data_size - field_data->len)) {
+			pr_err("Restoring the template field data failed\n");
+			ret = -EINVAL;
+			break;
+		}
+		offset += field_data->len;
+
+		(*entry)->template_data[i].len = field_data->len;
+		(*entry)->template_data_len += sizeof(field_data->len);
+
+		(*entry)->template_data[i].data =
+			kzalloc(field_data->len + 1, GFP_KERNEL);
+		if (!(*entry)->template_data[i].data) {
+			ret = -ENOMEM;
+			break;
+		}
+		memcpy((*entry)->template_data[i].data, field_data->data,
+			field_data->len);
+		(*entry)->template_data_len += field_data->len;
+	}
+
+	if (ret < 0) {
+		ima_free_template_entry(*entry);
+		*entry = NULL;
+	}
+
+	return ret;
+}
+
+#define MAX_TEMPLATE_NAME_LEN 30
+
+/* Restore the serialized binary measurement list without extending PCRs. */
+int ima_restore_measurement_list(loff_t size, void *buf)
+{
+	struct binary_hdr_v1 {
+		u32 pcr;
+		u8 digest[TPM_DIGEST_SIZE];
+		u32 template_name_len;
+		char template_name[0];
+	} __packed;
+	char template_name[MAX_TEMPLATE_NAME_LEN];
+
+	struct binary_data_v1 {
+		u32 template_data_size;
+		char template_data[0];
+	} __packed;
+
+	struct ima_kexec_hdr *khdr = buf;
+	struct binary_hdr_v1 *hdr_v1;
+	struct binary_data_v1 *data_v1;
+
+	void *bufp = buf + sizeof(*khdr);
+	void *bufendp = buf + khdr->buffer_size;
+	struct ima_template_entry *entry;
+	struct ima_template_desc *template_desc;
+	unsigned long count = 0;
+	int ret = 0;
+
+	if (!buf || size < sizeof(*khdr))
+		return 0;
+
+	if (khdr->version != 1) {
+		pr_err("attempting to restore a incompatible measurement list");
+		return 0;
+	}
+
+	/*
+	 * ima kexec buffer prefix: version, buffer size, count
+	 * v1 format: pcr, digest, template-name-len, template-name,
+	 *	      template-data-size, template-data
+	 */
+	while ((bufp < bufendp) && (count++ < khdr->count)) {
+		if (count > ULONG_MAX - 1) {
+			pr_err("attempting to restore too many measurements");
+			ret = -EINVAL;
+		}
+
+		hdr_v1 = bufp;
+		if ((hdr_v1->template_name_len > MAX_TEMPLATE_NAME_LEN) ||
+		    ((bufp + hdr_v1->template_name_len) > bufendp)) {
+			pr_err("attempting to restore a template name \
+				that is too long\n");
+			ret = -EINVAL;
+			break;
+		}
+		bufp += sizeof(*hdr_v1);
+
+		/* template name is not null terminated */
+		memcpy(template_name, bufp, hdr_v1->template_name_len);
+		template_name[hdr_v1->template_name_len] = 0;
+
+		if (strcmp(template_name, "ima") == 0) {
+			pr_err("attempting to restore an unsupported \
+				template \"%s\" failed\n", template_name);
+			ret = -EINVAL;
+			break;
+		}
+		data_v1 = bufp += (u_int8_t)hdr_v1->template_name_len;
+
+		/* get template format */
+		template_desc = lookup_template_desc(template_name);
+		if (!template_desc) {
+			pr_err("template \"%s\" not found\n", template_name);
+			ret = -EINVAL;
+			break;
+		}
+
+		if (bufp > (bufendp - sizeof(data_v1->template_data_size))) {
+			pr_err("restoring the template data size failed\n");
+			ret = -EINVAL;
+			break;
+		}
+		bufp += (u_int8_t) sizeof(data_v1->template_data_size);
+
+		if (bufp > (bufendp - data_v1->template_data_size)) {
+			pr_err("restoring the template data failed\n");
+			ret = -EINVAL;
+			break;
+		}
+
+		ret = ima_restore_template_data(template_desc,
+						data_v1->template_data,
+						data_v1->template_data_size,
+						&entry);
+		if (ret < 0)
+			break;
+
+		memcpy(entry->digest, hdr_v1->digest, TPM_DIGEST_SIZE);
+		entry->pcr = hdr_v1->pcr;
+		ret = ima_restore_measurement_entry(entry);
+		if (ret < 0)
+			break;
+
+		bufp += data_v1->template_data_size;
+	}
+	return ret;
+}
-- 
2.1.0

[PATCH 2/7] ima: permit duplicate measurement list entries

[Mimi Zohar]

Measurements carried across kexec need to be added to the IMA
measurement list, but should not prevent measurements of the newly
booted kernel from being added to the measurement list. This patch
adds support for allowing duplicate measurements.

The "boot_aggregate" measurement entry is the delimiter between soft
boots.

Signed-off-by: Mimi Zohar <zohar@linuv.vnet.ibm.com>
---
 security/integrity/ima/ima_queue.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c
index 4b1bb77..12d1b04 100644
--- a/security/integrity/ima/ima_queue.c
+++ b/security/integrity/ima/ima_queue.c
@@ -65,11 +65,12 @@ static struct ima_queue_entry *ima_lookup_digest_entry(u8 *digest_value,
 }
 
 /* ima_add_template_entry helper function:
- * - Add template entry to measurement list and hash table.
+ * - Add template entry to the measurement list and hash table, for
+ *   all entries except those carried across kexec.
  *
  * (Called with ima_extend_list_mutex held.)
  */
-static int ima_add_digest_entry(struct ima_template_entry *entry)
+static int ima_add_digest_entry(struct ima_template_entry *entry, int flags)
 {
 	struct ima_queue_entry *qe;
 	unsigned int key;
@@ -85,8 +86,10 @@ static int ima_add_digest_entry(struct ima_template_entry *entry)
 	list_add_tail_rcu(&qe->later, &ima_measurements);
 
 	atomic_long_inc(&ima_htable.len);
-	key = ima_hash_key(entry->digest);
-	hlist_add_head_rcu(&qe->hnext, &ima_htable.queue[key]);
+	if (flags) {
+		key = ima_hash_key(entry->digest);
+		hlist_add_head_rcu(&qe->hnext, &ima_htable.queue[key]);
+	}
 	return 0;
 }
 
@@ -126,7 +129,7 @@ int ima_add_template_entry(struct ima_template_entry *entry, int violation,
 		}
 	}
 
-	result = ima_add_digest_entry(entry);
+	result = ima_add_digest_entry(entry, 1);
 	if (result < 0) {
 		audit_cause = "ENOMEM";
 		audit_info = 0;
@@ -155,7 +158,7 @@ int ima_restore_measurement_entry(struct ima_template_entry *entry)
 	int result = 0;
 
 	mutex_lock(&ima_extend_list_mutex);
-	result = ima_add_digest_entry(entry);
+	result = ima_add_digest_entry(entry, 0);
 	mutex_unlock(&ima_extend_list_mutex);
 	return result;
 }
-- 
2.1.0

[PATCH 3/7] ima: maintain memory size needed for serializing the measurement list

[Mimi Zohar]

In preparation for serializing the binary_runtime_measurements, this patch
maintains the amount of memory required.

Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
---
 security/integrity/ima/Kconfig     | 12 ++++++++++
 security/integrity/ima/ima.h       |  1 +
 security/integrity/ima/ima_queue.c | 49 ++++++++++++++++++++++++++++++++++++--
 3 files changed, 60 insertions(+), 2 deletions(-)

diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
index 0fb54d3..d764027 100644
--- a/security/integrity/ima/Kconfig
+++ b/security/integrity/ima/Kconfig
@@ -27,6 +27,18 @@ config IMA
 	  to learn more about IMA.
 	  If unsure, say N.
 
+config IMA_KEXEC
+	bool "Enable carrying the IMA measurement list across a soft boot"
+	depends on IMA && TCG_TPM && KEXEC_FILE
+	default n
+	help
+	   TPM PCRs are only reset on a hard reboot.  In order to validate
+	   a TPM's quote after a soft boot, the IMA measurement list of the
+	   running kernel must be saved and restored on boot.
+
+	   Depending on the IMA policy, the measurement list can grow to
+	   be very large.
+
 config IMA_MEASURE_PCR_IDX
 	int
 	depends on IMA
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 84e8d36..222668f 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -132,6 +132,7 @@ struct ima_template_desc *ima_template_desc_current(void);
 void ima_load_kexec_buffer(void);
 int ima_restore_measurement_entry(struct ima_template_entry *entry);
 int ima_restore_measurement_list(loff_t bufsize, void *buf);
+unsigned long ima_get_binary_runtime_size(void);
 int ima_init_template(void);
 
 /*
diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c
index 12d1b04..8f0661b 100644
--- a/security/integrity/ima/ima_queue.c
+++ b/security/integrity/ima/ima_queue.c
@@ -29,6 +29,11 @@
 #define AUDIT_CAUSE_LEN_MAX 32
 
 LIST_HEAD(ima_measurements);	/* list of all measurements */
+#ifdef CONFIG_IMA_KEXEC
+static unsigned long binary_runtime_size;
+#else
+static unsigned long binary_runtime_size = ULONG_MAX;
+#endif
 
 /* key: inode (before secure-hashing a file) */
 struct ima_h_table ima_htable = {
@@ -64,6 +69,24 @@ static struct ima_queue_entry *ima_lookup_digest_entry(u8 *digest_value,
 	return ret;
 }
 
+/*
+ * Calculate the memory required for serializing a single
+ * binary_runtime_measurement list entry, which contains a
+ * couple of variable length fields (e.g template name and data).
+ */
+static int get_binary_runtime_size(struct ima_template_entry *entry)
+{
+	int size = 0;
+
+	size += sizeof(u32);	/* pcr */
+	size += sizeof(entry->digest);
+	size += sizeof(int);	/* template name size field */
+	size += strlen(entry->template_desc->name);
+	size += sizeof(entry->template_data_len);
+	size += entry->template_data_len;
+	return size;
+}
+
 /* ima_add_template_entry helper function:
  * - Add template entry to the measurement list and hash table, for
  *   all entries except those carried across kexec.
@@ -90,9 +113,26 @@ static int ima_add_digest_entry(struct ima_template_entry *entry, int flags)
 		key = ima_hash_key(entry->digest);
 		hlist_add_head_rcu(&qe->hnext, &ima_htable.queue[key]);
 	}
+
+	if (binary_runtime_size != ULONG_MAX) {
+		int size;
+
+		size = get_binary_runtime_size(entry);
+		binary_runtime_size = (binary_runtime_size < ULONG_MAX - size) ?
+		     binary_runtime_size + size : ULONG_MAX;
+	}
 	return 0;
 }
 
+/*
+ * Return the amount of memory required for serializing the
+ * entire binary_runtime_measurement list.
+ */
+unsigned long ima_get_binary_runtime_size(void)
+{
+	return binary_runtime_size;
+};
+
 static int ima_pcr_extend(const u8 *hash, int pcr)
 {
 	int result = 0;
@@ -106,8 +146,13 @@ static int ima_pcr_extend(const u8 *hash, int pcr)
 	return result;
 }
 
-/* Add template entry to the measurement list and hash table,
- * and extend the pcr.
+/*
+ * Add template entry to the measurement list and hash table, and
+ * extend the pcr.
+ *
+ * On systems which support carrying the IMA measurement list across
+ * kexec, maintain the total memory size required for serializing the
+ * binary_runtime_measurements.
  */
 int ima_add_template_entry(struct ima_template_entry *entry, int violation,
 			   const char *op, struct inode *inode,
-- 
2.1.0

[PATCH 4/7] ima: serialize the binary_runtime_measurements

[Mimi Zohar]

The TPM PCRs are only reset on a hard reboot.  In order to validate a
TPM's quote after a soft reboot (eg. kexec -e), the IMA measurement list
of the running kernel must be saved and restored on boot.  This patch
serializes the IMA measurement list in the binary_runtime_measurements
format.

Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
---
 security/integrity/ima/ima.h       |  1 +
 security/integrity/ima/ima_fs.c    |  2 +-
 security/integrity/ima/ima_kexec.c | 51 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 53 insertions(+), 1 deletion(-)

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 222668f..f972296 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -132,6 +132,7 @@ struct ima_template_desc *ima_template_desc_current(void);
 void ima_load_kexec_buffer(void);
 int ima_restore_measurement_entry(struct ima_template_entry *entry);
 int ima_restore_measurement_list(loff_t bufsize, void *buf);
+int ima_measurements_show(struct seq_file *m, void *v);
 unsigned long ima_get_binary_runtime_size(void);
 int ima_init_template(void);
 
diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
index c07a384..66e5dd5 100644
--- a/security/integrity/ima/ima_fs.c
+++ b/security/integrity/ima/ima_fs.c
@@ -116,7 +116,7 @@ void ima_putc(struct seq_file *m, void *data, int datalen)
  *       [eventdata length]
  *       eventdata[n]=template specific data
  */
-static int ima_measurements_show(struct seq_file *m, void *v)
+int ima_measurements_show(struct seq_file *m, void *v)
 {
 	/* the list never shrinks, so we don't need a lock here */
 	struct ima_queue_entry *qe = v;
diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
index 6a046ad..e77ca9d 100644
--- a/security/integrity/ima/ima_kexec.c
+++ b/security/integrity/ima/ima_kexec.c
@@ -23,6 +23,57 @@
 
 #include "ima.h"
 
+static int ima_dump_measurement_list(unsigned long *buffer_size, void **buffer,
+				     unsigned long segment_size)
+{
+	struct ima_queue_entry *qe;
+	struct seq_file file;
+	struct ima_kexec_hdr khdr = {
+		.version = 1, .buffer_size = 0, .count = 0};
+	int ret = 0;
+
+	/* segment size can't change between kexec load and execute */
+	file.buf = vmalloc(segment_size);
+	if (!file.buf) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	file.size = segment_size;
+	file.read_pos = 0;
+	file.count = sizeof(khdr);	/* reserved space */
+
+	list_for_each_entry_rcu(qe, &ima_measurements, later) {
+		if (file.count < file.size) {
+			khdr.count++;
+			ima_measurements_show(&file, qe);
+		} else {
+			ret = -EINVAL;
+			break;
+		}
+	}
+
+	if (ret < 0)
+		goto out;
+
+	/*
+	 * fill in reserved space with some buffer details
+	 * (eg. version, buffer size, number of measurements)
+	 */
+	khdr.buffer_size = file.count;
+	memcpy(file.buf, &khdr, sizeof(khdr));
+	print_hex_dump(KERN_DEBUG, "ima dump: ", DUMP_PREFIX_NONE,
+			16, 1, file.buf,
+			file.count < 100 ? file.count : 100, true);
+
+	*buffer_size = file.count;
+	*buffer = file.buf;
+out:
+	if (ret == -EINVAL)
+		vfree(file.buf);
+	return ret;
+}
+
 /*
  * Restore the measurement list from the previous kernel.
  */
-- 
2.1.0

[PATCH 5/7] ima: on soft reboot, save the measurement list

[Mimi Zohar]

From: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>

This patch uses the kexec buffer passing mechanism to pass the
serialized IMA binary_runtime_measurements to the next kernel.

Changelog:
- updated to call IMA functions  (Mimi)
- move code from ima_template.c to ima_kexec.c (Mimi)

Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
---
 include/linux/ima.h                | 15 +++++++
 kernel/kexec_file.c                |  3 ++
 security/integrity/ima/ima_kexec.c | 83 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 101 insertions(+)

diff --git a/include/linux/ima.h b/include/linux/ima.h
index b553367..ba484ed 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -11,6 +11,7 @@
 #define _LINUX_IMA_H
 
 #include <linux/fs.h>
+#include <linux/kexec.h>
 struct linux_binprm;
 
 enum ima_buffer_id {
@@ -35,6 +36,14 @@ extern int ima_add_measurement_check(const char *hashname, u8 *digest,
 				     loff_t size, enum ima_buffer_id buffer_id,
 				     char *hint);
 
+#ifdef CONFIG_IMA_KEXEC
+extern void ima_add_kexec_buffer(struct kimage *image);
+#else
+static inline void ima_add_kexec_buffer(struct kimage *image)
+{
+}
+#endif
+
 #else
 static inline int ima_bprm_check(struct linux_binprm *bprm)
 {
@@ -85,6 +94,12 @@ static inline int ima_add_measurement_check(const char *hashname, u8 *digest,
 {
 	return 0;
 }
+
+#ifdef CONFIG_IMA_KEXEC
+static inline void ima_add_kexec_buffer(struct kimage *image)
+{
+}
+#endif
 #endif /* CONFIG_IMA */
 
 #ifdef CONFIG_IMA_APPRAISE
diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index 852adb2..622c126 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -202,6 +202,9 @@ kimage_file_prepare_segments(struct kimage *image, int kernel_fd, int initrd_fd,
 		return ret;
 	image->kernel_buf_len = size;
 
+	/* IMA needs to pass the measurement list to the next kernel. */
+	ima_add_kexec_buffer(image);
+
 	/* Call arch image probe handlers */
 	ret = arch_kexec_kernel_image_probe(image, image->kernel_buf,
 					    image->kernel_buf_len);
diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
index e77ca9d..3fed417 100644
--- a/security/integrity/ima/ima_kexec.c
+++ b/security/integrity/ima/ima_kexec.c
@@ -23,6 +23,11 @@
 
 #include "ima.h"
 
+#ifdef CONFIG_IMA_KEXEC
+/* Physical address of the measurement buffer in the next kernel. */
+static unsigned long kexec_buffer_load_addr;
+static size_t kexec_segment_size;
+
 static int ima_dump_measurement_list(unsigned long *buffer_size, void **buffer,
 				     unsigned long segment_size)
 {
@@ -75,6 +80,84 @@ out:
 }
 
 /*
+ * Called during kexec execute so that IMA can save the measurement list.
+ */
+static int ima_update_kexec_buffer(struct notifier_block *self,
+				   unsigned long action, void *data)
+{
+	void *kexec_buffer = NULL;
+	size_t kexec_buffer_size;
+	int ret;
+
+	if (!kexec_in_progress)
+		return NOTIFY_OK;
+
+	kexec_buffer_size = ima_get_binary_runtime_size();
+	if (kexec_buffer_size >
+	    (kexec_segment_size - sizeof(struct ima_kexec_hdr))) {
+		pr_err("Binary measurement list grew too large.\n");
+		goto out;
+	}
+
+	ima_dump_measurement_list(&kexec_buffer_size, &kexec_buffer,
+				  kexec_segment_size);
+	if (!kexec_buffer) {
+		pr_err("Not enough memory for the kexec measurement buffer.\n");
+		goto out;
+	}
+	ret = kexec_update_segment(kexec_buffer, kexec_buffer_size,
+				   kexec_buffer_load_addr, kexec_segment_size);
+	if (ret)
+		pr_err("Error updating kexec buffer: %d\n", ret);
+out:
+	return NOTIFY_OK;
+}
+
+struct notifier_block update_buffer_nb = {
+	.notifier_call = ima_update_kexec_buffer,
+};
+
+/*
+ * Called during kexec_file_load so that IMA can add a segment to the kexec
+ * image for the measurement list for the next kernel.
+ */
+void ima_add_kexec_buffer(struct kimage *image)
+{
+	struct kexec_buf kbuf = { .image = image, .buf_align = PAGE_SIZE,
+				  .buf_min = 0, .buf_max = ULONG_MAX,
+				  .top_down = true };
+	int ret;
+
+	if (!kexec_can_hand_over_buffer())
+		return;
+
+	kexec_segment_size = ALIGN(ima_get_binary_runtime_size() + PAGE_SIZE,
+				   PAGE_SIZE);
+
+	if (kexec_segment_size >= (ULONG_MAX - sizeof(long))) {
+		pr_err("Binary measurement list too large.\n");
+		return;
+	}
+
+	/* Ask not to checksum the segment, we will update it later. */
+	kbuf.buffer = NULL;
+	kbuf.bufsz = 0;
+	kbuf.memsz = kexec_segment_size;
+	ret = kexec_add_handover_buffer(&kbuf, false);
+	if (ret) {
+		pr_err("Error passing over kexec measurement buffer.\n");
+		return;
+	}
+	kexec_buffer_load_addr = kbuf.mem;
+
+	register_reboot_notifier(&update_buffer_nb);
+
+	pr_debug("kexec measurement buffer for the loaded kernel at 0x%lx.\n",
+		 kexec_buffer_load_addr);
+}
+#endif /* IMA_KEXEC */
+
+/*
  * Restore the measurement list from the previous kernel.
  */
 void ima_load_kexec_buffer(void)
-- 
2.1.0

[PATCH 6/7] ima: store the builtin/custom template definitions in a list

[Mimi Zohar]

The builtin and single custom templates are currently stored in an
array.  In preparation for being able to restore a measurement list
containing multiple builtin/custom templates, this patch stores the
builtin and custom templates as a linked list.  This will permit
defining more than one custom template per boot.

Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
---
 security/integrity/ima/ima.h          |  2 ++
 security/integrity/ima/ima_main.c     |  1 +
 security/integrity/ima/ima_template.c | 37 +++++++++++++++++++++++++++--------
 3 files changed, 32 insertions(+), 8 deletions(-)

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index f972296..9d7fdd5 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -81,6 +81,7 @@ struct ima_template_field {
 
 /* IMA template descriptor definition */
 struct ima_template_desc {
+	struct list_head list;
 	char *name;
 	char *fmt;
 	int num_fields;
@@ -135,6 +136,7 @@ int ima_restore_measurement_list(loff_t bufsize, void *buf);
 int ima_measurements_show(struct seq_file *m, void *v);
 unsigned long ima_get_binary_runtime_size(void);
 int ima_init_template(void);
+void ima_init_template_list(void);
 
 /*
  * used to protect h_table and sha_table
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 596ef61..592f318 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -418,6 +418,7 @@ static int __init init_ima(void)
 {
 	int error;
 
+	ima_init_template_list();
 	hash_setup(CONFIG_IMA_DEFAULT_HASH);
 	error = ima_init();
 	if (!error) {
diff --git a/security/integrity/ima/ima_template.c b/security/integrity/ima/ima_template.c
index c6510f0..b7bcb62 100644
--- a/security/integrity/ima/ima_template.c
+++ b/security/integrity/ima/ima_template.c
@@ -15,16 +15,20 @@
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
+#include <linux/rculist.h>
 #include "ima.h"
 #include "ima_template_lib.h"
 
-static struct ima_template_desc defined_templates[] = {
+static struct ima_template_desc builtin_templates[] = {
 	{.name = IMA_TEMPLATE_IMA_NAME, .fmt = IMA_TEMPLATE_IMA_FMT},
 	{.name = "ima-ng", .fmt = "d-ng|n-ng"},
 	{.name = "ima-sig", .fmt = "d-ng|n-ng|sig"},
 	{.name = "", .fmt = ""},	/* placeholder for a custom format */
 };
 
+static LIST_HEAD(defined_templates);
+spinlock_t template_list;
+
 static struct ima_template_field supported_fields[] = {
 	{.field_id = "d", .field_init = ima_eventdigest_init,
 	 .field_show = ima_show_template_digest},
@@ -80,7 +84,7 @@ __setup("ima_template=", ima_template_setup);
 
 static int __init ima_template_fmt_setup(char *str)
 {
-	int num_templates = ARRAY_SIZE(defined_templates);
+	int num_templates = ARRAY_SIZE(builtin_templates);
 
 	if (ima_template)
 		return 1;
@@ -91,20 +95,24 @@ static int __init ima_template_fmt_setup(char *str)
 		return 1;
 	}
 
-	defined_templates[num_templates - 1].fmt = str;
-	ima_template = defined_templates + num_templates - 1;
+	builtin_templates[num_templates - 1].fmt = str;
+	ima_template = builtin_templates + num_templates - 1;
+
 	return 1;
 }
 __setup("ima_template_fmt=", ima_template_fmt_setup);
 
 static struct ima_template_desc *lookup_template_desc(const char *name)
 {
-	int i;
+	struct ima_template_desc *template_desc;
 
-	for (i = 0; i < ARRAY_SIZE(defined_templates); i++) {
-		if (strcmp(defined_templates[i].name, name) == 0)
-			return defined_templates + i;
+	rcu_read_lock();
+	list_for_each_entry_rcu(template_desc, &defined_templates, list) {
+		if ((strcmp(template_desc->name, name) == 0) ||
+		    (strcmp(template_desc->fmt, name) == 0))
+			return template_desc;
 	}
+	rcu_read_unlock();
 
 	return NULL;
 }
@@ -190,6 +198,19 @@ struct ima_template_desc *ima_template_desc_current(void)
 	return ima_template;
 }
 
+void __init ima_init_template_list(void)
+{
+	int i;
+
+	spin_lock(&template_list);
+	for (i = 0; i < ARRAY_SIZE(builtin_templates); i++) {
+		list_add_tail_rcu(&builtin_templates[i].list,
+				  &defined_templates);
+	}
+	spin_unlock(&template_list);
+	synchronize_rcu();
+}
+
 int __init ima_init_template(void)
 {
 	struct ima_template_desc *template = ima_template_desc_current();
-- 
2.1.0

[PATCH 7/7] ima: support restoring multiple template formats

[Mimi Zohar]

The configured IMA measurement list template format can be replaced at
runtime on the boot command line, including a custom template format.
This patch adds support for restoring a measuremement list containing
multiple builtin/custom template formats.

Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
---
 security/integrity/ima/ima_template.c | 58 +++++++++++++++++++++++++++++++++--
 1 file changed, 55 insertions(+), 3 deletions(-)

diff --git a/security/integrity/ima/ima_template.c b/security/integrity/ima/ima_template.c
index b7bcb62..3923718 100644
--- a/security/integrity/ima/ima_template.c
+++ b/security/integrity/ima/ima_template.c
@@ -56,6 +56,8 @@ static int __init ima_template_setup(char *str)
 	if (ima_template)
 		return 1;
 
+	ima_init_template_list();
+
 	/*
 	 * Verify that a template with the supplied name exists.
 	 * If not, use CONFIG_IMA_DEFAULT_TEMPLATE.
@@ -150,9 +152,14 @@ static int template_desc_init_fields(const char *template_fmt,
 {
 	const char *template_fmt_ptr;
 	struct ima_template_field *found_fields[IMA_TEMPLATE_NUM_FIELDS_MAX];
-	int template_num_fields = template_fmt_size(template_fmt);
+	int template_num_fields;
 	int i, len;
 
+	if (num_fields && *num_fields > 0) /* already initialized? */
+		return 0;
+
+	template_num_fields = template_fmt_size(template_fmt);
+
 	if (template_num_fields > IMA_TEMPLATE_NUM_FIELDS_MAX) {
 		pr_err("format string '%s' contains too many fields\n",
 		       template_fmt);
@@ -202,6 +209,9 @@ void __init ima_init_template_list(void)
 {
 	int i;
 
+	if (!list_empty(&defined_templates))
+		return;
+
 	spin_lock(&template_list);
 	for (i = 0; i < ARRAY_SIZE(builtin_templates); i++) {
 		list_add_tail_rcu(&builtin_templates[i].list,
@@ -227,6 +237,35 @@ int __init ima_init_template(void)
 	return result;
 }
 
+static struct ima_template_desc *restore_template_fmt(char *template_name)
+{
+	struct ima_template_desc *template_desc = NULL;
+	int ret;
+
+	ret = template_desc_init_fields(template_name, NULL, NULL);
+	if (ret < 0) {
+		pr_err("attempting to initialize the template \"%s\" failed\n",
+			template_name);
+		goto out;
+	}
+
+	template_desc = kzalloc(sizeof(*template_desc), GFP_KERNEL);
+	if (!template_desc)
+		goto out;
+
+	template_desc->name = "";
+	template_desc->fmt = kstrdup(template_name, GFP_KERNEL);
+	if (!template_desc->fmt)
+		goto out;
+
+	spin_lock(&template_list);
+	list_add_tail_rcu(&template_desc->list, &defined_templates);
+	spin_unlock(&template_list);
+	synchronize_rcu();
+out:
+	return template_desc;
+}
+
 static int ima_restore_template_data(struct ima_template_desc *template_desc,
 				     void *template_data,
 				     int template_data_size,
@@ -359,10 +398,23 @@ int ima_restore_measurement_list(loff_t size, void *buf)
 		}
 		data_v1 = bufp += (u_int8_t)hdr_v1->template_name_len;
 
-		/* get template format */
 		template_desc = lookup_template_desc(template_name);
 		if (!template_desc) {
-			pr_err("template \"%s\" not found\n", template_name);
+			template_desc = restore_template_fmt(template_name);
+			if (!template_desc)
+				break;
+		}
+
+		/*
+		 * Only the running system's template format is initialized
+		 * on boot.  As needed, initialize the other template formats.
+		 */
+		ret = template_desc_init_fields(template_desc->fmt,
+						&(template_desc->fields),
+						&(template_desc->num_fields));
+		if (ret < 0) {
+			pr_err("attempting to restore the template fmt \"%s\" \
+				failed\n", template_desc->fmt);
 			ret = -EINVAL;
 			break;
 		}
-- 
2.1.0

Re: [PATCH 1/7] ima: on soft reboot, restore the measurement list

[Petko Manolov]

On 16-08-04 08:24:29, Mimi Zohar wrote:
> The TPM PCRs are only reset on a hard reboot.  In order to validate a
> TPM's quote after a soft reboot (eg. kexec -e), the IMA measurement list
> of the running kernel must be saved and restored on boot.  This patch
> restores the measurement list.
> 
> Changelog:
> - call ima_load_kexec_buffer() (Thiago)
> 
> Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
> ---
>  security/integrity/ima/Makefile       |   1 +
>  security/integrity/ima/ima.h          |  10 ++
>  security/integrity/ima/ima_init.c     |   2 +
>  security/integrity/ima/ima_kexec.c    |  55 +++++++++++
>  security/integrity/ima/ima_queue.c    |  10 ++
>  security/integrity/ima/ima_template.c | 171 ++++++++++++++++++++++++++++++++++
>  6 files changed, 249 insertions(+)
>  create mode 100644 security/integrity/ima/ima_kexec.c
> 
> diff --git a/security/integrity/ima/Makefile b/security/integrity/ima/Makefile
> index c34599f..c0ce7b1 100644
> --- a/security/integrity/ima/Makefile
> +++ b/security/integrity/ima/Makefile
> @@ -8,4 +8,5 @@ obj-$(CONFIG_IMA) += ima.o
>  ima-y := ima_fs.o ima_queue.o ima_init.o ima_main.o ima_crypto.o ima_api.o \
>  	 ima_policy.o ima_template.o ima_template_lib.o ima_buffer.o
>  ima-$(CONFIG_IMA_APPRAISE) += ima_appraise.o
> +ima-$(CONFIG_KEXEC_FILE) += ima_kexec.o
>  obj-$(CONFIG_IMA_BLACKLIST_KEYRING) += ima_mok.o
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index b5728da..84e8d36 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -102,6 +102,13 @@ struct ima_queue_entry {
>  };
>  extern struct list_head ima_measurements;	/* list of all measurements */
>  
> +/* Some details preceding the binary serialized measurement list */
> +struct ima_kexec_hdr {
> +	unsigned short version;
> +	unsigned long buffer_size;
> +	unsigned long count;
> +} __packed;

Unless there is no real need for this structure to be packed i suggest dropping 
the attribute.  When referenced through pointer 32bit ARM and MIPS (and likely 
all other 32bit RISC CPUs) use rather inefficient byte loads and stores.

Worse, if, for example, ->count is going to be read/written concurrently from 
multiple threads we get torn loads/stores thus losing atomicity of the access.


		Petko

Re: [PATCH 1/7] ima: on soft reboot, restore the measurement list

[Mimi Zohar]

Hi Petko,

Thank you for review!

On Fri, 2016-08-05 at 11:44 +0300, Petko Manolov wrote:
> On 16-08-04 08:24:29, Mimi Zohar wrote:
> > The TPM PCRs are only reset on a hard reboot.  In order to validate a
> > TPM's quote after a soft reboot (eg. kexec -e), the IMA measurement list
> > of the running kernel must be saved and restored on boot.  This patch
> > restores the measurement list.
> > 
> > Changelog:
> > - call ima_load_kexec_buffer() (Thiago)
> > 
> > Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
> > ---
> >  security/integrity/ima/Makefile       |   1 +
> >  security/integrity/ima/ima.h          |  10 ++
> >  security/integrity/ima/ima_init.c     |   2 +
> >  security/integrity/ima/ima_kexec.c    |  55 +++++++++++
> >  security/integrity/ima/ima_queue.c    |  10 ++
> >  security/integrity/ima/ima_template.c | 171 ++++++++++++++++++++++++++++++++++
> >  6 files changed, 249 insertions(+)
> >  create mode 100644 security/integrity/ima/ima_kexec.c
> > 
> > diff --git a/security/integrity/ima/Makefile b/security/integrity/ima/Makefile
> > index c34599f..c0ce7b1 100644
> > --- a/security/integrity/ima/Makefile
> > +++ b/security/integrity/ima/Makefile
> > @@ -8,4 +8,5 @@ obj-$(CONFIG_IMA) += ima.o
> >  ima-y := ima_fs.o ima_queue.o ima_init.o ima_main.o ima_crypto.o ima_api.o \
> >  	 ima_policy.o ima_template.o ima_template_lib.o ima_buffer.o
> >  ima-$(CONFIG_IMA_APPRAISE) += ima_appraise.o
> > +ima-$(CONFIG_KEXEC_FILE) += ima_kexec.o
> >  obj-$(CONFIG_IMA_BLACKLIST_KEYRING) += ima_mok.o
> > diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> > index b5728da..84e8d36 100644
> > --- a/security/integrity/ima/ima.h
> > +++ b/security/integrity/ima/ima.h
> > @@ -102,6 +102,13 @@ struct ima_queue_entry {
> >  };
> >  extern struct list_head ima_measurements;	/* list of all measurements */
> >  
> > +/* Some details preceding the binary serialized measurement list */
> > +struct ima_kexec_hdr {
> > +	unsigned short version;
> > +	unsigned long buffer_size;
> > +	unsigned long count;
> > +} __packed;
> 
> Unless there is no real need for this structure to be packed i suggest dropping 
> the attribute.  When referenced through pointer 32bit ARM and MIPS (and likely 
> all other 32bit RISC CPUs) use rather inefficient byte loads and stores.
> 
> Worse, if, for example, ->count is going to be read/written concurrently from 
> multiple threads we get torn loads/stores thus losing atomicity of the access.

This header is used to prefix the serialized binary measurement list
with some meta-data about the measurement list being restored.
Unfortunately kexec_get_handover_buffer() returns the segment size, not
the actual ima measurement list buffer size.  The header info is set
using memcpy() once in ima_dump_measurement_list() and then the fields
are used in ima_restore_measurement_list() to verify the buffer.

The binary runtime measurement list is packed, so the other two
structures - binary_hdr_v1 and binary_data_v1 - must be packed.  Does it
make sense for this header not to be packed as well?  Would copying the
header fields to local variables before being used solve your concern?

Remember this code is used once on the kexec execute and again on
reboot.

Mimi

Re: [PATCH 1/7] ima: on soft reboot, restore the measurement list

[Petko Manolov]

On 16-08-05 09:34:38, Mimi Zohar wrote:
> Hi Petko,
> 
> Thank you for review!
> 
> On Fri, 2016-08-05 at 11:44 +0300, Petko Manolov wrote:
> > On 16-08-04 08:24:29, Mimi Zohar wrote:
> > > The TPM PCRs are only reset on a hard reboot.  In order to validate a
> > > TPM's quote after a soft reboot (eg. kexec -e), the IMA measurement list
> > > of the running kernel must be saved and restored on boot.  This patch
> > > restores the measurement list.
> > > 
> > > Changelog:
> > > - call ima_load_kexec_buffer() (Thiago)
> > > 
> > > Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
> > > ---
> > >  security/integrity/ima/Makefile       |   1 +
> > >  security/integrity/ima/ima.h          |  10 ++
> > >  security/integrity/ima/ima_init.c     |   2 +
> > >  security/integrity/ima/ima_kexec.c    |  55 +++++++++++
> > >  security/integrity/ima/ima_queue.c    |  10 ++
> > >  security/integrity/ima/ima_template.c | 171 ++++++++++++++++++++++++++++++++++
> > >  6 files changed, 249 insertions(+)
> > >  create mode 100644 security/integrity/ima/ima_kexec.c
> > > 
> > > diff --git a/security/integrity/ima/Makefile b/security/integrity/ima/Makefile
> > > index c34599f..c0ce7b1 100644
> > > --- a/security/integrity/ima/Makefile
> > > +++ b/security/integrity/ima/Makefile
> > > @@ -8,4 +8,5 @@ obj-$(CONFIG_IMA) += ima.o
> > >  ima-y := ima_fs.o ima_queue.o ima_init.o ima_main.o ima_crypto.o ima_api.o \
> > >  	 ima_policy.o ima_template.o ima_template_lib.o ima_buffer.o
> > >  ima-$(CONFIG_IMA_APPRAISE) += ima_appraise.o
> > > +ima-$(CONFIG_KEXEC_FILE) += ima_kexec.o
> > >  obj-$(CONFIG_IMA_BLACKLIST_KEYRING) += ima_mok.o
> > > diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> > > index b5728da..84e8d36 100644
> > > --- a/security/integrity/ima/ima.h
> > > +++ b/security/integrity/ima/ima.h
> > > @@ -102,6 +102,13 @@ struct ima_queue_entry {
> > >  };
> > >  extern struct list_head ima_measurements;	/* list of all measurements */
> > >  
> > > +/* Some details preceding the binary serialized measurement list */
> > > +struct ima_kexec_hdr {
> > > +	unsigned short version;
> > > +	unsigned long buffer_size;
> > > +	unsigned long count;
> > > +} __packed;
> > 
> > Unless there is no real need for this structure to be packed i suggest 
> > dropping the attribute.  When referenced through pointer 32bit ARM and MIPS 
> > (and likely all other 32bit RISC CPUs) use rather inefficient byte loads and 
> > stores.
> > 
> > Worse, if, for example, ->count is going to be read/written concurrently 
> > from multiple threads we get torn loads/stores thus losing atomicity of the 
> > access.
> 
> This header is used to prefix the serialized binary measurement list with some 
> meta-data about the measurement list being restored. Unfortunately 
> kexec_get_handover_buffer() returns the segment size, not the actual ima 
> measurement list buffer size.  The header info is set using memcpy() once in 
> ima_dump_measurement_list() and then the fields are used in 
> ima_restore_measurement_list() to verify the buffer.

As long as there is no concurrent reads/writes this should be OK.

> The binary runtime measurement list is packed, so the other two structures - 
> binary_hdr_v1 and binary_data_v1 - must be packed.  Does it make sense for 
> this header not to be packed as well?  Would copying the header fields to 
> local variables before being used solve your concern?

Copying to aligned variables would be necessary only if:

	a) some sort of atomicity is needed, and/or
	б) speed is of concern;

> Remember this code is used once on the kexec execute and again on reboot.

If we don't need a) _and_ b) then you don't need to bother.


		Petko

Re: [PATCH 0/7] ima: carry the measurement list across kexec

[Balbir Singh]

On 04/08/16 22:24, Mimi Zohar wrote:
> The TPM PCRs are only reset on a hard reboot.  In order to validate a
> TPM's quote after a soft reboot (eg. kexec -e), the IMA measurement list
> of the running kernel must be saved and then restored on the subsequent
> boot.
> 
> The existing securityfs binary_runtime_measurements file conveniently
> provides a serialized format of the IMA measurement list. This patch
> set serializes the measurement list in this format and restores it.
> 
> This patch set pre-req's Thiago Bauermann's "kexec_file: Add buffer
> hand-over for the next kernel" patch set* for actually carrying the
> serialized measurement list across the kexec.
> 
> Mimi
> 

Hi, Mimi

I am trying to convince myself of the security of the solution. I asked
Thiago as well, but may be I am be lagging behind in understanding.

We trust the kernel to hand over PCR values of the old kernel (which
cannot be validated) to the IMA subsystem in the new kernel for storage.
I guess the idea is for ima_add_boot_aggregate to do the right thing?
How do we validate what the old kernel is giving us? Why do we care for
the old measurement list? Is it still of significance in the new kernel?


Balbir Singh.

Re: [PATCH 1/7] ima: on soft reboot, restore the measurement list

[Michael Ellerman]

Mimi Zohar <zohar@linux.vnet.ibm.com> writes:

> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index b5728da..84e8d36 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -102,6 +102,13 @@ struct ima_queue_entry {
>  };
>  extern struct list_head ima_measurements;	/* list of all measurements */
>  
> +/* Some details preceding the binary serialized measurement list */
> +struct ima_kexec_hdr {
> +	unsigned short version;
> +	unsigned long buffer_size;
> +	unsigned long count;
> +} __packed;
> +

Am I understanding it correctly that this structure is passed between kernels?

If so it's an ABI and should use types with well defined sizes, as if it was
going out to userspace, shouldn't it?

cheers

Re: [PATCH 0/7] ima: carry the measurement list across kexec

[Mimi Zohar]

On Tue, 2016-08-09 at 15:19 +1000, Balbir Singh wrote:
> 
> On 04/08/16 22:24, Mimi Zohar wrote:
> > The TPM PCRs are only reset on a hard reboot.  In order to validate a
> > TPM's quote after a soft reboot (eg. kexec -e), the IMA measurement list
> > of the running kernel must be saved and then restored on the subsequent
> > boot.
> > 
> > The existing securityfs binary_runtime_measurements file conveniently
> > provides a serialized format of the IMA measurement list. This patch
> > set serializes the measurement list in this format and restores it.
> > 
> > This patch set pre-req's Thiago Bauermann's "kexec_file: Add buffer
> > hand-over for the next kernel" patch set* for actually carrying the
> > serialized measurement list across the kexec.
> > 
> > Mimi
> > 
> 
> Hi, Mimi
> 
> I am trying to convince myself of the security of the solution. I asked
> Thiago as well, but may be I am be lagging behind in understanding.
> 
> We trust the kernel to hand over PCR values of the old kernel (which
> cannot be validated) to the IMA subsystem in the new kernel for storage.
> I guess the idea is for ima_add_boot_aggregate to do the right thing?
> How do we validate what the old kernel is giving us? Why do we care for
> the old measurement list? Is it still of significance in the new kernel?
> 

Hi Balbir,

To validate the hardware TPM PCR values requires walking the measurement
list simulating the TPM extend operation.  The resulting values should
match the hardware TPM PCRs.

In the case of a soft reboot, the TPM PCRs are not reset to 0, so all
the measurements of the running system, including those from previous
soft reboots, need to be included in the measurement list.   Without
these measurements, the simulated PCR values will not match the hardware
TPM PCR values.  Thus the need for this patch set.

Measurements can not be added/removed/changed in the measurement list
without it being detectable.

Mimi

Re: [PATCH 1/7] ima: on soft reboot, restore the measurement list

[Mimi Zohar]

On Tue, 2016-08-09 at 20:59 +1000, Michael Ellerman wrote:
> Mimi Zohar <zohar@linux.vnet.ibm.com> writes:
> 
> > diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> > index b5728da..84e8d36 100644
> > --- a/security/integrity/ima/ima.h
> > +++ b/security/integrity/ima/ima.h
> > @@ -102,6 +102,13 @@ struct ima_queue_entry {
> >  };
> >  extern struct list_head ima_measurements;	/* list of all measurements */
> >  
> > +/* Some details preceding the binary serialized measurement list */
> > +struct ima_kexec_hdr {
> > +	unsigned short version;
> > +	unsigned long buffer_size;
> > +	unsigned long count;
> > +} __packed;
> > +
> 
> Am I understanding it correctly that this structure is passed between kernels?

Yes, the header prefixes the measurement list, which is being passed on
the same computer to the next kernel.  Could the architecture (eg.
LE/BE) change between soft re-boots?

> If so it's an ABI and should use types with well defined sizes, as if it was
> going out to userspace, shouldn't it?

Ok

Mimi

Re: [PATCH 1/7] ima: on soft reboot, restore the measurement list

[Thiago Jung Bauermann]

Am Dienstag, 09 August 2016, 09:01:13 schrieb Mimi Zohar:
> On Tue, 2016-08-09 at 20:59 +1000, Michael Ellerman wrote:
> > Mimi Zohar <zohar@linux.vnet.ibm.com> writes:
> > > diff --git a/security/integrity/ima/ima.h
> > > b/security/integrity/ima/ima.h
> > > index b5728da..84e8d36 100644
> > > --- a/security/integrity/ima/ima.h
> > > +++ b/security/integrity/ima/ima.h
> > > @@ -102,6 +102,13 @@ struct ima_queue_entry {
> > > 
> > >  };
> > >  extern struct list_head ima_measurements;	/* list of all 
measurements
> > >  */
> > > 
> > > +/* Some details preceding the binary serialized measurement list */
> > > +struct ima_kexec_hdr {
> > > +	unsigned short version;
> > > +	unsigned long buffer_size;
> > > +	unsigned long count;
> > > +} __packed;
> > > +
> > 
> > Am I understanding it correctly that this structure is passed between
> > kernels?
> Yes, the header prefixes the measurement list, which is being passed on
> the same computer to the next kernel.  Could the architecture (eg.
> LE/BE) change between soft re-boots?

Yes. I am able to boot a BE kernel from an LE kernel with my patches. 
Whether we want to support that or not is another question...
-- 
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center

RE: [PATCH 1/7] ima: on soft reboot, restore the measurement list

[David Laight]

From: Thiago Jung Bauermann
> Sent: 09 August 2016 14:19
...
> > > > +/* Some details preceding the binary serialized measurement list */
> > > > +struct ima_kexec_hdr {
> > > > +	unsigned short version;
> > > > +	unsigned long buffer_size;
> > > > +	unsigned long count;
> > > > +} __packed;
> > > > +
> > >
> > > Am I understanding it correctly that this structure is passed between
> > > kernels?
> > Yes, the header prefixes the measurement list, which is being passed on
> > the same computer to the next kernel.  Could the architecture (eg.
> > LE/BE) change between soft re-boots?
> 
> Yes. I am able to boot a BE kernel from an LE kernel with my patches.

64bit kernel to/from 32bit one? (if that makes sense on the hardware).

> Whether we want to support that or not is another question...

In which case shouldn't they be annotated with the endianness??

Also why '__packed' - guarantees sub-optimal code generation.
Much better to include explicit padding to align everything.

	David

Re: [PATCH 1/7] ima: on soft reboot, restore the measurement list

[Mimi Zohar]

On Tue, 2016-08-09 at 10:19 -0300, Thiago Jung Bauermann wrote:
> Am Dienstag, 09 August 2016, 09:01:13 schrieb Mimi Zohar:
> > On Tue, 2016-08-09 at 20:59 +1000, Michael Ellerman wrote:
> > > Mimi Zohar <zohar@linux.vnet.ibm.com> writes:
> > > > diff --git a/security/integrity/ima/ima.h
> > > > b/security/integrity/ima/ima.h
> > > > index b5728da..84e8d36 100644
> > > > --- a/security/integrity/ima/ima.h
> > > > +++ b/security/integrity/ima/ima.h
> > > > @@ -102,6 +102,13 @@ struct ima_queue_entry {
> > > > 
> > > >  };
> > > >  extern struct list_head ima_measurements;	/* list of all 
> measurements
> > > >  */
> > > > 
> > > > +/* Some details preceding the binary serialized measurement list */
> > > > +struct ima_kexec_hdr {
> > > > +	unsigned short version;
> > > > +	unsigned long buffer_size;
> > > > +	unsigned long count;
> > > > +} __packed;
> > > > +
> > > 
> > > Am I understanding it correctly that this structure is passed between
> > > kernels?
> > Yes, the header prefixes the measurement list, which is being passed on
> > the same computer to the next kernel.  Could the architecture (eg.
> > LE/BE) change between soft re-boots?
> 
> Yes. I am able to boot a BE kernel from an LE kernel with my patches. 
> Whether we want to support that or not is another question...

The <securityfs/ima/binary_runtime_measurements is system architecture
dependent.  It looks like the khdr->version check in
ima_restore_measurement_list() would fail if the architecture changes.

If/when we update the binary measurement list format to support multiple
TPM PCRs, we should address the endianness as well.

Mimi

Re: [PATCH 1/7] ima: on soft reboot, restore the measurement list

[Mimi Zohar]

On Tue, 2016-08-09 at 13:35 +0000, David Laight wrote:

> Also why '__packed' - guarantees sub-optimal code generation.
> Much better to include explicit padding to align everything.

This patch set does not define a new format, but piggy backs on top of
the existing <securityfs>/ima/binary_runtime_measurements list.  The
prefixed buffer header includes a version, so that if in the future we
need to modify the format, we would be able to.

In terms of the prefixed header, how would you define the fields:
version, buffer size, number of measurements? 

Mimi

Re: [PATCH 1/7] ima: on soft reboot, restore the measurement list

[Mimi Zohar]

On Tue, 2016-08-09 at 09:55 -0400, Mimi Zohar wrote:
> On Tue, 2016-08-09 at 10:19 -0300, Thiago Jung Bauermann wrote:
> > Am Dienstag, 09 August 2016, 09:01:13 schrieb Mimi Zohar:
> > > On Tue, 2016-08-09 at 20:59 +1000, Michael Ellerman wrote:
> > > > Mimi Zohar <zohar@linux.vnet.ibm.com> writes:
> > > > > diff --git a/security/integrity/ima/ima.h
> > > > > b/security/integrity/ima/ima.h
> > > > > index b5728da..84e8d36 100644
> > > > > --- a/security/integrity/ima/ima.h
> > > > > +++ b/security/integrity/ima/ima.h
> > > > > @@ -102,6 +102,13 @@ struct ima_queue_entry {
> > > > > 
> > > > >  };
> > > > >  extern struct list_head ima_measurements;	/* list of all 
> > measurements
> > > > >  */
> > > > > 
> > > > > +/* Some details preceding the binary serialized measurement list */
> > > > > +struct ima_kexec_hdr {
> > > > > +	unsigned short version;
> > > > > +	unsigned long buffer_size;
> > > > > +	unsigned long count;
> > > > > +} __packed;
> > > > > +
> > > > 
> > > > Am I understanding it correctly that this structure is passed between
> > > > kernels?
> > > Yes, the header prefixes the measurement list, which is being passed on
> > > the same computer to the next kernel.  Could the architecture (eg.
> > > LE/BE) change between soft re-boots?
> > 
> > Yes. I am able to boot a BE kernel from an LE kernel with my patches. 
> > Whether we want to support that or not is another question...
> 
> The <securityfs/ima/binary_runtime_measurements is system architecture
> dependent.  It looks like the khdr->version check in
> ima_restore_measurement_list() would fail if the architecture changes.
> 
> If/when we update the binary measurement list format to support multiple
> TPM PCRs, we should address the endianness as well.

That should have been "TPM PCR banks".  TPM 2.0 allows for multiple TPM
PCR banks.

Mimi

Re: [PATCH 1/7] ima: on soft reboot, restore the measurement list

[Samuel Mendoza-Jonas]

On Tue, 2016-08-09 at 10:19 -0300, Thiago Jung Bauermann wrote:
> Am Dienstag, 09 August 2016, 09:01:13 schrieb Mimi Zohar:
> > 
> > On Tue, 2016-08-09 at 20:59 +1000, Michael Ellerman wrote:
> > > 
> > > Mimi Zohar <zohar@linux.vnet.ibm.com> writes:
> > > > 
> > > > diff --git a/security/integrity/ima/ima.h
> > > > b/security/integrity/ima/ima.h
> > > > index b5728da..84e8d36 100644
> > > > --- a/security/integrity/ima/ima.h
> > > > +++ b/security/integrity/ima/ima.h
> > > > @@ -102,6 +102,13 @@ struct ima_queue_entry {
> > > > 
> > > >  };
> > > >  extern struct list_head ima_measurements;	/* list of all 
> measurements
> > 
> > > 
> > > > 
> > > >  */
> > > > 
> > > > +/* Some details preceding the binary serialized measurement list */
> > > > +struct ima_kexec_hdr {
> > > > +	unsigned short version;
> > > > +	unsigned long buffer_size;
> > > > +	unsigned long count;
> > > > +} __packed;
> > > > +
> > > 
> > > Am I understanding it correctly that this structure is passed between
> > > kernels?
> > Yes, the header prefixes the measurement list, which is being passed on
> > the same computer to the next kernel.  Could the architecture (eg.
> > LE/BE) change between soft re-boots?
> 
> Yes. I am able to boot a BE kernel from an LE kernel with my patches. 
> Whether we want to support that or not is another question...

The answer to that question is almost certainly yes - on POWER the most
immediate example would be soft-rebooting from a BE host into an LE
Petitboot kernel, or vice versa.

Re: [PATCH 1/7] ima: on soft reboot, restore the measurement list

[Michael Ellerman]

Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com> writes:

> Am Dienstag, 09 August 2016, 09:01:13 schrieb Mimi Zohar:
>> On Tue, 2016-08-09 at 20:59 +1000, Michael Ellerman wrote:
>> > Mimi Zohar <zohar@linux.vnet.ibm.com> writes:
>> > > diff --git a/security/integrity/ima/ima.h
>> > > b/security/integrity/ima/ima.h
>> > > index b5728da..84e8d36 100644
>> > > --- a/security/integrity/ima/ima.h
>> > > +++ b/security/integrity/ima/ima.h
>> > > @@ -102,6 +102,13 @@ struct ima_queue_entry {
>> > > 
>> > >  };
>> > >  extern struct list_head ima_measurements;	/* list of all 
> measurements
>> > >  */
>> > > 
>> > > +/* Some details preceding the binary serialized measurement list */
>> > > +struct ima_kexec_hdr {
>> > > +	unsigned short version;
>> > > +	unsigned long buffer_size;
>> > > +	unsigned long count;
>> > > +} __packed;
>> > > +
>> > 
>> > Am I understanding it correctly that this structure is passed between
>> > kernels?
>> Yes, the header prefixes the measurement list, which is being passed on
>> the same computer to the next kernel.  Could the architecture (eg.
>> LE/BE) change between soft re-boots?
>
> Yes. I am able to boot a BE kernel from an LE kernel with my patches. 
> Whether we want to support that or not is another question...

Yes you must support that. BE -> LE and vice versa.

You should also consider the possibility that the next kernel is not
Linux.

cheers

Re: [PATCH 1/7] ima: on soft reboot, restore the measurement list

[Thiago Jung Bauermann]

Am Mittwoch, 10 August 2016, 13:41:08 schrieb Michael Ellerman:
> Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com> writes:
> > Am Dienstag, 09 August 2016, 09:01:13 schrieb Mimi Zohar:
> >> On Tue, 2016-08-09 at 20:59 +1000, Michael Ellerman wrote:
> >> > Mimi Zohar <zohar@linux.vnet.ibm.com> writes: 
> >> > > +/* Some details preceding the binary serialized measurement list
> >> > > */
> >> > > +struct ima_kexec_hdr {
> >> > > +	unsigned short version;
> >> > > +	unsigned long buffer_size;
> >> > > +	unsigned long count;
> >> > > +} __packed;
> >> > > +
> >> > 
> >> > Am I understanding it correctly that this structure is passed between
> >> > kernels?
> >> 
> >> Yes, the header prefixes the measurement list, which is being passed on
> >> the same computer to the next kernel.  Could the architecture (eg.
> >> LE/BE) change between soft re-boots?
> > 
> > Yes. I am able to boot a BE kernel from an LE kernel with my patches.
> > Whether we want to support that or not is another question...
> 
> Yes you must support that. BE -> LE and vice versa.

I didn't test BE - LE yet, but will do.

> You should also consider the possibility that the next kernel is not
> Linux.

If the next kernel is an ELF binary and it supports the kexec "calling 
convention", it should work too. What could possibly go wrong? I can try 
FreeBSD (I suppose it's an ELF kernel) and see what happens.

-- 
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center

Re: [PATCH 1/7] ima: on soft reboot, restore the measurement list

[Michael Ellerman]

Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com> writes:

> Am Mittwoch, 10 August 2016, 13:41:08 schrieb Michael Ellerman:
>> Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com> writes:
>> > Am Dienstag, 09 August 2016, 09:01:13 schrieb Mimi Zohar:
>> >> On Tue, 2016-08-09 at 20:59 +1000, Michael Ellerman wrote:
>> >> > Mimi Zohar <zohar@linux.vnet.ibm.com> writes: 
>> >> > > +/* Some details preceding the binary serialized measurement list
>> >> > > */
>> >> > > +struct ima_kexec_hdr {
>> >> > > +	unsigned short version;
>> >> > > +	unsigned long buffer_size;
>> >> > > +	unsigned long count;
>> >> > > +} __packed;
>> >> > > +
>> >> > 
>> >> > Am I understanding it correctly that this structure is passed between
>> >> > kernels?
>> >> 
>> >> Yes, the header prefixes the measurement list, which is being passed on
>> >> the same computer to the next kernel.  Could the architecture (eg.
>> >> LE/BE) change between soft re-boots?
>> > 
>> > Yes. I am able to boot a BE kernel from an LE kernel with my patches.
>> > Whether we want to support that or not is another question...
>> 
>> Yes you must support that. BE -> LE and vice versa.
>
> I didn't test BE - LE yet, but will do.

Thanks.

>> You should also consider the possibility that the next kernel is not
>> Linux.
>
> If the next kernel is an ELF binary and it supports the kexec "calling 
> convention", it should work too. What could possibly go wrong? I can try 
> FreeBSD (I suppose it's an ELF kernel) and see what happens.

At least for old style kexec (not sys_kexec_load()) I don't think it
even needs to be an ELF binary.

I think there are folks working on FreeBSD (or $?BSD), so I think the
basic kexec part works.

There's nothing (yet) that wants to use this measurement list obviously,
but it should be designed such that it could be used by an unknown
future kernel that knows the ABI.

So given what you have above, you'd use something like:

struct ima_kexec_hdr {
	u16 version;
	u16 _reserved0;
	u32 _reserved1;
	u64 buffer_size;
	u64 count;
};

cheers

Re: [PATCH 1/7] ima: on soft reboot, restore the measurement list

[Mimi Zohar]

On Wed, 2016-08-10 at 19:52 +1000, Michael Ellerman wrote:
> Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com> writes:
> 
> > Am Mittwoch, 10 August 2016, 13:41:08 schrieb Michael Ellerman:
> >> Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com> writes:
> >> > Am Dienstag, 09 August 2016, 09:01:13 schrieb Mimi Zohar:
> >> >> On Tue, 2016-08-09 at 20:59 +1000, Michael Ellerman wrote:
> >> >> > Mimi Zohar <zohar@linux.vnet.ibm.com> writes: 
> >> >> > > +/* Some details preceding the binary serialized measurement list
> >> >> > > */
> >> >> > > +struct ima_kexec_hdr {
> >> >> > > +	unsigned short version;
> >> >> > > +	unsigned long buffer_size;
> >> >> > > +	unsigned long count;
> >> >> > > +} __packed;
> >> >> > > +
> >> >> > 
> >> >> > Am I understanding it correctly that this structure is passed between
> >> >> > kernels?
> >> >> 
> >> >> Yes, the header prefixes the measurement list, which is being passed on
> >> >> the same computer to the next kernel.  Could the architecture (eg.
> >> >> LE/BE) change between soft re-boots?
> >> > 
> >> > Yes. I am able to boot a BE kernel from an LE kernel with my patches.
> >> > Whether we want to support that or not is another question...
> >> 
> >> Yes you must support that. BE -> LE and vice versa.
> >
> > I didn't test BE - LE yet, but will do.
> 
> Thanks.

Ok. There have been requests for making the binary_runtime_measurements
architecture independent.  As this was not a network facing interface,
we left it in native format.  With the kernel now consuming this data,
it makes sense for the binary_runtime_measurements to be in an
architecture independent format.

Unfortunately, as the <securityfs>/ima/binary_runtime_measurements is
not prefixed with any metadata, this change would need to be Kconfig
based, but kexec would always use the architecture independent format.

> >> You should also consider the possibility that the next kernel is not
> >> Linux.

Oh!

> > If the next kernel is an ELF binary and it supports the kexec "calling 
> > convention", it should work too. What could possibly go wrong? I can try 
> > FreeBSD (I suppose it's an ELF kernel) and see what happens.
> 
> At least for old style kexec (not sys_kexec_load()) I don't think it
> even needs to be an ELF binary.
> 
> I think there are folks working on FreeBSD (or $?BSD), so I think the
> basic kexec part works.
> 
> There's nothing (yet) that wants to use this measurement list obviously,
> but it should be designed such that it could be used by an unknown
> future kernel that knows the ABI.
> 
> So given what you have above, you'd use something like:
> 
> struct ima_kexec_hdr {
> 	u16 version;
> 	u16 _reserved0;
> 	u32 _reserved1;
> 	u64 buffer_size;
> 	u64 count;
> };
> 
> cheers

Thanks, I'll make this change.

Mimi

Re: [Linux-ima-devel] [PATCH 1/7] ima: on soft reboot, restore the measurement list

[Petko Manolov]

On 16-08-10 08:54:36, Mimi Zohar wrote:
> On Wed, 2016-08-10 at 19:52 +1000, Michael Ellerman wrote:
> > Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com> writes:
> > 
> > > Am Mittwoch, 10 August 2016, 13:41:08 schrieb Michael Ellerman:
> > >> Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com> writes:
> > >> > Am Dienstag, 09 August 2016, 09:01:13 schrieb Mimi Zohar:
> > >> >> On Tue, 2016-08-09 at 20:59 +1000, Michael Ellerman wrote:
> > >> >> > Mimi Zohar <zohar@linux.vnet.ibm.com> writes: 
> > >> >> > > +/* Some details preceding the binary serialized measurement list
> > >> >> > > */
> > >> >> > > +struct ima_kexec_hdr {
> > >> >> > > +	unsigned short version;
> > >> >> > > +	unsigned long buffer_size;
> > >> >> > > +	unsigned long count;
> > >> >> > > +} __packed;
> > >> >> > > +
> > >> >> > 
> > >> >> > Am I understanding it correctly that this structure is passed between
> > >> >> > kernels?
> > >> >> 
> > >> >> Yes, the header prefixes the measurement list, which is being passed on
> > >> >> the same computer to the next kernel.  Could the architecture (eg.
> > >> >> LE/BE) change between soft re-boots?
> > >> > 
> > >> > Yes. I am able to boot a BE kernel from an LE kernel with my patches.
> > >> > Whether we want to support that or not is another question...
> > >> 
> > >> Yes you must support that. BE -> LE and vice versa.
> > >
> > > I didn't test BE - LE yet, but will do.
> > 
> > Thanks.
> 
> Ok. There have been requests for making the binary_runtime_measurements
> architecture independent.  As this was not a network facing interface,
> we left it in native format.  With the kernel now consuming this data,
> it makes sense for the binary_runtime_measurements to be in an
> architecture independent format.
> 
> Unfortunately, as the <securityfs>/ima/binary_runtime_measurements is
> not prefixed with any metadata, this change would need to be Kconfig
> based, but kexec would always use the architecture independent format.
> 
> > >> You should also consider the possibility that the next kernel is not
> > >> Linux.
> 
> Oh!
> 
> > > If the next kernel is an ELF binary and it supports the kexec "calling 
> > > convention", it should work too. What could possibly go wrong? I can try 
> > > FreeBSD (I suppose it's an ELF kernel) and see what happens.
> > 
> > At least for old style kexec (not sys_kexec_load()) I don't think it
> > even needs to be an ELF binary.
> > 
> > I think there are folks working on FreeBSD (or $?BSD), so I think the
> > basic kexec part works.
> > 
> > There's nothing (yet) that wants to use this measurement list obviously,
> > but it should be designed such that it could be used by an unknown
> > future kernel that knows the ABI.
> > 
> > So given what you have above, you'd use something like:
> > 
> > struct ima_kexec_hdr {
> > 	u16 version;
> > 	u16 _reserved0;
> > 	u32 _reserved1;
> > 	u64 buffer_size;
> > 	u64 count;
> > };
> > 
> > cheers
> 
> Thanks, I'll make this change.

I would suggest:

struct ima_kexec_hdr {
	u64 buffer_size;
	u64 count;
	u16 version;
};

and let the compiler add the proper padding, depending on the architecture.  On 
32bit machine we'll have 4 bytes smaller allocations (compared to 64bit) while 
retaining the same functionality.


cheers,
Petko

RE: [Linux-ima-devel] [PATCH 1/7] ima: on soft reboot, restore the measurement list

[David Laight]

From: Linuxppc-dev [mailto:linuxppc-dev-bounces+david.laight=aculab.com@lists.ozlabs.org] On Behalf Of
> > > So given what you have above, you'd use something like:
> > >
> > > struct ima_kexec_hdr {
> > > 	u16 version;
> > > 	u16 _reserved0;
> > > 	u32 _reserved1;
> > > 	u64 buffer_size;
> > > 	u64 count;
> > > };
> > >
> > > cheers
> >
> > Thanks, I'll make this change.
> 
> I would suggest:
> 
> struct ima_kexec_hdr {
> 	u64 buffer_size;
> 	u64 count;
> 	u16 version;
> };
> 
> and let the compiler add the proper padding, depending on the architecture.  On
> 32bit machine we'll have 4 bytes smaller allocations (compared to 64bit) while
> retaining the same functionality.

AAAArrrrgggg.....

That doesn't work for 32bit applications on 64bit hosts.
The extra bytes will make 0 difference to the allocation cost and
lots to the processing.

	David

Re: [Linux-ima-devel] [PATCH 1/7] ima: on soft reboot, restore the measurement list

[Petko Manolov]

On 16-08-10 14:40:13, David Laight wrote:
> From: Linuxppc-dev [mailto:linuxppc-dev-bounces+david.laight=aculab.com@lists.ozlabs.org] On Behalf Of
> > > > So given what you have above, you'd use something like:
> > > >
> > > > struct ima_kexec_hdr {
> > > > 	u16 version;
> > > > 	u16 _reserved0;
> > > > 	u32 _reserved1;
> > > > 	u64 buffer_size;
> > > > 	u64 count;
> > > > };
> > > >
> > > > cheers
> > >
> > > Thanks, I'll make this change.
> > 
> > I would suggest:
> > 
> > struct ima_kexec_hdr {
> > 	u64 buffer_size;
> > 	u64 count;
> > 	u16 version;
> > };
> > 
> > and let the compiler add the proper padding, depending on the architecture.  On
> > 32bit machine we'll have 4 bytes smaller allocations (compared to 64bit) while
> > retaining the same functionality.
> 
> AAAArrrrgggg.....
> 
> That doesn't work for 32bit applications on 64bit hosts.

Which part won't work?

> The extra bytes will make 0 difference to the allocation cost and lots to the 
> processing.

An example?


		Petko

Re: [PATCH 0/7] ima: carry the measurement list across kexec

[Balbir Singh]

On 09/08/16 22:36, Mimi Zohar wrote:
> On Tue, 2016-08-09 at 15:19 +1000, Balbir Singh wrote:
>>
>> On 04/08/16 22:24, Mimi Zohar wrote:
>>> The TPM PCRs are only reset on a hard reboot.  In order to validate a
>>> TPM's quote after a soft reboot (eg. kexec -e), the IMA measurement list
>>> of the running kernel must be saved and then restored on the subsequent
>>> boot.
>>>
>>> The existing securityfs binary_runtime_measurements file conveniently
>>> provides a serialized format of the IMA measurement list. This patch
>>> set serializes the measurement list in this format and restores it.
>>>
>>> This patch set pre-req's Thiago Bauermann's "kexec_file: Add buffer
>>> hand-over for the next kernel" patch set* for actually carrying the
>>> serialized measurement list across the kexec.
>>>
>>> Mimi
>>>
>>
>> Hi, Mimi
>>
>> I am trying to convince myself of the security of the solution. I asked
>> Thiago as well, but may be I am be lagging behind in understanding.
>>
>> We trust the kernel to hand over PCR values of the old kernel (which
>> cannot be validated) to the IMA subsystem in the new kernel for storage.
>> I guess the idea is for ima_add_boot_aggregate to do the right thing?
>> How do we validate what the old kernel is giving us? Why do we care for
>> the old measurement list? Is it still of significance in the new kernel?
>>
> 
> Hi Balbir,
> 
> To validate the hardware TPM PCR values requires walking the measurement
> list simulating the TPM extend operation.  The resulting values should
> match the hardware TPM PCRs.
> 
> In the case of a soft reboot, the TPM PCRs are not reset to 0, so all
> the measurements of the running system, including those from previous
> soft reboots, need to be included in the measurement list.   Without
> these measurements, the simulated PCR values will not match the hardware
> TPM PCR values.  Thus the need for this patch set.
> 
> Measurements can not be added/removed/changed in the measurement list
> without it being detectable.
> 

Thanks Mimi

I think that makes sense

So effectively we do

first kernel boot -> <measurements match PCR and measurements are saved>
second kernel boot -> <new PCR = first save measurements + new measurements>

and so on

Balbir Singh

Re: [PATCH 0/7] ima: carry the measurement list across kexec

[Mimi Zohar]

On Thu, 2016-08-11 at 17:38 +1000, Balbir Singh wrote:
> 
> On 09/08/16 22:36, Mimi Zohar wrote:
> > On Tue, 2016-08-09 at 15:19 +1000, Balbir Singh wrote:
> >>
> >> On 04/08/16 22:24, Mimi Zohar wrote:
> >>> The TPM PCRs are only reset on a hard reboot.  In order to validate a
> >>> TPM's quote after a soft reboot (eg. kexec -e), the IMA measurement list
> >>> of the running kernel must be saved and then restored on the subsequent
> >>> boot.
> >>>
> >>> The existing securityfs binary_runtime_measurements file conveniently
> >>> provides a serialized format of the IMA measurement list. This patch
> >>> set serializes the measurement list in this format and restores it.
> >>>
> >>> This patch set pre-req's Thiago Bauermann's "kexec_file: Add buffer
> >>> hand-over for the next kernel" patch set* for actually carrying the
> >>> serialized measurement list across the kexec.
> >>>
> >>> Mimi
> >>>
> >>
> >> Hi, Mimi
> >>
> >> I am trying to convince myself of the security of the solution. I asked
> >> Thiago as well, but may be I am be lagging behind in understanding.
> >>
> >> We trust the kernel to hand over PCR values of the old kernel (which
> >> cannot be validated) to the IMA subsystem in the new kernel for storage.
> >> I guess the idea is for ima_add_boot_aggregate to do the right thing?
> >> How do we validate what the old kernel is giving us? Why do we care for
> >> the old measurement list? Is it still of significance in the new kernel?
> >>
> > 
> > Hi Balbir,
> > 
> > To validate the hardware TPM PCR values requires walking the measurement
> > list simulating the TPM extend operation.  The resulting values should
> > match the hardware TPM PCRs.
> > 
> > In the case of a soft reboot, the TPM PCRs are not reset to 0, so all
> > the measurements of the running system, including those from previous
> > soft reboots, need to be included in the measurement list.   Without
> > these measurements, the simulated PCR values will not match the hardware
> > TPM PCR values.  Thus the need for this patch set.
> > 
> > Measurements can not be added/removed/changed in the measurement list
> > without it being detectable.
> > 
> 
> Thanks Mimi
> 
> I think that makes sense
> 
> So effectively we do
> 
> first kernel boot -> <measurements match PCR and measurements are saved>
> second kernel boot -> <new PCR = first save measurements + new measurements>
> 
> and so on

No, the running system doesn't verify the measurement list against the
PCRs, before saving and carrying it across kexec. If the system has been
compromised, it can't be trusted to verify itself.  Verifying the
measurement list needs to be done by a trusted third party.  The system
just carries the measurement list(s) across kexec.

Mimi