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
...
# 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.
>
> 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?
>
> 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`
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.
Thanks again,
Raul
> --
> thanks,
>
> Mimi
>
