| Message ID | 20231025143906.133218-1-zohar@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] ima: detect changes to the backing overlay file | expand |
On Wed, Oct 25, 2023 at 8:39 AM Mimi Zohar <zohar@linux.ibm.com> wrote: > + if (!IS_I_VERSION(backing_inode) || > + backing_inode->i_sb->s_dev != iint->real_dev || > + backing_inode->i_ino != iint->real_ino || > + !inode_eq_iversion(backing_inode, iint->version)) { > + iint->flags &= ~IMA_DONE_MASK; > + iint->measured_pcrs = 0; > + } > + } > + Does this mean I need to mount ext4 with `-o iversion`? Or has it been enabled by default? I can test this patch out sometime this week and verify it fixes the performance regression. Thanks!
On Wed, 2023-10-25 at 08:59 -0600, Raul Rangel wrote: > On Wed, Oct 25, 2023 at 8:39 AM Mimi Zohar <zohar@linux.ibm.com> wrote: > > + if (!IS_I_VERSION(backing_inode) || > > + backing_inode->i_sb->s_dev != iint->real_dev || > > + backing_inode->i_ino != iint->real_ino || > > + !inode_eq_iversion(backing_inode, iint->version)) { > > + iint->flags &= ~IMA_DONE_MASK; > > + iint->measured_pcrs = 0; > > + } > > + } > > + > Does this mean I need to mount ext4 with `-o iversion`? Or has it been > enabled by default? According to commit 1ff20307393e ("ext4: unconditionally enable the i_version counter") it's now enabled by default. > > I can test this patch out sometime this week and verify it fixes the > performance regression. > > Thanks! Much appreciated!
> On Oct 25, 2023, at 8:39 AM, 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 s_dev > as well. > > Signed-off-by: Mimi Zohar <zohar@linux.ibm.com> I ran the file integrity tests that originally uncovered the need for "Commit 18b44bc5a672 ("ovl: Always reevaluate the file signature for IMA”). When the backing file is changed, file integrity remains. For that part, feel free to add: Tested-by: Eric Snowberg <eric.snowberg@oracle.com>
On Wed, 2023-10-25 at 16:27 +0000, Eric Snowberg wrote: > > > On Oct 25, 2023, at 8:39 AM, 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 s_dev > > as well. > > > > Signed-off-by: Mimi Zohar <zohar@linux.ibm.com> > > I ran the file integrity tests that originally uncovered the need for > "Commit 18b44bc5a672 ("ovl: Always reevaluate the file signature for > IMA”). When the backing file is changed, file integrity remains. For that > part, feel free to add: > > Tested-by: Eric Snowberg <eric.snowberg@oracle.com> Thanks! Mimi
On Wed, Oct 25, 2023 at 12:01 PM Mimi Zohar <zohar@linux.ibm.com> wrote: > > On Wed, 2023-10-25 at 16:27 +0000, Eric Snowberg wrote: > > > > > On Oct 25, 2023, at 8:39 AM, 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 s_dev > > > as well. > > > > > > Signed-off-by: Mimi Zohar <zohar@linux.ibm.com> > > > > I ran the file integrity tests that originally uncovered the need for > > "Commit 18b44bc5a672 ("ovl: Always reevaluate the file signature for > > IMA”). When the backing file is changed, file integrity remains. For that > > part, feel free to add: > > > > Tested-by: Eric Snowberg <eric.snowberg@oracle.com> > > Thanks! > > Mimi > I just verified this fixes the speed regression: ``` rrangel920 / # time clang --version >/dev/null real 0m0.369s user 0m0.000s sys 0m0.368s rrangel920 / # rrangel920 / # time clang --version >/dev/null real 0m0.017s user 0m0.004s sys 0m0.013s rrangel920 / # time clang --version >/dev/null real 0m0.012s user 0m0.000s sys 0m0.012s rrangel920 / # time clang --version >/dev/null real 0m0.012s user 0m0.000s sys 0m0.012s ``` Tested-by: Raul E Rangel <rrangel@chromium.org> Thanks again for the quick fix!
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..597ea0c4d72f 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; @@ -302,6 +303,10 @@ 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) { + iint->real_ino = real_inode->i_ino; + iint->real_dev = real_inode->i_sb->s_dev; + } /* 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..cc1217ac2c6f 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_sb->s_dev != iint->real_dev || + 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..9561db7cf6b4 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_dev; 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 s_dev as well. Signed-off-by: Mimi Zohar <zohar@linux.ibm.com> --- Changelog: - Changes made based on Amir's review: v2: Use s_dev, not i_rdev. Limit setting real_ino, real_dev. v1: 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 | 5 +++++ security/integrity/ima/ima_main.c | 16 +++++++++++++++- security/integrity/integrity.h | 2 ++ 4 files changed, 23 insertions(+), 2 deletions(-)