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

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

[syzbot]

Hello,

syzbot found the following issue on:

HEAD commit:    97d8894b6f4c Merge tag 'riscv-for-linus-6.12-mw1' of git:/..
git tree:       upstream
console+strace: https://syzkaller.appspot.com/x/log.txt?x=14138a80580000
kernel config:  https://syzkaller.appspot.com/x/.config?x=bc30a30374b0753
dashboard link: https://syzkaller.appspot.com/bug?extid=1cd571a672400ef3a930
compiler:       Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40
syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=118fd2a9980000
C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=1038299f980000

Downloadable assets:
disk image: https://storage.googleapis.com/syzbot-assets/f181c147328d/disk-97d8894b.raw.xz
vmlinux: https://storage.googleapis.com/syzbot-assets/b8b0160d9b09/vmlinux-97d8894b.xz
kernel image: https://storage.googleapis.com/syzbot-assets/c5dab0d4f811/bzImage-97d8894b.xz

The issue was bisected to:

commit ea7e2d5e49c05e5db1922387b09ca74aa40f46e2
Author: Shu Han <ebpqwerty472123@gmail.com>
Date:   Tue Sep 17 09:41:04 2024 +0000

    mm: call the security_mmap_file() LSM hook in remap_file_pages()

bisection log:  https://syzkaller.appspot.com/x/bisect.txt?x=1554a99f980000
final oops:     https://syzkaller.appspot.com/x/report.txt?x=1754a99f980000
console output: https://syzkaller.appspot.com/x/log.txt?x=1354a99f980000

IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+1cd571a672400ef3a930@syzkaller.appspotmail.com
Fixes: ea7e2d5e49c0 ("mm: call the security_mmap_file() LSM hook in remap_file_pages()")

mmap: syz-executor369 (5231) uses deprecated remap_file_pages() syscall. See Documentation/mm/remap_file_pages.rst.
======================================================
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.


the existing dependency chain (in reverse order) is:

-> #1 (&mm->mmap_lock){++++}-{3:3}:
       lock_acquire+0x1ed/0x550 kernel/locking/lockdep.c:5822
       down_read_killable+0xca/0xd30 kernel/locking/rwsem.c:1549
       mmap_read_lock_killable+0x1d/0x70 include/linux/mmap_lock.h:153
       get_mmap_lock_carefully mm/memory.c:6108 [inline]
       lock_mm_and_find_vma+0x29c/0x2f0 mm/memory.c:6159
       do_user_addr_fault arch/x86/mm/fault.c:1361 [inline]
       handle_page_fault arch/x86/mm/fault.c:1481 [inline]
       exc_page_fault+0x1bf/0x8c0 arch/x86/mm/fault.c:1539
       asm_exc_page_fault+0x26/0x30 arch/x86/include/asm/idtentry.h:623
       fault_in_readable+0x108/0x2b0 mm/gup.c:2227
       fault_in_iov_iter_readable+0x229/0x280 lib/iov_iter.c:94
       generic_perform_write+0x259/0x6d0 mm/filemap.c:4040
       shmem_file_write_iter+0xf9/0x120 mm/shmem.c:3221
       new_sync_write fs/read_write.c:590 [inline]
       vfs_write+0xa6d/0xc90 fs/read_write.c:683
       ksys_write+0x183/0x2b0 fs/read_write.c:736
       do_syscall_x64 arch/x86/entry/common.c:52 [inline]
       do_syscall_64+0xf3/0x230 arch/x86/entry/common.c:83
       entry_SYSCALL_64_after_hwframe+0x77/0x7f

-> #0 (&sb->s_type->i_mutex_key#12){+.+.}-{3:3}:
       check_prev_add kernel/locking/lockdep.c:3158 [inline]
       check_prevs_add kernel/locking/lockdep.c:3277 [inline]
       validate_chain+0x18ef/0x5920 kernel/locking/lockdep.c:3901
       __lock_acquire+0x1384/0x2050 kernel/locking/lockdep.c:5199
       lock_acquire+0x1ed/0x550 kernel/locking/lockdep.c:5822
       down_write+0x99/0x220 kernel/locking/rwsem.c:1579
       inode_lock include/linux/fs.h:815 [inline]
       process_measurement+0x439/0x1fb0 security/integrity/ima/ima_main.c:250
       ima_file_mmap+0x13d/0x2b0 security/integrity/ima/ima_main.c:455
       security_mmap_file+0x7e7/0xa40 security/security.c:2977
       __do_sys_remap_file_pages mm/mmap.c:1692 [inline]
       __se_sys_remap_file_pages+0x6e6/0xa50 mm/mmap.c:1624
       do_syscall_x64 arch/x86/entry/common.c:52 [inline]
       do_syscall_64+0xf3/0x230 arch/x86/entry/common.c:83
       entry_SYSCALL_64_after_hwframe+0x77/0x7f

other info that might help us debug this:

 Possible unsafe locking scenario:

       CPU0                    CPU1
       ----                    ----
  lock(&mm->mmap_lock);
                               lock(&sb->s_type->i_mutex_key#12);
                               lock(&mm->mmap_lock);
  lock(&sb->s_type->i_mutex_key#12);

 *** DEADLOCK ***

1 lock held by syz-executor369/5231:
 #0: ffff88807ac9a798 (&mm->mmap_lock){++++}-{3:3}, at: mmap_write_lock_killable include/linux/mmap_lock.h:122 [inline]
 #0: ffff88807ac9a798 (&mm->mmap_lock){++++}-{3:3}, at: __do_sys_remap_file_pages mm/mmap.c:1649 [inline]
 #0: ffff88807ac9a798 (&mm->mmap_lock){++++}-{3:3}, at: __se_sys_remap_file_pages+0x22d/0xa50 mm/mmap.c:1624

stack backtrace:
CPU: 1 UID: 0 PID: 5231 Comm: syz-executor369 Not tainted 6.11.0-syzkaller-10045-g97d8894b6f4c #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 08/06/2024
Call Trace:
 <TASK>
 __dump_stack lib/dump_stack.c:94 [inline]
 dump_stack_lvl+0x241/0x360 lib/dump_stack.c:120
 print_circular_bug+0x13a/0x1b0 kernel/locking/lockdep.c:2074
 check_noncircular+0x36a/0x4a0 kernel/locking/lockdep.c:2203
 check_prev_add kernel/locking/lockdep.c:3158 [inline]
 check_prevs_add kernel/locking/lockdep.c:3277 [inline]
 validate_chain+0x18ef/0x5920 kernel/locking/lockdep.c:3901
 __lock_acquire+0x1384/0x2050 kernel/locking/lockdep.c:5199
 lock_acquire+0x1ed/0x550 kernel/locking/lockdep.c:5822
 down_write+0x99/0x220 kernel/locking/rwsem.c:1579
 inode_lock include/linux/fs.h:815 [inline]
 process_measurement+0x439/0x1fb0 security/integrity/ima/ima_main.c:250
 ima_file_mmap+0x13d/0x2b0 security/integrity/ima/ima_main.c:455
 security_mmap_file+0x7e7/0xa40 security/security.c:2977
 __do_sys_remap_file_pages mm/mmap.c:1692 [inline]
 __se_sys_remap_file_pages+0x6e6/0xa50 mm/mmap.c:1624
 do_syscall_x64 arch/x86/entry/common.c:52 [inline]
 do_syscall_64+0xf3/0x230 arch/x86/entry/common.c:83
 entry_SYSCALL_64_after_hwframe+0x77/0x7f
RIP: 0033:0x7ff317efa919
Code: 28 00 00 00 75 05 48 83 c4 28 c3 e8 e1 18 00 00 90 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 b0 ff ff ff f7 d8 64 89 01 48
RSP: 002b:00007ff317e94238 EFLAGS: 00000246 ORIG_RAX: 00000000000000d8
RAX: ffffffffffffffda RBX: 00007ff317f85318 RCX: 00007ff317efa919
RDX: 0000000000000000 RSI: 0000000000800000 RDI: 0000000020800000
RBP: 00007ff317f85310 R08: 0000000000010000 R09: 00007ff317e946c0
R10: 0000000000000000 R11: 0000000000000246 R12: 00007ff317f8531c
R13: 000000000000006e R14: 00007ffd154a5180 R15: 00007ffd154a5268
 </TASK>


---
This report is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.

syzbot will keep track of this issue. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
For information about bisection process see: https://goo.gl/tpsmEJ#bisection

If the report is already addressed, let syzbot know by replying with:
#syz fix: exact-commit-title

If you want syzbot to run the reproducer, reply with:
#syz test: git://repo/address.git branch-or-commit-hash
If you attach or paste a git patch, syzbot will apply it before testing.

If you want to overwrite report's subsystems, reply with:
#syz set subsystems: new-subsystem
(See the list of subsystem names on the web dashboard)

If the report is a duplicate of another one, reply with:
#syz dup: exact-subject-of-another-report

If you want to undo deduplication, reply with:
#syz undup

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

[Andrew Morton]

On Sat, 28 Sep 2024 00:32:30 -0700 syzbot <syzbot+1cd571a672400ef3a930@syzkaller.appspotmail.com> wrote:

> Hello,
> 
> syzbot found the following issue on:

Thanks.

> HEAD commit:    97d8894b6f4c Merge tag 'riscv-for-linus-6.12-mw1' of git:/..
> git tree:       upstream
> console+strace: https://syzkaller.appspot.com/x/log.txt?x=14138a80580000
> kernel config:  https://syzkaller.appspot.com/x/.config?x=bc30a30374b0753
> dashboard link: https://syzkaller.appspot.com/bug?extid=1cd571a672400ef3a930
> compiler:       Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40
> syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=118fd2a9980000
> C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=1038299f980000
> 
> Downloadable assets:
> disk image: https://storage.googleapis.com/syzbot-assets/f181c147328d/disk-97d8894b.raw.xz
> vmlinux: https://storage.googleapis.com/syzbot-assets/b8b0160d9b09/vmlinux-97d8894b.xz
> kernel image: https://storage.googleapis.com/syzbot-assets/c5dab0d4f811/bzImage-97d8894b.xz
> 
> The issue was bisected to:
> 
> commit ea7e2d5e49c05e5db1922387b09ca74aa40f46e2
> Author: Shu Han <ebpqwerty472123@gmail.com>
> Date:   Tue Sep 17 09:41:04 2024 +0000
> 
>     mm: call the security_mmap_file() LSM hook in remap_file_pages()

That commit has cc:stable.

Can I suggest that you change sysbot so it includes a cc:stable if the
faulty commit had cc:stable?  If Greg agrees that would be useful?

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

[Shu Han]

> ======================================================
> 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.

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.

So, possible solutions may include:
1. Remove the security check by removing the the possibility of the `prot`
   change:
1.1. move the the processing logic for READ_IMPLIES_EXEC out of the
     do_mmap(). This also ensures that such missing checks which the
     previous commit fixes will not occur again(suggested).
     See the RFC PATCH
     https://lore.kernel.org/all/20240928180044.50-1-ebpqwerty472123@gmail.com/
1.2. Replace do_mmap() in remap_file_pages() to mmap_region(), which do
     the actually memory mapping without the respect to READ_IMPLIES_EXEC.
     But this requires other checks in do_mmap() is performed in
     remap_file_pages(), such as the `file_mmap_ok`(may complex).
2. Perform operations similar to updating a value by CAS(may slow):
for (;;) {
    mmap_write_lock();
    prot = get_prot();
    mmap_write_unlock();
    if (!call_lsm(prot)) return;
    mmap_write_lock();
    if (prot != get_prot()) continue;
    do_mmap();
    mmap_write_unlock();
}

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

[Paul Moore]

On Sat, Sep 28, 2024 at 2:08 PM Shu Han <ebpqwerty472123@gmail.com> 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

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

[Shu Han]

> 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.

This is the mistake I made when I first watched the report.

It should be a deadlock.

> 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()?

It would be better if IMA could avoid acqurie inode_lock().

If not, then we may need to consider solutions I mentioned in my
previous reply.

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

[Roberto Sassu]

On Wed, 2024-10-02 at 23:09 -0400, Paul Moore wrote:
> On Sat, Sep 28, 2024 at 2:08 PM Shu Han <ebpqwerty472123@gmail.com> 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.

Roberto

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

[Paul Moore]

On Mon, Oct 7, 2024 at 11:31 AM Roberto Sassu
<roberto.sassu@huaweicloud.com> wrote:
> On Wed, 2024-10-02 at 23:09 -0400, Paul Moore wrote:
> > On Sat, Sep 28, 2024 at 2:08 PM Shu Han <ebpqwerty472123@gmail.com> 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.

-- 
paul-moore.com

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

[Roberto Sassu]

On Mon, 2024-10-07 at 12:35 -0400, Paul Moore wrote:
> On Mon, Oct 7, 2024 at 11:31 AM Roberto Sassu
> <roberto.sassu@huaweicloud.com> wrote:
> > On Wed, 2024-10-02 at 23:09 -0400, Paul Moore wrote:
> > > On Sat, Sep 28, 2024 at 2:08 PM Shu Han <ebpqwerty472123@gmail.com> 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.


Roberto

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

[Paul Moore]

On Mon, Oct 7, 2024 at 12:49 PM Roberto Sassu
<roberto.sassu@huaweicloud.com> wrote:
> On Mon, 2024-10-07 at 12:35 -0400, Paul Moore wrote:
> > On Mon, Oct 7, 2024 at 11:31 AM Roberto Sassu
> > <roberto.sassu@huaweicloud.com> wrote:
> > > On Wed, 2024-10-02 at 23:09 -0400, Paul Moore wrote:
> > > > On Sat, Sep 28, 2024 at 2:08 PM Shu Han <ebpqwerty472123@gmail.com> 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.

-- 
paul-moore.com

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

[Roberto Sassu]

On Mon, 2024-10-07 at 12:58 -0400, Paul Moore wrote:
> On Mon, Oct 7, 2024 at 12:49 PM Roberto Sassu
> <roberto.sassu@huaweicloud.com> wrote:
> > On Mon, 2024-10-07 at 12:35 -0400, Paul Moore wrote:
> > > On Mon, Oct 7, 2024 at 11:31 AM Roberto Sassu
> > > <roberto.sassu@huaweicloud.com> wrote:
> > > > On Wed, 2024-10-02 at 23:09 -0400, Paul Moore wrote:
> > > > > On Sat, Sep 28, 2024 at 2:08 PM Shu Han <ebpqwerty472123@gmail.com> 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

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

[syzbot]

Hello,

syzbot has tested the proposed patch and the reproducer did not trigger any issue:

Reported-by: syzbot+1cd571a672400ef3a930@syzkaller.appspotmail.com
Tested-by: syzbot+1cd571a672400ef3a930@syzkaller.appspotmail.com

Tested on:

commit:         0438fbb6 ima: Mark concurrent accesses to the iint poi..
git tree:       https://github.com/robertosassu/linux.git ima-remove-inode-lock-v1
console output: https://syzkaller.appspot.com/x/log.txt?x=15ead780580000
kernel config:  https://syzkaller.appspot.com/x/.config?x=74c522fa0761706b
dashboard link: https://syzkaller.appspot.com/bug?extid=1cd571a672400ef3a930
compiler:       Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40

Note: no patches were applied.
Note: testing is done by a robot and is best-effort only.

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

[Roberto Sassu]

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
> > <roberto.sassu@huaweicloud.com> wrote:
> > > On Mon, 2024-10-07 at 12:35 -0400, Paul Moore wrote:
> > > > On Mon, Oct 7, 2024 at 11:31 AM Roberto Sassu
> > > > <roberto.sassu@huaweicloud.com> wrote:
> > > > > On Wed, 2024-10-02 at 23:09 -0400, Paul Moore wrote:
> > > > > > On Sat, Sep 28, 2024 at 2:08 PM Shu Han <ebpqwerty472123@gmail.com> 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

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

[syzbot]

Hello,

syzbot has tested the proposed patch and the reproducer did not trigger any issue:

Reported-by: syzbot+1cd571a672400ef3a930@syzkaller.appspotmail.com
Tested-by: syzbot+1cd571a672400ef3a930@syzkaller.appspotmail.com

Tested on:

commit:         31063ab7 mm: Split locks in remap_file_pages()
git tree:       https://github.com/robertosassu/linux.git remap-file-pages-locking-v1
console output: https://syzkaller.appspot.com/x/log.txt?x=15502240580000
kernel config:  https://syzkaller.appspot.com/x/.config?x=5b6f5c91e13aedf5
dashboard link: https://syzkaller.appspot.com/bug?extid=1cd571a672400ef3a930
compiler:       Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40

Note: no patches were applied.
Note: testing is done by a robot and is best-effort only.