[PATCH 0/3] ima: only call integrity_kernel_read to calc file hash

[PATCH 0/3] ima: only call integrity_kernel_read to calc file hash

[Mimi Zohar]

The integrity_kernel_read() function was originally introduced to
read a file and calculate the file hash by-passing any security
checks.  Support subsequently was added allowing the kernel to read
a file containing a signed x509 certificate and load it onto either
the IMA or EVM keyring.

This patch set replaces the call to integrity_kernel_read() with the
common kernel_read_file_from_path() function, for reading and
loading an x509 certificate onto either the IMA or EVM keyring.  The
remaining calls to integrity_kernel_read() calculate a file hash, by
calling the new integrity_read file operation method.

Mimi


Christoph Hellwig (2):
  integrity: replace call to integrity_read_file with kernel version
  ima: use fs method to read integrity data

Mimi Zohar (1):
  vfs: constify path argument to kernel_read_file_from_path

 fs/btrfs/file.c                   |  1 +
 fs/efivarfs/file.c                |  1 +
 fs/exec.c                         |  2 +-
 fs/ext2/file.c                    | 17 ++++++++++
 fs/ext4/file.c                    | 20 ++++++++++++
 fs/f2fs/file.c                    |  1 +
 fs/jffs2/file.c                   |  1 +
 fs/jfs/file.c                     |  1 +
 fs/nilfs2/file.c                  |  1 +
 fs/ramfs/file-mmu.c               |  1 +
 fs/ramfs/file-nommu.c             |  1 +
 fs/ubifs/file.c                   |  1 +
 fs/xfs/xfs_file.c                 | 21 ++++++++++++
 include/linux/fs.h                |  4 ++-
 mm/shmem.c                        |  1 +
 security/integrity/digsig.c       | 14 +++++---
 security/integrity/iint.c         | 69 ++++++++-------------------------------
 security/integrity/ima/ima_main.c |  4 +++
 security/integrity/integrity.h    |  2 --
 sound/oss/sound_firmware.h        |  2 +-
 20 files changed, 100 insertions(+), 65 deletions(-)

-- 
2.7.4

[PATCH 1/3] vfs: constify path argument to kernel_read_file_from_path

[Mimi Zohar]

This patch constifies the path argument to kernel_read_file_from_path.

Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
---
 fs/exec.c                  | 2 +-
 include/linux/fs.h         | 2 +-
 sound/oss/sound_firmware.h | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index 62175cbcc801..54a4847649cc 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -974,7 +974,7 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size,
 }
 EXPORT_SYMBOL_GPL(kernel_read_file);
 
-int kernel_read_file_from_path(char *path, void **buf, loff_t *size,
+int kernel_read_file_from_path(const char *path, void **buf, loff_t *size,
 			       loff_t max_size, enum kernel_read_file_id id)
 {
 	struct file *file;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index fdec9b763b54..d783cc8340de 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2775,7 +2775,7 @@ static inline const char *kernel_read_file_id_str(enum kernel_read_file_id id)
 extern int kernel_read(struct file *, loff_t, char *, unsigned long);
 extern int kernel_read_file(struct file *, void **, loff_t *, loff_t,
 			    enum kernel_read_file_id);
-extern int kernel_read_file_from_path(char *, void **, loff_t *, loff_t,
+extern int kernel_read_file_from_path(const char *, void **, loff_t *, loff_t,
 				      enum kernel_read_file_id);
 extern int kernel_read_file_from_fd(int, void **, loff_t *, loff_t,
 				    enum kernel_read_file_id);
diff --git a/sound/oss/sound_firmware.h b/sound/oss/sound_firmware.h
index da4c67e005ed..2be465277ba0 100644
--- a/sound/oss/sound_firmware.h
+++ b/sound/oss/sound_firmware.h
@@ -21,7 +21,7 @@ static inline int mod_firmware_load(const char *fn, char **fp)
 	loff_t size;
 	int err;
 
-	err = kernel_read_file_from_path((char *)fn, (void **)fp, &size,
+	err = kernel_read_file_from_path(fn, (void **)fp, &size,
 					 131072, READING_FIRMWARE);
 	if (err < 0)
 		return 0;
-- 
2.7.4

Re: [PATCH 1/3] vfs: constify path argument to kernel_read_file_from_path

[Linus Torvalds]

On Thu, Sep 14, 2017 at 9:58 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> This patch constifies the path argument to kernel_read_file_from_path.

I've applied this upstream independently of everything else, because
it's obviously the right thing to do (as the sound_firmware.h part of
the patch shows, never mind the whole "we're just passing the pathname
to filp_open() which takes a const char"  thing).

               lINUS

[PATCH 2/3] integrity: replace call to integrity_read_file with kernel version

[Mimi Zohar]

From: Christoph Hellwig <hch@lst.de>

The CONFIG_IMA_LOAD_X509 and CONFIG_EVM_LOAD_X509 options permit
loading x509 signed certificates onto the trusted keyrings without
verifying the x509 certificate file's signature.

This patch replaces the call to the integrity_read_file() specific
function with the common kernel_read_file_from_path() function.
To avoid verifying the file signature, this patch defines
READING_X509_CERTFICATE.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>

---
Changelog:
- rewrote patch description
- fixed parameters to kernel_read_file_from_path() and key_create_or_update()
- defined READING_X509_CERTIFICATE, a new __kernel_read_file enumeration
- removed constify change
---
 include/linux/fs.h                |  1 +
 security/integrity/digsig.c       | 14 +++++++----
 security/integrity/iint.c         | 49 ---------------------------------------
 security/integrity/ima/ima_main.c |  4 ++++
 security/integrity/integrity.h    |  2 --
 5 files changed, 14 insertions(+), 56 deletions(-)

diff --git a/include/linux/fs.h b/include/linux/fs.h
index d783cc8340de..e522d25d0836 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2751,6 +2751,7 @@ extern int do_pipe_flags(int *, int);
 	id(KEXEC_IMAGE, kexec-image)		\
 	id(KEXEC_INITRAMFS, kexec-initramfs)	\
 	id(POLICY, security-policy)		\
+	id(X509_CERTIFICATE, x509-certificate)	\
 	id(MAX_ID, )
 
 #define __fid_enumify(ENUM, dummy) READING_ ## ENUM,
diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
index 06554c448dce..6f9e4ce568cd 100644
--- a/security/integrity/digsig.c
+++ b/security/integrity/digsig.c
@@ -112,21 +112,25 @@ int __init integrity_init_keyring(const unsigned int id)
 int __init integrity_load_x509(const unsigned int id, const char *path)
 {
 	key_ref_t key;
-	char *data;
+	void *data;
+	loff_t size;
 	int rc;
 
 	if (!keyring[id])
 		return -EINVAL;
 
-	rc = integrity_read_file(path, &data);
-	if (rc < 0)
+	rc = kernel_read_file_from_path(path, &data, &size, 0,
+					READING_X509_CERTIFICATE);
+	if (rc < 0) {
+		pr_err("Unable to open file: %s (%d)", path, rc);
 		return rc;
+	}
 
 	key = key_create_or_update(make_key_ref(keyring[id], 1),
 				   "asymmetric",
 				   NULL,
 				   data,
-				   rc,
+				   size,
 				   ((KEY_POS_ALL & ~KEY_POS_SETATTR) |
 				    KEY_USR_VIEW | KEY_USR_READ),
 				   KEY_ALLOC_NOT_IN_QUOTA);
@@ -139,6 +143,6 @@ int __init integrity_load_x509(const unsigned int id, const char *path)
 			  key_ref_to_ptr(key)->description, path);
 		key_ref_put(key);
 	}
-	kfree(data);
+	vfree(data);
 	return 0;
 }
diff --git a/security/integrity/iint.c b/security/integrity/iint.c
index 6fc888ca468e..c84e05866052 100644
--- a/security/integrity/iint.c
+++ b/security/integrity/iint.c
@@ -200,55 +200,6 @@ int integrity_kernel_read(struct file *file, loff_t offset,
 }
 
 /*
- * integrity_read_file - read entire file content into the buffer
- *
- * This is function opens a file, allocates the buffer of required
- * size, read entire file content to the buffer and closes the file
- *
- * It is used only by init code.
- *
- */
-int __init integrity_read_file(const char *path, char **data)
-{
-	struct file *file;
-	loff_t size;
-	char *buf;
-	int rc = -EINVAL;
-
-	if (!path || !*path)
-		return -EINVAL;
-
-	file = filp_open(path, O_RDONLY, 0);
-	if (IS_ERR(file)) {
-		rc = PTR_ERR(file);
-		pr_err("Unable to open file: %s (%d)", path, rc);
-		return rc;
-	}
-
-	size = i_size_read(file_inode(file));
-	if (size <= 0)
-		goto out;
-
-	buf = kmalloc(size, GFP_KERNEL);
-	if (!buf) {
-		rc = -ENOMEM;
-		goto out;
-	}
-
-	rc = integrity_kernel_read(file, 0, buf, size);
-	if (rc == size) {
-		*data = buf;
-	} else {
-		kfree(buf);
-		if (rc >= 0)
-			rc = -EIO;
-	}
-out:
-	fput(file);
-	return rc;
-}
-
-/*
  * integrity_load_keys - load integrity keys hook
  *
  * Hooks is called from init/main.c:kernel_init_freeable()
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index b00186914df8..72bd2b666a31 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -413,6 +413,10 @@ int ima_post_read_file(struct file *file, void *buf, loff_t size,
 	if (!file && read_id == READING_MODULE) /* MODULE_SIG_FORCE enabled */
 		return 0;
 
+	/* permit signed certs */
+	if (!file && read_id == READING_X509_CERTIFICATE)
+		return 0;
+
 	if (!file || !buf || size == 0) { /* should never happen */
 		if (ima_appraise & IMA_APPRAISE_ENFORCE)
 			return -EACCES;
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index a53e7e4ab06c..e1bf040fb110 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -120,8 +120,6 @@ struct integrity_iint_cache *integrity_iint_find(struct inode *inode);
 int integrity_kernel_read(struct file *file, loff_t offset,
 			  void *addr, unsigned long count);
 
-int __init integrity_read_file(const char *path, char **data);
-
 #define INTEGRITY_KEYRING_EVM		0
 #define INTEGRITY_KEYRING_IMA		1
 #define INTEGRITY_KEYRING_MODULE	2
-- 
2.7.4

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

[Mimi Zohar]

From: Christoph Hellwig <hch@lst.de>

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.

(Based on Christoph's original patch.)

Signed-off-by: Christoph Hellwig <hch@lst.de>
Cc: Matthew Garrett <mjg59@srcf.ucam.org>
Cc: Jan Kara <jack@suse.com>
Cc: "Theodore Ts'o" <tytso@mit.edu>
Cc: Andreas Dilger <adilger.kernel@dilger.ca>
Cc: Jaegeuk Kim <jaegeuk@kernel.org>
Cc: Chao Yu <yuchao0@huawei.com>
Cc: Steven Whitehouse <swhiteho@redhat.com>
Cc: Bob Peterson <rpeterso@redhat.com>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Dave Kleikamp <shaggy@kernel.org>
Cc: Ryusuke Konishi <konishi.ryusuke@lab.ntt.co.jp>
Cc: Mark Fasheh <mfasheh@versity.com>
Cc: Joel Becker <jlbec@evilplan.org>
Cc: Richard Weinberger <richard@nod.at>
Cc: "Darrick J. Wong" <darrick.wong@oracle.com>
Cc: Hugh Dickins <hughd@google.com>
Cc: Chris Mason <clm@fb.com>
Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
Reviewed-by: Jan Kara <jack@suse.cz>
Reviewed-by: Dmitry Kasatkin <dmitry.kasatkin@huawei.com>
---
 fs/btrfs/file.c           |  1 +
 fs/efivarfs/file.c        |  1 +
 fs/ext2/file.c            | 17 +++++++++++++++++
 fs/ext4/file.c            | 20 ++++++++++++++++++++
 fs/f2fs/file.c            |  1 +
 fs/jffs2/file.c           |  1 +
 fs/jfs/file.c             |  1 +
 fs/nilfs2/file.c          |  1 +
 fs/ramfs/file-mmu.c       |  1 +
 fs/ramfs/file-nommu.c     |  1 +
 fs/ubifs/file.c           |  1 +
 fs/xfs/xfs_file.c         | 21 +++++++++++++++++++++
 include/linux/fs.h        |  1 +
 mm/shmem.c                |  1 +
 security/integrity/iint.c | 20 ++++++++++++++------
 15 files changed, 83 insertions(+), 6 deletions(-)

diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index 9e75d8a39aac..2542dc66c85c 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -3125,6 +3125,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/efivarfs/file.c b/fs/efivarfs/file.c
index 863f1b100165..17955a92a5b3 100644
--- a/fs/efivarfs/file.c
+++ b/fs/efivarfs/file.c
@@ -179,4 +179,5 @@ const struct file_operations efivarfs_file_operations = {
 	.write	= efivarfs_file_write,
 	.llseek	= no_llseek,
 	.unlocked_ioctl = efivarfs_file_ioctl,
+	.integrity_read	= efivarfs_file_read_iter,
 };
diff --git a/fs/ext2/file.c b/fs/ext2/file.c
index d34d32bdc944..111069de1973 100644
--- a/fs/ext2/file.c
+++ b/fs/ext2/file.c
@@ -192,6 +192,22 @@ static ssize_t ext2_file_read_iter(struct kiocb *iocb, struct iov_iter *to)
 	return generic_file_read_iter(iocb, to);
 }
 
+static ssize_t ext2_file_integrity_read_iter(struct kiocb *iocb,
+					     struct iov_iter *to)
+{
+	struct inode *inode = file_inode(iocb->ki_filp);
+
+	lockdep_assert_held(&inode->i_rwsem);
+#ifdef CONFIG_FS_DAX
+	if (!iov_iter_count(to))
+		return 0; /* skip atime */
+
+	if (IS_DAX(iocb->ki_filp->f_mapping->host))
+		return dax_iomap_rw(iocb, to, &ext2_iomap_ops);
+#endif
+	return generic_file_read_iter(iocb, to);
+}
+
 static ssize_t ext2_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
 {
 #ifdef CONFIG_FS_DAX
@@ -216,6 +232,7 @@ const struct file_operations ext2_file_operations = {
 	.get_unmapped_area = thp_get_unmapped_area,
 	.splice_read	= generic_file_splice_read,
 	.splice_write	= iter_file_splice_write,
+	.integrity_read	= ext2_file_integrity_read_iter,
 };
 
 const struct inode_operations ext2_file_inode_operations = {
diff --git a/fs/ext4/file.c b/fs/ext4/file.c
index 58294c9a7e1d..3ab4105c8578 100644
--- a/fs/ext4/file.c
+++ b/fs/ext4/file.c
@@ -74,6 +74,25 @@ static ssize_t ext4_file_read_iter(struct kiocb *iocb, struct iov_iter *to)
 	return generic_file_read_iter(iocb, to);
 }
 
+static ssize_t ext4_file_integrity_read_iter(struct kiocb *iocb,
+					     struct iov_iter *to)
+{
+	struct inode *inode = file_inode(iocb->ki_filp);
+
+	lockdep_assert_held(&inode->i_rwsem);
+	if (unlikely(ext4_forced_shutdown(EXT4_SB(inode->i_sb))))
+		return -EIO;
+
+	if (!iov_iter_count(to))
+		return 0; /* skip atime */
+
+#ifdef CONFIG_FS_DAX
+	if (IS_DAX(inode))
+		return dax_iomap_rw(iocb, to, &ext4_iomap_ops);
+#endif
+	return generic_file_read_iter(iocb, to);
+}
+
 /*
  * Called when an inode is released. Note that this is different
  * from ext4_file_open: open gets called at every open, but release
@@ -747,6 +766,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_integrity_read_iter,
 };
 
 const struct inode_operations ext4_file_inode_operations = {
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 2706130c261b..82ea81da0b2d 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -2514,4 +2514,5 @@ const struct file_operations f2fs_file_operations = {
 #endif
 	.splice_read	= generic_file_splice_read,
 	.splice_write	= iter_file_splice_write,
+	.integrity_read	= generic_file_read_iter,
 };
diff --git a/fs/jffs2/file.c b/fs/jffs2/file.c
index c12476e309c6..5a63034cccf5 100644
--- a/fs/jffs2/file.c
+++ b/fs/jffs2/file.c
@@ -57,6 +57,7 @@ const struct file_operations jffs2_file_operations =
 	.mmap =		generic_file_readonly_mmap,
 	.fsync =	jffs2_fsync,
 	.splice_read =	generic_file_splice_read,
+	.integrity_read = generic_file_read_iter,
 };
 
 /* jffs2_file_inode_operations */
diff --git a/fs/jfs/file.c b/fs/jfs/file.c
index 739492c7a3fd..423512a810e4 100644
--- a/fs/jfs/file.c
+++ b/fs/jfs/file.c
@@ -162,4 +162,5 @@ const struct file_operations jfs_file_operations = {
 #ifdef CONFIG_COMPAT
 	.compat_ioctl	= jfs_compat_ioctl,
 #endif
+	.integrity_read	= generic_file_read_iter,
 };
diff --git a/fs/nilfs2/file.c b/fs/nilfs2/file.c
index c5fa3dee72fc..55e058ac487f 100644
--- a/fs/nilfs2/file.c
+++ b/fs/nilfs2/file.c
@@ -150,6 +150,7 @@ const struct file_operations nilfs_file_operations = {
 	/* .release	= nilfs_release_file, */
 	.fsync		= nilfs_sync_file,
 	.splice_read	= generic_file_splice_read,
+	.integrity_read	= generic_file_read_iter,
 };
 
 const struct inode_operations nilfs_file_inode_operations = {
diff --git a/fs/ramfs/file-mmu.c b/fs/ramfs/file-mmu.c
index 12af0490322f..4f24d1b589b1 100644
--- a/fs/ramfs/file-mmu.c
+++ b/fs/ramfs/file-mmu.c
@@ -47,6 +47,7 @@ const struct file_operations ramfs_file_operations = {
 	.splice_write	= iter_file_splice_write,
 	.llseek		= generic_file_llseek,
 	.get_unmapped_area	= ramfs_mmu_get_unmapped_area,
+	.integrity_read	= generic_file_read_iter,
 };
 
 const struct inode_operations ramfs_file_inode_operations = {
diff --git a/fs/ramfs/file-nommu.c b/fs/ramfs/file-nommu.c
index 2ef7ce75c062..5ee704fa84e0 100644
--- a/fs/ramfs/file-nommu.c
+++ b/fs/ramfs/file-nommu.c
@@ -50,6 +50,7 @@ const struct file_operations ramfs_file_operations = {
 	.splice_read		= generic_file_splice_read,
 	.splice_write		= iter_file_splice_write,
 	.llseek			= generic_file_llseek,
+	.integrity_read		= generic_file_read_iter,
 };
 
 const struct inode_operations ramfs_file_inode_operations = {
diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
index 8cad0b19b404..5e52a315e18b 100644
--- a/fs/ubifs/file.c
+++ b/fs/ubifs/file.c
@@ -1747,4 +1747,5 @@ const struct file_operations ubifs_file_operations = {
 #ifdef CONFIG_COMPAT
 	.compat_ioctl   = ubifs_compat_ioctl,
 #endif
+	.integrity_read = generic_file_read_iter,
 };
diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index c4893e226fd8..0a6704b563d6 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -292,6 +292,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.
  *
@@ -1175,6 +1195,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 e522d25d0836..f6a01d3efce5 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1699,6 +1699,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 *);
 } __randomize_layout;
 
 struct inode_operations {
diff --git a/mm/shmem.c b/mm/shmem.c
index b0aa6075d164..805d99011ca4 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -3849,6 +3849,7 @@ static const struct file_operations shmem_file_operations = {
 	.splice_read	= generic_file_splice_read,
 	.splice_write	= iter_file_splice_write,
 	.fallocate	= shmem_fallocate,
+	.integrity_read	= shmem_file_read_iter,
 #endif
 };
 
diff --git a/security/integrity/iint.c b/security/integrity/iint.c
index c84e05866052..c3a07276f745 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,
 			  void *addr, unsigned long count)
 {
-	mm_segment_t old_fs;
-	char __user *buf = (char __user *)addr;
+	struct inode *inode = file_inode(file);
+	struct kvec 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_kvec(&iter, READ | ITER_KVEC, &iov, 1, count);
 
+	ret = file->f_op->integrity_read(&kiocb, &iter);
+	BUG_ON(ret == -EIOCBQUEUED);
 	return ret;
 }
 
-- 
2.7.4

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

[Mimi Zohar]

On Thu, 2017-09-14 at 22:50 -0700, Linus Torvalds wrote:
> This is still wrong.
> 
> (a) there is no explanation for why we need that exclusive lock in the
> first place

> Why should a read need exclusive access? You'd think shared is sufficient.

True, reading a file shouldn't require an exclusive lock.  The
exclusive lock is taken to prevent the file from changing while the
file hash is being calculated.

> But regardless, it needs *explanation*.

Agreed.  A fuller explanation was included in the cover letter that
should have also been included in the patch description.  The
following is taken from the cover letter:

With the introduction of IMA-appraisal and the need to write file
hashes as security xattrs, IMA needed to take the global i_mutex
lock.  process_measurement() took the iint->mutex first and then
the i_mutex, while setxattr, chmod and chown took the locks in
reverse order.  To resolve this potential deadlock, the iint->mutex
was removed.

Some filesystems have recently replaced their filesystem dependent
lock with the global i_rwsem (formerly the i_mutex) to read a file.
As a result, when IMA attempts to calculate the file hash, reading
the file attempts to take the i_rwsem again.

To resolve this locking problem, this patch set defines a new
->integrity_read file operation method.

(Fixes: Commit 6552321831dc "xfs: remove i_iolock and use i_rwsem in
the VFS inode instead")

Mimi

> (b) the lockdep test isn't for the exclusive lock that the code comment
> says it is
> 
> So no, this needs more work.
> 
>     Linus
> 

> 
> On Sep 14, 2017 21:59, "Mimi Zohar" <zohar@linux.vnet.ibm.com> wrote:
> 
> > From: Christoph Hellwig <hch@lst.de>
> >
> > 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.
> >
> > (Based on Christoph's original patch.)
> >
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > Cc: Matthew Garrett <mjg59@srcf.ucam.org>
> > Cc: Jan Kara <jack@suse.com>
> > Cc: "Theodore Ts'o" <tytso@mit.edu>
> > Cc: Andreas Dilger <adilger.kernel@dilger.ca>
> > Cc: Jaegeuk Kim <jaegeuk@kernel.org>
> > Cc: Chao Yu <yuchao0@huawei.com>
> > Cc: Steven Whitehouse <swhiteho@redhat.com>
> > Cc: Bob Peterson <rpeterso@redhat.com>
> > Cc: David Woodhouse <dwmw2@infradead.org>
> > Cc: Dave Kleikamp <shaggy@kernel.org>
> > Cc: Ryusuke Konishi <konishi.ryusuke@lab.ntt.co.jp>
> > Cc: Mark Fasheh <mfasheh@versity.com>
> > Cc: Joel Becker <jlbec@evilplan.org>
> > Cc: Richard Weinberger <richard@nod.at>
> > Cc: "Darrick J. Wong" <darrick.wong@oracle.com>
> > Cc: Hugh Dickins <hughd@google.com>
> > Cc: Chris Mason <clm@fb.com>
> > Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
> > Reviewed-by: Jan Kara <jack@suse.cz>
> > Reviewed-by: Dmitry Kasatkin <dmitry.kasatkin@huawei.com>
> > ---
> >  fs/btrfs/file.c           |  1 +
> >  fs/efivarfs/file.c        |  1 +
> >  fs/ext2/file.c            | 17 +++++++++++++++++
> >  fs/ext4/file.c            | 20 ++++++++++++++++++++
> >  fs/f2fs/file.c            |  1 +
> >  fs/jffs2/file.c           |  1 +
> >  fs/jfs/file.c             |  1 +
> >  fs/nilfs2/file.c          |  1 +
> >  fs/ramfs/file-mmu.c       |  1 +
> >  fs/ramfs/file-nommu.c     |  1 +
> >  fs/ubifs/file.c           |  1 +
> >  fs/xfs/xfs_file.c         | 21 +++++++++++++++++++++
> >  include/linux/fs.h        |  1 +
> >  mm/shmem.c                |  1 +
> >  security/integrity/iint.c | 20 ++++++++++++++------
> >  15 files changed, 83 insertions(+), 6 deletions(-)
> >
> > diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
> > index 9e75d8a39aac..2542dc66c85c 100644
> > --- a/fs/btrfs/file.c
> > +++ b/fs/btrfs/file.c
> > @@ -3125,6 +3125,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/efivarfs/file.c b/fs/efivarfs/file.c
> > index 863f1b100165..17955a92a5b3 100644
> > --- a/fs/efivarfs/file.c
> > +++ b/fs/efivarfs/file.c
> > @@ -179,4 +179,5 @@ const struct file_operations efivarfs_file_operations
> > = {
> >         .write  = efivarfs_file_write,
> >         .llseek = no_llseek,
> >         .unlocked_ioctl = efivarfs_file_ioctl,
> > +       .integrity_read = efivarfs_file_read_iter,
> >  };
> > diff --git a/fs/ext2/file.c b/fs/ext2/file.c
> > index d34d32bdc944..111069de1973 100644
> > --- a/fs/ext2/file.c
> > +++ b/fs/ext2/file.c
> > @@ -192,6 +192,22 @@ static ssize_t ext2_file_read_iter(struct kiocb
> > *iocb, struct iov_iter *to)
> >         return generic_file_read_iter(iocb, to);
> >  }
> >
> > +static ssize_t ext2_file_integrity_read_iter(struct kiocb *iocb,
> > +                                            struct iov_iter *to)
> > +{
> > +       struct inode *inode = file_inode(iocb->ki_filp);
> > +
> > +       lockdep_assert_held(&inode->i_rwsem);
> > +#ifdef CONFIG_FS_DAX
> > +       if (!iov_iter_count(to))
> > +               return 0; /* skip atime */
> > +
> > +       if (IS_DAX(iocb->ki_filp->f_mapping->host))
> > +               return dax_iomap_rw(iocb, to, &ext2_iomap_ops);
> > +#endif
> > +       return generic_file_read_iter(iocb, to);
> > +}
> > +
> >  static ssize_t ext2_file_write_iter(struct kiocb *iocb, struct iov_iter
> > *from)
> >  {
> >  #ifdef CONFIG_FS_DAX
> > @@ -216,6 +232,7 @@ const struct file_operations ext2_file_operations = {
> >         .get_unmapped_area = thp_get_unmapped_area,
> >         .splice_read    = generic_file_splice_read,
> >         .splice_write   = iter_file_splice_write,
> > +       .integrity_read = ext2_file_integrity_read_iter,
> >  };
> >
> >  const struct inode_operations ext2_file_inode_operations = {
> > diff --git a/fs/ext4/file.c b/fs/ext4/file.c
> > index 58294c9a7e1d..3ab4105c8578 100644
> > --- a/fs/ext4/file.c
> > +++ b/fs/ext4/file.c
> > @@ -74,6 +74,25 @@ static ssize_t ext4_file_read_iter(struct kiocb *iocb,
> > struct iov_iter *to)
> >         return generic_file_read_iter(iocb, to);
> >  }
> >
> > +static ssize_t ext4_file_integrity_read_iter(struct kiocb *iocb,
> > +                                            struct iov_iter *to)
> > +{
> > +       struct inode *inode = file_inode(iocb->ki_filp);
> > +
> > +       lockdep_assert_held(&inode->i_rwsem);
> > +       if (unlikely(ext4_forced_shutdown(EXT4_SB(inode->i_sb))))
> > +               return -EIO;
> > +
> > +       if (!iov_iter_count(to))
> > +               return 0; /* skip atime */
> > +
> > +#ifdef CONFIG_FS_DAX
> > +       if (IS_DAX(inode))
> > +               return dax_iomap_rw(iocb, to, &ext4_iomap_ops);
> > +#endif
> > +       return generic_file_read_iter(iocb, to);
> > +}
> > +
> >  /*
> >   * Called when an inode is released. Note that this is different
> >   * from ext4_file_open: open gets called at every open, but release
> > @@ -747,6 +766,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_integrity_read_iter,
> >  };
> >
> >  const struct inode_operations ext4_file_inode_operations = {
> > diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> > index 2706130c261b..82ea81da0b2d 100644
> > --- a/fs/f2fs/file.c
> > +++ b/fs/f2fs/file.c
> > @@ -2514,4 +2514,5 @@ const struct file_operations f2fs_file_operations = {
> >  #endif
> >         .splice_read    = generic_file_splice_read,
> >         .splice_write   = iter_file_splice_write,
> > +       .integrity_read = generic_file_read_iter,
> >  };
> > diff --git a/fs/jffs2/file.c b/fs/jffs2/file.c
> > index c12476e309c6..5a63034cccf5 100644
> > --- a/fs/jffs2/file.c
> > +++ b/fs/jffs2/file.c
> > @@ -57,6 +57,7 @@ const struct file_operations jffs2_file_operations =
> >         .mmap =         generic_file_readonly_mmap,
> >         .fsync =        jffs2_fsync,
> >         .splice_read =  generic_file_splice_read,
> > +       .integrity_read = generic_file_read_iter,
> >  };
> >
> >  /* jffs2_file_inode_operations */
> > diff --git a/fs/jfs/file.c b/fs/jfs/file.c
> > index 739492c7a3fd..423512a810e4 100644
> > --- a/fs/jfs/file.c
> > +++ b/fs/jfs/file.c
> > @@ -162,4 +162,5 @@ const struct file_operations jfs_file_operations = {
> >  #ifdef CONFIG_COMPAT
> >         .compat_ioctl   = jfs_compat_ioctl,
> >  #endif
> > +       .integrity_read = generic_file_read_iter,
> >  };
> > diff --git a/fs/nilfs2/file.c b/fs/nilfs2/file.c
> > index c5fa3dee72fc..55e058ac487f 100644
> > --- a/fs/nilfs2/file.c
> > +++ b/fs/nilfs2/file.c
> > @@ -150,6 +150,7 @@ const struct file_operations nilfs_file_operations = {
> >         /* .release     = nilfs_release_file, */
> >         .fsync          = nilfs_sync_file,
> >         .splice_read    = generic_file_splice_read,
> > +       .integrity_read = generic_file_read_iter,
> >  };
> >
> >  const struct inode_operations nilfs_file_inode_operations = {
> > diff --git a/fs/ramfs/file-mmu.c b/fs/ramfs/file-mmu.c
> > index 12af0490322f..4f24d1b589b1 100644
> > --- a/fs/ramfs/file-mmu.c
> > +++ b/fs/ramfs/file-mmu.c
> > @@ -47,6 +47,7 @@ const struct file_operations ramfs_file_operations = {
> >         .splice_write   = iter_file_splice_write,
> >         .llseek         = generic_file_llseek,
> >         .get_unmapped_area      = ramfs_mmu_get_unmapped_area,
> > +       .integrity_read = generic_file_read_iter,
> >  };
> >
> >  const struct inode_operations ramfs_file_inode_operations = {
> > diff --git a/fs/ramfs/file-nommu.c b/fs/ramfs/file-nommu.c
> > index 2ef7ce75c062..5ee704fa84e0 100644
> > --- a/fs/ramfs/file-nommu.c
> > +++ b/fs/ramfs/file-nommu.c
> > @@ -50,6 +50,7 @@ const struct file_operations ramfs_file_operations = {
> >         .splice_read            = generic_file_splice_read,
> >         .splice_write           = iter_file_splice_write,
> >         .llseek                 = generic_file_llseek,
> > +       .integrity_read         = generic_file_read_iter,
> >  };
> >
> >  const struct inode_operations ramfs_file_inode_operations = {
> > diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
> > index 8cad0b19b404..5e52a315e18b 100644
> > --- a/fs/ubifs/file.c
> > +++ b/fs/ubifs/file.c
> > @@ -1747,4 +1747,5 @@ const struct file_operations ubifs_file_operations =
> > {
> >  #ifdef CONFIG_COMPAT
> >         .compat_ioctl   = ubifs_compat_ioctl,
> >  #endif
> > +       .integrity_read = generic_file_read_iter,
> >  };
> > diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> > index c4893e226fd8..0a6704b563d6 100644
> > --- a/fs/xfs/xfs_file.c
> > +++ b/fs/xfs/xfs_file.c
> > @@ -292,6 +292,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.
> >   *
> > @@ -1175,6 +1195,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 e522d25d0836..f6a01d3efce5 100644
> > --- a/include/linux/fs.h
> > +++ b/include/linux/fs.h
> > @@ -1699,6 +1699,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 *);
> >  } __randomize_layout;
> >
> >  struct inode_operations {
> > diff --git a/mm/shmem.c b/mm/shmem.c
> > index b0aa6075d164..805d99011ca4 100644
> > --- a/mm/shmem.c
> > +++ b/mm/shmem.c
> > @@ -3849,6 +3849,7 @@ static const struct file_operations
> > shmem_file_operations = {
> >         .splice_read    = generic_file_splice_read,
> >         .splice_write   = iter_file_splice_write,
> >         .fallocate      = shmem_fallocate,
> > +       .integrity_read = shmem_file_read_iter,
> >  #endif
> >  };
> >
> > diff --git a/security/integrity/iint.c b/security/integrity/iint.c
> > index c84e05866052..c3a07276f745 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,
> >                           void *addr, unsigned long count)
> >  {
> > -       mm_segment_t old_fs;
> > -       char __user *buf = (char __user *)addr;
> > +       struct inode *inode = file_inode(file);
> > +       struct kvec 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_kvec(&iter, READ | ITER_KVEC, &iov, 1, count);
> >
> > +       ret = file->f_op->integrity_read(&kiocb, &iter);
> > +       BUG_ON(ret == -EIOCBQUEUED);
> >         return ret;
> >  }
> >
> > --
> > 2.7.4
> >
> >

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

[Mimi Zohar]

On Fri, 2017-09-15 at 05:04 -0400, Mimi Zohar wrote:
> On Thu, 2017-09-14 at 22:50 -0700, Linus Torvalds wrote:
> > This is still wrong.
> > 
> > (a) there is no explanation for why we need that exclusive lock in the
> > first place
> 
> > Why should a read need exclusive access? You'd think shared is sufficient.
> 
> True, reading a file shouldn't require an exclusive lock.  The
> exclusive lock is taken to prevent the file from changing while the
> file hash is being calculated.

And also to prevent the file hash from being calculated multiple
times.

> 
> > But regardless, it needs *explanation*.
> 
> Agreed.  A fuller explanation was included in the cover letter that
> should have also been included in the patch description.  The
> following is taken from the cover letter:
> 
> With the introduction of IMA-appraisal and the need to write file
> hashes as security xattrs, IMA needed to take the global i_mutex
> lock.  process_measurement() took the iint->mutex first and then
> the i_mutex, while setxattr, chmod and chown took the locks in
> reverse order.  To resolve this potential deadlock, the iint->mutex
> was removed.
> 
> Some filesystems have recently replaced their filesystem dependent
> lock with the global i_rwsem (formerly the i_mutex) to read a file.
> As a result, when IMA attempts to calculate the file hash, reading
> the file attempts to take the i_rwsem again.
> 
> To resolve this locking problem, this patch set defines a new
> ->integrity_read file operation method.
> 
> (Fixes: Commit 6552321831dc "xfs: remove i_iolock and use i_rwsem in
> the VFS inode instead")
> 
> Mimi
> 
> > (b) the lockdep test isn't for the exclusive lock that the code comment
> > says it is
> > 
> > So no, this needs more work.
> > 
> >     Linus
> > 
> 
> > 
> > On Sep 14, 2017 21:59, "Mimi Zohar" <zohar@linux.vnet.ibm.com> wrote:
> > 
> > > From: Christoph Hellwig <hch@lst.de>
> > >
> > > 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.
> > >
> > > (Based on Christoph's original patch.)
> > >
> > > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > > Cc: Matthew Garrett <mjg59@srcf.ucam.org>
> > > Cc: Jan Kara <jack@suse.com>
> > > Cc: "Theodore Ts'o" <tytso@mit.edu>
> > > Cc: Andreas Dilger <adilger.kernel@dilger.ca>
> > > Cc: Jaegeuk Kim <jaegeuk@kernel.org>
> > > Cc: Chao Yu <yuchao0@huawei.com>
> > > Cc: Steven Whitehouse <swhiteho@redhat.com>
> > > Cc: Bob Peterson <rpeterso@redhat.com>
> > > Cc: David Woodhouse <dwmw2@infradead.org>
> > > Cc: Dave Kleikamp <shaggy@kernel.org>
> > > Cc: Ryusuke Konishi <konishi.ryusuke@lab.ntt.co.jp>
> > > Cc: Mark Fasheh <mfasheh@versity.com>
> > > Cc: Joel Becker <jlbec@evilplan.org>
> > > Cc: Richard Weinberger <richard@nod.at>
> > > Cc: "Darrick J. Wong" <darrick.wong@oracle.com>
> > > Cc: Hugh Dickins <hughd@google.com>
> > > Cc: Chris Mason <clm@fb.com>
> > > Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
> > > Reviewed-by: Jan Kara <jack@suse.cz>
> > > Reviewed-by: Dmitry Kasatkin <dmitry.kasatkin@huawei.com>
> > > ---
> > >  fs/btrfs/file.c           |  1 +
> > >  fs/efivarfs/file.c        |  1 +
> > >  fs/ext2/file.c            | 17 +++++++++++++++++
> > >  fs/ext4/file.c            | 20 ++++++++++++++++++++
> > >  fs/f2fs/file.c            |  1 +
> > >  fs/jffs2/file.c           |  1 +
> > >  fs/jfs/file.c             |  1 +
> > >  fs/nilfs2/file.c          |  1 +
> > >  fs/ramfs/file-mmu.c       |  1 +
> > >  fs/ramfs/file-nommu.c     |  1 +
> > >  fs/ubifs/file.c           |  1 +
> > >  fs/xfs/xfs_file.c         | 21 +++++++++++++++++++++
> > >  include/linux/fs.h        |  1 +
> > >  mm/shmem.c                |  1 +
> > >  security/integrity/iint.c | 20 ++++++++++++++------
> > >  15 files changed, 83 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
> > > index 9e75d8a39aac..2542dc66c85c 100644
> > > --- a/fs/btrfs/file.c
> > > +++ b/fs/btrfs/file.c
> > > @@ -3125,6 +3125,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/efivarfs/file.c b/fs/efivarfs/file.c
> > > index 863f1b100165..17955a92a5b3 100644
> > > --- a/fs/efivarfs/file.c
> > > +++ b/fs/efivarfs/file.c
> > > @@ -179,4 +179,5 @@ const struct file_operations efivarfs_file_operations
> > > = {
> > >         .write  = efivarfs_file_write,
> > >         .llseek = no_llseek,
> > >         .unlocked_ioctl = efivarfs_file_ioctl,
> > > +       .integrity_read = efivarfs_file_read_iter,
> > >  };
> > > diff --git a/fs/ext2/file.c b/fs/ext2/file.c
> > > index d34d32bdc944..111069de1973 100644
> > > --- a/fs/ext2/file.c
> > > +++ b/fs/ext2/file.c
> > > @@ -192,6 +192,22 @@ static ssize_t ext2_file_read_iter(struct kiocb
> > > *iocb, struct iov_iter *to)
> > >         return generic_file_read_iter(iocb, to);
> > >  }
> > >
> > > +static ssize_t ext2_file_integrity_read_iter(struct kiocb *iocb,
> > > +                                            struct iov_iter *to)
> > > +{
> > > +       struct inode *inode = file_inode(iocb->ki_filp);
> > > +
> > > +       lockdep_assert_held(&inode->i_rwsem);
> > > +#ifdef CONFIG_FS_DAX
> > > +       if (!iov_iter_count(to))
> > > +               return 0; /* skip atime */
> > > +
> > > +       if (IS_DAX(iocb->ki_filp->f_mapping->host))
> > > +               return dax_iomap_rw(iocb, to, &ext2_iomap_ops);
> > > +#endif
> > > +       return generic_file_read_iter(iocb, to);
> > > +}
> > > +
> > >  static ssize_t ext2_file_write_iter(struct kiocb *iocb, struct iov_iter
> > > *from)
> > >  {
> > >  #ifdef CONFIG_FS_DAX
> > > @@ -216,6 +232,7 @@ const struct file_operations ext2_file_operations = {
> > >         .get_unmapped_area = thp_get_unmapped_area,
> > >         .splice_read    = generic_file_splice_read,
> > >         .splice_write   = iter_file_splice_write,
> > > +       .integrity_read = ext2_file_integrity_read_iter,
> > >  };
> > >
> > >  const struct inode_operations ext2_file_inode_operations = {
> > > diff --git a/fs/ext4/file.c b/fs/ext4/file.c
> > > index 58294c9a7e1d..3ab4105c8578 100644
> > > --- a/fs/ext4/file.c
> > > +++ b/fs/ext4/file.c
> > > @@ -74,6 +74,25 @@ static ssize_t ext4_file_read_iter(struct kiocb *iocb,
> > > struct iov_iter *to)
> > >         return generic_file_read_iter(iocb, to);
> > >  }
> > >
> > > +static ssize_t ext4_file_integrity_read_iter(struct kiocb *iocb,
> > > +                                            struct iov_iter *to)
> > > +{
> > > +       struct inode *inode = file_inode(iocb->ki_filp);
> > > +
> > > +       lockdep_assert_held(&inode->i_rwsem);
> > > +       if (unlikely(ext4_forced_shutdown(EXT4_SB(inode->i_sb))))
> > > +               return -EIO;
> > > +
> > > +       if (!iov_iter_count(to))
> > > +               return 0; /* skip atime */
> > > +
> > > +#ifdef CONFIG_FS_DAX
> > > +       if (IS_DAX(inode))
> > > +               return dax_iomap_rw(iocb, to, &ext4_iomap_ops);
> > > +#endif
> > > +       return generic_file_read_iter(iocb, to);
> > > +}
> > > +
> > >  /*
> > >   * Called when an inode is released. Note that this is different
> > >   * from ext4_file_open: open gets called at every open, but release
> > > @@ -747,6 +766,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_integrity_read_iter,
> > >  };
> > >
> > >  const struct inode_operations ext4_file_inode_operations = {
> > > diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> > > index 2706130c261b..82ea81da0b2d 100644
> > > --- a/fs/f2fs/file.c
> > > +++ b/fs/f2fs/file.c
> > > @@ -2514,4 +2514,5 @@ const struct file_operations f2fs_file_operations = {
> > >  #endif
> > >         .splice_read    = generic_file_splice_read,
> > >         .splice_write   = iter_file_splice_write,
> > > +       .integrity_read = generic_file_read_iter,
> > >  };
> > > diff --git a/fs/jffs2/file.c b/fs/jffs2/file.c
> > > index c12476e309c6..5a63034cccf5 100644
> > > --- a/fs/jffs2/file.c
> > > +++ b/fs/jffs2/file.c
> > > @@ -57,6 +57,7 @@ const struct file_operations jffs2_file_operations =
> > >         .mmap =         generic_file_readonly_mmap,
> > >         .fsync =        jffs2_fsync,
> > >         .splice_read =  generic_file_splice_read,
> > > +       .integrity_read = generic_file_read_iter,
> > >  };
> > >
> > >  /* jffs2_file_inode_operations */
> > > diff --git a/fs/jfs/file.c b/fs/jfs/file.c
> > > index 739492c7a3fd..423512a810e4 100644
> > > --- a/fs/jfs/file.c
> > > +++ b/fs/jfs/file.c
> > > @@ -162,4 +162,5 @@ const struct file_operations jfs_file_operations = {
> > >  #ifdef CONFIG_COMPAT
> > >         .compat_ioctl   = jfs_compat_ioctl,
> > >  #endif
> > > +       .integrity_read = generic_file_read_iter,
> > >  };
> > > diff --git a/fs/nilfs2/file.c b/fs/nilfs2/file.c
> > > index c5fa3dee72fc..55e058ac487f 100644
> > > --- a/fs/nilfs2/file.c
> > > +++ b/fs/nilfs2/file.c
> > > @@ -150,6 +150,7 @@ const struct file_operations nilfs_file_operations = {
> > >         /* .release     = nilfs_release_file, */
> > >         .fsync          = nilfs_sync_file,
> > >         .splice_read    = generic_file_splice_read,
> > > +       .integrity_read = generic_file_read_iter,
> > >  };
> > >
> > >  const struct inode_operations nilfs_file_inode_operations = {
> > > diff --git a/fs/ramfs/file-mmu.c b/fs/ramfs/file-mmu.c
> > > index 12af0490322f..4f24d1b589b1 100644
> > > --- a/fs/ramfs/file-mmu.c
> > > +++ b/fs/ramfs/file-mmu.c
> > > @@ -47,6 +47,7 @@ const struct file_operations ramfs_file_operations = {
> > >         .splice_write   = iter_file_splice_write,
> > >         .llseek         = generic_file_llseek,
> > >         .get_unmapped_area      = ramfs_mmu_get_unmapped_area,
> > > +       .integrity_read = generic_file_read_iter,
> > >  };
> > >
> > >  const struct inode_operations ramfs_file_inode_operations = {
> > > diff --git a/fs/ramfs/file-nommu.c b/fs/ramfs/file-nommu.c
> > > index 2ef7ce75c062..5ee704fa84e0 100644
> > > --- a/fs/ramfs/file-nommu.c
> > > +++ b/fs/ramfs/file-nommu.c
> > > @@ -50,6 +50,7 @@ const struct file_operations ramfs_file_operations = {
> > >         .splice_read            = generic_file_splice_read,
> > >         .splice_write           = iter_file_splice_write,
> > >         .llseek                 = generic_file_llseek,
> > > +       .integrity_read         = generic_file_read_iter,
> > >  };
> > >
> > >  const struct inode_operations ramfs_file_inode_operations = {
> > > diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
> > > index 8cad0b19b404..5e52a315e18b 100644
> > > --- a/fs/ubifs/file.c
> > > +++ b/fs/ubifs/file.c
> > > @@ -1747,4 +1747,5 @@ const struct file_operations ubifs_file_operations =
> > > {
> > >  #ifdef CONFIG_COMPAT
> > >         .compat_ioctl   = ubifs_compat_ioctl,
> > >  #endif
> > > +       .integrity_read = generic_file_read_iter,
> > >  };
> > > diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> > > index c4893e226fd8..0a6704b563d6 100644
> > > --- a/fs/xfs/xfs_file.c
> > > +++ b/fs/xfs/xfs_file.c
> > > @@ -292,6 +292,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.
> > >   *
> > > @@ -1175,6 +1195,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 e522d25d0836..f6a01d3efce5 100644
> > > --- a/include/linux/fs.h
> > > +++ b/include/linux/fs.h
> > > @@ -1699,6 +1699,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 *);
> > >  } __randomize_layout;
> > >
> > >  struct inode_operations {
> > > diff --git a/mm/shmem.c b/mm/shmem.c
> > > index b0aa6075d164..805d99011ca4 100644
> > > --- a/mm/shmem.c
> > > +++ b/mm/shmem.c
> > > @@ -3849,6 +3849,7 @@ static const struct file_operations
> > > shmem_file_operations = {
> > >         .splice_read    = generic_file_splice_read,
> > >         .splice_write   = iter_file_splice_write,
> > >         .fallocate      = shmem_fallocate,
> > > +       .integrity_read = shmem_file_read_iter,
> > >  #endif
> > >  };
> > >
> > > diff --git a/security/integrity/iint.c b/security/integrity/iint.c
> > > index c84e05866052..c3a07276f745 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,
> > >                           void *addr, unsigned long count)
> > >  {
> > > -       mm_segment_t old_fs;
> > > -       char __user *buf = (char __user *)addr;
> > > +       struct inode *inode = file_inode(file);
> > > +       struct kvec 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_kvec(&iter, READ | ITER_KVEC, &iov, 1, count);
> > >
> > > +       ret = file->f_op->integrity_read(&kiocb, &iter);
> > > +       BUG_ON(ret == -EIOCBQUEUED);
> > >         return ret;
> > >  }
> > >
> > > --
> > > 2.7.4
> > >
> > >
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

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

[Linus Torvalds]

On Fri, Sep 15, 2017 at 2:04 AM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> On Thu, 2017-09-14 at 22:50 -0700, Linus Torvalds wrote:
>> This is still wrong.
>>
>> (a) there is no explanation for why we need that exclusive lock in the
>> first place
>
>> Why should a read need exclusive access? You'd think shared is sufficient.
>
> True, reading a file shouldn't require an exclusive lock.  The
> exclusive lock is taken to prevent the file from changing while the
> file hash is being calculated.

That really shouldn't need an exclusive lock either. The whole point
is that you're just reading the file, so a shared lock should be fine.

There may be other *higher* level reasons why the caller then might
want an exclusive lock for other reasons, but that should have nothing
to do with the reading part.

So this is the thing I want explained. Right now there are no
explanations, and the few comments there are about exclusive locking
don't make sense, and don't match the lockdep tests.

So the patch itself may be fine, but the commentary and explanations
are broken and/or missing.

                     Linus

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

[Christoph Hellwig]

On Thu, Sep 14, 2017 at 10:50:27PM -0700, Linus Torvalds wrote:
> This is still wrong.
> 
> (a) there is no explanation for why we need that exclusive lock in the
> first place
> 
> Why should a read need exclusive access? You'd think shared is sufficient.
> But regardless, it needs *explanation*.

Shared is sufficient, and nothing in the patch (except for the
description) actually requires an exclusive lock.  It just happens that
ima holds it exclusive for other internal reasons.

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

[Mimi Zohar]

On Fri, 2017-09-15 at 07:49 -0700, Christoph Hellwig wrote:
> On Thu, Sep 14, 2017 at 10:50:27PM -0700, Linus Torvalds wrote:
> > This is still wrong.
> > 
> > (a) there is no explanation for why we need that exclusive lock in the
> > first place
> > 
> > Why should a read need exclusive access? You'd think shared is sufficient.
> > But regardless, it needs *explanation*.
> 
> Shared is sufficient, and nothing in the patch (except for the
> description) actually requires an exclusive lock.  It just happens that
> ima holds it exclusive for other internal reasons.

Although reading the file to calculate the file hash doesn't require
taking the lock exclusively, in either "fix" mode or called from
__fput, immediately after calculating the file hash, the file hash is
written out as an xattr.  Writing the xattr requires taking the lock
exclusively.

Mimi

Re: [PATCH 3/3] ima: use fs method to read integrity data (updated patch description)

[Mimi Zohar]

From: Christoph Hellwig <hch@lst.de>

Writing extended attributes requires exclusively taking the i_rwsem
lock.  To synchronize the file hash calculation and writing the file
hash as security.ima xattr, IMA-appraisal takes the i_rwsem lock
exclusively before calculating the file hash.  (Once the file hash
is calculated, the result is cached.  Taking the lock exclusively
prevents calculating the file hash multiple times.)

Some filesystems have recently replaced their filesystem dependent
lock with the global i_rwsem to read a file.  As a result, when IMA
attempts to calculate the file hash, reading the file attempts to
take the i_rwsem again.

To resolve this locking problem, this patch defines a new
->integrity_read file operation method, which is equivalent to
->read_iter, except that it will not take the i_rwsem lock, but will
be called with the i_rwsem held exclusively.

Since taking the i_rwsem exclusively is not required for reading the
file in order to calculate the file hash, the code only verifies
that the lock has been taken.

(Based on Christoph's original patch.)

Signed-off-by: Christoph Hellwig <hch@lst.de>
Cc: Matthew Garrett <mjg59@srcf.ucam.org>
Cc: Jan Kara <jack@suse.com>
Cc: "Theodore Ts'o" <tytso@mit.edu>
Cc: Andreas Dilger <adilger.kernel@dilger.ca>
Cc: Jaegeuk Kim <jaegeuk@kernel.org>
Cc: Chao Yu <yuchao0@huawei.com>
Cc: Steven Whitehouse <swhiteho@redhat.com>
Cc: Bob Peterson <rpeterso@redhat.com>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Dave Kleikamp <shaggy@kernel.org>
Cc: Ryusuke Konishi <konishi.ryusuke@lab.ntt.co.jp>
Cc: Mark Fasheh <mfasheh@versity.com>
Cc: Joel Becker <jlbec@evilplan.org>
Cc: Richard Weinberger <richard@nod.at>
Cc: "Darrick J. Wong" <darrick.wong@oracle.com>
Cc: Hugh Dickins <hughd@google.com>
Cc: Chris Mason <clm@fb.com>

Fixes: Commit 6552321831dc "xfs: remove i_iolock and use i_rwsem in
the VFS inode instead"
Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
Reviewed-by: Jan Kara <jack@suse.cz>
Reviewed-by: Dmitry Kasatkin <dmitry.kasatkin@huawei.com>

---
Changelog:

- Updated patch description

---
 fs/btrfs/file.c           |  1 +
 fs/efivarfs/file.c        |  1 +
 fs/ext2/file.c            | 17 +++++++++++++++++
 fs/ext4/file.c            | 20 ++++++++++++++++++++
 fs/f2fs/file.c            |  1 +
 fs/jffs2/file.c           |  1 +
 fs/jfs/file.c             |  1 +
 fs/nilfs2/file.c          |  1 +
 fs/ramfs/file-mmu.c       |  1 +
 fs/ramfs/file-nommu.c     |  1 +
 fs/ubifs/file.c           |  1 +
 fs/xfs/xfs_file.c         | 21 +++++++++++++++++++++
 include/linux/fs.h        |  1 +
 mm/shmem.c                |  1 +
 security/integrity/iint.c | 20 ++++++++++++++------
 15 files changed, 83 insertions(+), 6 deletions(-)

diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index 9e75d8a39aac..2542dc66c85c 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -3125,6 +3125,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/efivarfs/file.c b/fs/efivarfs/file.c
index 863f1b100165..17955a92a5b3 100644
--- a/fs/efivarfs/file.c
+++ b/fs/efivarfs/file.c
@@ -179,4 +179,5 @@ const struct file_operations efivarfs_file_operations = {
 	.write	= efivarfs_file_write,
 	.llseek	= no_llseek,
 	.unlocked_ioctl = efivarfs_file_ioctl,
+	.integrity_read	= efivarfs_file_read_iter,
 };
diff --git a/fs/ext2/file.c b/fs/ext2/file.c
index d34d32bdc944..111069de1973 100644
--- a/fs/ext2/file.c
+++ b/fs/ext2/file.c
@@ -192,6 +192,22 @@ static ssize_t ext2_file_read_iter(struct kiocb *iocb, struct iov_iter *to)
 	return generic_file_read_iter(iocb, to);
 }
 
+static ssize_t ext2_file_integrity_read_iter(struct kiocb *iocb,
+					     struct iov_iter *to)
+{
+	struct inode *inode = file_inode(iocb->ki_filp);
+
+	lockdep_assert_held(&inode->i_rwsem);
+#ifdef CONFIG_FS_DAX
+	if (!iov_iter_count(to))
+		return 0; /* skip atime */
+
+	if (IS_DAX(iocb->ki_filp->f_mapping->host))
+		return dax_iomap_rw(iocb, to, &ext2_iomap_ops);
+#endif
+	return generic_file_read_iter(iocb, to);
+}
+
 static ssize_t ext2_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
 {
 #ifdef CONFIG_FS_DAX
@@ -216,6 +232,7 @@ const struct file_operations ext2_file_operations = {
 	.get_unmapped_area = thp_get_unmapped_area,
 	.splice_read	= generic_file_splice_read,
 	.splice_write	= iter_file_splice_write,
+	.integrity_read	= ext2_file_integrity_read_iter,
 };
 
 const struct inode_operations ext2_file_inode_operations = {
diff --git a/fs/ext4/file.c b/fs/ext4/file.c
index 58294c9a7e1d..3ab4105c8578 100644
--- a/fs/ext4/file.c
+++ b/fs/ext4/file.c
@@ -74,6 +74,25 @@ static ssize_t ext4_file_read_iter(struct kiocb *iocb, struct iov_iter *to)
 	return generic_file_read_iter(iocb, to);
 }
 
+static ssize_t ext4_file_integrity_read_iter(struct kiocb *iocb,
+					     struct iov_iter *to)
+{
+	struct inode *inode = file_inode(iocb->ki_filp);
+
+	lockdep_assert_held(&inode->i_rwsem);
+	if (unlikely(ext4_forced_shutdown(EXT4_SB(inode->i_sb))))
+		return -EIO;
+
+	if (!iov_iter_count(to))
+		return 0; /* skip atime */
+
+#ifdef CONFIG_FS_DAX
+	if (IS_DAX(inode))
+		return dax_iomap_rw(iocb, to, &ext4_iomap_ops);
+#endif
+	return generic_file_read_iter(iocb, to);
+}
+
 /*
  * Called when an inode is released. Note that this is different
  * from ext4_file_open: open gets called at every open, but release
@@ -747,6 +766,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_integrity_read_iter,
 };
 
 const struct inode_operations ext4_file_inode_operations = {
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 2706130c261b..82ea81da0b2d 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -2514,4 +2514,5 @@ const struct file_operations f2fs_file_operations = {
 #endif
 	.splice_read	= generic_file_splice_read,
 	.splice_write	= iter_file_splice_write,
+	.integrity_read	= generic_file_read_iter,
 };
diff --git a/fs/jffs2/file.c b/fs/jffs2/file.c
index c12476e309c6..5a63034cccf5 100644
--- a/fs/jffs2/file.c
+++ b/fs/jffs2/file.c
@@ -57,6 +57,7 @@ const struct file_operations jffs2_file_operations =
 	.mmap =		generic_file_readonly_mmap,
 	.fsync =	jffs2_fsync,
 	.splice_read =	generic_file_splice_read,
+	.integrity_read = generic_file_read_iter,
 };
 
 /* jffs2_file_inode_operations */
diff --git a/fs/jfs/file.c b/fs/jfs/file.c
index 739492c7a3fd..423512a810e4 100644
--- a/fs/jfs/file.c
+++ b/fs/jfs/file.c
@@ -162,4 +162,5 @@ const struct file_operations jfs_file_operations = {
 #ifdef CONFIG_COMPAT
 	.compat_ioctl	= jfs_compat_ioctl,
 #endif
+	.integrity_read	= generic_file_read_iter,
 };
diff --git a/fs/nilfs2/file.c b/fs/nilfs2/file.c
index c5fa3dee72fc..55e058ac487f 100644
--- a/fs/nilfs2/file.c
+++ b/fs/nilfs2/file.c
@@ -150,6 +150,7 @@ const struct file_operations nilfs_file_operations = {
 	/* .release	= nilfs_release_file, */
 	.fsync		= nilfs_sync_file,
 	.splice_read	= generic_file_splice_read,
+	.integrity_read	= generic_file_read_iter,
 };
 
 const struct inode_operations nilfs_file_inode_operations = {
diff --git a/fs/ramfs/file-mmu.c b/fs/ramfs/file-mmu.c
index 12af0490322f..4f24d1b589b1 100644
--- a/fs/ramfs/file-mmu.c
+++ b/fs/ramfs/file-mmu.c
@@ -47,6 +47,7 @@ const struct file_operations ramfs_file_operations = {
 	.splice_write	= iter_file_splice_write,
 	.llseek		= generic_file_llseek,
 	.get_unmapped_area	= ramfs_mmu_get_unmapped_area,
+	.integrity_read	= generic_file_read_iter,
 };
 
 const struct inode_operations ramfs_file_inode_operations = {
diff --git a/fs/ramfs/file-nommu.c b/fs/ramfs/file-nommu.c
index 2ef7ce75c062..5ee704fa84e0 100644
--- a/fs/ramfs/file-nommu.c
+++ b/fs/ramfs/file-nommu.c
@@ -50,6 +50,7 @@ const struct file_operations ramfs_file_operations = {
 	.splice_read		= generic_file_splice_read,
 	.splice_write		= iter_file_splice_write,
 	.llseek			= generic_file_llseek,
+	.integrity_read		= generic_file_read_iter,
 };
 
 const struct inode_operations ramfs_file_inode_operations = {
diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
index 8cad0b19b404..5e52a315e18b 100644
--- a/fs/ubifs/file.c
+++ b/fs/ubifs/file.c
@@ -1747,4 +1747,5 @@ const struct file_operations ubifs_file_operations = {
 #ifdef CONFIG_COMPAT
 	.compat_ioctl   = ubifs_compat_ioctl,
 #endif
+	.integrity_read = generic_file_read_iter,
 };
diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index c4893e226fd8..0a6704b563d6 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -292,6 +292,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.
  *
@@ -1175,6 +1195,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 e522d25d0836..f6a01d3efce5 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1699,6 +1699,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 *);
 } __randomize_layout;
 
 struct inode_operations {
diff --git a/mm/shmem.c b/mm/shmem.c
index b0aa6075d164..805d99011ca4 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -3849,6 +3849,7 @@ static const struct file_operations shmem_file_operations = {
 	.splice_read	= generic_file_splice_read,
 	.splice_write	= iter_file_splice_write,
 	.fallocate	= shmem_fallocate,
+	.integrity_read	= shmem_file_read_iter,
 #endif
 };
 
diff --git a/security/integrity/iint.c b/security/integrity/iint.c
index c84e05866052..c3a07276f745 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,
 			  void *addr, unsigned long count)
 {
-	mm_segment_t old_fs;
-	char __user *buf = (char __user *)addr;
+	struct inode *inode = file_inode(file);
+	struct kvec 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_kvec(&iter, READ | ITER_KVEC, &iov, 1, count);
 
+	ret = file->f_op->integrity_read(&kiocb, &iter);
+	BUG_ON(ret == -EIOCBQUEUED);
 	return ret;
 }
 
-- 
2.7.4

Re: [PATCH 3/3] ima: use fs method to read integrity data (updated patch description)

[Linus Torvalds]

On Fri, Sep 15, 2017 at 1:25 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
>
> To resolve this locking problem, this patch defines a new
> ->integrity_read file operation method, which is equivalent to
> ->read_iter, except that it will not take the i_rwsem lock, but will
> be called with the i_rwsem held exclusively.
>
> Since taking the i_rwsem exclusively is not required for reading the
> file in order to calculate the file hash, the code only verifies
> that the lock has been taken.

Ok, so I'm onboard with the commit message now, but realized that I'm
not actually convinced that i_rwsem is even meaningful.

Sure, generic_file_write_iter() does take that lock exclusively, but
not everybody uses generic_file_write_iter() at all for writing.

For example, xfs still uses that i_rwsem, but for block-aligned writes
it will only get it shared. And I'm not convinced some other
filesystem might not end up using some other lock entirely.

So I'm basically not entirely convinced that these i_rwsem games make
any sense at all.

The filesystem can do its own locking, and I'm starting to think that
it would be better to just pass this "this is an integrity read" down
to the filesystem, and expect the filesystem to do the locking based
on that.

                 Linus

Re: [PATCH 3/3] ima: use fs method to read integrity data (updated patch description)

[Mimi Zohar]

On Sat, 2017-09-16 at 11:20 -0700, Linus Torvalds wrote:
> On Fri, Sep 15, 2017 at 1:25 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> >
> > To resolve this locking problem, this patch defines a new
> > ->integrity_read file operation method, which is equivalent to
> > ->read_iter, except that it will not take the i_rwsem lock, but will
> > be called with the i_rwsem held exclusively.
> >
> > Since taking the i_rwsem exclusively is not required for reading the
> > file in order to calculate the file hash, the code only verifies
> > that the lock has been taken.
> 
> Ok, so I'm onboard with the commit message now, but realized that I'm
> not actually convinced that i_rwsem is even meaningful.
> 
> Sure, generic_file_write_iter() does take that lock exclusively, but
> not everybody uses generic_file_write_iter() at all for writing.

> For example, xfs still uses that i_rwsem, but for block-aligned writes
> it will only get it shared. And I'm not convinced some other
> filesystem might not end up using some other lock entirely.
> 
> So I'm basically not entirely convinced that these i_rwsem games make
> any sense at all.
> 
> The filesystem can do its own locking, and I'm starting to think that
> it would be better to just pass this "this is an integrity read" down
> to the filesystem, and expect the filesystem to do the locking based
> on that.

IMA would still need to take the i_rwsem to write the xattr.  Unless
the i_rwsem was taken before calling the integrity_read, calculating
the file hash would be serialized, but would not prevent the file hash
from being calculated multiple times.

(Introducing a new lock would result in the locks being taken in
reverse order for setxattr, chown, chmod syscalls.)

Mimi

Re: [PATCH 3/3] ima: use fs method to read integrity data (updated patch description)

[Christoph Hellwig]

On Sat, Sep 16, 2017 at 11:20:47AM -0700, Linus Torvalds wrote:
> Sure, generic_file_write_iter() does take that lock exclusively, but
> not everybody uses generic_file_write_iter() at all for writing.
> 
> For example, xfs still uses that i_rwsem, but for block-aligned writes
> it will only get it shared. And I'm not convinced some other
> filesystem might not end up using some other lock entirely.

Only for direct I/O, and IMA and direct I/O don't work together.
>From ima_collect_measurement:

		if (file->f_flags & O_DIRECT) {
			audit_cause = "failed(directio)";
			result = -EACCES;
			goto out;
		}

(and yes, it should be checking for IOCB_DIRECT to avoid racy
f_flags manipulations, but that's another issue)

> The filesystem can do its own locking, and I'm starting to think that
> it would be better to just pass this "this is an integrity read" down
> to the filesystem, and expect the filesystem to do the locking based
> on that.

Well, that's exactly the point of the new ->integrity_read routine
I proposed and prototype.  The important thing is that it is called
with i_rwsem held because code mugh higher in the chain already
acquired it, but except for that it's entirely up to the file system.

Re: [PATCH 3/3] ima: use fs method to read integrity data (updated patch description)

[Linus Torvalds]

On Sun, Sep 17, 2017 at 8:17 AM, Christoph Hellwig <hch@infradead.org> wrote:
>
> Only for direct I/O, and IMA and direct I/O don't work together.
> From ima_collect_measurement:
>
>                 if (file->f_flags & O_DIRECT) {
>                         audit_cause = "failed(directio)";
>                         result = -EACCES;
>                         goto out;
>                 }

That's not the issue.

The issue is that somebody else can come in - using direct IO - at the
same time as the first person is collecting measurements, and thus
race with the collector.

So now the measurements are not trustworthy any more.

> Well, that's exactly the point of the new ->integrity_read routine
> I proposed and prototype.  The important thing is that it is called
> with i_rwsem held because code mugh higher in the chain already
> acquired it, but except for that it's entirely up to the file system.

.. and *my* point is that it's the wrong lock for actually checking
integrity (it doesn't actually guarantee exclusion, even though in
practice it's almost always the case), and so we're adding a nasty
callback that in 99% of all cases is the same as the normal read, and
we *could* have just added it with a RWF flag instead.

Is there some reason why integrity has to use that particular lock
that is so inconvenient for the filesystems it wants to check?

                 Linus

Re: [PATCH 3/3] ima: use fs method to read integrity data (updated patch description)

[Christoph Hellwig]

On Sun, Sep 17, 2017 at 08:28:40AM -0700, Linus Torvalds wrote:
> The issue is that somebody else can come in - using direct IO - at the
> same time as the first person is collecting measurements, and thus
> race with the collector.
>
> So now the measurements are not trustworthy any more.

Yes.  And it's always been that way with IMA.

> .. and *my* point is that it's the wrong lock for actually checking
> integrity (it doesn't actually guarantee exclusion, even though in
> practice it's almost always the case), and so we're adding a nasty
> callback that in 99% of all cases is the same as the normal read, and
> we *could* have just added it with a RWF flag instead.
> 
> Is there some reason why integrity has to use that particular lock
> that is so inconvenient for the filesystems it wants to check?

I'll have to defer that to Mimi - I just jumped into this whole mess
to help fixing the deadlocks we saw on XFS and NFS.

Unfortunately the whole security code is a giant mess that doesn't
document assumptions, threat models or gets any sort of verification
of those through automated testing.

Re: [PATCH 3/3] ima: use fs method to read integrity data (updated patch description)

[Mimi Zohar]

On Sun, 2017-09-17 at 08:28 -0700, Linus Torvalds wrote:
> On Sun, Sep 17, 2017 at 8:17 AM, Christoph Hellwig <hch@infradead.org> wrote:
> >
> > Only for direct I/O, and IMA and direct I/O don't work together.
> > From ima_collect_measurement:
> >
> >                 if (file->f_flags & O_DIRECT) {
> >                         audit_cause = "failed(directio)";
> >                         result = -EACCES;
> >                         goto out;
> >                 }
> 
> That's not the issue.
> 
> The issue is that somebody else can come in - using direct IO - at the
> same time as the first person is collecting measurements, and thus
> race with the collector.
> 
> So now the measurements are not trustworthy any more.

Unless I'm missing something, that would only be possible with an IMA
policy rule that permits direct IO (eg. permit_directio).  Otherwise
the direct IO is denied.

> > Well, that's exactly the point of the new ->integrity_read routine
> > I proposed and prototype.  The important thing is that it is called
> > with i_rwsem held because code mugh higher in the chain already
> > acquired it, but except for that it's entirely up to the file system.
> 
> .. and *my* point is that it's the wrong lock for actually checking
> integrity (it doesn't actually guarantee exclusion, even though in
> practice it's almost always the case), and so we're adding a nasty
> callback that in 99% of all cases is the same as the normal read, and
> we *could* have just added it with a RWF flag instead.
> 
> Is there some reason why integrity has to use that particular lock
> that is so inconvenient for the filesystems it wants to check?

Originally IMA had its own lock (iint->mutex), prior to IMA-appraisal
being upstreamed.  With a separate lock, the iint->mutex and i_rwsem
would be taken in reverse order in process_measurements() and in the
setxattr, chown, chmod syscalls.

I'm at the airport on my way back home.

Mimi

Re: [PATCH 3/3] ima: use fs method to read integrity data (updated patch description)

[Linus Torvalds]

On Sun, Sep 17, 2017 at 9:15 AM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
>
> Unless I'm missing something, that would only be possible with an IMA
> policy rule that permits direct IO (eg. permit_directio).  Otherwise
> the direct IO is denied.

Note that the "XFS and directio" was only an example.

There is absolutely nothing that says that a  filesystem has to use
i_rwsem for IO serialization at all. Even for the regular write path.

Now, I suspect most (all?) do, but that's a historical artifact rather
than "design". In particular, the VFS layer used to do the locking for
the filesystems, to guarantee the POSIX requirements (POSIX requires
that writes be seen atomically).

But that lock was pushed down into the filesystems, since some
filesystems really wanted to have parallel writes (particularly for
direct IO, where that POSIX serialization requirement doesn't exist).

That's all many years ago, though. New filesystems are likely to have
copied the pattern from old ones, but even then..

Also, it's worth noting that "inode->i_rwlock" isn't even well-defined
as a lock. You can have the question of *which* inode gets talked
about when you have things like eoverlayfs etc. Normally it would be
obvious, but sometimes you'd use "file->f_mapping->host" (which is the
same thing in the simple cases), and sometimes it really wouldn't be
obvious at all..

So... I'm really not at all convinced that i_rwsem is sensible. It's
one of those things that are "mostly right for the simple cases",
but...

              Linus

Re: [PATCH 3/3] ima: use fs method to read integrity data (updated patch description)

[Al Viro]

On Sun, Sep 17, 2017 at 09:34:01AM -0700, Linus Torvalds wrote:
> Now, I suspect most (all?) do, but that's a historical artifact rather
> than "design". In particular, the VFS layer used to do the locking for
> the filesystems, to guarantee the POSIX requirements (POSIX requires
> that writes be seen atomically).
> 
> But that lock was pushed down into the filesystems, since some
> filesystems really wanted to have parallel writes (particularly for
> direct IO, where that POSIX serialization requirement doesn't exist).
> 
> That's all many years ago, though. New filesystems are likely to have
> copied the pattern from old ones, but even then..
> 
> Also, it's worth noting that "inode->i_rwlock" isn't even well-defined
> as a lock. You can have the question of *which* inode gets talked
> about when you have things like eoverlayfs etc. Normally it would be
> obvious, but sometimes you'd use "file->f_mapping->host" (which is the
> same thing in the simple cases), and sometimes it really wouldn't be
> obvious at all..
> 
> So... I'm really not at all convinced that i_rwsem is sensible. It's
> one of those things that are "mostly right for the simple cases",
> but...

The thing pretty much common to all of them is that write() might need
to modify permissions (suid removal), which brings ->i_rwsem in one
way or another - notify_change() needs that held...

Re: [PATCH 3/3] ima: use fs method to read integrity data (updated patch description)

[Steven Whitehouse]

Hi,


On 17/09/17 17:38, Al Viro wrote:
> On Sun, Sep 17, 2017 at 09:34:01AM -0700, Linus Torvalds wrote:
>> Now, I suspect most (all?) do, but that's a historical artifact rather
>> than "design". In particular, the VFS layer used to do the locking for
>> the filesystems, to guarantee the POSIX requirements (POSIX requires
>> that writes be seen atomically).
>>
>> But that lock was pushed down into the filesystems, since some
>> filesystems really wanted to have parallel writes (particularly for
>> direct IO, where that POSIX serialization requirement doesn't exist).
>>
>> That's all many years ago, though. New filesystems are likely to have
>> copied the pattern from old ones, but even then..
>>
>> Also, it's worth noting that "inode->i_rwlock" isn't even well-defined
>> as a lock. You can have the question of *which* inode gets talked
>> about when you have things like eoverlayfs etc. Normally it would be
>> obvious, but sometimes you'd use "file->f_mapping->host" (which is the
>> same thing in the simple cases), and sometimes it really wouldn't be
>> obvious at all..
>>
>> So... I'm really not at all convinced that i_rwsem is sensible. It's
>> one of those things that are "mostly right for the simple cases",
>> but...
> The thing pretty much common to all of them is that write() might need
> to modify permissions (suid removal), which brings ->i_rwsem in one
> way or another - notify_change() needs that held...

For GFS2, if we are to hold the inode info constant while it is checked, 
we would need to take a glock (read lock in this case) across the 
relevant operations. The glock will be happy under i_rwlock, since we 
have a lock ordering that takes local locks ahead of cluster locks. I've 
not dug into this enough to figure out whether the current proposal will 
allow this to work with GFS2 though. Does IMA cache the results from the 
->read_integrity() operation?

Steve.

Re: [PATCH 3/3] ima: use fs method to read integrity data (updated patch description)

[Jan Kara]

On Mon 18-09-17 10:19:25, Steven Whitehouse wrote:
> On 17/09/17 17:38, Al Viro wrote:
> >On Sun, Sep 17, 2017 at 09:34:01AM -0700, Linus Torvalds wrote:
> >>Now, I suspect most (all?) do, but that's a historical artifact rather
> >>than "design". In particular, the VFS layer used to do the locking for
> >>the filesystems, to guarantee the POSIX requirements (POSIX requires
> >>that writes be seen atomically).
> >>
> >>But that lock was pushed down into the filesystems, since some
> >>filesystems really wanted to have parallel writes (particularly for
> >>direct IO, where that POSIX serialization requirement doesn't exist).
> >>
> >>That's all many years ago, though. New filesystems are likely to have
> >>copied the pattern from old ones, but even then..
> >>
> >>Also, it's worth noting that "inode->i_rwlock" isn't even well-defined
> >>as a lock. You can have the question of *which* inode gets talked
> >>about when you have things like eoverlayfs etc. Normally it would be
> >>obvious, but sometimes you'd use "file->f_mapping->host" (which is the
> >>same thing in the simple cases), and sometimes it really wouldn't be
> >>obvious at all..
> >>
> >>So... I'm really not at all convinced that i_rwsem is sensible. It's
> >>one of those things that are "mostly right for the simple cases",
> >>but...
> >The thing pretty much common to all of them is that write() might need
> >to modify permissions (suid removal), which brings ->i_rwsem in one
> >way or another - notify_change() needs that held...
> 
> For GFS2, if we are to hold the inode info constant while it is checked, we
> would need to take a glock (read lock in this case) across the relevant
> operations. The glock will be happy under i_rwlock, since we have a lock
> ordering that takes local locks ahead of cluster locks. I've not dug into
> this enough to figure out whether the current proposal will allow this to
> work with GFS2 though. Does IMA cache the results from the
> ->read_integrity() operation?

So I have asked Mimi about clustered filesystems before. And for now the
answer was that IMA for clustered filesystems is not supported (it will
return some error since ->integrity_read is NULL). If we would ever want to
support those it would require larger overhaul of the IMA architecture to
give filesystem more control over the locking (which is essentially what
Linus wants).

								Honza

-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 3/3] ima: use fs method to read integrity data (updated patch description)

[Mimi Zohar]

On Mon, 2017-09-18 at 12:13 +0200, Jan Kara wrote:
> On Mon 18-09-17 10:19:25, Steven Whitehouse wrote:
> > On 17/09/17 17:38, Al Viro wrote:
> > >On Sun, Sep 17, 2017 at 09:34:01AM -0700, Linus Torvalds wrote:
> > >>Now, I suspect most (all?) do, but that's a historical artifact rather
> > >>than "design". In particular, the VFS layer used to do the locking for
> > >>the filesystems, to guarantee the POSIX requirements (POSIX requires
> > >>that writes be seen atomically).
> > >>
> > >>But that lock was pushed down into the filesystems, since some
> > >>filesystems really wanted to have parallel writes (particularly for
> > >>direct IO, where that POSIX serialization requirement doesn't exist).
> > >>
> > >>That's all many years ago, though. New filesystems are likely to have
> > >>copied the pattern from old ones, but even then..
> > >>
> > >>Also, it's worth noting that "inode->i_rwlock" isn't even well-defined
> > >>as a lock. You can have the question of *which* inode gets talked
> > >>about when you have things like eoverlayfs etc. Normally it would be
> > >>obvious, but sometimes you'd use "file->f_mapping->host" (which is the
> > >>same thing in the simple cases), and sometimes it really wouldn't be
> > >>obvious at all..
> > >>
> > >>So... I'm really not at all convinced that i_rwsem is sensible. It's
> > >>one of those things that are "mostly right for the simple cases",
> > >>but...
> > >The thing pretty much common to all of them is that write() might need
> > >to modify permissions (suid removal), which brings ->i_rwsem in one
> > >way or another - notify_change() needs that held...
> > 
> > For GFS2, if we are to hold the inode info constant while it is checked, we
> > would need to take a glock (read lock in this case) across the relevant
> > operations. The glock will be happy under i_rwlock, since we have a lock
> > ordering that takes local locks ahead of cluster locks. I've not dug into
> > this enough to figure out whether the current proposal will allow this to
> > work with GFS2 though. Does IMA cache the results from the
> > ->read_integrity() operation?

Up to now, the hash calculation was stored in the iint structure,
which is then used to extend the TPM, verify the file's integrity
compared to the value stored in the xattr, and included in an audit
message.

A new patch set by Thiago Bauermann will add appended signature
support, re-using the kernel module signature appended method, which
might require re-calculating the file hash based on a different hash
algorithm.

> So I have asked Mimi about clustered filesystems before. And for now the
> answer was that IMA for clustered filesystems is not supported (it will
> return some error since ->integrity_read is NULL). If we would ever want to
> support those it would require larger overhaul of the IMA architecture to
> give filesystem more control over the locking (which is essentially what
> Linus wants).

For performance reasons, IMA is not on a write hook, but detects file
change on the last __fput() opened for write.  At that point, the
cached info is reset.  The file hash is re-calculated and written out
as an xattr.  On the next file access (in policy), the file hash is
re-calculated and stored in the iint.

In terms of remote/clustered/fuse filesystems, we wouldn't be on the
__fput() path.  Support for remote/clustered/fuse filesystems, would
be similar to filesystems that do not support i_version.  Meaning only
the first file access (in policy) would be measured/appraised, but not
subsequent ones.  Even if we could detect file change, we would be
dependent on the remote/clustered/fuse filesystem to inform us of the
change.  What type of integrity guarantees would that provide?

Mimi

Re: [PATCH 3/3] ima: use fs method to read integrity data (updated patch description)

[Mimi Zohar]

On Mon, 2017-09-18 at 10:55 -0400, Mimi Zohar wrote:
> On Mon, 2017-09-18 at 12:13 +0200, Jan Kara wrote:
> > On Mon 18-09-17 10:19:25, Steven Whitehouse wrote:
> > > On 17/09/17 17:38, Al Viro wrote:
> > > >On Sun, Sep 17, 2017 at 09:34:01AM -0700, Linus Torvalds wrote:
> > > >>Now, I suspect most (all?) do, but that's a historical artifact rather
> > > >>than "design". In particular, the VFS layer used to do the locking for
> > > >>the filesystems, to guarantee the POSIX requirements (POSIX requires
> > > >>that writes be seen atomically).
> > > >>
> > > >>But that lock was pushed down into the filesystems, since some
> > > >>filesystems really wanted to have parallel writes (particularly for
> > > >>direct IO, where that POSIX serialization requirement doesn't exist).
> > > >>
> > > >>That's all many years ago, though. New filesystems are likely to have
> > > >>copied the pattern from old ones, but even then..
> > > >>
> > > >>Also, it's worth noting that "inode->i_rwlock" isn't even well-defined
> > > >>as a lock. You can have the question of *which* inode gets talked
> > > >>about when you have things like eoverlayfs etc. Normally it would be
> > > >>obvious, but sometimes you'd use "file->f_mapping->host" (which is the
> > > >>same thing in the simple cases), and sometimes it really wouldn't be
> > > >>obvious at all..
> > > >>
> > > >>So... I'm really not at all convinced that i_rwsem is sensible. It's
> > > >>one of those things that are "mostly right for the simple cases",
> > > >>but...
> > > >The thing pretty much common to all of them is that write() might need
> > > >to modify permissions (suid removal), which brings ->i_rwsem in one
> > > >way or another - notify_change() needs that held...


> > > For GFS2, if we are to hold the inode info constant while it is checked, we
> > > would need to take a glock (read lock in this case) across the relevant
> > > operations. The glock will be happy under i_rwlock, since we have a lock
> > > ordering that takes local locks ahead of cluster locks. I've not dug into
> > > this enough to figure out whether the current proposal will allow this to
> > > work with GFS2 though. Does IMA cache the results from the
> > > ->read_integrity() operation?
> 
> Up to now, the hash calculation was stored in the iint structure,
> which is then used to extend the TPM, verify the file's integrity
> compared to the value stored in the xattr, and included in an audit
> message.
> 
> A new patch set by Thiago Bauermann will add appended signature
> support, re-using the kernel module signature appended method, which
> might require re-calculating the file hash based on a different hash
> algorithm.
> 
> > So I have asked Mimi about clustered filesystems before. And for now the
> > answer was that IMA for clustered filesystems is not supported (it will
> > return some error since ->integrity_read is NULL). If we would ever want to
> > support those it would require larger overhaul of the IMA architecture to
> > give filesystem more control over the locking (which is essentially what
> > Linus wants).
> 
> For performance reasons, IMA is not on a write hook, but detects file
> change on the last __fput() opened for write.  At that point, the
> cached info is reset.  The file hash is re-calculated and written out
> as an xattr.  On the next file access (in policy), the file hash is
> re-calculated and stored in the iint.
> 
> In terms of remote/clustered/fuse filesystems, we wouldn't be on the
> __fput() path.  Support for remote/clustered/fuse filesystems, would
> be similar to filesystems that do not support i_version.  Meaning only
> the first file access (in policy) would be measured/appraised, but not
> subsequent ones.  Even if we could detect file change, we would be
> dependent on the remote/clustered/fuse filesystem to inform us of the
> change.  What type of integrity guarantees would that provide?

After thinking this over a bit, perhaps we shouldn't cache the file
integrity results for these filesystems, since we can't rely on them
to notify us of a change (eg. malicious fs), but simply re-measure/re-
validate files each time.

Mimi