Received-SPF: pass (google.com: domain of linux-kernel+bounces-46541-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Precedence: bulk
MIME-Version: 1.0
References: <20240130214620.3155380-1-stefanb@linux.ibm.com> <20240130214620.3155380-4-stefanb@linux.ibm.com>
In-Reply-To: <20240130214620.3155380-4-stefanb@linux.ibm.com>
From: Amir Goldstein <amir73il@gmail.com>
Date: Wed, 31 Jan 2024 15:56:28 +0200
Message-ID: <CAOQ4uxjC=-GOFi3J4ctcNgdMfaerkae30OH9=TkKTWCf=TP95g@mail.gmail.com>
Subject: Re: [PATCH 3/5] ima: Reset EVM status upon detecting changes to
 overlay backing file
To: Stefan Berger <stefanb@linux.ibm.com>
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, miklos@szeredi.hu, 
	Christian Brauner <brauner@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Tue, Jan 30, 2024 at 11:46=E2=80=AFPM Stefan Berger <stefanb@linux.ibm.c=
om> wrote:
>
> To avoid caching effects to take effect reset the EVM status upon
> detecting changes to the overlay backing files. This prevents a not-yet-
> copied-up file on the overlay from executing if for example the
> security.evm xattr on the file on the 'lower' layer has been removed.
>

And what is expected to happen when file is executed after copy up?
Doesn't this change also protect the same threat after copy up?

> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> ---
>  include/linux/evm.h               | 8 ++++++++
>  security/integrity/evm/evm_main.c | 7 +++++++
>  security/integrity/ima/ima_main.c | 2 ++
>  3 files changed, 17 insertions(+)
>
> diff --git a/include/linux/evm.h b/include/linux/evm.h
> index d8c0343436b8..e7d6742eee9d 100644
> --- a/include/linux/evm.h
> +++ b/include/linux/evm.h
> @@ -66,6 +66,8 @@ extern int evm_protected_xattr_if_enabled(const char *r=
eq_xattr_name);
>  extern int evm_read_protected_xattrs(struct dentry *dentry, u8 *buffer,
>                                      int buffer_size, char type,
>                                      bool canonical_fmt);
> +extern void evm_reset_cache_status(struct dentry *dentry,
> +                                  struct integrity_iint_cache *iint);
>  #ifdef CONFIG_FS_POSIX_ACL
>  extern int posix_xattr_acl(const char *xattrname);
>  #else
> @@ -189,5 +191,11 @@ static inline int evm_read_protected_xattrs(struct d=
entry *dentry, u8 *buffer,
>         return -EOPNOTSUPP;
>  }
>
> +static inline void evm_reset_cache_status(struct dentry *dentry,
> +                                         struct integrity_iint_cache *ii=
nt)
> +{
> +       return;
> +}
> +
>  #endif /* CONFIG_EVM */
>  #endif /* LINUX_EVM_H */
> diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/e=
vm_main.c
> index 22a5e26860ea..e96d127b48a2 100644
> --- a/security/integrity/evm/evm_main.c
> +++ b/security/integrity/evm/evm_main.c
> @@ -721,6 +721,13 @@ static void evm_reset_status(struct inode *inode)
>                 iint->evm_status =3D INTEGRITY_UNKNOWN;
>  }
>
> +void evm_reset_cache_status(struct dentry *dentry,
> +                           struct integrity_iint_cache *iint)
> +{
> +       if (d_real_inode(dentry) !=3D d_backing_inode(dentry))
> +               iint->evm_status =3D INTEGRITY_UNKNOWN;
> +}
> +
>  /**
>   * evm_revalidate_status - report whether EVM status re-validation is ne=
cessary
>   * @xattr_name: pointer to the affected extended attribute name
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/i=
ma_main.c
> index cc1217ac2c6f..84bdc6e58329 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -26,6 +26,7 @@
>  #include <linux/ima.h>
>  #include <linux/fs.h>
>  #include <linux/iversion.h>
> +#include <linux/evm.h>
>
>  #include "ima.h"
>
> @@ -295,6 +296,7 @@ static int process_measurement(struct file *file, con=
st struct cred *cred,
>                     !inode_eq_iversion(backing_inode, iint->version)) {
>                         iint->flags &=3D ~IMA_DONE_MASK;
>                         iint->measured_pcrs =3D 0;
> +                       evm_reset_cache_status(file_dentry(file), iint);
>                 }
>         }

Make sense.
Unrelated to your change, I now noticed something odd about Mimi's change:

        backing_inode =3D d_real_inode(file_dentry(file));

I find the choice of variable name to be quite confusing, because ima/evm c=
ode
uses  d_backing_inode() all over the place and d_real_inode() !=3D
d_backing_inode().

First of all, there is never any reason to use d_backing_inode() and its na=
me is
quite confusing in the first place, but it will be a big cleanup to
remove them all.

Suggest to rename the variable to real_inode, same as in
ima_collect_measurement()
to be consistent and reduce confusion factor, which is already high enough =
;)

Thanks,
Amir.