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

  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(-)
  

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);
>         }
>

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.

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."
Jeff?


Thanks,
Amir.
  

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.
> 
> 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
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?
  

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.