Re: [PATCH v2 9/9] ima: Record i_version of real_inode for change detection

Stefan Berger
2024-02-13T23:14:31Z

On 2/6/24 10:54, Jeff Layton wrote:
> On Tue, 2024-02-06 at 17:23 +0200, Amir Goldstein wrote:
>> On Mon, Feb 5, 2024 at 8:25 PM 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/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);
>
> You only want to do this if IS_I_VERSION(inode) is true. If the
> underlying filesystem is doing its own thing wrt the i_version field,
> calling inode_query_iversion on it may corrupt it.
>
>
>>> }
>>>
>>
>> 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.

Following this patch inode_query_version() would only be used when
real_inode != inode, such as when a copy-up has not occurred, yet. If
real_inode == inode then this is the case for the 'overlay' layer of
overlayfs as well as any other non-stacked filesystem that would then
still use vfs_getattr_nosec(). So is vfs_getattr_nosec() NOT the more
general approach for all filesystems to use here?

>>
>> 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."
>
> I don't have a real opinion here. When I did the original patch that
> switched this over to to use vfs_getattr_nosec, I didn't consider that
> it could end up being called from an atomic context. Reverting that

Under what conditions do we have an atomic context here? I was/am not
aware of this.

> seems like the correct thing to do if it's still broken.
>
> If you're fine with this only working on a subset of local filesystems,
> then doing something like this is probably fine:
>
> if (IS_I_VERSION(real_inode))
> iint->version = inode_query_iversion(real_inode);
>
> ...but it's not clear to me what you should do if IS_I_VERSION is false.
> I guess IMA just falls back to checking the ctime in that case?

It does not use ctime but assumes that something has changed.