Please use "__fput()" rather than "file close". Perhaps update the subject line to
something like "ima: Defer fixing security.ima to __fput()".
On Thu, 2024-11-28 at 11:06 +0100, Roberto Sassu wrote:
> From: Roberto Sassu <roberto.sassu@xxxxxxxxxx>
>
> IMA-Appraisal implements a fix mode, selectable from the kernel command
> line by specifying ima_appraise=fix.
>
> The fix mode is meant to be used in a TOFU (trust on first use) model,
> where systems are supposed to work under controlled conditions before the
> real enforcement starts.
>
> Since the systems are under controlled conditions, it is assumed that the
> files are not corrupted, and thus their current data digest can be trusted,
> and written to security.ima.
>
> When IMA-Appraisal is switched to enforcing mode, the security.ima value
> collected during the fix mode is used as a reference value, and a mismatch
> with the current value cause the access request to be denied.
>
> However, since fixing security.ima is placed in ima_appraise_measurement()
> during the integrity check, it requires the inode lock to be taken in
> process_measurement(), in addition to ima_update_xattr() invoked at file
> close.
>
> Postpone the security.ima update to ima_check_last_writer(), by setting the
> new atomic flag IMA_UPDATE_XATTR_FIX in the inode integrity metadata, in
> ima_appraise_measurement(), if security.ima needs to be fixed. In this way,
> the inode lock can be removed from process_measurement(). Also, set the
> cause appropriately for the fix operation and for allowing access to new
> and empty signed files.
>
> Finally, update security.ima when IMA_UPDATE_XATTR_FIX is set, and when
> there wasn't a previous security.ima update, which occurs if the process
> closing the file descriptor is the last writer.
>
> Signed-off-by: Roberto Sassu <roberto.sassu@xxxxxxxxxx>
Roberto, I really like the idea of removing the inode_lock in process_measurement()
needed for writing xattrs, but I'm concerned about the delay being introduced. For
example, does it interfere with labeling the filesystem with file signatures
(with/without EVM enabled)?
> ---
> security/integrity/ima/ima.h | 1 +
> security/integrity/ima/ima_appraise.c | 7 +++++--
> security/integrity/ima/ima_main.c | 18 +++++++++++-------
> 3 files changed, 17 insertions(+), 9 deletions(-)
>
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index b4eeab48f08a..22c3b87cfcac 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -179,6 +179,7 @@ struct ima_kexec_hdr {
> #define IMA_CHANGE_ATTR 2
> #define IMA_DIGSIG 3
> #define IMA_MUST_MEASURE 4
> +#define IMA_UPDATE_XATTR_FIX 5
>
> /* IMA integrity metadata associated with an inode */
> struct ima_iint_cache {
> diff --git a/security/integrity/ima/ima_appraise.c
> b/security/integrity/ima/ima_appraise.c
> index 656c709b974f..94401de8b805 100644
> --- a/security/integrity/ima/ima_appraise.c
> +++ b/security/integrity/ima/ima_appraise.c
> @@ -576,8 +576,10 @@ int ima_appraise_measurement(enum ima_hooks func, struct
> ima_iint_cache *iint,
> if ((ima_appraise & IMA_APPRAISE_FIX) && !try_modsig &&
> (!xattr_value ||
> xattr_value->type != EVM_IMA_XATTR_DIGSIG)) {
> - if (!ima_fix_xattr(dentry, iint))
> - status = INTEGRITY_PASS;
> + /* Fix by setting security.ima on file close. */
> + set_bit(IMA_UPDATE_XATTR_FIX, &iint->atomic_flags);
> + status = INTEGRITY_PASS;
> + cause = "fix";
> }
>
> /*
> @@ -587,6 +589,7 @@ int ima_appraise_measurement(enum ima_hooks func, struct
> ima_iint_cache *iint,
> if (inode->i_size == 0 && iint->flags & IMA_NEW_FILE &&
> test_bit(IMA_DIGSIG, &iint->atomic_flags)) {
> status = INTEGRITY_PASS;
> + cause = "new-signed-file";
> }
>
> integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, filename,
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 1e474ff6a777..50b37420ea2c 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -158,13 +158,16 @@ static void ima_check_last_writer(struct ima_iint_cache
> *iint,
> struct inode *inode, struct file *file)
> {
> fmode_t mode = file->f_mode;
> - bool update;
> + bool update = false, update_fix;
>
> - if (!(mode & FMODE_WRITE))
> + update_fix = test_and_clear_bit(IMA_UPDATE_XATTR_FIX,
> + &iint->atomic_flags);
> +
> + if (!(mode & FMODE_WRITE) && !update_fix)
> return;
>
> ima_iint_lock(inode);
> - if (atomic_read(&inode->i_writecount) == 1) {
> + if (atomic_read(&inode->i_writecount) == 1 && (mode & FMODE_WRITE)) {
Probably better to reverse the "mode & FMODE_WRITE" and atomic_read() test order.
Mimi
> struct kstat stat;
>
> update = test_and_clear_bit(IMA_UPDATE_XATTR,
> @@ -181,6 +184,10 @@ static void ima_check_last_writer(struct ima_iint_cache *iint,
> ima_update_xattr(iint, file);
> }
> }
> +
> + if (!update && update_fix)
> + ima_update_xattr(iint, file);
> +
> ima_iint_unlock(inode);
> }
>
> @@ -378,13 +385,10 @@ static int process_measurement(struct file *file, const
> struct cred *cred,
> template_desc);
> if (rc == 0 && (action & IMA_APPRAISE_SUBMASK)) {
> rc = ima_check_blacklist(iint, modsig, pcr);
> - if (rc != -EPERM) {
> - inode_lock(inode);
> + if (rc != -EPERM)
> rc = ima_appraise_measurement(func, iint, file,
> pathname, xattr_value,
> xattr_len, modsig);
> - inode_unlock(inode);
> - }
> if (!rc)
> rc = mmap_violation_check(func, file, &pathbuf,
> &pathname, filename);
