diff mbox series

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

Message ID 20190902094540.12786-1-janne.karhunen@gmail.com (mailing list archive)
State New, archived
Headers show
Series [1/3] ima: keep the integrity state of open files up to date | expand

Commit Message

Janne Karhunen Sept. 2, 2019, 9:45 a.m. UTC 
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(-)

Comments

Mimi Zohar Sept. 8, 2019, 4:35 p.m. UTC | #1 
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
Eric Biggers Sept. 9, 2019, 9:39 p.m. UTC | #2 
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
Janne Karhunen Sept. 10, 2019, 7:04 a.m. UTC | #3 
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
Eric Biggers Sept. 15, 2019, 8:24 p.m. UTC | #4 
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
Janne Karhunen Sept. 16, 2019, 11:45 a.m. UTC | #5 
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
Eric Biggers Sept. 17, 2019, 4:23 a.m. UTC | #6 
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
Janne Karhunen Sept. 17, 2019, 7:24 a.m. UTC | #7 
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
diff mbox series

Patch

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