On Fri, 2023-09-15 at 12:57 +0300, Amir Goldstein wrote: > [adding back fsdevel] > > On Fri, Sep 15, 2023 at 7:04 AM Mimi Zohar <zohar@xxxxxxxxxxxxx> wrote: > > > > On Fri, 2023-09-15 at 06:21 +0300, Amir Goldstein wrote: > > > On Thu, Sep 14, 2023 at 11:01 PM Mimi Zohar <zohar@xxxxxxxxxxxxx> wrote: > > > > > > > > Hi Amir, Goldwyn, > > > > > > > > FYI, > > > > 1. ima_file_free() is called to check whether the file is in policy. > > > > 2. ima_check_last_writer() is called to determine whether or not the > > > > file hash stored as an xattr needs to be updated. > > > > > > > > 3. As ima_update_xattr() is not being called, I assume there is no > > > > appraise rule. I asked on the thread which policy rules are being used > > > > and for the boot command line, but assume that they're specifying > > > > 'ima_policy=tcb" on the boot command line. > > > > > > Yes, here is the kconfig from syzbot report dashboard > > > https://syzkaller.appspot.com/x/.config?x=df91a3034fe3f122 > > > > > > > > > > > 4. Is commit db1d1e8b9867 ("IMA: use vfs_getattr_nosec to get the > > > > i_version") the problem? > > > > > > IIUC, this commit is responsible for the ovl_getattr() call in the stack > > > trace. syzbot did not bisect the bug yet, but now that it has found > > > a reproducer, it is just a matter of time. > > > However, all the bug reports in the dashboard are only from upstream, > > > so I think that means that this bug was not found on any stable kernels. > > > > > > > > > > > 5. ima_file_free() is being called twice. We should not be seeing > > > > ima_get_current_hash_algo() in the trace. > > > > > > > > [ 66.991195][ T5030] ovl_getattr+0x1b1/0xf70 > > > > [ 66.995635][ T5030] ? ovl_setattr+0x4e0/0x4e0 > > > > [ 67.000229][ T5030] ? trace_raw_output_contention_end+0xd0/0xd0 > > > > [ 67.006387][ T5030] ? rcu_is_watching+0x15/0xb0 > > > > [ 67.011154][ T5030] ? rcu_is_watching+0x15/0xb0 > > > > [ 67.015920][ T5030] ? trace_contention_end+0x3c/0xf0 > > > > [ 67.021122][ T5030] ? __mutex_lock_common+0x42d/0x2530 > > > > [ 67.026506][ T5030] ? lock_release+0xbf/0x9d0 > > > > [ 67.031126][ T5030] ? read_lock_is_recursive+0x20/0x20 > > > > [ 67.036719][ T5030] ? ima_file_free+0x17c/0x4b0 > > > > [ 67.041578][ T5030] ? __lock_acquire+0x7f70/0x7f70 > > > > [ 67.046615][ T5030] ? locks_remove_file+0x429/0x1040 > > > > [ 67.051820][ T5030] ? mutex_lock_io_nested+0x60/0x60 > > > > [ 67.057030][ T5030] ? _raw_spin_unlock+0x40/0x40 > > > > [ 67.061894][ T5030] ? __asan_memset+0x23/0x40 > > > > [ 67.066577][ T5030] ima_file_free+0x26e/0x4b0 > > > > [ 67.071279][ T5030] ? ima_get_current_hash_algo+0x10/0x10 > > > > [ 67.076929][ T5030] ? __rwlock_init+0x150/0x150 > > > > [ 67.081694][ T5030] ? __lock_acquire+0x7f70/0x7f70 > > > > [ 67.086727][ T5030] __fput+0x36a/0x910 > > > > [ 67.090728][ T5030] task_work_run+0x24a/0x300 > > > > > > > > > > > > Were you able to duplicate this locally? > > > > > > > > > > I did not try. Honestly, I don't know how to enable IMA. > > > Is the only thing that I need to do is set the IMA policy > > > in the kernel command line? > > > > > > Does IMA need to be enabled per fs? per sb? > > > > > > If so, I can run the overlay test suite with IMA enabled and > > > see what happens. > > > > Yes, you'll definitely will be able to see the measurement list. > > > > [Setting up the system to verify file signatures is a bit more > > difficult: files need to be signed, keys need to be loaded. Finally > > CentOS and RHEL 9.3 will have file signatures and will publish the IMA > > code signing key.] > > > > Assuming IMA is configured, just add "ima_policy=tcb" to the command > > line. This will measure all files executed, mmap'ed, kernel modules, > > firmware, and all files opened by root. Normally the builtin policy is > > replaced with a finer grained one. > > > > Below are a few commands, but Ken Goldman is writing documentation - > > https://ima-doc.readthedocs.io/en/latest/ > > > > 1. Display the IMA measurement list: > > # cat /sys/kernel/security/ima/ascii_runtime_measurements > > # cat /sys/kernel/security/ima/binary_runtime_measurements > > > > 2. Display the IMA policy (or append to the policy) > > # cat /sys/kernel/security/ima/policy > > > > 3. Display number of measurements > > # cat /sys/kernel/security/ima/runtime_measurements_count > > > > Nice. > This seems to work fine and nothing pops up when running > fstests unionmount tests of overlayfs over xfs. > > What strikes me as strange is that there are measurements > of files in xfs and in overlayfs, but no measurements of files in tmpfs. tmpfs is excluded from policy, since there is no way of storing the file signature in the initramfs (CPIO). There have been a number of attempts of extending the initramfs CPIO format. As you know Al's reasons for not using some other format persist today. "cat /sys/kernel/security/ima/policy" will list the current policy. The rules is based on the fsmagic labels. The builtin policy can be replaced with a custom policy. >From include/uapi/linux/magic.h: #define TMPFS_MAGIC 0x01021994 > I suppose that is because no one can tamper with the storage > of tmpfs, but following the same logic, nobody can tamper with > the storage of overlayfs files without tampering with storage of > underlying fs (e.g. xfs), so measuring overlayfs files should not > bring any extra security to the system. Sorry, files, especially random name files, can be stored on tmpfs and do need to be measured and appraised. > Especially, since if files are signed they are signed in the real > storage (e.g. xfs) and not in overlayfs. IMA-appraisal needs to prevent executing files that aren't properly signed no matter the filesystem. We can't just ignore the upper filesystem. What happens if file metadata - ower, group, modes - changes. Is the file data and metadata copied up to the overlay? If the file data remains the same, then the file signature should still verify. So it isn't as simple as saying the upper layer shouldn't ever be verified. > So in theory, we should never ever measure files in the > "virtual" overlayfs and only measure them in the real fs. > The only problem is the the IMA hooks when executing, > mmaping, reading files from overlayfs, don't work on the real fs. Right, if the file data changed, then signature verification would fail with the existing signature. And in most situations that is exactly what is desired. It's possible someone would want to sign files on upper layer with their own key, but let's ignore that use case scenario for now. > fsnotify also was not working correctly in that respect, because > fs operations on overlayfs did not always trigger fsnotify events > on the underlying real fs. Yes, that will probably address detecting file change on the real inode when accessing the lower overlay. Not sure that it will address the upper overlay issues. > This was fixed in 6.5 by commit bc2473c90fca ("ovl: enable fsnotify > events on underlying real files") and the file_real_path() infrastructure > was added to enable this. Ok, will take a closer look later. > This is why I say, that in most likelihood, IMA hook should always use > file_real_path() and file_dentry() to perform the measurements > and record the path of the real fs when overlayfs is performing the > actual read/mmap on the real fs and IMA hooks should ideally > do nothing at all (as in tmpfs) when the operation is performed > on the "virtual" overlayfs object. An IMA policy rule could be defined to ignore the upper layer, but it can't be the default. If the policy is always measure and appraise executables and mmap'ed libraries regardless of the filesystem, then we can't just ignore the upper layer. Shannah tova! Mimi
