| Message ID | 20231025112932.84336-1-zohar@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] ima: detect changes to the backing overlay file | expand |
On Wed, Oct 25, 2023 at 2:29 PM Mimi Zohar <zohar@linux.ibm.com> wrote: > > Commit 18b44bc5a672 ("ovl: Always reevaluate the file signature for > IMA") forced signature re-evaulation on every file access. > > Instead of always re-evaluating the file's integrity, detect a change > to the backing file, by comparing the cached file metadata with the > backing file's metadata. Verifying just the i_version has not changed > is insufficient. In addition save and compare the i_ino and i_rdev > as well. > > Signed-off-by: Mimi Zohar <zohar@linux.ibm.com> > --- > Changelog: > - Changes made based on Amir's review: removal of unnecessary overlay > magic test, verify i_version, i_ino and i_rdev haven't changed. > > fs/overlayfs/super.c | 2 +- > security/integrity/ima/ima_api.c | 4 ++++ > security/integrity/ima/ima_main.c | 16 +++++++++++++++- > security/integrity/integrity.h | 2 ++ > 4 files changed, 22 insertions(+), 2 deletions(-) > > diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c > index 3fa2416264a4..c71d185980c0 100644 > --- a/fs/overlayfs/super.c > +++ b/fs/overlayfs/super.c > @@ -1489,7 +1489,7 @@ int ovl_fill_super(struct super_block *sb, struct fs_context *fc) > ovl_trusted_xattr_handlers; > sb->s_fs_info = ofs; > sb->s_flags |= SB_POSIXACL; > - sb->s_iflags |= SB_I_SKIP_SYNC | SB_I_IMA_UNVERIFIABLE_SIGNATURE; > + sb->s_iflags |= SB_I_SKIP_SYNC; > > err = -ENOMEM; > root_dentry = ovl_get_root(sb, ctx->upper.dentry, oe); > diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c > index 452e80b541e5..f191bdcceef8 100644 > --- a/security/integrity/ima/ima_api.c > +++ b/security/integrity/ima/ima_api.c > @@ -243,6 +243,7 @@ int ima_collect_measurement(struct integrity_iint_cache *iint, > { > const char *audit_cause = "failed"; > struct inode *inode = file_inode(file); > + struct inode *real_inode = d_real_inode(file_dentry(file)); > const char *filename = file->f_path.dentry->d_name.name; > struct ima_max_digest_data hash; > struct kstat stat; > @@ -272,6 +273,7 @@ int ima_collect_measurement(struct integrity_iint_cache *iint, > 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,6 +304,8 @@ int ima_collect_measurement(struct integrity_iint_cache *iint, > iint->ima_hash = tmpbuf; > memcpy(iint->ima_hash, &hash, length); > iint->version = i_version; You don't have to store them if real_inode == inode. > + iint->real_ino = real_inode->i_ino; > + iint->real_rdev = real_inode->i_rdev; iint->real_dev = real_inode->i_sb->s_dev; i_rdev is something else. it's the device pointed to by a blockdev/chardev inode. Thanks, Amir. > > /* Possibly temporary failure due to type of read (eg. O_DIRECT) */ > if (!result) > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c > index 365db0e43d7c..4a6a22f8805b 100644 > --- a/security/integrity/ima/ima_main.c > +++ b/security/integrity/ima/ima_main.c > @@ -25,6 +25,7 @@ > #include <linux/xattr.h> > #include <linux/ima.h> > #include <linux/fs.h> > +#include <linux/iversion.h> > > #include "ima.h" > > @@ -207,7 +208,7 @@ static int process_measurement(struct file *file, const struct cred *cred, > u32 secid, char *buf, loff_t size, int mask, > enum ima_hooks func) > { > - struct inode *inode = file_inode(file); > + struct inode *backing_inode, *inode = file_inode(file); > struct integrity_iint_cache *iint = NULL; > struct ima_template_desc *template_desc = NULL; > char *pathbuf = NULL; > @@ -284,6 +285,19 @@ static int process_measurement(struct file *file, const struct cred *cred, > iint->measured_pcrs = 0; > } > > + /* Detect and re-evaluate changes made to the backing file. */ > + backing_inode = d_real_inode(file_dentry(file)); > + if (backing_inode != inode && > + (action & IMA_DO_MASK) && (iint->flags & IMA_DONE_MASK)) { > + if (!IS_I_VERSION(backing_inode) || > + backing_inode->i_rdev != iint->real_rdev || > + backing_inode->i_ino != iint->real_ino || > + !inode_eq_iversion(backing_inode, iint->version)) { > + iint->flags &= ~IMA_DONE_MASK; > + iint->measured_pcrs = 0; > + } > + } > + > /* Determine if already appraised/measured based on bitmask > * (IMA_MEASURE, IMA_MEASURED, IMA_XXXX_APPRAISE, IMA_XXXX_APPRAISED, > * IMA_AUDIT, IMA_AUDITED) > diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h > index d7553c93f5c0..dd2bb2d150f6 100644 > --- a/security/integrity/integrity.h > +++ b/security/integrity/integrity.h > @@ -164,6 +164,8 @@ struct integrity_iint_cache { > unsigned long flags; > unsigned long measured_pcrs; > unsigned long atomic_flags; > + unsigned long real_ino; > + dev_t real_rdev; > enum integrity_status ima_file_status:4; > enum integrity_status ima_mmap_status:4; > enum integrity_status ima_bprm_status:4; > -- > 2.39.3 >
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c index 3fa2416264a4..c71d185980c0 100644 --- a/fs/overlayfs/super.c +++ b/fs/overlayfs/super.c @@ -1489,7 +1489,7 @@ int ovl_fill_super(struct super_block *sb, struct fs_context *fc) ovl_trusted_xattr_handlers; sb->s_fs_info = ofs; sb->s_flags |= SB_POSIXACL; - sb->s_iflags |= SB_I_SKIP_SYNC | SB_I_IMA_UNVERIFIABLE_SIGNATURE; + sb->s_iflags |= SB_I_SKIP_SYNC; err = -ENOMEM; root_dentry = ovl_get_root(sb, ctx->upper.dentry, oe); diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c index 452e80b541e5..f191bdcceef8 100644 --- a/security/integrity/ima/ima_api.c +++ b/security/integrity/ima/ima_api.c @@ -243,6 +243,7 @@ int ima_collect_measurement(struct integrity_iint_cache *iint, { const char *audit_cause = "failed"; struct inode *inode = file_inode(file); + struct inode *real_inode = d_real_inode(file_dentry(file)); const char *filename = file->f_path.dentry->d_name.name; struct ima_max_digest_data hash; struct kstat stat; @@ -272,6 +273,7 @@ int ima_collect_measurement(struct integrity_iint_cache *iint, 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,6 +304,8 @@ int ima_collect_measurement(struct integrity_iint_cache *iint, iint->ima_hash = tmpbuf; memcpy(iint->ima_hash, &hash, length); iint->version = i_version; + iint->real_ino = real_inode->i_ino; + iint->real_rdev = real_inode->i_rdev; /* Possibly temporary failure due to type of read (eg. O_DIRECT) */ if (!result) diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index 365db0e43d7c..4a6a22f8805b 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -25,6 +25,7 @@ #include <linux/xattr.h> #include <linux/ima.h> #include <linux/fs.h> +#include <linux/iversion.h> #include "ima.h" @@ -207,7 +208,7 @@ static int process_measurement(struct file *file, const struct cred *cred, u32 secid, char *buf, loff_t size, int mask, enum ima_hooks func) { - struct inode *inode = file_inode(file); + struct inode *backing_inode, *inode = file_inode(file); struct integrity_iint_cache *iint = NULL; struct ima_template_desc *template_desc = NULL; char *pathbuf = NULL; @@ -284,6 +285,19 @@ static int process_measurement(struct file *file, const struct cred *cred, iint->measured_pcrs = 0; } + /* Detect and re-evaluate changes made to the backing file. */ + backing_inode = d_real_inode(file_dentry(file)); + if (backing_inode != inode && + (action & IMA_DO_MASK) && (iint->flags & IMA_DONE_MASK)) { + if (!IS_I_VERSION(backing_inode) || + backing_inode->i_rdev != iint->real_rdev || + backing_inode->i_ino != iint->real_ino || + !inode_eq_iversion(backing_inode, iint->version)) { + iint->flags &= ~IMA_DONE_MASK; + iint->measured_pcrs = 0; + } + } + /* Determine if already appraised/measured based on bitmask * (IMA_MEASURE, IMA_MEASURED, IMA_XXXX_APPRAISE, IMA_XXXX_APPRAISED, * IMA_AUDIT, IMA_AUDITED) diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h index d7553c93f5c0..dd2bb2d150f6 100644 --- a/security/integrity/integrity.h +++ b/security/integrity/integrity.h @@ -164,6 +164,8 @@ struct integrity_iint_cache { unsigned long flags; unsigned long measured_pcrs; unsigned long atomic_flags; + unsigned long real_ino; + dev_t real_rdev; enum integrity_status ima_file_status:4; enum integrity_status ima_mmap_status:4; enum integrity_status ima_bprm_status:4;
Commit 18b44bc5a672 ("ovl: Always reevaluate the file signature for IMA") forced signature re-evaulation on every file access. Instead of always re-evaluating the file's integrity, detect a change to the backing file, by comparing the cached file metadata with the backing file's metadata. Verifying just the i_version has not changed is insufficient. In addition save and compare the i_ino and i_rdev as well. Signed-off-by: Mimi Zohar <zohar@linux.ibm.com> --- Changelog: - Changes made based on Amir's review: removal of unnecessary overlay magic test, verify i_version, i_ino and i_rdev haven't changed. fs/overlayfs/super.c | 2 +- security/integrity/ima/ima_api.c | 4 ++++ security/integrity/ima/ima_main.c | 16 +++++++++++++++- security/integrity/integrity.h | 2 ++ 4 files changed, 22 insertions(+), 2 deletions(-)