On Tue, 2023-10-17 at 17:00 -0600, Raul Rangel wrote:
> On Tue, Oct 17, 2023 at 12:27 PM Mimi Zohar <zohar@xxxxxxxxxxxxx> wrote:
> >
> > On Tue, 2023-10-17 at 10:08 -0600, Raul E Rangel wrote:
> > > On Mon, Sep 11, 2023 at 03:38:20PM +0200, Greg Kroah-Hartman wrote:
> > > > 6.4-stable review patch. If anyone has any objections, please let me know.
> > > >
> > > > ------------------
> > > >
> > > > From: Eric Snowberg <eric.snowberg@xxxxxxxxxx>
> > > >
> > > > [ Upstream commit 18b44bc5a67275641fb26f2c54ba7eef80ac5950 ]
> > > >
> > > > Commit db1d1e8b9867 ("IMA: use vfs_getattr_nosec to get the i_version")
> > > > partially closed an IMA integrity issue when directly modifying a file
> > > > on the lower filesystem. If the overlay file is first opened by a user
> > > > and later the lower backing file is modified by root, but the extended
> > > > attribute is NOT updated, the signature validation succeeds with the old
> > > > original signature.
> > > >
> > > > Update the super_block s_iflags to SB_I_IMA_UNVERIFIABLE_SIGNATURE to
> > > > force signature reevaluation on every file access until a fine grained
> > > > solution can be found.
> > > >
> > >
> > > Sorry for replying to the 6.4-stable patch, I couldn't find the original
> > > patch in the mailing list.
> > >
> > > We recently upgraded from 6.4.4 to 6.5.3. We have the integrity LSM
> > > enabled, and are using overlayfs. When we try and execute a binary from
> > > the overlayfs filesystem, the integrity LSM hashes the binary and all
> > > its shared objects every single invocation. This causes a serious
> > > performance regression when invoking clang thousands of times while
> > > building a package. We bisected the culprit down to this patch.
> > >
> > > Here are some numbers:
> > >
> > > With this patch + overlayfs:
> > >
> > > $ time /usr/bin/clang-17 --version > /dev/null
> > >
> > > real 0m0.628s
> > > user 0m0.004s
> > > sys 0m0.624s
> > > $ time /usr/bin/clang-17 --version > /dev/null
> > >
> > > real 0m0.597s
> > > user 0m0.004s
> > > sys 0m0.593s
> > >
> > > With this patch - overlayfs:
> > >
> > > $ truncate -s 1G foo.bin
> > > $ mkfs.ext4 foo.bin
> > > $ mount foo.bin /foo
> > > $ cp /usr/bin/clang-17 /foo
> > > $ time /foo/clang-17 --version > /dev/null
> > >
> > > real 0m0.040s
> > > user 0m0.009s
> > > sys 0m0.031s
> > > $ time /foo/clang-17 --version > /dev/null
> > >
> > > real 0m0.036s
> > > user 0m0.000s
> > > sys 0m0.037s
> > >
> > > Without this path + overlayfs:
> > > $ time /usr/bin/clang-17 --version > /dev/null
> > >
> > > real 0m0.017s
> > > user 0m0.007s
> > > sys 0m0.011s
> > > $ time /usr/bin/clang-17 --version > /dev/null
> > >
> > > real 0m0.018s
> > > user 0m0.000s
> > > sys 0m0.018s
> > >
> > > i.e., we go from ~30ms / invocation to 600ms / invocation. Building
> > > glibc used to take about 3 minutes, but now its taking about 20 minutes.
> > >
> > > Our clang binary is about 100 MiB in size.
> > >
> > > Using `perf` the following sticks out:
> > > $ perf record -g time /usr/bin/clang-17 --version
> > > --92.03%--elf_map
> > > vm_mmap_pgoff
> > > ima_file_mmap
> > > process_measurement
> > > ima_collect_measurement
> > > |
> > > --91.95%--ima_calc_file_hash
> > > ima_calc_file_hash_tfm
> > > |
> > > |--82.85%--_sha256_update
> > > | |
> > > | --82.47%--lib_sha256_base_do_update.isra.0
> > > | |
> > > | --82.39%--sha256_transform_rorx
> > > |
> > > --9.10%--integrity_kernel_read
> > >
> > > The audit.log is also logging every clang invocation as well.
> > >
> > > Was such a large performance regression expected? Can the commit be
> > > reverted until the more fine grained solution mentioned in the commit
> > > message be implemented?
> >
>
> First off, thanks for the quick reply. And I apologize in advance for
> any naive questions. I'm still learning how the IMA system works.
>
> > IMA is always based on policy. Having the "integrity LSM enabled and
> > using overlayfs" will not cause any measurements or signature
> > verifications, unless the files are in policy.
>
> Good point. The policy we have loaded is very similar to the one we
> get from setting `ima_tcb`on the kernel command line. We just remove
> the uid=0 constraint. i.e.,
> ```
> # SECURITYFS_MAGIC
> dont_measure fsmagic=0x73636673
> # SELINUXFS_MAGIC
> dont_measure fsmagic=0xf97cff8c
> ...
The following are new rules:
> # audit files executed.
> audit func=BPRM_CHECK
> # audit executable libraries mmap'd.
> audit func=FILE_MMAP mask=MAY_EXEC
> # audit loaded kernel modules
> audit func=MODULE_CHECK
> ```
>
> We don't have any appraisal rules loaded.
Okay. The appraisal result of the overlay file is being cached and not
cleared on file change of the lower file.
> >
> > The problem is that unless the lower layer file is in policy, file
> > change will not be detected on the overlay filesystem. Reverting this
> > change will allow access to a modified file without re-verifying its
> > integrity.
>
> Given our simple policy, I think the lower layer file is included in the
> policy. So if I understand correctly, you are saying that this patch
> was meant to address the case where the lower layer wasn't
> covered by the policy?
Yes
> >
> > Instead of reverting the patch, perhaps allow users to take this risk
> > by defining a Kconfig, since they're aware of their policy rules.
> >
>
> That sounds good. Or would it make sense to add an option to the
> policy file? i.e., `verifiable fsmagic=0x794c7630
Perhaps instead of introducing a new "action" (measure/dont_measure,
appraise/dont_appraise, audit), it should be more granular at the
policy rule level.
Something like ignore_cache/dont_ignore_cache, depending on the
default.
Eric, does that make sense?
> FWIW, I also added the following to my policy file:
> ```
> # OVERLAYFS_SUPER_MAGIC
> dont_appraise fsmagic=0x794c7630
> dont_measure fsmagic=0x794c7630
> dont_hash fsmagic=0x794c7630
> ```
>
> I didn't get any entries in my audit.log, but the hashing was still
> performed. I figured since tmpfs and ramfs were already marked
> as dont_measure, adding overlayfs shouldn't really be any
> different.
If you're using a modified "ima_tcb" there are "measure" action rules
which would cause files to be re-measured. Look at the IMA measurement
list.
If you're only accessing files via the overlayfs and not the lower
layer, then there wouldn't be any audit records.
Mimi
