Re: [syzbot] [integrity?] [lsm?] possible deadlock in process_measurement (4)

[Paul Moore]

On Sat, Sep 28, 2024 at 2:08 PM Shu Han <ebpqwerty472123@xxxxxxxxx> 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()?

-- 
paul-moore.com