[PATCH 1/3] ima: keep the integrity state of open files up to date

[PATCH 1/3] ima: keep the integrity state of open files up to date

[Janne Karhunen]

When a file is open for writing, kernel crash or power outage
is guaranteed to corrupt the inode integrity state leading to
file appraisal failure on the subsequent boot. Add some basic
infrastructure to keep the integrity measurements up to date
as the files are written to.

Core file operations (open, close, sync, msync, truncate) are
now allowed to update the measurement immediately. In order
to maintain sufficient write performance for writes, add a
latency tunable delayed work workqueue for computing the
measurements.

Changelog v2:
- Make write measurements optional
- Add support for MMIO measurements
- Handle all writes via page flush
- Add work cancellation support, files are properly closed
  after last_writer checks out. This fixes a userspace break
  where the file was still open for writing after closing it.
- Fix/unify IMA_UPDATE_XATTR usage

Signed-off-by: Janne Karhunen <janne.karhunen@gmail.com>
Signed-off-by: Konsta Karsisto <konsta.karsisto@gmail.com>
---
 include/linux/ima.h                   |  14 +++
 security/integrity/ima/Kconfig        |  30 ++++++
 security/integrity/ima/ima.h          |  13 +++
 security/integrity/ima/ima_appraise.c |  13 ++-
 security/integrity/ima/ima_main.c     | 128 +++++++++++++++++++++++++-
 security/integrity/integrity.h        |  18 ++++
 6 files changed, 213 insertions(+), 3 deletions(-)

diff --git a/include/linux/ima.h b/include/linux/ima.h
index a20ad398d260..6736844e90d3 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -93,6 +93,20 @@ static inline void ima_post_path_mknod(struct dentry *dentry)
 static inline void ima_kexec_cmdline(const void *buf, int size) {}
 #endif /* CONFIG_IMA */
 
+#if ((defined CONFIG_IMA) && defined(CONFIG_IMA_MEASURE_WRITES))
+void ima_file_update(struct file *file);
+void ima_file_delayed_update(struct file *file);
+#else
+static inline void ima_file_update(struct file *file)
+{
+	return;
+}
+static inline void ima_file_delayed_update(struct file *file)
+{
+	return;
+}
+#endif
+
 #ifndef CONFIG_IMA_KEXEC
 struct kimage;
 
diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
index 897bafc59a33..df1cf684a442 100644
--- a/security/integrity/ima/Kconfig
+++ b/security/integrity/ima/Kconfig
@@ -310,3 +310,33 @@ config IMA_APPRAISE_SIGNED_INIT
 	default n
 	help
 	   This option requires user-space init to be signed.
+
+config IMA_MEASURE_WRITES
+	bool "Measure file writes (EXPERIMENTAL)"
+	depends on IMA
+	depends on EVM
+	default n
+	help
+	   By default IMA measures files only when they close or sync.
+	   Choose this option if you want the integrity measurements on
+	   the disk to update when the files are still open for writing.
+
+config IMA_MEASUREMENT_LATENCY
+	int
+	depends on IMA
+	range 0 60000
+	default 50
+	help
+	   This value defines the measurement interval when files are
+	   being written. Value is in milliseconds.
+
+config IMA_MEASUREMENT_LATENCY_CEILING
+	int
+	depends on IMA
+	range 100 60000
+	default 5000
+	help
+	   In order to maintain high write performance for large files,
+	   IMA increases the measurement interval as the file size grows.
+	   This value defines the ceiling for the measurement delay in
+	   milliseconds.
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 19769bf5f6ab..195e67631f70 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -160,6 +160,19 @@ void ima_init_template_list(void);
 int __init ima_init_digests(void);
 int ima_lsm_policy_change(struct notifier_block *nb, unsigned long event,
 			  void *lsm_data);
+#if ((defined CONFIG_IMA) && defined(CONFIG_IMA_MEASURE_WRITES))
+void ima_cancel_measurement(struct integrity_iint_cache *iint);
+#else
+static inline void ima_cancel_measurement(struct integrity_iint_cache *iint)
+{
+	return;
+}
+static inline void ima_init_measurement(struct integrity_iint_cache *iint,
+					struct dentry *dentry)
+{
+	return;
+}
+#endif
 
 /*
  * used to protect h_table and sha_table
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index 136ae4e0ee92..6c137fb5289b 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -78,6 +78,15 @@ static int ima_fix_xattr(struct dentry *dentry,
 	return rc;
 }
 
+#ifdef CONFIG_IMA_MEASURE_WRITES
+inline void ima_init_measurement(struct integrity_iint_cache *iint,
+				 struct dentry *dentry)
+{
+	if (test_bit(IMA_UPDATE_XATTR, &iint->atomic_flags))
+		ima_fix_xattr(dentry, iint);
+}
+#endif
+
 /* Return specific func appraised cached result */
 enum integrity_status ima_get_cache_status(struct integrity_iint_cache *iint,
 					   enum ima_hooks func)
@@ -341,8 +350,10 @@ int ima_appraise_measurement(enum ima_hooks func,
 			iint->flags |= IMA_NEW_FILE;
 		if ((iint->flags & IMA_NEW_FILE) &&
 		    (!(iint->flags & IMA_DIGSIG_REQUIRED) ||
-		     (inode->i_size == 0)))
+		    (inode->i_size == 0))) {
+			ima_init_measurement(iint, dentry);
 			status = INTEGRITY_PASS;
+		}
 		goto out;
 	}
 
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 79c01516211b..46d28cdb6466 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -12,7 +12,7 @@
  *
  * File: ima_main.c
  *	implements the IMA hooks: ima_bprm_check, ima_file_mmap,
- *	and ima_file_check.
+ *	ima_file_delayed_update, ima_file_update and ima_file_check.
  */
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
@@ -26,6 +26,8 @@
 #include <linux/xattr.h>
 #include <linux/ima.h>
 #include <linux/iversion.h>
+#include <linux/workqueue.h>
+#include <linux/sizes.h>
 #include <linux/fs.h>
 
 #include "ima.h"
@@ -42,6 +44,7 @@ static int hash_setup_done;
 static struct notifier_block ima_lsm_policy_notifier = {
 	.notifier_call = ima_lsm_policy_change,
 };
+static struct workqueue_struct *ima_update_wq;
 
 static int __init hash_setup(char *str)
 {
@@ -151,6 +154,7 @@ static void ima_check_last_writer(struct integrity_iint_cache *iint,
 
 	if (!(mode & FMODE_WRITE))
 		return;
+	ima_cancel_measurement(iint);
 
 	mutex_lock(&iint->mutex);
 	if (atomic_read(&inode->i_writecount) == 1) {
@@ -420,6 +424,117 @@ int ima_bprm_check(struct linux_binprm *bprm)
 				   MAY_EXEC, CREDS_CHECK);
 }
 
+#ifdef CONFIG_IMA_MEASURE_WRITES
+static unsigned long ima_inode_update_delay(struct inode *inode)
+{
+	unsigned long blocks, msecs;
+
+	blocks = i_size_read(inode) / SZ_1M + 1;
+	msecs = blocks * IMA_LATENCY_INCREMENT;
+	if (msecs > CONFIG_IMA_MEASUREMENT_LATENCY_CEILING)
+		msecs = CONFIG_IMA_MEASUREMENT_LATENCY_CEILING;
+
+	return msecs;
+}
+
+static void ima_delayed_update_handler(struct work_struct *work)
+{
+	struct ima_work_entry *entry;
+
+	entry = container_of(work, typeof(*entry), work.work);
+
+	ima_file_update(entry->file);
+	entry->file = NULL;
+	entry->state = IMA_WORK_INACTIVE;
+}
+
+void ima_cancel_measurement(struct integrity_iint_cache *iint)
+{
+	if (iint->ima_work.state != IMA_WORK_ACTIVE)
+		return;
+
+	cancel_delayed_work_sync(&iint->ima_work.work);
+	iint->ima_work.state = IMA_WORK_CANCELLED;
+}
+
+/**
+ * ima_file_delayed_update
+ * @file: pointer to file structure being updated
+ */
+void ima_file_delayed_update(struct file *file)
+{
+	struct inode *inode = file_inode(file);
+	struct integrity_iint_cache *iint;
+	unsigned long msecs;
+	bool creq;
+
+	if (!ima_policy_flag || !S_ISREG(inode->i_mode))
+		return;
+
+	iint = integrity_iint_find(inode);
+	if (!iint)
+		return;
+
+	if (!test_bit(IMA_UPDATE_XATTR, &iint->atomic_flags))
+		return;
+
+	mutex_lock(&iint->mutex);
+	if (iint->ima_work.state == IMA_WORK_ACTIVE)
+		goto out;
+
+	msecs = ima_inode_update_delay(inode);
+	iint->ima_work.file = file;
+	iint->ima_work.state = IMA_WORK_ACTIVE;
+	INIT_DELAYED_WORK(&iint->ima_work.work, ima_delayed_update_handler);
+
+	creq = queue_delayed_work(ima_update_wq,
+				  &iint->ima_work.work,
+				  msecs_to_jiffies(msecs));
+	if (creq == false) {
+		iint->ima_work.file = NULL;
+		iint->ima_work.state = IMA_WORK_INACTIVE;
+	}
+out:
+	mutex_unlock(&iint->mutex);
+}
+EXPORT_SYMBOL_GPL(ima_file_delayed_update);
+
+/**
+ * ima_file_update - update the file measurement
+ * @file: pointer to file structure being updated
+ */
+void ima_file_update(struct file *file)
+{
+	struct inode *inode = file_inode(file);
+	struct integrity_iint_cache *iint;
+	bool should_measure = true;
+	u64 i_version;
+
+	if (!ima_policy_flag || !S_ISREG(inode->i_mode))
+		return;
+
+	iint = integrity_iint_find(inode);
+	if (!iint)
+		return;
+
+	if (!test_bit(IMA_UPDATE_XATTR, &iint->atomic_flags))
+		return;
+
+	mutex_lock(&iint->mutex);
+	if (IS_I_VERSION(inode)) {
+		i_version = inode_query_iversion(inode);
+		if (i_version == iint->version)
+			should_measure = false;
+	}
+	if (should_measure) {
+		iint->flags &= ~IMA_COLLECTED;
+		ima_update_xattr(iint, file);
+	}
+	mutex_unlock(&iint->mutex);
+}
+EXPORT_SYMBOL_GPL(ima_file_update);
+#endif /* CONFIG_IMA_MEASURE_WRITES */
+
 /**
  * ima_path_check - based on policy, collect/store measurement.
  * @file: pointer to the file to be measured
@@ -716,9 +831,18 @@ static int __init init_ima(void)
 	if (error)
 		pr_warn("Couldn't register LSM notifier, error %d\n", error);
 
-	if (!error)
+	if (!error) {
 		ima_update_policy_flag();
 
+		ima_update_wq = alloc_workqueue("ima-update-wq",
+						WQ_MEM_RECLAIM |
+						WQ_CPU_INTENSIVE,
+						0);
+		if (!ima_update_wq) {
+			pr_err("Failed to allocate write measurement workqueue\n");
+			error = -ENOMEM;
+		}
+	}
 	return error;
 }
 
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index d9323d31a3a8..0f80c3d2e079 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -64,6 +64,11 @@
 #define IMA_DIGSIG		3
 #define IMA_MUST_MEASURE	4
 
+/* delayed measurement job state */
+#define IMA_WORK_INACTIVE	0
+#define IMA_WORK_ACTIVE		1
+#define IMA_WORK_CANCELLED	2
+
 enum evm_ima_xattr_type {
 	IMA_XATTR_DIGEST = 0x01,
 	EVM_XATTR_HMAC,
@@ -115,6 +120,18 @@ struct signature_v2_hdr {
 	uint8_t sig[0];		/* signature payload */
 } __packed;
 
+#if CONFIG_IMA_MEASUREMENT_LATENCY == 0
+#define IMA_LATENCY_INCREMENT	100
+#else
+#define IMA_LATENCY_INCREMENT	CONFIG_IMA_MEASUREMENT_LATENCY
+#endif
+
+struct ima_work_entry {
+	struct delayed_work work;
+	struct file *file;
+	uint8_t state;
+};
+
 /* integrity data associated with an inode */
 struct integrity_iint_cache {
 	struct rb_node rb_node;	/* rooted in integrity_iint_tree */
@@ -131,6 +148,7 @@ struct integrity_iint_cache {
 	enum integrity_status ima_creds_status:4;
 	enum integrity_status evm_status:4;
 	struct ima_digest_data *ima_hash;
+	struct ima_work_entry ima_work;
 };
 
 /* rbtree tree calls to lookup, insert, delete
-- 
2.17.1

[PATCH 2/3] ima: update the file measurement on truncate

[Janne Karhunen]

Let IMA know when a file is being opened with truncate
or truncated directly.

Depends on commit 72649b7862a7 ("ima: keep the integrity state of open files up to date")'

Signed-off-by: Janne Karhunen <janne.karhunen@gmail.com>
Signed-off-by: Konsta Karsisto <konsta.karsisto@gmail.com>
---
 fs/namei.c | 5 ++++-
 fs/open.c  | 3 +++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/fs/namei.c b/fs/namei.c
index 209c51a5226c..0994fe26bef1 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -3418,8 +3418,11 @@ static int do_last(struct nameidata *nd,
 		goto out;
 opened:
 	error = ima_file_check(file, op->acc_mode);
-	if (!error && will_truncate)
+	if (!error && will_truncate) {
 		error = handle_truncate(file);
+		if (!error)
+			ima_file_update(file);
+	}
 out:
 	if (unlikely(error > 0)) {
 		WARN_ON(1);
diff --git a/fs/open.c b/fs/open.c
index a59abe3c669a..98c2d4629371 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -63,6 +63,9 @@ int do_truncate(struct dentry *dentry, loff_t length, unsigned int time_attrs,
 	/* Note any delegations or leases have already been broken: */
 	ret = notify_change(dentry, &newattrs, NULL);
 	inode_unlock(dentry->d_inode);
+
+	if (filp)
+		ima_file_update(filp);
 	return ret;
 }
 
-- 
2.17.1

[PATCH 3/3] ima: update the file measurement on writes

[Janne Karhunen]

From: Konsta Karsisto <konsta.karsisto@gmail.com>

Hook do_writepages() in order to do IMA measurement of an inode
that is written out.

Depends on commit 72649b7862a7 ("ima: keep the integrity state of open files up to date")'

Signed-off-by: Konsta Karsisto <konsta.karsisto@gmail.com>
Signed-off-by: Janne Karhunen <janne.karhunen@gmail.com>
---
 include/linux/ima.h               |  10 +++
 mm/page-writeback.c               |   7 ++
 security/integrity/iint.c         |   3 +
 security/integrity/ima/ima.h      |  14 ++++
 security/integrity/ima/ima_main.c | 115 ++++++++++++++++++++++++++++--
 security/integrity/integrity.h    |   6 ++
 6 files changed, 150 insertions(+), 5 deletions(-)

diff --git a/include/linux/ima.h b/include/linux/ima.h
index 6736844e90d3..9d83f4beac7f 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -96,6 +96,8 @@ static inline void ima_kexec_cmdline(const void *buf, int size) {}
 #if ((defined CONFIG_IMA) && defined(CONFIG_IMA_MEASURE_WRITES))
 void ima_file_update(struct file *file);
 void ima_file_delayed_update(struct file *file);
+void ima_inode_update(struct inode *inode);
+void ima_inode_delayed_update(struct inode *inode);
 #else
 static inline void ima_file_update(struct file *file)
 {
@@ -105,6 +107,14 @@ static inline void ima_file_delayed_update(struct file *file)
 {
 	return;
 }
+static inline void ima_inode_update(struct inode *inode)
+{
+	return;
+}
+static inline void ima_inode_delayed_update(struct inode *inode)
+{
+	return;
+}
 #endif
 
 #ifndef CONFIG_IMA_KEXEC
diff --git a/mm/page-writeback.c b/mm/page-writeback.c
index 1804f64ff43c..d5041c625529 100644
--- a/mm/page-writeback.c
+++ b/mm/page-writeback.c
@@ -38,6 +38,7 @@
 #include <linux/sched/rt.h>
 #include <linux/sched/signal.h>
 #include <linux/mm_inline.h>
+#include <linux/ima.h>
 #include <trace/events/writeback.h>
 
 #include "internal.h"
@@ -2347,6 +2348,12 @@ int do_writepages(struct address_space *mapping, struct writeback_control *wbc)
 		cond_resched();
 		congestion_wait(BLK_RW_ASYNC, HZ/50);
 	}
+	if (ret == 0) {
+		if (wbc->sync_mode == WB_SYNC_ALL)
+			ima_inode_update(mapping->host);
+		else
+			ima_inode_delayed_update(mapping->host);
+	}
 	return ret;
 }
 
diff --git a/security/integrity/iint.c b/security/integrity/iint.c
index e12c4900510f..bac15a2b8bee 100644
--- a/security/integrity/iint.c
+++ b/security/integrity/iint.c
@@ -82,6 +82,8 @@ static void iint_free(struct integrity_iint_cache *iint)
 	iint->ima_creds_status = INTEGRITY_UNKNOWN;
 	iint->evm_status = INTEGRITY_UNKNOWN;
 	iint->measured_pcrs = 0;
+	WARN_ON(iint->ima_work.file);
+	WARN_ON(!list_empty(&iint->file_list));
 	kmem_cache_free(iint_cache, iint);
 }
 
@@ -161,6 +163,7 @@ static void init_once(void *foo)
 	iint->ima_read_status = INTEGRITY_UNKNOWN;
 	iint->ima_creds_status = INTEGRITY_UNKNOWN;
 	iint->evm_status = INTEGRITY_UNKNOWN;
+	INIT_LIST_HEAD(&iint->file_list);
 	mutex_init(&iint->mutex);
 }
 
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 195e67631f70..04ba4888b764 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -162,6 +162,10 @@ int ima_lsm_policy_change(struct notifier_block *nb, unsigned long event,
 			  void *lsm_data);
 #if ((defined CONFIG_IMA) && defined(CONFIG_IMA_MEASURE_WRITES))
 void ima_cancel_measurement(struct integrity_iint_cache *iint);
+void ima_get_file(struct integrity_iint_cache *iint,
+		  struct file *file);
+void ima_put_file(struct integrity_iint_cache *iint,
+		  struct file *file);
 #else
 static inline void ima_cancel_measurement(struct integrity_iint_cache *iint)
 {
@@ -172,6 +176,16 @@ static inline void ima_init_measurement(struct integrity_iint_cache *iint,
 {
 	return;
 }
+static inline void ima_get_file(struct integrity_iint_cache *iint,
+				struct file *file)
+{
+	return;
+}
+static inline void ima_put_file(struct integrity_iint_cache *iint,
+				struct file *file)
+{
+	return;
+}
 #endif
 
 /*
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 46d28cdb6466..affc74a07125 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -12,7 +12,8 @@
  *
  * File: ima_main.c
  *	implements the IMA hooks: ima_bprm_check, ima_file_mmap,
- *	ima_file_delayed_update, ima_file_update and ima_file_check.
+ *	ima_file_update, ima_file_delayed_update, ima_inode_update,
+ *	ima_inode_delayed_update and ima_file_check.
  */
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
@@ -157,6 +158,7 @@ static void ima_check_last_writer(struct integrity_iint_cache *iint,
 	ima_cancel_measurement(iint);
 
 	mutex_lock(&iint->mutex);
+	ima_put_file(iint, file);
 	if (atomic_read(&inode->i_writecount) == 1) {
 		update = test_and_clear_bit(IMA_UPDATE_XATTR,
 					    &iint->atomic_flags);
@@ -295,6 +297,12 @@ static int process_measurement(struct file *file, const struct cred *cred,
 		set_bit(IMA_UPDATE_XATTR, &iint->atomic_flags);
 	}
 
+	if (must_appraise && (file->f_mode & FMODE_WRITE))
+		set_bit(IMA_UPDATE_XATTR, &iint->atomic_flags);
+
+	/* Cache file for measurements triggered from inode writeback */
+	ima_get_file(iint, file);
+
 	/* Nothing to do, just return existing appraised status */
 	if (!action) {
 		if (must_appraise) {
@@ -362,12 +370,9 @@ static int process_measurement(struct file *file, const struct cred *cred,
 out:
 	if (pathbuf)
 		__putname(pathbuf);
-	if (must_appraise) {
+	if (must_appraise)
 		if (rc && (ima_appraise & IMA_APPRAISE_ENFORCE))
 			return -EACCES;
-		if (file->f_mode & FMODE_WRITE)
-			set_bit(IMA_UPDATE_XATTR, &iint->atomic_flags);
-	}
 	return 0;
 }
 
@@ -425,6 +430,42 @@ int ima_bprm_check(struct linux_binprm *bprm)
 }
 
 #ifdef CONFIG_IMA_MEASURE_WRITES
+void ima_get_file(struct integrity_iint_cache *iint,
+		  struct file *file)
+{
+	struct ima_fl_entry *e;
+
+	if (!iint || !file)
+		return;
+	if (!(file->f_mode & FMODE_WRITE) ||
+	    !test_bit(IMA_UPDATE_XATTR, &iint->atomic_flags))
+		return;
+
+	list_for_each_entry(e, &iint->file_list, list) {
+		if (e->file == file)
+			return;
+	}
+	e = kmalloc(sizeof(*e), GFP_KERNEL);
+	if (!e)
+		return;
+	e->file = file;
+	list_add(&e->list, &iint->file_list);
+}
+
+void ima_put_file(struct integrity_iint_cache *iint,
+		  struct file *file)
+{
+	struct ima_fl_entry *e;
+
+	list_for_each_entry(e, &iint->file_list, list) {
+		if (e->file == file) {
+			list_del(&e->list);
+			kfree(e);
+			break;
+		}
+	}
+}
+
 static unsigned long ima_inode_update_delay(struct inode *inode)
 {
 	unsigned long blocks, msecs;
@@ -454,6 +495,7 @@ void ima_cancel_measurement(struct integrity_iint_cache *iint)
 		return;
 
 	cancel_delayed_work_sync(&iint->ima_work.work);
+	iint->ima_work.file = NULL;
 	iint->ima_work.state = IMA_WORK_CANCELLED;
 }
 
@@ -499,6 +541,69 @@ void ima_file_delayed_update(struct file *file)
 }
 EXPORT_SYMBOL_GPL(ima_file_delayed_update);
 
+/**
+ * ima_inode_delayed_update - delayed measurement update of an inode
+ * @inode: dirty inode chosen for writeback
+ *
+ * Schedule work to measure the first available 'struct file' cached
+ * in the iint entry that references this inode. This allows IMA to
+ * track inode writebacks.
+ *
+ * Note that we haven't incremented the refcount for the files we keep
+ * track of in order to not mess up the normal file refcounting. If we
+ * see a file whose f_count is already zero, we simply skip it. If we
+ * fail to find any available file reference, the measurement will be
+ * handled by the ima_check_last_writer().
+ */
+void ima_inode_delayed_update(struct inode *inode)
+{
+	struct integrity_iint_cache *iint;
+	struct ima_fl_entry *e;
+	bool found = false;
+
+	iint = integrity_iint_find(inode);
+	if (!iint)
+		return;
+
+	if (iint->ima_work.state == IMA_WORK_ACTIVE)
+		return;
+
+	mutex_lock(&iint->mutex);
+	list_for_each_entry(e, &iint->file_list, list) {
+		if (file_count(e->file) == 0)
+			continue;
+		found = true;
+		break;
+	}
+	mutex_unlock(&iint->mutex);
+	if (found && e->file)
+		ima_file_delayed_update(e->file);
+}
+EXPORT_SYMBOL_GPL(ima_inode_delayed_update);
+
+void ima_inode_update(struct inode *inode)
+{
+	struct integrity_iint_cache *iint;
+	struct ima_fl_entry *e;
+	bool found = false;
+
+	iint = integrity_iint_find(inode);
+	if (!iint)
+		return;
+
+	mutex_lock(&iint->mutex);
+	list_for_each_entry(e, &iint->file_list, list) {
+		if (file_count(e->file) == 0)
+			continue;
+		found = true;
+		break;
+	}
+	mutex_unlock(&iint->mutex);
+	if (found && e->file)
+		ima_file_update(e->file);
+}
+EXPORT_SYMBOL_GPL(ima_inode_update);
+
 /**
  * ima_file_update - update the file measurement
  * @file: pointer to file structure being updated
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index 0f80c3d2e079..17af72683b87 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -132,6 +132,11 @@ struct ima_work_entry {
 	uint8_t state;
 };
 
+struct ima_fl_entry {
+	struct list_head list;
+	struct file *file;
+};
+
 /* integrity data associated with an inode */
 struct integrity_iint_cache {
 	struct rb_node rb_node;	/* rooted in integrity_iint_tree */
@@ -149,6 +154,7 @@ struct integrity_iint_cache {
 	enum integrity_status evm_status:4;
 	struct ima_digest_data *ima_hash;
 	struct ima_work_entry ima_work;
+	struct list_head file_list;
 };
 
 /* rbtree tree calls to lookup, insert, delete
-- 
2.17.1

Re: [PATCH 1/3] ima: keep the integrity state of open files up to date

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1396 bytes --]

Hi Janne,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[cannot apply to v5.3-rc6 next-20190830]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Janne-Karhunen/ima-keep-the-integrity-state-of-open-files-up-to-date/20190902-182420
config: i386-defconfig (attached as .config)
compiler: gcc-7 (Debian 7.4.0-11) 7.4.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   In file included from security/integrity/iint.c:22:0:
>> security/integrity/integrity.h:122:5: warning: "CONFIG_IMA_MEASUREMENT_LATENCY" is not defined, evaluates to 0 [-Wundef]
    #if CONFIG_IMA_MEASUREMENT_LATENCY == 0
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

vim +/CONFIG_IMA_MEASUREMENT_LATENCY +122 security/integrity/integrity.h

   121	
 > 122	#if CONFIG_IMA_MEASUREMENT_LATENCY == 0
   123	#define IMA_LATENCY_INCREMENT	100
   124	#else
   125	#define IMA_LATENCY_INCREMENT	CONFIG_IMA_MEASUREMENT_LATENCY
   126	#endif
   127	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 28046 bytes --]

Re: [PATCH 1/3] ima: keep the integrity state of open files up to date

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1493 bytes --]

Hi Janne,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[cannot apply to v5.3-rc6 next-20190830]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Janne-Karhunen/ima-keep-the-integrity-state-of-open-files-up-to-date/20190902-182420
config: x86_64-randconfig-s0-09021304 (attached as .config)
compiler: gcc-7 (Debian 7.4.0-11) 7.4.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   In file included from security/integrity/evm/evm.h:18:0,
                    from security/integrity/evm/evm_main.c:27:
>> security/integrity/evm/../integrity.h:122:5: warning: "CONFIG_IMA_MEASUREMENT_LATENCY" is not defined, evaluates to 0 [-Wundef]
    #if CONFIG_IMA_MEASUREMENT_LATENCY == 0
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

vim +/CONFIG_IMA_MEASUREMENT_LATENCY +122 security/integrity/evm/../integrity.h

   121	
 > 122	#if CONFIG_IMA_MEASUREMENT_LATENCY == 0
   123	#define IMA_LATENCY_INCREMENT	100
   124	#else
   125	#define IMA_LATENCY_INCREMENT	CONFIG_IMA_MEASUREMENT_LATENCY
   126	#endif
   127	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 30808 bytes --]

Re: [PATCH 2/3] ima: update the file measurement on truncate

[Mimi Zohar]

On Mon, 2019-09-02 at 12:45 +0300, Janne Karhunen wrote:
> Let IMA know when a file is being opened with truncate
> or truncated directly.
> 
> Depends on commit 72649b7862a7 ("ima: keep the integrity state of open files up to date")'
> 
> Signed-off-by: Janne Karhunen <janne.karhunen@gmail.com>
> Signed-off-by: Konsta Karsisto <konsta.karsisto@gmail.com>
> ---
>  fs/namei.c | 5 ++++-
>  fs/open.c  | 3 +++
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index 209c51a5226c..0994fe26bef1 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -3418,8 +3418,11 @@ static int do_last(struct nameidata *nd,
>  		goto out;
>  opened:
>  	error = ima_file_check(file, op->acc_mode);
> -	if (!error && will_truncate)
> +	if (!error && will_truncate) {
>  		error = handle_truncate(file);
> +		if (!error)
> +			ima_file_update(file);

Security and IMA hooks are normally named after the function.  For
example, there's a security hook named security_path_truncate() in
handle_truncate().  The new hook after the truncate would either be
named security_post_path_truncate() or ima_post_path_truncate().

> +	}
>  out:
>  	if (unlikely(error > 0)) {
>  		WARN_ON(1);
> diff --git a/fs/open.c b/fs/open.c
> index a59abe3c669a..98c2d4629371 100644
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -63,6 +63,9 @@ int do_truncate(struct dentry *dentry, loff_t length, unsigned int time_attrs,
>  	/* Note any delegations or leases have already been broken: */
>  	ret = notify_change(dentry, &newattrs, NULL);
>  	inode_unlock(dentry->d_inode);
> +
> +	if (filp)
> +		ima_file_update(filp);
>  	return ret;
>  }
>  

do_truncate() is called from a number of places.  Are you sure that
the call to IMA should be in all of them?  security_path_truncate()
isn't in all of the callers.

Mimi

Re: [PATCH 1/3] ima: keep the integrity state of open files up to date

[Mimi Zohar]

On Mon, 2019-09-02 at 12:45 +0300, Janne Karhunen wrote:
> When a file is open for writing, kernel crash or power outage
> is guaranteed to corrupt the inode integrity state leading to
> file appraisal failure on the subsequent boot. Add some basic
> infrastructure to keep the integrity measurements up to date
> as the files are written to.

The term "measurement" refers to the file hash stored in the IMA
measurement list and used to extend the TPM.  IMA-appraisal verifies
the file hash against a known good value stored as an extended
attribute(xattr).  For immutable files, the known good value should be
a file signature.  For mutable files, the known good value is a file
hash.  The purpose of this patch set is to increase the frequency in
which the file hash, stored as an xattr, is updated.

Throughout this patch set the term "measurement" or "measure" is
inappropriately used.

> 
> Core file operations (open, close, sync, msync, truncate) are
> now allowed to update the measurement immediately. In order
> to maintain sufficient write performance for writes, add a
> latency tunable delayed work workqueue for computing the
> measurements.
> 
> Changelog v2:
> - Make write measurements optional
> - Add support for MMIO measurements
> - Handle all writes via page flush
> - Add work cancellation support, files are properly closed
>   after last_writer checks out. This fixes a userspace break
>   where the file was still open for writing after closing it.
> - Fix/unify IMA_UPDATE_XATTR usage
> 
> Signed-off-by: Janne Karhunen <janne.karhunen@gmail.com>
> Signed-off-by: Konsta Karsisto <konsta.karsisto@gmail.com>
> ---
>  include/linux/ima.h                   |  14 +++
>  security/integrity/ima/Kconfig        |  30 ++++++
>  security/integrity/ima/ima.h          |  13 +++
>  security/integrity/ima/ima_appraise.c |  13 ++-
>  security/integrity/ima/ima_main.c     | 128 +++++++++++++++++++++++++-
>  security/integrity/integrity.h        |  18 ++++
>  6 files changed, 213 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/ima.h b/include/linux/ima.h
> index a20ad398d260..6736844e90d3 100644
> --- a/include/linux/ima.h
> +++ b/include/linux/ima.h
> @@ -93,6 +93,20 @@ static inline void ima_post_path_mknod(struct dentry *dentry)
>  static inline void ima_kexec_cmdline(const void *buf, int size) {}
>  #endif /* CONFIG_IMA */
>  
> +#if ((defined CONFIG_IMA) && defined(CONFIG_IMA_MEASURE_WRITES))
> +void ima_file_update(struct file *file);
> +void ima_file_delayed_update(struct file *file);
> +#else
> +static inline void ima_file_update(struct file *file)
> +{
> +	return;
> +}
> +static inline void ima_file_delayed_update(struct file *file)
> +{
> +	return;
> +}
> +#endif
> +
>  #ifndef CONFIG_IMA_KEXEC
>  struct kimage;
>  
> diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
> index 897bafc59a33..df1cf684a442 100644
> --- a/security/integrity/ima/Kconfig
> +++ b/security/integrity/ima/Kconfig
> @@ -310,3 +310,33 @@ config IMA_APPRAISE_SIGNED_INIT
>  	default n
>  	help
>  	   This option requires user-space init to be signed.
> +
> +config IMA_MEASURE_WRITES
> +	bool "Measure file writes (EXPERIMENTAL)"

"MEASURE" is the wrong term.

> +	depends on IMA

Only IMA_APPRAISE updates the security.ima.  This should be "depends
on IMA_APPRAISE".

> +	depends on EVM

Anyone relying on file hashes to verify a file's integrity should
enable EVM, but as much as possible IMA and EVM are defined
independently of each other.  Please remove this dependency.

> +	default n
> +	help
> +	   By default IMA measures files only when they close or sync.

By default IMA-appraisal updates the file hash, stored as an xattr,
...
 
> +	   Choose this option if you want the integrity measurements on
> +	   the disk to update when the files are still open for writing.
> +
> +config IMA_MEASUREMENT_LATENCY
> +	int
> +	depends on IMA

More specific dependency is required.

I'd like to see smaller patches that build upon each other.  For
example, the initial design should be in the first patch.  Performance
improvements, like the latency and latency_ceiling, could be
subsequent patches.

> +	range 0 60000
> +	default 50
> +	help
> +	   This value defines the measurement interval when files are
> +	   being written. Value is in milliseconds.
> +
> +config IMA_MEASUREMENT_LATENCY_CEILING
> +	int
> +	depends on IMA

More specific dependency required.

> +	range 100 60000

The "ceiling" needs to be greater than the "latency".  If it isn't
possible to implement in the Kconfig, then at least check in the code.

> +	default 5000
> +	help
> +	   In order to maintain high write performance for large files,
> +	   IMA increases the measurement interval as the file size grows.
> +	   This value defines the ceiling for the measurement delay in
> +	   milliseconds.
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index 19769bf5f6ab..195e67631f70 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -160,6 +160,19 @@ void ima_init_template_list(void);
>  int __init ima_init_digests(void);
>  int ima_lsm_policy_change(struct notifier_block *nb, unsigned long event,
>  			  void *lsm_data);
> +#if ((defined CONFIG_IMA) && defined(CONFIG_IMA_MEASURE_WRITES))

Both Kconfig options shouldn't be required.  Use the more specific
one.

> +void ima_cancel_measurement(struct integrity_iint_cache *iint);

(The function needs to be renamed to something without the word
"measurement".)

Currently ima_cancel_measurement() is defined and called from
ima_main.c.

> +#else
> +static inline void ima_cancel_measurement(struct integrity_iint_cache *iint)
> +{
> +	return;
> +}
> +static inline void ima_init_measurement(struct integrity_iint_cache *iint,
> +					struct dentry *dentry)
> +{
> +	return;
> +}
> +#endif

If the function definition and usage are in the same file, the
function should be defined as static.  There shouldn't be a need for
these the function declarations or stub functions.

>  
>  /*
>   * used to protect h_table and sha_table
> diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
> index 136ae4e0ee92..6c137fb5289b 100644
> --- a/security/integrity/ima/ima_appraise.c
> +++ b/security/integrity/ima/ima_appraise.c
> @@ -78,6 +78,15 @@ static int ima_fix_xattr(struct dentry *dentry,
>  	return rc;
>  }
>  
> +#ifdef CONFIG_IMA_MEASURE_WRITES

ifdef's don't belong in C code.  Refer to section 21 "Conditional
Compilation" in Documentation/process/coding-style.rst.

> +inline void ima_init_measurement(struct integrity_iint_cache *iint,
> +				 struct dentry *dentry)

(

(The function needs to be renamed to something without the word
"measurement".)

Writing xattrs requires the i_rwsem.  Please add a comment indicating
that callers must take the i_rwsem.

> +{
> +	if (test_bit(IMA_UPDATE_XATTR, &iint->atomic_flags))
> +		ima_fix_xattr(dentry, iint);
> +}
> +#endif
> +
>  /* Return specific func appraised cached result */
>  enum integrity_status ima_get_cache_status(struct integrity_iint_cache *iint,
>  					   enum ima_hooks func)
> @@ -341,8 +350,10 @@ int ima_appraise_measurement(enum ima_hooks func,
>  			iint->flags |= IMA_NEW_FILE;
>  		if ((iint->flags & IMA_NEW_FILE) &&
>  		    (!(iint->flags & IMA_DIGSIG_REQUIRED) ||
> -		     (inode->i_size == 0)))
> +		    (inode->i_size == 0))) {
> +			ima_init_measurement(iint, dentry);

Do we really need to write the file hashes for 0 length files?


>  			status = INTEGRITY_PASS;
> +		}
>  		goto out;
>  	}
>  
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 79c01516211b..46d28cdb6466 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -12,7 +12,7 @@
>   *
>   * File: ima_main.c
>   *	implements the IMA hooks: ima_bprm_check, ima_file_mmap,
> - *	and ima_file_check.
> + *	ima_file_delayed_update, ima_file_update and ima_file_check.
>   */
>  
>  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> @@ -26,6 +26,8 @@
>  #include <linux/xattr.h>
>  #include <linux/ima.h>
>  #include <linux/iversion.h>
> +#include <linux/workqueue.h>
> +#include <linux/sizes.h>
>  #include <linux/fs.h>
>  
>  #include "ima.h"
> @@ -42,6 +44,7 @@ static int hash_setup_done;
>  static struct notifier_block ima_lsm_policy_notifier = {
>  	.notifier_call = ima_lsm_policy_change,
>  };
> +static struct workqueue_struct *ima_update_wq;

All of the workqueue support should probably be defined in a separate
file, not here in ima_main.c.

>  
>  static int __init hash_setup(char *str)
>  {
> @@ -151,6 +154,7 @@ static void ima_check_last_writer(struct integrity_iint_cache *iint,
>  
>  	if (!(mode & FMODE_WRITE))
>  		return;
> +	ima_cancel_measurement(iint);
>  
>  	mutex_lock(&iint->mutex);
>  	if (atomic_read(&inode->i_writecount) == 1) {
> @@ -420,6 +424,117 @@ int ima_bprm_check(struct linux_binprm *bprm)
>  				   MAY_EXEC, CREDS_CHECK);
>  }
>  
> +#ifdef CONFIG_IMA_MEASURE_WRITES

ifdef's don't belong in C code.

> +static unsigned long ima_inode_update_delay(struct inode *inode)
> +{
> +	unsigned long blocks, msecs;
> +
> +	blocks = i_size_read(inode) / SZ_1M + 1;
> +	msecs = blocks * IMA_LATENCY_INCREMENT;
> +	if (msecs > CONFIG_IMA_MEASUREMENT_LATENCY_CEILING)
> +		msecs = CONFIG_IMA_MEASUREMENT_LATENCY_CEILING;
> +
> +	return msecs;
> +}
> +
> +static void ima_delayed_update_handler(struct work_struct *work)
> +{
> +	struct ima_work_entry *entry;
> +
> +	entry = container_of(work, typeof(*entry), work.work);
> +
> +	ima_file_update(entry->file);
> +	entry->file = NULL;
> +	entry->state = IMA_WORK_INACTIVE;
> +}
> +
> +void ima_cancel_measurement(struct integrity_iint_cache *iint)
> +{
> +	if (iint->ima_work.state != IMA_WORK_ACTIVE)
> +		return;
> +
> +	cancel_delayed_work_sync(&iint->ima_work.work);
> +	iint->ima_work.state = IMA_WORK_CANCELLED;
> +}
> +
> +/**
> + * ima_file_delayed_update
> + * @file: pointer to file structure being updated
> + */
> +void ima_file_delayed_update(struct file *file)
> +{
> +	struct inode *inode = file_inode(file);
> +	struct integrity_iint_cache *iint;
> +	unsigned long msecs;
> +	bool creq;
> +
> +	if (!ima_policy_flag || !S_ISREG(inode->i_mode))
> +		return;
> +
> +	iint = integrity_iint_find(inode);
> +	if (!iint)
> +		return;
> +
> +	if (!test_bit(IMA_UPDATE_XATTR, &iint->atomic_flags))
> +		return;
> +
> +	mutex_lock(&iint->mutex);
> +	if (iint->ima_work.state == IMA_WORK_ACTIVE)
> +		goto out;
> +
> +	msecs = ima_inode_update_delay(inode);
> +	iint->ima_work.file = file;
> +	iint->ima_work.state = IMA_WORK_ACTIVE;
> +	INIT_DELAYED_WORK(&iint->ima_work.work, ima_delayed_update_handler);
> +
> +	creq = queue_delayed_work(ima_update_wq,
> +				  &iint->ima_work.work,
> +				  msecs_to_jiffies(msecs));
> +	if (creq == false) {
> +		iint->ima_work.file = NULL;
> +		iint->ima_work.state = IMA_WORK_INACTIVE;
> +	}
> +out:
> +	mutex_unlock(&iint->mutex);
> +}
> +EXPORT_SYMBOL_GPL(ima_file_delayed_update);
> +
> +/**
> + * ima_file_update - update the file measurement
> + * @file: pointer to file structure being updated
> + */
> +void ima_file_update(struct file *file)
> +{
> +	struct inode *inode = file_inode(file);
> +	struct integrity_iint_cache *iint;
> +	bool should_measure = true;
> +	u64 i_version;
> +
> +	if (!ima_policy_flag || !S_ISREG(inode->i_mode))
> +		return;
> +
> +	iint = integrity_iint_find(inode);
> +	if (!iint)
> +		return;
> +
> +	if (!test_bit(IMA_UPDATE_XATTR, &iint->atomic_flags))
> +		return;
> +
> +	mutex_lock(&iint->mutex);
> +	if (IS_I_VERSION(inode)) {
> +		i_version = inode_query_iversion(inode);
> +		if (i_version == iint->version)
> +			should_measure = false;
> +	}
> +	if (should_measure) {
> +		iint->flags &= ~IMA_COLLECTED;
> +		ima_update_xattr(iint, file);
> +	}
> +	mutex_unlock(&iint->mutex);
> +}
> +EXPORT_SYMBOL_GPL(ima_file_update);
> +#endif /* CONFIG_IMA_MEASURE_WRITES */
> +
>  /**
>   * ima_path_check - based on policy, collect/store measurement.
>   * @file: pointer to the file to be measured
> @@ -716,9 +831,18 @@ static int __init init_ima(void)
>  	if (error)
>  		pr_warn("Couldn't register LSM notifier, error %d\n", error);
>  
> -	if (!error)
> +	if (!error) {
>  		ima_update_policy_flag();
>  
> +		ima_update_wq = alloc_workqueue("ima-update-wq",
> +						WQ_MEM_RECLAIM |
> +						WQ_CPU_INTENSIVE,
> +						0);
> +		if (!ima_update_wq) {
> +			pr_err("Failed to allocate write measurement workqueue\n");
> +			error = -ENOMEM;
> +		}
> +	}
>  	return error;
>  }
>  
> diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
> index d9323d31a3a8..0f80c3d2e079 100644
> --- a/security/integrity/integrity.h
> +++ b/security/integrity/integrity.h
> @@ -64,6 +64,11 @@
>  #define IMA_DIGSIG		3
>  #define IMA_MUST_MEASURE	4
>  
> +/* delayed measurement job state */
> +#define IMA_WORK_INACTIVE	0
> +#define IMA_WORK_ACTIVE		1
> +#define IMA_WORK_CANCELLED	2
> +
>  enum evm_ima_xattr_type {
>  	IMA_XATTR_DIGEST = 0x01,
>  	EVM_XATTR_HMAC,
> @@ -115,6 +120,18 @@ struct signature_v2_hdr {
>  	uint8_t sig[0];		/* signature payload */
>  } __packed;
>  
> +#if CONFIG_IMA_MEASUREMENT_LATENCY == 0
> +#define IMA_LATENCY_INCREMENT	100
> +#else
> +#define IMA_LATENCY_INCREMENT	CONFIG_IMA_MEASUREMENT_LATENCY
> +#endif
> +
> +struct ima_work_entry {
> +	struct delayed_work work;
> +	struct file *file;
> +	uint8_t state;
> +};
> +
 
Please add a comment indicating the type of work or maybe update the
struct name.

Mimi

>  /* integrity data associated with an inode */
>  struct integrity_iint_cache {
>  	struct rb_node rb_node;	/* rooted in integrity_iint_tree */
> @@ -131,6 +148,7 @@ struct integrity_iint_cache {
>  	enum integrity_status ima_creds_status:4;
>  	enum integrity_status evm_status:4;
>  	struct ima_digest_data *ima_hash;
> +	struct ima_work_entry ima_work;
>  };
>  
>  /* rbtree tree calls to lookup, insert, delete

Re: [PATCH 3/3] ima: update the file measurement on writes

[Mimi Zohar]

On Mon, 2019-09-02 at 12:45 +0300, Janne Karhunen wrote:
> From: Konsta Karsisto <konsta.karsisto@gmail.com>
> 
> Hook do_writepages() in order to do IMA measurement of an inode
> that is written out.

With this explanation I would expect to see a security/ima hook in
do_writepages(), nothing else.  There's a lot more going on here than
that.  The memory management maintainer(s) doesn't really care about
IMA.  Please break this patch up so that it is easier to review and
upstream.

My comments on the first patch in this patch set (e.g. function names,
ifdefs in C code, workqueue belonging in a separate patch) are also
applicable to this patch.

> 
> Depends on commit 72649b7862a7 ("ima: keep the integrity state of open files up to date")'
> 
> Signed-off-by: Konsta Karsisto <konsta.karsisto@gmail.com>
> Signed-off-by: Janne Karhunen <janne.karhunen@gmail.com>
> ---

[snip]


> diff --git a/mm/page-writeback.c b/mm/page-writeback.c
> index 1804f64ff43c..d5041c625529 100644
> --- a/mm/page-writeback.c
> +++ b/mm/page-writeback.c
> @@ -38,6 +38,7 @@
>  #include <linux/sched/rt.h>
>  #include <linux/sched/signal.h>
>  #include <linux/mm_inline.h>
> +#include <linux/ima.h>
>  #include <trace/events/writeback.h>
>  
>  #include "internal.h"
> @@ -2347,6 +2348,12 @@ int do_writepages(struct address_space *mapping, struct writeback_control *wbc)
>  		cond_resched();
>  		congestion_wait(BLK_RW_ASYNC, HZ/50);
>  	}
> +	if (ret == 0) {
> +		if (wbc->sync_mode == WB_SYNC_ALL)
> +			ima_inode_update(mapping->host);
> +		else
> +			ima_inode_delayed_update(mapping->host);

It's hard enough upstreaming a single security or IMA hook.  There's
no need for two.  security/IMA hooks are normally based on function
names (eg. ima_do_writepages).  The sync_mode should be an argument.

> +	}
>  	return ret;
>  }
>  


> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 46d28cdb6466..affc74a07125 100644

> @@ -425,6 +430,42 @@ int ima_bprm_check(struct linux_binprm *bprm)
>  }
>  
>  #ifdef CONFIG_IMA_MEASURE_WRITES
> +void ima_get_file(struct integrity_iint_cache *iint,
> +		  struct file *file)
> +{
> +	struct ima_fl_entry *e;
> +
> +	if (!iint || !file)
> +		return;
> +	if (!(file->f_mode & FMODE_WRITE) ||
> +	    !test_bit(IMA_UPDATE_XATTR, &iint->atomic_flags))
> +		return;
> +
> +	list_for_each_entry(e, &iint->file_list, list) {
> +		if (e->file == file)
> +			return;
> +	}
> +	e = kmalloc(sizeof(*e), GFP_KERNEL);
> +	if (!e)
> +		return;
> +	e->file = file;
> +	list_add(&e->list, &iint->file_list);
> +}
> +
> +void ima_put_file(struct integrity_iint_cache *iint,
> +		  struct file *file)
> +{
> +	struct ima_fl_entry *e;
> +
> +	list_for_each_entry(e, &iint->file_list, list) {
> +		if (e->file == file) {
> +			list_del(&e->list);
> +			kfree(e);
> +			break;
> +		}
> +	}
> +}
> +

Functions with "get" or "put" in their name increment/decrement a
reference count.  No reference count is being updated here.

Mimi

Re: [PATCH 1/3] ima: keep the integrity state of open files up to date

[Eric Biggers]

On Mon, Sep 02, 2019 at 12:45:38PM +0300, Janne Karhunen wrote:
> When a file is open for writing, kernel crash or power outage
> is guaranteed to corrupt the inode integrity state leading to
> file appraisal failure on the subsequent boot. Add some basic
> infrastructure to keep the integrity measurements up to date
> as the files are written to.
> 
> Core file operations (open, close, sync, msync, truncate) are
> now allowed to update the measurement immediately. In order
> to maintain sufficient write performance for writes, add a
> latency tunable delayed work workqueue for computing the
> measurements.
> 

This still doesn't make it crash-safe.  So why is it okay?

- Eric

Re: [PATCH 1/3] ima: keep the integrity state of open files up to date

[Janne Karhunen]

On Tue, Sep 10, 2019 at 12:39 AM Eric Biggers <ebiggers@kernel.org> wrote:
> > Core file operations (open, close, sync, msync, truncate) are
> > now allowed to update the measurement immediately. In order
> > to maintain sufficient write performance for writes, add a
> > latency tunable delayed work workqueue for computing the
> > measurements.
>
> This still doesn't make it crash-safe.  So why is it okay?

If Android is the load, this makes it crash safe 99% of the time and
that is considerably better than 0% of the time.

That said, we have now a patch draft forming up that pushes the update
to the ext4 journal. With this patch on top we should reach the
magical 100% given data=journal mount. One step at a time.


--
Janne

Re: [PATCH 1/3] ima: keep the integrity state of open files up to date

[Eric Biggers]

On Tue, Sep 10, 2019 at 10:04:53AM +0300, Janne Karhunen wrote:
> On Tue, Sep 10, 2019 at 12:39 AM Eric Biggers <ebiggers@kernel.org> wrote:
> > > Core file operations (open, close, sync, msync, truncate) are
> > > now allowed to update the measurement immediately. In order
> > > to maintain sufficient write performance for writes, add a
> > > latency tunable delayed work workqueue for computing the
> > > measurements.
> >
> > This still doesn't make it crash-safe.  So why is it okay?
> 
> If Android is the load, this makes it crash safe 99% of the time and
> that is considerably better than 0% of the time.
> 

Who will use it if it isn't 100% safe?

- Eric

Re: [PATCH 1/3] ima: keep the integrity state of open files up to date

[Janne Karhunen]

On Sun, Sep 15, 2019 at 11:24 PM Eric Biggers <ebiggers@kernel.org> wrote:

> > > This still doesn't make it crash-safe.  So why is it okay?
> >
> > If Android is the load, this makes it crash safe 99% of the time and
> > that is considerably better than 0% of the time.
> >
>
> Who will use it if it isn't 100% safe?

I suppose anyone using mutable data with IMA appraise should, unless
they have a redundant power supply and a kernel that never crashes. In
a way this is like asking if the ima-appraise should be there for
mutable data at all. All this is doing is that it improves the crash
recovery reliability without taking anything away.

Anyway, I think I'm getting along with my understanding of the page
writeback slowly and the journal support will eventually be there at
least as an add-on patch for those that want to use it and really need
the last 0.n% reliability. Note that even without that patch you can
build ima-appraise based systems that are 99.999% reliable just by
having the patch we're discussing here. Without it you would be orders
of magnitude worse off. All we are doing is that we give it a fairly
good chance to recover instead of giving up without even trying.

That said, I'm not sure the 100% crash recovery is ever guaranteed in
any Linux system. We just have to do what we can, no?


--
Janne

Re: [PATCH 1/3] ima: keep the integrity state of open files up to date

[Eric Biggers]

On Mon, Sep 16, 2019 at 02:45:56PM +0300, Janne Karhunen wrote:
> On Sun, Sep 15, 2019 at 11:24 PM Eric Biggers <ebiggers@kernel.org> wrote:
> 
> > > > This still doesn't make it crash-safe.  So why is it okay?
> > >
> > > If Android is the load, this makes it crash safe 99% of the time and
> > > that is considerably better than 0% of the time.
> > >
> >
> > Who will use it if it isn't 100% safe?
> 
> I suppose anyone using mutable data with IMA appraise should, unless
> they have a redundant power supply and a kernel that never crashes. In
> a way this is like asking if the ima-appraise should be there for
> mutable data at all. All this is doing is that it improves the crash
> recovery reliability without taking anything away.

Okay, so why would anyone use mutable data with IMA appraise if it corrupts your
files by design, both with and without this patchset?

> 
> Anyway, I think I'm getting along with my understanding of the page
> writeback slowly and the journal support will eventually be there at
> least as an add-on patch for those that want to use it and really need
> the last 0.n% reliability. Note that even without that patch you can
> build ima-appraise based systems that are 99.999% reliable just by

On what storage devices, workloads, and filesystems is this number for?

> having the patch we're discussing here. Without it you would be orders
> of magnitude worse off. All we are doing is that we give it a fairly
> good chance to recover instead of giving up without even trying.
> 
> That said, I'm not sure the 100% crash recovery is ever guaranteed in
> any Linux system. We just have to do what we can, no?
> 

Filesystems implement consistency mechanisms, e.g. journalling or copy-on-write,
to recover from crashes by design.  This patchset doesn't implement or use any
such mechanism, so it's not crash-safe.  It's not clear that it's even a step in
the right direction, as no patches have been proposed for a correct solution so
we can see what it actually involves.

- Eric

Re: [PATCH 1/3] ima: keep the integrity state of open files up to date

[Janne Karhunen]

On Tue, Sep 17, 2019 at 7:23 AM Eric Biggers <ebiggers@kernel.org> wrote:

> > > Who will use it if it isn't 100% safe?
> >
> > I suppose anyone using mutable data with IMA appraise should, unless
> > they have a redundant power supply and a kernel that never crashes. In
> > a way this is like asking if the ima-appraise should be there for
> > mutable data at all. All this is doing is that it improves the crash
> > recovery reliability without taking anything away.
>
> Okay, so why would anyone use mutable data with IMA appraise if it corrupts your
> files by design, both with and without this patchset?

Now you are exaggerating heavily: it does not corrupt your files by
design. A crash in any security related system is supposed to be
pretty rare occurrence.


> > Anyway, I think I'm getting along with my understanding of the page
> > writeback slowly and the journal support will eventually be there at
> > least as an add-on patch for those that want to use it and really need
> > the last 0.n% reliability. Note that even without that patch you can
> > build ima-appraise based systems that are 99.999% reliable just by
>
> On what storage devices, workloads, and filesystems is this number for?

I reached 99.2% recovery rate with the AOSP without touching the
android on top by crashing the kernel with a test case while the
device was in use. 80% if I crash it while the device is in the
busiest write cycle (the first boot, I guess we would suck quite
royally if we never made past this point without dying).

99.95+% of course requires a high-availability system that probably
crashes once per year at best and recovers in seconds. In that case
this will recover it with pretty high odds, so reliability is not all
that much reduced from it's normal reliability statistics. So, the
ima-appraise for the mutable data could be in use even in a
high-availability system. 99% recovery probability for the crash that
occurs once per year would be OK; 0% would not be. I suppose it all
depends on your requirements.


> > having the patch we're discussing here. Without it you would be orders
> > of magnitude worse off. All we are doing is that we give it a fairly
> > good chance to recover instead of giving up without even trying.
> >
> > That said, I'm not sure the 100% crash recovery is ever guaranteed in
> > any Linux system. We just have to do what we can, no?
>
> Filesystems implement consistency mechanisms, e.g. journalling or copy-on-write,
> to recover from crashes by design.  This patchset doesn't implement or use any
> such mechanism, so it's not crash-safe.  It's not clear that it's even a step in
> the right direction, as no patches have been proposed for a correct solution so
> we can see what it actually involves.

Great, what would be the better alternative? I guess the suggestion
cannot be that 'don't use it' since the code is there?

As for the 'step to the right direction': before we could talk about
any of this journaling stuff we had to make sure that we have the
plumbing where the measurements are accurate. These patches do that
and the journaling is the next step. All the journaling add-on does
now is that it binds the page write and the xattr update into one
transaction, so both of those run as sub-transactions of one master.
Now, only when the master ends the data is moved out of the journal in
one bundle. All this is so ridiculously simple I doubt my own eyes,
but it seems to work fine apart from some slowdown on shutdown when
processes call sync() like there is no tomorrow. Nevertheless,
understanding the related code (the page writeback and the ext4) is
pretty nasty and there are lots of things I need to understand about
that still. The thing I'm currently trying to get my head around is
that whether or not it is possible that we have a measurement over a
page that was not eligible for the writeback. I'm also no ext4 expert
so all help in that regard is highly appreciated if this type of thing
is interesting to others.

Anyway, all this is good info. If this code is not needed upstream,
I'm happy to stop working with it and will maintain this for my use
only. Let me know,


--
Janne