[PATCH 0/7] IMA: new parser for ima_restore_measurement_list()

[PATCH 0/7] IMA: new parser for ima_restore_measurement_list()

[Roberto Sassu]

A new IMA measurement list format, called Crypto Agile, will be introduced
shortly to take full advantage of the algorithm flexibility of TPM 2.0.
With the new format, it will be possible to provide for each list entry
multiple digests, each calculated with an algorithm supported by the TPM.
Those digests will be used by remote entities to verify the integrity of
the measurements list.

The current (SHA1) and the new (Crypto Agile) format definitions are:

SHA1: pcr[4] digest[20]
      template_name_len[4] template_name[template_name_len]
      template_data_len[4] template_data[template_data_len]

Crypto Agile: pcr[4] total_digest_len[4]
              digest1_len[4] digest1[digest1_len] ...
              digestN_len[4] digestN[digestN_len]
              template_name_len[4] template_name[template_name_len]
              template_data_len[4] template_data[template_data_len]

IMA must be able to parse lists with these formats, in order to restore
a measurement list after kexec(). This functionality is implemented
in ima_restore_measurement_list().

With the SHA1 format, the measurement entry header is parsed by casting
a buffer to two structures (binary_hdr_v1 and binary_data_v1), whose
members have the same length of the fields mentioned above.

Since clang does not support variable-length arrays in structures (VLAIS),
the binary_hdr_v1 should be split in two parts, to parse the Crypto Agile
format: one for pcr and digests with variable length, and the other
for template name. This would make the parsing code more complex,
as casting would have been done with three structures.

Given that in most cases, in the binary format of measurements, a length
field is prepended to data, a better solution would be to use only one
function to parse all length-data combinations.

This patch set introduces a new function, called ima_parse_buf(), which
takes as input a buffer and writes lengths and pointers of parsed data
to an array of ima_field_data structures, currently used for template
data fields. ima_restore_measurement_list() and ima_restore_template_data()
are modified to use the new function.

This patch set also introduces two new securityfs interfaces, for testing
purposes, to save the binary measurements list with the kexec header and
to restore it.

Finally, a list entry size calculation issue is fixed in the last patch.

Roberto Sassu (7):
  ima: introduce ima_parse_buf()
  ima: use ima_parse_buf() to parse measurements headers
  ima: use ima_parse_buf() to parse template data
  ima: declare get_binary_runtime_size() as non-static
  ima: add securityfs interface to save a measurements list with kexec
    header
  ima: add securityfs interface to restore a measurements list
  ima: fix get_binary_runtime_size()

 security/integrity/ima/Kconfig            |   8 ++
 security/integrity/ima/ima.h              |   3 +
 security/integrity/ima/ima_fs.c           |  80 +++++++++++++++++--
 security/integrity/ima/ima_kexec.c        |   2 +-
 security/integrity/ima/ima_queue.c        |   6 +-
 security/integrity/ima/ima_template.c     | 126 ++++++++++--------------------
 security/integrity/ima/ima_template_lib.c |  80 +++++++++++++++++++
 security/integrity/ima/ima_template_lib.h |   8 ++
 8 files changed, 217 insertions(+), 96 deletions(-)

-- 
2.9.3

[PATCH 0/7] IMA: new parser for ima_restore_measurement_list()

[Roberto Sassu]

A new IMA measurement list format, called Crypto Agile, will be introduced
shortly to take full advantage of the algorithm flexibility of TPM 2.0.
With the new format, it will be possible to provide for each list entry
multiple digests, each calculated with an algorithm supported by the TPM.
Those digests will be used by remote entities to verify the integrity of
the measurements list.

The current (SHA1) and the new (Crypto Agile) format definitions are:

SHA1: pcr[4] digest[20]
      template_name_len[4] template_name[template_name_len]
      template_data_len[4] template_data[template_data_len]

Crypto Agile: pcr[4] total_digest_len[4]
              digest1_len[4] digest1[digest1_len] ...
              digestN_len[4] digestN[digestN_len]
              template_name_len[4] template_name[template_name_len]
              template_data_len[4] template_data[template_data_len]

IMA must be able to parse lists with these formats, in order to restore
a measurement list after kexec(). This functionality is implemented
in ima_restore_measurement_list().

With the SHA1 format, the measurement entry header is parsed by casting
a buffer to two structures (binary_hdr_v1 and binary_data_v1), whose
members have the same length of the fields mentioned above.

Since clang does not support variable-length arrays in structures (VLAIS),
the binary_hdr_v1 should be split in two parts, to parse the Crypto Agile
format: one for pcr and digests with variable length, and the other
for template name. This would make the parsing code more complex,
as casting would have been done with three structures.

Given that in most cases, in the binary format of measurements, a length
field is prepended to data, a better solution would be to use only one
function to parse all length-data combinations.

This patch set introduces a new function, called ima_parse_buf(), which
takes as input a buffer and writes lengths and pointers of parsed data
to an array of ima_field_data structures, currently used for template
data fields. ima_restore_measurement_list() and ima_restore_template_data()
are modified to use the new function.

This patch set also introduces two new securityfs interfaces, for testing
purposes, to save the binary measurements list with the kexec header and
to restore it.

Finally, a list entry size calculation issue is fixed in the last patch.

Roberto Sassu (7):
  ima: introduce ima_parse_buf()
  ima: use ima_parse_buf() to parse measurements headers
  ima: use ima_parse_buf() to parse template data
  ima: declare get_binary_runtime_size() as non-static
  ima: add securityfs interface to save a measurements list with kexec
    header
  ima: add securityfs interface to restore a measurements list
  ima: fix get_binary_runtime_size()

 security/integrity/ima/Kconfig            |   8 ++
 security/integrity/ima/ima.h              |   3 +
 security/integrity/ima/ima_fs.c           |  80 +++++++++++++++++--
 security/integrity/ima/ima_kexec.c        |   2 +-
 security/integrity/ima/ima_queue.c        |   6 +-
 security/integrity/ima/ima_template.c     | 126 ++++++++++--------------------
 security/integrity/ima/ima_template_lib.c |  80 +++++++++++++++++++
 security/integrity/ima/ima_template_lib.h |   8 ++
 8 files changed, 217 insertions(+), 96 deletions(-)

-- 
2.9.3

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/7] ima: introduce ima_parse_buf()

[Roberto Sassu]

ima_parse_buf() takes as input the buffer start and end pointers, and
stores the result in a static array of ima_field_data structures,
where the len field contains the length parsed from the buffer, and
the data field contains the address of the buffer just after the length.
Optionally, the function returns the current value of the buffer pointer
and the number of array elements written.

A bitmap has been added as parameter of ima_parse_buf() to handle
the cases where the length is not prepended to data. Each bit corresponds
to an element of the ima_field_data array. If a bit is set, the length
is not parsed from the buffer, but is read from the corresponding element
of the array (the length must be set before calling the function).

ima_parse_buf() can perform three checks upon request by callers,
depending on the enforce mask passed to it:

- ENFORCE_FIELDS: matching of number of fields (length-data combination)
  - there must be enough data in the buffer to parse the number of fields
    requested (output: current value of buffer pointer)
- ENFORCE_BUFEND: matching of buffer end
  - the ima_field_data array must be large enough to contain lengths and
    data pointers for the amount of data requested (output: number
    of fields written)
- ENFORCE_FIELDS | ENFORCE_BUFEND: matching of both

Use cases

- measurement entry header: ENFORCE_FIELDS | ENFORCE_BUFEND
  - four fields must be parsed: pcr, digest, template name, template data
  - ENFORCE_BUFEND is enforced only for the last measurement entry
- template digest (Crypto Agile): ENFORCE_BUFEND
  - since only the total template digest length is known, the function
    parses length-data combinations until the buffer end is reached
- template data: ENFORCE_FIELDS | ENFORCE_BUFEND
  - since the number of fields and the total template data length
    are known, the function can perform both checks

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 security/integrity/ima/ima_template_lib.c | 61 +++++++++++++++++++++++++++++++
 security/integrity/ima/ima_template_lib.h |  6 +++
 2 files changed, 67 insertions(+)

diff --git a/security/integrity/ima/ima_template_lib.c b/security/integrity/ima/ima_template_lib.c
index f9ba37b..28af43f 100644
--- a/security/integrity/ima/ima_template_lib.c
+++ b/security/integrity/ima/ima_template_lib.c
@@ -159,6 +159,67 @@ 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);
 }
 
+/**
+ * ima_parse_buf() - Parses lengths and data from an input buffer
+ * @bufstartp:       Buffer start address.
+ * @bufendp:         Buffer end address.
+ * @bufcurp:         Pointer to remaining (non-parsed) data.
+ * @maxfields:       Length of fields array.
+ * @fields:          Array containing lengths and pointers of parsed data.
+ * @curfields:       Number of array items containing parsed data.
+ * @len_mask:        Bitmap (if bit is set, data length should not be parsed).
+ * @enforce_mask:    Check if curfields == maxfields and/or bufcurp == bufendp.
+ * @bufname:         String identifier of the input buffer.
+ *
+ * Return: 0 on success, -EINVAL on error.
+ */
+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)
+{
+	void *bufp = bufstartp;
+	int i;
+
+	for (i = 0; i < maxfields; i++) {
+		if (len_mask == NULL || !test_bit(i, len_mask)) {
+			if (bufp > (bufendp - sizeof(u32)))
+				break;
+
+			fields[i].len = *(u32 *)bufp;
+			if (ima_canonical_fmt)
+				fields[i].len = le32_to_cpu(fields[i].len);
+
+			bufp += sizeof(u32);
+		}
+
+		if (bufp > (bufendp - fields[i].len))
+			break;
+
+		fields[i].data = bufp;
+		bufp += fields[i].len;
+	}
+
+	if ((enforce_mask & ENFORCE_FIELDS) && i != maxfields) {
+		pr_err("%s: nr of fields mismatch: expected: %d, current: %d\n",
+		       bufname, maxfields, i);
+		return -EINVAL;
+	}
+
+	if ((enforce_mask & ENFORCE_BUFEND) && bufp != bufendp) {
+		pr_err("%s: buf end mismatch: expected: %p, current: %p\n",
+		       bufname, bufendp, bufp);
+		return -EINVAL;
+	}
+
+	if (curfields)
+		*curfields = i;
+
+	if (bufcurp)
+		*bufcurp = bufp;
+
+	return 0;
+}
+
 static int ima_eventdigest_init_common(u8 *digest, u32 digestsize, u8 hash_algo,
 				       struct ima_field_data *field_data)
 {
diff --git a/security/integrity/ima/ima_template_lib.h b/security/integrity/ima/ima_template_lib.h
index c344530..6a3d8b8 100644
--- a/security/integrity/ima/ima_template_lib.h
+++ b/security/integrity/ima/ima_template_lib.h
@@ -18,6 +18,9 @@
 #include <linux/seq_file.h>
 #include "ima.h"
 
+#define ENFORCE_FIELDS 0x00000001
+#define ENFORCE_BUFEND 0x00000002
+
 void ima_show_template_digest(struct seq_file *m, enum ima_show_type show,
 			      struct ima_field_data *field_data);
 void ima_show_template_digest_ng(struct seq_file *m, enum ima_show_type show,
@@ -26,6 +29,9 @@ 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);
+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);
 int ima_eventdigest_init(struct ima_event_data *event_data,
 			 struct ima_field_data *field_data);
 int ima_eventname_init(struct ima_event_data *event_data,
-- 
2.9.3

[PATCH 1/7] ima: introduce ima_parse_buf()

[Roberto Sassu]

ima_parse_buf() takes as input the buffer start and end pointers, and
stores the result in a static array of ima_field_data structures,
where the len field contains the length parsed from the buffer, and
the data field contains the address of the buffer just after the length.
Optionally, the function returns the current value of the buffer pointer
and the number of array elements written.

A bitmap has been added as parameter of ima_parse_buf() to handle
the cases where the length is not prepended to data. Each bit corresponds
to an element of the ima_field_data array. If a bit is set, the length
is not parsed from the buffer, but is read from the corresponding element
of the array (the length must be set before calling the function).

ima_parse_buf() can perform three checks upon request by callers,
depending on the enforce mask passed to it:

- ENFORCE_FIELDS: matching of number of fields (length-data combination)
  - there must be enough data in the buffer to parse the number of fields
    requested (output: current value of buffer pointer)
- ENFORCE_BUFEND: matching of buffer end
  - the ima_field_data array must be large enough to contain lengths and
    data pointers for the amount of data requested (output: number
    of fields written)
- ENFORCE_FIELDS | ENFORCE_BUFEND: matching of both

Use cases

- measurement entry header: ENFORCE_FIELDS | ENFORCE_BUFEND
  - four fields must be parsed: pcr, digest, template name, template data
  - ENFORCE_BUFEND is enforced only for the last measurement entry
- template digest (Crypto Agile): ENFORCE_BUFEND
  - since only the total template digest length is known, the function
    parses length-data combinations until the buffer end is reached
- template data: ENFORCE_FIELDS | ENFORCE_BUFEND
  - since the number of fields and the total template data length
    are known, the function can perform both checks

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 security/integrity/ima/ima_template_lib.c | 61 +++++++++++++++++++++++++++++++
 security/integrity/ima/ima_template_lib.h |  6 +++
 2 files changed, 67 insertions(+)

diff --git a/security/integrity/ima/ima_template_lib.c b/security/integrity/ima/ima_template_lib.c
index f9ba37b..28af43f 100644
--- a/security/integrity/ima/ima_template_lib.c
+++ b/security/integrity/ima/ima_template_lib.c
@@ -159,6 +159,67 @@ 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);
 }
 
+/**
+ * ima_parse_buf() - Parses lengths and data from an input buffer
+ * @bufstartp:       Buffer start address.
+ * @bufendp:         Buffer end address.
+ * @bufcurp:         Pointer to remaining (non-parsed) data.
+ * @maxfields:       Length of fields array.
+ * @fields:          Array containing lengths and pointers of parsed data.
+ * @curfields:       Number of array items containing parsed data.
+ * @len_mask:        Bitmap (if bit is set, data length should not be parsed).
+ * @enforce_mask:    Check if curfields == maxfields and/or bufcurp == bufendp.
+ * @bufname:         String identifier of the input buffer.
+ *
+ * Return: 0 on success, -EINVAL on error.
+ */
+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)
+{
+	void *bufp = bufstartp;
+	int i;
+
+	for (i = 0; i < maxfields; i++) {
+		if (len_mask == NULL || !test_bit(i, len_mask)) {
+			if (bufp > (bufendp - sizeof(u32)))
+				break;
+
+			fields[i].len = *(u32 *)bufp;
+			if (ima_canonical_fmt)
+				fields[i].len = le32_to_cpu(fields[i].len);
+
+			bufp += sizeof(u32);
+		}
+
+		if (bufp > (bufendp - fields[i].len))
+			break;
+
+		fields[i].data = bufp;
+		bufp += fields[i].len;
+	}
+
+	if ((enforce_mask & ENFORCE_FIELDS) && i != maxfields) {
+		pr_err("%s: nr of fields mismatch: expected: %d, current: %d\n",
+		       bufname, maxfields, i);
+		return -EINVAL;
+	}
+
+	if ((enforce_mask & ENFORCE_BUFEND) && bufp != bufendp) {
+		pr_err("%s: buf end mismatch: expected: %p, current: %p\n",
+		       bufname, bufendp, bufp);
+		return -EINVAL;
+	}
+
+	if (curfields)
+		*curfields = i;
+
+	if (bufcurp)
+		*bufcurp = bufp;
+
+	return 0;
+}
+
 static int ima_eventdigest_init_common(u8 *digest, u32 digestsize, u8 hash_algo,
 				       struct ima_field_data *field_data)
 {
diff --git a/security/integrity/ima/ima_template_lib.h b/security/integrity/ima/ima_template_lib.h
index c344530..6a3d8b8 100644
--- a/security/integrity/ima/ima_template_lib.h
+++ b/security/integrity/ima/ima_template_lib.h
@@ -18,6 +18,9 @@
 #include <linux/seq_file.h>
 #include "ima.h"
 
+#define ENFORCE_FIELDS 0x00000001
+#define ENFORCE_BUFEND 0x00000002
+
 void ima_show_template_digest(struct seq_file *m, enum ima_show_type show,
 			      struct ima_field_data *field_data);
 void ima_show_template_digest_ng(struct seq_file *m, enum ima_show_type show,
@@ -26,6 +29,9 @@ 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);
+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);
 int ima_eventdigest_init(struct ima_event_data *event_data,
 			 struct ima_field_data *field_data);
 int ima_eventname_init(struct ima_event_data *event_data,
-- 
2.9.3

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/7] ima: use ima_parse_buf() to parse measurements headers

[Roberto Sassu]

The binary_hdr_v1 and binary_data_v1 structures defined in
ima_restore_measurement_list() have been replaced with an array of four
ima_field_data structures where pcr, digest, template name and
template data lengths and pointers are stored.

The length of pcr and digest in the ima_field_data array and the bits
in the bitmap are set before ima_parse_buf() is called. The ENFORCE_FIELDS
bit is set for all entries except the last one (there is still data to
parse), and ENFORCE_BUFEND is set only for the last entry.

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 security/integrity/ima/ima_template.c | 80 ++++++++++++-----------------------
 1 file changed, 28 insertions(+), 52 deletions(-)

diff --git a/security/integrity/ima/ima_template.c b/security/integrity/ima/ima_template.c
index cebb37c..624e2a1 100644
--- a/security/integrity/ima/ima_template.c
+++ b/security/integrity/ima/ima_template.c
@@ -19,6 +19,9 @@
 #include "ima.h"
 #include "ima_template_lib.h"
 
+enum header_fields { HDR_PCR, HDR_DIGEST, HDR_TEMPLATE_NAME,
+		     HDR_TEMPLATE_DATA, HDR__LAST };
+
 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"},
@@ -337,27 +340,19 @@ static int ima_restore_template_data(struct ima_template_desc *template_desc,
 /* 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;
+	struct ima_field_data hdr[HDR__LAST] = {
+		[HDR_PCR] = {.len = sizeof(u32)},
+		[HDR_DIGEST] = {.len = TPM_DIGEST_SIZE},
+	};
 
 	void *bufp = buf + sizeof(*khdr);
 	void *bufendp;
 	struct ima_template_entry *entry;
 	struct ima_template_desc *template_desc;
+	DECLARE_BITMAP(hdr_mask, HDR__LAST);
 	unsigned long count = 0;
 	int ret = 0;
 
@@ -380,6 +375,10 @@ int ima_restore_measurement_list(loff_t size, void *buf)
 		return -EINVAL;
 	}
 
+	bitmap_zero(hdr_mask, HDR__LAST);
+	bitmap_set(hdr_mask, HDR_PCR, 1);
+	bitmap_set(hdr_mask, HDR_DIGEST, 1);
+
 	/*
 	 * ima kexec buffer prefix: version, buffer size, count
 	 * v1 format: pcr, digest, template-name-len, template-name,
@@ -387,31 +386,25 @@ int ima_restore_measurement_list(loff_t size, void *buf)
 	 */
 	bufendp = buf + khdr->buffer_size;
 	while ((bufp < bufendp) && (count++ < khdr->count)) {
-		hdr_v1 = bufp;
-		if (bufp > (bufendp - sizeof(*hdr_v1))) {
-			pr_err("attempting to restore partial measurement\n");
-			ret = -EINVAL;
-			break;
-		}
-		bufp += sizeof(*hdr_v1);
+		int enforce_mask = ENFORCE_FIELDS;
 
-		if (ima_canonical_fmt)
-			hdr_v1->template_name_len =
-			    le32_to_cpu(hdr_v1->template_name_len);
+		enforce_mask |= (count == khdr->count) ? ENFORCE_BUFEND : 0;
+		ret = ima_parse_buf(bufp, bufendp, &bufp, HDR__LAST, hdr, NULL,
+				    hdr_mask, enforce_mask, "entry header");
+		if (ret < 0)
+			break;
 
-		if ((hdr_v1->template_name_len >= MAX_TEMPLATE_NAME_LEN) ||
-		    (bufp > (bufendp - hdr_v1->template_name_len))) {
+		if (hdr[HDR_TEMPLATE_NAME].len >= MAX_TEMPLATE_NAME_LEN) {
 			pr_err("attempting to restore a template name \
 				that is too long\n");
 			ret = -EINVAL;
 			break;
 		}
-		data_v1 = bufp += (u_int8_t)hdr_v1->template_name_len;
 
 		/* template name is not null terminated */
-		memcpy(template_name, hdr_v1->template_name,
-		       hdr_v1->template_name_len);
-		template_name[hdr_v1->template_name_len] = 0;
+		memcpy(template_name, hdr[HDR_TEMPLATE_NAME].data,
+		       hdr[HDR_TEMPLATE_NAME].len);
+		template_name[hdr[HDR_TEMPLATE_NAME].len] = 0;
 
 		if (strcmp(template_name, "ima") == 0) {
 			pr_err("attempting to restore an unsupported \
@@ -441,34 +434,17 @@ int ima_restore_measurement_list(loff_t size, void *buf)
 			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 (ima_canonical_fmt)
-			data_v1->template_data_size =
-			    le32_to_cpu(data_v1->template_data_size);
-
-		if (bufp > (bufendp - data_v1->template_data_size)) {
-			pr_err("restoring the template data failed\n");
-			ret = -EINVAL;
-			break;
-		}
-		bufp += data_v1->template_data_size;
-
 		ret = ima_restore_template_data(template_desc,
-						data_v1->template_data,
-						data_v1->template_data_size,
+						hdr[HDR_TEMPLATE_DATA].data,
+						hdr[HDR_TEMPLATE_DATA].len,
 						&entry);
 		if (ret < 0)
 			break;
 
-		memcpy(entry->digest, hdr_v1->digest, TPM_DIGEST_SIZE);
-		entry->pcr =
-		    !ima_canonical_fmt ? hdr_v1->pcr : le32_to_cpu(hdr_v1->pcr);
+		memcpy(entry->digest, hdr[HDR_DIGEST].data,
+		       hdr[HDR_DIGEST].len);
+		entry->pcr = !ima_canonical_fmt ? *(hdr[HDR_PCR].data) :
+			     le32_to_cpu(*(hdr[HDR_PCR].data));
 		ret = ima_restore_measurement_entry(entry);
 		if (ret < 0)
 			break;
-- 
2.9.3

[PATCH 2/7] ima: use ima_parse_buf() to parse measurements headers

[Roberto Sassu]

The binary_hdr_v1 and binary_data_v1 structures defined in
ima_restore_measurement_list() have been replaced with an array of four
ima_field_data structures where pcr, digest, template name and
template data lengths and pointers are stored.

The length of pcr and digest in the ima_field_data array and the bits
in the bitmap are set before ima_parse_buf() is called. The ENFORCE_FIELDS
bit is set for all entries except the last one (there is still data to
parse), and ENFORCE_BUFEND is set only for the last entry.

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 security/integrity/ima/ima_template.c | 80 ++++++++++++-----------------------
 1 file changed, 28 insertions(+), 52 deletions(-)

diff --git a/security/integrity/ima/ima_template.c b/security/integrity/ima/ima_template.c
index cebb37c..624e2a1 100644
--- a/security/integrity/ima/ima_template.c
+++ b/security/integrity/ima/ima_template.c
@@ -19,6 +19,9 @@
 #include "ima.h"
 #include "ima_template_lib.h"
 
+enum header_fields { HDR_PCR, HDR_DIGEST, HDR_TEMPLATE_NAME,
+		     HDR_TEMPLATE_DATA, HDR__LAST };
+
 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"},
@@ -337,27 +340,19 @@ static int ima_restore_template_data(struct ima_template_desc *template_desc,
 /* 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;
+	struct ima_field_data hdr[HDR__LAST] = {
+		[HDR_PCR] = {.len = sizeof(u32)},
+		[HDR_DIGEST] = {.len = TPM_DIGEST_SIZE},
+	};
 
 	void *bufp = buf + sizeof(*khdr);
 	void *bufendp;
 	struct ima_template_entry *entry;
 	struct ima_template_desc *template_desc;
+	DECLARE_BITMAP(hdr_mask, HDR__LAST);
 	unsigned long count = 0;
 	int ret = 0;
 
@@ -380,6 +375,10 @@ int ima_restore_measurement_list(loff_t size, void *buf)
 		return -EINVAL;
 	}
 
+	bitmap_zero(hdr_mask, HDR__LAST);
+	bitmap_set(hdr_mask, HDR_PCR, 1);
+	bitmap_set(hdr_mask, HDR_DIGEST, 1);
+
 	/*
 	 * ima kexec buffer prefix: version, buffer size, count
 	 * v1 format: pcr, digest, template-name-len, template-name,
@@ -387,31 +386,25 @@ int ima_restore_measurement_list(loff_t size, void *buf)
 	 */
 	bufendp = buf + khdr->buffer_size;
 	while ((bufp < bufendp) && (count++ < khdr->count)) {
-		hdr_v1 = bufp;
-		if (bufp > (bufendp - sizeof(*hdr_v1))) {
-			pr_err("attempting to restore partial measurement\n");
-			ret = -EINVAL;
-			break;
-		}
-		bufp += sizeof(*hdr_v1);
+		int enforce_mask = ENFORCE_FIELDS;
 
-		if (ima_canonical_fmt)
-			hdr_v1->template_name_len =
-			    le32_to_cpu(hdr_v1->template_name_len);
+		enforce_mask |= (count == khdr->count) ? ENFORCE_BUFEND : 0;
+		ret = ima_parse_buf(bufp, bufendp, &bufp, HDR__LAST, hdr, NULL,
+				    hdr_mask, enforce_mask, "entry header");
+		if (ret < 0)
+			break;
 
-		if ((hdr_v1->template_name_len >= MAX_TEMPLATE_NAME_LEN) ||
-		    (bufp > (bufendp - hdr_v1->template_name_len))) {
+		if (hdr[HDR_TEMPLATE_NAME].len >= MAX_TEMPLATE_NAME_LEN) {
 			pr_err("attempting to restore a template name \
 				that is too long\n");
 			ret = -EINVAL;
 			break;
 		}
-		data_v1 = bufp += (u_int8_t)hdr_v1->template_name_len;
 
 		/* template name is not null terminated */
-		memcpy(template_name, hdr_v1->template_name,
-		       hdr_v1->template_name_len);
-		template_name[hdr_v1->template_name_len] = 0;
+		memcpy(template_name, hdr[HDR_TEMPLATE_NAME].data,
+		       hdr[HDR_TEMPLATE_NAME].len);
+		template_name[hdr[HDR_TEMPLATE_NAME].len] = 0;
 
 		if (strcmp(template_name, "ima") == 0) {
 			pr_err("attempting to restore an unsupported \
@@ -441,34 +434,17 @@ int ima_restore_measurement_list(loff_t size, void *buf)
 			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 (ima_canonical_fmt)
-			data_v1->template_data_size =
-			    le32_to_cpu(data_v1->template_data_size);
-
-		if (bufp > (bufendp - data_v1->template_data_size)) {
-			pr_err("restoring the template data failed\n");
-			ret = -EINVAL;
-			break;
-		}
-		bufp += data_v1->template_data_size;
-
 		ret = ima_restore_template_data(template_desc,
-						data_v1->template_data,
-						data_v1->template_data_size,
+						hdr[HDR_TEMPLATE_DATA].data,
+						hdr[HDR_TEMPLATE_DATA].len,
 						&entry);
 		if (ret < 0)
 			break;
 
-		memcpy(entry->digest, hdr_v1->digest, TPM_DIGEST_SIZE);
-		entry->pcr =
-		    !ima_canonical_fmt ? hdr_v1->pcr : le32_to_cpu(hdr_v1->pcr);
+		memcpy(entry->digest, hdr[HDR_DIGEST].data,
+		       hdr[HDR_DIGEST].len);
+		entry->pcr = !ima_canonical_fmt ? *(hdr[HDR_PCR].data) :
+			     le32_to_cpu(*(hdr[HDR_PCR].data));
 		ret = ima_restore_measurement_entry(entry);
 		if (ret < 0)
 			break;
-- 
2.9.3

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info@ http://vger.kernel.org/majordomo-info.html

[PATCH 3/7] ima: use ima_parse_buf() to parse template data

[Roberto Sassu]

The binary_field_data structure definition has been removed from
ima_restore_template_data(). The lengths and data pointers are directly
stored into the template_data array of the ima_template_entry structure.
For template data, both the number of fields and buffer end checks can
be done, as these information are known (respectively from the template
descriptor, and from the measurement header field).

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 security/integrity/ima/ima_template.c | 44 +++++++++++------------------------
 1 file changed, 13 insertions(+), 31 deletions(-)

diff --git a/security/integrity/ima/ima_template.c b/security/integrity/ima/ima_template.c
index 624e2a1..7412d02 100644
--- a/security/integrity/ima/ima_template.c
+++ b/security/integrity/ima/ima_template.c
@@ -277,13 +277,6 @@ static int ima_restore_template_data(struct ima_template_desc *template_desc,
 				     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;
 
@@ -293,30 +286,19 @@ static int ima_restore_template_data(struct ima_template_desc *template_desc,
 	if (!*entry)
 		return -ENOMEM;
 
+	ret = ima_parse_buf(template_data, template_data + template_data_size,
+			    NULL, template_desc->num_fields,
+			    (*entry)->template_data, NULL, NULL,
+			    ENFORCE_FIELDS | ENFORCE_BUFEND, "template data");
+	if (ret < 0) {
+		kfree(*entry);
+		return ret;
+	}
+
 	(*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))) {
-			pr_err("Restoring the template field failed\n");
-			ret = -EINVAL;
-			break;
-		}
-		offset += sizeof(*field_data);
-
-		if (ima_canonical_fmt)
-			field_data->len = le32_to_cpu(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);
+		struct ima_field_data *field_data = &(*entry)->template_data[i];
+		u8 *data = field_data->data;
 
 		(*entry)->template_data[i].data =
 			kzalloc(field_data->len + 1, GFP_KERNEL);
@@ -324,8 +306,8 @@ static int ima_restore_template_data(struct ima_template_desc *template_desc,
 			ret = -ENOMEM;
 			break;
 		}
-		memcpy((*entry)->template_data[i].data, field_data->data,
-			field_data->len);
+		memcpy((*entry)->template_data[i].data, data, field_data->len);
+		(*entry)->template_data_len += sizeof(field_data->len);
 		(*entry)->template_data_len += field_data->len;
 	}
 
-- 
2.9.3

[PATCH 3/7] ima: use ima_parse_buf() to parse template data

[Roberto Sassu]

The binary_field_data structure definition has been removed from
ima_restore_template_data(). The lengths and data pointers are directly
stored into the template_data array of the ima_template_entry structure.
For template data, both the number of fields and buffer end checks can
be done, as these information are known (respectively from the template
descriptor, and from the measurement header field).

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 security/integrity/ima/ima_template.c | 44 +++++++++++------------------------
 1 file changed, 13 insertions(+), 31 deletions(-)

diff --git a/security/integrity/ima/ima_template.c b/security/integrity/ima/ima_template.c
index 624e2a1..7412d02 100644
--- a/security/integrity/ima/ima_template.c
+++ b/security/integrity/ima/ima_template.c
@@ -277,13 +277,6 @@ static int ima_restore_template_data(struct ima_template_desc *template_desc,
 				     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;
 
@@ -293,30 +286,19 @@ static int ima_restore_template_data(struct ima_template_desc *template_desc,
 	if (!*entry)
 		return -ENOMEM;
 
+	ret = ima_parse_buf(template_data, template_data + template_data_size,
+			    NULL, template_desc->num_fields,
+			    (*entry)->template_data, NULL, NULL,
+			    ENFORCE_FIELDS | ENFORCE_BUFEND, "template data");
+	if (ret < 0) {
+		kfree(*entry);
+		return ret;
+	}
+
 	(*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))) {
-			pr_err("Restoring the template field failed\n");
-			ret = -EINVAL;
-			break;
-		}
-		offset += sizeof(*field_data);
-
-		if (ima_canonical_fmt)
-			field_data->len = le32_to_cpu(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);
+		struct ima_field_data *field_data = &(*entry)->template_data[i];
+		u8 *data = field_data->data;
 
 		(*entry)->template_data[i].data =
 			kzalloc(field_data->len + 1, GFP_KERNEL);
@@ -324,8 +306,8 @@ static int ima_restore_template_data(struct ima_template_desc *template_desc,
 			ret = -ENOMEM;
 			break;
 		}
-		memcpy((*entry)->template_data[i].data, field_data->data,
-			field_data->len);
+		memcpy((*entry)->template_data[i].data, data, field_data->len);
+		(*entry)->template_data_len += sizeof(field_data->len);
 		(*entry)->template_data_len += field_data->len;
 	}
 
-- 
2.9.3

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4/7] ima: declare get_binary_runtime_size() as non-static

[Roberto Sassu]

The function get_binary_runtime_size(), renamed to
ima_get_template_entry_size(), is now declared as non-static, so that
it can be used by callers outside ima_queue.c to calculate the size
of a given measurement entry.

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 security/integrity/ima/ima.h       | 1 +
 security/integrity/ima/ima_queue.c | 4 ++--
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index b563fbd..10ef9c8 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -131,6 +131,7 @@ extern bool ima_canonical_fmt;
 /* Internal IMA function definitions */
 int ima_init(void);
 int ima_fs_init(void);
+int ima_get_template_entry_size(struct ima_template_entry *entry);
 int ima_add_template_entry(struct ima_template_entry *entry, int violation,
 			   const char *op, struct inode *inode,
 			   const unsigned char *filename);
diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c
index f628968..24984a2 100644
--- a/security/integrity/ima/ima_queue.c
+++ b/security/integrity/ima/ima_queue.c
@@ -74,7 +74,7 @@ static struct ima_queue_entry *ima_lookup_digest_entry(u8 *digest_value,
  * 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 ima_get_template_entry_size(struct ima_template_entry *entry)
 {
 	int size = 0;
 
@@ -118,7 +118,7 @@ static int ima_add_digest_entry(struct ima_template_entry *entry,
 	if (binary_runtime_size != ULONG_MAX) {
 		int size;
 
-		size = get_binary_runtime_size(entry);
+		size = ima_get_template_entry_size(entry);
 		binary_runtime_size = (binary_runtime_size < ULONG_MAX - size) ?
 		     binary_runtime_size + size : ULONG_MAX;
 	}
-- 
2.9.3

[PATCH 4/7] ima: declare get_binary_runtime_size() as non-static

[Roberto Sassu]

The function get_binary_runtime_size(), renamed to
ima_get_template_entry_size(), is now declared as non-static, so that
it can be used by callers outside ima_queue.c to calculate the size
of a given measurement entry.

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 security/integrity/ima/ima.h       | 1 +
 security/integrity/ima/ima_queue.c | 4 ++--
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index b563fbd..10ef9c8 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -131,6 +131,7 @@ extern bool ima_canonical_fmt;
 /* Internal IMA function definitions */
 int ima_init(void);
 int ima_fs_init(void);
+int ima_get_template_entry_size(struct ima_template_entry *entry);
 int ima_add_template_entry(struct ima_template_entry *entry, int violation,
 			   const char *op, struct inode *inode,
 			   const unsigned char *filename);
diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c
index f628968..24984a2 100644
--- a/security/integrity/ima/ima_queue.c
+++ b/security/integrity/ima/ima_queue.c
@@ -74,7 +74,7 @@ static struct ima_queue_entry *ima_lookup_digest_entry(u8 *digest_value,
  * 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 ima_get_template_entry_size(struct ima_template_entry *entry)
 {
 	int size = 0;
 
@@ -118,7 +118,7 @@ static int ima_add_digest_entry(struct ima_template_entry *entry,
 	if (binary_runtime_size != ULONG_MAX) {
 		int size;
 
-		size = get_binary_runtime_size(entry);
+		size = ima_get_template_entry_size(entry);
 		binary_runtime_size = (binary_runtime_size < ULONG_MAX - size) ?
 		     binary_runtime_size + size : ULONG_MAX;
 	}
-- 
2.9.3

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info@ http://vger.kernel.org/majordomo-info.html

[PATCH 5/7] ima: add securityfs interface to save a measurements list with kexec header

[Roberto Sassu]

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(-)

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);
-- 
2.9.3

[PATCH 5/7] ima: add securityfs interface to save a measurements list with kexec header

[Roberto Sassu]

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(-)

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);
-- 
2.9.3

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 6/7] ima: add securityfs interface to restore a measurements list

[Roberto Sassu]

Through the new interface restore_kexec_list, it will be possible
to restore a measurements list, previously read from
binary_kexec_runtime_measurements.

The patch reuses the policy functions to create a buffer, read
the measurements and call ima_restore_measurement_list().

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 security/integrity/ima/ima_fs.c | 38 ++++++++++++++++++++++++++++++++++----
 1 file changed, 34 insertions(+), 4 deletions(-)

diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
index a93f941..6e3f93f 100644
--- a/security/integrity/ima/ima_fs.c
+++ b/security/integrity/ima/ima_fs.c
@@ -77,6 +77,7 @@ static const struct file_operations ima_measurements_count_ops = {
 };
 
 static struct dentry *binary_kexec_runtime_measurements;
+static struct dentry *restore_kexec_list;
 
 /* returns pointer to hlist_node */
 static void *ima_measurements_start(struct seq_file *m, loff_t *pos)
@@ -297,7 +298,7 @@ static const struct file_operations ima_ascii_measurements_ops = {
 	.release = seq_release,
 };
 
-static ssize_t ima_read_policy(char *path)
+static ssize_t ima_read_policy(char *path, bool khdr)
 {
 	void *data;
 	char *datap;
@@ -317,6 +318,13 @@ static ssize_t ima_read_policy(char *path)
 	}
 
 	datap = data;
+
+	if (khdr) {
+		rc = ima_restore_measurement_list(size, data);
+		size = 0;
+		goto out;
+	}
+
 	while (size > 0 && (p = strsep(&datap, "\n"))) {
 		pr_debug("rule: %s\n", p);
 		rc = ima_parse_add_rule(p);
@@ -325,6 +333,7 @@ static ssize_t ima_read_policy(char *path)
 		size -= rc;
 	}
 
+out:
 	vfree(data);
 	if (rc < 0)
 		return rc;
@@ -337,6 +346,7 @@ static ssize_t ima_read_policy(char *path)
 static ssize_t ima_write_policy(struct file *file, const char __user *buf,
 				size_t datalen, loff_t *ppos)
 {
+	bool khdr = file->f_path.dentry == restore_kexec_list;
 	char *data;
 	ssize_t result;
 
@@ -363,8 +373,13 @@ static ssize_t ima_write_policy(struct file *file, const char __user *buf,
 	if (result < 0)
 		goto out_free;
 
+	if (khdr && data[0] != '/') {
+		mutex_unlock(&ima_write_mutex);
+		goto out_free;
+	}
+
 	if (data[0] == '/') {
-		result = ima_read_policy(data);
+		result = ima_read_policy(data, khdr);
 	} else if (ima_appraise & IMA_APPRAISE_POLICY) {
 		pr_err("IMA: signed policy file (specified as an absolute pathname) required\n");
 		integrity_audit_msg(AUDIT_INTEGRITY_STATUS, NULL, NULL,
@@ -393,7 +408,7 @@ static struct dentry *violations;
 static struct dentry *ima_policy;
 
 enum ima_fs_flags {
-	IMA_FS_BUSY,
+	IMA_FS_BUSY, IMA_RESTORE_LIST_BUSY,
 };
 
 static unsigned long ima_fs_flags;
@@ -412,6 +427,9 @@ static const struct seq_operations ima_policy_seqops = {
  */
 static int ima_open_policy(struct inode *inode, struct file *filp)
 {
+	bool khdr = filp->f_path.dentry == restore_kexec_list;
+	unsigned long bit = khdr ? IMA_RESTORE_LIST_BUSY : IMA_FS_BUSY;
+
 	if (!(filp->f_flags & O_WRONLY)) {
 #ifndef	CONFIG_IMA_READ_POLICY
 		return -EACCES;
@@ -423,7 +441,7 @@ static int ima_open_policy(struct inode *inode, struct file *filp)
 		return seq_open(filp, &ima_policy_seqops);
 #endif
 	}
-	if (test_and_set_bit(IMA_FS_BUSY, &ima_fs_flags))
+	if (test_and_set_bit(bit, &ima_fs_flags))
 		return -EBUSY;
 	return 0;
 }
@@ -439,6 +457,11 @@ static int ima_release_policy(struct inode *inode, struct file *file)
 {
 	const char *cause = valid_policy ? "completed" : "failed";
 
+	if (file->f_path.dentry == restore_kexec_list) {
+		clear_bit(IMA_RESTORE_LIST_BUSY, &ima_fs_flags);
+		return 0;
+	}
+
 	if ((file->f_flags & O_ACCMODE) == O_RDONLY)
 		return seq_release(inode, file);
 
@@ -522,9 +545,16 @@ int __init ima_fs_init(void)
 				   &ima_measurements_ops);
 	if (IS_ERR(binary_kexec_runtime_measurements))
 		goto out;
+
+	restore_kexec_list = securityfs_create_file("restore_kexec_list",
+						    S_IWUSR, ima_dir, NULL,
+						    &ima_measure_policy_ops);
+	if (IS_ERR(restore_kexec_list))
+		goto out;
 #endif
 	return 0;
 out:
+	securityfs_remove(restore_kexec_list);
 	securityfs_remove(binary_kexec_runtime_measurements);
 	securityfs_remove(violations);
 	securityfs_remove(runtime_measurements_count);
-- 
2.9.3

[PATCH 6/7] ima: add securityfs interface to restore a measurements list

[Roberto Sassu]

Through the new interface restore_kexec_list, it will be possible
to restore a measurements list, previously read from
binary_kexec_runtime_measurements.

The patch reuses the policy functions to create a buffer, read
the measurements and call ima_restore_measurement_list().

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 security/integrity/ima/ima_fs.c | 38 ++++++++++++++++++++++++++++++++++----
 1 file changed, 34 insertions(+), 4 deletions(-)

diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
index a93f941..6e3f93f 100644
--- a/security/integrity/ima/ima_fs.c
+++ b/security/integrity/ima/ima_fs.c
@@ -77,6 +77,7 @@ static const struct file_operations ima_measurements_count_ops = {
 };
 
 static struct dentry *binary_kexec_runtime_measurements;
+static struct dentry *restore_kexec_list;
 
 /* returns pointer to hlist_node */
 static void *ima_measurements_start(struct seq_file *m, loff_t *pos)
@@ -297,7 +298,7 @@ static const struct file_operations ima_ascii_measurements_ops = {
 	.release = seq_release,
 };
 
-static ssize_t ima_read_policy(char *path)
+static ssize_t ima_read_policy(char *path, bool khdr)
 {
 	void *data;
 	char *datap;
@@ -317,6 +318,13 @@ static ssize_t ima_read_policy(char *path)
 	}
 
 	datap = data;
+
+	if (khdr) {
+		rc = ima_restore_measurement_list(size, data);
+		size = 0;
+		goto out;
+	}
+
 	while (size > 0 && (p = strsep(&datap, "\n"))) {
 		pr_debug("rule: %s\n", p);
 		rc = ima_parse_add_rule(p);
@@ -325,6 +333,7 @@ static ssize_t ima_read_policy(char *path)
 		size -= rc;
 	}
 
+out:
 	vfree(data);
 	if (rc < 0)
 		return rc;
@@ -337,6 +346,7 @@ static ssize_t ima_read_policy(char *path)
 static ssize_t ima_write_policy(struct file *file, const char __user *buf,
 				size_t datalen, loff_t *ppos)
 {
+	bool khdr = file->f_path.dentry == restore_kexec_list;
 	char *data;
 	ssize_t result;
 
@@ -363,8 +373,13 @@ static ssize_t ima_write_policy(struct file *file, const char __user *buf,
 	if (result < 0)
 		goto out_free;
 
+	if (khdr && data[0] != '/') {
+		mutex_unlock(&ima_write_mutex);
+		goto out_free;
+	}
+
 	if (data[0] == '/') {
-		result = ima_read_policy(data);
+		result = ima_read_policy(data, khdr);
 	} else if (ima_appraise & IMA_APPRAISE_POLICY) {
 		pr_err("IMA: signed policy file (specified as an absolute pathname) required\n");
 		integrity_audit_msg(AUDIT_INTEGRITY_STATUS, NULL, NULL,
@@ -393,7 +408,7 @@ static struct dentry *violations;
 static struct dentry *ima_policy;
 
 enum ima_fs_flags {
-	IMA_FS_BUSY,
+	IMA_FS_BUSY, IMA_RESTORE_LIST_BUSY,
 };
 
 static unsigned long ima_fs_flags;
@@ -412,6 +427,9 @@ static const struct seq_operations ima_policy_seqops = {
  */
 static int ima_open_policy(struct inode *inode, struct file *filp)
 {
+	bool khdr = filp->f_path.dentry == restore_kexec_list;
+	unsigned long bit = khdr ? IMA_RESTORE_LIST_BUSY : IMA_FS_BUSY;
+
 	if (!(filp->f_flags & O_WRONLY)) {
 #ifndef	CONFIG_IMA_READ_POLICY
 		return -EACCES;
@@ -423,7 +441,7 @@ static int ima_open_policy(struct inode *inode, struct file *filp)
 		return seq_open(filp, &ima_policy_seqops);
 #endif
 	}
-	if (test_and_set_bit(IMA_FS_BUSY, &ima_fs_flags))
+	if (test_and_set_bit(bit, &ima_fs_flags))
 		return -EBUSY;
 	return 0;
 }
@@ -439,6 +457,11 @@ static int ima_release_policy(struct inode *inode, struct file *file)
 {
 	const char *cause = valid_policy ? "completed" : "failed";
 
+	if (file->f_path.dentry == restore_kexec_list) {
+		clear_bit(IMA_RESTORE_LIST_BUSY, &ima_fs_flags);
+		return 0;
+	}
+
 	if ((file->f_flags & O_ACCMODE) == O_RDONLY)
 		return seq_release(inode, file);
 
@@ -522,9 +545,16 @@ int __init ima_fs_init(void)
 				   &ima_measurements_ops);
 	if (IS_ERR(binary_kexec_runtime_measurements))
 		goto out;
+
+	restore_kexec_list = securityfs_create_file("restore_kexec_list",
+						    S_IWUSR, ima_dir, NULL,
+						    &ima_measure_policy_ops);
+	if (IS_ERR(restore_kexec_list))
+		goto out;
 #endif
 	return 0;
 out:
+	securityfs_remove(restore_kexec_list);
 	securityfs_remove(binary_kexec_runtime_measurements);
 	securityfs_remove(violations);
 	securityfs_remove(runtime_measurements_count);
-- 
2.9.3

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 7/7] ima: fix get_binary_runtime_size()

[Roberto Sassu]

Remove '+ 1' from 'size += strlen(entry->template_desc->name) + 1;',
as the template name is sent to userspace without the '\0' character.

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 security/integrity/ima/ima_queue.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c
index 24984a2..2f7e03c 100644
--- a/security/integrity/ima/ima_queue.c
+++ b/security/integrity/ima/ima_queue.c
@@ -81,7 +81,7 @@ int ima_get_template_entry_size(struct ima_template_entry *entry)
 	size += sizeof(u32);	/* pcr */
 	size += sizeof(entry->digest);
 	size += sizeof(int);	/* template name size field */
-	size += strlen(entry->template_desc->name) + 1;
+	size += strlen(entry->template_desc->name);
 	size += sizeof(entry->template_data_len);
 	size += entry->template_data_len;
 	return size;
-- 
2.9.3

[PATCH 7/7] ima: fix get_binary_runtime_size()

[Roberto Sassu]

Remove '+ 1' from 'size += strlen(entry->template_desc->name) + 1;',
as the template name is sent to userspace without the '\0' character.

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 security/integrity/ima/ima_queue.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c
index 24984a2..2f7e03c 100644
--- a/security/integrity/ima/ima_queue.c
+++ b/security/integrity/ima/ima_queue.c
@@ -81,7 +81,7 @@ int ima_get_template_entry_size(struct ima_template_entry *entry)
 	size += sizeof(u32);	/* pcr */
 	size += sizeof(entry->digest);
 	size += sizeof(int);	/* template name size field */
-	size += strlen(entry->template_desc->name) + 1;
+	size += strlen(entry->template_desc->name);
 	size += sizeof(entry->template_data_len);
 	size += entry->template_data_len;
 	return size;
-- 
2.9.3

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Linux-ima-devel] [PATCH 0/7] IMA: new parser for ima_restore_measurement_list()

[Ken Goldman]

On 5/16/2017 8:53 AM, Roberto Sassu wrote:
> A new IMA measurement list format, called Crypto Agile, will be introduced
> shortly to take full advantage of the algorithm flexibility of TPM 2.0.
> With the new format, it will be possible to provide for each list entry
> multiple digests, each calculated with an algorithm supported by the TPM.
> Those digests will be used by remote entities to verify the integrity of
> the measurements list.
> 
> The current (SHA1) and the new (Crypto Agile) format definitions are:
> 
> SHA1: pcr[4] digest[20]
>        template_name_len[4] template_name[template_name_len]
>        template_data_len[4] template_data[template_data_len]
> 
> Crypto Agile: pcr[4] total_digest_len[4]
>                digest1_len[4] digest1[digest1_len] ...
>                digestN_len[4] digestN[digestN_len]
>                template_name_len[4] template_name[template_name_len]
>                template_data_len[4] template_data[template_data_len]

1 - In this proposed format, how does the parser or consumer of the log
know what algorithm is used for digestN.
For example, the TCG standard format uses TPML_DIGEST_VALUES
	uint32_t count - the number of digests TPMT_HA
	TPMT_HA digests[]

where a TPMT_HA is
	algorithm identifier
	digest byte array

2 - Not a criticism, just a question for understanding ...  Would it be 
true that the total_digest_length == the sum of all the digestN_len 
values plus 4 bytes for each length.

Does it determine how many digests there are by when the total length is 
consumed?

[Linux-ima-devel] [PATCH 0/7] IMA: new parser for ima_restore_measurement_list()

[Ken Goldman]

On 5/16/2017 8:53 AM, Roberto Sassu wrote:
> A new IMA measurement list format, called Crypto Agile, will be introduced
> shortly to take full advantage of the algorithm flexibility of TPM 2.0.
> With the new format, it will be possible to provide for each list entry
> multiple digests, each calculated with an algorithm supported by the TPM.
> Those digests will be used by remote entities to verify the integrity of
> the measurements list.
> 
> The current (SHA1) and the new (Crypto Agile) format definitions are:
> 
> SHA1: pcr[4] digest[20]
>        template_name_len[4] template_name[template_name_len]
>        template_data_len[4] template_data[template_data_len]
> 
> Crypto Agile: pcr[4] total_digest_len[4]
>                digest1_len[4] digest1[digest1_len] ...
>                digestN_len[4] digestN[digestN_len]
>                template_name_len[4] template_name[template_name_len]
>                template_data_len[4] template_data[template_data_len]

1 - In this proposed format, how does the parser or consumer of the log
know what algorithm is used for digestN.
For example, the TCG standard format uses TPML_DIGEST_VALUES
	uint32_t count - the number of digests TPMT_HA
	TPMT_HA digests[]

where a TPMT_HA is
	algorithm identifier
	digest byte array

2 - Not a criticism, just a question for understanding ...  Would it be 
true that the total_digest_length == the sum of all the digestN_len 
values plus 4 bytes for each length.

Does it determine how many digests there are by when the total length is 
consumed?

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Linux-ima-devel] [PATCH 0/7] IMA: new parser for ima_restore_measurement_list()

[Roberto Sassu]

On 5/16/2017 9:00 PM, Ken Goldman wrote:
> On 5/16/2017 8:53 AM, Roberto Sassu wrote:
>> A new IMA measurement list format, called Crypto Agile, will be introduced
>> shortly to take full advantage of the algorithm flexibility of TPM 2.0.
>> With the new format, it will be possible to provide for each list entry
>> multiple digests, each calculated with an algorithm supported by the TPM.
>> Those digests will be used by remote entities to verify the integrity of
>> the measurements list.
>>
>> The current (SHA1) and the new (Crypto Agile) format definitions are:
>>
>> SHA1: pcr[4] digest[20]
>>        template_name_len[4] template_name[template_name_len]
>>        template_data_len[4] template_data[template_data_len]
>>
>> Crypto Agile: pcr[4] total_digest_len[4]
>>                digest1_len[4] digest1[digest1_len] ...
>>                digestN_len[4] digestN[digestN_len]
>>                template_name_len[4] template_name[template_name_len]
>>                template_data_len[4] template_data[template_data_len]
>
> 1 - In this proposed format, how does the parser or consumer of the log
> know what algorithm is used for digestN.

The format of digestN is: <algo name>:\0<digest value>, the same used
for the file digest.


> For example, the TCG standard format uses TPML_DIGEST_VALUES
> 	uint32_t count - the number of digests TPMT_HA
> 	TPMT_HA digests[]
>
> where a TPMT_HA is
> 	algorithm identifier
> 	digest byte array
>
> 2 - Not a criticism, just a question for understanding ...  Would it be
> true that the total_digest_length == the sum of all the digestN_len
> values plus 4 bytes for each length.

Yes.


> Does it determine how many digests there are by when the total length is
> consumed?

Yes. Since the number of digests is unknown until the buffer is parsed,
the parser consumes the data until the buffer end is reached. Then,
it returns the number of digests to the caller.

Roberto

[Linux-ima-devel] [PATCH 0/7] IMA: new parser for ima_restore_measurement_list()

[Roberto Sassu]

On 5/16/2017 9:00 PM, Ken Goldman wrote:
> On 5/16/2017 8:53 AM, Roberto Sassu wrote:
>> A new IMA measurement list format, called Crypto Agile, will be introduced
>> shortly to take full advantage of the algorithm flexibility of TPM 2.0.
>> With the new format, it will be possible to provide for each list entry
>> multiple digests, each calculated with an algorithm supported by the TPM.
>> Those digests will be used by remote entities to verify the integrity of
>> the measurements list.
>>
>> The current (SHA1) and the new (Crypto Agile) format definitions are:
>>
>> SHA1: pcr[4] digest[20]
>>        template_name_len[4] template_name[template_name_len]
>>        template_data_len[4] template_data[template_data_len]
>>
>> Crypto Agile: pcr[4] total_digest_len[4]
>>                digest1_len[4] digest1[digest1_len] ...
>>                digestN_len[4] digestN[digestN_len]
>>                template_name_len[4] template_name[template_name_len]
>>                template_data_len[4] template_data[template_data_len]
>
> 1 - In this proposed format, how does the parser or consumer of the log
> know what algorithm is used for digestN.

The format of digestN is: <algo name>:\0<digest value>, the same used
for the file digest.


> For example, the TCG standard format uses TPML_DIGEST_VALUES
> 	uint32_t count - the number of digests TPMT_HA
> 	TPMT_HA digests[]
>
> where a TPMT_HA is
> 	algorithm identifier
> 	digest byte array
>
> 2 - Not a criticism, just a question for understanding ...  Would it be
> true that the total_digest_length == the sum of all the digestN_len
> values plus 4 bytes for each length.

Yes.


> Does it determine how many digests there are by when the total length is
> consumed?

Yes. Since the number of digests is unknown until the buffer is parsed,
the parser consumes the data until the buffer end is reached. Then,
it returns the number of digests to the caller.

Roberto
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Linux-ima-devel] [PATCH 0/7] IMA: new parser for ima_restore_measurement_list()

[Ken Goldman]

On 5/17/2017 3:25 AM, Roberto Sassu wrote:
> 
> The format of digestN is: <algo name>:\0<digest value>, the same used
> for the file digest.

Since the format is changing from the SHA-1 log format anyway ...

How do people feel about the colon and null terminated string format for 
algorithm identifiers?

The TCG standard enumerations are uint16_t, and there is a registry of 
hash algorithms.

As a consuming parser, it feels nice to know it's always 2 bytes and not 
have to worry about a missing colon or a missing nul terminator risking 
a buffer overflow.

[Linux-ima-devel] [PATCH 0/7] IMA: new parser for ima_restore_measurement_list()

[Ken Goldman]

On 5/17/2017 3:25 AM, Roberto Sassu wrote:
> 
> The format of digestN is: <algo name>:\0<digest value>, the same used
> for the file digest.

Since the format is changing from the SHA-1 log format anyway ...

How do people feel about the colon and null terminated string format for 
algorithm identifiers?

The TCG standard enumerations are uint16_t, and there is a registry of 
hash algorithms.

As a consuming parser, it feels nice to know it's always 2 bytes and not 
have to worry about a missing colon or a missing nul terminator risking 
a buffer overflow.

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Linux-ima-devel] [PATCH 0/7] IMA: new parser for ima_restore_measurement_list()

[Roberto Sassu]

On 5/17/2017 6:28 PM, Ken Goldman wrote:
> On 5/17/2017 3:25 AM, Roberto Sassu wrote:
>>
>> The format of digestN is: <algo name>:\0<digest value>, the same used
>> for the file digest.
>
> Since the format is changing from the SHA-1 log format anyway ...
>
> How do people feel about the colon and null terminated string format for
> algorithm identifiers?
>
> The TCG standard enumerations are uint16_t, and there is a registry of
> hash algorithms.
>
> As a consuming parser, it feels nice to know it's always 2 bytes and not
> have to worry about a missing colon or a missing nul terminator risking
> a buffer overflow.

There cannot be buffer overflow, because the length of each digest
field is known.

Roberto

[Linux-ima-devel] [PATCH 0/7] IMA: new parser for ima_restore_measurement_list()

[Roberto Sassu]

On 5/17/2017 6:28 PM, Ken Goldman wrote:
> On 5/17/2017 3:25 AM, Roberto Sassu wrote:
>>
>> The format of digestN is: <algo name>:\0<digest value>, the same used
>> for the file digest.
>
> Since the format is changing from the SHA-1 log format anyway ...
>
> How do people feel about the colon and null terminated string format for
> algorithm identifiers?
>
> The TCG standard enumerations are uint16_t, and there is a registry of
> hash algorithms.
>
> As a consuming parser, it feels nice to know it's always 2 bytes and not
> have to worry about a missing colon or a missing nul terminator risking
> a buffer overflow.

There cannot be buffer overflow, because the length of each digest
field is known.

Roberto
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Linux-ima-devel] [PATCH 0/7] IMA: new parser for ima_restore_measurement_list()

[Ken Goldman]

On 5/18/2017 5:38 AM, Roberto Sassu wrote:
> On 5/17/2017 6:28 PM, Ken Goldman wrote:
>> On 5/17/2017 3:25 AM, Roberto Sassu wrote:
>>>
>>> The format of digestN is: <algo name>:\0<digest value>, the same used
>>> for the file digest.
>>
>> Since the format is changing from the SHA-1 log format anyway ...
>>
>> How do people feel about the colon and null terminated string format for
>> algorithm identifiers?
>>
>> The TCG standard enumerations are uint16_t, and there is a registry of
>> hash algorithms.
>>
>> As a consuming parser, it feels nice to know it's always 2 bytes and not
>> have to worry about a missing colon or a missing nul terminator risking
>> a buffer overflow.
> 
> There cannot be buffer overflow, because the length of each digest
> field is known.
> 
> Roberto
> 

I was not referring to the digest, but the digest algorithm.

I wanted opinions on the colon and null terminated string format for 
algorithm identifiers.

The TCG standard log uses the TCG standard enumerations.  They're always 
exactly 2 bytes.  Parsing is trivial.

If IMA uses strings, the attacker can send, e.g., sha1: and not null 
terminate it.  A careful parser can go a byte at a time until it reaches 
a maximum length - if you specify a maximum length.  But it is an attack 
surface.  Is there a corresponding advantage?

[Linux-ima-devel] [PATCH 0/7] IMA: new parser for ima_restore_measurement_list()

[Ken Goldman]

On 5/18/2017 5:38 AM, Roberto Sassu wrote:
> On 5/17/2017 6:28 PM, Ken Goldman wrote:
>> On 5/17/2017 3:25 AM, Roberto Sassu wrote:
>>>
>>> The format of digestN is: <algo name>:\0<digest value>, the same used
>>> for the file digest.
>>
>> Since the format is changing from the SHA-1 log format anyway ...
>>
>> How do people feel about the colon and null terminated string format for
>> algorithm identifiers?
>>
>> The TCG standard enumerations are uint16_t, and there is a registry of
>> hash algorithms.
>>
>> As a consuming parser, it feels nice to know it's always 2 bytes and not
>> have to worry about a missing colon or a missing nul terminator risking
>> a buffer overflow.
> 
> There cannot be buffer overflow, because the length of each digest
> field is known.
> 
> Roberto
> 

I was not referring to the digest, but the digest algorithm.

I wanted opinions on the colon and null terminated string format for 
algorithm identifiers.

The TCG standard log uses the TCG standard enumerations.  They're always 
exactly 2 bytes.  Parsing is trivial.

If IMA uses strings, the attacker can send, e.g., sha1: and not null 
terminate it.  A careful parser can go a byte at a time until it reaches 
a maximum length - if you specify a maximum length.  But it is an attack 
surface.  Is there a corresponding advantage?

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Linux-ima-devel] [PATCH 0/7] IMA: new parser for ima_restore_measurement_list()

[Ken Goldman]

On 5/18/2017 5:38 AM, Roberto Sassu wrote:
> 
> There cannot be buffer overflow, because the length of each digest
> field is known.

Crypto Agile: pcr[4] total_digest_len[4]
                digest1_len[4] digest1[digest1_len] ...

The way I read this, the digest length is supplied by the caller, which 
is slightly different from "known".  For example, if I supply a digest 
length of 0xffffffff, a too trusting (buggy) parser could overflow the 
buffer.

total_digest_len is similarly untrusted.  The attacker can send invalid 
values.

[Linux-ima-devel] [PATCH 0/7] IMA: new parser for ima_restore_measurement_list()

[Ken Goldman]

On 5/18/2017 5:38 AM, Roberto Sassu wrote:
> 
> There cannot be buffer overflow, because the length of each digest
> field is known.

Crypto Agile: pcr[4] total_digest_len[4]
                digest1_len[4] digest1[digest1_len] ...

The way I read this, the digest length is supplied by the caller, which 
is slightly different from "known".  For example, if I supply a digest 
length of 0xffffffff, a too trusting (buggy) parser could overflow the 
buffer.

total_digest_len is similarly untrusted.  The attacker can send invalid 
values.


--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Linux-ima-devel] [PATCH 0/7] IMA: new parser for ima_restore_measurement_list()

[Roberto Sassu]

On 5/23/2017 10:48 PM, Ken Goldman wrote:
> On 5/18/2017 5:38 AM, Roberto Sassu wrote:
>> On 5/17/2017 6:28 PM, Ken Goldman wrote:
>>> On 5/17/2017 3:25 AM, Roberto Sassu wrote:
>>>>
>>>> The format of digestN is: <algo name>:\0<digest value>, the same used
>>>> for the file digest.
>>>
>>> Since the format is changing from the SHA-1 log format anyway ...
>>>
>>> How do people feel about the colon and null terminated string format for
>>> algorithm identifiers?
>>>
>>> The TCG standard enumerations are uint16_t, and there is a registry of
>>> hash algorithms.
>>>
>>> As a consuming parser, it feels nice to know it's always 2 bytes and not
>>> have to worry about a missing colon or a missing nul terminator risking
>>> a buffer overflow.
>>
>> There cannot be buffer overflow, because the length of each digest
>> field is known.
>>
>> Roberto
>>
>
> I was not referring to the digest, but the digest algorithm.
>
> I wanted opinions on the colon and null terminated string format for
> algorithm identifiers.
>
> The TCG standard log uses the TCG standard enumerations.  They're always
> exactly 2 bytes.  Parsing is trivial.

I have two concerns regarding this:

is there a standard way to convert TPM_ALG_ to strings, like a function
exposed by the TSS? If not, suppose that a parser uses openssl to verify
the integrity of event data, by calculating the digest. Then,
the parser should maintain an association table between TPM_ALG_
and a string (the string will be passed to EVP_get_digestbyname()).
When a new TPM algorithm is added to the TCG registry, all parsers
should be modified to update the association table. If IMA sends
a string, only the crypto subsystem has to be updated.

The format I'm proposing for event data digests would be the same
of that used for file digests. Should IMA provide a list with
two different formats?

Roberto


> If IMA uses strings, the attacker can send, e.g., sha1: and not null
> terminate it.  A careful parser can go a byte at a time until it reaches
> a maximum length - if you specify a maximum length.  But it is an attack
> surface.  Is there a corresponding advantage?
>
>
> ------------------------------------------------------------------------------
> Check out the vibrant tech community on one of the world's most
> engaging tech sites, Slashdot.org! http://sdm.link/slashdot
> _______________________________________________
> Linux-ima-devel mailing list
> Linux-ima-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-ima-devel
>

[Linux-ima-devel] [PATCH 0/7] IMA: new parser for ima_restore_measurement_list()

[Roberto Sassu]

On 5/23/2017 10:48 PM, Ken Goldman wrote:
> On 5/18/2017 5:38 AM, Roberto Sassu wrote:
>> On 5/17/2017 6:28 PM, Ken Goldman wrote:
>>> On 5/17/2017 3:25 AM, Roberto Sassu wrote:
>>>>
>>>> The format of digestN is: <algo name>:\0<digest value>, the same used
>>>> for the file digest.
>>>
>>> Since the format is changing from the SHA-1 log format anyway ...
>>>
>>> How do people feel about the colon and null terminated string format for
>>> algorithm identifiers?
>>>
>>> The TCG standard enumerations are uint16_t, and there is a registry of
>>> hash algorithms.
>>>
>>> As a consuming parser, it feels nice to know it's always 2 bytes and not
>>> have to worry about a missing colon or a missing nul terminator risking
>>> a buffer overflow.
>>
>> There cannot be buffer overflow, because the length of each digest
>> field is known.
>>
>> Roberto
>>
>
> I was not referring to the digest, but the digest algorithm.
>
> I wanted opinions on the colon and null terminated string format for
> algorithm identifiers.
>
> The TCG standard log uses the TCG standard enumerations.  They're always
> exactly 2 bytes.  Parsing is trivial.

I have two concerns regarding this:

is there a standard way to convert TPM_ALG_ to strings, like a function
exposed by the TSS? If not, suppose that a parser uses openssl to verify
the integrity of event data, by calculating the digest. Then,
the parser should maintain an association table between TPM_ALG_
and a string (the string will be passed to EVP_get_digestbyname()).
When a new TPM algorithm is added to the TCG registry, all parsers
should be modified to update the association table. If IMA sends
a string, only the crypto subsystem has to be updated.

The format I'm proposing for event data digests would be the same
of that used for file digests. Should IMA provide a list with
two different formats?

Roberto


> If IMA uses strings, the attacker can send, e.g., sha1: and not null
> terminate it.  A careful parser can go a byte at a time until it reaches
> a maximum length - if you specify a maximum length.  But it is an attack
> surface.  Is there a corresponding advantage?
>
>
> ------------------------------------------------------------------------------
> Check out the vibrant tech community on one of the world's most
> engaging tech sites, Slashdot.org! http://sdm.link/slashdot
> _______________________________________________
> Linux-ima-devel mailing list
> Linux-ima-devel at lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-ima-devel
>
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Linux-ima-devel] [PATCH 1/7] ima: introduce ima_parse_buf()

[Mimi Zohar]

Hi Roberto,

On Tue, 2017-05-16 at 14:53 +0200, Roberto Sassu wrote:
> ima_parse_buf() takes as input the buffer start and end pointers, and
> stores the result in a static array of ima_field_data structures,
> where the len field contains the length parsed from the buffer, and
> the data field contains the address of the buffer just after the length.
> Optionally, the function returns the current value of the buffer pointer
> and the number of array elements written.
> 
> A bitmap has been added as parameter of ima_parse_buf() to handle
> the cases where the length is not prepended to data. Each bit corresponds
> to an element of the ima_field_data array. If a bit is set, the length
> is not parsed from the buffer, but is read from the corresponding element
> of the array (the length must be set before calling the function).
> 
> ima_parse_buf() can perform three checks upon request by callers,
> depending on the enforce mask passed to it:
> 
> - ENFORCE_FIELDS: matching of number of fields (length-data combination)
>   - there must be enough data in the buffer to parse the number of fields
>     requested (output: current value of buffer pointer)
> - ENFORCE_BUFEND: matching of buffer end
>   - the ima_field_data array must be large enough to contain lengths and
>     data pointers for the amount of data requested (output: number
>     of fields written)
> - ENFORCE_FIELDS | ENFORCE_BUFEND: matching of both
> 
> Use cases
> 
> - measurement entry header: ENFORCE_FIELDS | ENFORCE_BUFEND
>   - four fields must be parsed: pcr, digest, template name, template data
>   - ENFORCE_BUFEND is enforced only for the last measurement entry
> - template digest (Crypto Agile): ENFORCE_BUFEND
>   - since only the total template digest length is known, the function
>     parses length-data combinations until the buffer end is reached
> - template data: ENFORCE_FIELDS | ENFORCE_BUFEND
>   - since the number of fields and the total template data length
>     are known, the function can perform both checks
> 

Thanks, Roberto.  Patches 1 - 3, and 7 look good.  I wish we didn't
need the len_mask or the enforce_mask fields.

Mimi

> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
>  security/integrity/ima/ima_template_lib.c | 61 +++++++++++++++++++++++++++++++
>  security/integrity/ima/ima_template_lib.h |  6 +++
>  2 files changed, 67 insertions(+)
> 
> diff --git a/security/integrity/ima/ima_template_lib.c b/security/integrity/ima/ima_template_lib.c
> index f9ba37b..28af43f 100644
> --- a/security/integrity/ima/ima_template_lib.c
> +++ b/security/integrity/ima/ima_template_lib.c
> @@ -159,6 +159,67 @@ 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);
>  }
> 
> +/**
> + * ima_parse_buf() - Parses lengths and data from an input buffer
> + * @bufstartp:       Buffer start address.
> + * @bufendp:         Buffer end address.
> + * @bufcurp:         Pointer to remaining (non-parsed) data.
> + * @maxfields:       Length of fields array.
> + * @fields:          Array containing lengths and pointers of parsed data.
> + * @curfields:       Number of array items containing parsed data.
> + * @len_mask:        Bitmap (if bit is set, data length should not be parsed).
> + * @enforce_mask:    Check if curfields == maxfields and/or bufcurp == bufendp.
> + * @bufname:         String identifier of the input buffer.
> + *
> + * Return: 0 on success, -EINVAL on error.
> + */
> +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)
> +{
> +	void *bufp = bufstartp;
> +	int i;
> +
> +	for (i = 0; i < maxfields; i++) {
> +		if (len_mask == NULL || !test_bit(i, len_mask)) {
> +			if (bufp > (bufendp - sizeof(u32)))
> +				break;
> +
> +			fields[i].len = *(u32 *)bufp;
> +			if (ima_canonical_fmt)
> +				fields[i].len = le32_to_cpu(fields[i].len);
> +
> +			bufp += sizeof(u32);
> +		}
> +
> +		if (bufp > (bufendp - fields[i].len))
> +			break;
> +
> +		fields[i].data = bufp;
> +		bufp += fields[i].len;
> +	}
> +
> +	if ((enforce_mask & ENFORCE_FIELDS) && i != maxfields) {
> +		pr_err("%s: nr of fields mismatch: expected: %d, current: %d\n",
> +		       bufname, maxfields, i);
> +		return -EINVAL;
> +	}
> +
> +	if ((enforce_mask & ENFORCE_BUFEND) && bufp != bufendp) {
> +		pr_err("%s: buf end mismatch: expected: %p, current: %p\n",
> +		       bufname, bufendp, bufp);
> +		return -EINVAL;
> +	}
> +
> +	if (curfields)
> +		*curfields = i;
> +
> +	if (bufcurp)
> +		*bufcurp = bufp;
> +
> +	return 0;
> +}
> +
>  static int ima_eventdigest_init_common(u8 *digest, u32 digestsize, u8 hash_algo,
>  				       struct ima_field_data *field_data)
>  {
> diff --git a/security/integrity/ima/ima_template_lib.h b/security/integrity/ima/ima_template_lib.h
> index c344530..6a3d8b8 100644
> --- a/security/integrity/ima/ima_template_lib.h
> +++ b/security/integrity/ima/ima_template_lib.h
> @@ -18,6 +18,9 @@
>  #include <linux/seq_file.h>
>  #include "ima.h"
> 
> +#define ENFORCE_FIELDS 0x00000001
> +#define ENFORCE_BUFEND 0x00000002
> +
>  void ima_show_template_digest(struct seq_file *m, enum ima_show_type show,
>  			      struct ima_field_data *field_data);
>  void ima_show_template_digest_ng(struct seq_file *m, enum ima_show_type show,
> @@ -26,6 +29,9 @@ 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);
> +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);
>  int ima_eventdigest_init(struct ima_event_data *event_data,
>  			 struct ima_field_data *field_data);
>  int ima_eventname_init(struct ima_event_data *event_data,

[Linux-ima-devel] [PATCH 1/7] ima: introduce ima_parse_buf()

[Mimi Zohar]

Hi Roberto,

On Tue, 2017-05-16 at 14:53 +0200, Roberto Sassu wrote:
> ima_parse_buf() takes as input the buffer start and end pointers, and
> stores the result in a static array of ima_field_data structures,
> where the len field contains the length parsed from the buffer, and
> the data field contains the address of the buffer just after the length.
> Optionally, the function returns the current value of the buffer pointer
> and the number of array elements written.
> 
> A bitmap has been added as parameter of ima_parse_buf() to handle
> the cases where the length is not prepended to data. Each bit corresponds
> to an element of the ima_field_data array. If a bit is set, the length
> is not parsed from the buffer, but is read from the corresponding element
> of the array (the length must be set before calling the function).
> 
> ima_parse_buf() can perform three checks upon request by callers,
> depending on the enforce mask passed to it:
> 
> - ENFORCE_FIELDS: matching of number of fields (length-data combination)
>   - there must be enough data in the buffer to parse the number of fields
>     requested (output: current value of buffer pointer)
> - ENFORCE_BUFEND: matching of buffer end
>   - the ima_field_data array must be large enough to contain lengths and
>     data pointers for the amount of data requested (output: number
>     of fields written)
> - ENFORCE_FIELDS | ENFORCE_BUFEND: matching of both
> 
> Use cases
> 
> - measurement entry header: ENFORCE_FIELDS | ENFORCE_BUFEND
>   - four fields must be parsed: pcr, digest, template name, template data
>   - ENFORCE_BUFEND is enforced only for the last measurement entry
> - template digest (Crypto Agile): ENFORCE_BUFEND
>   - since only the total template digest length is known, the function
>     parses length-data combinations until the buffer end is reached
> - template data: ENFORCE_FIELDS | ENFORCE_BUFEND
>   - since the number of fields and the total template data length
>     are known, the function can perform both checks
> 

Thanks, Roberto. ?Patches 1 - 3, and 7 look good. ?I wish we didn't
need the len_mask or the enforce_mask fields.

Mimi

> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
>  security/integrity/ima/ima_template_lib.c | 61 +++++++++++++++++++++++++++++++
>  security/integrity/ima/ima_template_lib.h |  6 +++
>  2 files changed, 67 insertions(+)
> 
> diff --git a/security/integrity/ima/ima_template_lib.c b/security/integrity/ima/ima_template_lib.c
> index f9ba37b..28af43f 100644
> --- a/security/integrity/ima/ima_template_lib.c
> +++ b/security/integrity/ima/ima_template_lib.c
> @@ -159,6 +159,67 @@ 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);
>  }
> 
> +/**
> + * ima_parse_buf() - Parses lengths and data from an input buffer
> + * @bufstartp:       Buffer start address.
> + * @bufendp:         Buffer end address.
> + * @bufcurp:         Pointer to remaining (non-parsed) data.
> + * @maxfields:       Length of fields array.
> + * @fields:          Array containing lengths and pointers of parsed data.
> + * @curfields:       Number of array items containing parsed data.
> + * @len_mask:        Bitmap (if bit is set, data length should not be parsed).
> + * @enforce_mask:    Check if curfields == maxfields and/or bufcurp == bufendp.
> + * @bufname:         String identifier of the input buffer.
> + *
> + * Return: 0 on success, -EINVAL on error.
> + */
> +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)
> +{
> +	void *bufp = bufstartp;
> +	int i;
> +
> +	for (i = 0; i < maxfields; i++) {
> +		if (len_mask == NULL || !test_bit(i, len_mask)) {
> +			if (bufp > (bufendp - sizeof(u32)))
> +				break;
> +
> +			fields[i].len = *(u32 *)bufp;
> +			if (ima_canonical_fmt)
> +				fields[i].len = le32_to_cpu(fields[i].len);
> +
> +			bufp += sizeof(u32);
> +		}
> +
> +		if (bufp > (bufendp - fields[i].len))
> +			break;
> +
> +		fields[i].data = bufp;
> +		bufp += fields[i].len;
> +	}
> +
> +	if ((enforce_mask & ENFORCE_FIELDS) && i != maxfields) {
> +		pr_err("%s: nr of fields mismatch: expected: %d, current: %d\n",
> +		       bufname, maxfields, i);
> +		return -EINVAL;
> +	}
> +
> +	if ((enforce_mask & ENFORCE_BUFEND) && bufp != bufendp) {
> +		pr_err("%s: buf end mismatch: expected: %p, current: %p\n",
> +		       bufname, bufendp, bufp);
> +		return -EINVAL;
> +	}
> +
> +	if (curfields)
> +		*curfields = i;
> +
> +	if (bufcurp)
> +		*bufcurp = bufp;
> +
> +	return 0;
> +}
> +
>  static int ima_eventdigest_init_common(u8 *digest, u32 digestsize, u8 hash_algo,
>  				       struct ima_field_data *field_data)
>  {
> diff --git a/security/integrity/ima/ima_template_lib.h b/security/integrity/ima/ima_template_lib.h
> index c344530..6a3d8b8 100644
> --- a/security/integrity/ima/ima_template_lib.h
> +++ b/security/integrity/ima/ima_template_lib.h
> @@ -18,6 +18,9 @@
>  #include <linux/seq_file.h>
>  #include "ima.h"
> 
> +#define ENFORCE_FIELDS 0x00000001
> +#define ENFORCE_BUFEND 0x00000002
> +
>  void ima_show_template_digest(struct seq_file *m, enum ima_show_type show,
>  			      struct ima_field_data *field_data);
>  void ima_show_template_digest_ng(struct seq_file *m, enum ima_show_type show,
> @@ -26,6 +29,9 @@ 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);
> +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);
>  int ima_eventdigest_init(struct ima_event_data *event_data,
>  			 struct ima_field_data *field_data);
>  int ima_eventname_init(struct ima_event_data *event_data,

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Linux-ima-devel] [PATCH 6/7] ima: add securityfs interface to restore a measurements list

[Mimi Zohar]

On Tue, 2017-05-16 at 14:53 +0200, Roberto Sassu wrote:
> Through the new interface restore_kexec_list, it will be possible
> to restore a measurements list, previously read from
> binary_kexec_runtime_measurements.

For development, this was fine.  You were able to save and restore the
measurement list with the kexec hdr, but do we really need two
securityfs files, one for saving the measurement list and another for
restoring it?  Just like we can read and write the IMA policy, we
should be able to save and restore the kexec list using a single
securityfs file.

> The patch reuses the policy functions to create a buffer, read
> the measurements and call ima_restore_measurement_list().

Calling a function named ima_read_policy() for restoring the
measurement list is unusual.  Again, this was fine for development,
but to upstream this functionality it should be cleaned up.  Normally,
there would be a call to a common generic function from multiple
places.

Mimi


> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
>  security/integrity/ima/ima_fs.c | 38 ++++++++++++++++++++++++++++++++++----
>  1 file changed, 34 insertions(+), 4 deletions(-)
> 
> diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
> index a93f941..6e3f93f 100644
> --- a/security/integrity/ima/ima_fs.c
> +++ b/security/integrity/ima/ima_fs.c
> @@ -77,6 +77,7 @@ static const struct file_operations ima_measurements_count_ops = {
>  };
> 
>  static struct dentry *binary_kexec_runtime_measurements;
> +static struct dentry *restore_kexec_list;
> 
>  /* returns pointer to hlist_node */
>  static void *ima_measurements_start(struct seq_file *m, loff_t *pos)
> @@ -297,7 +298,7 @@ static const struct file_operations ima_ascii_measurements_ops = {
>  	.release = seq_release,
>  };
> 
> -static ssize_t ima_read_policy(char *path)
> +static ssize_t ima_read_policy(char *path, bool khdr)
>  {
>  	void *data;
>  	char *datap;
> @@ -317,6 +318,13 @@ static ssize_t ima_read_policy(char *path)
>  	}
> 
>  	datap = data;
> +
> +	if (khdr) {
> +		rc = ima_restore_measurement_list(size, data);
> +		size = 0;
> +		goto out;
> +	}
> +
>  	while (size > 0 && (p = strsep(&datap, "\n"))) {
>  		pr_debug("rule: %s\n", p);
>  		rc = ima_parse_add_rule(p);
> @@ -325,6 +333,7 @@ static ssize_t ima_read_policy(char *path)
>  		size -= rc;
>  	}
> 
> +out:
>  	vfree(data);
>  	if (rc < 0)
>  		return rc;
> @@ -337,6 +346,7 @@ static ssize_t ima_read_policy(char *path)
>  static ssize_t ima_write_policy(struct file *file, const char __user *buf,
>  				size_t datalen, loff_t *ppos)
>  {
> +	bool khdr = file->f_path.dentry == restore_kexec_list;
>  	char *data;
>  	ssize_t result;
> 
> @@ -363,8 +373,13 @@ static ssize_t ima_write_policy(struct file *file, const char __user *buf,
>  	if (result < 0)
>  		goto out_free;
> 
> +	if (khdr && data[0] != '/') {
> +		mutex_unlock(&ima_write_mutex);
> +		goto out_free;
> +	}
> +
>  	if (data[0] == '/') {
> -		result = ima_read_policy(data);
> +		result = ima_read_policy(data, khdr);
>  	} else if (ima_appraise & IMA_APPRAISE_POLICY) {
>  		pr_err("IMA: signed policy file (specified as an absolute pathname) required\n");
>  		integrity_audit_msg(AUDIT_INTEGRITY_STATUS, NULL, NULL,
> @@ -393,7 +408,7 @@ static struct dentry *violations;
>  static struct dentry *ima_policy;
> 
>  enum ima_fs_flags {
> -	IMA_FS_BUSY,
> +	IMA_FS_BUSY, IMA_RESTORE_LIST_BUSY,
>  };
> 
>  static unsigned long ima_fs_flags;
> @@ -412,6 +427,9 @@ static const struct seq_operations ima_policy_seqops = {
>   */
>  static int ima_open_policy(struct inode *inode, struct file *filp)
>  {
> +	bool khdr = filp->f_path.dentry == restore_kexec_list;
> +	unsigned long bit = khdr ? IMA_RESTORE_LIST_BUSY : IMA_FS_BUSY;
> +
>  	if (!(filp->f_flags & O_WRONLY)) {
>  #ifndef	CONFIG_IMA_READ_POLICY
>  		return -EACCES;
> @@ -423,7 +441,7 @@ static int ima_open_policy(struct inode *inode, struct file *filp)
>  		return seq_open(filp, &ima_policy_seqops);
>  #endif
>  	}
> -	if (test_and_set_bit(IMA_FS_BUSY, &ima_fs_flags))
> +	if (test_and_set_bit(bit, &ima_fs_flags))
>  		return -EBUSY;
>  	return 0;
>  }
> @@ -439,6 +457,11 @@ static int ima_release_policy(struct inode *inode, struct file *file)
>  {
>  	const char *cause = valid_policy ? "completed" : "failed";
> 
> +	if (file->f_path.dentry == restore_kexec_list) {
> +		clear_bit(IMA_RESTORE_LIST_BUSY, &ima_fs_flags);
> +		return 0;
> +	}
> +
>  	if ((file->f_flags & O_ACCMODE) == O_RDONLY)
>  		return seq_release(inode, file);
> 
> @@ -522,9 +545,16 @@ int __init ima_fs_init(void)
>  				   &ima_measurements_ops);
>  	if (IS_ERR(binary_kexec_runtime_measurements))
>  		goto out;
> +
> +	restore_kexec_list = securityfs_create_file("restore_kexec_list",
> +						    S_IWUSR, ima_dir, NULL,
> +						    &ima_measure_policy_ops);
> +	if (IS_ERR(restore_kexec_list))
> +		goto out;
>  #endif
>  	return 0;
>  out:
> +	securityfs_remove(restore_kexec_list);
>  	securityfs_remove(binary_kexec_runtime_measurements);
>  	securityfs_remove(violations);
>  	securityfs_remove(runtime_measurements_count);

[Linux-ima-devel] [PATCH 6/7] ima: add securityfs interface to restore a measurements list

[Mimi Zohar]

On Tue, 2017-05-16 at 14:53 +0200, Roberto Sassu wrote:
> Through the new interface restore_kexec_list, it will be possible
> to restore a measurements list, previously read from
> binary_kexec_runtime_measurements.

For development, this was fine. ?You were able to save and restore the
measurement list with the kexec hdr, but do we really need two
securityfs files, one for saving the measurement list and another for
restoring it? ?Just like we can read and write the IMA policy, we
should be able to save and restore the kexec list using a single
securityfs file.

> The patch reuses the policy functions to create a buffer, read
> the measurements and call ima_restore_measurement_list().

Calling a function named ima_read_policy() for restoring the
measurement list is unusual. ?Again, this was fine for development,
but to upstream this functionality it should be cleaned up. ?Normally,
there would be a call to a common generic function from multiple
places.

Mimi


> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
>  security/integrity/ima/ima_fs.c | 38 ++++++++++++++++++++++++++++++++++----
>  1 file changed, 34 insertions(+), 4 deletions(-)
> 
> diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
> index a93f941..6e3f93f 100644
> --- a/security/integrity/ima/ima_fs.c
> +++ b/security/integrity/ima/ima_fs.c
> @@ -77,6 +77,7 @@ static const struct file_operations ima_measurements_count_ops = {
>  };
> 
>  static struct dentry *binary_kexec_runtime_measurements;
> +static struct dentry *restore_kexec_list;
> 
>  /* returns pointer to hlist_node */
>  static void *ima_measurements_start(struct seq_file *m, loff_t *pos)
> @@ -297,7 +298,7 @@ static const struct file_operations ima_ascii_measurements_ops = {
>  	.release = seq_release,
>  };
> 
> -static ssize_t ima_read_policy(char *path)
> +static ssize_t ima_read_policy(char *path, bool khdr)
>  {
>  	void *data;
>  	char *datap;
> @@ -317,6 +318,13 @@ static ssize_t ima_read_policy(char *path)
>  	}
> 
>  	datap = data;
> +
> +	if (khdr) {
> +		rc = ima_restore_measurement_list(size, data);
> +		size = 0;
> +		goto out;
> +	}
> +
>  	while (size > 0 && (p = strsep(&datap, "\n"))) {
>  		pr_debug("rule: %s\n", p);
>  		rc = ima_parse_add_rule(p);
> @@ -325,6 +333,7 @@ static ssize_t ima_read_policy(char *path)
>  		size -= rc;
>  	}
> 
> +out:
>  	vfree(data);
>  	if (rc < 0)
>  		return rc;
> @@ -337,6 +346,7 @@ static ssize_t ima_read_policy(char *path)
>  static ssize_t ima_write_policy(struct file *file, const char __user *buf,
>  				size_t datalen, loff_t *ppos)
>  {
> +	bool khdr = file->f_path.dentry == restore_kexec_list;
>  	char *data;
>  	ssize_t result;
> 
> @@ -363,8 +373,13 @@ static ssize_t ima_write_policy(struct file *file, const char __user *buf,
>  	if (result < 0)
>  		goto out_free;
> 
> +	if (khdr && data[0] != '/') {
> +		mutex_unlock(&ima_write_mutex);
> +		goto out_free;
> +	}
> +
>  	if (data[0] == '/') {
> -		result = ima_read_policy(data);
> +		result = ima_read_policy(data, khdr);
>  	} else if (ima_appraise & IMA_APPRAISE_POLICY) {
>  		pr_err("IMA: signed policy file (specified as an absolute pathname) required\n");
>  		integrity_audit_msg(AUDIT_INTEGRITY_STATUS, NULL, NULL,
> @@ -393,7 +408,7 @@ static struct dentry *violations;
>  static struct dentry *ima_policy;
> 
>  enum ima_fs_flags {
> -	IMA_FS_BUSY,
> +	IMA_FS_BUSY, IMA_RESTORE_LIST_BUSY,
>  };
> 
>  static unsigned long ima_fs_flags;
> @@ -412,6 +427,9 @@ static const struct seq_operations ima_policy_seqops = {
>   */
>  static int ima_open_policy(struct inode *inode, struct file *filp)
>  {
> +	bool khdr = filp->f_path.dentry == restore_kexec_list;
> +	unsigned long bit = khdr ? IMA_RESTORE_LIST_BUSY : IMA_FS_BUSY;
> +
>  	if (!(filp->f_flags & O_WRONLY)) {
>  #ifndef	CONFIG_IMA_READ_POLICY
>  		return -EACCES;
> @@ -423,7 +441,7 @@ static int ima_open_policy(struct inode *inode, struct file *filp)
>  		return seq_open(filp, &ima_policy_seqops);
>  #endif
>  	}
> -	if (test_and_set_bit(IMA_FS_BUSY, &ima_fs_flags))
> +	if (test_and_set_bit(bit, &ima_fs_flags))
>  		return -EBUSY;
>  	return 0;
>  }
> @@ -439,6 +457,11 @@ static int ima_release_policy(struct inode *inode, struct file *file)
>  {
>  	const char *cause = valid_policy ? "completed" : "failed";
> 
> +	if (file->f_path.dentry == restore_kexec_list) {
> +		clear_bit(IMA_RESTORE_LIST_BUSY, &ima_fs_flags);
> +		return 0;
> +	}
> +
>  	if ((file->f_flags & O_ACCMODE) == O_RDONLY)
>  		return seq_release(inode, file);
> 
> @@ -522,9 +545,16 @@ int __init ima_fs_init(void)
>  				   &ima_measurements_ops);
>  	if (IS_ERR(binary_kexec_runtime_measurements))
>  		goto out;
> +
> +	restore_kexec_list = securityfs_create_file("restore_kexec_list",
> +						    S_IWUSR, ima_dir, NULL,
> +						    &ima_measure_policy_ops);
> +	if (IS_ERR(restore_kexec_list))
> +		goto out;
>  #endif
>  	return 0;
>  out:
> +	securityfs_remove(restore_kexec_list);
>  	securityfs_remove(binary_kexec_runtime_measurements);
>  	securityfs_remove(violations);
>  	securityfs_remove(runtime_measurements_count);

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Linux-ima-devel] [PATCH 5/7] ima: add securityfs interface to save a measurements list with kexec header

[Mimi Zohar]

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);

[Linux-ima-devel] [PATCH 5/7] ima: add securityfs interface to save a measurements list with kexec header

[Mimi Zohar]

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 at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Linux-ima-devel] [PATCH 5/7] ima: add securityfs interface to save a measurements list with kexec header

[Roberto Sassu]

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);
>

-- 
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Bo PENG, Qiuen PENG, Shengli WANG

[Linux-ima-devel] [PATCH 5/7] ima: add securityfs interface to save a measurements list with kexec header

[Roberto Sassu]

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);
>

-- 
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Bo PENG, Qiuen PENG, Shengli WANG
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Linux-ima-devel] [PATCH 5/7] ima: add securityfs interface to save a measurements list with kexec header

[Roberto Sassu]

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);
>

-- 
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Bo PENG, Qiuen PENG, Shengli WANG

[Linux-ima-devel] [PATCH 5/7] ima: add securityfs interface to save a measurements list with kexec header

[Roberto Sassu]

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);
>

-- 
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Bo PENG, Qiuen PENG, Shengli WANG
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Linux-ima-devel] [PATCH 5/7] ima: add securityfs interface to save a measurements list with kexec header

[Mimi Zohar]

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

[Linux-ima-devel] [PATCH 5/7] ima: add securityfs interface to save a measurements list with kexec header

[Mimi Zohar]

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 at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Linux-ima-devel] [PATCH 5/7] ima: add securityfs interface to save a measurements list with kexec header

[Mimi Zohar]

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

[Linux-ima-devel] [PATCH 5/7] ima: add securityfs interface to save a measurements list with kexec header

[Mimi Zohar]

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 at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Linux-ima-devel] [PATCH 5/7] ima: add securityfs interface to save a measurements list with kexec header

[Roberto Sassu]

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
>

-- 
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Bo PENG, Qiuen PENG, Shengli WANG

[Linux-ima-devel] [PATCH 5/7] ima: add securityfs interface to save a measurements list with kexec header

[Roberto Sassu]

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
>

-- 
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Bo PENG, Qiuen PENG, Shengli WANG
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/7] ima: add securityfs interface to save a measurements list with kexec header

[Mimi Zohar]

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

[PATCH 5/7] ima: add securityfs interface to save a measurements list with kexec header

[Mimi Zohar]

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 at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/7] ima: add securityfs interface to save a measurements list with kexec header

[Roberto Sassu]

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

-- 
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Bo PENG, Qiuen PENG, Shengli WANG

[PATCH 5/7] ima: add securityfs interface to save a measurements list with kexec header

[Roberto Sassu]

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

-- 
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Bo PENG, Qiuen PENG, Shengli WANG
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/7] ima: add securityfs interface to save a measurements list with kexec header

[Mimi Zohar]

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

[PATCH 5/7] ima: add securityfs interface to save a measurements list with kexec header

[Mimi Zohar]

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 at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/7] ima: add securityfs interface to save a measurements list with kexec header

[Roberto Sassu]

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
>

-- 
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Bo PENG, Qiuen PENG, Shengli WANG

[PATCH 5/7] ima: add securityfs interface to save a measurements list with kexec header

[Roberto Sassu]

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
>

-- 
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Bo PENG, Qiuen PENG, Shengli WANG
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html