On Mon, Feb 5, 2024 at 8:25 PM Stefan Berger <stefanb@xxxxxxxxxxxxx> wrote:
>
> process_measurement() will try to detect file content changes for not-yet-
> copied-up files on a stacked filesystem based on the i_version number of
> the real inode: !inode_eq_iversion(real_inode, iint->version)
> Therefore, take a snapshot of the i_version of the real file to be used
> for i_version number-based file content change detection by IMA in
> process_meassurements().
>
> In this case vfs_getattr_nosec() cannot be used since it will return the
> i_version number of the file on the overlay layer which will trigger more
> iint resets in process_measurements() than necessary since this i_version
> number represents different state than that of the real_inode (of a
> not-yet-copied up file).
>
> Signed-off-by: Stefan Berger <stefanb@xxxxxxxxxxxxx>
> ---
> security/integrity/ima/ima_api.c | 28 +++++++++++++++-------------
> 1 file changed, 15 insertions(+), 13 deletions(-)
>
> diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
> index 597ea0c4d72f..530888cc481e 100644
> --- a/security/integrity/ima/ima_api.c
> +++ b/security/integrity/ima/ima_api.c
> @@ -14,6 +14,7 @@
> #include <linux/xattr.h>
> #include <linux/evm.h>
> #include <linux/fsverity.h>
> +#include <linux/iversion.h>
>
> #include "ima.h"
>
> @@ -250,7 +251,6 @@ int ima_collect_measurement(struct integrity_iint_cache *iint,
> int result = 0;
> int length;
> void *tmpbuf;
> - u64 i_version = 0;
>
> /*
> * Always collect the modsig, because IMA might have already collected
> @@ -263,16 +263,6 @@ int ima_collect_measurement(struct integrity_iint_cache *iint,
> if (iint->flags & IMA_COLLECTED)
> goto out;
>
> - /*
> - * Detecting file change is based on i_version. On filesystems
> - * which do not support i_version, support was originally limited
> - * 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);
> - if (!result && (stat.result_mask & STATX_CHANGE_COOKIE))
> - i_version = stat.change_cookie;
> hash.hdr.algo = algo;
> hash.hdr.length = hash_digest_size[algo];
>
> @@ -302,10 +292,22 @@ int ima_collect_measurement(struct integrity_iint_cache *iint,
>
> iint->ima_hash = tmpbuf;
> memcpy(iint->ima_hash, &hash, length);
> - iint->version = i_version;
> - if (real_inode != inode) {
> + if (real_inode == inode) {
> + /*
> + * Detecting file change is based on i_version. On filesystems
> + * which do not support i_version, support was originally limited
> + * 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);
> + if (!result && (stat.result_mask & STATX_CHANGE_COOKIE))
> + iint->version = stat.change_cookie;
> + } else {
> iint->real_ino = real_inode->i_ino;
> iint->real_dev = real_inode->i_sb->s_dev;
> + iint->version = inode_query_iversion(real_inode);
> }
>
The commit that removed inode_query_iversion db1d1e8b9867 ("IMA: use
vfs_getattr_nosec to get the i_version") claimed to do that because
inode_query_iversion() did not work in overlayfs and now this commit
uses inode_query_iversion() only for overlayfs.
STATX_CHANGE_COOKIE does not seem to make much sense in this
code anymore, unless it is still needed, according to original commit to
"allow IMA to work properly with a broader class of filesystems in the future."
Jeff?
Thanks,
Amir.
