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

[Stefan Berger <stefanb@linux.ibm.com>]

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.