On Wed, 2024-10-09 at 18:23 +0200, Roberto Sassu wrote: > On Mon, 2024-10-07 at 12:58 -0400, Paul Moore wrote: > > On Mon, Oct 7, 2024 at 12:49 PM Roberto Sassu > > <[email protected]> wrote: > > > On Mon, 2024-10-07 at 12:35 -0400, Paul Moore wrote: > > > > On Mon, Oct 7, 2024 at 11:31 AM Roberto Sassu > > > > <[email protected]> wrote: > > > > > On Wed, 2024-10-02 at 23:09 -0400, Paul Moore wrote: > > > > > > On Sat, Sep 28, 2024 at 2:08 PM Shu Han <[email protected]> > > > > > > wrote: > > > > > > > > > > > > > > > ====================================================== > > > > > > > > WARNING: possible circular locking dependency detected > > > > > > > > 6.11.0-syzkaller-10045-g97d8894b6f4c #0 Not tainted > > > > > > > > ------------------------------------------------------ > > > > > > > > syz-executor369/5231 is trying to acquire lock: > > > > > > > > ffff888072852370 (&sb->s_type->i_mutex_key#12){+.+.}-{3:3}, at: > > > > > > > > inode_lock include/linux/fs.h:815 [inline] > > > > > > > > ffff888072852370 (&sb->s_type->i_mutex_key#12){+.+.}-{3:3}, at: > > > > > > > > process_measurement+0x439/0x1fb0 > > > > > > > > security/integrity/ima/ima_main.c:250 > > > > > > > > > > > > > > > > but task is already holding lock: > > > > > > > > ffff88807ac9a798 (&mm->mmap_lock){++++}-{3:3}, at: > > > > > > > > mmap_write_lock_killable include/linux/mmap_lock.h:122 [inline] > > > > > > > > ffff88807ac9a798 (&mm->mmap_lock){++++}-{3:3}, at: > > > > > > > > __do_sys_remap_file_pages mm/mmap.c:1649 [inline] > > > > > > > > ffff88807ac9a798 (&mm->mmap_lock){++++}-{3:3}, at: > > > > > > > > __se_sys_remap_file_pages+0x22d/0xa50 mm/mmap.c:1624 > > > > > > > > > > > > > > > > which lock already depends on the new lock. > > > > > > > > > > > > > > This issue (if not a false positive?) is due to the possible > > > > > > > `prot` > > > > > > > change caused by the processing logic for READ_IMPLIES_EXEC in > > > > > > > do_mmap(), > > > > > > > so the remap_file_pages() must perform LSM check before calling > > > > > > > do_mmap(), > > > > > > > this is what the previous commit want to do. > > > > > > > > > > > > My apologies for the delay on this, I was traveling for a bit and > > > > > > missed this issue while away. > > > > > > > > > > > > Looking quickly at the report, I don't believe this is a false > > > > > > positive. > > > > > > > > > > > > > The LSM check is required to know what the `prot` is, but `prot` > > > > > > > must be > > > > > > > obtained after holding the `mmap_write_lock`. > > > > > > > > > > > > > > If the `mmap_write_lock` is released after getting the `prot` and > > > > > > > before > > > > > > > the LSM call in remap_file_pages(), it may cause TOCTOU. > > > > > > > > > > > > Looking at the IMA code, specifically the process_measurement() > > > > > > function which is called from the security_mmap_file() LSM hook, I'm > > > > > > not sure why there is the inode_lock() protected region. Mimi? > > > > > > Roberto? My best guess is that locking the inode may have been > > > > > > necessary before we moved the IMA inode state into the inode's LSM > > > > > > security blob, but I'm not certain. > > > > > > > > > > > > Mimi and Roberto, can we safely remove the inode locking in > > > > > > process_measurement()? > > > > > > > > > > I discussed a bit with Mimi. Her concern was the duplicate iint > > > > > structure creation during concurrent file accesses. Now that inode > > > > > integrity metadata have been moved to the inode security blob, we can > > > > > take the iint->mutex out of the ima_iint_cache structure, and store it > > > > > directly in the security blob. In this way, we can remove the inode > > > > > lock. > > > > > > > > > > Will write a patch and see if it passes our tests. > > > > > > > > That's great, thanks Roberto. Assuming all goes well we'll want to > > > > backport this everywhere we merged the remap_file_pages() patch. > > > > > > Welcome. Probably it can go down only until the kernel where IMA and > > > EVM are LSMs. > > > > Yes, we'll need to look at that once we solve this in Linus' tree. > > #syz test: https://github.com/robertosassu/linux.git ima-remove-inode-lock-v1
#syz test: https://github.com/robertosassu/linux.git remap-file-pages-locking-v1
