[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. 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. Especially, since if files are signed they are signed in the real storage (e.g. xfs) and not in overlayfs. 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. 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. 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. 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. Thanks, Amir.
