On Tue, 2023-10-17 at 17:00 -0600, Raul Rangel wrote:
On Tue, Oct 17, 2023 at 12:27 PM Mimi Zohar zohar@linux.ibm.com 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@oracle.com
[ 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.593sWith 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.037sWithout 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.018si.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=0x794c7630I 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