linux-kernel - Re: [PATCH v2 9/9] ima: Record i_version of real

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Tue, 6 Feb 2024 17:23:14 +0200
From: Amir Goldstein <amir73il@...il.com>
To: Stefan Berger <stefanb@...ux.ibm.com>, Jeff Layton <jlayton@...nel.org>
Cc: linux-integrity@...r.kernel.org, linux-security-module@...r.kernel.org, 
	linux-unionfs@...r.kernel.org, linux-kernel@...r.kernel.org, 
	paul@...l-moore.com, jmorris@...ei.org, serge@...lyn.com, zohar@...ux.ibm.com, 
	roberto.sassu@...wei.com, brauner@...nel.org, miklos@...redi.hu
Subject: Re: [PATCH v2 9/9] ima: Record i_version of real_inode for change detection

On Mon, Feb 5, 2024 at 8:25 PM Stefan Berger <stefanb@...ux.ibm.com> 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@...ux.ibm.com>
> ---
>  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.