| Message ID | 20240205182506.3569743-10-stefanb@linux.ibm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | evm: Support signatures on stacked filesystem | expand |
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.
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); } /* Possibly temporary failure due to type of read (eg. O_DIRECT) */
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(-)