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
