| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 13 Feb 2024 18:14:00 -0500
From: Stefan Berger <stefanb@...ux.ibm.com>
To: Jeff Layton <jlayton@...nel.org>, Amir Goldstein <amir73il@...il.com>
Cc: linux-integrity@...r.kernel.org, linux-security-module@...r.kernel.org,
linux-unionfs@...r.kernel.org, linux-kernel@...r.kernel.org,
paul@...l-moore.com, jmorris@...ei.org, serge@...lyn.com,
zohar@...ux.ibm.com, roberto.sassu@...wei.com, brauner@...nel.org,
miklos@...redi.hu
Subject: Re: [PATCH v2 9/9] ima: Record i_version of real_inode for change
detection
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@...ux.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@...ux.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.
Powered by blists - more mailing lists