This patchset contains the LIM and IMA integrity system patches.
To address concerns raised on the mailing list of the IMA patch
size, the IMA patch has been broken up into three smaller patches
for easier review.
1 - TPM internal interfaces (unchanged)
2 - LIM - (minor cleanup)
3 - IMA - Main (updates in response to comments)
4 - IMA - securityfs - display (unchanged)
5 - IMA - securityfs - policy (unchanged)
6 - (credential patch for security-testing-2.6/next)
The first five patches apply cleanly to linux-2.6.28-rc6 and with minor
offsets to security-testing-2.6/#next. The last patch addresses the
security-testing-2.6/#next credential merge issues.
Mimi Zohar (6):
integrity: TPM internel kernel interface
integrity: Linux Integrity Module(LIM)
integrity: IMA as an integrity service provider
integrity: IMA display
integrity: IMA policy
integrity: replace task uid with cred uid
This patch addresses the credential merge changes in the
security-testing-2.6/next tree.
Signed-off-by: Mimi Zohar <[email protected]>
---
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 8672f86..4b74197 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -135,7 +135,7 @@ static int ima_path_check_integrity(struct path *path, int mask)
struct vfsmount *mnt = mntget(path->mnt);
struct file *file;
- file = dentry_open(de, mnt, O_RDONLY);
+ file = dentry_open(de, mnt, O_RDONLY, current->cred);
if (IS_ERR(file)) {
ima_info("%s dentry_open failed\n",
de->d_name.name);
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index c887379..f803d1a 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -92,7 +92,7 @@ static bool ima_match_rules(struct ima_measure_rule_entry *rule,
if ((rule->flags & IMA_FSMAGIC)
&& rule->fsmagic != inode->i_sb->s_magic)
return false;
- if ((rule->flags & IMA_UID) && rule->uid != tsk->uid)
+ if ((rule->flags & IMA_UID) && rule->uid != tsk->cred->uid)
return false;
for (i = 0; i < MAX_LSM_RULES; i++) {
int rc;
diff --git a/security/integrity/integrity_audit.c b/security/integrity/integrity_audit.c
index 29518a6..142ba8b 100644
--- a/security/integrity/integrity_audit.c
+++ b/security/integrity/integrity_audit.c
@@ -48,7 +48,7 @@ void integrity_audit_msg(int audit_msgno, struct inode *inode,
ab = audit_log_start(current->audit_context, GFP_KERNEL, audit_msgno);
audit_log_format(ab, "integrity: pid=%d uid=%u auid=%u",
- current->pid, current->uid,
+ current->pid, current->cred->uid,
audit_get_loginuid(current));
audit_log_task_context(ab);
switch (audit_msgno) {
--
1.5.6.5
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]>
---
diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c
index 9c47dc4..17d2849 100644
--- a/drivers/char/tpm/tpm.c
+++ b/drivers/char/tpm/tpm.c
@@ -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;
+ 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;
+}
+
+/**
+ * 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;
+ }
+ if (!try_module_get(chip->dev->driver->owner)) {
+ rcu_read_unlock();
+ return -ENODEV;
+ }
+ rcu_read_unlock();
+
+ 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();
+
+ BUILD_BUG_ON(sizeof(pcrextend) > EXTEND_PCR_SIZE);
+ memcpy(data, pcrextend, sizeof(pcrextend));
+ index = cpu_to_be32(pcr_idx);
+ memcpy(data + 10, &index, 4);
+ 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;
+}
+EXPORT_SYMBOL_GPL(tpm_pcr_extend);
+
#define READ_PUBEK_RESULT_SIZE 314
static const u8 readpubek[] = {
0, 193, /* TPM_TAG_RQU_COMMAND */
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 8e30df4..e0ffddb 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2004 IBM Corporation
+ * Copyright (C) 2004, 2007, 2008 IBM Corporation
*
* Authors:
* Leendert van Doorn <[email protected]>
@@ -26,6 +26,7 @@
#include <linux/miscdevice.h>
#include <linux/platform_device.h>
#include <linux/io.h>
+#include <linux/tpm.h>
enum tpm_timeout {
TPM_TIMEOUT = 5, /* msecs */
@@ -128,6 +129,7 @@ extern void tpm_get_timeouts(struct tpm_chip *);
extern void tpm_gen_interrupt(struct tpm_chip *);
extern void tpm_continue_selftest(struct tpm_chip *);
extern unsigned long tpm_calc_ordinal_duration(struct tpm_chip *, u32);
+ssize_t tpm_transmit(struct tpm_chip *chip, char *buf, size_t bufsiz);
extern struct tpm_chip* tpm_register_hardware(struct device *,
const struct tpm_vendor_specific *);
extern int tpm_open(struct inode *, struct file *);
diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index 717af7a..bc26116 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -642,6 +642,9 @@ static __devexit void tpm_tis_pnp_remove(struct pnp_dev *dev)
static struct pnp_driver tis_pnp_driver = {
.name = "tpm_tis",
+ .driver = {
+ .owner = THIS_MODULE,
+ },
.id_table = tpm_pnp_tbl,
.probe = tpm_tis_pnp_init,
.suspend = tpm_tis_pnp_suspend,
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
new file mode 100644
index 0000000..355a442
--- /dev/null
+++ b/include/linux/tpm.h
@@ -0,0 +1,49 @@
+/*
+ * 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]>
+ *
+ * Device driver for TCG/TCPA TPM (trusted platform module).
+ * Specifications at http://www.trustedcomputinggroup.org
+ *
+ * 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.
+ *
+ */
+#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 {
+ TPM_ANY_NUM = 0xFFFF,
+};
+
+
+#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
--
1.5.6.5
Make the measurement lists available through securityfs.
Signed-off-by: Mimi Zohar <[email protected]>
---
diff --git a/security/integrity/ima/Makefile b/security/integrity/ima/Makefile
index d9d4405..959ae66 100644
--- a/security/integrity/ima/Makefile
+++ b/security/integrity/ima/Makefile
@@ -5,5 +5,5 @@
obj-$(CONFIG_IMA) += ima.o
-ima-y := ima_queue.o ima_init.o ima_main.o ima_crypto.o ima_api.o \
+ima-y := ima_fs.o ima_queue.o ima_init.o ima_main.o ima_crypto.o ima_api.o \
ima_policy.o ima_iint.o
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index b727a5e..795b552 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -64,6 +64,9 @@ extern const struct template_operations ima_template_ops;
/* Internal IMA function definitions */
int ima_init(void);
+void ima_cleanup(void);
+int ima_fs_init(void);
+void ima_fs_cleanup(void);
void ima_create_htable(void);
int ima_add_template_entry(struct ima_template_entry *entry, int violation);
int ima_calc_hash(struct file *file, char *digest);
@@ -117,6 +120,8 @@ int ima_collect_measurement(void *d);
int ima_appraise_measurement(void *d);
void ima_store_measurement(void *d);
int ima_store_template(void *d);
+void ima_template_show(struct seq_file *m, void *e,
+ enum integrity_show_type show);
/* For use with ima_must_measure() */
struct ima_measure_data {
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index 83ab012..f2b53c7 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -21,6 +21,7 @@ const struct template_operations ima_template_ops = {
.collect_measurement = ima_collect_measurement,
.store_measurement = ima_store_measurement,
.store_template = ima_store_template,
+ .display_template = ima_template_show
};
/**
diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
new file mode 100644
index 0000000..2627e56
--- /dev/null
+++ b/security/integrity/ima/ima_fs.c
@@ -0,0 +1,339 @@
+/*
+ * Copyright (C) 2005,2006,2007,2008 IBM Corporation
+ *
+ * Authors:
+ * Kylene Hall <[email protected]>
+ * Reiner Sailer <[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_fs.c
+ * implemenents security file system for reporting
+ * current measurement list and IMA statistics
+ */
+#include <linux/module.h>
+#include <linux/seq_file.h>
+#include <linux/rculist.h>
+#include <linux/rcupdate.h>
+
+#include "../integrity.h"
+#include "ima.h"
+
+#define TMPBUFLEN 12
+static ssize_t ima_show_htable_value(char __user *buf, size_t count,
+ loff_t *ppos, atomic_long_t *val)
+{
+ char tmpbuf[TMPBUFLEN];
+ ssize_t len;
+
+ len = scnprintf(tmpbuf, TMPBUFLEN, "%li\n", atomic_long_read(val));
+ return simple_read_from_buffer(buf, count, ppos, tmpbuf, len);
+}
+
+static ssize_t ima_show_htable_violations(struct file *filp,
+ char __user *buf,
+ size_t count, loff_t *ppos)
+{
+ return ima_show_htable_value(buf, count, ppos, &ima_htable.violations);
+}
+
+static struct file_operations ima_htable_violations_ops = {
+ .read = ima_show_htable_violations
+};
+
+static ssize_t ima_show_measurements_count(struct file *filp,
+ char __user *buf,
+ size_t count, loff_t *ppos)
+{
+ return ima_show_htable_value(buf, count, ppos, &ima_htable.len);
+
+}
+
+static struct file_operations ima_measurements_count_ops = {
+ .read = ima_show_measurements_count
+};
+
+/* returns pointer to hlist_node */
+static void *ima_measurements_start(struct seq_file *m, loff_t *pos)
+{
+ struct list_head *lpos;
+ loff_t l = *pos;
+ /* we need a lock since pos could point beyond last element */
+ rcu_read_lock();
+ __list_for_each_rcu(lpos, &ima_measurements) {
+ if (!l--) {
+ rcu_read_unlock();
+ return lpos;
+ }
+ }
+ rcu_read_unlock();
+ return NULL;
+}
+
+static void *ima_measurements_next(struct seq_file *m, void *v, loff_t *pos)
+{
+ /* lock protects when reading beyond last element
+ * against concurrent list-extension */
+ struct list_head *lpos = (struct list_head *)v;
+
+ rcu_read_lock();
+ lpos = rcu_dereference(lpos->next);
+ rcu_read_unlock();
+ (*pos)++;
+
+ return (lpos == &ima_measurements) ? NULL : lpos;
+}
+
+static void ima_measurements_stop(struct seq_file *m, void *v)
+{
+}
+
+/* print format:
+ * 32bit-le=pcr#
+ * char[20]=template digest
+ * 32bit-le=template size
+ * 32bit-le=template name size
+ * eventdata[n] = template name
+ *
+ */
+static int ima_measurements_show(struct seq_file *m, void *v)
+{
+ /* the list never shrinks, so we don't need a lock here */
+ struct list_head *lpos = v;
+ struct ima_queue_entry *qe;
+ struct ima_template_entry *e;
+ struct ima_inode_measure_entry *entry;
+ struct template_list_entry *template_entry;
+ int templatename_len;
+ int i;
+ u32 pcr = CONFIG_IMA_MEASURE_PCR_IDX;
+ char data[4];
+
+ /* get entry */
+ qe = list_entry(lpos, struct ima_queue_entry, later);
+ e = qe->entry;
+ if (e == NULL)
+ return -1;
+
+ /*
+ * 1st: PCRIndex
+ * PCR used is always the same (config option) in
+ * little-endian format
+ */
+ memcpy(data, &pcr, 4);
+ for (i = 0; i < 4; i++)
+ seq_putc(m, data[i]);
+
+ /* 2nd: template digest */
+ for (i = 0; i < 20; i++)
+ seq_putc(m, e->digest[i]);
+
+ /* 3rd: template name size */
+ templatename_len = strlen(e->template_name);
+ if (templatename_len > IMA_EVENT_NAME_LEN_MAX)
+ templatename_len = IMA_EVENT_NAME_LEN_MAX;
+
+ memcpy(data, &templatename_len, 4);
+ for (i = 0; i < 4; i++)
+ seq_putc(m, data[i]);
+
+ /* 4th: template name */
+ for (i = 0; i < templatename_len; i++)
+ seq_putc(m, e->template_name[i]);
+
+ /* 5th: template dependent */
+ entry = (struct ima_inode_measure_entry *)e->template;
+ template_entry = integrity_find_get_template(e->template_name);
+ if (template_entry) {
+ const struct template_operations *ops;
+
+ ops = template_entry->template_ops;
+ ops->display_template(m, entry, INTEGRITY_SHOW_BINARY);
+ kref_put(&template_entry->refcount, template_release);
+ } else
+ seq_printf(m, " \n");
+ return 0;
+}
+
+static struct seq_operations ima_measurments_seqops = {
+ .start = ima_measurements_start,
+ .next = ima_measurements_next,
+ .stop = ima_measurements_stop,
+ .show = ima_measurements_show
+};
+
+static int ima_measurements_open(struct inode *inode, struct file *file)
+{
+ return seq_open(file, &ima_measurments_seqops);
+}
+
+static struct file_operations ima_measurements_ops = {
+ .open = ima_measurements_open,
+ .read = seq_read,
+ .llseek = seq_lseek,
+ .release = seq_release,
+};
+
+void ima_template_show(struct seq_file *m, void *e,
+ enum integrity_show_type show)
+{
+ struct ima_inode_measure_entry *entry =
+ (struct ima_inode_measure_entry *)e;
+ int filename_len;
+ char data[4];
+ int i;
+
+ /* Display file digest */
+ for (i = 0; i < 20; i++) {
+ switch (show) {
+ case INTEGRITY_SHOW_ASCII:
+ seq_printf(m, "%02x", entry->digest[i]);
+ break;
+ case INTEGRITY_SHOW_BINARY:
+ seq_putc(m, entry->digest[i]);
+ default:
+ break;
+ }
+ }
+
+ switch (show) {
+ case INTEGRITY_SHOW_ASCII:
+ seq_printf(m, " %s\n", entry->file_name);
+ break;
+ case INTEGRITY_SHOW_BINARY:
+ filename_len = strlen(entry->file_name);
+ if (filename_len > IMA_EVENT_NAME_LEN_MAX)
+ filename_len = IMA_EVENT_NAME_LEN_MAX;
+
+ memcpy(data, &filename_len, 4);
+ for (i = 0; i < 4; i++)
+ seq_putc(m, data[i]);
+ for (i = 0; i < filename_len; i++)
+ seq_putc(m, entry->file_name[i]);
+ default:
+ break;
+ }
+}
+
+/* print in ascii */
+static int ima_ascii_measurements_show(struct seq_file *m, void *v)
+{
+ /* the list never shrinks, so we don't need a lock here */
+ struct list_head *lpos = v;
+ struct ima_queue_entry *qe;
+ struct ima_template_entry *e;
+ struct ima_inode_measure_entry *entry;
+ struct template_list_entry *template_entry;
+ int i;
+
+ /* get entry */
+ qe = list_entry(lpos, struct ima_queue_entry, later);
+ e = qe->entry;
+ if (e == NULL)
+ return -1;
+
+ /* 1st: PCR used (config option) */
+ seq_printf(m, "%2d ", CONFIG_IMA_MEASURE_PCR_IDX);
+
+ /* 2nd: SHA1 template hash */
+ for (i = 0; i < 20; i++)
+ seq_printf(m, "%02x", e->digest[i]);
+
+ /* 3th: template name */
+ seq_printf(m, " %s ", e->template_name);
+
+ /* 4th: filename <= max + \'0' delimiter */
+ entry = (struct ima_inode_measure_entry *)e->template;
+ template_entry = integrity_find_get_template(e->template_name);
+ if (template_entry) {
+ const struct template_operations *ops;
+
+ ops = template_entry->template_ops;
+ ops->display_template(m, entry, INTEGRITY_SHOW_ASCII);
+ kref_put(&template_entry->refcount, template_release);
+ } else
+ seq_printf(m, " \n");
+
+ return 0;
+}
+
+static struct seq_operations ima_ascii_measurements_seqops = {
+ .start = ima_measurements_start,
+ .next = ima_measurements_next,
+ .stop = ima_measurements_stop,
+ .show = ima_ascii_measurements_show
+};
+
+static int ima_ascii_measurements_open(struct inode *inode, struct file *file)
+{
+ return seq_open(file, &ima_ascii_measurements_seqops);
+}
+
+static struct file_operations ima_ascii_measurements_ops = {
+ .open = ima_ascii_measurements_open,
+ .read = seq_read,
+ .llseek = seq_lseek,
+ .release = seq_release,
+};
+
+static struct dentry *ima_dir;
+static struct dentry *binary_runtime_measurements;
+static struct dentry *ascii_runtime_measurements;
+static struct dentry *runtime_measurements_count;
+static struct dentry *violations;
+
+int ima_fs_init(void)
+{
+ ima_dir = securityfs_create_dir("ima", NULL);
+ if (!ima_dir || IS_ERR(ima_dir))
+ return -1;
+
+ binary_runtime_measurements =
+ securityfs_create_file("binary_runtime_measurements",
+ S_IRUSR | S_IRGRP, ima_dir, NULL,
+ &ima_measurements_ops);
+ if (!binary_runtime_measurements || IS_ERR(binary_runtime_measurements))
+ goto out;
+
+ ascii_runtime_measurements =
+ securityfs_create_file("ascii_runtime_measurements",
+ S_IRUSR | S_IRGRP, ima_dir, NULL,
+ &ima_ascii_measurements_ops);
+ if (!ascii_runtime_measurements || IS_ERR(ascii_runtime_measurements))
+ goto out;
+
+ runtime_measurements_count =
+ securityfs_create_file("runtime_measurements_count",
+ S_IRUSR | S_IRGRP, ima_dir, NULL,
+ &ima_measurements_count_ops);
+ if (!runtime_measurements_count || IS_ERR(runtime_measurements_count))
+ goto out;
+
+ violations =
+ securityfs_create_file("violations", S_IRUSR | S_IRGRP,
+ ima_dir, NULL, &ima_htable_violations_ops);
+ if (!violations || IS_ERR(violations))
+ goto out;
+
+ return 0;
+
+out:
+ securityfs_remove(runtime_measurements_count);
+ securityfs_remove(ascii_runtime_measurements);
+ securityfs_remove(binary_runtime_measurements);
+ securityfs_remove(ima_dir);
+ return -1;
+}
+
+void __exit ima_fs_cleanup(void)
+{
+ securityfs_remove(violations);
+ securityfs_remove(runtime_measurements_count);
+ securityfs_remove(ascii_runtime_measurements);
+ securityfs_remove(binary_runtime_measurements);
+ securityfs_remove(ima_dir);
+}
diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c
index 374b368..de78f93 100644
--- a/security/integrity/ima/ima_init.c
+++ b/security/integrity/ima/ima_init.c
@@ -86,5 +86,11 @@ int ima_init(void)
ima_create_htable(); /* for measurements */
ima_add_boot_aggregate(); /* boot aggregate must be first entry */
ima_init_policy();
- return 0;
+
+ return ima_fs_init();
+}
+
+void __exit ima_cleanup(void)
+{
+ ima_fs_cleanup();
}
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 2dcc3b5..8672f86 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -265,6 +265,7 @@ static void __exit cleanup_ima(void)
{
integrity_unregister_template("ima");
unregister_integrity(&ima_integrity_ops);
+ ima_cleanup();
}
late_initcall(init_ima); /* Start IMA after the TPM is available */
--
1.5.6.5
Support for a user loadable policy through securityfs
with support for LSM specific policy data.
Signed-off-by: Mimi Zohar <[email protected]>
---
diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
new file mode 100644
index 0000000..6434f0d
--- /dev/null
+++ b/Documentation/ABI/testing/ima_policy
@@ -0,0 +1,61 @@
+What: security/ima/policy
+Date: May 2008
+Contact: Mimi Zohar <[email protected]>
+Description:
+ 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. At runtime,
+ the policy can be constrained based on LSM specific data.
+ Policies are loaded into the securityfs file ima/policy
+ by opening the file, writing the rules one at a time and
+ then closing the file. The new policy takes effect after
+ the file ima/policy is closed.
+
+ rule format: action [condition ...]
+
+ action: measure | dont_measure
+ condition:= base | lsm
+ base: [[func=] [mask=] [fsmagic=] [uid=]]
+ lsm: [[subj_user=] [subj_role=] [subj_type=]
+ [obj_user=] [obj_role=] [obj_type=]]
+
+ base: func:= [BPRM_CHECK][FILE_MMAP][INODE_PERMISSION]
+ mask:= [MAY_READ] [MAY_WRITE] [MAY_APPEND] [MAY_EXEC]
+ fsmagic:= hex value
+ uid:= decimal value
+ lsm: are LSM specific
+
+ default policy:
+ # PROC_SUPER_MAGIC
+ dont_measure fsmagic=0x9fa0
+ # SYSFS_MAGIC
+ dont_measure fsmagic=0x62656572
+ # DEBUGFS_MAGIC
+ dont_measure fsmagic=0x64626720
+ # TMPFS_MAGIC
+ dont_measure fsmagic=0x01021994
+ # SECURITYFS_MAGIC
+ dont_measure fsmagic=0x73636673
+
+ measure func=BPRM_CHECK
+ measure func=FILE_MMAP mask=MAY_EXEC
+ measure func=INODE_PERM mask=MAY_READ uid=0
+
+ The default policy measures all executables in bprm_check,
+ all files mmapped executable in file_mmap, and all files
+ open for read by root in inode_permission.
+
+ Examples of LSM specific definitions:
+
+ SELinux:
+ # SELINUX_MAGIC
+ dont_measure fsmagic=0xF97CFF8C
+
+ dont_measure obj_type=var_log_t
+ dont_measure obj_type=auditd_log_t
+ measure subj_user=system_u func=INODE_PERM mask=MAY_READ
+ measure subj_role=system_r func=INODE_PERM mask=MAY_READ
+
+ Smack:
+ measure subj_user=_ func=INODE_PERM mask=MAY_READ
diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
index 6c6fcd9..3d5ccdb 100644
--- a/security/integrity/ima/Kconfig
+++ b/security/integrity/ima/Kconfig
@@ -32,3 +32,10 @@ config IMA_MEASURE_PCR_IDX
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.
+
+config IMA_LSM_RULES
+ bool "Enable LSM measurement policy rules"
+ depends on IMA && (SECURITY_SELINUX || SECURITY_SMACK)
+ default y
+ help
+ Disabling this option will not enforce LSM based policy rules.
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 795b552..5bab990 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -171,4 +171,26 @@ int ima_add_rule(int, char *subj_user, char *subj_role, char *subj_type,
char *func, char *mask, char *fsmagic, char *uid);
void ima_init_policy(void);
void ima_update_policy(void);
+
+/* LSM based policy rules require audit */
+#ifdef CONFIG_IMA_LSM_RULES
+
+#define security_filter_rule_init security_audit_rule_init
+#define security_filter_rule_match security_audit_rule_match
+
+#else
+
+static inline int security_filter_rule_init(u32 field, u32 op, char *rulestr,
+ void **lsmrule)
+{
+ return -EINVAL;
+}
+
+static inline int security_filter_rule_match(u32 secid, u32 field, u32 op,
+ void *lsmrule,
+ struct audit_context *actx)
+{
+ return -EINVAL;
+}
+#endif
#endif
diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
index 2627e56..0742520 100644
--- a/security/integrity/ima/ima_fs.c
+++ b/security/integrity/ima/ima_fs.c
@@ -280,11 +280,141 @@ static struct file_operations ima_ascii_measurements_ops = {
.release = seq_release,
};
+static char *get_tag(char *bufStart, char *bufEnd, char delimiter, int *taglen)
+{
+ char *bufp = bufStart;
+ char *tag;
+
+ /* Get start of tag */
+ while (bufp < bufEnd) {
+ if (*bufp == ' ') /* skip blanks */
+ while ((*bufp == ' ') && (bufp++ < bufEnd)) ;
+ else if (*bufp == '#') { /* skip comment */
+ while ((*bufp != '\n') && (bufp++ < bufEnd)) ;
+ bufp++;
+ } else if (*bufp == '\n') /* skip newline */
+ bufp++;
+ else if (*bufp == '\t') /* skip tabs */
+ bufp++;
+ else
+ break;
+ }
+ if (bufp < bufEnd)
+ tag = bufp;
+ else
+ return NULL;
+
+ /* Get tag */
+ *taglen = 0;
+ while ((bufp < bufEnd) && (*taglen == 0)) {
+ if ((*bufp == delimiter) || (*bufp == '\n')) {
+ *taglen = bufp - tag;
+ *bufp = '\0';
+ }
+ bufp++;
+ }
+ if (*taglen == 0) /* Didn't find end delimiter */
+ tag = NULL;
+ return tag;
+}
+
+static ssize_t ima_write_policy(struct file *file, const char __user *buf,
+ size_t buflen, loff_t *ppos)
+{
+ size_t rc = 0, datalen;
+ int action = 0;
+ char *data, *datap, *dataend;
+ char *subj_user = NULL, *subj_role = NULL, *subj_type = NULL;
+ char *obj_user = NULL, *obj_role = NULL, *obj_type = NULL;
+ char *func = NULL, *mask = NULL, *fsmagic = NULL, *uid = NULL;
+ int err = 0;
+ char *tag;
+ int taglen, i;
+
+ datalen = buflen > 4095 ? 4095 : buflen;
+ data = kmalloc(datalen + 1, GFP_KERNEL);
+ if (!data)
+ rc = -ENOMEM;
+
+ if (copy_from_user(data, buf, datalen)) {
+ kfree(data);
+ return -EFAULT;
+ }
+
+ rc = datalen;
+ *(data + datalen) = ' ';
+
+ datap = data;
+ dataend = data + datalen;
+
+ if (strncmp(datap, "measure", 7) == 0) {
+ datap += 8;
+ action = 1;
+ } else if (strncmp(datap, "dont_measure", 12) == 0)
+ datap += 13;
+ else /* bad format */
+ goto out;
+
+ for (i = 0; i < 6; i++) {
+ tag = get_tag(datap, dataend, ' ', &taglen);
+ if (!tag)
+ break;
+ if (strncmp(tag, "obj_user=", 9) == 0)
+ obj_user = tag + 9;
+ else if (strncmp(tag, "obj_role=", 9) == 0)
+ obj_role = tag + 9;
+ else if (strncmp(tag, "obj_type=", 9) == 0)
+ obj_type = tag + 9;
+ else if (strncmp(tag, "subj_user=", 10) == 0)
+ subj_user = tag + 10;
+ else if (strncmp(tag, "subj_role=", 10) == 0)
+ subj_role = tag + 10;
+ else if (strncmp(tag, "subj_type=", 10) == 0)
+ subj_type = tag + 10;
+ else if (strncmp(tag, "func=", 5) == 0)
+ func = tag + 5;
+ else if (strncmp(tag, "mask=", 5) == 0)
+ mask = tag + 5;
+ else if (strncmp(tag, "fsmagic=", 8) == 0)
+ fsmagic = tag + 8;
+ else if (strncmp(tag, "uid=", 4) == 0)
+ uid = tag + 4;
+ else { /* bad format */
+ err = 1;
+ break;
+ }
+ datap += taglen + 1;
+ }
+
+ if (!err)
+ ima_add_rule(action, subj_user, subj_role, subj_type,
+ obj_user, obj_role, obj_type,
+ func, mask, fsmagic, uid);
+out:
+ if (!data)
+ kfree(data);
+ return rc;
+}
+
static struct dentry *ima_dir;
static struct dentry *binary_runtime_measurements;
static struct dentry *ascii_runtime_measurements;
static struct dentry *runtime_measurements_count;
static struct dentry *violations;
+static struct dentry *ima_policy;
+
+static int ima_release_policy(struct inode *inode, struct file *file)
+{
+ ima_update_policy();
+ securityfs_remove(ima_policy);
+ ima_policy = NULL;
+ return 0;
+}
+
+static struct file_operations ima_measure_policy_ops = {
+ .write = ima_write_policy,
+ .release = ima_release_policy
+};
int ima_fs_init(void)
{
@@ -319,13 +449,17 @@ int ima_fs_init(void)
if (!violations || IS_ERR(violations))
goto out;
+ ima_policy = securityfs_create_file("policy",
+ S_IRUSR | S_IRGRP | S_IWUSR,
+ ima_dir, NULL,
+ &ima_measure_policy_ops);
return 0;
-
out:
securityfs_remove(runtime_measurements_count);
securityfs_remove(ascii_runtime_measurements);
securityfs_remove(binary_runtime_measurements);
securityfs_remove(ima_dir);
+ securityfs_remove(ima_policy);
return -1;
}
@@ -336,4 +470,5 @@ void __exit ima_fs_cleanup(void)
securityfs_remove(ascii_runtime_measurements);
securityfs_remove(binary_runtime_measurements);
securityfs_remove(ima_dir);
+ securityfs_remove(ima_policy);
}
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 260f71c..c887379 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -8,6 +8,7 @@
*
* ima_policy.c
* - initialize default measure policy rules
+ - load a policy ruleset
*
*/
#include <linux/module.h>
@@ -19,9 +20,18 @@
#include "ima.h"
+#define MAX_LSM_RULES 6
+enum lsm_rule_types { LSM_OBJ_USER, LSM_OBJ_ROLE, LSM_OBJ_TYPE,
+ LSM_SUBJ_USER, LSM_SUBJ_ROLE, LSM_SUBJ_TYPE
+};
+
struct ima_measure_rule_entry {
struct list_head list;
int action;
+ struct {
+ void *rule;
+ int type; /* audit type */
+ } lsm_field[MAX_LSM_RULES];
unsigned int flags;
enum lim_hooks func;
int mask;
@@ -55,8 +65,11 @@ static struct ima_measure_rule_entry default_rules[] = {
};
static struct list_head measure_default_rules;
+static struct list_head measure_policy_rules;
static struct list_head *ima_measure;
+static DEFINE_MUTEX(ima_measure_mutex);
+
/**
* ima_match_rules - determine whether an inode matches the measure rule.
* @rule: a pointer to a rule
@@ -70,6 +83,7 @@ static bool ima_match_rules(struct ima_measure_rule_entry *rule,
struct inode *inode, enum lim_hooks func, int mask)
{
struct task_struct *tsk = current;
+ int i;
if ((rule->flags & IMA_FUNC) && rule->func != func)
return false;
@@ -80,6 +94,39 @@ static bool ima_match_rules(struct ima_measure_rule_entry *rule,
return false;
if ((rule->flags & IMA_UID) && rule->uid != tsk->uid)
return false;
+ for (i = 0; i < MAX_LSM_RULES; i++) {
+ int rc;
+ u32 osid, sid;
+
+ if (!rule->lsm_field[i].rule)
+ continue;
+
+ switch (i) {
+ case LSM_OBJ_USER:
+ case LSM_OBJ_ROLE:
+ case LSM_OBJ_TYPE:
+ security_inode_getsecid(inode, &osid);
+ rc = security_filter_rule_match(osid,
+ rule->lsm_field[i].type,
+ AUDIT_EQUAL,
+ rule->lsm_field[i].rule,
+ NULL);
+ break;
+ case LSM_SUBJ_USER:
+ case LSM_SUBJ_ROLE:
+ case LSM_SUBJ_TYPE:
+ security_task_getsecid(tsk, &sid);
+ rc = security_filter_rule_match(sid,
+ rule->lsm_field[i].type,
+ AUDIT_EQUAL,
+ rule->lsm_field[i].rule,
+ NULL);
+ default:
+ break;
+ }
+ if (!rc)
+ return false;
+ }
return true;
}
@@ -121,4 +168,164 @@ void ima_init_policy(void)
for (i = 0; i < ARRAY_SIZE(default_rules); i++)
list_add_tail(&default_rules[i].list, &measure_default_rules);
ima_measure = &measure_default_rules;
+
+ INIT_LIST_HEAD(&measure_policy_rules);
+}
+
+/**
+ * ima_update_policy - update default_rules with new measure rules
+ *
+ * Wait to update the default rules with a complete new set of measure rules.
+ */
+void ima_update_policy(void)
+{
+ char *op = "policy_update";
+ char *cause = "already exists";
+ int result = 1;
+
+ if (ima_measure == &measure_default_rules) {
+ ima_measure = &measure_policy_rules;
+ cause = "complete";
+ result = 0;
+ }
+ integrity_audit_msg(AUDIT_INTEGRITY_STATUS, NULL,
+ NULL, op, cause, result);
+}
+
+/**
+ * ima_add_rule - add ima measure rules
+ * @action: integer 1 indicating MEASURE, 0 indicating DONT_MEASURE
+ * @subj_user: pointer to an LSM subject's user value
+ * @subj_role: pointer to an LSM subject's role value
+ * @subj_type: pointer to an LSM subject's type value
+ * @obj_user: pointer to an LSM object's user value
+ * @obj_role: pointer to an LSM object's role value
+ * @obj_type: pointer to an LSM object's type value
+ * @func: LIM hook identifier
+ * @mask: requested action (MAY_READ | MAY_WRITE | MAY_APPEND | MAY_EXEC)
+ * @fsmagic: fs magic hex value string
+ * @uid: uid value string
+ *
+ * Returns 0 on success, an error code on failure.
+ */
+int ima_add_rule(int action,
+ char *subj_user, char *subj_role, char *subj_type,
+ char *obj_user, char *obj_role, char *obj_type,
+ char *func, char *mask, char *fsmagic, char *uid)
+{
+ struct ima_measure_rule_entry *entry;
+ int i, result = 0;
+
+ /* Prevent installed policy from changing */
+ if (ima_measure != &measure_default_rules) {
+ integrity_audit_msg(AUDIT_INTEGRITY_STATUS, NULL,
+ NULL, "policy_update", "already exists", 0);
+ return -EACCES;
+ }
+
+ entry = kzalloc(sizeof(*entry), GFP_KERNEL);
+ INIT_LIST_HEAD(&entry->list);
+ if (action < 0 || action > 1)
+ result = -EINVAL;
+ else
+ entry->action = action;
+
+ if (!result && subj_user) {
+ i = LSM_SUBJ_USER;
+ entry->lsm_field[i].type = AUDIT_SUBJ_USER;
+ result = security_filter_rule_init(entry->lsm_field[i].type,
+ AUDIT_EQUAL, subj_user,
+ &entry->lsm_field[i].rule);
+ }
+ if (!result && subj_role) {
+ i = LSM_SUBJ_ROLE;
+ entry->lsm_field[i].type = AUDIT_SUBJ_ROLE;
+ result = security_filter_rule_init(entry->lsm_field[i].type,
+ AUDIT_EQUAL, subj_role,
+ &entry->lsm_field[i].rule);
+ }
+ if (!result && subj_type) {
+ i = LSM_SUBJ_TYPE;
+ entry->lsm_field[i].type = AUDIT_SUBJ_TYPE;
+ result = security_filter_rule_init(entry->lsm_field[i].type,
+ AUDIT_EQUAL, subj_type,
+ &entry->lsm_field[i].rule);
+ }
+ if (!result && obj_user) {
+ i = LSM_OBJ_USER;
+ entry->lsm_field[i].type = AUDIT_OBJ_USER;
+ result = security_filter_rule_init(entry->lsm_field[i].type,
+ AUDIT_EQUAL, obj_user,
+ &entry->lsm_field[i].rule);
+ }
+ if (!result && obj_role) {
+ i = LSM_OBJ_ROLE;
+ entry->lsm_field[i].type = AUDIT_OBJ_ROLE;
+ result = security_filter_rule_init(entry->lsm_field[i].type,
+ AUDIT_EQUAL, obj_role,
+ &entry->lsm_field[i].rule);
+ }
+ if (!result && obj_type) {
+ i = LSM_OBJ_TYPE;
+ entry->lsm_field[i].type = AUDIT_OBJ_TYPE;
+ result = security_filter_rule_init(entry->lsm_field[i].type,
+ AUDIT_EQUAL, obj_type,
+ &entry->lsm_field[i].rule);
+ }
+ if (!result && func) {
+ if (strcmp(func, "PATH_CHECK") == 0)
+ entry->func = PATH_CHECK;
+ else if (strcmp(func, "FILE_MMAP") == 0)
+ entry->func = FILE_MMAP;
+ else if (strcmp(func, "BPRM_CHECK") == 0)
+ entry->func = BPRM_CHECK;
+ else
+ result = -EINVAL;
+ if (!result)
+ entry->flags |= IMA_FUNC;
+ }
+ if (!result && mask) {
+ if (strcmp(mask, "MAY_EXEC") == 0)
+ entry->mask = MAY_EXEC;
+ else if (strcmp(mask, "MAY_WRITE") == 0)
+ entry->mask = MAY_WRITE;
+ else if (strcmp(mask, "MAY_READ") == 0)
+ entry->mask = MAY_READ;
+ else if (strcmp(mask, "MAY_APPEND") == 0)
+ entry->mask = MAY_APPEND;
+ else
+ result = -EINVAL;
+ if (!result)
+ entry->flags |= IMA_MASK;
+ }
+ if (!result && fsmagic) {
+ int rc;
+
+ rc = strict_strtoul(fsmagic, 16, &entry->fsmagic);
+ if (rc)
+ result = -EINVAL;
+ else
+ entry->flags |= IMA_FSMAGIC;
+ }
+ if (!result && uid) {
+ unsigned long lnum;
+ int rc;
+
+ rc = strict_strtoul(uid, 10, &lnum);
+ if (rc)
+ result = -EINVAL;
+ else {
+ entry->uid = (uid_t) lnum;
+ if (entry->uid != lnum)
+ result = -EINVAL;
+ else
+ entry->flags |= IMA_UID;
+ }
+ }
+ if (!result) {
+ mutex_lock(&ima_measure_mutex);
+ list_add_tail(&entry->list, &measure_policy_rules);
+ mutex_unlock(&ima_measure_mutex);
+ }
+ return result;
}
--
1.5.6.5
Based on comments on the mailing list, this patch:
- replaces the integrity_find_template() with integrity_find_get_template(),
which finds the template and increments the template refcount.
- replaces the local template tget()/tput() calls with kref calls.
This patch provides an integrity framework(api and hooks) and placement
of the integrity hooks in the appropriate places in the fs directory.
Collecting, appraising, and storing of file and other types of integrity
data is supported. Multiple integrity templates, which implement the
integrity API, may register themselves.
Signed-off-by: Mimi Zohar <[email protected]>
---
diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index e0f346d..f98984e 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -44,6 +44,7 @@ parameter is applicable:
FB The frame buffer device is enabled.
HW Appropriate hardware is enabled.
IA-64 IA-64 architecture is enabled.
+ INTEGRITY Integrity support is enabled.
IOSCHED More than one I/O scheduler is enabled.
IP_PNP IP DHCP, BOOTP, or RARP is enabled.
ISAPNP ISA PnP code is enabled.
@@ -896,6 +897,11 @@ and is between 256 and 4096 characters. It is defined in the file
inport.irq= [HW] Inport (ATI XL and Microsoft) busmouse driver
Format: <irq>
+ integrity_audit= [INTEGRITY]
+ Format: { "0" | "1" }
+ 0 -- integrity auditing messages. (Default)
+ 1 -- enable informational integrity auditing messages.
+
inttest= [IA64]
iommu= [x86]
diff --git a/fs/exec.c b/fs/exec.c
index 4e834f1..3494c4c 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -45,6 +45,7 @@
#include <linux/proc_fs.h>
#include <linux/mount.h>
#include <linux/security.h>
+#include <linux/integrity.h>
#include <linux/syscalls.h>
#include <linux/tsacct_kern.h>
#include <linux/cn_proc.h>
@@ -129,6 +130,9 @@ asmlinkage long sys_uselib(const char __user * library)
error = vfs_permission(&nd, MAY_READ | MAY_EXEC | MAY_OPEN);
if (error)
goto exit;
+ error = integrity_path_check(&nd.path, MAY_READ | MAY_EXEC | MAY_OPEN);
+ if (error)
+ goto exit;
file = nameidata_to_filp(&nd, O_RDONLY|O_LARGEFILE);
error = PTR_ERR(file);
@@ -1199,6 +1203,9 @@ int search_binary_handler(struct linux_binprm *bprm,struct pt_regs *regs)
retval = security_bprm_check(bprm);
if (retval)
return retval;
+ retval = integrity_bprm_check(bprm);
+ if (retval)
+ return retval;
/* kernel module loader fixup */
/* so we don't try to load run modprobe in kernel space. */
diff --git a/fs/file_table.c b/fs/file_table.c
index 5ad0eca..db32740 100644
--- a/fs/file_table.c
+++ b/fs/file_table.c
@@ -13,6 +13,7 @@
#include <linux/module.h>
#include <linux/fs.h>
#include <linux/security.h>
+#include <linux/integrity.h>
#include <linux/eventpoll.h>
#include <linux/rcupdate.h>
#include <linux/mount.h>
@@ -276,6 +277,7 @@ void __fput(struct file *file)
if (file->f_op && file->f_op->release)
file->f_op->release(inode, file);
security_file_free(file);
+ integrity_file_free(file);
if (unlikely(S_ISCHR(inode->i_mode) && inode->i_cdev != NULL))
cdev_put(inode->i_cdev);
fops_put(file->f_op);
diff --git a/fs/inode.c b/fs/inode.c
index 0487ddb..9fb48e7 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -17,6 +17,7 @@
#include <linux/hash.h>
#include <linux/swap.h>
#include <linux/security.h>
+#include <linux/integrity.h>
#include <linux/pagemap.h>
#include <linux/cdev.h>
#include <linux/bootmem.h>
@@ -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;
}
return inode;
+
+out_free_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;
}
void destroy_inode(struct inode *inode)
{
BUG_ON(inode_has_buffers(inode));
security_inode_free(inode);
+ integrity_inode_free(inode);
if (inode->i_sb->s_op->destroy_inode)
inode->i_sb->s_op->destroy_inode(inode);
else
diff --git a/fs/namei.c b/fs/namei.c
index d34e0f9..7e27006 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -24,6 +24,7 @@
#include <linux/fsnotify.h>
#include <linux/personality.h>
#include <linux/security.h>
+#include <linux/integrity.h>
#include <linux/syscalls.h>
#include <linux/mount.h>
#include <linux/audit.h>
@@ -1523,6 +1524,11 @@ int may_open(struct nameidata *nd, int acc_mode, int flag)
error = vfs_permission(nd, acc_mode);
if (error)
return error;
+
+ error = integrity_path_check(&nd->path, acc_mode
+ & (MAY_READ | MAY_WRITE | MAY_EXEC));
+ if (error)
+ return error;
/*
* An append-only file must be opened in append mode for writing.
*/
diff --git a/include/linux/audit.h b/include/linux/audit.h
index 6272a39..63bb3ec 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -123,6 +123,11 @@
#define AUDIT_LAST_KERN_ANOM_MSG 1799
#define AUDIT_ANOM_PROMISCUOUS 1700 /* Device changed promiscuous mode */
#define AUDIT_ANOM_ABEND 1701 /* Process ended abnormally */
+#define AUDIT_INTEGRITY_DATA 1800 /* Data integrity verification */
+#define AUDIT_INTEGRITY_METADATA 1801 /* Metadata integrity verification */
+#define AUDIT_INTEGRITY_STATUS 1802 /* Integrity enable status */
+#define AUDIT_INTEGRITY_HASH 1803 /* Integrity HASH type */
+#define AUDIT_INTEGRITY_PCR 1804 /* PCR invalidation msgs */
#define AUDIT_KERNEL 2000 /* Asynchronous audit record. NOT A REQUEST. */
diff --git a/include/linux/integrity.h b/include/linux/integrity.h
new file mode 100644
index 0000000..070f0bc
--- /dev/null
+++ b/include/linux/integrity.h
@@ -0,0 +1,172 @@
+/*
+ * Copyright (C) 2005,2006,2007,2008 IBM Corporation
+ * Author: 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.
+ */
+
+#ifndef _LINUX_INTEGRITY_H
+#define _LINUX_INTEGRITY_H
+
+#include <linux/fs.h>
+#include <linux/audit.h>
+
+#ifdef CONFIG_INTEGRITY
+void integrity_audit_msg(int audit_msgno, struct inode *inode,
+ const unsigned char *fname, char *op,
+ char *cause, int result);
+
+/*
+ * 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.
+ *
+ */
+
+enum integrity_show_type { INTEGRITY_SHOW_BINARY, INTEGRITY_SHOW_ASCII };
+
+struct template_operations {
+ int (*collect_measurement) (void *);
+ int (*appraise_measurement) (void *);
+ void (*store_measurement) (void *);
+ int (*store_template) (void *);
+ int (*must_measure) (void *);
+ void (*display_template) (struct seq_file *m, void *,
+ enum integrity_show_type);
+};
+extern int integrity_register_template(const char *template_name,
+ const struct template_operations *ops);
+extern int integrity_unregister_template(const char *template_name);
+
+/*
+ * Integrity hooks:
+ *
+ * @bprm_check_integrity:
+ * This hook mediates the point when a search for a binary handler will
+ * begin. At this point, the OS protects against an executable file,
+ * already open for write, from being executed; and an executable file
+ * already open for execute, from being modified. So we can be certain
+ * that any measurements(collect, appraise, store) done here are of
+ * the file being executed.
+ * @bprm contains the linux_binprm structure.
+ * Return 0 if the hook is successful and permission is granted.
+ *
+ * @inode_alloc_integrity:
+ * Allocate an integrity structure associated with an inode.
+ * @inode contains the inode structure.
+ * Return 0 if operation was successful.
+ *
+ * @inode_free_integrity:
+ * @inode contains the inode structure.
+ * Deallocate the integrity structure associated with the inode
+ *
+ * @path_check_integrity:
+ * This hook is called by the existing Linux may_open function,
+ * after calling verify_permission(), as a file is opened. At
+ * this point, measurements(collect, appraise, store) of files
+ * open for read can be made.
+ * @path contains the path structure.
+ * @mask contains the permission mask.
+ * Return 0 if the hook is successful and permission is granted.
+ *
+ * @file_free_integrity:
+ * Update the integrity xattr value as necessary.
+ * @file contains the file structure being closed.
+ *
+ * @file_mmap :
+ * Measurements(collect, appraise, store) of files mmaped for EXEC
+ * can be made.
+ * @file contains the file structure of the file to map (may be NULL).
+ * @prot contains the protection that will be applied by the kernel.
+ * Return 0 if the hook is successful and permission is granted.
+ */
+
+enum lim_hooks { PATH_CHECK = 1, FILE_MMAP, BPRM_CHECK };
+
+struct integrity_operations {
+ int (*bprm_check_integrity) (struct linux_binprm *bprm);
+ int (*inode_alloc_integrity) (struct inode *inode);
+ void (*inode_free_integrity) (struct inode *inode);
+ int (*path_check_integrity) (struct path *path, int mask);
+ void (*file_free_integrity) (struct file *file);
+ int (*file_mmap) (struct file *file, unsigned long prot);
+};
+extern int register_integrity(const struct integrity_operations *ops);
+extern int unregister_integrity(const struct integrity_operations *ops);
+
+/* global variables */
+int integrity_collect_measurement(const char *template_name, void *data);
+int integrity_appraise_measurement(const char *template_name, void *data);
+int integrity_must_measure(const char *template_name, void *data);
+int integrity_store_measurement(const char *template_name, void *data);
+int integrity_store_template(const char *template_name, void *data);
+
+int integrity_bprm_check(struct linux_binprm *bprm);
+int integrity_inode_alloc(struct inode *inode);
+void integrity_inode_free(struct inode *inode);
+int integrity_path_check(struct path *path, int mask);
+void integrity_file_free(struct file *file);
+int integrity_file_mmap(struct file *file, unsigned long prot);
+#else
+
+static inline int integrity_bprm_check(struct linux_binprm *bprm)
+{
+ return 0;
+}
+
+static inline int integrity_inode_alloc(struct inode *inode)
+{
+ return 0;
+}
+
+static inline void integrity_inode_free(struct inode *inode)
+{
+ return;
+}
+
+static inline int integrity_path_check(struct path *path, int mask)
+{
+ return 0;
+}
+
+static inline void integrity_file_free(struct file *file)
+{
+ return;
+}
+
+static inline int integrity_file_mmap(struct file *file, unsigned long prot)
+{
+ return 0;
+}
+#endif
+#endif
diff --git a/mm/mmap.c b/mm/mmap.c
index d4855a6..7cdfd3a 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -20,6 +20,7 @@
#include <linux/fs.h>
#include <linux/personality.h>
#include <linux/security.h>
+#include <linux/integrity.h>
#include <linux/hugetlb.h>
#include <linux/profile.h>
#include <linux/module.h>
@@ -1050,6 +1051,9 @@ unsigned long do_mmap_pgoff(struct file * file, unsigned long addr,
error = security_file_mmap(file, reqprot, prot, flags, addr, 0);
if (error)
return error;
+ error = integrity_file_mmap(file, prot);
+ if (error)
+ return error;
return mmap_region(file, addr, len, flags, vm_flags, pgoff,
accountable);
diff --git a/security/Kconfig b/security/Kconfig
index d9f47ce..4c602be 100644
--- a/security/Kconfig
+++ b/security/Kconfig
@@ -126,5 +126,7 @@ config SECURITY_DEFAULT_MMAP_MIN_ADDR
source security/selinux/Kconfig
source security/smack/Kconfig
+source security/integrity/Kconfig
+
endmenu
diff --git a/security/Makefile b/security/Makefile
index c05c127..db9efb5 100644
--- a/security/Makefile
+++ b/security/Makefile
@@ -17,3 +17,7 @@ obj-$(CONFIG_SECURITY_SELINUX) += selinux/built-in.o
obj-$(CONFIG_SECURITY_SMACK) += smack/built-in.o
obj-$(CONFIG_SECURITY_ROOTPLUG) += root_plug.o
obj-$(CONFIG_CGROUP_DEVICE) += device_cgroup.o
+
+# Object integrity file lists
+subdir-$(CONFIG_INTEGRITY) += integrity
+obj-$(CONFIG_INTEGRITY) += integrity/built-in.o
diff --git a/security/integrity/Kconfig b/security/integrity/Kconfig
new file mode 100644
index 0000000..2123dfc
--- /dev/null
+++ b/security/integrity/Kconfig
@@ -0,0 +1,24 @@
+#
+# Integrity configuration
+#
+
+config INTEGRITY
+ bool "Enable different integrity models"
+ help
+ This allows you to choose different integrity modules to be
+ configured into your kernel. Integrity models can collect,
+ verify, and record measurements, such as of a file's data
+ and metadata before it is read or executed.
+
+ If you are unsure how to answer this question, answer N.
+
+config INTEGRITY_AUDIT
+ bool "Integrity audit boot parameter"
+ depends on INTEGRITY
+ default n
+ help
+ This option adds a kernel parameter 'integrity_audit', which
+ allows informational integrity auditing messages to be enabled
+ at boot. If this option is selected, informational integrity
+ auditing messages can be enabled with 'integrity_audit=1' on
+ the kernel command line.
diff --git a/security/integrity/Makefile b/security/integrity/Makefile
new file mode 100644
index 0000000..a6e1405
--- /dev/null
+++ b/security/integrity/Makefile
@@ -0,0 +1,6 @@
+#
+# Makefile for the kernel integrity code
+#
+
+# Object file lists
+obj-$(CONFIG_INTEGRITY) += integrity.o integrity_audit.o
diff --git a/security/integrity/integrity.c b/security/integrity/integrity.c
new file mode 100644
index 0000000..35931e7
--- /dev/null
+++ b/security/integrity/integrity.c
@@ -0,0 +1,329 @@
+/*
+ * Copyright (C) 2006,2007,2008 IBM Corporation
+ * Author: 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: integrity.c
+ * register integrity subsystem
+ * register integrity template
+ */
+
+#include <asm/atomic.h>
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/integrity.h>
+#include "integrity.h"
+
+static const struct integrity_operations *integrity_ops;
+static LIST_HEAD(integrity_templates);
+static DEFINE_MUTEX(integrity_templates_mutex);
+
+/**
+ * register_integrity - registers an integrity framework with the kernel
+ * @ops: a pointer to the struct integrity_options that is to be registered
+ *
+ * Perhaps in the future integrity module stacking will be necessary, but
+ * for the time being, this function permits only one integrity module to
+ * register itself with the kernel integrity subsystem.
+ *
+ * If another integrity module is already registered, an error code is
+ * returned. On success 0 is returned.
+ */
+int register_integrity(const struct integrity_operations *ops)
+{
+ if (integrity_ops != NULL)
+ return -EAGAIN;
+ integrity_ops = ops;
+ return 0;
+}
+EXPORT_SYMBOL_GPL(register_integrity);
+
+/**
+ * unregister_integrity - unregisters an integrity framework from the kernel
+ * @ops: a pointer to the struct integrity_options that is to be registered
+ *
+ * Returns 0 on success, -EINVAL on failure.
+ */
+int unregister_integrity(const struct integrity_operations *ops)
+{
+ if (ops != integrity_ops)
+ return -EINVAL;
+
+ integrity_ops = NULL;
+ return 0;
+}
+EXPORT_SYMBOL_GPL(unregister_integrity);
+
+/**
+ * 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);
+ entry->template_ops = template_ops;
+
+ mutex_lock(&integrity_templates_mutex);
+ list_add_rcu(&entry->template, &integrity_templates);
+ mutex_unlock(&integrity_templates_mutex);
+ synchronize_rcu();
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(integrity_register_template);
+
+void template_release(struct kref *kref)
+{
+ struct template_list_entry *entry = container_of(kref,
+ struct template_list_entry,
+ refcount);
+ kfree(entry);
+}
+
+/**
+ * integrity_unregister_template: unregister a template
+ * @template_name: a pointer to a string containing the template name.
+ *
+ * Returns 0 on success, -EINVAL on failure.
+ */
+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;
+ }
+ }
+ mutex_unlock(&integrity_templates_mutex);
+ return -EINVAL;
+}
+EXPORT_SYMBOL_GPL(integrity_unregister_template);
+
+/**
+ * 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;
+ }
+ }
+ if (template_entry)
+ kref_get(&template_entry->refcount);
+ rcu_read_unlock();
+ return template_entry;
+}
+EXPORT_SYMBOL_GPL(integrity_find_get_template);
+
+
+/* Start of the integrity API calls */
+
+/**
+ * integrity_collect_measurement - collect template specific measurement
+ * @template_name: a pointer to a string containing the template name.
+ * @data: pointer to template specific data
+ *
+ * Returns 0 on success, an error code on failure.
+ */
+int integrity_collect_measurement(const char *template_name, void *data)
+{
+ struct template_list_entry *template_entry;
+ int rc = -EINVAL;
+
+ 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);
+
+/**
+ * integrity_appraise_measurement - appraise template specific measurement
+ * @template_name: a pointer to a string containing the template name.
+ * @data: pointer to template specific data
+ *
+ * Returns 0 on success, an error code on failure
+ */
+int integrity_appraise_measurement(const char *template_name, void *data)
+{
+ struct template_list_entry *template_entry;
+ int rc = -EINVAL;
+
+ template_entry = integrity_find_get_template(template_name);
+ if (template_entry) {
+ rc = template_entry->template_ops->appraise_measurement(data);
+ kref_put(&template_entry->refcount, template_release);
+ }
+ return rc;
+}
+EXPORT_SYMBOL_GPL(integrity_appraise_measurement);
+
+/**
+ * integrity_store_measurement - store template specific measurement
+ * @template_name: a pointer to a string containing the template name.
+ * @data: pointer to template specific data
+ *
+ * Store template specific integrity measurement.
+ */
+int integrity_store_measurement(const char *template_name, void *data)
+{
+ struct template_list_entry *template_entry;
+ int rc = -EINVAL;
+
+ template_entry = integrity_find_get_template(template_name);
+ if (template_entry) {
+ rc = 0;
+ template_entry->template_ops->store_measurement(data);
+ kref_put(&template_entry->refcount, template_release);
+
+ }
+ return rc;
+}
+EXPORT_SYMBOL_GPL(integrity_store_measurement);
+
+/**
+ * integrity_store_template - store entry containing template specific data
+ * @template_name: a pointer to a string containing the template name.
+ * @data: pointer to template specific data
+ *
+ * Returns 0 on success, an error code on failure.
+ */
+int integrity_store_template(const char *template_name, void *data)
+{
+ struct template_list_entry *template_entry;
+ int rc = -EINVAL;
+
+ template_entry = integrity_find_get_template(template_name);
+ if (template_entry) {
+ rc = template_entry->template_ops->store_template(data);
+ kref_put(&template_entry->refcount, template_release);
+
+ }
+ return rc;
+}
+EXPORT_SYMBOL_GPL(integrity_store_template);
+
+/**
+ * integrity_must_measure - measure decision based on template policy
+ * @template_name: a pointer to a string containing the template name.
+ * @data: pointer to template specific data
+ *
+ * Returns 0 on success, an error code on failure.
+ */
+int integrity_must_measure(const char *template_name, void *data)
+{
+ struct template_list_entry *template_entry;
+ int rc = -EINVAL;
+
+ template_entry = integrity_find_get_template(template_name);
+ if (template_entry) {
+ rc = template_entry->template_ops->must_measure(data);
+ kref_put(&template_entry->refcount, template_release);
+ }
+ return rc;
+}
+EXPORT_SYMBOL_GPL(integrity_must_measure);
+
+/* Start of the integrity Hooks */
+
+/* Hook used to measure executable file integrity. */
+int integrity_bprm_check(struct linux_binprm *bprm)
+{
+ int rc = 0;
+
+ if (integrity_ops && integrity_ops->bprm_check_integrity)
+ rc = integrity_ops->bprm_check_integrity(bprm);
+ return rc;
+}
+
+/* Allocate, attach and initialize an inode's i_integrity,
+ * if INTEGRITY is configured.
+ */
+int integrity_inode_alloc(struct inode *inode)
+{
+ int rc = 0;
+
+ if (integrity_ops && integrity_ops->inode_alloc_integrity)
+ rc = integrity_ops->inode_alloc_integrity(inode);
+ return rc;
+}
+
+/* Hook used to free an inode's i_integrity structure. */
+void integrity_inode_free(struct inode *inode)
+{
+ if (integrity_ops && integrity_ops->inode_free_integrity)
+ integrity_ops->inode_free_integrity(inode);
+}
+
+/* Hook used to measure a file's integrity. */
+int integrity_path_check(struct path *path, int mask)
+{
+ int rc = 0;
+
+ if (integrity_ops && integrity_ops->path_check_integrity)
+ rc = integrity_ops->path_check_integrity(path, mask);
+ return rc;
+}
+
+/* Hook used to update i_integrity data and integrity xattr values
+ * as necessary.
+ */
+void integrity_file_free(struct file *file)
+{
+ if (integrity_ops && integrity_ops->file_free_integrity)
+ integrity_ops->file_free_integrity(file);
+}
+
+/* Hook used to measure integrity of an mmapped file */
+int integrity_file_mmap(struct file *file, unsigned long prot)
+{
+ int rc = 0;
+
+ if (integrity_ops && integrity_ops->file_mmap)
+ rc = integrity_ops->file_mmap(file, prot);
+ return rc;
+}
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
new file mode 100644
index 0000000..6ead5ed
--- /dev/null
+++ b/security/integrity/integrity.h
@@ -0,0 +1,19 @@
+/*
+ * Copyright (C) 2008 IBM Corporation
+ * Author: 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.
+ */
+
+#define TEMPLATE_NAME_LEN_MAX 12
+struct template_list_entry {
+ struct list_head template;
+ char template_name[TEMPLATE_NAME_LEN_MAX + 1];
+ const struct template_operations *template_ops;
+ struct kref refcount;
+};
+
+extern void template_release(struct kref *kref);
+extern struct template_list_entry *integrity_find_get_template(const char *);
diff --git a/security/integrity/integrity_audit.c b/security/integrity/integrity_audit.c
new file mode 100644
index 0000000..29518a6
--- /dev/null
+++ b/security/integrity/integrity_audit.c
@@ -0,0 +1,78 @@
+/*
+ * Copyright (C) 2008 IBM Corporation
+ * Author: 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: integrity_audit.c
+ * Audit calls for the integrity subsystem
+ */
+
+#include <linux/audit.h>
+#include <linux/fs.h>
+#include <linux/integrity.h>
+
+static int integrity_audit;
+
+#ifdef CONFIG_INTEGRITY_AUDIT
+static int __init integrity_audit_setup(char *str)
+{
+ unsigned long audit;
+ int rc;
+ char *op;
+
+ rc = strict_strtoul(str, 0, &audit);
+ if (rc || audit > 1)
+ printk(KERN_INFO "integrity: invalid integrity_audit value\n");
+ else
+ integrity_audit = audit;
+
+ op = integrity_audit ? "integrity_audit_enabled" :
+ "integrity_audit_not_enabled";
+ integrity_audit_msg(AUDIT_INTEGRITY_STATUS, NULL, NULL, NULL, op, 0);
+ return 1;
+}
+__setup("integrity_audit=", integrity_audit_setup);
+#endif
+
+void integrity_audit_msg(int audit_msgno, struct inode *inode,
+ const unsigned char *fname, char *op,
+ char *cause, int result)
+{
+ struct audit_buffer *ab;
+
+ if (!integrity_audit && result == 1) /* Skip information messages */
+ return;
+
+ ab = audit_log_start(current->audit_context, GFP_KERNEL, audit_msgno);
+ audit_log_format(ab, "integrity: pid=%d uid=%u auid=%u",
+ current->pid, current->uid,
+ audit_get_loginuid(current));
+ audit_log_task_context(ab);
+ switch (audit_msgno) {
+ case AUDIT_INTEGRITY_DATA:
+ case AUDIT_INTEGRITY_METADATA:
+ case AUDIT_INTEGRITY_PCR:
+ audit_log_format(ab, " op=%s cause=%s", op, cause);
+ break;
+ case AUDIT_INTEGRITY_HASH:
+ audit_log_format(ab, " op=%s hash=%s", op, cause);
+ break;
+ case AUDIT_INTEGRITY_STATUS:
+ default:
+ audit_log_format(ab, " op=%s", op);
+ }
+ audit_log_format(ab, " comm=");
+ audit_log_untrustedstring(ab, current->comm);
+ if (fname) {
+ audit_log_format(ab, " name=");
+ audit_log_untrustedstring(ab, fname);
+ }
+ if (inode)
+ audit_log_format(ab, " dev=%s ino=%lu",
+ inode->i_sb->s_id, inode->i_ino);
+ audit_log_format(ab, " res=%d", result);
+ audit_log_end(ab);
+}
--
1.5.6.5
Based on suggestions on the mailing list:
- Due to the large patch size, IMA is broken up into three smaller patches:
ima-service-provider, ima-display, and ima-policy.
- Replaced skip_measurement() function with single S_ISREG() call.
- Simplified ima_calc_hash() and removed update_file_hash() by
eliminating the 'path' parameter. Getting a 'file' handle is now
done by the caller.
- Removed tiny wrappers ima_inode_alloc_integrity()/free_integrity().
Now calls ima_iint_alloc()/delete() directly.
- Removed all references to d_iname.
- Corrected ima_path_check() mask comparisons, removing MAY_APPEND.
- For readability, removed audit_type and lsm_type defines and usage.
- Replaced the generic 'struct ima_args_data' with integrity API
function specific structures. (definitions in include/linux/ima.h and "ima.h")
- ima_store_measurement() now only forms an "ima" specific template measurement
- Defined a new integrity API call store_template().
- ima_store_template wraps a template specific measurement with a header
containing: template hash, template name, template data length and data.
- Replaced local iint_get/iint_put calls with kref calls.
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.
Signed-off-by: Mimi Zohar <[email protected]>
---
diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index f98984e..aff7933 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -44,6 +44,7 @@ parameter is applicable:
FB The frame buffer device is enabled.
HW Appropriate hardware is enabled.
IA-64 IA-64 architecture is enabled.
+ IMA Integrity measurement architecture is enabled.
INTEGRITY Integrity support is enabled.
IOSCHED More than one I/O scheduler is enabled.
IP_PNP IP DHCP, BOOTP, or RARP is enabled.
@@ -880,6 +881,10 @@ and is between 256 and 4096 characters. It is defined in the file
ihash_entries= [KNL]
Set number of hash buckets for inode cache.
+ ima_hash= [IMA] runtime ability to define hash crypto algorithm.
+ Format: { "MD5" | "SHA1" }
+ Default is "SHA1".
+
in2000= [HW,SCSI]
See header of drivers/scsi/in2000.c.
diff --git a/include/linux/ima.h b/include/linux/ima.h
new file mode 100644
index 0000000..6453100
--- /dev/null
+++ b/include/linux/ima.h
@@ -0,0 +1,24 @@
+/*
+ * ima.h
+ *
+ * Copyright (C) 2008 IBM Corporation
+ * Author: 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.
+ */
+
+#ifndef _LINUX_IMA_H
+#define _LINUX_IMA_H
+
+/* Store template measurements for use with ima_store_template() */
+struct ima_store_template_data {
+ char *name;
+ int len;
+ char *data;
+ int violation;
+ struct inode *inode;
+};
+
+#endif
diff --git a/security/Kconfig b/security/Kconfig
index 4c602be..665a1b7 100644
--- a/security/Kconfig
+++ b/security/Kconfig
@@ -55,7 +55,8 @@ config SECURITYFS
bool "Enable the securityfs filesystem"
help
This will build the securityfs filesystem. It is currently used by
- the TPM bios character driver. It is not used by SELinux or SMACK.
+ the TPM bios character driver and IMA, an integrity provider. It
+ is not used by SELinux or SMACK.
If you are unsure how to answer this question, answer N.
diff --git a/security/integrity/Kconfig b/security/integrity/Kconfig
index 2123dfc..57213d6 100644
--- a/security/integrity/Kconfig
+++ b/security/integrity/Kconfig
@@ -22,3 +22,5 @@ config INTEGRITY_AUDIT
at boot. If this option is selected, informational integrity
auditing messages can be enabled with 'integrity_audit=1' on
the kernel command line.
+
+source security/integrity/ima/Kconfig
diff --git a/security/integrity/Makefile b/security/integrity/Makefile
index a6e1405..107dafd 100644
--- a/security/integrity/Makefile
+++ b/security/integrity/Makefile
@@ -4,3 +4,4 @@
# Object file lists
obj-$(CONFIG_INTEGRITY) += integrity.o integrity_audit.o
+obj-$(CONFIG_IMA) += ima/
diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
new file mode 100644
index 0000000..6c6fcd9
--- /dev/null
+++ b/security/integrity/ima/Kconfig
@@ -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
+ 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.
+
+config IMA_MEASURE_PCR_IDX
+ int "PCR for Aggregate (8 <= Index <= 14)"
+ depends on IMA
+ range 8 14
+ 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.
diff --git a/security/integrity/ima/Makefile b/security/integrity/ima/Makefile
new file mode 100644
index 0000000..d9d4405
--- /dev/null
+++ b/security/integrity/ima/Makefile
@@ -0,0 +1,9 @@
+#
+# Makefile for building Trusted Computing Group's(TCG) runtime Integrity
+# Measurement Architecture(IMA).
+#
+
+obj-$(CONFIG_IMA) += ima.o
+
+ima-y := ima_queue.o ima_init.o ima_main.o ima_crypto.o ima_api.o \
+ ima_policy.o ima_iint.o
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
new file mode 100644
index 0000000..b727a5e
--- /dev/null
+++ b/security/integrity/ima/ima.h
@@ -0,0 +1,169 @@
+/*
+ * Copyright (C) 2005,2006,2007,2008 IBM Corporation
+ *
+ * Authors:
+ * Reiner Sailer <[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.h
+ * internal ima definitions
+ */
+
+#ifndef __LINUX_IMA_H
+#define __LINUX_IMA_H
+
+#include <linux/types.h>
+#include <linux/crypto.h>
+#include <linux/security.h>
+#include <linux/integrity.h>
+#include <linux/hash.h>
+#include <linux/tpm.h>
+#include <linux/ima.h>
+
+#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)
+
+/* digest size for IMA, fits SHA1 or MD5 */
+#define IMA_DIGEST_SIZE 20
+#define IMA_EVENT_NAME_LEN_MAX 255
+
+#define IMA_HASH_BITS 9
+#define IMA_MEASURE_HTABLE_SIZE (1 << IMA_HASH_BITS)
+
+/* set during initialization */
+extern int ima_used_chip;
+extern char *ima_hash;
+
+struct ima_template_entry {
+ u8 digest[IMA_DIGEST_SIZE]; /* sha1 or md5 measurement hash */
+ char template_name[IMA_EVENT_NAME_LEN_MAX + 1]; /* name + \0 */
+ int template_len;
+ char *template;
+};
+
+struct ima_queue_entry {
+ struct hlist_node hnext; /* place in hash collision list */
+ struct list_head later; /* place in ima_measurements list */
+ struct ima_template_entry *entry;
+};
+extern struct list_head ima_measurements; /* list of all measurements */
+
+/* declarations */
+extern const struct template_operations ima_template_ops;
+
+/* Internal IMA function definitions */
+int ima_init(void);
+void ima_create_htable(void);
+int ima_add_template_entry(struct ima_template_entry *entry, int violation);
+int ima_calc_hash(struct file *file, char *digest);
+int ima_calc_template_hash(int template_len, char *template, char *digest);
+void ima_add_violation(struct inode *inode, const unsigned char *filename,
+ char *op, char *cause);
+
+/*
+ * used to protect h_table and sha_table
+ */
+extern spinlock_t ima_queue_lock;
+
+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));
+}
+
+/* TPM "Glue" definitions */
+
+#define IMA_TPM ((((u32)TPM_ANY_TYPE)<<16) | (u32)TPM_ANY_NUM)
+static inline void ima_extend(const u8 *hash)
+{
+ if (!ima_used_chip)
+ return;
+
+ if (tpm_pcr_extend(IMA_TPM, CONFIG_IMA_MEASURE_PCR_IDX, hash) != 0)
+ ima_error("Error Communicating to TPM chip\n");
+}
+
+static inline void ima_pcrread(int idx, u8 *pcr, int pcr_size)
+{
+ if (!ima_used_chip)
+ return;
+
+ if (tpm_pcr_read(IMA_TPM, idx, pcr) != 0)
+ ima_error("Error Communicating to TPM chip\n");
+}
+
+/* LIM API function definitions */
+int ima_must_measure(void *d);
+int ima_collect_measurement(void *d);
+int ima_appraise_measurement(void *d);
+void ima_store_measurement(void *d);
+int ima_store_template(void *d);
+
+/* For use with ima_must_measure() */
+struct ima_measure_data {
+ struct inode *inode;
+ int function;
+ int mask;
+};
+
+/* For use with ima_store_measurement() */
+struct ima_store_data {
+ struct file *file;
+ const unsigned char *filename;
+};
+
+/* IMA inode template definition */
+struct ima_inode_measure_entry {
+ u8 digest[IMA_DIGEST_SIZE]; /* sha1/md5 measurement hash */
+ char file_name[IMA_EVENT_NAME_LEN_MAX + 1]; /* name + \0 */
+};
+
+/* integrity data associated with an inode */
+struct ima_iint_cache {
+ u64 version;
+ ulong flags;
+ u8 hmac[IMA_DIGEST_SIZE];
+ u8 digest[IMA_DIGEST_SIZE];
+ struct mutex mutex;
+ struct kref refcount;
+};
+#define IMA_IINT_INIT 1
+#define IMA_MUST_MEASURE 2
+#define IMA_MEASURED 4
+
+/* radix tree calls to lookup, insert, delete
+ * integrity data associated with an inode.
+ */
+struct ima_iint_cache *ima_iint_lookup(struct inode *inode);
+int ima_iint_insert(struct inode *inode);
+void ima_iint_delete(struct inode *inode);
+void iint_free(struct kref *kref);
+
+/* IMA policy related functions */
+enum ima_action { DONT_MEASURE, MEASURE };
+int ima_match_policy(struct inode *inode, enum lim_hooks func, int mask);
+int ima_add_rule(int, char *subj_user, char *subj_role, char *subj_type,
+ char *obj_user, char *obj_role, char *obj_type,
+ char *func, char *mask, char *fsmagic, char *uid);
+void ima_init_policy(void);
+void ima_update_policy(void);
+#endif
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
new file mode 100644
index 0000000..83ab012
--- /dev/null
+++ b/security/integrity/ima/ima_api.c
@@ -0,0 +1,309 @@
+/*
+ * Copyright (C) 2008 IBM Corporation
+ *
+ * Author: 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_api.c
+ * - implements the LIM API
+ */
+#include <linux/module.h>
+#include <linux/integrity.h>
+
+#include "ima.h"
+
+const struct template_operations ima_template_ops = {
+ .must_measure = ima_must_measure,
+ .collect_measurement = ima_collect_measurement,
+ .store_measurement = ima_store_measurement,
+ .store_template = ima_store_template,
+};
+
+/**
+ * ima_store_template - collect and protect template measurements
+ * @data: pointer to struct ima_store_template_data
+ *
+ * ima_struct_template_data contains the template name, template data length,
+ * template data, inode, and violation (invalidate pcr measurement indication).
+ *
+ * Calculate the hash of a template entry, add the template entry
+ * to an ordered list of measurement entries maintained inside the kernel,
+ * and also update the aggregate integrity value (maintained inside the
+ * configured TPM PCR) over the hashes of the current list of measurement
+ * entries.
+ *
+ * Applications retrieve the current kernel-held measurement list through
+ * the securityfs entries in /sys/kernel/security/ima. The signed aggregate
+ * TPM PCR (called quote) can be retrieved using a TPM user space library
+ * and is used to validate the measurement list.
+ *
+ * Returns 0 on success, error code otherwise
+ */
+int ima_store_template(void *data)
+{
+ struct ima_store_template_data *template =
+ (struct ima_store_template_data *)data;
+ struct ima_template_entry *entry;
+ char *op = "add_template_measure";
+ char *audit_cause = "";
+ int namelen, result = 0;
+
+ entry = kzalloc(sizeof(*entry), GFP_KERNEL);
+ if (!entry) {
+ result = -ENOMEM;
+ goto out;
+ }
+
+ entry->template = kzalloc(template->len, GFP_KERNEL);
+ if (!entry->template) {
+ kfree(entry);
+ result = -ENOMEM;
+ goto out;
+ }
+ namelen = strlen(template->name);
+ if (namelen > IMA_EVENT_NAME_LEN_MAX)
+ namelen = IMA_EVENT_NAME_LEN_MAX;
+ memcpy(entry->template_name, template->name, namelen);
+
+ entry->template_len = template->len;
+ memcpy(entry->template, template->data, template->len);
+
+ if (!template->violation) {
+ result = ima_calc_template_hash(template->len, template->data,
+ entry->digest);
+ if (result < 0)
+ goto free_out;
+ }
+
+ result = ima_add_template_entry(entry, template->violation);
+free_out:
+ if (result < 0) {
+ kfree(entry->template);
+ kfree(entry);
+ }
+out:
+ switch (result) {
+ case -EEXIST:
+ audit_cause = "hash_exists";
+ break;
+ case -ENOMEM:
+ audit_cause = "ENOMEM";
+ break;
+ default:
+ break;
+ }
+
+ if (!result || result == -EEXIST) {
+ integrity_audit_msg(AUDIT_INTEGRITY_PCR, template->inode,
+ template->name, op, audit_cause, 1);
+ } else if (result < 0)
+ integrity_audit_msg(AUDIT_INTEGRITY_PCR, template->inode,
+ template->name, op, audit_cause, 0);
+ return result;
+}
+
+/**
+ * ima_add_violation - add violation to measurement list.
+ * @inode: inode associated with the violation
+ * @filename: the file name associated with the inode
+ * @op: string pointer to audit operation (i.e. "invalid_pcr", "add_measure")
+ * @cause: string pointer to reason for violation (i.e. "ToMToU")
+ *
+ * Violations are flagged in the measurement list with zero hash values.
+ * By extending the PCR with 0xFF's instead of with zeroes, the PCR
+ * value is invalidated.
+ */
+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,
+ };
+
+ /* can overflow, only indicator */
+ atomic_long_inc(&ima_htable.violations);
+
+ memset(&measure_entry, 0, sizeof measure_entry);
+ namelen = strlen(filename);
+ if (namelen > IMA_EVENT_NAME_LEN_MAX)
+ namelen = IMA_EVENT_NAME_LEN_MAX;
+ memcpy(measure_entry.file_name, filename, namelen);
+ result = ima_store_template(&template);
+ integrity_audit_msg(AUDIT_INTEGRITY_PCR, inode, filename, op, cause, 0);
+}
+
+/**
+ * ima_must_measure - measure decision based on policy.
+ * @data: pointer to struct ima_measure_data(contains inode, function, mask)
+ *
+ * The policy is defined in terms of keypairs:
+ * subj=, obj=, type=, func=, mask=, fsmagic=
+ * subj,obj, and type: are LSM specific.
+ * func: PATH_CHECK | BPRM_CHECK | FILE_MMAP
+ * mask: contains the permission mask
+ * fsmagic: hex value
+ *
+ * Return 0 to measure. For matching a DONT_MEASURE policy, no policy,
+ * or other error, return an error code.
+*/
+int ima_must_measure(void *data)
+{
+ struct ima_measure_data *mdata = (struct ima_measure_data *)data;
+ 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();
+ iint = ima_iint_lookup(mdata->inode);
+ if (iint)
+ kref_get(&iint->refcount);
+ rcu_read_unlock();
+
+ 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();
+ return -ENOMEM;
+ }
+ kref_get(&iint->refcount);
+ rcu_read_unlock();
+ }
+
+ mutex_lock(&iint->mutex);
+ /* short circuit out of here, if iint initialized
+ * and either no measurement required or already measured.
+ */
+ if (iint->flags & IMA_IINT_INIT) {
+ if (!(iint->flags & IMA_MUST_MEASURE))
+ must_measure = -EACCES;
+ else if (iint->flags & IMA_MEASURED)
+ must_measure = -EEXIST;
+ else
+ must_measure = 0;
+ } else {
+ int rc;
+
+ rc = ima_match_policy(mdata->inode, mdata->function,
+ mdata->mask);
+ if (rc) {
+ iint->flags = IMA_MUST_MEASURE;
+ must_measure = 0;
+ } else
+ must_measure = -EACCES;
+ iint->flags |= IMA_IINT_INIT;
+ }
+ mutex_unlock(&iint->mutex);
+ kref_put(&iint->refcount, iint_free);
+ return must_measure;
+}
+
+/**
+ * ima_collect_measurement - collect file measurement
+ * @data: is a pointer to the file to be measured
+ *
+ * Calculate the file hash, if it doesn't already exist,
+ * and store the measurement in the radix tree.
+ * Return 0 on success, error code otherwise
+ */
+int ima_collect_measurement(void *data)
+{
+ struct file *file = (struct file *)data;
+ struct inode *inode = file->f_dentry->d_inode;
+ struct ima_iint_cache *iint;
+ int result = 0;
+
+ rcu_read_lock();
+ iint = ima_iint_lookup(inode);
+ if (!iint) {
+ rcu_read_unlock();
+ return -ENOMEM;
+ }
+ kref_get(&iint->refcount);
+ rcu_read_unlock();
+
+ mutex_lock(&iint->mutex);
+ if (!(iint->flags & IMA_MEASURED)) {
+ memset(iint->digest, 0, IMA_DIGEST_SIZE);
+ result = ima_calc_hash(file, iint->digest);
+ } else
+ result = -EEXIST;
+ mutex_unlock(&iint->mutex);
+ kref_put(&iint->refcount, iint_free);
+ return result;
+}
+
+/**
+ * ima_store_measurement - store file measurement
+ * @data: pointer to struct ima_store_data (contains file and filename)
+ *
+ * Create an "ima" template and then store the template by calling
+ * ima_store_template.
+ */
+void ima_store_measurement(void *data)
+{
+ struct ima_store_data *sdata = (struct ima_store_data *)data;
+ struct ima_iint_cache *iint = NULL;
+ int result;
+ int namelen;
+ struct ima_inode_measure_entry measure_entry;
+ struct ima_store_template_data template = {
+ .name = "ima",
+ .len = sizeof(measure_entry),
+ .data = (char *)&measure_entry,
+ };
+
+ template.inode = sdata->file->f_dentry->d_inode;
+ rcu_read_lock();
+ iint = ima_iint_lookup(template.inode);
+ if (!iint) {
+ rcu_read_unlock();
+ return;
+ }
+ kref_get(&iint->refcount);
+ rcu_read_unlock();
+
+ mutex_lock(&iint->mutex);
+ if (iint->flags & IMA_MEASURED) { /* already exists */
+ mutex_unlock(&iint->mutex);
+ return;
+ }
+ memset(&measure_entry, 0, sizeof measure_entry);
+ memcpy(measure_entry.digest, iint->digest, IMA_DIGEST_SIZE);
+ namelen = strlen(sdata->filename);
+ if (namelen > IMA_EVENT_NAME_LEN_MAX)
+ namelen = IMA_EVENT_NAME_LEN_MAX;
+ memcpy(measure_entry.file_name, sdata->filename, namelen);
+
+ result = ima_store_template(&template);
+ if (!result || result == -EEXIST) {
+ iint->flags |= IMA_MEASURED;
+ iint->version = template.inode->i_version;
+ }
+ mutex_unlock(&iint->mutex);
+ kref_put(&iint->refcount, iint_free);
+}
diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
new file mode 100644
index 0000000..79d591c
--- /dev/null
+++ b/security/integrity/ima/ima_crypto.c
@@ -0,0 +1,102 @@
+/*
+ * Copyright (C) 2005,2006,2007,2008 IBM Corporation
+ *
+ * Authors:
+ * Mimi Zohar <[email protected]>
+ * Kylene Hall <[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_crypto.c
+ * Calculate a file's or a template's hash.
+ */
+
+#include <linux/kernel.h>
+#include <linux/file.h>
+#include <linux/crypto.h>
+#include <linux/scatterlist.h>
+#include "ima.h"
+
+/*
+ * Calculate the MD5/SHA1 digest
+ */
+int ima_calc_hash(struct file *file, char *digest)
+{
+ struct hash_desc desc;
+ struct scatterlist sg[1];
+ loff_t i_size;
+ char *rbuf;
+ int rc, offset = 0;
+
+ desc.tfm = crypto_alloc_hash(ima_hash, 0, CRYPTO_ALG_ASYNC);
+ if (IS_ERR(desc.tfm)) {
+ ima_info("failed to load %s transform: %ld\n",
+ ima_hash, PTR_ERR(desc.tfm));
+ rc = PTR_ERR(desc.tfm);
+ return rc;
+ }
+ desc.flags = 0;
+ rc = crypto_hash_init(&desc);
+ if (rc)
+ goto out;
+
+ rbuf = kzalloc(PAGE_SIZE, GFP_KERNEL);
+ if (!rbuf) {
+ rc = -ENOMEM;
+ goto out;
+ }
+ i_size = i_size_read(file->f_dentry->d_inode);
+ while (offset < i_size) {
+ int rbuf_len;
+
+ rbuf_len = kernel_read(file, offset, rbuf, PAGE_SIZE);
+ if (rbuf_len < 0) {
+ rc = rbuf_len;
+ break;
+ }
+ offset += rbuf_len;
+ sg_set_buf(sg, rbuf, rbuf_len);
+
+ rc = crypto_hash_update(&desc, sg, rbuf_len);
+ if (rc)
+ break;
+ }
+ kfree(rbuf);
+ if (!rc)
+ rc = crypto_hash_final(&desc, digest);
+out:
+ crypto_free_hash(desc.tfm);
+ return rc;
+}
+
+/*
+ * Calculate the hash of a given template
+ */
+int ima_calc_template_hash(int template_len, char *template, char *digest)
+{
+ struct hash_desc desc;
+ struct scatterlist sg[1];
+ int rc;
+
+ desc.tfm = crypto_alloc_hash(ima_hash, 0, CRYPTO_ALG_ASYNC);
+ if (IS_ERR(desc.tfm)) {
+ ima_info("failed to load %s transform: %ld\n",
+ ima_hash, PTR_ERR(desc.tfm));
+ rc = PTR_ERR(desc.tfm);
+ return rc;
+ }
+ desc.flags = 0;
+ rc = crypto_hash_init(&desc);
+ if (rc)
+ goto out;
+
+ sg_set_buf(sg, template, template_len);
+ rc = crypto_hash_update(&desc, sg, template_len);
+ if (!rc)
+ rc = crypto_hash_final(&desc, digest);
+out:
+ crypto_free_hash(desc.tfm);
+ return rc;
+}
diff --git a/security/integrity/ima/ima_iint.c b/security/integrity/ima/ima_iint.c
new file mode 100644
index 0000000..420855c
--- /dev/null
+++ b/security/integrity/ima/ima_iint.c
@@ -0,0 +1,71 @@
+/*
+ * Copyright (C) 2008 IBM Corporation
+ *
+ * Authors:
+ * 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_iint.c
+ * cache integrity information associated with an inode
+ * using a radix tree.
+ */
+#include <linux/module.h>
+#include <linux/spinlock.h>
+#include <linux/radix-tree.h>
+#include "ima.h"
+
+RADIX_TREE(ima_iint_store, GFP_ATOMIC);
+DEFINE_SPINLOCK(ima_iint_lock);
+
+/* Call with rcu_read_lock */
+struct ima_iint_cache *ima_iint_lookup(struct inode *inode)
+{
+ return radix_tree_lookup(&ima_iint_store, (unsigned long)inode);
+}
+
+int ima_iint_insert(struct inode *inode)
+{
+ struct ima_iint_cache *iint;
+ int rc = 0;
+
+ iint = kzalloc(sizeof(*iint), GFP_KERNEL);
+ if (!iint)
+ return -ENOMEM;
+ mutex_init(&iint->mutex);
+ kref_set(&iint->refcount, 1);
+ iint->version = inode->i_version;
+
+ rc = radix_tree_preload(GFP_KERNEL);
+ if (rc < 0) {
+ kfree(iint);
+ return rc;
+ }
+
+ spin_lock(&ima_iint_lock);
+ rc = radix_tree_insert(&ima_iint_store, (unsigned long)inode, iint);
+ spin_unlock(&ima_iint_lock);
+ radix_tree_preload_end();
+ return rc;
+}
+
+void iint_free(struct kref *kref)
+{
+ struct ima_iint_cache *iint = container_of(kref, struct ima_iint_cache,
+ refcount);
+ kfree(iint);
+}
+
+void ima_iint_delete(struct inode *inode)
+{
+ struct ima_iint_cache *iint;
+
+ spin_lock(&ima_iint_lock);
+ iint = radix_tree_delete(&ima_iint_store, (unsigned long)inode);
+ spin_unlock(&ima_iint_lock);
+ if (iint)
+ kref_put(&iint->refcount, iint_free);
+}
diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c
new file mode 100644
index 0000000..374b368
--- /dev/null
+++ b/security/integrity/ima/ima_init.c
@@ -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);
+ 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) {
+ int i;
+ u8 pcr_i[IMA_DIGEST_SIZE];
+ struct hash_desc desc;
+ struct crypto_hash *tfm;
+ struct scatterlist sg;
+
+ tfm = crypto_alloc_hash(ima_hash, 0, CRYPTO_ALG_ASYNC);
+ if (!tfm || IS_ERR(tfm)) {
+ ima_error("error initializing digest.\n");
+ return;
+ }
+ desc.tfm = tfm;
+ desc.flags = 0;
+ crypto_hash_init(&desc);
+
+ /* cumulative sha1 over tpm registers 0-7 */
+ for (i = 0; i < 8; i++) {
+ 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)
+{
+ int rc;
+
+ ima_used_chip = 0;
+ rc = tpm_pcr_read(IMA_TPM, 0, NULL);
+ if (rc == 0)
+ ima_used_chip = 1;
+
+ if (!ima_used_chip)
+ ima_info("No TPM chip found(rc = %d), activating TPM-bypass!\n",
+ rc);
+
+ ima_create_htable(); /* for measurements */
+ ima_add_boot_aggregate(); /* boot aggregate must be first entry */
+ ima_init_policy();
+ return 0;
+}
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
new file mode 100644
index 0000000..2dcc3b5
--- /dev/null
+++ b/security/integrity/ima/ima_main.c
@@ -0,0 +1,274 @@
+/*
+ * Copyright (C) 2005,2006,2007,2008 IBM Corporation
+ *
+ * Authors:
+ * Reiner Sailer <[email protected]>
+ * Serge Hallyn <[email protected]>
+ * Kylene Hall <[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_main.c
+ * implements the IMA LIM hooks
+ */
+#include <linux/module.h>
+#include <linux/file.h>
+#include <linux/mount.h>
+#include <linux/mman.h>
+
+#include "ima.h"
+
+char *ima_hash = "sha1";
+static int __init hash_setup(char *str)
+{
+ char *op = "setup";
+ char *hash = "sha1";
+
+ if (strncmp(str, "md5", 3) == 0) {
+ op = "setup";
+ hash = "md5";
+ ima_hash = str;
+ } else if (strncmp(str, "sha1", 4) != 0) {
+ op = "hash_setup";
+ hash = "invalid_hash_type";
+ }
+ integrity_audit_msg(AUDIT_INTEGRITY_HASH, NULL, NULL, op, hash, 0);
+ return 1;
+}
+__setup("ima_hash=", hash_setup);
+
+/**
+ * ima_file_free - called on close
+ * @file: pointer to file being closed
+ *
+ * Flag files that changed, based on i_version.
+ */
+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;
+ mutex_unlock(&iint->mutex);
+ kref_put(&iint->refcount, iint_free);
+ }
+}
+
+/**
+ * ima_path_check_integrity - based on policy, collect/store measurement.
+ * @path: contains a pointer to the path to be measured
+ * @mask: contains MAY_READ, MAY_WRITE or MAY_EXECUTE
+ *
+ * Measure the file being open for read, based on the ima_must_measure()
+ * policy decision.
+ *
+ * Invalidate the PCR:
+ * - Opening a file for write when already open for read,
+ * results in a time of measure, time of use (ToMToU) error.
+ * - Opening a file for read when already open for write,
+ * could result in a file measurement error.
+ *
+ * Return 0 on success, an error code on failure.
+ * (Based on the results of appraise_measurement().)
+ */
+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) {
+ 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");
+ }
+ return 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;
+ sdata.filename =
+ file->f_dentry->d_name.name;
+ ima_store_measurement(&sdata);
+ }
+ fput(file);
+ }
+ }
+ }
+ return 0;
+}
+
+/**
+ * ima_file_mmap - based on policy, collect/store measurement.
+ * @file: pointer to the file to be measured (May be NULL)
+ * @prot contains the protection that will be applied by the kernel.
+ *
+ * Measure files being mmapped executable based on the ima_must_measure()
+ * policy decision.
+ *
+ * Return 0 on success, an error code on failure.
+ * (Based on the results of appraise_measurement().)
+ */
+static int ima_file_mmap(struct file *file, unsigned long prot)
+{
+ struct ima_measure_data mdata;
+ int rc = 0;
+
+ if (!file)
+ return rc;
+ if (!(prot & VM_EXEC))
+ return rc;
+
+ memset(&mdata, 0, sizeof mdata);
+ mdata.inode = file->f_dentry->d_inode;
+ mdata.mask = MAY_EXEC;
+ mdata.function = FILE_MMAP;
+
+ rc = ima_must_measure(&mdata);
+ if (!rc) {
+ rc = ima_collect_measurement(file);
+ if (!rc) {
+ struct ima_store_data sdata;
+
+ sdata.file = file;
+ sdata.filename = file->f_dentry->d_name.name;
+ ima_store_measurement(&sdata);
+ }
+ }
+ return 0;
+}
+
+/**
+ * ima_bprm_check_integrity - based on policy, collect/store measurement.
+ * @bprm: contains the linux_binprm structure
+ *
+ * The OS protects against an executable file, already open for write,
+ * from being executed in deny_write_access() and an executable file,
+ * already open for execute, from being modified in get_write_access().
+ * So we can be certain that what we verify and measure here is actually
+ * what is being executed.
+ *
+ * Return 0 on success, an error code on failure.
+ * (Based on the results of appraise_measurement().)
+ */
+static int ima_bprm_check_integrity(struct linux_binprm *bprm)
+{
+ struct ima_measure_data mdata;
+ int rc = 0;
+
+ memset(&mdata, 0, sizeof mdata);
+ mdata.inode = bprm->file->f_dentry->d_inode;
+ mdata.mask = MAY_EXEC;
+ mdata.function = BPRM_CHECK;
+
+ rc = ima_must_measure(&mdata);
+ if (!rc) {
+ rc = ima_collect_measurement(bprm->file);
+ if (!rc) {
+ struct ima_store_data sdata;
+
+ sdata.file = bprm->file;
+ sdata.filename = bprm->filename;
+ ima_store_measurement(&sdata);
+ }
+ }
+ return 0;
+}
+
+static const struct integrity_operations ima_integrity_ops = {
+ .bprm_check_integrity = ima_bprm_check_integrity,
+ .path_check_integrity = ima_path_check_integrity,
+ .inode_alloc_integrity = ima_iint_insert,
+ .inode_free_integrity = ima_iint_delete,
+ .file_free_integrity = ima_file_free,
+ .file_mmap = ima_file_mmap,
+};
+
+/* After the TPM is available, start IMA
+ */
+static int __init init_ima(void)
+{
+ int error;
+
+ error = ima_init();
+ if (error)
+ goto out;
+
+ error = register_integrity(&ima_integrity_ops);
+ if (error)
+ goto out;
+
+ integrity_register_template("ima", &ima_template_ops);
+out:
+ return error;
+}
+
+static void __exit cleanup_ima(void)
+{
+ integrity_unregister_template("ima");
+ unregister_integrity(&ima_integrity_ops);
+}
+
+late_initcall(init_ima); /* Start IMA after the TPM is available */
+module_exit(cleanup_ima);
+
+MODULE_DESCRIPTION("Integrity Measurement Architecture");
+MODULE_LICENSE("GPL");
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
new file mode 100644
index 0000000..260f71c
--- /dev/null
+++ b/security/integrity/ima/ima_policy.c
@@ -0,0 +1,124 @@
+/*
+ * Copyright (C) 2008 IBM Corporation
+ * Author: 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.
+ *
+ * ima_policy.c
+ * - initialize default measure policy rules
+ *
+ */
+#include <linux/module.h>
+#include <linux/list.h>
+#include <linux/audit.h>
+#include <linux/security.h>
+#include <linux/integrity.h>
+#include <linux/magic.h>
+
+#include "ima.h"
+
+struct ima_measure_rule_entry {
+ struct list_head list;
+ int action;
+ unsigned int flags;
+ enum lim_hooks func;
+ int mask;
+ unsigned long fsmagic;
+ uid_t uid;
+};
+
+/* flags definitions */
+#define IMA_FUNC 0x0001
+#define IMA_MASK 0x0002
+#define IMA_FSMAGIC 0x0004
+#define IMA_UID 0x0008
+
+/* Without LSM specific knowledge, default policy can only be
+ * written in terms of .action, .func, .mask, .fsmagic, and .uid
+ */
+static struct ima_measure_rule_entry default_rules[] = {
+ {.action = DONT_MEASURE,.fsmagic = PROC_SUPER_MAGIC,
+ .flags = IMA_FSMAGIC},
+ {.action = DONT_MEASURE,.fsmagic = SYSFS_MAGIC,.flags = IMA_FSMAGIC},
+ {.action = DONT_MEASURE,.fsmagic = DEBUGFS_MAGIC,.flags = IMA_FSMAGIC},
+ {.action = DONT_MEASURE,.fsmagic = TMPFS_MAGIC,.flags = IMA_FSMAGIC},
+ {.action = DONT_MEASURE,.fsmagic = SECURITYFS_MAGIC,
+ .flags = IMA_FSMAGIC},
+ {.action = MEASURE,.func = FILE_MMAP,.mask = MAY_EXEC,
+ .flags = IMA_FUNC | IMA_MASK},
+ {.action = MEASURE,.func = BPRM_CHECK,.mask = MAY_EXEC,
+ .flags = IMA_FUNC | IMA_MASK},
+ {.action = MEASURE,.func = PATH_CHECK,.mask = MAY_READ,.uid = 0,
+ .flags = IMA_FUNC | IMA_MASK | IMA_UID}
+};
+
+static struct list_head measure_default_rules;
+static struct list_head *ima_measure;
+
+/**
+ * ima_match_rules - determine whether an inode matches the measure rule.
+ * @rule: a pointer to a rule
+ * @inode: a pointer to an inode
+ * @func: LIM hook identifier
+ * @mask: requested action (MAY_READ | MAY_WRITE | MAY_APPEND | MAY_EXEC)
+ *
+ * Returns true on rule match, false on failure.
+ */
+static bool ima_match_rules(struct ima_measure_rule_entry *rule,
+ struct inode *inode, enum lim_hooks func, int mask)
+{
+ struct task_struct *tsk = current;
+
+ if ((rule->flags & IMA_FUNC) && rule->func != func)
+ return false;
+ if ((rule->flags & IMA_MASK) && rule->mask != mask)
+ return false;
+ if ((rule->flags & IMA_FSMAGIC)
+ && rule->fsmagic != inode->i_sb->s_magic)
+ return false;
+ if ((rule->flags & IMA_UID) && rule->uid != tsk->uid)
+ return false;
+ return true;
+}
+
+/**
+ * ima_match_policy - decision based on LSM and other conditions
+ * @inode: pointer to an inode
+ * @func: IMA hook identifier
+ * @mask: requested action (MAY_READ | MAY_WRITE | MAY_APPEND | MAY_EXEC)
+ *
+ * Measure decision based on func/mask/fsmagic and LSM(subj/obj/type)
+ * conditions. Returns rule action on rule match, 0 on failure.
+ */
+int ima_match_policy(struct inode *inode, enum lim_hooks func, int mask)
+{
+ struct ima_measure_rule_entry *entry;
+
+ rcu_read_lock();
+ list_for_each_entry_rcu(entry, ima_measure, list) {
+ bool rc;
+
+ rc = ima_match_rules(entry, inode, func, mask);
+ if (rc) {
+ rcu_read_unlock();
+ return entry->action;
+ }
+ }
+ rcu_read_unlock();
+ return 0;
+}
+
+/**
+ * ima_init_policy - initialize the default and policy measure rules.
+ */
+void ima_init_policy(void)
+{
+ int i;
+
+ INIT_LIST_HEAD(&measure_default_rules);
+ for (i = 0; i < ARRAY_SIZE(default_rules); i++)
+ list_add_tail(&default_rules[i].list, &measure_default_rules);
+ ima_measure = &measure_default_rules;
+}
diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c
new file mode 100644
index 0000000..4224920
--- /dev/null
+++ b/security/integrity/ima/ima_queue.c
@@ -0,0 +1,125 @@
+/*
+ * Copyright (C) 2005,2006,2007,2008 IBM Corporation
+ *
+ * Authors:
+ * Serge Hallyn <[email protected]>
+ * Reiner Sailer <[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_queue.c
+ * implements queues that store IMA measurements and
+ * maintains aggregate over the stored measurements
+ * in the pre-configured TPM PCR (if available)
+ * The measurement list is append-only. No entry is
+ * ever removed or changed during the boot-cycle.
+ */
+#include <linux/module.h>
+
+#include "ima.h"
+
+struct list_head ima_measurements; /* list of all measurements */
+struct ima_h_table ima_htable; /* key: inode (before secure-hashing a file) */
+
+/* mutex protects atomicity of extending measurement list
+ * and extending the TPM PCR aggregate. Since tpm_extend can take
+ * long (and the tpm driver uses a mutex), we can't use the spinlock.
+ */
+static DEFINE_MUTEX(ima_extend_list_mutex);
+
+void ima_create_htable(void)
+{
+ int i;
+
+ INIT_LIST_HEAD(&ima_measurements);
+ atomic_set(&ima_htable.len, 0);
+ atomic_long_set(&ima_htable.violations, 0);
+ ima_htable.max_htable_size = IMA_MEASURE_HTABLE_SIZE;
+
+ for (i = 0; i < ima_htable.max_htable_size; i++) {
+ INIT_HLIST_HEAD(&ima_htable.queue[i]);
+ atomic_set(&ima_htable.queue_len[i], 0);
+ }
+}
+
+static struct ima_queue_entry *ima_lookup_digest_entry(u8 *digest_value)
+{
+ struct ima_queue_entry *qe, *ret = NULL;
+ unsigned int key;
+ struct hlist_node *pos;
+
+ key = IMA_HASH_KEY(digest_value);
+ rcu_read_lock();
+ hlist_for_each_entry_rcu(qe, pos, &ima_htable.queue[key], hnext) {
+ if (memcmp(qe->entry->digest, digest_value, 20) == 0) {
+ ret = qe;
+ break;
+ }
+ }
+ rcu_read_unlock();
+ return ret;
+}
+
+/* Called with mutex held */
+static int ima_add_digest_entry(struct ima_template_entry *entry)
+{
+ struct ima_queue_entry *qe;
+ unsigned int key;
+
+ key = IMA_HASH_KEY(entry->digest);
+ qe = kmalloc(sizeof(*entry), GFP_KERNEL);
+ if (qe == NULL) {
+ ima_error("OUT OF MEMORY ERROR creating queue entry.\n");
+ return -ENOMEM;
+ }
+ qe->entry = entry;
+
+ hlist_add_head_rcu(&qe->hnext, &ima_htable.queue[key]);
+ atomic_inc(&ima_htable.queue_len[key]);
+ return 0;
+}
+
+int ima_add_template_entry(struct ima_template_entry *entry, int violation)
+{
+ struct ima_queue_entry *qe;
+ int error = 0;
+
+ mutex_lock(&ima_extend_list_mutex);
+ if (!violation) {
+ if (ima_lookup_digest_entry(entry->digest)) {
+ error = -EEXIST;
+ goto out;
+ }
+ }
+ qe = kmalloc(sizeof(struct ima_queue_entry), GFP_KERNEL);
+ if (qe == NULL) {
+ ima_error("OUT OF MEMORY in %s.\n", __func__);
+ error = -ENOMEM;
+ goto out;
+ }
+ qe->entry = entry;
+
+ INIT_LIST_HEAD(&qe->later);
+ list_add_tail_rcu(&qe->later, &ima_measurements);
+
+ atomic_long_inc(&ima_htable.len);
+ if (ima_add_digest_entry(entry)) {
+ kfree(qe);
+ error = -ENOMEM;
+ goto out;
+ }
+ if (violation) { /* Replace 0x00 with 0xFF */
+ u8 digest[IMA_DIGEST_SIZE];
+
+ memset(digest, 0xff, sizeof digest);
+ ima_extend(digest);
+ } else
+ ima_extend(entry->digest);
+out:
+ mutex_unlock(&ima_extend_list_mutex);
+ return error;
+}
--
1.5.6.5
On Tue, 2008-12-02 at 16:47 -0500, Mimi Zohar wrote:
> 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]>
> ---
> diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c
> index 9c47dc4..17d2849 100644
> --- a/drivers/char/tpm/tpm.c
> +++ b/drivers/char/tpm/tpm.c
> @@ -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;
> + 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;
> +
> +/**
> + * 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;
> + }
> + 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?
> + 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();
> +
> + 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.
> + 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.
> +enum tpm_chip_num {
> + TPM_ANY_NUM = 0xFFFF,
> +};
Why bother even checking this sucker if there's only one value?
> +#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
On Tue, 2008-12-02 at 16:47 -0500, Mimi Zohar wrote:
> @@ -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;
> }
> return inode;
> +
> +out_free_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;
> }
You were saying that this is a very hard patch set to break up. So, I'm
trying to find some places that could trim a line or two here and there.
Stuff like this is a primary example.
Pulling that security_inode_alloc() 'if' condition out and sticking it
at the bottom of the function is a change that can stand on its own.
You could put it up at the top of your series, or even send it
separately. It makes this patch smaller and more obvious then.
> +#endif
> +#endif
Personally, I love to see comments on these suckers after a long header
file. My memory sucks.
> +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?
> +/**
> + * 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);
> + 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?
> +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;
> + }
> + }
> + 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().
> +/**
> + * 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;
> + }
> + }
> + 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.
> +int integrity_collect_measurement(const char *template_name, void *data)
> +{
> + struct template_list_entry *template_entry;
> + int rc = -EINVAL;
> +
> + 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:
+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
Mimi Zohar wrote:
> 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]>
> ---
> diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c
> index 9c47dc4..17d2849 100644
> --- a/drivers/char/tpm/tpm.c
> +++ b/drivers/char/tpm/tpm.c
> @@ -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
[...]
> +#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 {
> + TPM_ANY_NUM = 0xFFFF,
> +};
> +
> +
> +#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
Minor nits:
* this is a bit schizophrenic with regards to defining named constants;
sometimes enum is used, other times #define is used. It would be better
to just use enum, which is what the current code does (see top of
pre-patched tpm.c)
* you really shouldn't hide PCI_DEVICE_ID_xxx constants in places other
than pci_ids.h.
* furthermore, is that constant even used? used in a later patch? if
the constant is only used once, e.g. in a pci_device_list list, then
consider eliminating the constant altogether and directly using the PCI
device id hexidecimal number in the target location.
Jeff
On Tue, 2008-12-02 at 16:47 -0500, Mimi Zohar wrote:
> index 0000000..6c6fcd9
> --- /dev/null
> +++ b/security/integrity/ima/Kconfig
> @@ -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
> + 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?
> +config IMA_MEASURE_PCR_IDX
> + int "PCR for Aggregate (8 <= Index <= 14)"
> + depends on IMA
> + range 8 14
> + 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. :(
> +#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?
> +/* 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?
> +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.
> +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.
> +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.
> + 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();
> + 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.
> + 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();
> + 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?
> +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.
> +void ima_iint_delete(struct inode *inode)
> +{
> + struct ima_iint_cache *iint;
> +
> + spin_lock(&ima_iint_lock);
> + iint = radix_tree_delete(&ima_iint_store, (unsigned long)inode);
> + spin_unlock(&ima_iint_lock);
> + if (iint)
> + kref_put(&iint->refcount, iint_free);
> +}
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.
> diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c
> new file mode 100644
> index 0000000..374b368
> --- /dev/null
> +++ b/security/integrity/ima/ima_init.c
> @@ -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);
> + 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) {
> + 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?
> + tfm = crypto_alloc_hash(ima_hash, 0, CRYPTO_ALG_ASYNC);
> + if (!tfm || IS_ERR(tfm)) {
> + ima_error("error initializing digest.\n");
> + return;
> + }
> + 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.
> + 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)
> +{
> + int rc;
> +
> + ima_used_chip = 0;
> + rc = tpm_pcr_read(IMA_TPM, 0, NULL);
> + if (rc == 0)
> + ima_used_chip = 1;
> +
> + 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.
> +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;
> + 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.
> +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?
> + 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");
> + }
> + 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?
Could you please break this out into a separate function where you have
some room to comment it and don't have to wrap it at 80 columns so
badly?
> + /* 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;
> + sdata.filename =
> + file->f_dentry->d_name.name;
> + ima_store_measurement(&sdata);
> + }
> + fput(file);
> + }
> + }
> + }
> + return 0;
> +}
I really think this hurts readability. I also see three callers to
ima_collect_measurement() that do the exact same thing.
Mimi, I'm really getting the idea that this patch hasn't had much work
put into its structure. I'm going to stop reviewing it now because I'm
running into the same issues again and again and I think these issues
are really slowing down my ability to look at it.
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.
If I've misunderstood things in your code, please go back and comment
them. If I got confused, surely someone else could.
-- Dave
On Tue, Dec 02, 2008 at 04:47:56PM -0500, Mimi Zohar wrote:
> +/*
> + * 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.
> + *
> + */
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.
On Tue, Dec 02, 2008 at 03:35:25PM -0800, Dave Hansen wrote:
> 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.
> > + 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.
Or just don't bother printing the useless and ugly rc value and you're
under it immediately..
> > + 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;
> > + 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..
> > +
> > + /* 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;
Umm, no. The above is a perfectly fine idiom for testing flags in C.
The helper would just obsfucated it.
> 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.
> 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.
On Wed, 2008-12-03 at 08:03 -0500, Christoph Hellwig wrote:
> On Tue, Dec 02, 2008 at 03:35:25PM -0800, Dave Hansen wrote:
> > 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.
If Christoph and I tell you conflicting things, it goes without saying
to listen to Christoph. :)
-- Dave
On Wed, Dec 03, 2008 at 08:55:54AM -0800, Dave Hansen wrote:
> On Wed, 2008-12-03 at 08:03 -0500, Christoph Hellwig wrote:
> > On Tue, Dec 02, 2008 at 03:35:25PM -0800, Dave Hansen wrote:
> > > 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.
>
> If Christoph and I tell you conflicting things, it goes without saying
> to listen to Christoph. :)
Or listen to what makes more sense. It's not the most important comment
of the review anyway..
Quoting Mimi Zohar ([email protected]):
> 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]>
(/me wonders why I can't seem to get any direct emails from you)
Dave's comments are good, and I'm sure you'll address them in a followup :),
but the basic chip lookup idiom (which as Dave says should be consolidated)
does look correct.
Acked-by: Serge Hallyn <[email protected]>
-serge
On Tue, 2008-12-02 at 14:43 -0800, Dave Hansen wrote:
> On Tue, 2008-12-02 at 16:47 -0500, Mimi Zohar wrote:
> > +#endif
> > +#endif
>
> Personally, I love to see comments on these suckers after a long header
> file. My memory sucks.
ok
> > +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.
> > +/**
> > + * 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);
> > + 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?
good question.
> > +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;
> > + }
> > + }
> > + 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_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;
> > + }
> > + }
> > + 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.
none, will do.
> > +int integrity_collect_measurement(const char *template_name, void *data)
> > +{
> > + struct template_list_entry *template_entry;
> > + int rc = -EINVAL;
> > +
> > + 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:
>
> +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
Yes, will re-factor the code. Thanks!
Mimi
On Tue, 2008-12-02 at 15:35 -0800, Dave Hansen wrote:
> On Tue, 2008-12-02 at 16:47 -0500, Mimi Zohar wrote:
> > index 0000000..6c6fcd9
> > --- /dev/null
> > +++ b/security/integrity/ima/Kconfig
> > @@ -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
> > + 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?
Yes, that is a clearer description. Thanks.
> > +config IMA_MEASURE_PCR_IDX
> > + int "PCR for Aggregate (8 <= Index <= 14)"
> > + depends on IMA
> > + range 8 14
> > + 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.
> > +#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?
will clean this up.
> > +/* 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?
>
As the kernel command line option "ima_hash=" verifies the crypto
algorithm specified, changes to the code would be required to support a
new algorithm anyway.
> > +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.
Ok
> > +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.
>
> > +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.
Thank you. will be in the next set patches.
> > + 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();
> > + 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.
cleaner, thanks.
> > + 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();
> > + 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?
After the ima_iint_find_get() recommendation above, this becomes a lot
smaller.
> > +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.
> > +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) {
> > + 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?
> > + tfm = crypto_alloc_hash(ima_hash, 0, CRYPTO_ALG_ASYNC);
> > + if (!tfm || IS_ERR(tfm)) {
> > + ima_error("error initializing digest.\n");
> > + return;
> > + }
> > + 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.
Will look.
> > +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;
> > + 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.
Unlike the ima_iint_lookup(), the code within the mutex locking differs
between calls.
> > +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?
In addition to MAY_READ/MAY_WRITE, there might be other flags.
Mimi
On Wed, 2008-12-03 at 07:30 -0500, Christoph Hellwig wrote:
> On Tue, Dec 02, 2008 at 04:47:56PM -0500, Mimi Zohar wrote:
> > +/*
> > + * 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.
> > + *
> > + */
>
> 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.
Mimi
On Wed, Dec 03, 2008 at 01:18:43PM -0500, Mimi Zohar wrote:
> 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.
On Wed, 2008-12-03 at 08:03 -0500, Christoph Hellwig wrote:
> On Tue, Dec 02, 2008 at 03:35:25PM -0800, Dave Hansen wrote:
> > 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.
I'm not disagreeing, just letting you know that in this case, the first
IMA patch builds cleanly and works, with each of the subsequent patches
adding new functionality.
> > > + 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;
> > > + 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..
hm, i_version seems to be working properly for regular files on ext3.
>
> > 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).
> > 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.
The parameters to the integrity API calls (collect, measure, store, etc)
need to be generic (void *), as different templates measure different
types of data, requiring different parameters.
As Dave Hansen, pointed out, the casting of void pointers is
unnecessary. This will be cleaned up in the next patch set.
Mimi
On Wed, 2008-12-03 at 13:15 -0500, Mimi Zohar wrote:
> > > +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.
If it is a Kconfig issue, then we don't really need all the function
pointers and ops things. Just pick which integrity infrastructure you
want at compile time. You also wouldn't need that integrity_ops
function at all.
> > > +int integrity_register_template(const char *template_name,
> > > + const struct template_operations *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?
>
> good question.
So, you'll go investigate this, fix it up in a future patch and if you
decide it needs to be kept, it will be commented, right?
> > > +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;
> > > + }
> > > + }
> > > + 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.
synchronize_rcu() is an "expensive" operation in wall clock time. It
won't cost you a lot of CPU or anything, but it could cause this code to
block for a bit of time.
What you've said there is:
1. delete the template from the list
2. sleep until no one can possibly see the template any more
3. delete template
Call RCU would, instead, queue the template (and the kref_put()) in a
list. That list gets called in a batched manner once a grace period has
elapsed. The batching speeds things up, and it also doesn't block in
the caller.
I do see some other use like this in the kernel, but I have a suspicion
that there's a better way to do it. Could you have Paul McKenney take a
quick look? Dipankar or Josh Triplett seem to also love to look at
RCU. :)
-- Dave
On Wed, 2008-12-03 at 13:17 -0500, Mimi Zohar wrote:
> On Tue, 2008-12-02 at 15:35 -0800, Dave Hansen wrote:
> > > +config IMA_MEASURE_PCR_IDX
> > > + int "PCR for Aggregate (8 <= Index <= 14)"
> > > + depends on IMA
> > > + range 8 14
> > > + 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.
OK. Could you take out the prompt for now? You can use Kconfig for
values that don't give user prompts. I just don't think it is something
that people need to see.
in mm/Kconfig, for instance:
config NR_QUICK
int
depends on QUICKLIST
default "2" if SUPERH || AVR32
default "1"
> > > +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.
I'd be willing to bet that you'll see a measurable performance
improvement if you decide to use a slab here. All of the inodes for the
different fs's use slabs and these are at least as common as any single
fs's inode. Also, using the con/destructors will save some work at each
object creation.
> > > +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) {
> > > + 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?
That's reassuring, thanks. :)
-- Dave
On Wed, 2008-12-03 at 13:24 -0500, Mimi Zohar wrote:
> On Wed, 2008-12-03 at 08:03 -0500, Christoph Hellwig wrote:
> > On Tue, Dec 02, 2008 at 03:35:25PM -0800, Dave Hansen wrote:
> > > 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."
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.
> 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.
-- Dave
On Tue, 2 Dec 2008, Mimi Zohar wrote:
> +config IMA
> + bool "Integrity Measurement Architecture(IMA)"
> + depends on INTEGRITY
> + depends on ACPI
ACPI? why?
--
-Len Brown
Intel Open Source Technology Center
Quoting Christoph Hellwig ([email protected]):
> On Tue, Dec 02, 2008 at 04:47:56PM -0500, Mimi Zohar wrote:
> > +/*
> > + * 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.
This comment isn't right - return 0 if a measurement must be
taken, no?
> > + *
> > + * @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,
I'm not sure what you mean here - must_measure for instance is used (in
patch 3) in the integrity hooks (i.e. file_mmap) to decide whether or not
the object (action target) must be measured.
> and that seems to be better
> implented directly as a securityfs file, without the indirection.
That comment doesn't make sense to me (unless you're saying to punt
on the generic integrity infrastructure and hook all of the IMA
code straight into the kernel) so I suspect I'm misreading
something.
-serge
On Wed, Dec 03, 2008 at 02:13:20PM -0600, Serge E. Hallyn wrote:
> > Can you explain what all this template stuff is about? The only method
> > of these ever called is display_template,
>
> I'm not sure what you mean here - must_measure for instance is used (in
> patch 3) in the integrity hooks (i.e. file_mmap) to decide whether or not
> the object (action target) must be measured.
ima_must_measure (or the other implementation bits) are called a lot.
But never through the indirection I quoted.
> > and that seems to be better
> > implented directly as a securityfs file, without the indirection.
>
> That comment doesn't make sense to me (unless you're saying to punt
> on the generic integrity infrastructure and hook all of the IMA
> code straight into the kernel) so I suspect I'm misreading
> something.
ima_measurements_show alaways calls ima_template_show, and both are
implemented inside the ima module. There's absolutely no point for the
indirection here.
On Tue, 2008-12-02 at 16:47 -0500, Mimi Zohar wrote:
> 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.
I think this needs a bit of a plain-text explanation, sans acronyms.
Perhaps a real-world example with files like /etc/passwd or /bin/sh
would help me understand it better.
I'm still trying to wrap my head around the whole "invalidate something
when there are both readers and writers around" situation. How does
that fit into the description above? Does that tie into the guarantee
that "IMA measures all files before they are accessed in any way"?
-- Dave
On Wed, 2008-12-03 at 13:23 -0500, Christoph Hellwig wrote:
> On Wed, Dec 03, 2008 at 01:18:43PM -0500, Mimi Zohar wrote:
> > 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.
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
On Wed, Dec 03, 2008 at 05:17:35PM -0500, Mimi Zohar wrote:
> > 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.
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.
>
> Mimi
>
>
>
---end quoted text---
On Wed, 2008-12-03 at 14:01 -0500, Len Brown wrote:
>
> On Tue, 2 Dec 2008, Mimi Zohar wrote:
>
> > +config IMA
> > + bool "Integrity Measurement Architecture(IMA)"
> > + depends on INTEGRITY
> > + depends on ACPI
>
> ACPI? why?
It's needed for accessing the TPM.
Mimi
On Wed, 2008-12-03 at 10:50 -0800, Dave Hansen wrote:
> On Wed, 2008-12-03 at 13:24 -0500, Mimi Zohar wrote:
> > On Wed, 2008-12-03 at 08:03 -0500, Christoph Hellwig wrote:
> > > On Tue, Dec 02, 2008 at 03:35:25PM -0800, Dave Hansen wrote:
> > > > 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."
>
> 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.
>
> > 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.
The original IMA maintained its own readcount, but we thought the code
would be simpler if we used existing kernel reference counts. Based on
both Christoph's and your concerns, we probably should go back to
maintaining our own readcount for user level files.
Mimi
Quoting Christoph Hellwig ([email protected]):
> On Wed, Dec 03, 2008 at 05:17:35PM -0500, Mimi Zohar wrote:
> > > 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.
>
> 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.
Yeah I can definately see that.
Mimi, you used to have another template (I thought) which just tracked
security_ops to try and prevent subversion of the LSM hooks. Or something like
that. That was a separate template_ops, right? Can you post that again? That
might answer both Christoph's query about the usefulness of the indirection,
and Dave's question about "how could I use this, anyway".
If you do repost it, please be very clear about what it is expected
to do/protect against, and how, using no acronyms which you don't
define on first use :)
thanks,
-serge
On Tue, 2008-12-02 at 14:19 -0800, Dave Hansen wrote:
> On Tue, 2008-12-02 at 16:47 -0500, Mimi Zohar wrote:
> > 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]>
> > ---
> > diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c
> > index 9c47dc4..17d2849 100644
> > --- a/drivers/char/tpm/tpm.c
> > +++ b/drivers/char/tpm/tpm.c
> > @@ -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;
> > + 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;
>
If that's so confusing, it will be included in the next patchset.
> > +
> > +/**
> > + * 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;
> > + }
> > + 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.
> 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.
> > + 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();
> > +
> > + 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.
>
10 is pcrextend's size, and 14 is pcrextend's size + pcr_idx's size.
Probably this little math will be clearer by defining this values
previously, assigning a meaningful name to them..
> > + 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.
>
It will be used, replacing the magical 6.
> 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.
>
> > +enum tpm_chip_num {
> > + TPM_ANY_NUM = 0xFFFF,
> > +};
>
> Why bother even checking this sucker if there's only one value?
>
> > +#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. :)
>
We just added another 1, Debora Velarde, the new maintainer, and it's
for the whole tpm.c, not just this patch.
Thanks!
Rajiv
> -- 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/
--
Rajiv Andrade <[email protected]>
Security Development
IBM Linux Technology Center
On Thu, 2008-12-04 at 08:09 -0500, Christoph Hellwig wrote:
>
> In integrity.h there are two operation vectors defines:
In the past, the integrity hooks were discussed with the
LSM/security community, and others expressed interest in using
them, and we were trying to accommodate their requests. On the
other hand, we have separately asked them to chime in here to
defend these hooks, and have heard nothing. As everyone has
pointed out, if no one uses them, they are bloat, and if
someone else wants them in the future, they can easily be
added back. In a little more detail:
> - 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.
> - 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.
These hooks were for potential use by LSM modules to inquire about the
integrity of files, and for other modules to be able to anchor data
in the TPM list. Again, since no one has chimed in to defend the
hooks, we have no problem removing them.
Sorry about arguing too long on this, and thanks for all the reviews...
dave
> > > +
> > > +/**
> > > + * 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;
> > > + }
> > > + 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.
>
> > 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.
>
Wait.. chip_num, the other parameter, depends on chip_id and a
previously defined constant, so, you are right, it's saner to just pass
chip_id to it.. sorry, and thanks. This will be included also on the
next patchset I'm about to send.
Rajiv
On Thu, 4 Dec 2008, david safford wrote:
> > - 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.
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 committments to push these upstream when the
integrity framework is merged?
- James
--
James Morris
<[email protected]>
On Fri, 2008-12-05 at 12:42 +1100, James Morris wrote:
> On Thu, 4 Dec 2008, david safford wrote:
>
> > 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?
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.
dave
Quoting david safford ([email protected]):
> On Fri, 2008-12-05 at 12:42 +1100, James Morris wrote:
> > On Thu, 4 Dec 2008, david safford wrote:
> >
> > > 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?
>
> 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
On Fri, 2008-12-05 at 09:23 -0600, Serge E. Hallyn wrote:
> Quoting david safford ([email protected]):
> > On Fri, 2008-12-05 at 12:42 +1100, James Morris wrote:
> > > On Thu, 4 Dec 2008, david safford wrote:
> > >
> > > > 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?
> >
> > 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
I believe they are using an older version of IMA without templating,
but either way really shouldn't affect them.
dave
On Tue, Dec 02, 2008 at 03:35:25PM -0800, Dave Hansen wrote:
>
> > + 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");
> > + }
> > + 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?
This bit is crap, plain and simple. d_count doesn't work as a proxy for
"how many times had we opened this file". At all.
a) stat(2) and just about anything else that looks funny at the pathname
will bump d_count.
b) there may be several links to given file; all will share inode and have
different dentries.
In other words, result of that comparison happens to be junk.