[PATCH] ima: fix wrong dereferences of file->f_path

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



When storing IMA xattr on an overlayfs inode, the xattr is actually
stored in the inode of the underlying (a.k.a real) filesystem, so there
is an ambiguity whether this IMA xattr describes the integrity of the
overlayfs inode or the real inode.

For this reason and other reasons, IMA is not supported on overlayfs,
in the sense that integrity checking on the overlayfs inode/file/path
do not work correctly and have undefined behavior and the IMA xattr
always describes the integrity of the real inode.

When a user operates on an overlayfs file, whose underlying real file
has IMA enabled, IMA should always operate on the real path and not
on the overlayfs path.

IMA code already uses the helper file_dentry() to get the dentry
of the real file. Dereferencing file->f_path directly means that IMA
will operate on the overlayfs inode, which is wrong.

Therefore, all dereferences to f_path were converted to use the
file_real_path() helper.

Reported-by: syzbot+a67fc5321ffb4b311c98@xxxxxxxxxxxxxxxxxxxxxxxxx
Closes: https://lore.kernel.org/linux-unionfs/0000000000005bd097060530b758@xxxxxxxxxx/
Fixes: db1d1e8b9867 ("IMA: use vfs_getattr_nosec to get the i_version")
Cc: Christian Brauner <brauner@xxxxxxxxxx>
Cc: Jeff Layton <jlayton@xxxxxxxxxx>
Cc: Goldwyn Rodrigues <rgoldwyn@xxxxxxxx>
Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
---

Mimi,

Some of the wrong f_path dereferences are much older than the Fixes
commit, but they did not have as big an impact as the wrong f_path
dereference that the Fixes commit introduced.

For example, commit a408e4a86b36 ("ima: open a new file instance if no
read permissions") worked because reading the content of the overlayfs
file has the same result as reading the content of the real file, but it
is actually the real file integrity that we want to verify.

Anyway, the real path information, that is now available via the
file_real_path() helper, was not available in IMA itegrity check context
at the time that commit a408e4a86b36 was merged.

Thanks,
Amir.

 security/integrity/ima/ima_api.c    |  4 ++--
 security/integrity/ima/ima_crypto.c |  2 +-
 security/integrity/ima/ima_main.c   | 10 +++++-----
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index 452e80b541e5..badf3784a1a0 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -268,8 +268,8 @@ int ima_collect_measurement(struct integrity_iint_cache *iint,
 	 * to an initial measurement/appraisal/audit, but was modified to
 	 * assume the file changed.
 	 */
-	result = vfs_getattr_nosec(&file->f_path, &stat, STATX_CHANGE_COOKIE,
-				   AT_STATX_SYNC_AS_STAT);
+	result = vfs_getattr_nosec(file_real_path(file), &stat,
+				   STATX_CHANGE_COOKIE, AT_STATX_SYNC_AS_STAT);
 	if (!result && (stat.result_mask & STATX_CHANGE_COOKIE))
 		i_version = stat.change_cookie;
 	hash.hdr.algo = algo;
diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
index 51ad29940f05..e6c52f3c8f37 100644
--- a/security/integrity/ima/ima_crypto.c
+++ b/security/integrity/ima/ima_crypto.c
@@ -555,7 +555,7 @@ int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash)
 		int flags = file->f_flags & ~(O_WRONLY | O_APPEND |
 				O_TRUNC | O_CREAT | O_NOCTTY | O_EXCL);
 		flags |= O_RDONLY;
-		f = dentry_open(&file->f_path, flags, file->f_cred);
+		f = dentry_open(file_real_path(file), flags, file->f_cred);
 		if (IS_ERR(f))
 			return PTR_ERR(f);
 
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 365db0e43d7c..87c13effbdf4 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -94,7 +94,7 @@ static int mmap_violation_check(enum ima_hooks func, struct file *file,
 		inode = file_inode(file);
 
 		if (!*pathbuf)	/* ima_rdwr_violation possibly pre-fetched */
-			*pathname = ima_d_path(&file->f_path, pathbuf,
+			*pathname = ima_d_path(file_real_path(file), pathbuf,
 					       filename);
 		integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, *pathname,
 				    "mmap_file", "mmapped_writers", rc, 0);
@@ -142,7 +142,7 @@ static void ima_rdwr_violation_check(struct file *file,
 	if (!send_tomtou && !send_writers)
 		return;
 
-	*pathname = ima_d_path(&file->f_path, pathbuf, filename);
+	*pathname = ima_d_path(file_real_path(file), pathbuf, filename);
 
 	if (send_tomtou)
 		ima_add_violation(file, *pathname, iint,
@@ -168,7 +168,7 @@ static void ima_check_last_writer(struct integrity_iint_cache *iint,
 		update = test_and_clear_bit(IMA_UPDATE_XATTR,
 					    &iint->atomic_flags);
 		if ((iint->flags & IMA_NEW_FILE) ||
-		    vfs_getattr_nosec(&file->f_path, &stat,
+		    vfs_getattr_nosec(file_real_path(file), &stat,
 				      STATX_CHANGE_COOKIE,
 				      AT_STATX_SYNC_AS_STAT) ||
 		    !(stat.result_mask & STATX_CHANGE_COOKIE) ||
@@ -347,7 +347,7 @@ static int process_measurement(struct file *file, const struct cred *cred,
 		goto out_locked;
 
 	if (!pathbuf)	/* ima_rdwr_violation possibly pre-fetched */
-		pathname = ima_d_path(&file->f_path, &pathbuf, filename);
+		pathname = ima_d_path(file_real_path(file), &pathbuf, filename);
 
 	if (action & IMA_MEASURE)
 		ima_store_measurement(iint, file, pathname,
@@ -487,7 +487,7 @@ int ima_file_mprotect(struct vm_area_struct *vma, unsigned long prot)
 		result = -EPERM;
 
 	file = vma->vm_file;
-	pathname = ima_d_path(&file->f_path, &pathbuf, filename);
+	pathname = ima_d_path(file_real_path(file), &pathbuf, filename);
 	integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, pathname,
 			    "collect_data", "failed-mprotect", result, 0);
 	if (pathbuf)
-- 
2.34.1




[Index of Archives]     [Linux Filesystems Devel]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux