fix IMA deadlock

fix IMA deadlock

[Christoph Hellwig]

IMA currently tries to read the file to calculate a hash from inside
i_rwsem.  On file system like XFS that also take i_rwsem for reads this
deadlocks.

This patch instead adds a new ->integrity_read method for IMA that can
be used to read data with i_rwsem held.  For XFS it has an implementation
that skips the locking, other file systems can just set it to the default
implementation.

As a side effect this also gets rid of playing with the address limit.

[PATCH] security/ima: use fs method to read integrity data

[Christoph Hellwig]

Add a new ->integrity_read file operation to read data for
integrity hash collection.  This is defined to be equivalent
to ->read_iter, except that it will be called with the i_rwsem
held exclusively.  Also the presence of ->integrity_read indicates
that the file system can support IMA.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/btrfs/file.c           |  1 +
 fs/ext4/file.c            |  1 +
 fs/xfs/xfs_file.c         | 21 +++++++++++++++++++++
 include/linux/fs.h        |  1 +
 security/integrity/iint.c | 20 ++++++++++++++------
 5 files changed, 38 insertions(+), 6 deletions(-)

diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index 520cb7230b2d..c5afb2264737 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -3041,6 +3041,7 @@ const struct file_operations btrfs_file_operations = {
 #endif
 	.clone_file_range = btrfs_clone_file_range,
 	.dedupe_file_range = btrfs_dedupe_file_range,
+	.integrity_read = generic_file_read_iter,
 };
 
 void btrfs_auto_defrag_exit(void)
diff --git a/fs/ext4/file.c b/fs/ext4/file.c
index 8210c1f43556..b2a5025c7e0d 100644
--- a/fs/ext4/file.c
+++ b/fs/ext4/file.c
@@ -740,6 +740,7 @@ const struct file_operations ext4_file_operations = {
 	.splice_read	= generic_file_splice_read,
 	.splice_write	= iter_file_splice_write,
 	.fallocate	= ext4_fallocate,
+	.integrity_read	= ext4_file_read_iter,
 };
 
 const struct inode_operations ext4_file_inode_operations = {
diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index 35703a801372..3d6ace2a79bc 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -288,6 +288,26 @@ xfs_file_read_iter(
 	return ret;
 }
 
+static ssize_t
+xfs_integrity_read(
+	struct kiocb		*iocb,
+	struct iov_iter		*to)
+{
+	struct inode		*inode = file_inode(iocb->ki_filp);
+	struct xfs_mount	*mp = XFS_I(inode)->i_mount;
+
+	lockdep_assert_held(&inode->i_rwsem);
+
+	XFS_STATS_INC(mp, xs_read_calls);
+
+	if (XFS_FORCED_SHUTDOWN(mp))
+		return -EIO;
+
+	if (IS_DAX(inode))
+		return dax_iomap_rw(iocb, to, &xfs_iomap_ops);
+	return generic_file_read_iter(iocb, to);
+}
+
 /*
  * Zero any on disk space between the current EOF and the new, larger EOF.
  *
@@ -1534,6 +1554,7 @@ const struct file_operations xfs_file_operations = {
 	.fallocate	= xfs_file_fallocate,
 	.clone_file_range = xfs_file_clone_range,
 	.dedupe_file_range = xfs_file_dedupe_range,
+	.integrity_read	= xfs_integrity_read,
 };
 
 const struct file_operations xfs_dir_file_operations = {
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 7251f7bb45e8..0c86816ca25f 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1689,6 +1689,7 @@ struct file_operations {
 			u64);
 	ssize_t (*dedupe_file_range)(struct file *, u64, u64, struct file *,
 			u64);
+	ssize_t (*integrity_read)(struct kiocb *, struct iov_iter *);
 };
 
 struct inode_operations {
diff --git a/security/integrity/iint.c b/security/integrity/iint.c
index c710d22042f9..c5489672b5aa 100644
--- a/security/integrity/iint.c
+++ b/security/integrity/iint.c
@@ -21,6 +21,7 @@
 #include <linux/rbtree.h>
 #include <linux/file.h>
 #include <linux/uaccess.h>
+#include <linux/uio.h>
 #include "integrity.h"
 
 static struct rb_root integrity_iint_tree = RB_ROOT;
@@ -184,18 +185,25 @@ security_initcall(integrity_iintcache_init);
 int integrity_kernel_read(struct file *file, loff_t offset,
 			  char *addr, unsigned long count)
 {
-	mm_segment_t old_fs;
-	char __user *buf = (char __user *)addr;
+	struct inode *inode = file_inode(file);
+	struct iovec iov = { .iov_base = addr, .iov_len = count };
+	struct kiocb kiocb;
+	struct iov_iter iter;
 	ssize_t ret;
 
+	lockdep_assert_held(&inode->i_rwsem);
+
 	if (!(file->f_mode & FMODE_READ))
 		return -EBADF;
+	if (!file->f_op->integrity_read)
+		return -EBADF;
 
-	old_fs = get_fs();
-	set_fs(get_ds());
-	ret = __vfs_read(file, buf, count, &offset);
-	set_fs(old_fs);
+	init_sync_kiocb(&kiocb, file);
+	kiocb.ki_pos = offset;
+	iov_iter_init(&iter, READ | ITER_KVEC, &iov, 1, count);
 
+	ret = file->f_op->integrity_read(&kiocb, &iter);
+	BUG_ON(ret == -EIOCBQUEUED);
 	return ret;
 }
 
-- 
2.11.0

Re: [PATCH] security/ima: use fs method to read integrity data

[Boaz Harrosh]

On 05/10/2017 09:45 AM, Christoph Hellwig wrote:
> Add a new ->integrity_read file operation to read data for
> integrity hash collection.  This is defined to be equivalent
> to ->read_iter, except that it will be called with the i_rwsem
> held exclusively.  Also the presence of ->integrity_read indicates
> that the file system can support IMA.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/btrfs/file.c           |  1 +
>  fs/ext4/file.c            |  1 +
>  fs/xfs/xfs_file.c         | 21 +++++++++++++++++++++
>  include/linux/fs.h        |  1 +
>  security/integrity/iint.c | 20 ++++++++++++++------
>  5 files changed, 38 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
> index 520cb7230b2d..c5afb2264737 100644
> --- a/fs/btrfs/file.c
> +++ b/fs/btrfs/file.c
> @@ -3041,6 +3041,7 @@ const struct file_operations btrfs_file_operations = {
>  #endif
>  	.clone_file_range = btrfs_clone_file_range,
>  	.dedupe_file_range = btrfs_dedupe_file_range,
> +	.integrity_read = generic_file_read_iter,
>  };
>  
>  void btrfs_auto_defrag_exit(void)
> diff --git a/fs/ext4/file.c b/fs/ext4/file.c
> index 8210c1f43556..b2a5025c7e0d 100644
> --- a/fs/ext4/file.c
> +++ b/fs/ext4/file.c
> @@ -740,6 +740,7 @@ const struct file_operations ext4_file_operations = {
>  	.splice_read	= generic_file_splice_read,
>  	.splice_write	= iter_file_splice_write,
>  	.fallocate	= ext4_fallocate,
> +	.integrity_read	= ext4_file_read_iter,
>  };
>  
>  const struct inode_operations ext4_file_inode_operations = {
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index 35703a801372..3d6ace2a79bc 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -288,6 +288,26 @@ xfs_file_read_iter(
>  	return ret;
>  }
>  
> +static ssize_t
> +xfs_integrity_read(
> +	struct kiocb		*iocb,
> +	struct iov_iter		*to)
> +{
> +	struct inode		*inode = file_inode(iocb->ki_filp);
> +	struct xfs_mount	*mp = XFS_I(inode)->i_mount;
> +
> +	lockdep_assert_held(&inode->i_rwsem);
> +
> +	XFS_STATS_INC(mp, xs_read_calls);
> +
> +	if (XFS_FORCED_SHUTDOWN(mp))
> +		return -EIO;
> +
> +	if (IS_DAX(inode))
> +		return dax_iomap_rw(iocb, to, &xfs_iomap_ops);
> +	return generic_file_read_iter(iocb, to);
> +}
> +
>  /*
>   * Zero any on disk space between the current EOF and the new, larger EOF.
>   *
> @@ -1534,6 +1554,7 @@ const struct file_operations xfs_file_operations = {
>  	.fallocate	= xfs_file_fallocate,
>  	.clone_file_range = xfs_file_clone_range,
>  	.dedupe_file_range = xfs_file_dedupe_range,
> +	.integrity_read	= xfs_integrity_read,
>  };
>  
>  const struct file_operations xfs_dir_file_operations = {
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 7251f7bb45e8..0c86816ca25f 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -1689,6 +1689,7 @@ struct file_operations {
>  			u64);
>  	ssize_t (*dedupe_file_range)(struct file *, u64, u64, struct file *,
>  			u64);
> +	ssize_t (*integrity_read)(struct kiocb *, struct iov_iter *);
>  };
>  
>  struct inode_operations {
> diff --git a/security/integrity/iint.c b/security/integrity/iint.c
> index c710d22042f9..c5489672b5aa 100644
> --- a/security/integrity/iint.c
> +++ b/security/integrity/iint.c
> @@ -21,6 +21,7 @@
>  #include <linux/rbtree.h>
>  #include <linux/file.h>
>  #include <linux/uaccess.h>
> +#include <linux/uio.h>
>  #include "integrity.h"
>  
>  static struct rb_root integrity_iint_tree = RB_ROOT;
> @@ -184,18 +185,25 @@ security_initcall(integrity_iintcache_init);
>  int integrity_kernel_read(struct file *file, loff_t offset,
>  			  char *addr, unsigned long count)
>  {
> -	mm_segment_t old_fs;
> -	char __user *buf = (char __user *)addr;
> +	struct inode *inode = file_inode(file);
> +	struct iovec iov = { .iov_base = addr, .iov_len = count };
> +	struct kiocb kiocb;
> +	struct iov_iter iter;
>  	ssize_t ret;
>  
> +	lockdep_assert_held(&inode->i_rwsem);
> +
>  	if (!(file->f_mode & FMODE_READ))
>  		return -EBADF;
> +	if (!file->f_op->integrity_read)
> +		return -EBADF;

Would you not want to call ->read_iter() in the NULL case
and have all FSs supported as today?

Thanks
Boaz

>  
> -	old_fs = get_fs();
> -	set_fs(get_ds());
> -	ret = __vfs_read(file, buf, count, &offset);
> -	set_fs(old_fs);
> +	init_sync_kiocb(&kiocb, file);
> +	kiocb.ki_pos = offset;
> +	iov_iter_init(&iter, READ | ITER_KVEC, &iov, 1, count);
>  
> +	ret = file->f_op->integrity_read(&kiocb, &iter);
> +	BUG_ON(ret == -EIOCBQUEUED);
>  	return ret;
>  }
>  
> 

Re: [PATCH] security/ima: use fs method to read integrity data

[Christoph Hellwig]

On Wed, May 10, 2017 at 03:20:41PM +0300, Boaz Harrosh wrote:
> Would you not want to call ->read_iter() in the NULL case
> and have all FSs supported as today?

As IMA has particular requirements on the fs (e.g. that it can
read with i_rwsem held as seen in this patch, or useful i_version
which only the file systems converted in this patch do), having
an explicit opt-in seems much safer.  This optional method is
a very easy way to provide this opt-in behavior.

Re: [PATCH] security/ima: use fs method to read integrity data

[Boaz Harrosh]

On 05/10/2017 04:24 PM, Christoph Hellwig wrote:
> On Wed, May 10, 2017 at 03:20:41PM +0300, Boaz Harrosh wrote:
>> Would you not want to call ->read_iter() in the NULL case
>> and have all FSs supported as today?
> 
> As IMA has particular requirements on the fs (e.g. that it can
> read with i_rwsem held as seen in this patch, or useful i_version
> which only the file systems converted in this patch do), having
> an explicit opt-in seems much safer.  This optional method is
> a very easy way to provide this opt-in behavior.
> 

>> +	if (!file->f_op->integrity_read)
>> +		return -EBADF;
> 
> Would you not want to call ->read_iter() in the NULL case
> and have all FSs supported as today?
> 
> Thanks
> Boaz
> 
>>  
>> -	old_fs = get_fs();
>> -	set_fs(get_ds());
>> -	ret = __vfs_read(file, buf, count, &offset);
>> -	set_fs(old_fs);

If you look here above, it used to call __vfs_read regardless
of i_version and "opt-in" so it looks like a regression for
all those FSs that used to work.

So I did not understand, but I guess you are right opt-in seems much
safer.

Thanks
Boaz

>> +	init_sync_kiocb(&kiocb, file);
>> +	kiocb.ki_pos = offset;
>> +	iov_iter_init(&iter, READ | ITER_KVEC, &iov, 1, count);

Re: [PATCH] security/ima: use fs method to read integrity data

[Mimi Zohar]

On Wed, 2017-05-10 at 15:24 +0200, Christoph Hellwig wrote:
> On Wed, May 10, 2017 at 03:20:41PM +0300, Boaz Harrosh wrote:
> > Would you not want to call ->read_iter() in the NULL case
> > and have all FSs supported as today?
> 
> As IMA has particular requirements on the fs (e.g. that it can
> read with i_rwsem held as seen in this patch, or useful i_version
> which only the file systems converted in this patch do), having
> an explicit opt-in seems much safer.  This optional method is
> a very easy way to provide this opt-in behavior.

Without i_version support the file is measured/appraised once.  With
i_version support it will be re-measured/appraised. As a file system
is mounted/remounted, some sort of message should be emitted
indicating whether i_version is supported.  That does not imply that
there is no value in measuring/appraising the file only once.

With this patch, the "opt-in" behavior, is only for measurement, not
appraisal.  For appraisal, it still enforces file hash/signature
verification, as it should, based on policy.

Christoph, could we call ->read_iter() in the NULL case as Boaz
suggested?

thanks!

Mimi

Re: [PATCH] security/ima: use fs method to read integrity data

[James Morris]

On Wed, 10 May 2017, Christoph Hellwig wrote:

> Add a new ->integrity_read file operation to read data for
> integrity hash collection.  This is defined to be equivalent
> to ->read_iter, except that it will be called with the i_rwsem
> held exclusively.  Also the presence of ->integrity_read indicates
> that the file system can support IMA.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Fixes an IMA+XFS deadlock I've been seeing.

Tested-by: James Morris <james.l.morris@oracle.com>


-- 
James Morris
<jmorris@namei.org>

Re: [PATCH] security/ima: use fs method to read integrity data

[Li Kun]

Why we don't make some rules like"the fs should use nested i_rwsem in 
read_iter" instead?

Although i think your approach is safer.


锟斤拷 2017/5/10 14:45, Christoph Hellwig 写锟斤拷:
> Add a new ->integrity_read file operation to read data for
> integrity hash collection.  This is defined to be equivalent
> to ->read_iter, except that it will be called with the i_rwsem
> held exclusively.  Also the presence of ->integrity_read indicates
> that the file system can support IMA.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   fs/btrfs/file.c           |  1 +
>   fs/ext4/file.c            |  1 +
>   fs/xfs/xfs_file.c         | 21 +++++++++++++++++++++
>   include/linux/fs.h        |  1 +
>   security/integrity/iint.c | 20 ++++++++++++++------
>   5 files changed, 38 insertions(+), 6 deletions(-)
>
> diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
> index 520cb7230b2d..c5afb2264737 100644
> --- a/fs/btrfs/file.c
> +++ b/fs/btrfs/file.c
> @@ -3041,6 +3041,7 @@ const struct file_operations btrfs_file_operations = {
>   #endif
>   	.clone_file_range = btrfs_clone_file_range,
>   	.dedupe_file_range = btrfs_dedupe_file_range,
> +	.integrity_read = generic_file_read_iter,
>   };
>   
>   void btrfs_auto_defrag_exit(void)
> diff --git a/fs/ext4/file.c b/fs/ext4/file.c
> index 8210c1f43556..b2a5025c7e0d 100644
> --- a/fs/ext4/file.c
> +++ b/fs/ext4/file.c
> @@ -740,6 +740,7 @@ const struct file_operations ext4_file_operations = {
>   	.splice_read	= generic_file_splice_read,
>   	.splice_write	= iter_file_splice_write,
>   	.fallocate	= ext4_fallocate,
> +	.integrity_read	= ext4_file_read_iter,
>   };
>   
>   const struct inode_operations ext4_file_inode_operations = {
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index 35703a801372..3d6ace2a79bc 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -288,6 +288,26 @@ xfs_file_read_iter(
>   	return ret;
>   }
>   
> +static ssize_t
> +xfs_integrity_read(
> +	struct kiocb		*iocb,
> +	struct iov_iter		*to)
> +{
> +	struct inode		*inode = file_inode(iocb->ki_filp);
> +	struct xfs_mount	*mp = XFS_I(inode)->i_mount;
> +
> +	lockdep_assert_held(&inode->i_rwsem);
> +
> +	XFS_STATS_INC(mp, xs_read_calls);
> +
> +	if (XFS_FORCED_SHUTDOWN(mp))
> +		return -EIO;
> +
> +	if (IS_DAX(inode))
> +		return dax_iomap_rw(iocb, to, &xfs_iomap_ops);
> +	return generic_file_read_iter(iocb, to);
> +}
> +
>   /*
>    * Zero any on disk space between the current EOF and the new, larger EOF.
>    *
> @@ -1534,6 +1554,7 @@ const struct file_operations xfs_file_operations = {
>   	.fallocate	= xfs_file_fallocate,
>   	.clone_file_range = xfs_file_clone_range,
>   	.dedupe_file_range = xfs_file_dedupe_range,
> +	.integrity_read	= xfs_integrity_read,
>   };
>   
>   const struct file_operations xfs_dir_file_operations = {
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 7251f7bb45e8..0c86816ca25f 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -1689,6 +1689,7 @@ struct file_operations {
>   			u64);
>   	ssize_t (*dedupe_file_range)(struct file *, u64, u64, struct file *,
>   			u64);
> +	ssize_t (*integrity_read)(struct kiocb *, struct iov_iter *);
>   };
>   
>   struct inode_operations {
> diff --git a/security/integrity/iint.c b/security/integrity/iint.c
> index c710d22042f9..c5489672b5aa 100644
> --- a/security/integrity/iint.c
> +++ b/security/integrity/iint.c
> @@ -21,6 +21,7 @@
>   #include <linux/rbtree.h>
>   #include <linux/file.h>
>   #include <linux/uaccess.h>
> +#include <linux/uio.h>
>   #include "integrity.h"
>   
>   static struct rb_root integrity_iint_tree = RB_ROOT;
> @@ -184,18 +185,25 @@ security_initcall(integrity_iintcache_init);
>   int integrity_kernel_read(struct file *file, loff_t offset,
>   			  char *addr, unsigned long count)
>   {
> -	mm_segment_t old_fs;
> -	char __user *buf = (char __user *)addr;
> +	struct inode *inode = file_inode(file);
> +	struct iovec iov = { .iov_base = addr, .iov_len = count };
> +	struct kiocb kiocb;
> +	struct iov_iter iter;
>   	ssize_t ret;
>   
> +	lockdep_assert_held(&inode->i_rwsem);
> +
>   	if (!(file->f_mode & FMODE_READ))
>   		return -EBADF;
> +	if (!file->f_op->integrity_read)
> +		return -EBADF;
>   
> -	old_fs = get_fs();
> -	set_fs(get_ds());
> -	ret = __vfs_read(file, buf, count, &offset);
> -	set_fs(old_fs);
> +	init_sync_kiocb(&kiocb, file);
> +	kiocb.ki_pos = offset;
> +	iov_iter_init(&iter, READ | ITER_KVEC, &iov, 1, count);
>   
> +	ret = file->f_op->integrity_read(&kiocb, &iter);
> +	BUG_ON(ret == -EIOCBQUEUED);
>   	return ret;
>   }
>   

-- 
Best Regards
Li Kun

Re: [PATCH] security/ima: use fs method to read integrity data

[Al Viro]

On Thu, May 11, 2017 at 08:34:18AM +0800, Li Kun wrote:
> Why we don't make some rules like"the fs should use nested i_rwsem in
> read_iter" instead?

What "nested i_rwsem"?

Re: [PATCH] security/ima: use fs method to read integrity data

[Li Kun]

Maybe i have misunderstood something about the implemention of taking 
rwsem nestedly in kernel.
Ignore my former email plz:)
在 2017/5/11 8:59, Al Viro 写道:
> On Thu, May 11, 2017 at 08:34:18AM +0800, Li Kun wrote:
>> Why we don't make some rules like"the fs should use nested i_rwsem in
>> read_iter" instead?
> What "nested i_rwsem"?

-- 
Best Regards
Li Kun

Re: [PATCH] security/ima: use fs method to read integrity data

[Christoph Hellwig]

On Wed, May 10, 2017 at 05:00:47PM -0400, Mimi Zohar wrote:
> Without i_version support the file is measured/appraised once. �With
> i_version support it will be re-measured/appraised. As a file system
> is mounted/remounted, some sort of message should be emitted
> indicating whether i_version is supported.

You can check for (sb->s_flags & MS_I_VERSION) to see if it's supported.

> �That does not imply that
> there is no value in measuring/appraising the file only once.
> 
> With this patch, the "opt-in" behavior, is only for measurement, not
> appraisal. �For appraisal, it still enforces file hash/signature
> verification, as it should, based on policy.
> 
> Christoph, could we call ->read_iter() in the NULL case as Boaz
> suggested?

No - that way you get deadlocks for every fs that uses i_rwsem in
->read_iter, which is perfectly valid behavior.

We can set ->integrity_read for every file system that's been tested
with IMA, though.  Do you have a list of known-good file systems?

Re: [PATCH] security/ima: use fs method to read integrity data

[Mimi Zohar]

On Thu, 2017-05-11 at 10:16 +0200, Christoph Hellwig wrote:
> On Wed, May 10, 2017 at 05:00:47PM -0400, Mimi Zohar wrote:
> > Without i_version support the file is measured/appraised once.  With
> > i_version support it will be re-measured/appraised. As a file system
> > is mounted/remounted, some sort of message should be emitted
> > indicating whether i_version is supported.
> 
> You can check for (sb->s_flags & MS_I_VERSION) to see if it's supported.

Yes, I defined a new LSM hook to catch the new mounts, but there are
lots of mounts, even after to limiting it to non-kernel mounts
(MS_KERNMOUNT) and only checking if the MS_I_VERSION is set on
filesystems mounted read-write.  It would be nice if there was a way
of saying not pseudo filesystems (eg. CGROUP_SUPER_MAGIC,
DEBUGFS_MAGIC, DEVPTS_SUPER_MAGIC, PROC_SUPER_MAGIC, SECURITYFS_MAGIC,
SYSFS_MAGIC, etc).

> 
> >  That does not imply that
> > there is no value in measuring/appraising the file only once.
> > 
> > With this patch, the "opt-in" behavior, is only for measurement, not
> > appraisal.  For appraisal, it still enforces file hash/signature
> > verification, as it should, based on policy.
> > 
> > Christoph, could we call ->read_iter() in the NULL case as Boaz
> > suggested?
> 
> No - that way you get deadlocks for every fs that uses i_rwsem in
> ->read_iter, which is perfectly valid behavior.
> 
> We can set ->integrity_read for every file system that's been tested
> with IMA, though.  Do you have a list of known-good file systems?

In addition to the ones you've already defined, we need definitions in
ramfs/file-mmu.c and file-nommu.c, and the corresponding tmpfs, to get
the initial measurements from the initramfs.
 
We know that stacked filesystems have similar locking problems.  I'm
loop back mounting each filesystem and testing to see if files are
being measured/re-measured properly.  I haven't finished yet, but
there haven't been any problems so far.

Mimi

Re: [PATCH] security/ima: use fs method to read integrity data

[Christoph Hellwig]

On Thu, May 11, 2017 at 09:59:51AM +1000, James Morris wrote:
> On Wed, 10 May 2017, Christoph Hellwig wrote:
> 
> > Add a new ->integrity_read file operation to read data for
> > integrity hash collection.  This is defined to be equivalent
> > to ->read_iter, except that it will be called with the i_rwsem
> > held exclusively.  Also the presence of ->integrity_read indicates
> > that the file system can support IMA.
> > 
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> Fixes an IMA+XFS deadlock I've been seeing.

Are you going to pick this up?  Any feedback from the folks on the
LSM list on what additional file systems need to be wired up?

Re: [PATCH] security/ima: use fs method to read integrity data

[James Morris]

On Sun, 4 Jun 2017, Christoph Hellwig wrote:

> On Thu, May 11, 2017 at 09:59:51AM +1000, James Morris wrote:
> > On Wed, 10 May 2017, Christoph Hellwig wrote:
> > 
> > > Add a new ->integrity_read file operation to read data for
> > > integrity hash collection.  This is defined to be equivalent
> > > to ->read_iter, except that it will be called with the i_rwsem
> > > held exclusively.  Also the presence of ->integrity_read indicates
> > > that the file system can support IMA.
> > > 
> > > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > 
> > Fixes an IMA+XFS deadlock I've been seeing.
> 
> Are you going to pick this up?  Any feedback from the folks on the
> LSM list on what additional file systems need to be wired up?

That would normally come in via Mimi's tree, but I will pick it up if it 
doesn't.


-- 
James Morris
<jmorris@namei.org>

Re: [PATCH] security/ima: use fs method to read integrity data

[Mimi Zohar]

On Mon, 2017-06-05 at 10:23 +1000, James Morris wrote:
> On Sun, 4 Jun 2017, Christoph Hellwig wrote:
> 
> > On Thu, May 11, 2017 at 09:59:51AM +1000, James Morris wrote:
> > > On Wed, 10 May 2017, Christoph Hellwig wrote:
> > > 
> > > > Add a new ->integrity_read file operation to read data for
> > > > integrity hash collection.  This is defined to be equivalent
> > > > to ->read_iter, except that it will be called with the i_rwsem
> > > > held exclusively.  Also the presence of ->integrity_read indicates
> > > > that the file system can support IMA.
> > > > 
> > > > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > > 
> > > Fixes an IMA+XFS deadlock I've been seeing.
> > 
> > Are you going to pick this up?  Any feedback from the folks on the
> > LSM list on what additional file systems need to be wired up?
> 
> That would normally come in via Mimi's tree, but I will pick it up if it 
> doesn't.

With just this patch, a number of file measurements and appraisal
verifications are missing.  I'm hoping to have an initial patch set
ready to be posted later this week.

Sorry for the delay.

Mimi