integrity

This patch adds internal kernel support for:
- reading/extending a pcr value
- looking up the tpm_chip for a given chip number and type

Signed-off-by Mimi Zohar <[email protected]>

Signed-off-by Rajiv Andrade <[email protected]>

---

drivers/char/tpm/tpm.c +23 -3

index 9c47dc4..17d2849 100644
@@ -1,11 +1,12 @@
 /*
- * Copyright (C) 2004 IBM Corporation
+ * Copyright (C) 2004,2007,2008 IBM Corporation
  *
  * Authors:
  * Leendert van Doorn <[email protected]>
  * Dave Safford <[email protected]>
  * Reiner Sailer <[email protected]>
  * Kylene Hall <[email protected]>
+ * Debora Velarde <[email protected]>
  *
  * Maintained by: <[email protected]>
  *

@@ -28,6 +29,14 @@
 #include <linux/spinlock.h>
 #include <linux/smp_lock.h>
 
+#include <linux/mm.h>
+#include <linux/slab.h>
+#include <linux/string.h>
+#include <linux/crypto.h>
+#include <linux/fs.h>
+#include <linux/scatterlist.h>
+#include <linux/rcupdate.h>
+#include <asm/unaligned.h>
 #include "tpm.h"
 
 enum tpm_const {

@@ -50,6 +59,8 @@ enum tpm_duration {
 static LIST_HEAD(tpm_chip_list);
 static DEFINE_SPINLOCK(driver_lock);
 static DECLARE_BITMAP(dev_mask, TPM_NUM_DEVICES);
+#define TPM_CHIP_NUM_MASK       0x0000ffff
+#define TPM_CHIP_TYPE_SHIFT     16
 
 /*
  * Array with one entry per ordinal defining the maximum amount

@@ -366,8 +377,7 @@ EXPORT_SYMBOL_GPL(tpm_calc_ordinal_duration);
 /*
  * Internal kernel interface to transmit TPM commands
  */
-static ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf,
-			    size_t bufsiz)
+ssize_t tpm_transmit(struct tpm_chip *chip, char *buf, size_t bufsiz)
 {
 	ssize_t rc;
 	u32 count, ordinal;

@@ -425,6 +435,7 @@ out:
 	mutex_unlock(&chip->tpm_mutex);
 	return rc;
 }
+EXPORT_SYMBOL_GPL(tpm_transmit);
 
 #define TPM_DIGEST_SIZE 20
 #define TPM_ERROR_SIZE 10

@@ -717,6 +728,7 @@ ssize_t tpm_show_temp_deactivated(struct device * dev,
 }
 EXPORT_SYMBOL_GPL(tpm_show_temp_deactivated);
 
+#define READ_PCR_RESULT_SIZE 30
 static const u8 pcrread[] = {
 	0, 193,			/* TPM_TAG_RQU_COMMAND */
 	0, 0, 0, 14,		/* length */

@@ -772,6 +784,128 @@ out:
 }
 EXPORT_SYMBOL_GPL(tpm_show_pcrs);
 
+/*
+ * tpm_chip_lookup - return tpm_chip for given chip number and type
+ *
+ * Must be called with rcu_read_lock.
+ */
+static struct tpm_chip *tpm_chip_lookup(int chip_num, int chip_typ)
+{
+	struct tpm_chip *pos;

drivers/char/tpm/tpm.c +23 -0

+
+/**
+ * tpm_pcr_read - read a pcr value
+ * @chip_id: 	tpm chip identifier
+ * 		Upper 2 bytes: ANY, HW_ONLY or SW_ONLY
+ * 		Lower 2 bytes: tpm idx # or AN&
+ * @pcr_idx:	pcr idx to retrieve
+ * @res_buf: 	TPM_PCR value
+ * 		size of res_buf is 20 bytes (or NULL if you don't care)
+ *
+ * The TPM driver should be built-in, but for whatever reason it
+ * isn't, protect against the chip disappearing, by incrementing
+ * the module usage count.
+ */
+int tpm_pcr_read(u32 chip_id, int pcr_idx, u8 *res_buf)
+{
+	u8 data[READ_PCR_RESULT_SIZE];
+	int rc;
+	__be32 index;
+	int chip_num = chip_id & TPM_CHIP_NUM_MASK;
+	struct tpm_chip *chip;
+
+	rcu_read_lock();

drivers/char/tpm/tpm.c +50 -0

+	BUILD_BUG_ON(sizeof(pcrread) > READ_PCR_RESULT_SIZE);
+	memcpy(data, pcrread, sizeof(pcrread));
+	index = cpu_to_be32(pcr_idx);
+	memcpy(data + 10, &index, 4);
+	rc = tpm_transmit(chip, data, sizeof(data));
+	if (rc > 0)
+		rc = get_unaligned_be32((__be32 *) (data + 6));
+
+	if (rc == 0 && res_buf)
+		memcpy(res_buf, data + 10, TPM_DIGEST_SIZE);
+
+	module_put(chip->dev->driver->owner);
+	return rc;
+}
+EXPORT_SYMBOL_GPL(tpm_pcr_read);
+
+#define EXTEND_PCR_SIZE 34
+static const u8 pcrextend[] = {
+	0, 193,			/* TPM_TAG_RQU_COMMAND */
+	0, 0, 0, 34,		/* length */
+	0, 0, 0, 20,		/* TPM_ORD_Extend */
+	0, 0, 0, 0		/* PCR index */
+};
+
+/**
+ * tpm_pcr_extend - extend pcr value with hash
+ * @chip_id: 	tpm chip identifier
+ * 		Upper 2 bytes: ANY, HW_ONLY or SW_ONLY
+ * 		Lower 2 bytes: tpm idx # or AN&
+ * @pcr_idx:	pcr idx to extend
+ * @hash: 	hash value used to extend pcr value
+ *
+ * The TPM driver should be built-in, but for whatever reason it
+ * isn't, protect against the chip disappearing, by incrementing
+ * the module usage count.
+ */
+int tpm_pcr_extend(u32 chip_id, int pcr_idx, const u8 *hash)
+{
+	u8 data[EXTEND_PCR_SIZE];
+	int rc;
+	__be32 index;
+	int chip_num = chip_id & TPM_CHIP_NUM_MASK;
+	struct tpm_chip *chip;
+
+	rcu_read_lock();
+	chip = tpm_chip_lookup(chip_num, chip_id >> TPM_CHIP_TYPE_SHIFT);
+	if (chip == NULL) {
+		rcu_read_unlock();
+		return -ENODEV;
+	}

Quoted 87 lines · Mimi Zohar

This patch adds internal kernel support for:
- reading/extending a pcr value
- looking up the tpm_chip for a given chip number and type

Signed-off-by Mimi Zohar <[email protected]>

Signed-off-by Rajiv Andrade <[email protected]>

---

drivers/char/tpm/tpm.c +23 -3

index 9c47dc4..17d2849 100644
@@ -1,11 +1,12 @@
 /*
- * Copyright (C) 2004 IBM Corporation
+ * Copyright (C) 2004,2007,2008 IBM Corporation
  *
  * Authors:
  * Leendert van Doorn <[email protected]>
  * Dave Safford <[email protected]>
  * Reiner Sailer <[email protected]>
  * Kylene Hall <[email protected]>
+ * Debora Velarde <[email protected]>
  *
  * Maintained by: <[email protected]>
  *

@@ -28,6 +29,14 @@
 #include <linux/spinlock.h>
 #include <linux/smp_lock.h>
 
+#include <linux/mm.h>
+#include <linux/slab.h>
+#include <linux/string.h>
+#include <linux/crypto.h>
+#include <linux/fs.h>
+#include <linux/scatterlist.h>
+#include <linux/rcupdate.h>
+#include <asm/unaligned.h>
 #include "tpm.h"
 
 enum tpm_const {

@@ -50,6 +59,8 @@ enum tpm_duration {
 static LIST_HEAD(tpm_chip_list);
 static DEFINE_SPINLOCK(driver_lock);
 static DECLARE_BITMAP(dev_mask, TPM_NUM_DEVICES);
+#define TPM_CHIP_NUM_MASK       0x0000ffff
+#define TPM_CHIP_TYPE_SHIFT     16
 
 /*
  * Array with one entry per ordinal defining the maximum amount

@@ -366,8 +377,7 @@ EXPORT_SYMBOL_GPL(tpm_calc_ordinal_duration);
 /*
  * Internal kernel interface to transmit TPM commands
  */
-static ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf,
-			    size_t bufsiz)
+ssize_t tpm_transmit(struct tpm_chip *chip, char *buf, size_t bufsiz)
 {
 	ssize_t rc;
 	u32 count, ordinal;

@@ -425,6 +435,7 @@ out:
 	mutex_unlock(&chip->tpm_mutex);
 	return rc;
 }
+EXPORT_SYMBOL_GPL(tpm_transmit);
 
 #define TPM_DIGEST_SIZE 20
 #define TPM_ERROR_SIZE 10

@@ -717,6 +728,7 @@ ssize_t tpm_show_temp_deactivated(struct device * dev,
 }
 EXPORT_SYMBOL_GPL(tpm_show_temp_deactivated);
 
+#define READ_PCR_RESULT_SIZE 30
 static const u8 pcrread[] = {
 	0, 193,			/* TPM_TAG_RQU_COMMAND */
 	0, 0, 0, 14,		/* length */

@@ -772,6 +784,128 @@ out:
 }
 EXPORT_SYMBOL_GPL(tpm_show_pcrs);
 
+/*
+ * tpm_chip_lookup - return tpm_chip for given chip number and type
+ *
+ * Must be called with rcu_read_lock.
+ */
+static struct tpm_chip *tpm_chip_lookup(int chip_num, int chip_typ)
+{
+	struct tpm_chip *pos;

drivers/char/tpm/tpm.c out: · line 804

Quoted 10 lines

drivers/char/tpm/tpm.c +10 -0

+	int rc;
+
+	list_for_each_entry_rcu(pos, &tpm_chip_list, list) {
+		rc = (chip_num == TPM_ANY_NUM || pos->dev_num == chip_num)
+		    && (chip_typ == TPM_ANY_TYPE);
+		if (rc)
+			return pos;
+	}
+	return NULL;
+}

If you have to respin these patches could you consider simplifying that loop? I find that really hard to read. I think it's much easier to read if written out something like this:

	/* Dunno why they *must* specify TPM_ANY_TYPE, but they do */
	if (chip_typ != TPM_ANY_TYPE)
		continue;

	if (chip_num == TPM_ANY_NUM)
		return pos;
	if (pos->dev_num == chip_num)
		return pos;

Quoted 28 lines

drivers/char/tpm/tpm.c +28 -0

+
+/**
+ * tpm_pcr_read - read a pcr value
+ * @chip_id: 	tpm chip identifier
+ * 		Upper 2 bytes: ANY, HW_ONLY or SW_ONLY
+ * 		Lower 2 bytes: tpm idx # or AN&
+ * @pcr_idx:	pcr idx to retrieve
+ * @res_buf: 	TPM_PCR value
+ * 		size of res_buf is 20 bytes (or NULL if you don't care)
+ *
+ * The TPM driver should be built-in, but for whatever reason it
+ * isn't, protect against the chip disappearing, by incrementing
+ * the module usage count.
+ */
+int tpm_pcr_read(u32 chip_id, int pcr_idx, u8 *res_buf)
+{
+	u8 data[READ_PCR_RESULT_SIZE];
+	int rc;
+	__be32 index;
+	int chip_num = chip_id & TPM_CHIP_NUM_MASK;
+	struct tpm_chip *chip;
+
+	rcu_read_lock();
+	chip = tpm_chip_lookup(chip_num, chip_id >> TPM_CHIP_TYPE_SHIFT);
+	if (chip == NULL) {
+		rcu_read_unlock();
+		return -ENODEV;
+	}

drivers/char/tpm/tpm.c out: · line 837

Quoted 5 lines

drivers/char/tpm/tpm.c +5 -0

+	if (!try_module_get(chip->dev->driver->owner)) {
+		rcu_read_unlock();
+		return -ENODEV;
+	}
+	rcu_read_unlock();

This little bit of lookup, check for NULL, and try_module_get() looks cut-n-pasted in the next two functions. Should be consolidated.

Also, if you need to shift down the chip_id every time anyway, why not just do it inside the lookup function?

Quoted 55 lines

drivers/char/tpm/tpm.c +55 -0

+	BUILD_BUG_ON(sizeof(pcrread) > READ_PCR_RESULT_SIZE);
+	memcpy(data, pcrread, sizeof(pcrread));
+	index = cpu_to_be32(pcr_idx);
+	memcpy(data + 10, &index, 4);
+	rc = tpm_transmit(chip, data, sizeof(data));
+	if (rc > 0)
+		rc = get_unaligned_be32((__be32 *) (data + 6));
+
+	if (rc == 0 && res_buf)
+		memcpy(res_buf, data + 10, TPM_DIGEST_SIZE);
+
+	module_put(chip->dev->driver->owner);
+	return rc;
+}
+EXPORT_SYMBOL_GPL(tpm_pcr_read);
+
+#define EXTEND_PCR_SIZE 34
+static const u8 pcrextend[] = {
+	0, 193,			/* TPM_TAG_RQU_COMMAND */
+	0, 0, 0, 34,		/* length */
+	0, 0, 0, 20,		/* TPM_ORD_Extend */
+	0, 0, 0, 0		/* PCR index */
+};
+
+/**
+ * tpm_pcr_extend - extend pcr value with hash
+ * @chip_id: 	tpm chip identifier
+ * 		Upper 2 bytes: ANY, HW_ONLY or SW_ONLY
+ * 		Lower 2 bytes: tpm idx # or AN&
+ * @pcr_idx:	pcr idx to extend
+ * @hash: 	hash value used to extend pcr value
+ *
+ * The TPM driver should be built-in, but for whatever reason it
+ * isn't, protect against the chip disappearing, by incrementing
+ * the module usage count.
+ */
+int tpm_pcr_extend(u32 chip_id, int pcr_idx, const u8 *hash)
+{
+	u8 data[EXTEND_PCR_SIZE];
+	int rc;
+	__be32 index;
+	int chip_num = chip_id & TPM_CHIP_NUM_MASK;
+	struct tpm_chip *chip;
+
+	rcu_read_lock();
+	chip = tpm_chip_lookup(chip_num, chip_id >> TPM_CHIP_TYPE_SHIFT);
+	if (chip == NULL) {
+		rcu_read_unlock();
+		return -ENODEV;
+	}
+	if (!try_module_get(chip->dev->driver->owner)) {
+		rcu_read_unlock();
+		return -ENODEV;
+	}
+	rcu_read_unlock();

drivers/char/tpm/tpm.c out: · line 898

Quoted 5 lines

drivers/char/tpm/tpm.c +5 -0

+
+	BUILD_BUG_ON(sizeof(pcrextend) > EXTEND_PCR_SIZE);
+	memcpy(data, pcrextend, sizeof(pcrextend));
+	index = cpu_to_be32(pcr_idx);
+	memcpy(data + 10, &index, 4);

This bit of code looks duplicated too. I really wish these 10's and 14's weren't magic numbers, especially since they're used twice.

drivers/char/tpm/tpm.c out: · line 906

Quoted 8 lines

drivers/char/tpm/tpm.c +8 -0

+	memcpy(data + 14, hash, TPM_DIGEST_SIZE);
+	rc = tpm_transmit(chip, data, sizeof(data));
+	if (rc > 0)
+		rc = get_unaligned_be32((__be32 *) (data + 6));
+
+	module_put(chip->dev->driver->owner);
+	return rc;
+}

Looking at this, I can't help but think a couple of nicely laid out structs with a union or two could make this all look nicer. For instance, is the return code from the tpm_transmit() function always returned in the 6th byte?

It looks to me like there is a TPM_RET_CODE_IDX in drivers/char/tpm/tpm.c. Why on earth isn't that being used? That also makes me question all these other magic numbers.

Why not just integrate that rc tinkering into tpm_transmit(), or a variant of it. There appear to be at least three or four other users that could benefit from such a function. If you decide to mess with it further than just exporting it, please break that out into a separate patch, btw.

drivers/char/tpm/tpm.c out: · line 908

Quoted 3 lines

drivers/char/tpm/tpm.c +3 -0

+enum tpm_chip_num {
+	TPM_ANY_NUM = 0xFFFF,
+};

Why bother even checking this sucker if there's only one value?

drivers/char/tpm/tpm.c out:

Quoted 6 lines

drivers/char/tpm/tpm.c +6 -0

+#if defined(CONFIG_TCG_TPM) || defined(CONFIG_TCG_TPM_MODULE)
+
+extern int tpm_pcr_read(u32 chip_id, int pcr_idx, u8 *res_buf);
+extern int tpm_pcr_extend(u32 chip_id, int pcr_idx, const u8 *hash);
+#endif
+#endif

The " || defined(CONFIG_TCG_TPM_MODULE)" doesn't do anything. CONFIG_TCG_TPM is still true even when CONFIG_TCG_TPM_MODULE.

I also think so many authors on the header is a bit excessive. 5 authors for 2 enums and 2 function declarations. :)

-- Dave

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Quoted 28 lines

Quoted 28 lines

drivers/char/tpm/tpm.c +28 -0

+
+/**
+ * tpm_pcr_read - read a pcr value
+ * @chip_id: 	tpm chip identifier
+ * 		Upper 2 bytes: ANY, HW_ONLY or SW_ONLY
+ * 		Lower 2 bytes: tpm idx # or AN&
+ * @pcr_idx:	pcr idx to retrieve
+ * @res_buf: 	TPM_PCR value
+ * 		size of res_buf is 20 bytes (or NULL if you don't care)
+ *
+ * The TPM driver should be built-in, but for whatever reason it
+ * isn't, protect against the chip disappearing, by incrementing
+ * the module usage count.
+ */
+int tpm_pcr_read(u32 chip_id, int pcr_idx, u8 *res_buf)
+{
+	u8 data[READ_PCR_RESULT_SIZE];
+	int rc;
+	__be32 index;
+	int chip_num = chip_id & TPM_CHIP_NUM_MASK;
+	struct tpm_chip *chip;
+
+	rcu_read_lock();
+	chip = tpm_chip_lookup(chip_num, chip_id >> TPM_CHIP_TYPE_SHIFT);
+	if (chip == NULL) {
+		rcu_read_unlock();
+		return -ENODEV;
+	}

Quoted 9 lines

drivers/char/tpm/tpm.c out: · line 837

Quoted 5 lines

drivers/char/tpm/tpm.c +5 -0

+	if (!try_module_get(chip->dev->driver->owner)) {
+		rcu_read_unlock();
+		return -ENODEV;
+	}
+	rcu_read_unlock();

This little bit of lookup, check for NULL, and try_module_get() looks cut-n-pasted in the next two functions. Should be consolidated.

Same here.

Quoted 2 lines

Also, if you need to shift down the chip_id every time anyway, why not just do it inside the lookup function?

tpm_chip_lookup() only needs the chip type, not the entire chip_id, so its usage is probably clearer if written this way.

This patch adds internal kernel support for:
- reading/extending a pcr value
- looking up the tpm_chip for a given chip number and type

Signed-off-by Mimi Zohar <[email protected]>

Signed-off-by Rajiv Andrade <[email protected]>

---

drivers/char/tpm/tpm.c +15 -3

index 9c47dc4..17d2849 100644
@@ -1,11 +1,12 @@
 /*
- * Copyright (C) 2004 IBM Corporation
+ * Copyright (C) 2004,2007,2008 IBM Corporation
  *
  * Authors:
  * Leendert van Doorn <[email protected]>
  * Dave Safford <[email protected]>
  * Reiner Sailer <[email protected]>
  * Kylene Hall <[email protected]>
+ * Debora Velarde <[email protected]>
  *
  * Maintained by: <[email protected]>
  *

@@ -28,6 +29,14 @@
 #include <linux/spinlock.h>
 #include <linux/smp_lock.h>
 
+#include <linux/mm.h>
+#include <linux/slab.h>
+#include <linux/string.h>
+#include <linux/crypto.h>
+#include <linux/fs.h>
+#include <linux/scatterlist.h>
+#include <linux/rcupdate.h>
+#include <asm/unaligned.h>
 #include "tpm.h"
 
 enum tpm_const {

@@ -50,6 +59,8 @@ enum tpm_duration {
 static LIST_HEAD(tpm_chip_list);
 static DEFINE_SPINLOCK(driver_lock);
 static DECLARE_BITMAP(dev_mask, TPM_NUM_DEVICES);
+#define TPM_CHIP_NUM_MASK       0x0000ffff
+#define TPM_CHIP_TYPE_SHIFT     16
 
 /*
  * Array with one entry per ordinal defining the maximum amount

@@ -366,8 +377,7 @@ EXPORT_SYMBOL_GPL(tpm_calc_ordinal_duration);
 /*
  * Internal kernel interface to transmit TPM commands
  */
-static ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf,
-			    size_t bufsiz)
+ssize_t tpm_transmit(struct tpm_chip *chip, char *buf, size_t bufsiz)
 {
 	ssize_t rc;
 	u32 count, ordinal;

@@ -425,6 +435,7 @@ out:
 	mutex_unlock(&chip->tpm_mutex);
 	return rc;
 }
+EXPORT_SYMBOL_GPL(tpm_transmit);
 
 #define TPM_DIGEST_SIZE 20
 #define TPM_ERROR_SIZE 10

@@ -717,6 +728,7 @@ ssize_t tpm_show_temp_deactivated(struct device * dev,
 }
 EXPORT_SYMBOL_GPL(tpm_show_temp_deactivated);
 
+#define READ_PCR_RESULT_SIZE 30

drivers/char/tpm/tpm.c +18 -0

+#ifndef __LINUX_TPM_H__
+#define __LINUX_TPM_H__
+
+#define PCI_DEVICE_ID_AMD_8111_LPC    0x7468
+
+/*
+ * Chip type is one of these values in the upper two bytes of chip_id
+ */
+enum tpm_chip_type {
+	TPM_HW_TYPE = 0x0,
+	TPM_SW_TYPE = 0x1,
+	TPM_ANY_TYPE = 0xFFFF,
+};
+
+/*
+ * Chip num is this value or a valid tpm idx in lower two bytes of chip_id
+ */
+enum tpm_chip_num {

This patch adds internal kernel support for:
- reading/extending a pcr value
- looking up the tpm_chip for a given chip number and type

Signed-off-by Mimi Zohar <[email protected]>

Signed-off-by Rajiv Andrade <[email protected]>

fs/inode.c +7 -6

@@ -143,12 +144,13 @@ static struct inode *alloc_inode(struct super_block *sb)
 		inode->i_cdev = NULL;
 		inode->i_rdev = 0;
 		inode->dirtied_when = 0;
-		if (security_inode_alloc(inode)) {
-			if (inode->i_sb->s_op->destroy_inode)
-				inode->i_sb->s_op->destroy_inode(inode);
-			else
-				kmem_cache_free(inode_cachep, (inode));
-			return NULL;
+		if (security_inode_alloc(inode))
+			goto out_free_inode;
+
+		/* allocate and initialize an i_integrity */
+		if (integrity_inode_alloc(inode)) {
+			security_inode_free(inode);
+			goto out_free_inode;
 		}
 
 		spin_lock_init(&inode->i_lock);

@@ -185,12 +187,20 @@ static struct inode *alloc_inode(struct super_block *sb)
 		inode->i_mapping = mapping;

security/integrity/integrity.c new file +24 -0

+/**
+ * integrity_register_template - registers an integrity template with the kernel
+ * @template_name: a pointer to a string containing the template name.
+ * @template_ops: a pointer to the template functions
+ *
+ * Register a set of functions to collect, appraise, store, and display
+ * a template measurement, and a means to decide whether to do them.
+ * Unlike integrity modules, any number of templates may be registered.
+ *
+ * Returns 0 on success, an error code on failure.
+ */
+int integrity_register_template(const char *template_name,
+				const struct template_operations *template_ops)
+{
+	int template_len;
+	struct template_list_entry *entry;
+
+	template_len = strlen(template_name);
+	if (template_len > TEMPLATE_NAME_LEN_MAX)
+		return -EINVAL;
+
+	entry = kzalloc(sizeof(*entry), GFP_KERNEL);
+	if (!entry)
+		return -ENOMEM;

security/integrity/integrity.c new file +9 -0

+int integrity_unregister_template(const char *template_name)
+{
+	struct template_list_entry *entry;
+
+	mutex_lock(&integrity_templates_mutex);
+	list_for_each_entry(entry, &integrity_templates, template) {
+		if (strncmp(entry->template_name, template_name,
+			    strlen(entry->template_name)) == 0) {
+			list_del_rcu(&entry->template);

security/integrity/integrity.c new file +15 -0

+/**
+ * integrity_find_get_template - search the integrity_templates list
+ * @template_name: a pointer to a string containing the template name.
+ *
+ * Returns a pointer to an entry in the template list on success, NULL
+ * on failure.
+ */
+struct template_list_entry *integrity_find_get_template(const char
+							*template_name)
+{
+	struct template_list_entry *entry, *template_entry = NULL;
+
+	rcu_read_lock();
+	list_for_each_entry_rcu(entry, &integrity_templates, template) {
+		if (strncmp(entry->template_name, template_name,

security/integrity/integrity.c new file +4 -0

+int integrity_collect_measurement(const char *template_name, void *data)
+{
+	struct template_list_entry *template_entry;
+	int rc = -EINVAL;

include/linux/integrity.h line 172

Quoted 2 lines · Mimi Zohar

include/linux/integrity.h new file +2 -0

+#endif
+#endif

Personally, I love to see comments on these suckers after a long header file. My memory sucks.

security/integrity/integrity.c line 41

Quoted 7 lines

security/integrity/integrity.c new file +7 -0

+int register_integrity(const struct integrity_operations *ops)
+{
+	if (integrity_ops != NULL)
+		return -EAGAIN;
+	integrity_ops = ops;
+	return 0;
+}

Is there some locking to keep this from racing and two integrity modules both thinking they succeeded? Does it matter?

Quoted 28 lines

security/integrity/integrity.c new file +28 -0

+/**
+ * integrity_register_template - registers an integrity template with the kernel
+ * @template_name: a pointer to a string containing the template name.
+ * @template_ops: a pointer to the template functions
+ *
+ * Register a set of functions to collect, appraise, store, and display
+ * a template measurement, and a means to decide whether to do them.
+ * Unlike integrity modules, any number of templates may be registered.
+ *
+ * Returns 0 on success, an error code on failure.
+ */
+int integrity_register_template(const char *template_name,
+				const struct template_operations *template_ops)
+{
+	int template_len;
+	struct template_list_entry *entry;
+
+	template_len = strlen(template_name);
+	if (template_len > TEMPLATE_NAME_LEN_MAX)
+		return -EINVAL;
+
+	entry = kzalloc(sizeof(*entry), GFP_KERNEL);
+	if (!entry)
+		return -ENOMEM;
+	INIT_LIST_HEAD(&entry->template);
+
+	kref_set(&entry->refcount, 1);
+	strcpy(entry->template_name, template_name);

security/integrity/integrity.c line 93

Quoted 6 lines

security/integrity/integrity.c new file +6 -0

+	entry->template_ops = template_ops;
+
+	mutex_lock(&integrity_templates_mutex);
+	list_add_rcu(&entry->template, &integrity_templates);
+	mutex_unlock(&integrity_templates_mutex);
+	synchronize_rcu();

What's the synchronize_rcu() for here?

Quoted 14 lines

security/integrity/integrity.c new file +14 -0

+int integrity_unregister_template(const char *template_name)
+{
+	struct template_list_entry *entry;
+
+	mutex_lock(&integrity_templates_mutex);
+	list_for_each_entry(entry, &integrity_templates, template) {
+		if (strncmp(entry->template_name, template_name,
+			    strlen(entry->template_name)) == 0) {
+			list_del_rcu(&entry->template);
+			mutex_unlock(&integrity_templates_mutex);
+			synchronize_rcu();
+			kref_put(&entry->refcount, template_release);
+			return 0;
+		}

security/integrity/integrity.c line 131

Quoted 5 lines

security/integrity/integrity.c new file +5 -0

+	}
+	mutex_unlock(&integrity_templates_mutex);
+	return -EINVAL;
+}
+EXPORT_SYMBOL_GPL(integrity_unregister_template);

Is this frequently called? If so, it might be better to use call_rcu().

Quoted 20 lines

security/integrity/integrity.c new file +20 -0

+/**
+ * integrity_find_get_template - search the integrity_templates list
+ * @template_name: a pointer to a string containing the template name.
+ *
+ * Returns a pointer to an entry in the template list on success, NULL
+ * on failure.
+ */
+struct template_list_entry *integrity_find_get_template(const char
+							*template_name)
+{
+	struct template_list_entry *entry, *template_entry = NULL;
+
+	rcu_read_lock();
+	list_for_each_entry_rcu(entry, &integrity_templates, template) {
+		if (strncmp(entry->template_name, template_name,
+			    strlen(entry->template_name)) == 0) {
+			template_entry = entry;
+			break;
+		}
+	}

security/integrity/integrity.c line 157

Quoted 5 lines

security/integrity/integrity.c new file +5 -0

+	if (template_entry)
+		kref_get(&template_entry->refcount);
+	rcu_read_unlock();
+	return template_entry;
+}

Is there a reason not to do the kref_get() inside the loop? Would save a line of code.

Quoted 4 lines

security/integrity/integrity.c new file +4 -0

+int integrity_collect_measurement(const char *template_name, void *data)
+{
+	struct template_list_entry *template_entry;
+	int rc = -EINVAL;

security/integrity/integrity.c line 183

Quoted 10 lines

security/integrity/integrity.c new file +10 -0

+
+	template_entry = integrity_find_get_template(template_name);
+	if (template_entry) {
+		rc = template_entry->template_ops->collect_measurement(data);
+		kref_put(&template_entry->refcount, template_release);
+	}
+	return rc;
+}
+EXPORT_SYMBOL_GPL(integrity_collect_measurement);
+

It's kinda a shame to see 5 or 6 functions which are such carbon copies of each other. Could you do one of these, and just pass in the ops function as well as 'data'?

You would have one of these:

security/integrity/integrity.c new file +13 -0

+int integrity_generic_template(const char *template_name,
+			      void (*func)(void *data), void *data)
+{
+       struct template_list_entry *template_entry;
+       int rc = -EINVAL;
+
+       template_entry = integrity_find_get_template(template_name);
+       if (template_entry) {
+               rc = func(data);
+               kref_put(&template_entry->refcount, template_release);
+       }
+       return rc;
+}

And each measurement function could be something silly like:

int integrity_collect_measurement(const char *template_name, void *data)

{
	return integrity_generic_template(template_name,
		 template_entry->template_ops->collect_measurement,
		 data);
}

-- Dave

Quoted 10 lines

security/integrity/integrity.c line 41

Quoted 7 lines

security/integrity/integrity.c new file +7 -0

+int register_integrity(const struct integrity_operations *ops)
+{
+	if (integrity_ops != NULL)
+		return -EAGAIN;
+	integrity_ops = ops;
+	return 0;
+}

Is there some locking to keep this from racing and two integrity modules both thinking they succeeded? Does it matter?

Wouldn't this be a Kconfig issue.

Quoted 3 lines

Quoted 3 lines

security/integrity/integrity.c new file +3 -0

+int integrity_register_template(const char *template_name,
+				const struct template_operations *template_ops)
+{

Quoted 6 lines

security/integrity/integrity.c line 93

Quoted 4 lines

security/integrity/integrity.c new file +4 -0

+	mutex_lock(&integrity_templates_mutex);
+	list_add_rcu(&entry->template, &integrity_templates);
+	mutex_unlock(&integrity_templates_mutex);
+	synchronize_rcu();

What's the synchronize_rcu() for here?

good question.

Quoted 14 lines

Quoted 14 lines

security/integrity/integrity.c new file +14 -0

+int integrity_unregister_template(const char *template_name)
+{
+	struct template_list_entry *entry;
+
+	mutex_lock(&integrity_templates_mutex);
+	list_for_each_entry(entry, &integrity_templates, template) {
+		if (strncmp(entry->template_name, template_name,
+			    strlen(entry->template_name)) == 0) {
+			list_del_rcu(&entry->template);
+			mutex_unlock(&integrity_templates_mutex);
+			synchronize_rcu();
+			kref_put(&entry->refcount, template_release);
+			return 0;
+		}

Quoted 8 lines

security/integrity/integrity.c line 131

Quoted 5 lines

security/integrity/integrity.c new file +5 -0

+	}
+	mutex_unlock(&integrity_templates_mutex);
+	return -EINVAL;
+}
+EXPORT_SYMBOL_GPL(integrity_unregister_template);

Is this frequently called? If so, it might be better to use call_rcu().

I don't expect that Templates would be added/removed frequently, but I could be wrong. Only time will tell.

+/*
+ * Integrity API calls:
+ *
+ * @collect_measurement:
+ * Collect template specific measurement data.
+ * @data contains template specific data used for collecting the
+ * measurement.
+ * Return 0 if operation was successful.
+ *
+ * @appraise_measurement:
+ * Appraise the integrity of the template specific measurement data.
+ * @data contains template specific data used for appraising the
+ * measurement.
+ * Return 0 if operation was successful.
+ *
+ * @store_measurement:
+ * Store the template specific data.
+ * @data contains template specific data used for storing the
+ * measurement.
+ *
+ * @store_template:
+ * Store the entry containing the template specific data.
+ * @data contains template name, data length, and data.
+ *
+ * @must_measure:
+ * Measurement decision based on an integrity policy.

+ * @data contains template specific data used for making policy
+ * decision.
+ * Return 0 if operation was successful.
+ *
+ * @display_template:
+ * Display template specific data.
+ *
+ */

Quoted 26 lines · Mimi Zohar

+/*
+ * Integrity API calls:
+ *
+ * @collect_measurement:
+ * Collect template specific measurement data.
+ * @data contains template specific data used for collecting the
+ * measurement.
+ * Return 0 if operation was successful.
+ *
+ * @appraise_measurement:
+ * Appraise the integrity of the template specific measurement data.
+ * @data contains template specific data used for appraising the
+ * measurement.
+ * Return 0 if operation was successful.
+ *
+ * @store_measurement:
+ * Store the template specific data.
+ * @data contains template specific data used for storing the
+ * measurement.
+ *
+ * @store_template:
+ * Store the entry containing the template specific data.
+ * @data contains template name, data length, and data.
+ *
+ * @must_measure:
+ * Measurement decision based on an integrity policy.

Quoted 8 lines

+ * @data contains template specific data used for making policy
+ * decision.
+ * Return 0 if operation was successful.
+ *
+ * @display_template:
+ * Display template specific data.
+ *
+ */

Can you explain what all this template stuff is about? The only method of these ever called is display_template, and that seems to be better implented directly as a securityfs file, without the indirection.

IMA originally supported measurement and attestation only for file data. Templates provide an abstraction to add different types of integrity messages to the TPM based measurement list. Each type of integrity code knows how to format/display its own messages, while the TPM measurement list code remains generic.

Quoted 5 lines · Mimi Zohar

IMA originally supported measurement and attestation only for file data. Templates provide an abstraction to add different types of integrity messages to the TPM based measurement list. Each type of integrity code knows how to format/display its own messages, while the TPM measurement list code remains generic.

I have a bit of a problem parsing the above, and it certainly doesn't look like a justification for keeping all that unused code around.

Quoted 2 lines

I have a bit of a problem parsing the above, and it certainly doesn't look like a justification for keeping all that unused code around.

The purpose of LIM is to provide an integrity infrastructure to support different types of integrity data. IMA implements both the LIM API for it's own internal use, and exports it for others to call.

As Dave Safford pointed out in http://lkml.org/lkml/2008/11/17/362,
there are other projects that want to add differently structured measurements to the TPM measurement list. The template abstraction is critical to allowing these differently formatted messages to be added to the list.

Mimi

Quoted 7 lines · Mimi Zohar

Quoted 2 lines

I have a bit of a problem parsing the above, and it certainly doesn't look like a justification for keeping all that unused code around.

The purpose of LIM is to provide an integrity infrastructure to support different types of integrity data. IMA implements both the LIM API for it's own internal use, and exports it for others to call.

Quoted 5 lines

As Dave Safford pointed out in http://lkml.org/lkml/2008/11/17/362,
there are other projects that want to add differently structured measurements to the TPM measurement list. The template abstraction is critical to allowing these differently formatted messages to be added to the list.

I think we're talking past each other.

In integrity.h there are two operation vectors defines:

- struct integrity_operations delcares the operations called from the VFS. This one is actually used. While I don't agree to Dave's argument, because we don't put bloat in just because people might eventually some day use it when they are in the right mood and the sun shines, thisn't isn't the one I'm talking about in this thread.
- struct template_operations on the others is not only really badly named for appearing in a global header but also not used in a meaningfull way. There is one single instace of it, ima_template_ops, and while there are five helpers added in the

second patch that use it (integrity_collect_measurement, integrity_appraise_measurement, integrity_store_measurement, integrity_store_template, integrity_must_measure) none of them is used at all during the patch series. There are two direct uses of these template added in the third path, to implement the show operations for the "binary_runtime_measurements" and "ascii_runtime_measurements" files ins securityfs, but given that those are inside ima there no reason for the indirection at all.

In integrity.h there are two operation vectors defines:

- struct integrity_operations delcares the operations called from the VFS. This one is actually used. While I don't agree to Dave's argument, because we don't put bloat in just because people might eventually some day use it when they are in the right mood and the sun shines, thisn't isn't the one I'm talking about in this thread.

- struct template_operations on the others is not only really badly named for appearing in a global header but also not used in a meaningfull way. There is one single instace of it, ima_template_ops, and while there are five helpers added in the

second patch that use it (integrity_collect_measurement, integrity_appraise_measurement, integrity_store_measurement, integrity_store_template, integrity_must_measure) none of them is used at all during the patch series. There are two direct uses of these template added in the third path, to implement the show operations for the "binary_runtime_measurements" and "ascii_runtime_measurements" files ins securityfs, but given that those are inside ima there no reason for the indirection at all.

Quoted 5 lines

- struct integrity_operations delcares the operations called from the VFS. This one is actually used. While I don't agree to Dave's argument, because we don't put bloat in just because people might eventually some day use it when they are in the right mood and the sun shines, thisn't isn't the one I'm talking about in this thread.

These hooks were for alternate integrity modules, and since no one else has defended them, we have to agree that they should be replaced with direct calls.

Quoted 3 lines · david safford

These hooks were for alternate integrity modules, and since no one else has defended them, we have to agree that they should be replaced with direct calls.

If you know of other modules which are planned to be ported to this framework, merged upstream and supported, then this would be similar to the situation when LSM was initially developed.

You've previously mentioned some active projects here:
http://lkml.org/lkml/2008/11/17/362

Are there any definite commitments to push these upstream when the integrity framework is merged?

Quoted 10 lines · James Morris

Quoted 3 lines · david safford

These hooks were for alternate integrity modules, and since no one else has defended them, we have to agree that they should be replaced with direct calls.

If you know of other modules which are planned to be ported to this framework, merged upstream and supported, then this would be similar to the situation when LSM was initially developed.

Quoted 5 lines

You've previously mentioned some active projects here:
http://lkml.org/lkml/2008/11/17/362

Are there any definite commitments to push these upstream when the integrity framework is merged?

All of the projects listed in that posting were ones depending on IMA, with no requirements for alternate modules. I do hope that there will be other integrity modules in addition to the TPM oriented IMA, and I do know of several research projects in this space, but I don't know if/when any of these are planning on submission. If others are submitted, it would certainly be simple to add the hooks back in.

Quoted 17 lines · david safford

Quoted 10 lines · James Morris

Quoted 3 lines · david safford

These hooks were for alternate integrity modules, and since no one else has defended them, we have to agree that they should be replaced with direct calls.

If you know of other modules which are planned to be ported to this framework, merged upstream and supported, then this would be similar to the situation when LSM was initially developed.

Quoted 5 lines

You've previously mentioned some active projects here:
http://lkml.org/lkml/2008/11/17/362

Are there any definite commitments to push these upstream when the integrity framework is merged?

Quoted 6 lines

All of the projects listed in that posting were ones depending on IMA, with no requirements for alternate modules. I do hope that there will be other integrity modules in addition to the TPM oriented IMA, and I do know of several research projects in this space, but I don't know if/when any of these are planning on submission. If others are submitted, it would certainly be simple to add the hooks back in.

Too bad the main trusted knoppix site appears to be dead. Was it actually making use of the templating api?

-serge

index 0000000..6c6fcd9

security/integrity/ima/Kconfig new file +14 -0

@@ -0,0 +1,34 @@
+#
+# IBM Integrity Measurement Architecture
+#
+config IMA
+	bool "Integrity Measurement Architecture(IMA)"
+	depends on INTEGRITY
+	depends on ACPI
+	select SECURITYFS
+	select CRYPTO
+	select CRYPTO_HMAC
+	select CRYPTO_MD5
+	select CRYPTO_SHA1
+	select TCG_TPM
+	select TCG_TIS

security/integrity/ima/Kconfig new file +11 -0

+	if (!iint) {
+		int rc;
+
+		/* Most insertions are done at inode_alloc,
+		 * except those allocated before late_initcall.
+		 * Can't initialize at security_initcall,
+		 * got to wait at least until proc_init.
+		 */
+		rc = ima_iint_insert(mdata->inode);
+		if (rc < 0)
+			return rc;

security/integrity/ima/ima_init.c new file +36 -0

new file mode 100644
index 0000000..374b368
@@ -0,0 +1,90 @@
+/*
+ * Copyright (C) 2005,2006,2007,2008 IBM Corporation
+ *
+ * Authors:
+ * Reiner Sailer      <[email protected]>
+ * Leendert van Doorn <[email protected]>
+ * Mimi Zohar         <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation, version 2 of the
+ * License.
+ *
+ * File: ima_init.c
+ *             initialization and cleanup functions
+ */
+#include <linux/module.h>
+#include <linux/scatterlist.h>
+#include "ima.h"
+
+/* name for boot aggregate entry */
+static char *boot_aggregate_name = "boot_aggregate";
+int ima_used_chip;
+
+static void ima_add_boot_aggregate(void)
+{
+	struct ima_inode_measure_entry measure_entry;
+	struct ima_store_template_data template = {
+		.name = "ima",
+		.len = sizeof(measure_entry),
+		.data = (char *)&measure_entry,
+	};
+	int namelen, result;
+
+	memset(&measure_entry, 0, sizeof measure_entry);
+	namelen = strlen(boot_aggregate_name);

security/integrity/ima/ima_init.c new file +16 -0

+			ima_pcrread(i, pcr_i, sizeof(pcr_i));
+			/* now accumulate with current aggregate */
+			sg_init_one(&sg, (u8 *) pcr_i, IMA_DIGEST_SIZE);
+			crypto_hash_update(&desc, &sg, IMA_DIGEST_SIZE);
+		}
+		crypto_hash_final(&desc, measure_entry.digest);
+		crypto_free_hash(tfm);
+	} else
+		template.violation = 1;
+
+	/* now add measurement; if TPM bypassed, we have a ff..ff entry */
+	result = ima_store_template(&template);
+}
+
+int ima_init(void)
+{

security/integrity/ima/ima_init.c new file +15 -0

+static void ima_file_free(struct file *file)
+{
+	struct inode *inode = file->f_dentry->d_inode;
+	struct ima_iint_cache *iint;
+
+	if (S_ISDIR(inode->i_mode))
+		return;
+	if ((file->f_mode & FMODE_WRITE) &&
+	    (atomic_read(&inode->i_writecount) == 1)) {
+		rcu_read_lock();
+		iint = ima_iint_lookup(inode);
+		if (!iint) {
+			rcu_read_unlock();
+			return;
+		}

security/integrity/ima/ima_init.c new file +30 -0

+	/* measure executables later */
+	if ((mdata.mask & (MAY_WRITE | MAY_READ)) == MAY_READ) {
+		int rc;
+
+		rc = ima_must_measure(&mdata);
+		if (!rc || rc == -EEXIST) {
+			/* Invalidate PCR, if a measured file is
+			 * already open for write.
+			 */
+			if (atomic_read(&(mdata.inode->i_writecount)) > 0)
+				ima_add_violation(mdata.inode,
+						  path->dentry->d_name.name,
+						  "invalid_pcr",
+						  "open_writers");
+		}
+		if (!rc) {
+			struct dentry *de = dget(path->dentry);
+			struct vfsmount *mnt = mntget(path->mnt);
+			struct file *file;
+
+			file = dentry_open(de, mnt, O_RDONLY);
+			if (IS_ERR(file)) {
+				ima_info("%s dentry_open failed\n",
+					 de->d_name.name);
+			} else if (file) {
+				rc = ima_collect_measurement(file);
+				if (!rc) {
+					struct ima_store_data sdata;
+
+					sdata.file = file;

If you're looking for another way to split your patch out, you could just stick use inode->integrity for the first patch, then introduce your radix-tree in a subsequent patch. It would be functionally fine, and more clearly separate out the ideas.

security/integrity/ima/ima_init.c line 84

Quoted 3 lines

security/integrity/ima/ima_init.c new file +3 -0

+	if (!ima_used_chip)
+		ima_info("No TPM chip found(rc = %d), activating TPM-bypass!\n",
+			 rc);

For the record, I think this is the kind of place that it's worth going over 80 chars.

Quoted 14 lines

security/integrity/ima/ima_main.c new file +14 -0

+	if ((file->f_mode & FMODE_WRITE) &&
+	    (atomic_read(&inode->i_writecount) == 1)) {
+		rcu_read_lock();
+		iint = ima_iint_lookup(inode);
+		if (!iint) {
+			rcu_read_unlock();
+			return;
+		}
+		kref_get(&iint->refcount);
+		rcu_read_unlock();
+
+		mutex_lock(&iint->mutex);
+		if (iint->version != inode->i_version)
+			iint->flags &= ~IMA_MEASURED;

security/integrity/ima/ima_api.c line 245

Quoted 4 lines

security/integrity/ima/ima_api.c new file +4 -0

+		mutex_unlock(&iint->mutex);
+		kref_put(&iint->refcount, iint_free);
+	}
+}

I'm also wondering if there's a way to wrap up the mutex operations since this seems to be done the exact same way every time. Dunno, maybe it is too much locking obfuscation for just a few lines saved.

security/integrity/ima/ima_main.c line 103

Quoted 3 lines

security/integrity/ima/ima_main.c new file +3 -0

+
+	/* Invalidate PCR, if a measured file is already open for read */
+	if ((mdata.mask & (MAY_WRITE | MAY_READ)) == MAY_WRITE) {

It would warm my heart to see something like this:

int mdata_is_write_only(struct ima_measure_data *mdata)

{
	if (mdata.mask & MAY_READ)
		return 0;
	return mdata.mask & MAY_WRITE;

I have memories of talking about this bit. I was confused and you explained it to me, but it still isn't explained in the code. :( Again, I'm not convinced that this works. Can the code convince me, or should I go digging in my inbox?

Please take a really, really hard look at these patches, all 3000 lines of it, and try to rework them. Find common bits of code that are duplicated or copy-n-pasted around. Find functions that have gotten too long and break them up. I bet you can knock a couple hundred lines of code off this sucker, easy.

Quoted 4 lines · Dave Hansen

If you're looking for another way to split your patch out, you could just stick use inode->integrity for the first patch, then introduce your radix-tree in a subsequent patch. It would be functionally fine, and more clearly separate out the ideas.

This was done earlier and is a really bad thing for review. Just as the breakout happening in this round.

Quoted 6 lines · Christoph Hellwig

Quoted 4 lines · Dave Hansen

If you're looking for another way to split your patch out, you could just stick use inode->integrity for the first patch, then introduce your radix-tree in a subsequent patch. It would be functionally fine, and more clearly separate out the ideas.

Quoted 2 lines

This was done earlier and is a really bad thing for review. Just as the breakout happening in this round.

If Christoph and I tell you conflicting things, it goes without saying to listen to Christoph. :)

Quoted 4 lines · Dave Hansen

If you're looking for another way to split your patch out, you could just stick use inode->integrity for the first patch, then introduce your radix-tree in a subsequent patch. It would be functionally fine, and more clearly separate out the ideas.

This was done earlier and is a really bad thing for review. Just as the breakout happening in this round.

Folks, breaking out logical changes is good, but splitting new code for the sake of it is just a bloody stupid idea. It makes reviewing things much harder and makes the submitter to stupid work. If a single new driver really is large splitting the patch doesn't help.

Quoted 14 lines

Quoted 14 lines

security/integrity/ima/ima_main.c new file +14 -0

+	if ((file->f_mode & FMODE_WRITE) &&
+	    (atomic_read(&inode->i_writecount) == 1)) {
+		rcu_read_lock();
+		iint = ima_iint_lookup(inode);
+		if (!iint) {
+			rcu_read_unlock();
+			return;
+		}
+		kref_get(&iint->refcount);
+		rcu_read_unlock();
+
+		mutex_lock(&iint->mutex);
+		if (iint->version != inode->i_version)
+			iint->flags &= ~IMA_MEASURED;

Quoted 8 lines

security/integrity/ima/ima_api.c line 245

Quoted 4 lines

security/integrity/ima/ima_api.c new file +4 -0

+		mutex_unlock(&iint->mutex);
+		kref_put(&iint->refcount, iint_free);
+	}
+}

I'm also wondering if there's a way to wrap up the mutex operations since this seems to be done the exact same way every time. Dunno, maybe it is too much locking obfuscation for just a few lines saved.

Biggest problem here is the i_version checks. i_version is only updated for directories unless you're on ext4 and use an undocumented mount option..

Quoted 5 lines

I have memories of talking about this bit. I was confused and you explained it to me, but it still isn't explained in the code. :( Again, I'm not convinced that this works. Can the code convince me, or should I go digging in my inbox?

I also haven't seen a good explanation for it yet.

Quoted 5 lines

Please take a really, really hard look at these patches, all 3000 lines of it, and try to rework them. Find common bits of code that are duplicated or copy-n-pasted around. Find functions that have gotten too long and break them up. I bet you can knock a couple hundred lines of code off this sucker, easy.

*nod* And change all the places that pass a pointer to a structure as a void pointer instead of a few normal paramters. That part really drives me nuts.

Quoted 7 lines · Christoph Hellwig

Quoted 4 lines · Dave Hansen

I have memories of talking about this bit. I was confused and you explained it to me, but it still isn't explained in the code. :( Again, I'm not convinced that this works. Can the code convince me, or should I go digging in my inbox?

I also haven't seen a good explanation for it yet.

Previous posting:
"The OS protects against an executable file, already open for write, from being executed; and an executable file, open for execute, from being modified. In the same vein, we want to know that the file measured is the same file read, that it hasn't been modified. So, if a file already open for read is then opened for write, we log it with a "Time of Measure, Time of Use" error (ToMToU) and invalidate the PCR.....

This is important when measuring configuration files, shell scripts (not measured in brpm_check_integrity which are protected by the OS), and files imported/included by scripts."

Another posting:
"From an integrity perspective, a file measurement might be invalidated unnecessarily, but it is safe. For any file when opened for write, while having an existing reader, will cause the file measurement to be invalidated."

I'm just not seeing a problem. Perhaps because only regular files are being measured, and of them, only those defined by the policy, which most likely would not include pseudo filesystems (i.e. sysfs, procfs, tmpfs, securityfs).

Quoted 22 lines · Mimi Zohar

Quoted 7 lines · Christoph Hellwig

Quoted 4 lines · Dave Hansen

I have memories of talking about this bit. I was confused and you explained it to me, but it still isn't explained in the code. :( Again, I'm not convinced that this works. Can the code convince me, or should I go digging in my inbox?

I also haven't seen a good explanation for it yet.

Previous posting:
"The OS protects against an executable file, already open for write, from being executed; and an executable file, open for execute, from being modified. In the same vein, we want to know that the file measured is the same file read, that it hasn't been modified. So, if a file already open for read is then opened for write, we log it with a "Time of Measure, Time of Use" error (ToMToU) and invalidate the PCR.....

This is important when measuring configuration files, shell scripts (not measured in brpm_check_integrity which are protected by the OS), and files imported/included by scripts."

Quoted 5 lines

Another posting:
"From an integrity perspective, a file measurement might be invalidated unnecessarily, but it is safe. For any file when opened for write, while having an existing reader, will cause the file measurement to be invalidated."

Those are all great explanations. But, some of that needs to get in the patch somehow. This is a subtle thing and someone looking at this a year for now is going to have difficulty understanding why it was done.

Quoted 4 lines

I'm just not seeing a problem. Perhaps because only regular files are being measured, and of them, only those defined by the policy, which most likely would not include pseudo filesystems (i.e. sysfs, procfs, tmpfs, securityfs).

There is no practical problem if you have false-positives on this check and do extra invalidations. But, I think both Christoph and I are nervous that this check is racy and there may be false-negatives and thus may miss some invalidations (which would be harmful).

The check is racy which is cause for concern by itself. But, with careful consideration, it may not be a dangerous or harmful race. Could you please consider it carefully and share some of your thoughts in a comment in the next version?

You need to check very, very carefully that there are no possible ways for i_writecount to be elevated without a corresponding elevation of d_count. I'm especially concerned as I look at some of the mmap() code. It appears that there are some temporary i_writecount elevations as VM_DENYWRITE is figured out. That needs some careful auditing to ensure it doesn't violate what is being assumed in the integrity code.

Quoted 24 lines · Mimi Zohar

index 0000000..6c6fcd9

security/integrity/ima/Kconfig new file +20 -0

@@ -0,0 +1,34 @@
+#
+# IBM Integrity Measurement Architecture
+#
+config IMA
+	bool "Integrity Measurement Architecture(IMA)"
+	depends on INTEGRITY
+	depends on ACPI
+	select SECURITYFS
+	select CRYPTO
+	select CRYPTO_HMAC
+	select CRYPTO_MD5
+	select CRYPTO_SHA1
+	select TCG_TPM
+	select TCG_TIS
+	help
+	  The Trusted Computing Group(TCG) runtime Integrity
+	  Measurement Architecture(IMA) maintains a list of hash
+	  values of executables and other sensitive system files
+	  loaded into the run-time of this system.  If your system
+	  has a TPM chip, then IMA also maintains an aggregate

security/integrity/ima/Kconfig line 24

Quoted 4 lines

security/integrity/ima/Kconfig new file +4 -0

+	  integrity value over this list inside the TPM hardware.
+	  These measurements and the aggregate (signed inside the
+	  TPM) can be retrieved and presented to remote parties to
+	  establish system properties. If unsure, say N.

This still doesn't tell me how it helps me. "If an attacker managed to change the contents of an important system file being measured, we can tell." Right?

Quoted 4 lines

security/integrity/ima/Kconfig new file +4 -0

+config IMA_MEASURE_PCR_IDX
+	int "PCR for Aggregate (8 <= Index <= 14)"
+	depends on IMA
+	range 8 14

security/integrity/ima/Kconfig line 34

Quoted 5 lines

security/integrity/ima/Kconfig new file +5 -0

+	default 10
+	help
+	  IMA_MEASURE_PCR_IDX determines the TPM PCR register index
+	  that IMA uses to maintain the integrity aggregate of the
+	  measurement list.  If unsure, use the default 10.

Why would you want to change this? Can it be done at runtime instead of compile time? I don't know what a PCR is.

security/integrity/ima/Kconfig

Quoted 8 lines

security/integrity/ima/Kconfig new file +8 -0

+#define ima_printk(level, format, arg...)		\
+	printk(level "ima (%s): " format, __func__, ## arg)
+
+#define ima_error(format, arg...)	\
+	ima_printk(KERN_ERR, format, ## arg)
+
+#define ima_info(format, arg...)	\
+	ima_printk(KERN_INFO, format, ## arg)

Please don't. Can you use pr_debug() and friends?

security/integrity/ima/Kconfig

Quoted 2 lines

security/integrity/ima/Kconfig new file +2 -0

+/* digest size for IMA, fits SHA1 or MD5 */
+#define IMA_DIGEST_SIZE		20

When another algorithm (with a longer digest) is added, will we detect that without this just plain breaking?

security/integrity/ima/Kconfig

Quoted 13 lines

security/integrity/ima/Kconfig new file +13 -0

+struct ima_h_table {
+	atomic_long_t len;	/* number of stored measurements in the list */
+	atomic_long_t violations;
+	unsigned int max_htable_size;
+	struct hlist_head queue[IMA_MEASURE_HTABLE_SIZE];
+	atomic_t queue_len[IMA_MEASURE_HTABLE_SIZE];
+};
+extern struct ima_h_table ima_htable;
+
+static inline unsigned long IMA_HASH_KEY(u8 *digest)
+{
+	return (hash_long(*digest, IMA_HASH_BITS));
+}

'return' isn't a function. :)

Also, please lower-case this sucker so we know it isn't a macro.

security/integrity/ima/Kconfig

Quoted 11 lines

security/integrity/ima/Kconfig new file +11 -0

+void ima_add_violation(struct inode *inode, const unsigned char *filename,
+		       char *op, char *cause)
+{
+	int result, namelen;
+	struct ima_inode_measure_entry measure_entry;
+	struct ima_store_template_data template = {
+		.name = "ima",
+		.len = sizeof(measure_entry),
+		.data = (char *)&measure_entry,
+		.violation = 1,
+	};

If '.data' is a char*, perhaps it should be a void*. If it already is a void*, you don't need a cast.

security/integrity/ima/Kconfig

Quoted 3 lines

security/integrity/ima/Kconfig new file +3 -0

+int ima_must_measure(void *data)
+{
+	struct ima_measure_data *mdata = (struct ima_measure_data *)data;

No need to cast a void*. You have several of these. Please find all of them and fix them up.

Quoted 9 lines

security/integrity/ima/Kconfig new file +9 -0

+	struct ima_iint_cache *iint;
+	int must_measure = -EACCES;
+
+	if (!S_ISREG(mdata->inode->i_mode))
+		return -EPERM;
+	if ((mdata->mask & MAY_WRITE) || (mdata->mask & MAY_APPEND))
+		return -EPERM;
+
+	rcu_read_lock();

security/integrity/ima/Kconfig

Quoted 4 lines

security/integrity/ima/Kconfig new file +4 -0

+	iint = ima_iint_lookup(mdata->inode);
+	if (iint)
+		kref_get(&iint->refcount);
+	rcu_read_unlock();

All of ima_iint_lookup()'s callers do the exact same thing. Please just make it ima_iint_find_get(), and do the RCU and kref_get() internally and once.

Quoted 16 lines

security/integrity/ima/Kconfig new file +16 -0

+	if (!iint) {
+		int rc;
+
+		/* Most insertions are done at inode_alloc,
+		 * except those allocated before late_initcall.
+		 * Can't initialize at security_initcall,
+		 * got to wait at least until proc_init.
+		 */
+		rc = ima_iint_insert(mdata->inode);
+		if (rc < 0)
+			return rc;
+
+		rcu_read_lock();
+		iint = ima_iint_lookup(mdata->inode);
+		if (!iint) {
+			rcu_read_unlock();

security/integrity/ima/Kconfig

Quoted 5 lines

security/integrity/ima/Kconfig new file +5 -0

+			return -ENOMEM;
+		}
+		kref_get(&iint->refcount);
+		rcu_read_unlock();
+	}

How about a retry goto instead of just copying the code again? Better yet, can you just stick all of this in a helper function?

security/integrity/ima/Kconfig

Quoted 6 lines

security/integrity/ima/Kconfig new file +6 -0

+int ima_iint_insert(struct inode *inode)
+{
+	struct ima_iint_cache *iint;
+	int rc = 0;
+
+	iint = kzalloc(sizeof(*iint), GFP_KERNEL);

Does this basically get done for every inode, or only special ones? I just wonder if having a dedicated slab with a constructor to do redundant things like mutex_init() would be helpful.

Quoted 17 lines

security/integrity/ima/Kconfig new file +17 -0

+static void ima_add_boot_aggregate(void)
+{
+	struct ima_inode_measure_entry measure_entry;
+	struct ima_store_template_data template = {
+		.name = "ima",
+		.len = sizeof(measure_entry),
+		.data = (char *)&measure_entry,
+	};
+	int namelen, result;
+
+	memset(&measure_entry, 0, sizeof measure_entry);
+	namelen = strlen(boot_aggregate_name);
+	if (namelen > IMA_EVENT_NAME_LEN_MAX)
+		namelen = IMA_EVENT_NAME_LEN_MAX;
+	memcpy(measure_entry.file_name, boot_aggregate_name, namelen);
+
+	if (ima_used_chip) {

security/integrity/ima/Kconfig

Quoted 5 lines

security/integrity/ima/Kconfig new file +5 -0

+		int i;
+		u8 pcr_i[IMA_DIGEST_SIZE];
+		struct hash_desc desc;
+		struct crypto_hash *tfm;
+		struct scatterlist sg;

All of this stack stuff with very important, large sounding names makes me nervous. Can you reassure me?

Quoted 5 lines

security/integrity/ima/Kconfig new file +5 -0

+		tfm = crypto_alloc_hash(ima_hash, 0, CRYPTO_ALG_ASYNC);
+		if (!tfm || IS_ERR(tfm)) {
+			ima_error("error initializing digest.\n");
+			return;
+		}

security/integrity/ima/Kconfig

Quoted 6 lines

security/integrity/ima/Kconfig new file +6 -0

+		desc.tfm = tfm;
+		desc.flags = 0;
+		crypto_hash_init(&desc);
+
+		/* cumulative sha1 over tpm registers 0-7 */
+		for (i = 0; i < 8; i++) {

Surely there's a NR_TPM_REGISTERS or similar somewhere.

Quoted 21 lines

security/integrity/ima/Kconfig new file +21 -0

+static void ima_file_free(struct file *file)
+{
+	struct inode *inode = file->f_dentry->d_inode;
+	struct ima_iint_cache *iint;
+
+	if (S_ISDIR(inode->i_mode))
+		return;
+	if ((file->f_mode & FMODE_WRITE) &&
+	    (atomic_read(&inode->i_writecount) == 1)) {
+		rcu_read_lock();
+		iint = ima_iint_lookup(inode);
+		if (!iint) {
+			rcu_read_unlock();
+			return;
+		}
+		kref_get(&iint->refcount);
+		rcu_read_unlock();
+
+		mutex_lock(&iint->mutex);
+		if (iint->version != inode->i_version)
+			iint->flags &= ~IMA_MEASURED;

security/integrity/ima/Kconfig

Quoted 4 lines

security/integrity/ima/Kconfig new file +4 -0

+		mutex_unlock(&iint->mutex);
+		kref_put(&iint->refcount, iint_free);
+	}
+}

I'm also wondering if there's a way to wrap up the mutex operations since this seems to be done the exact same way every time. Dunno, maybe it is too much locking obfuscation for just a few lines saved.

security/integrity/ima/Kconfig

Quoted 11 lines

security/integrity/ima/Kconfig new file +11 -0

+static int ima_path_check_integrity(struct path *path, int mask)
+{
+	struct ima_measure_data mdata;
+
+	memset(&mdata, 0, sizeof mdata);
+	mdata.inode = path->dentry->d_inode;
+	mdata.mask = mask;
+	mdata.function = PATH_CHECK;
+
+	/* Invalidate PCR, if a measured file is already open for read */
+	if ((mdata.mask & (MAY_WRITE | MAY_READ)) == MAY_WRITE) {

It would warm my heart to see something like this:

int mdata_is_write_only(struct ima_measure_data *mdata)

{
	if (mdata.mask & MAY_READ)
		return 0;
	return mdata.mask & MAY_WRITE;
}

I don't know about you, but I find that immeasurably nicer. Is it even right?

Quoted 4 lines · Dave Hansen

Quoted 4 lines

security/integrity/ima/Kconfig new file +4 -0

+config IMA_MEASURE_PCR_IDX
+	int "PCR for Aggregate (8 <= Index <= 14)"
+	depends on IMA
+	range 8 14

Quoted 8 lines

security/integrity/ima/Kconfig line 34

Quoted 5 lines

security/integrity/ima/Kconfig new file +5 -0

+	default 10
+	help
+	  IMA_MEASURE_PCR_IDX determines the TPM PCR register index
+	  that IMA uses to maintain the integrity aggregate of the
+	  measurement list.  If unsure, use the default 10.

Why would you want to change this? Can it be done at runtime instead of compile time? I don't know what a PCR is.

The only reason to change it would be if in the future, TCG decides on a standard PCR for IMA, other than 10, or if they pick 10 for something else. We really don't need a runtime variable for this, but kconfig makes it easy to change once if necessary in the future.

Quoted 10 lines

security/integrity/ima/ima_iint.c line 35

Quoted 6 lines

security/integrity/ima/ima_iint.c new file +6 -0

+int ima_iint_insert(struct inode *inode)
+{
+	struct ima_iint_cache *iint;
+	int rc = 0;
+
+	iint = kzalloc(sizeof(*iint), GFP_KERNEL);

Does this basically get done for every inode, or only special ones? I just wonder if having a dedicated slab with a constructor to do redundant things like mutex_init() would be helpful.

every inode, except those allocated before init_latecall.

Quoted 17 lines

Quoted 17 lines

security/integrity/ima/ima_init.c new file +17 -0

+static void ima_add_boot_aggregate(void)
+{
+	struct ima_inode_measure_entry measure_entry;
+	struct ima_store_template_data template = {
+		.name = "ima",
+		.len = sizeof(measure_entry),
+		.data = (char *)&measure_entry,
+	};
+	int namelen, result;
+
+	memset(&measure_entry, 0, sizeof measure_entry);
+	namelen = strlen(boot_aggregate_name);
+	if (namelen > IMA_EVENT_NAME_LEN_MAX)
+		namelen = IMA_EVENT_NAME_LEN_MAX;
+	memcpy(measure_entry.file_name, boot_aggregate_name, namelen);
+
+	if (ima_used_chip) {

Quoted 8 lines

security/integrity/ima/ima_init.c line 46

Quoted 5 lines

security/integrity/ima/ima_init.c new file +5 -0

+		int i;
+		u8 pcr_i[IMA_DIGEST_SIZE];
+		struct hash_desc desc;
+		struct crypto_hash *tfm;
+		struct scatterlist sg;

All of this stack stuff with very important, large sounding names makes me nervous. Can you reassure me?

The crypto code here will be moved to ima_crypto.c and will be refactored, cleaning up the code. Both measure_entry and template could be allocated/freed each time, but does that make sense?

Quoted 12 lines

security/integrity/ima/ima_main.c new file +11 -0

+		int rc;
+
+		mdata.mask = MAY_READ;
+		rc = ima_must_measure(&mdata);
+		if (!rc || rc == -EEXIST) {
+			if (atomic_read(&(path->dentry->d_count)) - 1 >
+			    atomic_read(&(mdata.inode->i_writecount)))
+				ima_add_violation(mdata.inode,
+						  path->dentry->d_name.name,
+						  "invalid_pcr", "ToMToU");
+		}

Review

Quoted 2 lines

(quoted diff) +2 -0

+		return 0;
+	}

I have memories of talking about this bit. I was confused and you explained it to me, but it still isn't explained in the code. :( Again, I'm not convinced that this works. Can the code convince me, or should I go digging in my inbox?

security/integrity/ima/Kconfig line 7

Quoted 4 lines · Mimi Zohar

security/integrity/ima/Kconfig new file +4 -0

+config IMA
+	bool "Integrity Measurement Architecture(IMA)"
+	depends on INTEGRITY
+	depends on ACPI

ACPI? why?

IMA provides hardware (TPM) based measurement and attestation for both files and other types of template measurements. As the Trusted Computing (TPM) model requires, IMA measures all files before they are accessed in any way (on the bprm_check_integrity, path_check_integrity, and file_mmap hooks), and commits the measurements to the TPM. In addition, IMA maintains a list of these hash values, which can be used to validate the aggregate PCR value. The TPM can sign these measurements, and thus the system can prove to itself and to a third party these measurements in a way that cannot be circumvented by malicious or compromised software.