Received-SPF: pass (google.com: domain of linux-kernel+bounces-55236-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240205182506.3569743-1-stefanb@linux.ibm.com> <20240205182506.3569743-10-stefanb@linux.ibm.com>
In-Reply-To: <20240205182506.3569743-10-stefanb@linux.ibm.com>
From: Amir Goldstein <amir73il@gmail.com>
Date: Tue, 6 Feb 2024 17:23:14 +0200
Message-ID: <CAOQ4uxhQgCqNZ_4Dy1PghCHBg7H4qaf-st2pWhXe9eq1+dsHHA@mail.gmail.com>
Subject: Re: [PATCH v2 9/9] ima: Record i_version of real_inode for change detection
To: Stefan Berger <stefanb@linux.ibm.com>, Jeff Layton <jlayton@kernel.org>
Cc: linux-integrity@vger.kernel.org, linux-security-module@vger.kernel.org, 
	linux-unionfs@vger.kernel.org, linux-kernel@vger.kernel.org, 
	paul@paul-moore.com, jmorris@namei.org, serge@hallyn.com, zohar@linux.ibm.com, 
	roberto.sassu@huawei.com, brauner@kernel.org, miklos@szeredi.hu
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Mon, Feb 5, 2024 at 8:25=E2=80=AFPM Stefan Berger <stefanb@linux.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@linux.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/im=
a_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_cac=
he *iint,
>         int result =3D 0;
>         int length;
>         void *tmpbuf;
> -       u64 i_version =3D 0;
>
>         /*
>          * Always collect the modsig, because IMA might have already coll=
ected
> @@ -263,16 +263,6 @@ int ima_collect_measurement(struct integrity_iint_ca=
che *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 =3D vfs_getattr_nosec(&file->f_path, &stat, STATX_CHANGE_C=
OOKIE,
> -                                  AT_STATX_SYNC_AS_STAT);
> -       if (!result && (stat.result_mask & STATX_CHANGE_COOKIE))
> -               i_version =3D stat.change_cookie;
>         hash.hdr.algo =3D algo;
>         hash.hdr.length =3D hash_digest_size[algo];
>
> @@ -302,10 +292,22 @@ int ima_collect_measurement(struct integrity_iint_c=
ache *iint,
>
>         iint->ima_hash =3D tmpbuf;
>         memcpy(iint->ima_hash, &hash, length);
> -       iint->version =3D i_version;
> -       if (real_inode !=3D inode) {
> +       if (real_inode =3D=3D inode) {
> +               /*
> +                * Detecting file change is based on i_version. On filesy=
stems
> +                * which do not support i_version, support was originally=
 limited
> +                * to an initial measurement/appraisal/audit, but was mod=
ified to
> +                * assume the file changed.
> +                */
> +               result =3D 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 =3D stat.change_cookie;
> +       } else {
>                 iint->real_ino =3D real_inode->i_ino;
>                 iint->real_dev =3D real_inode->i_sb->s_dev;
> +               iint->version =3D 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 futu=
re."
Jeff?


Thanks,
Amir.