Received-SPF: pass (google.com: domain of linux-kernel+bounces-55270-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240205182506.3569743-1-stefanb@linux.ibm.com> <20240205182506.3569743-5-stefanb@linux.ibm.com>
In-Reply-To: <20240205182506.3569743-5-stefanb@linux.ibm.com>
From: Amir Goldstein <amir73il@gmail.com>
Date: Tue, 6 Feb 2024 17:44:36 +0200
Message-ID: <CAOQ4uxgbURYypK1Ycib8ojrRvZGtQYBMF6CwY86DDSWqMVHW2Q@mail.gmail.com>
Subject: Re: [PATCH v2 4/9] ima: Reset EVM status upon detecting changes to
 the real 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, 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:
>
> Piggyback the resetting of EVM status on IMA's file content detection tha=
t
> is triggered when a not-yet-copied-up file on the 'lower' layer was
> changed. However, since EVM only cares about changes to the file metadata=
,
> only reset the EVM status if the 'lower' layer file is also the one holdi=
ng
> the file metadata.
>
> Note that in the case of a stacked filesystem (e.g., overlayfs) the iint
> represents the file_inode() of a file on the overlay layer. The data in
> the in iint must help detect file content (IMA) and file metadata (EVM)
> changes occurring on the lower layer for as long as the content or
> metadata have not been copied up yet. After copy-up the iit must continue
> detecting them on the overlay layer.
>
> Changes to the file metadata on the overlay layer are causing an EVM
> status reset through existing evm_inode_post_sattr/setxattr/removexattr
> functions *if* an iint for a file exist. An iint exists if the file is
> 'in (IMA) policy', meaning that IMA created an iint for the file's inode
> since the file is covered by the IMA policy.
>
> 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 | 5 +++++
>  3 files changed, 20 insertions(+)
>
> diff --git a/include/linux/evm.h b/include/linux/evm.h
> index 840ffbdc2860..eade9fff7d0b 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
> @@ -190,5 +192,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 565c36471408..81c94967f136 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))

Is this really needed?
You get here after checking (real_inode !=3D inode) already

> +               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 f1a01d32b92a..b6ba829c4e67 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"
>
> @@ -297,6 +298,10 @@ static int process_measurement(struct file *file, co=
nst struct cred *cred,
>                     !inode_eq_iversion(real_inode, iint->version)) {
>                         iint->flags &=3D ~IMA_DONE_MASK;
>                         iint->measured_pcrs =3D 0;
> +
> +                       if (real_inode =3D=3D d_inode(d_real(file_dentry(=
file),
> +                                                        D_REAL_METADATA)=
))
> +                               evm_reset_cache_status(file_dentry(file),=
 iint);

Technically, you'd also need to store iint->real_meta_{dev,ino}
when calculating EVM to be sure if the metadata inode had changed,
because there is a possibility that file was not copied up yet, but the fil=
e
is a metacopy in a middle layer and the lower data is in another layer.

Think file metadata was copied from lower to upper layer, then the
upper layer was made a middle layer and another upper layer added
on top of it.

In this situation, real_inode is in the lower layer, real_meta_inode is in
the middle layer and after copy up of metadata, real_meta_inode will
become in the upper layer.

Not sure if this use case is interesting to EVM.

Thanks,
Amir.