On Wed, Sep 13, 2023 at 10:38 AM Amir Goldstein <amir73il@xxxxxxxxx> wrote: > > 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 integrity check context > at the time that commit a408e4a86b36 was merged. > Only problem is that fix did not resolve the syzbot bug, which seems to do the IMA integrity check on overlayfs file (not sure). I am pretty sure that this patch fixes "a bug" when IMA is on the filesystem under overlayfs and this is a pretty important use case. But I guess there are still issues with IMA over overlayfs and this is not the only one. Is this really a use case that needs to be supported? Isn't the newly added SB_I_IMA_UNVERIFIABLE_SIGNATURE flag a hint that IMA on overlayfs is not a good idea at all? 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 >
