On Wed, Oct 18, 2023 at 9:03 AM Mimi Zohar <zohar@xxxxxxxxxxxxx> wrote:
>
> 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?
I guess if one of the lower layers was a tmpfs that no longer holds.
Can overlayfs determine if the lower file is covered by a policy
before setting the SB_I_IMA_UNVERIFIABLE_SIGNATURE flag? This way the
policy writer doesn't need to get involved with the specifics of how
the overlayfs layers are constructed.
In the original commit message it was mentioned that there was a more
fine grained approach. If that's in the pipeline, maybe it makes sense
to just wait for that instead of adding a new keyword? We just revered
this patch internally to avoid the performance penalty, but we don't
want to carry this patch indefinitely.
>
> > 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.
Oh, I assumed `audit` == `MEASURE`, so I was thinking that
`dont_measure` would negate it. Thanks for that clarification.
>
> If you're only accessing files via the overlayfs and not the lower
> layer, then there wouldn't be any audit records.
>
> Mimi
>
Thanks,
Raul
