All of lore.kernel.org
 help / color / mirror / Atom feed
* [syzbot] [lsm?] general protection fault in hook_inode_free_security
@ 2024-05-08 19:32 syzbot
  2024-05-10  0:01 ` Paul Moore
  0 siblings, 1 reply; 24+ messages in thread
From: syzbot @ 2024-05-08 19:32 UTC (permalink / raw)
  To: jmorris, linux-kernel, linux-security-module, mic, paul, serge,
	syzkaller-bugs

Hello,

syzbot found the following issue on:

HEAD commit:    dccb07f2914c Merge tag 'for-6.9-rc7-tag' of git://git.kern..
git tree:       upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=14a46760980000
kernel config:  https://syzkaller.appspot.com/x/.config?x=6d14c12b661fb43
dashboard link: https://syzkaller.appspot.com/bug?extid=5446fbf332b0602ede0b
compiler:       Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40

Unfortunately, I don't have any reproducer for this issue yet.

Downloadable assets:
disk image: https://storage.googleapis.com/syzbot-assets/39d66018d8ad/disk-dccb07f2.raw.xz
vmlinux: https://storage.googleapis.com/syzbot-assets/c160b651d1bc/vmlinux-dccb07f2.xz
kernel image: https://storage.googleapis.com/syzbot-assets/3662a33ac713/bzImage-dccb07f2.xz

IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+5446fbf332b0602ede0b@syzkaller.appspotmail.com

general protection fault, probably for non-canonical address 0xdffffc018f62f515: 0000 [#1] PREEMPT SMP KASAN NOPTI
KASAN: probably user-memory-access in range [0x0000000c7b17a8a8-0x0000000c7b17a8af]
CPU: 1 PID: 5102 Comm: syz-executor.1 Not tainted 6.9.0-rc7-syzkaller-00012-gdccb07f2914c #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 04/02/2024
RIP: 0010:hook_inode_free_security+0x5b/0xb0 security/landlock/fs.c:1047
Code: 8a fd 48 8b 1b 48 c7 c0 c4 4e d5 8d 48 c1 e8 03 42 0f b6 04 30 84 c0 75 3e 48 63 05 33 59 65 09 48 01 c3 48 89 d8 48 c1 e8 03 <42> 80 3c 30 00 74 08 48 89 df e8 66 be 8a fd 48 83 3b 00 75 0d e8
RSP: 0018:ffffc9000307f9a8 EFLAGS: 00010212
RAX: 000000018f62f515 RBX: 0000000c7b17a8a8 RCX: ffff888027668000
RDX: 0000000000000000 RSI: 0000000000000040 RDI: ffff88805c0bb270
RBP: ffffffff8c01fb00 R08: ffffffff82132a15 R09: 1ffff1100b81765f
R10: dffffc0000000000 R11: ffffffff846ff540 R12: dffffc0000000000
R13: 1ffff1100b817683 R14: dffffc0000000000 R15: dffffc0000000000
FS:  0000000000000000(0000) GS:ffff8880b9500000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007f43c42de000 CR3: 00000000635f8000 CR4: 0000000000350ef0
Call Trace:
 <TASK>
 security_inode_free+0x4a/0xd0 security/security.c:1613
 __destroy_inode+0x2d9/0x650 fs/inode.c:286
 destroy_inode fs/inode.c:309 [inline]
 evict+0x521/0x630 fs/inode.c:682
 dispose_list fs/inode.c:700 [inline]
 evict_inodes+0x5f9/0x690 fs/inode.c:750
 generic_shutdown_super+0x9d/0x2d0 fs/super.c:626
 kill_block_super+0x44/0x90 fs/super.c:1675
 deactivate_locked_super+0xc6/0x130 fs/super.c:472
 cleanup_mnt+0x426/0x4c0 fs/namespace.c:1267
 task_work_run+0x251/0x310 kernel/task_work.c:180
 exit_task_work include/linux/task_work.h:38 [inline]
 do_exit+0xa1b/0x27e0 kernel/exit.c:878
 do_group_exit+0x207/0x2c0 kernel/exit.c:1027
 __do_sys_exit_group kernel/exit.c:1038 [inline]
 __se_sys_exit_group kernel/exit.c:1036 [inline]
 __x64_sys_exit_group+0x3f/0x40 kernel/exit.c:1036
 do_syscall_x64 arch/x86/entry/common.c:52 [inline]
 do_syscall_64+0xf5/0x240 arch/x86/entry/common.c:83
 entry_SYSCALL_64_after_hwframe+0x77/0x7f
RIP: 0033:0x7f731567dd69
Code: Unable to access opcode bytes at 0x7f731567dd3f.
RSP: 002b:00007fff4f0804d8 EFLAGS: 00000246 ORIG_RAX: 00000000000000e7
RAX: ffffffffffffffda RBX: 00007f73156c93a3 RCX: 00007f731567dd69
RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000000
RBP: 0000000000000002 R08: 00007fff4f07e277 R09: 00007fff4f081790
R10: 0000000000000000 R11: 0000000000000246 R12: 00007fff4f081790
R13: 00007f73156c937e R14: 00000000000154d0 R15: 000000000000001e
 </TASK>
Modules linked in:
---[ end trace 0000000000000000 ]---
RIP: 0010:hook_inode_free_security+0x5b/0xb0 security/landlock/fs.c:1047
Code: 8a fd 48 8b 1b 48 c7 c0 c4 4e d5 8d 48 c1 e8 03 42 0f b6 04 30 84 c0 75 3e 48 63 05 33 59 65 09 48 01 c3 48 89 d8 48 c1 e8 03 <42> 80 3c 30 00 74 08 48 89 df e8 66 be 8a fd 48 83 3b 00 75 0d e8
RSP: 0018:ffffc9000307f9a8 EFLAGS: 00010212
RAX: 000000018f62f515 RBX: 0000000c7b17a8a8 RCX: ffff888027668000
RDX: 0000000000000000 RSI: 0000000000000040 RDI: ffff88805c0bb270
RBP: ffffffff8c01fb00 R08: ffffffff82132a15 R09: 1ffff1100b81765f
R10: dffffc0000000000 R11: ffffffff846ff540 R12: dffffc0000000000
R13: 1ffff1100b817683 R14: dffffc0000000000 R15: dffffc0000000000
FS:  0000000000000000(0000) GS:ffff8880b9400000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000555587f03978 CR3: 0000000049876000 CR4: 0000000000350ef0
----------------
Code disassembly (best guess):
   0:	8a fd                	mov    %ch,%bh
   2:	48 8b 1b             	mov    (%rbx),%rbx
   5:	48 c7 c0 c4 4e d5 8d 	mov    $0xffffffff8dd54ec4,%rax
   c:	48 c1 e8 03          	shr    $0x3,%rax
  10:	42 0f b6 04 30       	movzbl (%rax,%r14,1),%eax
  15:	84 c0                	test   %al,%al
  17:	75 3e                	jne    0x57
  19:	48 63 05 33 59 65 09 	movslq 0x9655933(%rip),%rax        # 0x9655953
  20:	48 01 c3             	add    %rax,%rbx
  23:	48 89 d8             	mov    %rbx,%rax
  26:	48 c1 e8 03          	shr    $0x3,%rax
* 2a:	42 80 3c 30 00       	cmpb   $0x0,(%rax,%r14,1) <-- trapping instruction
  2f:	74 08                	je     0x39
  31:	48 89 df             	mov    %rbx,%rdi
  34:	e8 66 be 8a fd       	call   0xfd8abe9f
  39:	48 83 3b 00          	cmpq   $0x0,(%rbx)
  3d:	75 0d                	jne    0x4c
  3f:	e8                   	.byte 0xe8


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

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

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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [syzbot] [lsm?] general protection fault in hook_inode_free_security
  2024-05-08 19:32 [syzbot] [lsm?] general protection fault in hook_inode_free_security syzbot
@ 2024-05-10  0:01 ` Paul Moore
  2024-05-15 15:12   ` Mickaël Salaün
  0 siblings, 1 reply; 24+ messages in thread
From: Paul Moore @ 2024-05-10  0:01 UTC (permalink / raw)
  To: syzbot
  Cc: jmorris, linux-kernel, linux-security-module, mic, serge, syzkaller-bugs

On Wed, May 8, 2024 at 3:32 PM syzbot
<syzbot+5446fbf332b0602ede0b@syzkaller.appspotmail.com> wrote:
>
> Hello,
>
> syzbot found the following issue on:
>
> HEAD commit:    dccb07f2914c Merge tag 'for-6.9-rc7-tag' of git://git.kern..
> git tree:       upstream
> console output: https://syzkaller.appspot.com/x/log.txt?x=14a46760980000
> kernel config:  https://syzkaller.appspot.com/x/.config?x=6d14c12b661fb43
> dashboard link: https://syzkaller.appspot.com/bug?extid=5446fbf332b0602ede0b
> compiler:       Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40
>
> Unfortunately, I don't have any reproducer for this issue yet.
>
> Downloadable assets:
> disk image: https://storage.googleapis.com/syzbot-assets/39d66018d8ad/disk-dccb07f2.raw.xz
> vmlinux: https://storage.googleapis.com/syzbot-assets/c160b651d1bc/vmlinux-dccb07f2.xz
> kernel image: https://storage.googleapis.com/syzbot-assets/3662a33ac713/bzImage-dccb07f2.xz
>
> IMPORTANT: if you fix the issue, please add the following tag to the commit:
> Reported-by: syzbot+5446fbf332b0602ede0b@syzkaller.appspotmail.com
>
> general protection fault, probably for non-canonical address 0xdffffc018f62f515: 0000 [#1] PREEMPT SMP KASAN NOPTI
> KASAN: probably user-memory-access in range [0x0000000c7b17a8a8-0x0000000c7b17a8af]
> CPU: 1 PID: 5102 Comm: syz-executor.1 Not tainted 6.9.0-rc7-syzkaller-00012-gdccb07f2914c #0
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 04/02/2024
> RIP: 0010:hook_inode_free_security+0x5b/0xb0 security/landlock/fs.c:1047

Possibly a Landlock issue, Mickaël?

> Code: 8a fd 48 8b 1b 48 c7 c0 c4 4e d5 8d 48 c1 e8 03 42 0f b6 04 30 84 c0 75 3e 48 63 05 33 59 65 09 48 01 c3 48 89 d8 48 c1 e8 03 <42> 80 3c 30 00 74 08 48 89 df e8 66 be 8a fd 48 83 3b 00 75 0d e8
> RSP: 0018:ffffc9000307f9a8 EFLAGS: 00010212
> RAX: 000000018f62f515 RBX: 0000000c7b17a8a8 RCX: ffff888027668000
> RDX: 0000000000000000 RSI: 0000000000000040 RDI: ffff88805c0bb270
> RBP: ffffffff8c01fb00 R08: ffffffff82132a15 R09: 1ffff1100b81765f
> R10: dffffc0000000000 R11: ffffffff846ff540 R12: dffffc0000000000
> R13: 1ffff1100b817683 R14: dffffc0000000000 R15: dffffc0000000000
> FS:  0000000000000000(0000) GS:ffff8880b9500000(0000) knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 00007f43c42de000 CR3: 00000000635f8000 CR4: 0000000000350ef0
> Call Trace:
>  <TASK>
>  security_inode_free+0x4a/0xd0 security/security.c:1613
>  __destroy_inode+0x2d9/0x650 fs/inode.c:286
>  destroy_inode fs/inode.c:309 [inline]
>  evict+0x521/0x630 fs/inode.c:682
>  dispose_list fs/inode.c:700 [inline]
>  evict_inodes+0x5f9/0x690 fs/inode.c:750
>  generic_shutdown_super+0x9d/0x2d0 fs/super.c:626
>  kill_block_super+0x44/0x90 fs/super.c:1675
>  deactivate_locked_super+0xc6/0x130 fs/super.c:472
>  cleanup_mnt+0x426/0x4c0 fs/namespace.c:1267
>  task_work_run+0x251/0x310 kernel/task_work.c:180
>  exit_task_work include/linux/task_work.h:38 [inline]
>  do_exit+0xa1b/0x27e0 kernel/exit.c:878
>  do_group_exit+0x207/0x2c0 kernel/exit.c:1027
>  __do_sys_exit_group kernel/exit.c:1038 [inline]
>  __se_sys_exit_group kernel/exit.c:1036 [inline]
>  __x64_sys_exit_group+0x3f/0x40 kernel/exit.c:1036
>  do_syscall_x64 arch/x86/entry/common.c:52 [inline]
>  do_syscall_64+0xf5/0x240 arch/x86/entry/common.c:83
>  entry_SYSCALL_64_after_hwframe+0x77/0x7f
> RIP: 0033:0x7f731567dd69
> Code: Unable to access opcode bytes at 0x7f731567dd3f.
> RSP: 002b:00007fff4f0804d8 EFLAGS: 00000246 ORIG_RAX: 00000000000000e7
> RAX: ffffffffffffffda RBX: 00007f73156c93a3 RCX: 00007f731567dd69
> RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000000
> RBP: 0000000000000002 R08: 00007fff4f07e277 R09: 00007fff4f081790
> R10: 0000000000000000 R11: 0000000000000246 R12: 00007fff4f081790
> R13: 00007f73156c937e R14: 00000000000154d0 R15: 000000000000001e
>  </TASK>
> Modules linked in:
> ---[ end trace 0000000000000000 ]---

-- 
paul-moore.com

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [syzbot] [lsm?] general protection fault in hook_inode_free_security
  2024-05-10  0:01 ` Paul Moore
@ 2024-05-15 15:12   ` Mickaël Salaün
  2024-05-16  7:31     ` Mickaël Salaün
  0 siblings, 1 reply; 24+ messages in thread
From: Mickaël Salaün @ 2024-05-15 15:12 UTC (permalink / raw)
  To: Paul Moore, Jann Horn
  Cc: syzbot, jmorris, linux-kernel, linux-security-module, serge,
	syzkaller-bugs, linux-fsdevel, Casey Schaufler,
	Christian Brauner

On Thu, May 09, 2024 at 08:01:49PM -0400, Paul Moore wrote:
> On Wed, May 8, 2024 at 3:32 PM syzbot
> <syzbot+5446fbf332b0602ede0b@syzkaller.appspotmail.com> wrote:
> >
> > Hello,
> >
> > syzbot found the following issue on:
> >
> > HEAD commit:    dccb07f2914c Merge tag 'for-6.9-rc7-tag' of git://git.kern..
> > git tree:       upstream
> > console output: https://syzkaller.appspot.com/x/log.txt?x=14a46760980000
> > kernel config:  https://syzkaller.appspot.com/x/.config?x=6d14c12b661fb43
> > dashboard link: https://syzkaller.appspot.com/bug?extid=5446fbf332b0602ede0b
> > compiler:       Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40
> >
> > Unfortunately, I don't have any reproducer for this issue yet.
> >
> > Downloadable assets:
> > disk image: https://storage.googleapis.com/syzbot-assets/39d66018d8ad/disk-dccb07f2.raw.xz
> > vmlinux: https://storage.googleapis.com/syzbot-assets/c160b651d1bc/vmlinux-dccb07f2.xz
> > kernel image: https://storage.googleapis.com/syzbot-assets/3662a33ac713/bzImage-dccb07f2.xz
> >
> > IMPORTANT: if you fix the issue, please add the following tag to the commit:
> > Reported-by: syzbot+5446fbf332b0602ede0b@syzkaller.appspotmail.com
> >
> > general protection fault, probably for non-canonical address 0xdffffc018f62f515: 0000 [#1] PREEMPT SMP KASAN NOPTI
> > KASAN: probably user-memory-access in range [0x0000000c7b17a8a8-0x0000000c7b17a8af]
> > CPU: 1 PID: 5102 Comm: syz-executor.1 Not tainted 6.9.0-rc7-syzkaller-00012-gdccb07f2914c #0
> > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 04/02/2024
> > RIP: 0010:hook_inode_free_security+0x5b/0xb0 security/landlock/fs.c:1047
> 
> Possibly a Landlock issue, Mickaël?

It looks like security_inode_free() is called two times on the same
inode.  This could happen if an inode labeled by Landlock is put
concurrently with release_inode() for a closed ruleset or with
hook_sb_delete().  I didn't find any race condition that could lead to
two calls to iput() though.  Could WRITE_ONCE(object->underobj, NULL)
change anything even if object->lock is locked?

A bit unrelated but looking at the SELinux code, I see that selinux_inode()
checks `!inode->i_security`.  In which case could this happen?

> 
> > Code: 8a fd 48 8b 1b 48 c7 c0 c4 4e d5 8d 48 c1 e8 03 42 0f b6 04 30 84 c0 75 3e 48 63 05 33 59 65 09 48 01 c3 48 89 d8 48 c1 e8 03 <42> 80 3c 30 00 74 08 48 89 df e8 66 be 8a fd 48 83 3b 00 75 0d e8
> > RSP: 0018:ffffc9000307f9a8 EFLAGS: 00010212
> > RAX: 000000018f62f515 RBX: 0000000c7b17a8a8 RCX: ffff888027668000
> > RDX: 0000000000000000 RSI: 0000000000000040 RDI: ffff88805c0bb270
> > RBP: ffffffff8c01fb00 R08: ffffffff82132a15 R09: 1ffff1100b81765f
> > R10: dffffc0000000000 R11: ffffffff846ff540 R12: dffffc0000000000
> > R13: 1ffff1100b817683 R14: dffffc0000000000 R15: dffffc0000000000
> > FS:  0000000000000000(0000) GS:ffff8880b9500000(0000) knlGS:0000000000000000
> > CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > CR2: 00007f43c42de000 CR3: 00000000635f8000 CR4: 0000000000350ef0
> > Call Trace:
> >  <TASK>
> >  security_inode_free+0x4a/0xd0 security/security.c:1613
> >  __destroy_inode+0x2d9/0x650 fs/inode.c:286
> >  destroy_inode fs/inode.c:309 [inline]
> >  evict+0x521/0x630 fs/inode.c:682
> >  dispose_list fs/inode.c:700 [inline]
> >  evict_inodes+0x5f9/0x690 fs/inode.c:750
> >  generic_shutdown_super+0x9d/0x2d0 fs/super.c:626
> >  kill_block_super+0x44/0x90 fs/super.c:1675
> >  deactivate_locked_super+0xc6/0x130 fs/super.c:472
> >  cleanup_mnt+0x426/0x4c0 fs/namespace.c:1267
> >  task_work_run+0x251/0x310 kernel/task_work.c:180
> >  exit_task_work include/linux/task_work.h:38 [inline]
> >  do_exit+0xa1b/0x27e0 kernel/exit.c:878
> >  do_group_exit+0x207/0x2c0 kernel/exit.c:1027
> >  __do_sys_exit_group kernel/exit.c:1038 [inline]
> >  __se_sys_exit_group kernel/exit.c:1036 [inline]
> >  __x64_sys_exit_group+0x3f/0x40 kernel/exit.c:1036
> >  do_syscall_x64 arch/x86/entry/common.c:52 [inline]
> >  do_syscall_64+0xf5/0x240 arch/x86/entry/common.c:83
> >  entry_SYSCALL_64_after_hwframe+0x77/0x7f
> > RIP: 0033:0x7f731567dd69
> > Code: Unable to access opcode bytes at 0x7f731567dd3f.
> > RSP: 002b:00007fff4f0804d8 EFLAGS: 00000246 ORIG_RAX: 00000000000000e7
> > RAX: ffffffffffffffda RBX: 00007f73156c93a3 RCX: 00007f731567dd69
> > RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000000
> > RBP: 0000000000000002 R08: 00007fff4f07e277 R09: 00007fff4f081790
> > R10: 0000000000000000 R11: 0000000000000246 R12: 00007fff4f081790
> > R13: 00007f73156c937e R14: 00000000000154d0 R15: 000000000000001e
> >  </TASK>
> > Modules linked in:
> > ---[ end trace 0000000000000000 ]---
> 
> -- 
> paul-moore.com
> 

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [syzbot] [lsm?] general protection fault in hook_inode_free_security
  2024-05-15 15:12   ` Mickaël Salaün
@ 2024-05-16  7:31     ` Mickaël Salaün
  2024-05-16 13:07       ` Mathieu Desnoyers
  2024-06-27 13:34       ` Mickaël Salaün
  0 siblings, 2 replies; 24+ messages in thread
From: Mickaël Salaün @ 2024-05-16  7:31 UTC (permalink / raw)
  To: Paul Moore, Jann Horn, Mathieu Desnoyers, Paul E. McKenney
  Cc: syzbot, jmorris, linux-kernel, linux-security-module, serge,
	syzkaller-bugs, linux-fsdevel, Casey Schaufler,
	Christian Brauner

Adding membarrier experts.

On Wed, May 15, 2024 at 05:12:58PM +0200, Mickaël Salaün wrote:
> On Thu, May 09, 2024 at 08:01:49PM -0400, Paul Moore wrote:
> > On Wed, May 8, 2024 at 3:32 PM syzbot
> > <syzbot+5446fbf332b0602ede0b@syzkaller.appspotmail.com> wrote:
> > >
> > > Hello,
> > >
> > > syzbot found the following issue on:
> > >
> > > HEAD commit:    dccb07f2914c Merge tag 'for-6.9-rc7-tag' of git://git.kern..
> > > git tree:       upstream
> > > console output: https://syzkaller.appspot.com/x/log.txt?x=14a46760980000
> > > kernel config:  https://syzkaller.appspot.com/x/.config?x=6d14c12b661fb43
> > > dashboard link: https://syzkaller.appspot.com/bug?extid=5446fbf332b0602ede0b
> > > compiler:       Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40
> > >
> > > Unfortunately, I don't have any reproducer for this issue yet.
> > >
> > > Downloadable assets:
> > > disk image: https://storage.googleapis.com/syzbot-assets/39d66018d8ad/disk-dccb07f2.raw.xz
> > > vmlinux: https://storage.googleapis.com/syzbot-assets/c160b651d1bc/vmlinux-dccb07f2.xz
> > > kernel image: https://storage.googleapis.com/syzbot-assets/3662a33ac713/bzImage-dccb07f2.xz
> > >
> > > IMPORTANT: if you fix the issue, please add the following tag to the commit:
> > > Reported-by: syzbot+5446fbf332b0602ede0b@syzkaller.appspotmail.com
> > >
> > > general protection fault, probably for non-canonical address 0xdffffc018f62f515: 0000 [#1] PREEMPT SMP KASAN NOPTI
> > > KASAN: probably user-memory-access in range [0x0000000c7b17a8a8-0x0000000c7b17a8af]
> > > CPU: 1 PID: 5102 Comm: syz-executor.1 Not tainted 6.9.0-rc7-syzkaller-00012-gdccb07f2914c #0
> > > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 04/02/2024
> > > RIP: 0010:hook_inode_free_security+0x5b/0xb0 security/landlock/fs.c:1047
> > 
> > Possibly a Landlock issue, Mickaël?
> 
> It looks like security_inode_free() is called two times on the same
> inode.  This could happen if an inode labeled by Landlock is put
> concurrently with release_inode() for a closed ruleset or with
> hook_sb_delete().  I didn't find any race condition that could lead to
> two calls to iput() though.  Could WRITE_ONCE(object->underobj, NULL)
> change anything even if object->lock is locked?
> 
> A bit unrelated but looking at the SELinux code, I see that selinux_inode()
> checks `!inode->i_security`.  In which case could this happen?
> 
> > 
> > > Code: 8a fd 48 8b 1b 48 c7 c0 c4 4e d5 8d 48 c1 e8 03 42 0f b6 04 30 84 c0 75 3e 48 63 05 33 59 65 09 48 01 c3 48 89 d8 48 c1 e8 03 <42> 80 3c 30 00 74 08 48 89 df e8 66 be 8a fd 48 83 3b 00 75 0d e8
> > > RSP: 0018:ffffc9000307f9a8 EFLAGS: 00010212
> > > RAX: 000000018f62f515 RBX: 0000000c7b17a8a8 RCX: ffff888027668000
> > > RDX: 0000000000000000 RSI: 0000000000000040 RDI: ffff88805c0bb270
> > > RBP: ffffffff8c01fb00 R08: ffffffff82132a15 R09: 1ffff1100b81765f
> > > R10: dffffc0000000000 R11: ffffffff846ff540 R12: dffffc0000000000
> > > R13: 1ffff1100b817683 R14: dffffc0000000000 R15: dffffc0000000000
> > > FS:  0000000000000000(0000) GS:ffff8880b9500000(0000) knlGS:0000000000000000
> > > CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > > CR2: 00007f43c42de000 CR3: 00000000635f8000 CR4: 0000000000350ef0
> > > Call Trace:
> > >  <TASK>
> > >  security_inode_free+0x4a/0xd0 security/security.c:1613
> > >  __destroy_inode+0x2d9/0x650 fs/inode.c:286
> > >  destroy_inode fs/inode.c:309 [inline]
> > >  evict+0x521/0x630 fs/inode.c:682
> > >  dispose_list fs/inode.c:700 [inline]
> > >  evict_inodes+0x5f9/0x690 fs/inode.c:750
> > >  generic_shutdown_super+0x9d/0x2d0 fs/super.c:626
> > >  kill_block_super+0x44/0x90 fs/super.c:1675
> > >  deactivate_locked_super+0xc6/0x130 fs/super.c:472
> > >  cleanup_mnt+0x426/0x4c0 fs/namespace.c:1267
> > >  task_work_run+0x251/0x310 kernel/task_work.c:180
> > >  exit_task_work include/linux/task_work.h:38 [inline]
> > >  do_exit+0xa1b/0x27e0 kernel/exit.c:878
> > >  do_group_exit+0x207/0x2c0 kernel/exit.c:1027
> > >  __do_sys_exit_group kernel/exit.c:1038 [inline]
> > >  __se_sys_exit_group kernel/exit.c:1036 [inline]
> > >  __x64_sys_exit_group+0x3f/0x40 kernel/exit.c:1036
> > >  do_syscall_x64 arch/x86/entry/common.c:52 [inline]
> > >  do_syscall_64+0xf5/0x240 arch/x86/entry/common.c:83
> > >  entry_SYSCALL_64_after_hwframe+0x77/0x7f
> > > RIP: 0033:0x7f731567dd69
> > > Code: Unable to access opcode bytes at 0x7f731567dd3f.
> > > RSP: 002b:00007fff4f0804d8 EFLAGS: 00000246 ORIG_RAX: 00000000000000e7
> > > RAX: ffffffffffffffda RBX: 00007f73156c93a3 RCX: 00007f731567dd69
> > > RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000000
> > > RBP: 0000000000000002 R08: 00007fff4f07e277 R09: 00007fff4f081790
> > > R10: 0000000000000000 R11: 0000000000000246 R12: 00007fff4f081790
> > > R13: 00007f73156c937e R14: 00000000000154d0 R15: 000000000000001e
> > >  </TASK>
> > > Modules linked in:
> > > ---[ end trace 0000000000000000 ]---
> > 
> > -- 
> > paul-moore.com
> > 

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [syzbot] [lsm?] general protection fault in hook_inode_free_security
  2024-05-16  7:31     ` Mickaël Salaün
@ 2024-05-16 13:07       ` Mathieu Desnoyers
  2024-06-27 13:33         ` Mickaël Salaün
  2024-06-27 13:34       ` Mickaël Salaün
  1 sibling, 1 reply; 24+ messages in thread
From: Mathieu Desnoyers @ 2024-05-16 13:07 UTC (permalink / raw)
  To: Mickaël Salaün, Paul Moore, Jann Horn, Paul E. McKenney
  Cc: syzbot, jmorris, linux-kernel, linux-security-module, serge,
	syzkaller-bugs, linux-fsdevel, Casey Schaufler,
	Christian Brauner

On 2024-05-16 03:31, Mickaël Salaün wrote:
> Adding membarrier experts.

I do not see how this relates to the membarrier(2) system call.

Thanks,

Mathieu

> 
> On Wed, May 15, 2024 at 05:12:58PM +0200, Mickaël Salaün wrote:
>> On Thu, May 09, 2024 at 08:01:49PM -0400, Paul Moore wrote:
>>> On Wed, May 8, 2024 at 3:32 PM syzbot
>>> <syzbot+5446fbf332b0602ede0b@syzkaller.appspotmail.com> wrote:
>>>>
>>>> Hello,
>>>>
>>>> syzbot found the following issue on:
>>>>
>>>> HEAD commit:    dccb07f2914c Merge tag 'for-6.9-rc7-tag' of git://git.kern..
>>>> git tree:       upstream
>>>> console output: https://syzkaller.appspot.com/x/log.txt?x=14a46760980000
>>>> kernel config:  https://syzkaller.appspot.com/x/.config?x=6d14c12b661fb43
>>>> dashboard link: https://syzkaller.appspot.com/bug?extid=5446fbf332b0602ede0b
>>>> compiler:       Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40
>>>>
>>>> Unfortunately, I don't have any reproducer for this issue yet.
>>>>
>>>> Downloadable assets:
>>>> disk image: https://storage.googleapis.com/syzbot-assets/39d66018d8ad/disk-dccb07f2.raw.xz
>>>> vmlinux: https://storage.googleapis.com/syzbot-assets/c160b651d1bc/vmlinux-dccb07f2.xz
>>>> kernel image: https://storage.googleapis.com/syzbot-assets/3662a33ac713/bzImage-dccb07f2.xz
>>>>
>>>> IMPORTANT: if you fix the issue, please add the following tag to the commit:
>>>> Reported-by: syzbot+5446fbf332b0602ede0b@syzkaller.appspotmail.com
>>>>
>>>> general protection fault, probably for non-canonical address 0xdffffc018f62f515: 0000 [#1] PREEMPT SMP KASAN NOPTI
>>>> KASAN: probably user-memory-access in range [0x0000000c7b17a8a8-0x0000000c7b17a8af]
>>>> CPU: 1 PID: 5102 Comm: syz-executor.1 Not tainted 6.9.0-rc7-syzkaller-00012-gdccb07f2914c #0
>>>> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 04/02/2024
>>>> RIP: 0010:hook_inode_free_security+0x5b/0xb0 security/landlock/fs.c:1047
>>>
>>> Possibly a Landlock issue, Mickaël?
>>
>> It looks like security_inode_free() is called two times on the same
>> inode.  This could happen if an inode labeled by Landlock is put
>> concurrently with release_inode() for a closed ruleset or with
>> hook_sb_delete().  I didn't find any race condition that could lead to
>> two calls to iput() though.  Could WRITE_ONCE(object->underobj, NULL)
>> change anything even if object->lock is locked?
>>
>> A bit unrelated but looking at the SELinux code, I see that selinux_inode()
>> checks `!inode->i_security`.  In which case could this happen?
>>
>>>
>>>> Code: 8a fd 48 8b 1b 48 c7 c0 c4 4e d5 8d 48 c1 e8 03 42 0f b6 04 30 84 c0 75 3e 48 63 05 33 59 65 09 48 01 c3 48 89 d8 48 c1 e8 03 <42> 80 3c 30 00 74 08 48 89 df e8 66 be 8a fd 48 83 3b 00 75 0d e8
>>>> RSP: 0018:ffffc9000307f9a8 EFLAGS: 00010212
>>>> RAX: 000000018f62f515 RBX: 0000000c7b17a8a8 RCX: ffff888027668000
>>>> RDX: 0000000000000000 RSI: 0000000000000040 RDI: ffff88805c0bb270
>>>> RBP: ffffffff8c01fb00 R08: ffffffff82132a15 R09: 1ffff1100b81765f
>>>> R10: dffffc0000000000 R11: ffffffff846ff540 R12: dffffc0000000000
>>>> R13: 1ffff1100b817683 R14: dffffc0000000000 R15: dffffc0000000000
>>>> FS:  0000000000000000(0000) GS:ffff8880b9500000(0000) knlGS:0000000000000000
>>>> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>>> CR2: 00007f43c42de000 CR3: 00000000635f8000 CR4: 0000000000350ef0
>>>> Call Trace:
>>>>   <TASK>
>>>>   security_inode_free+0x4a/0xd0 security/security.c:1613
>>>>   __destroy_inode+0x2d9/0x650 fs/inode.c:286
>>>>   destroy_inode fs/inode.c:309 [inline]
>>>>   evict+0x521/0x630 fs/inode.c:682
>>>>   dispose_list fs/inode.c:700 [inline]
>>>>   evict_inodes+0x5f9/0x690 fs/inode.c:750
>>>>   generic_shutdown_super+0x9d/0x2d0 fs/super.c:626
>>>>   kill_block_super+0x44/0x90 fs/super.c:1675
>>>>   deactivate_locked_super+0xc6/0x130 fs/super.c:472
>>>>   cleanup_mnt+0x426/0x4c0 fs/namespace.c:1267
>>>>   task_work_run+0x251/0x310 kernel/task_work.c:180
>>>>   exit_task_work include/linux/task_work.h:38 [inline]
>>>>   do_exit+0xa1b/0x27e0 kernel/exit.c:878
>>>>   do_group_exit+0x207/0x2c0 kernel/exit.c:1027
>>>>   __do_sys_exit_group kernel/exit.c:1038 [inline]
>>>>   __se_sys_exit_group kernel/exit.c:1036 [inline]
>>>>   __x64_sys_exit_group+0x3f/0x40 kernel/exit.c:1036
>>>>   do_syscall_x64 arch/x86/entry/common.c:52 [inline]
>>>>   do_syscall_64+0xf5/0x240 arch/x86/entry/common.c:83
>>>>   entry_SYSCALL_64_after_hwframe+0x77/0x7f
>>>> RIP: 0033:0x7f731567dd69
>>>> Code: Unable to access opcode bytes at 0x7f731567dd3f.
>>>> RSP: 002b:00007fff4f0804d8 EFLAGS: 00000246 ORIG_RAX: 00000000000000e7
>>>> RAX: ffffffffffffffda RBX: 00007f73156c93a3 RCX: 00007f731567dd69
>>>> RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000000
>>>> RBP: 0000000000000002 R08: 00007fff4f07e277 R09: 00007fff4f081790
>>>> R10: 0000000000000000 R11: 0000000000000246 R12: 00007fff4f081790
>>>> R13: 00007f73156c937e R14: 00000000000154d0 R15: 000000000000001e
>>>>   </TASK>
>>>> Modules linked in:
>>>> ---[ end trace 0000000000000000 ]---
>>>
>>> -- 
>>> paul-moore.com
>>>

-- 
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [syzbot] [lsm?] general protection fault in hook_inode_free_security
  2024-05-16 13:07       ` Mathieu Desnoyers
@ 2024-06-27 13:33         ` Mickaël Salaün
  0 siblings, 0 replies; 24+ messages in thread
From: Mickaël Salaün @ 2024-06-27 13:33 UTC (permalink / raw)
  To: Mathieu Desnoyers
  Cc: Paul Moore, Jann Horn, Paul E. McKenney, syzbot, jmorris,
	linux-kernel, linux-security-module, serge, syzkaller-bugs,
	linux-fsdevel, Casey Schaufler, Christian Brauner

On Thu, May 16, 2024 at 09:07:29AM GMT, Mathieu Desnoyers wrote:
> On 2024-05-16 03:31, Mickaël Salaün wrote:
> > Adding membarrier experts.
> 
> I do not see how this relates to the membarrier(2) system call.

I meant SMP barrier and RCU, so mostly Paul E. McKenney.
I'll remove you from the thread.

> 
> Thanks,
> 
> Mathieu
> 
> > 
> > On Wed, May 15, 2024 at 05:12:58PM +0200, Mickaël Salaün wrote:
> > > On Thu, May 09, 2024 at 08:01:49PM -0400, Paul Moore wrote:
> > > > On Wed, May 8, 2024 at 3:32 PM syzbot
> > > > <syzbot+5446fbf332b0602ede0b@syzkaller.appspotmail.com> wrote:
> > > > > 
> > > > > Hello,
> > > > > 
> > > > > syzbot found the following issue on:
> > > > > 
> > > > > HEAD commit:    dccb07f2914c Merge tag 'for-6.9-rc7-tag' of git://git.kern..
> > > > > git tree:       upstream
> > > > > console output: https://syzkaller.appspot.com/x/log.txt?x=14a46760980000
> > > > > kernel config:  https://syzkaller.appspot.com/x/.config?x=6d14c12b661fb43
> > > > > dashboard link: https://syzkaller.appspot.com/bug?extid=5446fbf332b0602ede0b
> > > > > compiler:       Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40
> > > > > 
> > > > > Unfortunately, I don't have any reproducer for this issue yet.
> > > > > 
> > > > > Downloadable assets:
> > > > > disk image: https://storage.googleapis.com/syzbot-assets/39d66018d8ad/disk-dccb07f2.raw.xz
> > > > > vmlinux: https://storage.googleapis.com/syzbot-assets/c160b651d1bc/vmlinux-dccb07f2.xz
> > > > > kernel image: https://storage.googleapis.com/syzbot-assets/3662a33ac713/bzImage-dccb07f2.xz
> > > > > 
> > > > > IMPORTANT: if you fix the issue, please add the following tag to the commit:
> > > > > Reported-by: syzbot+5446fbf332b0602ede0b@syzkaller.appspotmail.com
> > > > > 
> > > > > general protection fault, probably for non-canonical address 0xdffffc018f62f515: 0000 [#1] PREEMPT SMP KASAN NOPTI
> > > > > KASAN: probably user-memory-access in range [0x0000000c7b17a8a8-0x0000000c7b17a8af]
> > > > > CPU: 1 PID: 5102 Comm: syz-executor.1 Not tainted 6.9.0-rc7-syzkaller-00012-gdccb07f2914c #0
> > > > > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 04/02/2024
> > > > > RIP: 0010:hook_inode_free_security+0x5b/0xb0 security/landlock/fs.c:1047
> > > > 
> > > > Possibly a Landlock issue, Mickaël?
> > > 
> > > It looks like security_inode_free() is called two times on the same
> > > inode.  This could happen if an inode labeled by Landlock is put
> > > concurrently with release_inode() for a closed ruleset or with
> > > hook_sb_delete().  I didn't find any race condition that could lead to
> > > two calls to iput() though.  Could WRITE_ONCE(object->underobj, NULL)
> > > change anything even if object->lock is locked?
> > > 
> > > A bit unrelated but looking at the SELinux code, I see that selinux_inode()
> > > checks `!inode->i_security`.  In which case could this happen?
> > > 
> > > > 
> > > > > Code: 8a fd 48 8b 1b 48 c7 c0 c4 4e d5 8d 48 c1 e8 03 42 0f b6 04 30 84 c0 75 3e 48 63 05 33 59 65 09 48 01 c3 48 89 d8 48 c1 e8 03 <42> 80 3c 30 00 74 08 48 89 df e8 66 be 8a fd 48 83 3b 00 75 0d e8
> > > > > RSP: 0018:ffffc9000307f9a8 EFLAGS: 00010212
> > > > > RAX: 000000018f62f515 RBX: 0000000c7b17a8a8 RCX: ffff888027668000
> > > > > RDX: 0000000000000000 RSI: 0000000000000040 RDI: ffff88805c0bb270
> > > > > RBP: ffffffff8c01fb00 R08: ffffffff82132a15 R09: 1ffff1100b81765f
> > > > > R10: dffffc0000000000 R11: ffffffff846ff540 R12: dffffc0000000000
> > > > > R13: 1ffff1100b817683 R14: dffffc0000000000 R15: dffffc0000000000
> > > > > FS:  0000000000000000(0000) GS:ffff8880b9500000(0000) knlGS:0000000000000000
> > > > > CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > > > > CR2: 00007f43c42de000 CR3: 00000000635f8000 CR4: 0000000000350ef0
> > > > > Call Trace:
> > > > >   <TASK>
> > > > >   security_inode_free+0x4a/0xd0 security/security.c:1613
> > > > >   __destroy_inode+0x2d9/0x650 fs/inode.c:286
> > > > >   destroy_inode fs/inode.c:309 [inline]
> > > > >   evict+0x521/0x630 fs/inode.c:682
> > > > >   dispose_list fs/inode.c:700 [inline]
> > > > >   evict_inodes+0x5f9/0x690 fs/inode.c:750
> > > > >   generic_shutdown_super+0x9d/0x2d0 fs/super.c:626
> > > > >   kill_block_super+0x44/0x90 fs/super.c:1675
> > > > >   deactivate_locked_super+0xc6/0x130 fs/super.c:472
> > > > >   cleanup_mnt+0x426/0x4c0 fs/namespace.c:1267
> > > > >   task_work_run+0x251/0x310 kernel/task_work.c:180
> > > > >   exit_task_work include/linux/task_work.h:38 [inline]
> > > > >   do_exit+0xa1b/0x27e0 kernel/exit.c:878
> > > > >   do_group_exit+0x207/0x2c0 kernel/exit.c:1027
> > > > >   __do_sys_exit_group kernel/exit.c:1038 [inline]
> > > > >   __se_sys_exit_group kernel/exit.c:1036 [inline]
> > > > >   __x64_sys_exit_group+0x3f/0x40 kernel/exit.c:1036
> > > > >   do_syscall_x64 arch/x86/entry/common.c:52 [inline]
> > > > >   do_syscall_64+0xf5/0x240 arch/x86/entry/common.c:83
> > > > >   entry_SYSCALL_64_after_hwframe+0x77/0x7f
> > > > > RIP: 0033:0x7f731567dd69
> > > > > Code: Unable to access opcode bytes at 0x7f731567dd3f.
> > > > > RSP: 002b:00007fff4f0804d8 EFLAGS: 00000246 ORIG_RAX: 00000000000000e7
> > > > > RAX: ffffffffffffffda RBX: 00007f73156c93a3 RCX: 00007f731567dd69
> > > > > RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000000
> > > > > RBP: 0000000000000002 R08: 00007fff4f07e277 R09: 00007fff4f081790
> > > > > R10: 0000000000000000 R11: 0000000000000246 R12: 00007fff4f081790
> > > > > R13: 00007f73156c937e R14: 00000000000154d0 R15: 000000000000001e
> > > > >   </TASK>
> > > > > Modules linked in:
> > > > > ---[ end trace 0000000000000000 ]---
> > > > 
> > > > -- 
> > > > paul-moore.com
> > > > 
> 
> -- 
> Mathieu Desnoyers
> EfficiOS Inc.
> https://www.efficios.com
> 
> 

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [syzbot] [lsm?] general protection fault in hook_inode_free_security
  2024-05-16  7:31     ` Mickaël Salaün
  2024-05-16 13:07       ` Mathieu Desnoyers
@ 2024-06-27 13:34       ` Mickaël Salaün
  2024-06-27 18:12         ` Kees Cook
  2024-06-27 18:28         ` Paul Moore
  1 sibling, 2 replies; 24+ messages in thread
From: Mickaël Salaün @ 2024-06-27 13:34 UTC (permalink / raw)
  To: Paul Moore, Jann Horn, Christian Brauner, Paul E. McKenney,
	Casey Schaufler, Kees Cook
  Cc: syzbot, jmorris, linux-kernel, linux-security-module, serge,
	syzkaller-bugs, linux-fsdevel

I didn't find specific issues with Landlock's code except the extra
check in hook_inode_free_security().  It looks like inode->i_security is
a dangling pointer, leading to UAF.

Reading security_inode_free() comments, two things looks weird to me:
> /**
>  * security_inode_free() - Free an inode's LSM blob
>  * @inode: the inode
>  *
>  * Deallocate the inode security structure and set @inode->i_security to NULL.

I don't see where i_security is set to NULL.

>  */
> void security_inode_free(struct inode *inode)
> {

Shouldn't we add this check here?
if (!inode->i_security)
	return;

> 	call_void_hook(inode_free_security, inode);
> 	/*
> 	 * The inode may still be referenced in a path walk and
> 	 * a call to security_inode_permission() can be made
> 	 * after inode_free_security() is called. Ideally, the VFS
> 	 * wouldn't do this, but fixing that is a much harder
> 	 * job. For now, simply free the i_security via RCU, and
> 	 * leave the current inode->i_security pointer intact.
> 	 * The inode will be freed after the RCU grace period too.

It's not clear to me why this should be safe if an LSM try to use the
partially-freed blob after the hook calls and before the actual blob
free.

> 	 */
> 	if (inode->i_security)
> 		call_rcu((struct rcu_head *)inode->i_security,
> 			 inode_free_by_rcu);

And then:
inode->i_security = NULL;

But why call_rcu()?  i_security is not protected by RCU barriers.

> }


On Thu, May 16, 2024 at 09:31:21AM GMT, Mickaël Salaün wrote:
> Adding membarrier experts.
> 
> On Wed, May 15, 2024 at 05:12:58PM +0200, Mickaël Salaün wrote:
> > On Thu, May 09, 2024 at 08:01:49PM -0400, Paul Moore wrote:
> > > On Wed, May 8, 2024 at 3:32 PM syzbot
> > > <syzbot+5446fbf332b0602ede0b@syzkaller.appspotmail.com> wrote:
> > > >
> > > > Hello,
> > > >
> > > > syzbot found the following issue on:
> > > >
> > > > HEAD commit:    dccb07f2914c Merge tag 'for-6.9-rc7-tag' of git://git.kern..
> > > > git tree:       upstream
> > > > console output: https://syzkaller.appspot.com/x/log.txt?x=14a46760980000
> > > > kernel config:  https://syzkaller.appspot.com/x/.config?x=6d14c12b661fb43
> > > > dashboard link: https://syzkaller.appspot.com/bug?extid=5446fbf332b0602ede0b
> > > > compiler:       Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40
> > > >
> > > > Unfortunately, I don't have any reproducer for this issue yet.
> > > >
> > > > Downloadable assets:
> > > > disk image: https://storage.googleapis.com/syzbot-assets/39d66018d8ad/disk-dccb07f2.raw.xz
> > > > vmlinux: https://storage.googleapis.com/syzbot-assets/c160b651d1bc/vmlinux-dccb07f2.xz
> > > > kernel image: https://storage.googleapis.com/syzbot-assets/3662a33ac713/bzImage-dccb07f2.xz
> > > >
> > > > IMPORTANT: if you fix the issue, please add the following tag to the commit:
> > > > Reported-by: syzbot+5446fbf332b0602ede0b@syzkaller.appspotmail.com
> > > >
> > > > general protection fault, probably for non-canonical address 0xdffffc018f62f515: 0000 [#1] PREEMPT SMP KASAN NOPTI
> > > > KASAN: probably user-memory-access in range [0x0000000c7b17a8a8-0x0000000c7b17a8af]
> > > > CPU: 1 PID: 5102 Comm: syz-executor.1 Not tainted 6.9.0-rc7-syzkaller-00012-gdccb07f2914c #0
> > > > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 04/02/2024
> > > > RIP: 0010:hook_inode_free_security+0x5b/0xb0 security/landlock/fs.c:1047
> > > 
> > > Possibly a Landlock issue, Mickaël?
> > 
> > It looks like security_inode_free() is called two times on the same
> > inode.  This could happen if an inode labeled by Landlock is put
> > concurrently with release_inode() for a closed ruleset or with
> > hook_sb_delete().  I didn't find any race condition that could lead to
> > two calls to iput() though.  Could WRITE_ONCE(object->underobj, NULL)
> > change anything even if object->lock is locked?

I don't think so anymore, the issue is with i_security, not the blob
content.

> > 
> > A bit unrelated but looking at the SELinux code, I see that selinux_inode()
> > checks `!inode->i_security`.  In which case could this happen?

I think this shouldn't happen, and that might actually be an issue for
SELinux.  See my above comment about security_free_inode().

> > 
> > > 
> > > > Code: 8a fd 48 8b 1b 48 c7 c0 c4 4e d5 8d 48 c1 e8 03 42 0f b6 04 30 84 c0 75 3e 48 63 05 33 59 65 09 48 01 c3 48 89 d8 48 c1 e8 03 <42> 80 3c 30 00 74 08 48 89 df e8 66 be 8a fd 48 83 3b 00 75 0d e8
> > > > RSP: 0018:ffffc9000307f9a8 EFLAGS: 00010212
> > > > RAX: 000000018f62f515 RBX: 0000000c7b17a8a8 RCX: ffff888027668000
> > > > RDX: 0000000000000000 RSI: 0000000000000040 RDI: ffff88805c0bb270
> > > > RBP: ffffffff8c01fb00 R08: ffffffff82132a15 R09: 1ffff1100b81765f
> > > > R10: dffffc0000000000 R11: ffffffff846ff540 R12: dffffc0000000000
> > > > R13: 1ffff1100b817683 R14: dffffc0000000000 R15: dffffc0000000000
> > > > FS:  0000000000000000(0000) GS:ffff8880b9500000(0000) knlGS:0000000000000000
> > > > CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > > > CR2: 00007f43c42de000 CR3: 00000000635f8000 CR4: 0000000000350ef0
> > > > Call Trace:
> > > >  <TASK>
> > > >  security_inode_free+0x4a/0xd0 security/security.c:1613
> > > >  __destroy_inode+0x2d9/0x650 fs/inode.c:286
> > > >  destroy_inode fs/inode.c:309 [inline]
> > > >  evict+0x521/0x630 fs/inode.c:682
> > > >  dispose_list fs/inode.c:700 [inline]
> > > >  evict_inodes+0x5f9/0x690 fs/inode.c:750
> > > >  generic_shutdown_super+0x9d/0x2d0 fs/super.c:626
> > > >  kill_block_super+0x44/0x90 fs/super.c:1675
> > > >  deactivate_locked_super+0xc6/0x130 fs/super.c:472
> > > >  cleanup_mnt+0x426/0x4c0 fs/namespace.c:1267
> > > >  task_work_run+0x251/0x310 kernel/task_work.c:180
> > > >  exit_task_work include/linux/task_work.h:38 [inline]
> > > >  do_exit+0xa1b/0x27e0 kernel/exit.c:878
> > > >  do_group_exit+0x207/0x2c0 kernel/exit.c:1027
> > > >  __do_sys_exit_group kernel/exit.c:1038 [inline]
> > > >  __se_sys_exit_group kernel/exit.c:1036 [inline]
> > > >  __x64_sys_exit_group+0x3f/0x40 kernel/exit.c:1036
> > > >  do_syscall_x64 arch/x86/entry/common.c:52 [inline]
> > > >  do_syscall_64+0xf5/0x240 arch/x86/entry/common.c:83
> > > >  entry_SYSCALL_64_after_hwframe+0x77/0x7f
> > > > RIP: 0033:0x7f731567dd69
> > > > Code: Unable to access opcode bytes at 0x7f731567dd3f.
> > > > RSP: 002b:00007fff4f0804d8 EFLAGS: 00000246 ORIG_RAX: 00000000000000e7
> > > > RAX: ffffffffffffffda RBX: 00007f73156c93a3 RCX: 00007f731567dd69
> > > > RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000000
> > > > RBP: 0000000000000002 R08: 00007fff4f07e277 R09: 00007fff4f081790
> > > > R10: 0000000000000000 R11: 0000000000000246 R12: 00007fff4f081790
> > > > R13: 00007f73156c937e R14: 00000000000154d0 R15: 000000000000001e
> > > >  </TASK>
> > > > Modules linked in:
> > > > ---[ end trace 0000000000000000 ]---
> > > 
> > > -- 
> > > paul-moore.com
> > > 

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [syzbot] [lsm?] general protection fault in hook_inode_free_security
  2024-06-27 13:34       ` Mickaël Salaün
@ 2024-06-27 18:12         ` Kees Cook
  2024-06-27 18:45           ` Paul E. McKenney
                             ` (2 more replies)
  2024-06-27 18:28         ` Paul Moore
  1 sibling, 3 replies; 24+ messages in thread
From: Kees Cook @ 2024-06-27 18:12 UTC (permalink / raw)
  To: Mickaël Salaün, Christian Brauner, Steven Rostedt
  Cc: Paul Moore, Jann Horn, Paul E. McKenney, Casey Schaufler, syzbot,
	jmorris, linux-kernel, linux-security-module, serge,
	syzkaller-bugs, linux-fsdevel

On Thu, Jun 27, 2024 at 03:34:41PM +0200, Mickaël Salaün wrote:
> I didn't find specific issues with Landlock's code except the extra
> check in hook_inode_free_security().  It looks like inode->i_security is
> a dangling pointer, leading to UAF.
> 
> Reading security_inode_free() comments, two things looks weird to me:
> > /**
> >  * security_inode_free() - Free an inode's LSM blob
> >  * @inode: the inode
> >  *
> >  * Deallocate the inode security structure and set @inode->i_security to NULL.
> 
> I don't see where i_security is set to NULL.

Yeah, I don't either...

> >  */
> > void security_inode_free(struct inode *inode)
> > {
> 
> Shouldn't we add this check here?
> if (!inode->i_security)
> 	return;

Probably, yes. The LSMs that check for NULL i_security in the free hook
all do so right at the beginning...

> 
> > 	call_void_hook(inode_free_security, inode);
> > 	/*
> > 	 * The inode may still be referenced in a path walk and
> > 	 * a call to security_inode_permission() can be made
> > 	 * after inode_free_security() is called. Ideally, the VFS
> > 	 * wouldn't do this, but fixing that is a much harder
> > 	 * job. For now, simply free the i_security via RCU, and
> > 	 * leave the current inode->i_security pointer intact.
> > 	 * The inode will be freed after the RCU grace period too.
> 
> It's not clear to me why this should be safe if an LSM try to use the
> partially-freed blob after the hook calls and before the actual blob
> free.

Yeah, it's not clear to me what the expected lifetime is here. How is
inode_permission() being called if all inode reference counts are 0? It
does seem intentional, though.

The RCU logic was introduced in commit 3dc91d4338d6 ("SELinux: Fix possible
NULL pointer dereference in selinux_inode_permission()"), with much
discussion:
https://lore.kernel.org/lkml/20140109101932.0508dec7@gandalf.local.home/
(This commit seems to remove setting "i_security = NULL", though, which
the comment implies is intended, but then it also seems to depend on
finding a NULL?)

LSMs using i_security are:

security/bpf/hooks.c:   .lbs_inode = sizeof(struct bpf_storage_blob),
security/integrity/evm/evm_main.c:      .lbs_inode = sizeof(struct evm_iint_cache),
security/integrity/ima/ima_main.c:      .lbs_inode = sizeof(struct ima_iint_cache *),
security/landlock/setup.c:      .lbs_inode = sizeof(struct landlock_inode_security),
security/selinux/hooks.c:       .lbs_inode = sizeof(struct inode_security_struct),
security/smack/smack_lsm.c:     .lbs_inode = sizeof(struct inode_smack),

SELinux is still checking for NULL. See selinux_inode() and
selinux_inode_free_security(), as do bpf_inode() and
bpf_inode_storage_free(). evm and ima also check for NULL.

landlock_inode() does not, though.

Smack doesn't hook the free, but it should still check for NULL, and it's not.

So I think this needs fixing in Landlock and Smack.

I kind of think that the LSM infrastructure needs to provide a common
helper for the "access the blob" action, as we've got it repeated in
each LSM, and we have 2 implementations that are missing NULL checks...

> 
> > 	 */
> > 	if (inode->i_security)
> > 		call_rcu((struct rcu_head *)inode->i_security,
> > 			 inode_free_by_rcu);
> 
> And then:
> inode->i_security = NULL;
> 
> But why call_rcu()?  i_security is not protected by RCU barriers.

I assume it's because security_inode_free() via __destroy_inode() via
destroy_inode() via evict() via iput_final() via iput() may be running
in interrupt context?

But I still don't see where i_security gets set to NULL. This won't fix
the permissions hook races for Landlock and Smack, but should make
lifetime a bit more clear?


diff --git a/security/security.c b/security/security.c
index 9c3fb2f60e2a..a8658ebcaf0c 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1613,7 +1613,8 @@ static void inode_free_by_rcu(struct rcu_head *head)
  */
 void security_inode_free(struct inode *inode)
 {
-	call_void_hook(inode_free_security, inode);
+	struct rcu_head *inode_blob = inode->i_security;
+
 	/*
 	 * The inode may still be referenced in a path walk and
 	 * a call to security_inode_permission() can be made
@@ -1623,9 +1624,11 @@ void security_inode_free(struct inode *inode)
 	 * leave the current inode->i_security pointer intact.
 	 * The inode will be freed after the RCU grace period too.
 	 */
-	if (inode->i_security)
-		call_rcu((struct rcu_head *)inode->i_security,
-			 inode_free_by_rcu);
+	if (inode_blob) {
+		call_void_hook(inode_free_security, inode);
+		inode->i_security = NULL;
+		call_rcu(inode_blob, inode_free_by_rcu);
+	}
 }
 
 /**


-Kees

> 
> > }
> 
> 
> On Thu, May 16, 2024 at 09:31:21AM GMT, Mickaël Salaün wrote:
> > Adding membarrier experts.
> > 
> > On Wed, May 15, 2024 at 05:12:58PM +0200, Mickaël Salaün wrote:
> > > On Thu, May 09, 2024 at 08:01:49PM -0400, Paul Moore wrote:
> > > > On Wed, May 8, 2024 at 3:32 PM syzbot
> > > > <syzbot+5446fbf332b0602ede0b@syzkaller.appspotmail.com> wrote:
> > > > >
> > > > > Hello,
> > > > >
> > > > > syzbot found the following issue on:
> > > > >
> > > > > HEAD commit:    dccb07f2914c Merge tag 'for-6.9-rc7-tag' of git://git.kern..
> > > > > git tree:       upstream
> > > > > console output: https://syzkaller.appspot.com/x/log.txt?x=14a46760980000
> > > > > kernel config:  https://syzkaller.appspot.com/x/.config?x=6d14c12b661fb43
> > > > > dashboard link: https://syzkaller.appspot.com/bug?extid=5446fbf332b0602ede0b
> > > > > compiler:       Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40
> > > > >
> > > > > Unfortunately, I don't have any reproducer for this issue yet.
> > > > >
> > > > > Downloadable assets:
> > > > > disk image: https://storage.googleapis.com/syzbot-assets/39d66018d8ad/disk-dccb07f2.raw.xz
> > > > > vmlinux: https://storage.googleapis.com/syzbot-assets/c160b651d1bc/vmlinux-dccb07f2.xz
> > > > > kernel image: https://storage.googleapis.com/syzbot-assets/3662a33ac713/bzImage-dccb07f2.xz
> > > > >
> > > > > IMPORTANT: if you fix the issue, please add the following tag to the commit:
> > > > > Reported-by: syzbot+5446fbf332b0602ede0b@syzkaller.appspotmail.com
> > > > >
> > > > > general protection fault, probably for non-canonical address 0xdffffc018f62f515: 0000 [#1] PREEMPT SMP KASAN NOPTI
> > > > > KASAN: probably user-memory-access in range [0x0000000c7b17a8a8-0x0000000c7b17a8af]
> > > > > CPU: 1 PID: 5102 Comm: syz-executor.1 Not tainted 6.9.0-rc7-syzkaller-00012-gdccb07f2914c #0
> > > > > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 04/02/2024
> > > > > RIP: 0010:hook_inode_free_security+0x5b/0xb0 security/landlock/fs.c:1047
> > > > 
> > > > Possibly a Landlock issue, Mickaël?
> > > 
> > > It looks like security_inode_free() is called two times on the same
> > > inode.  This could happen if an inode labeled by Landlock is put
> > > concurrently with release_inode() for a closed ruleset or with
> > > hook_sb_delete().  I didn't find any race condition that could lead to
> > > two calls to iput() though.  Could WRITE_ONCE(object->underobj, NULL)
> > > change anything even if object->lock is locked?
> 
> I don't think so anymore, the issue is with i_security, not the blob
> content.
> 
> > > 
> > > A bit unrelated but looking at the SELinux code, I see that selinux_inode()
> > > checks `!inode->i_security`.  In which case could this happen?
> 
> I think this shouldn't happen, and that might actually be an issue for
> SELinux.  See my above comment about security_free_inode().
> 
> > > 
> > > > 
> > > > > Code: 8a fd 48 8b 1b 48 c7 c0 c4 4e d5 8d 48 c1 e8 03 42 0f b6 04 30 84 c0 75 3e 48 63 05 33 59 65 09 48 01 c3 48 89 d8 48 c1 e8 03 <42> 80 3c 30 00 74 08 48 89 df e8 66 be 8a fd 48 83 3b 00 75 0d e8
> > > > > RSP: 0018:ffffc9000307f9a8 EFLAGS: 00010212
> > > > > RAX: 000000018f62f515 RBX: 0000000c7b17a8a8 RCX: ffff888027668000
> > > > > RDX: 0000000000000000 RSI: 0000000000000040 RDI: ffff88805c0bb270
> > > > > RBP: ffffffff8c01fb00 R08: ffffffff82132a15 R09: 1ffff1100b81765f
> > > > > R10: dffffc0000000000 R11: ffffffff846ff540 R12: dffffc0000000000
> > > > > R13: 1ffff1100b817683 R14: dffffc0000000000 R15: dffffc0000000000
> > > > > FS:  0000000000000000(0000) GS:ffff8880b9500000(0000) knlGS:0000000000000000
> > > > > CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > > > > CR2: 00007f43c42de000 CR3: 00000000635f8000 CR4: 0000000000350ef0
> > > > > Call Trace:
> > > > >  <TASK>
> > > > >  security_inode_free+0x4a/0xd0 security/security.c:1613
> > > > >  __destroy_inode+0x2d9/0x650 fs/inode.c:286
> > > > >  destroy_inode fs/inode.c:309 [inline]
> > > > >  evict+0x521/0x630 fs/inode.c:682
> > > > >  dispose_list fs/inode.c:700 [inline]
> > > > >  evict_inodes+0x5f9/0x690 fs/inode.c:750
> > > > >  generic_shutdown_super+0x9d/0x2d0 fs/super.c:626
> > > > >  kill_block_super+0x44/0x90 fs/super.c:1675
> > > > >  deactivate_locked_super+0xc6/0x130 fs/super.c:472
> > > > >  cleanup_mnt+0x426/0x4c0 fs/namespace.c:1267
> > > > >  task_work_run+0x251/0x310 kernel/task_work.c:180
> > > > >  exit_task_work include/linux/task_work.h:38 [inline]
> > > > >  do_exit+0xa1b/0x27e0 kernel/exit.c:878
> > > > >  do_group_exit+0x207/0x2c0 kernel/exit.c:1027
> > > > >  __do_sys_exit_group kernel/exit.c:1038 [inline]
> > > > >  __se_sys_exit_group kernel/exit.c:1036 [inline]
> > > > >  __x64_sys_exit_group+0x3f/0x40 kernel/exit.c:1036
> > > > >  do_syscall_x64 arch/x86/entry/common.c:52 [inline]
> > > > >  do_syscall_64+0xf5/0x240 arch/x86/entry/common.c:83
> > > > >  entry_SYSCALL_64_after_hwframe+0x77/0x7f
> > > > > RIP: 0033:0x7f731567dd69
> > > > > Code: Unable to access opcode bytes at 0x7f731567dd3f.
> > > > > RSP: 002b:00007fff4f0804d8 EFLAGS: 00000246 ORIG_RAX: 00000000000000e7
> > > > > RAX: ffffffffffffffda RBX: 00007f73156c93a3 RCX: 00007f731567dd69
> > > > > RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000000
> > > > > RBP: 0000000000000002 R08: 00007fff4f07e277 R09: 00007fff4f081790
> > > > > R10: 0000000000000000 R11: 0000000000000246 R12: 00007fff4f081790
> > > > > R13: 00007f73156c937e R14: 00000000000154d0 R15: 000000000000001e
> > > > >  </TASK>
> > > > > Modules linked in:
> > > > > ---[ end trace 0000000000000000 ]---
> > > > 
> > > > -- 
> > > > paul-moore.com
> > > > 

-- 
Kees Cook

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* Re: [syzbot] [lsm?] general protection fault in hook_inode_free_security
  2024-06-27 13:34       ` Mickaël Salaün
  2024-06-27 18:12         ` Kees Cook
@ 2024-06-27 18:28         ` Paul Moore
  2024-07-08 14:11           ` Mickaël Salaün
                             ` (2 more replies)
  1 sibling, 3 replies; 24+ messages in thread
From: Paul Moore @ 2024-06-27 18:28 UTC (permalink / raw)
  To: Mickaël Salaün
  Cc: Jann Horn, Christian Brauner, Paul E. McKenney, Casey Schaufler,
	Kees Cook, syzbot, jmorris, linux-kernel, linux-security-module,
	serge, syzkaller-bugs, linux-fsdevel

On Thu, Jun 27, 2024 at 9:34 AM Mickaël Salaün <mic@digikod.net> wrote:
>
> I didn't find specific issues with Landlock's code except the extra
> check in hook_inode_free_security().  It looks like inode->i_security is
> a dangling pointer, leading to UAF.
>
> Reading security_inode_free() comments, two things looks weird to me:
>
> > /**
> >  * security_inode_free() - Free an inode's LSM blob
> >  * @inode: the inode
> >  *
> >  * Deallocate the inode security structure and set @inode->i_security to NULL.
>
> I don't see where i_security is set to NULL.

The function header comments are known to be a bit suspect, a side
effect of being detached from the functions for many years, this may
be one of those cases.  I tried to fix up the really awful ones when I
moved the comments back, back I didn't have time to go through each
one in detail.  Patches to correct the function header comments are
welcome and encouraged! :)

> >  */
> > void security_inode_free(struct inode *inode)
> > {
>
> Shouldn't we add this check here?
> if (!inode->i_security)
>         return;

Unless I'm remembering something wrong, I believe we *should* always
have a valid i_security pointer each time we are called, if not
something has gone wrong, e.g. the security_inode_free() hook is no
longer being called from the right place.  If we add a NULL check, we
should probably have a WARN_ON(), pr_err(), or something similar to
put some spew on the console/logs.

All that said, it would be good to hear some confirmation from the VFS
folks that the security_inode_free() hook is located in a spot such
that once it exits it's current RCU critical section it is safe to
release the associated LSM state.

It's also worth mentioning that while we always allocate i_security in
security_inode_alloc() right now, I can see a world where we allocate
the i_security field based on need using the lsm_blob_size info (maybe
that works today?  not sure how kmem_cache handled 0 length blobs?).
The result is that there might be a legitimate case where i_security
is NULL, yet we still want to call into the LSM using the
inode_free_security() implementation hook.

> >       call_void_hook(inode_free_security, inode);
> >       /*
> >        * The inode may still be referenced in a path walk and
> >        * a call to security_inode_permission() can be made
> >        * after inode_free_security() is called. Ideally, the VFS
> >        * wouldn't do this, but fixing that is a much harder
> >        * job. For now, simply free the i_security via RCU, and
> >        * leave the current inode->i_security pointer intact.
> >        * The inode will be freed after the RCU grace period too.
>
> It's not clear to me why this should be safe if an LSM try to use the
> partially-freed blob after the hook calls and before the actual blob
> free.

I had the same thought while looking at this just now.  At least in
the SELinux case I think this "works" simply because SELinux doesn't
do much here, it just drops the inode from a SELinux internal list
(long story) and doesn't actually release any memory or reset the
inode's SELinux state (there really isn't anything to "free" in the
SELinux case).  I haven't checked the other LSMs, but they may behave
similarly.

We may want (need?) to consider two LSM implementation hooks called
from within security_inode_free(): the first where the existing
inode_free_security() implementation hook is called, the second inside
the inode_free_by_rcu() callback immediately before the i_security
data is free'd.

... or we find a better placement in the VFS for
security_inode_free(), is that is possible.  It may not be, our VFS
friends should be able to help here.

> >        */
> >       if (inode->i_security)
> >               call_rcu((struct rcu_head *)inode->i_security,
> >                        inode_free_by_rcu);
>
> And then:
> inode->i_security = NULL;

According to the comment we may still need i_security for permission
checks.  See my comment about decomposing the LSM implementation into
two hooks to better handle this for LSMs.

> But why call_rcu()?  i_security is not protected by RCU barriers.

I believe the issue is that the inode is protected by RCU and that
affects the lifetime of the i_security blob.

-- 
paul-moore.com

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [syzbot] [lsm?] general protection fault in hook_inode_free_security
  2024-06-27 18:12         ` Kees Cook
@ 2024-06-27 18:45           ` Paul E. McKenney
  2024-06-27 19:29           ` Paul Moore
  2024-07-08 14:02           ` Mickaël Salaün
  2 siblings, 0 replies; 24+ messages in thread
From: Paul E. McKenney @ 2024-06-27 18:45 UTC (permalink / raw)
  To: Kees Cook
  Cc: Mickaël Salaün, Christian Brauner, Steven Rostedt,
	Paul Moore, Jann Horn, Casey Schaufler, syzbot, jmorris,
	linux-kernel, linux-security-module, serge, syzkaller-bugs,
	linux-fsdevel

On Thu, Jun 27, 2024 at 11:12:43AM -0700, Kees Cook wrote:
> On Thu, Jun 27, 2024 at 03:34:41PM +0200, Mickaël Salaün wrote:
> > I didn't find specific issues with Landlock's code except the extra
> > check in hook_inode_free_security().  It looks like inode->i_security is
> > a dangling pointer, leading to UAF.
> > 
> > Reading security_inode_free() comments, two things looks weird to me:
> > > /**
> > >  * security_inode_free() - Free an inode's LSM blob
> > >  * @inode: the inode
> > >  *
> > >  * Deallocate the inode security structure and set @inode->i_security to NULL.
> > 
> > I don't see where i_security is set to NULL.
> 
> Yeah, I don't either...
> 
> > >  */
> > > void security_inode_free(struct inode *inode)
> > > {
> > 
> > Shouldn't we add this check here?
> > if (!inode->i_security)
> > 	return;
> 
> Probably, yes. The LSMs that check for NULL i_security in the free hook
> all do so right at the beginning...

Given your fix below, I am assuming that they do this check within some
sort of an RCU read-side critical section.

> > > 	call_void_hook(inode_free_security, inode);
> > > 	/*
> > > 	 * The inode may still be referenced in a path walk and
> > > 	 * a call to security_inode_permission() can be made
> > > 	 * after inode_free_security() is called. Ideally, the VFS
> > > 	 * wouldn't do this, but fixing that is a much harder
> > > 	 * job. For now, simply free the i_security via RCU, and
> > > 	 * leave the current inode->i_security pointer intact.
> > > 	 * The inode will be freed after the RCU grace period too.
> > 
> > It's not clear to me why this should be safe if an LSM try to use the
> > partially-freed blob after the hook calls and before the actual blob
> > free.
> 
> Yeah, it's not clear to me what the expected lifetime is here. How is
> inode_permission() being called if all inode reference counts are 0? It
> does seem intentional, though.
> 
> The RCU logic was introduced in commit 3dc91d4338d6 ("SELinux: Fix possible
> NULL pointer dereference in selinux_inode_permission()"), with much
> discussion:
> https://lore.kernel.org/lkml/20140109101932.0508dec7@gandalf.local.home/
> (This commit seems to remove setting "i_security = NULL", though, which
> the comment implies is intended, but then it also seems to depend on
> finding a NULL?)
> 
> LSMs using i_security are:
> 
> security/bpf/hooks.c:   .lbs_inode = sizeof(struct bpf_storage_blob),
> security/integrity/evm/evm_main.c:      .lbs_inode = sizeof(struct evm_iint_cache),
> security/integrity/ima/ima_main.c:      .lbs_inode = sizeof(struct ima_iint_cache *),
> security/landlock/setup.c:      .lbs_inode = sizeof(struct landlock_inode_security),
> security/selinux/hooks.c:       .lbs_inode = sizeof(struct inode_security_struct),
> security/smack/smack_lsm.c:     .lbs_inode = sizeof(struct inode_smack),
> 
> SELinux is still checking for NULL. See selinux_inode() and
> selinux_inode_free_security(), as do bpf_inode() and
> bpf_inode_storage_free(). evm and ima also check for NULL.
> 
> landlock_inode() does not, though.
> 
> Smack doesn't hook the free, but it should still check for NULL, and it's not.
> 
> So I think this needs fixing in Landlock and Smack.
> 
> I kind of think that the LSM infrastructure needs to provide a common
> helper for the "access the blob" action, as we've got it repeated in
> each LSM, and we have 2 implementations that are missing NULL checks...
> 
> > 
> > > 	 */
> > > 	if (inode->i_security)
> > > 		call_rcu((struct rcu_head *)inode->i_security,
> > > 			 inode_free_by_rcu);
> > 
> > And then:
> > inode->i_security = NULL;
> > 
> > But why call_rcu()?  i_security is not protected by RCU barriers.
> 
> I assume it's because security_inode_free() via __destroy_inode() via
> destroy_inode() via evict() via iput_final() via iput() may be running
> in interrupt context?
> 
> But I still don't see where i_security gets set to NULL. This won't fix
> the permissions hook races for Landlock and Smack, but should make
> lifetime a bit more clear?
> 
> 
> diff --git a/security/security.c b/security/security.c
> index 9c3fb2f60e2a..a8658ebcaf0c 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1613,7 +1613,8 @@ static void inode_free_by_rcu(struct rcu_head *head)
>   */
>  void security_inode_free(struct inode *inode)
>  {
> -	call_void_hook(inode_free_security, inode);
> +	struct rcu_head *inode_blob = inode->i_security;
> +
>  	/*
>  	 * The inode may still be referenced in a path walk and
>  	 * a call to security_inode_permission() can be made
> @@ -1623,9 +1624,11 @@ void security_inode_free(struct inode *inode)
>  	 * leave the current inode->i_security pointer intact.
>  	 * The inode will be freed after the RCU grace period too.
>  	 */
> -	if (inode->i_security)
> -		call_rcu((struct rcu_head *)inode->i_security,
> -			 inode_free_by_rcu);
> +	if (inode_blob) {
> +		call_void_hook(inode_free_security, inode);
> +		inode->i_security = NULL;
> +		call_rcu(inode_blob, inode_free_by_rcu);
> +	}
>  }

This approach looks plausible to me.

							Thanx, Paul

>  /**
> 
> 
> -Kees
> 
> > 
> > > }
> > 
> > 
> > On Thu, May 16, 2024 at 09:31:21AM GMT, Mickaël Salaün wrote:
> > > Adding membarrier experts.
> > > 
> > > On Wed, May 15, 2024 at 05:12:58PM +0200, Mickaël Salaün wrote:
> > > > On Thu, May 09, 2024 at 08:01:49PM -0400, Paul Moore wrote:
> > > > > On Wed, May 8, 2024 at 3:32 PM syzbot
> > > > > <syzbot+5446fbf332b0602ede0b@syzkaller.appspotmail.com> wrote:
> > > > > >
> > > > > > Hello,
> > > > > >
> > > > > > syzbot found the following issue on:
> > > > > >
> > > > > > HEAD commit:    dccb07f2914c Merge tag 'for-6.9-rc7-tag' of git://git.kern..
> > > > > > git tree:       upstream
> > > > > > console output: https://syzkaller.appspot.com/x/log.txt?x=14a46760980000
> > > > > > kernel config:  https://syzkaller.appspot.com/x/.config?x=6d14c12b661fb43
> > > > > > dashboard link: https://syzkaller.appspot.com/bug?extid=5446fbf332b0602ede0b
> > > > > > compiler:       Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40
> > > > > >
> > > > > > Unfortunately, I don't have any reproducer for this issue yet.
> > > > > >
> > > > > > Downloadable assets:
> > > > > > disk image: https://storage.googleapis.com/syzbot-assets/39d66018d8ad/disk-dccb07f2.raw.xz
> > > > > > vmlinux: https://storage.googleapis.com/syzbot-assets/c160b651d1bc/vmlinux-dccb07f2.xz
> > > > > > kernel image: https://storage.googleapis.com/syzbot-assets/3662a33ac713/bzImage-dccb07f2.xz
> > > > > >
> > > > > > IMPORTANT: if you fix the issue, please add the following tag to the commit:
> > > > > > Reported-by: syzbot+5446fbf332b0602ede0b@syzkaller.appspotmail.com
> > > > > >
> > > > > > general protection fault, probably for non-canonical address 0xdffffc018f62f515: 0000 [#1] PREEMPT SMP KASAN NOPTI
> > > > > > KASAN: probably user-memory-access in range [0x0000000c7b17a8a8-0x0000000c7b17a8af]
> > > > > > CPU: 1 PID: 5102 Comm: syz-executor.1 Not tainted 6.9.0-rc7-syzkaller-00012-gdccb07f2914c #0
> > > > > > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 04/02/2024
> > > > > > RIP: 0010:hook_inode_free_security+0x5b/0xb0 security/landlock/fs.c:1047
> > > > > 
> > > > > Possibly a Landlock issue, Mickaël?
> > > > 
> > > > It looks like security_inode_free() is called two times on the same
> > > > inode.  This could happen if an inode labeled by Landlock is put
> > > > concurrently with release_inode() for a closed ruleset or with
> > > > hook_sb_delete().  I didn't find any race condition that could lead to
> > > > two calls to iput() though.  Could WRITE_ONCE(object->underobj, NULL)
> > > > change anything even if object->lock is locked?
> > 
> > I don't think so anymore, the issue is with i_security, not the blob
> > content.
> > 
> > > > 
> > > > A bit unrelated but looking at the SELinux code, I see that selinux_inode()
> > > > checks `!inode->i_security`.  In which case could this happen?
> > 
> > I think this shouldn't happen, and that might actually be an issue for
> > SELinux.  See my above comment about security_free_inode().
> > 
> > > > 
> > > > > 
> > > > > > Code: 8a fd 48 8b 1b 48 c7 c0 c4 4e d5 8d 48 c1 e8 03 42 0f b6 04 30 84 c0 75 3e 48 63 05 33 59 65 09 48 01 c3 48 89 d8 48 c1 e8 03 <42> 80 3c 30 00 74 08 48 89 df e8 66 be 8a fd 48 83 3b 00 75 0d e8
> > > > > > RSP: 0018:ffffc9000307f9a8 EFLAGS: 00010212
> > > > > > RAX: 000000018f62f515 RBX: 0000000c7b17a8a8 RCX: ffff888027668000
> > > > > > RDX: 0000000000000000 RSI: 0000000000000040 RDI: ffff88805c0bb270
> > > > > > RBP: ffffffff8c01fb00 R08: ffffffff82132a15 R09: 1ffff1100b81765f
> > > > > > R10: dffffc0000000000 R11: ffffffff846ff540 R12: dffffc0000000000
> > > > > > R13: 1ffff1100b817683 R14: dffffc0000000000 R15: dffffc0000000000
> > > > > > FS:  0000000000000000(0000) GS:ffff8880b9500000(0000) knlGS:0000000000000000
> > > > > > CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > > > > > CR2: 00007f43c42de000 CR3: 00000000635f8000 CR4: 0000000000350ef0
> > > > > > Call Trace:
> > > > > >  <TASK>
> > > > > >  security_inode_free+0x4a/0xd0 security/security.c:1613
> > > > > >  __destroy_inode+0x2d9/0x650 fs/inode.c:286
> > > > > >  destroy_inode fs/inode.c:309 [inline]
> > > > > >  evict+0x521/0x630 fs/inode.c:682
> > > > > >  dispose_list fs/inode.c:700 [inline]
> > > > > >  evict_inodes+0x5f9/0x690 fs/inode.c:750
> > > > > >  generic_shutdown_super+0x9d/0x2d0 fs/super.c:626
> > > > > >  kill_block_super+0x44/0x90 fs/super.c:1675
> > > > > >  deactivate_locked_super+0xc6/0x130 fs/super.c:472
> > > > > >  cleanup_mnt+0x426/0x4c0 fs/namespace.c:1267
> > > > > >  task_work_run+0x251/0x310 kernel/task_work.c:180
> > > > > >  exit_task_work include/linux/task_work.h:38 [inline]
> > > > > >  do_exit+0xa1b/0x27e0 kernel/exit.c:878
> > > > > >  do_group_exit+0x207/0x2c0 kernel/exit.c:1027
> > > > > >  __do_sys_exit_group kernel/exit.c:1038 [inline]
> > > > > >  __se_sys_exit_group kernel/exit.c:1036 [inline]
> > > > > >  __x64_sys_exit_group+0x3f/0x40 kernel/exit.c:1036
> > > > > >  do_syscall_x64 arch/x86/entry/common.c:52 [inline]
> > > > > >  do_syscall_64+0xf5/0x240 arch/x86/entry/common.c:83
> > > > > >  entry_SYSCALL_64_after_hwframe+0x77/0x7f
> > > > > > RIP: 0033:0x7f731567dd69
> > > > > > Code: Unable to access opcode bytes at 0x7f731567dd3f.
> > > > > > RSP: 002b:00007fff4f0804d8 EFLAGS: 00000246 ORIG_RAX: 00000000000000e7
> > > > > > RAX: ffffffffffffffda RBX: 00007f73156c93a3 RCX: 00007f731567dd69
> > > > > > RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000000
> > > > > > RBP: 0000000000000002 R08: 00007fff4f07e277 R09: 00007fff4f081790
> > > > > > R10: 0000000000000000 R11: 0000000000000246 R12: 00007fff4f081790
> > > > > > R13: 00007f73156c937e R14: 00000000000154d0 R15: 000000000000001e
> > > > > >  </TASK>
> > > > > > Modules linked in:
> > > > > > ---[ end trace 0000000000000000 ]---
> > > > > 
> > > > > -- 
> > > > > paul-moore.com
> > > > > 
> 
> -- 
> Kees Cook

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [syzbot] [lsm?] general protection fault in hook_inode_free_security
  2024-06-27 18:12         ` Kees Cook
  2024-06-27 18:45           ` Paul E. McKenney
@ 2024-06-27 19:29           ` Paul Moore
  2024-07-08 14:02           ` Mickaël Salaün
  2 siblings, 0 replies; 24+ messages in thread
From: Paul Moore @ 2024-06-27 19:29 UTC (permalink / raw)
  To: Kees Cook
  Cc: Mickaël Salaün, Christian Brauner, Steven Rostedt,
	Jann Horn, Paul E. McKenney, Casey Schaufler, syzbot, jmorris,
	linux-kernel, linux-security-module, serge, syzkaller-bugs,
	linux-fsdevel

On Thu, Jun 27, 2024 at 2:12 PM Kees Cook <kees@kernel.org> wrote:
> diff --git a/security/security.c b/security/security.c
> index 9c3fb2f60e2a..a8658ebcaf0c 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1613,7 +1613,8 @@ static void inode_free_by_rcu(struct rcu_head *head)
>   */
>  void security_inode_free(struct inode *inode)
>  {
> -       call_void_hook(inode_free_security, inode);
> +       struct rcu_head *inode_blob = inode->i_security;
> +
>         /*
>          * The inode may still be referenced in a path walk and
>          * a call to security_inode_permission() can be made
> @@ -1623,9 +1624,11 @@ void security_inode_free(struct inode *inode)
>          * leave the current inode->i_security pointer intact.
>          * The inode will be freed after the RCU grace period too.
>          */
> -       if (inode->i_security)
> -               call_rcu((struct rcu_head *)inode->i_security,
> -                        inode_free_by_rcu);
> +       if (inode_blob) {
> +               call_void_hook(inode_free_security, inode);
> +               inode->i_security = NULL;
> +               call_rcu(inode_blob, inode_free_by_rcu);

See my response to Mickaël, unless we can get some guidance from the
VFS folks on the possibility of calling security_inode_free() once
when the inode has already been marked for death and is no longer in
use, I'd rather see us split the LSM implementation into two hooks,
something like this pseudo code (very hand-wavy around the rcu_head
inode container bits):

void inode_free_rcu(rhead)
{
  inode = multiple_container_of_magic(rhead);
  /* LSMs can finally free any internal state */
  call_void_hook(inode_free_rcu, inode)
  inode->i_security = NULL;
}

void security_inode_free(inode)
{
  /* LSMs can mark their state as "dead", but perm checks may still happen */
  call_void_hook(inode_free, inode);
  if (inode->i_security)
    call_rcu(inode, inode_free_rcu);
}

> +       }
>  }

--
paul-moore.com

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [syzbot] [lsm?] general protection fault in hook_inode_free_security
  2024-06-27 18:12         ` Kees Cook
  2024-06-27 18:45           ` Paul E. McKenney
  2024-06-27 19:29           ` Paul Moore
@ 2024-07-08 14:02           ` Mickaël Salaün
  2024-07-08 19:25             ` Kees Cook
  2024-07-09  5:12             ` Christian Brauner
  2 siblings, 2 replies; 24+ messages in thread
From: Mickaël Salaün @ 2024-07-08 14:02 UTC (permalink / raw)
  To: Kees Cook
  Cc: Christian Brauner, Steven Rostedt, Paul Moore, Jann Horn,
	Paul E. McKenney, Casey Schaufler, syzbot, jmorris, linux-kernel,
	linux-security-module, serge, syzkaller-bugs, linux-fsdevel

On Thu, Jun 27, 2024 at 11:12:43AM -0700, Kees Cook wrote:
> On Thu, Jun 27, 2024 at 03:34:41PM +0200, Mickaël Salaün wrote:
> > I didn't find specific issues with Landlock's code except the extra
> > check in hook_inode_free_security().  It looks like inode->i_security is
> > a dangling pointer, leading to UAF.
> > 
> > Reading security_inode_free() comments, two things looks weird to me:
> > > /**
> > >  * security_inode_free() - Free an inode's LSM blob
> > >  * @inode: the inode
> > >  *
> > >  * Deallocate the inode security structure and set @inode->i_security to NULL.
> > 
> > I don't see where i_security is set to NULL.
> 
> Yeah, I don't either...
> 
> > >  */
> > > void security_inode_free(struct inode *inode)
> > > {
> > 
> > Shouldn't we add this check here?
> > if (!inode->i_security)
> > 	return;
> 
> Probably, yes. The LSMs that check for NULL i_security in the free hook
> all do so right at the beginning...
> 
> > 
> > > 	call_void_hook(inode_free_security, inode);
> > > 	/*
> > > 	 * The inode may still be referenced in a path walk and
> > > 	 * a call to security_inode_permission() can be made
> > > 	 * after inode_free_security() is called. Ideally, the VFS
> > > 	 * wouldn't do this, but fixing that is a much harder
> > > 	 * job. For now, simply free the i_security via RCU, and
> > > 	 * leave the current inode->i_security pointer intact.
> > > 	 * The inode will be freed after the RCU grace period too.
> > 
> > It's not clear to me why this should be safe if an LSM try to use the
> > partially-freed blob after the hook calls and before the actual blob
> > free.
> 
> Yeah, it's not clear to me what the expected lifetime is here. How is
> inode_permission() being called if all inode reference counts are 0? It
> does seem intentional, though.
> 
> The RCU logic was introduced in commit 3dc91d4338d6 ("SELinux: Fix possible
> NULL pointer dereference in selinux_inode_permission()"), with much
> discussion:
> https://lore.kernel.org/lkml/20140109101932.0508dec7@gandalf.local.home/
> (This commit seems to remove setting "i_security = NULL", though, which
> the comment implies is intended, but then it also seems to depend on
> finding a NULL?)
> 
> LSMs using i_security are:
> 
> security/bpf/hooks.c:   .lbs_inode = sizeof(struct bpf_storage_blob),
> security/integrity/evm/evm_main.c:      .lbs_inode = sizeof(struct evm_iint_cache),
> security/integrity/ima/ima_main.c:      .lbs_inode = sizeof(struct ima_iint_cache *),
> security/landlock/setup.c:      .lbs_inode = sizeof(struct landlock_inode_security),
> security/selinux/hooks.c:       .lbs_inode = sizeof(struct inode_security_struct),
> security/smack/smack_lsm.c:     .lbs_inode = sizeof(struct inode_smack),
> 
> SELinux is still checking for NULL. See selinux_inode() and
> selinux_inode_free_security(), as do bpf_inode() and
> bpf_inode_storage_free(). evm and ima also check for NULL.
> 
> landlock_inode() does not, though.
> 
> Smack doesn't hook the free, but it should still check for NULL, and it's not.
> 
> So I think this needs fixing in Landlock and Smack.
> 
> I kind of think that the LSM infrastructure needs to provide a common
> helper for the "access the blob" action, as we've got it repeated in
> each LSM, and we have 2 implementations that are missing NULL checks...
> 
> > 
> > > 	 */
> > > 	if (inode->i_security)
> > > 		call_rcu((struct rcu_head *)inode->i_security,
> > > 			 inode_free_by_rcu);
> > 
> > And then:
> > inode->i_security = NULL;
> > 
> > But why call_rcu()?  i_security is not protected by RCU barriers.
> 
> I assume it's because security_inode_free() via __destroy_inode() via
> destroy_inode() via evict() via iput_final() via iput() may be running
> in interrupt context?
> 
> But I still don't see where i_security gets set to NULL. This won't fix
> the permissions hook races for Landlock and Smack, but should make
> lifetime a bit more clear?

It should not change anything.  I don't see how inode->i_security can be
NULL and when such an inode can be passed to an LSM hook.

> 
> 
> diff --git a/security/security.c b/security/security.c
> index 9c3fb2f60e2a..a8658ebcaf0c 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1613,7 +1613,8 @@ static void inode_free_by_rcu(struct rcu_head *head)
>   */
>  void security_inode_free(struct inode *inode)
>  {
> -	call_void_hook(inode_free_security, inode);
> +	struct rcu_head *inode_blob = inode->i_security;
> +
>  	/*
>  	 * The inode may still be referenced in a path walk and
>  	 * a call to security_inode_permission() can be made
> @@ -1623,9 +1624,11 @@ void security_inode_free(struct inode *inode)
>  	 * leave the current inode->i_security pointer intact.
>  	 * The inode will be freed after the RCU grace period too.
>  	 */
> -	if (inode->i_security)
> -		call_rcu((struct rcu_head *)inode->i_security,
> -			 inode_free_by_rcu);
> +	if (inode_blob) {
> +		call_void_hook(inode_free_security, inode);
> +		inode->i_security = NULL;

If a path walk is ongoing, couldn't this lead to an LSM's security check
bypass?  Shouldn't we call all the inode_free_security() hooks in
inode_free_by_rcu()?  That would mean to reserve an rcu_head and then
probably use inode->i_rcu instead.

I think your patch is correct though.  Could you please send a full
patch?

> +		call_rcu(inode_blob, inode_free_by_rcu);
> +	}
>  }

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [syzbot] [lsm?] general protection fault in hook_inode_free_security
  2024-06-27 18:28         ` Paul Moore
@ 2024-07-08 14:11           ` Mickaël Salaün
  2024-07-08 22:32             ` Paul Moore
                               ` (2 more replies)
  2024-07-10 12:23           ` Mickaël Salaün
  2024-07-11  0:30           ` Tetsuo Handa
  2 siblings, 3 replies; 24+ messages in thread
From: Mickaël Salaün @ 2024-07-08 14:11 UTC (permalink / raw)
  To: Paul Moore, Christian Brauner, Al Viro
  Cc: Jann Horn, Paul E. McKenney, Casey Schaufler, Kees Cook, syzbot,
	jmorris, linux-kernel, linux-security-module, serge,
	syzkaller-bugs, linux-fsdevel

On Thu, Jun 27, 2024 at 02:28:03PM -0400, Paul Moore wrote:
> On Thu, Jun 27, 2024 at 9:34 AM Mickaël Salaün <mic@digikod.net> wrote:
> >
> > I didn't find specific issues with Landlock's code except the extra
> > check in hook_inode_free_security().  It looks like inode->i_security is
> > a dangling pointer, leading to UAF.
> >
> > Reading security_inode_free() comments, two things looks weird to me:
> >
> > > /**
> > >  * security_inode_free() - Free an inode's LSM blob
> > >  * @inode: the inode
> > >  *
> > >  * Deallocate the inode security structure and set @inode->i_security to NULL.
> >
> > I don't see where i_security is set to NULL.
> 
> The function header comments are known to be a bit suspect, a side
> effect of being detached from the functions for many years, this may
> be one of those cases.  I tried to fix up the really awful ones when I
> moved the comments back, back I didn't have time to go through each
> one in detail.  Patches to correct the function header comments are
> welcome and encouraged! :)
> 
> > >  */
> > > void security_inode_free(struct inode *inode)
> > > {
> >
> > Shouldn't we add this check here?
> > if (!inode->i_security)
> >         return;
> 
> Unless I'm remembering something wrong, I believe we *should* always
> have a valid i_security pointer each time we are called, if not
> something has gone wrong, e.g. the security_inode_free() hook is no
> longer being called from the right place.  If we add a NULL check, we
> should probably have a WARN_ON(), pr_err(), or something similar to
> put some spew on the console/logs.
> 
> All that said, it would be good to hear some confirmation from the VFS
> folks that the security_inode_free() hook is located in a spot such
> that once it exits it's current RCU critical section it is safe to
> release the associated LSM state.
> 
> It's also worth mentioning that while we always allocate i_security in
> security_inode_alloc() right now, I can see a world where we allocate
> the i_security field based on need using the lsm_blob_size info (maybe
> that works today?  not sure how kmem_cache handled 0 length blobs?).
> The result is that there might be a legitimate case where i_security
> is NULL, yet we still want to call into the LSM using the
> inode_free_security() implementation hook.
> 
> > >       call_void_hook(inode_free_security, inode);
> > >       /*
> > >        * The inode may still be referenced in a path walk and
> > >        * a call to security_inode_permission() can be made
> > >        * after inode_free_security() is called. Ideally, the VFS
> > >        * wouldn't do this, but fixing that is a much harder
> > >        * job. For now, simply free the i_security via RCU, and
> > >        * leave the current inode->i_security pointer intact.
> > >        * The inode will be freed after the RCU grace period too.
> >
> > It's not clear to me why this should be safe if an LSM try to use the
> > partially-freed blob after the hook calls and before the actual blob
> > free.
> 
> I had the same thought while looking at this just now.  At least in
> the SELinux case I think this "works" simply because SELinux doesn't
> do much here, it just drops the inode from a SELinux internal list
> (long story) and doesn't actually release any memory or reset the
> inode's SELinux state (there really isn't anything to "free" in the
> SELinux case).  I haven't checked the other LSMs, but they may behave
> similarly.
> 
> We may want (need?) to consider two LSM implementation hooks called
> from within security_inode_free(): the first where the existing
> inode_free_security() implementation hook is called, the second inside
> the inode_free_by_rcu() callback immediately before the i_security
> data is free'd.

Couldn't we call everything in inode_free_by_rcu()?
I replied here instead:
https://lore.kernel.org/r/20240708.hohNgieja0av@digikod.net

> 
> ... or we find a better placement in the VFS for
> security_inode_free(), is that is possible.  It may not be, our VFS
> friends should be able to help here.

Christian? Al?

> 
> > >        */
> > >       if (inode->i_security)
> > >               call_rcu((struct rcu_head *)inode->i_security,
> > >                        inode_free_by_rcu);
> >
> > And then:
> > inode->i_security = NULL;
> 
> According to the comment we may still need i_security for permission
> checks.  See my comment about decomposing the LSM implementation into
> two hooks to better handle this for LSMs.

That was my though too, but maybe not if the path walk just ends early.

> 
> > But why call_rcu()?  i_security is not protected by RCU barriers.
> 
> I believe the issue is that the inode is protected by RCU and that
> affects the lifetime of the i_security blob.

It seems to be related to commit fa0d7e3de6d6 ("fs: icache RCU free
inodes").

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [syzbot] [lsm?] general protection fault in hook_inode_free_security
  2024-07-08 14:02           ` Mickaël Salaün
@ 2024-07-08 19:25             ` Kees Cook
  2024-07-09  5:12             ` Christian Brauner
  1 sibling, 0 replies; 24+ messages in thread
From: Kees Cook @ 2024-07-08 19:25 UTC (permalink / raw)
  To: Mickaël Salaün
  Cc: Christian Brauner, Steven Rostedt, Paul Moore, Jann Horn,
	Paul E. McKenney, Casey Schaufler, syzbot, jmorris, linux-kernel,
	linux-security-module, serge, syzkaller-bugs, linux-fsdevel

On Mon, Jul 08, 2024 at 04:02:34PM +0200, Mickaël Salaün wrote:
> I think your patch is correct though.  Could you please send a full
> patch?

Can you take it over? I don't have an immediate reproducer, etc. I think
it'd be best for the changes to go along with fixes for Landlock and Smack.

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [syzbot] [lsm?] general protection fault in hook_inode_free_security
  2024-07-08 14:11           ` Mickaël Salaün
@ 2024-07-08 22:32             ` Paul Moore
  2024-07-09  5:46             ` Christian Brauner
  2024-07-10  5:52             ` Christian Brauner
  2 siblings, 0 replies; 24+ messages in thread
From: Paul Moore @ 2024-07-08 22:32 UTC (permalink / raw)
  To: Mickaël Salaün
  Cc: Christian Brauner, Al Viro, Jann Horn, Paul E. McKenney,
	Casey Schaufler, Kees Cook, syzbot, jmorris, linux-kernel,
	linux-security-module, serge, syzkaller-bugs, linux-fsdevel

On Mon, Jul 8, 2024 at 10:11 AM Mickaël Salaün <mic@digikod.net> wrote:
> On Thu, Jun 27, 2024 at 02:28:03PM -0400, Paul Moore wrote:
> > On Thu, Jun 27, 2024 at 9:34 AM Mickaël Salaün <mic@digikod.net> wrote:
> > >
> > > I didn't find specific issues with Landlock's code except the extra
> > > check in hook_inode_free_security().  It looks like inode->i_security is
> > > a dangling pointer, leading to UAF.
> > >
> > > Reading security_inode_free() comments, two things looks weird to me:
> > >
> > > > /**
> > > >  * security_inode_free() - Free an inode's LSM blob
> > > >  * @inode: the inode
> > > >  *
> > > >  * Deallocate the inode security structure and set @inode->i_security to NULL.
> > >
> > > I don't see where i_security is set to NULL.
> >
> > The function header comments are known to be a bit suspect, a side
> > effect of being detached from the functions for many years, this may
> > be one of those cases.  I tried to fix up the really awful ones when I
> > moved the comments back, back I didn't have time to go through each
> > one in detail.  Patches to correct the function header comments are
> > welcome and encouraged! :)
> >
> > > >  */
> > > > void security_inode_free(struct inode *inode)
> > > > {
> > >
> > > Shouldn't we add this check here?
> > > if (!inode->i_security)
> > >         return;
> >
> > Unless I'm remembering something wrong, I believe we *should* always
> > have a valid i_security pointer each time we are called, if not
> > something has gone wrong, e.g. the security_inode_free() hook is no
> > longer being called from the right place.  If we add a NULL check, we
> > should probably have a WARN_ON(), pr_err(), or something similar to
> > put some spew on the console/logs.
> >
> > All that said, it would be good to hear some confirmation from the VFS
> > folks that the security_inode_free() hook is located in a spot such
> > that once it exits it's current RCU critical section it is safe to
> > release the associated LSM state.
> >
> > It's also worth mentioning that while we always allocate i_security in
> > security_inode_alloc() right now, I can see a world where we allocate
> > the i_security field based on need using the lsm_blob_size info (maybe
> > that works today?  not sure how kmem_cache handled 0 length blobs?).
> > The result is that there might be a legitimate case where i_security
> > is NULL, yet we still want to call into the LSM using the
> > inode_free_security() implementation hook.
> >
> > > >       call_void_hook(inode_free_security, inode);
> > > >       /*
> > > >        * The inode may still be referenced in a path walk and
> > > >        * a call to security_inode_permission() can be made
> > > >        * after inode_free_security() is called. Ideally, the VFS
> > > >        * wouldn't do this, but fixing that is a much harder
> > > >        * job. For now, simply free the i_security via RCU, and
> > > >        * leave the current inode->i_security pointer intact.
> > > >        * The inode will be freed after the RCU grace period too.
> > >
> > > It's not clear to me why this should be safe if an LSM try to use the
> > > partially-freed blob after the hook calls and before the actual blob
> > > free.
> >
> > I had the same thought while looking at this just now.  At least in
> > the SELinux case I think this "works" simply because SELinux doesn't
> > do much here, it just drops the inode from a SELinux internal list
> > (long story) and doesn't actually release any memory or reset the
> > inode's SELinux state (there really isn't anything to "free" in the
> > SELinux case).  I haven't checked the other LSMs, but they may behave
> > similarly.
> >
> > We may want (need?) to consider two LSM implementation hooks called
> > from within security_inode_free(): the first where the existing
> > inode_free_security() implementation hook is called, the second inside
> > the inode_free_by_rcu() callback immediately before the i_security
> > data is free'd.
>
> Couldn't we call everything in inode_free_by_rcu()?

My two concerns would be 1) an LSM needed to mark the inode
immediately (to be fair, I'm not sure how plausible that is, and it
would likely require LSM specific locking/sync) and 2) if an LSM
needed more context than would be valid in the RCU callback case
(although that is questionable, I don't believe any of the current
users need anything like that, e.g. task_struct).  I'm not opposed to
just moving everything to a single hook done when the inode is
actually dropped from memory as long as the current LSMs which make
use of it are okay with that (Landlock, IMA, SELinux).

> > ... or we find a better placement in the VFS for
> > security_inode_free(), is that is possible.  It may not be, our VFS
> > friends should be able to help here.
>
> Christian? Al?

I poked around a bit more and it looks like bcachefs has some error
handling code that frees the inodes synchronously instead of using the
i_callback/inode::free_inode approach; xfs has its own thing too.
Although if it really is just some bcachefs/xfs error handlers maybe
inserting a LSM hook in the proper
the-memory-is-actually-being-dropped code path  (with adjustments for
bcachefs/xfs) isn't too bad.  Once again, we would need to hear from
the VFS folks regarding their thoughts on this, it wouldn't surprise
me if there is some detail missing in the above, or if they are
opposed to this for some other reason not mentioned yet in this
thread.

-- 
paul-moore.com

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [syzbot] [lsm?] general protection fault in hook_inode_free_security
  2024-07-08 14:02           ` Mickaël Salaün
  2024-07-08 19:25             ` Kees Cook
@ 2024-07-09  5:12             ` Christian Brauner
  1 sibling, 0 replies; 24+ messages in thread
From: Christian Brauner @ 2024-07-09  5:12 UTC (permalink / raw)
  To: Mickaël Salaün
  Cc: Kees Cook, Steven Rostedt, Paul Moore, Jann Horn,
	Paul E. McKenney, Casey Schaufler, syzbot, jmorris, linux-kernel,
	linux-security-module, serge, syzkaller-bugs, linux-fsdevel

> bypass?  Shouldn't we call all the inode_free_security() hooks in
> inode_free_by_rcu()?  That would mean to reserve an rcu_head and then
> probably use inode->i_rcu instead.

Note that you can't block in call_rcu(). From a cursory look at the
implementers of the hook it should be fine though.

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [syzbot] [lsm?] general protection fault in hook_inode_free_security
  2024-07-08 14:11           ` Mickaël Salaün
  2024-07-08 22:32             ` Paul Moore
@ 2024-07-09  5:46             ` Christian Brauner
  2024-07-09 23:13               ` Paul Moore
  2024-07-10  5:52             ` Christian Brauner
  2 siblings, 1 reply; 24+ messages in thread
From: Christian Brauner @ 2024-07-09  5:46 UTC (permalink / raw)
  To: Mickaël Salaün
  Cc: Paul Moore, Al Viro, Jann Horn, Paul E. McKenney,
	Casey Schaufler, Kees Cook, syzbot, jmorris, linux-kernel,
	linux-security-module, serge, syzkaller-bugs, linux-fsdevel

> > ... or we find a better placement in the VFS for
> > security_inode_free(), is that is possible.  It may not be, our VFS
> > friends should be able to help here.

The place where you do it currently is pretty good. I don't see an easy
way to call it from somewhere else without forcing every filesystem to
either implement a free_inode or destroy_inode hook.

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [syzbot] [lsm?] general protection fault in hook_inode_free_security
  2024-07-09  5:46             ` Christian Brauner
@ 2024-07-09 23:13               ` Paul Moore
  0 siblings, 0 replies; 24+ messages in thread
From: Paul Moore @ 2024-07-09 23:13 UTC (permalink / raw)
  To: Mickaël Salaün
  Cc: Christian Brauner, Al Viro, Jann Horn, Paul E. McKenney,
	Casey Schaufler, Kees Cook, syzbot, jmorris, linux-kernel,
	linux-security-module, serge, syzkaller-bugs, linux-fsdevel

On Tue, Jul 9, 2024 at 1:47 AM Christian Brauner <brauner@kernel.org> wrote:
> > > ... or we find a better placement in the VFS for
> > > security_inode_free(), is that is possible.  It may not be, our VFS
> > > friends should be able to help here.
>
> The place where you do it currently is pretty good. I don't see an easy
> way to call it from somewhere else without forcing every filesystem to
> either implement a free_inode or destroy_inode hook.

Mickaël, let me play around with some code, if you don't see anything
from me in a day or two, feel free to bug me :)

-- 
paul-moore.com

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [syzbot] [lsm?] general protection fault in hook_inode_free_security
  2024-07-08 14:11           ` Mickaël Salaün
  2024-07-08 22:32             ` Paul Moore
  2024-07-09  5:46             ` Christian Brauner
@ 2024-07-10  5:52             ` Christian Brauner
  2 siblings, 0 replies; 24+ messages in thread
From: Christian Brauner @ 2024-07-10  5:52 UTC (permalink / raw)
  To: Mickaël Salaün
  Cc: Paul Moore, Al Viro, Jann Horn, Paul E. McKenney,
	Casey Schaufler, Kees Cook, syzbot, jmorris, linux-kernel,
	linux-security-module, serge, syzkaller-bugs, linux-fsdevel

On Mon, Jul 08, 2024 at 04:11:41PM GMT, Mickaël Salaün wrote:
> On Thu, Jun 27, 2024 at 02:28:03PM -0400, Paul Moore wrote:
> > On Thu, Jun 27, 2024 at 9:34 AM Mickaël Salaün <mic@digikod.net> wrote:
> > >
> > > I didn't find specific issues with Landlock's code except the extra
> > > check in hook_inode_free_security().  It looks like inode->i_security is
> > > a dangling pointer, leading to UAF.
> > >
> > > Reading security_inode_free() comments, two things looks weird to me:
> > >
> > > > /**
> > > >  * security_inode_free() - Free an inode's LSM blob
> > > >  * @inode: the inode
> > > >  *
> > > >  * Deallocate the inode security structure and set @inode->i_security to NULL.
> > >
> > > I don't see where i_security is set to NULL.
> > 
> > The function header comments are known to be a bit suspect, a side
> > effect of being detached from the functions for many years, this may
> > be one of those cases.  I tried to fix up the really awful ones when I
> > moved the comments back, back I didn't have time to go through each
> > one in detail.  Patches to correct the function header comments are
> > welcome and encouraged! :)
> > 
> > > >  */
> > > > void security_inode_free(struct inode *inode)
> > > > {
> > >
> > > Shouldn't we add this check here?
> > > if (!inode->i_security)
> > >         return;
> > 
> > Unless I'm remembering something wrong, I believe we *should* always
> > have a valid i_security pointer each time we are called, if not
> > something has gone wrong, e.g. the security_inode_free() hook is no
> > longer being called from the right place.  If we add a NULL check, we
> > should probably have a WARN_ON(), pr_err(), or something similar to
> > put some spew on the console/logs.
> > 
> > All that said, it would be good to hear some confirmation from the VFS
> > folks that the security_inode_free() hook is located in a spot such
> > that once it exits it's current RCU critical section it is safe to
> > release the associated LSM state.
> > 
> > It's also worth mentioning that while we always allocate i_security in
> > security_inode_alloc() right now, I can see a world where we allocate
> > the i_security field based on need using the lsm_blob_size info (maybe
> > that works today?  not sure how kmem_cache handled 0 length blobs?).
> > The result is that there might be a legitimate case where i_security
> > is NULL, yet we still want to call into the LSM using the
> > inode_free_security() implementation hook.
> > 
> > > >       call_void_hook(inode_free_security, inode);
> > > >       /*
> > > >        * The inode may still be referenced in a path walk and
> > > >        * a call to security_inode_permission() can be made
> > > >        * after inode_free_security() is called. Ideally, the VFS
> > > >        * wouldn't do this, but fixing that is a much harder
> > > >        * job. For now, simply free the i_security via RCU, and
> > > >        * leave the current inode->i_security pointer intact.
> > > >        * The inode will be freed after the RCU grace period too.
> > >
> > > It's not clear to me why this should be safe if an LSM try to use the
> > > partially-freed blob after the hook calls and before the actual blob
> > > free.
> > 
> > I had the same thought while looking at this just now.  At least in
> > the SELinux case I think this "works" simply because SELinux doesn't
> > do much here, it just drops the inode from a SELinux internal list
> > (long story) and doesn't actually release any memory or reset the
> > inode's SELinux state (there really isn't anything to "free" in the
> > SELinux case).  I haven't checked the other LSMs, but they may behave
> > similarly.
> > 
> > We may want (need?) to consider two LSM implementation hooks called
> > from within security_inode_free(): the first where the existing
> > inode_free_security() implementation hook is called, the second inside
> > the inode_free_by_rcu() callback immediately before the i_security
> > data is free'd.
> 
> Couldn't we call everything in inode_free_by_rcu()?
> I replied here instead:
> https://lore.kernel.org/r/20240708.hohNgieja0av@digikod.net
> 
> > 
> > ... or we find a better placement in the VFS for
> > security_inode_free(), is that is possible.  It may not be, our VFS
> > friends should be able to help here.
> 
> Christian? Al?
> 
> > 
> > > >        */
> > > >       if (inode->i_security)
> > > >               call_rcu((struct rcu_head *)inode->i_security,
> > > >                        inode_free_by_rcu);
> > >
> > > And then:
> > > inode->i_security = NULL;
> > 
> > According to the comment we may still need i_security for permission
> > checks.  See my comment about decomposing the LSM implementation into
> > two hooks to better handle this for LSMs.
> 
> That was my though too, but maybe not if the path walk just ends early.
> 
> > 
> > > But why call_rcu()?  i_security is not protected by RCU barriers.
> > 
> > I believe the issue is that the inode is protected by RCU and that
> > affects the lifetime of the i_security blob.
> 
> It seems to be related to commit fa0d7e3de6d6 ("fs: icache RCU free
> inodes").

Path walking can happen in RCU- and ref-mode. We'll try RCU first and
fallback to ref mode if it fails with ECHILD. In RCU mode it's possible
to call inode_permission() on nd->inode (e.g., in may_lookup()) while
dentry->d_inode has changed (rename or similar)). This would be detected
later when we validate that the parent child relationship hasn't changed
while we were under RCU and would force a drop out of RCU into ref
walking mode.

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [syzbot] [lsm?] general protection fault in hook_inode_free_security
  2024-06-27 18:28         ` Paul Moore
  2024-07-08 14:11           ` Mickaël Salaün
@ 2024-07-10 12:23           ` Mickaël Salaün
  2024-07-10 13:53             ` Mickaël Salaün
  2024-07-10 21:22             ` Paul Moore
  2024-07-11  0:30           ` Tetsuo Handa
  2 siblings, 2 replies; 24+ messages in thread
From: Mickaël Salaün @ 2024-07-10 12:23 UTC (permalink / raw)
  To: Paul Moore
  Cc: Jann Horn, Christian Brauner, Paul E. McKenney, Casey Schaufler,
	Kees Cook, syzbot, jmorris, linux-kernel, linux-security-module,
	serge, syzkaller-bugs, linux-fsdevel, Mimi Zohar, Roberto Sassu

On Thu, Jun 27, 2024 at 02:28:03PM -0400, Paul Moore wrote:
> On Thu, Jun 27, 2024 at 9:34 AM Mickaël Salaün <mic@digikod.net> wrote:
> >
> > I didn't find specific issues with Landlock's code except the extra
> > check in hook_inode_free_security().  It looks like inode->i_security is
> > a dangling pointer, leading to UAF.
> >
> > Reading security_inode_free() comments, two things looks weird to me:
> >
> > > /**
> > >  * security_inode_free() - Free an inode's LSM blob
> > >  * @inode: the inode
> > >  *
> > >  * Deallocate the inode security structure and set @inode->i_security to NULL.
> >
> > I don't see where i_security is set to NULL.
> 
> The function header comments are known to be a bit suspect, a side
> effect of being detached from the functions for many years, this may
> be one of those cases.  I tried to fix up the really awful ones when I
> moved the comments back, back I didn't have time to go through each
> one in detail.  Patches to correct the function header comments are
> welcome and encouraged! :)
> 
> > >  */
> > > void security_inode_free(struct inode *inode)
> > > {
> >
> > Shouldn't we add this check here?
> > if (!inode->i_security)
> >         return;
> 
> Unless I'm remembering something wrong, I believe we *should* always
> have a valid i_security pointer each time we are called, if not
> something has gone wrong, e.g. the security_inode_free() hook is no
> longer being called from the right place.  If we add a NULL check, we
> should probably have a WARN_ON(), pr_err(), or something similar to
> put some spew on the console/logs.
> 
> All that said, it would be good to hear some confirmation from the VFS
> folks that the security_inode_free() hook is located in a spot such
> that once it exits it's current RCU critical section it is safe to
> release the associated LSM state.
> 
> It's also worth mentioning that while we always allocate i_security in
> security_inode_alloc() right now, I can see a world where we allocate
> the i_security field based on need using the lsm_blob_size info (maybe
> that works today?  not sure how kmem_cache handled 0 length blobs?).
> The result is that there might be a legitimate case where i_security
> is NULL, yet we still want to call into the LSM using the
> inode_free_security() implementation hook.

Looking at existing LSM implementations, even if some helpers (e.g.
selinux_inode) return NULL if inode->i_security is NULL, this may not be
handled by the callers.  For instance, SELinux always dereferences the
blob pointer in the security_inode_permission() hook.  EVM seems to be
the only one properly handling this case.

Shouldn't we remove all inode->i_security checks and assume it is always
set?  This is currently the case anyway, but it would be clearer this
way and avoid false sense of security (with useless checks).

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [syzbot] [lsm?] general protection fault in hook_inode_free_security
  2024-07-10 12:23           ` Mickaël Salaün
@ 2024-07-10 13:53             ` Mickaël Salaün
  2024-07-10 21:22             ` Paul Moore
  1 sibling, 0 replies; 24+ messages in thread
From: Mickaël Salaün @ 2024-07-10 13:53 UTC (permalink / raw)
  To: Paul Moore
  Cc: Jann Horn, Christian Brauner, Paul E. McKenney, Casey Schaufler,
	Kees Cook, syzbot, jmorris, linux-kernel, linux-security-module,
	serge, syzkaller-bugs, linux-fsdevel, Mimi Zohar, Roberto Sassu

On Wed, Jul 10, 2024 at 02:23:23PM +0200, Mickaël Salaün wrote:
> On Thu, Jun 27, 2024 at 02:28:03PM -0400, Paul Moore wrote:
> > On Thu, Jun 27, 2024 at 9:34 AM Mickaël Salaün <mic@digikod.net> wrote:
> > >
> > > I didn't find specific issues with Landlock's code except the extra
> > > check in hook_inode_free_security().  It looks like inode->i_security is
> > > a dangling pointer, leading to UAF.
> > >
> > > Reading security_inode_free() comments, two things looks weird to me:
> > >
> > > > /**
> > > >  * security_inode_free() - Free an inode's LSM blob
> > > >  * @inode: the inode
> > > >  *
> > > >  * Deallocate the inode security structure and set @inode->i_security to NULL.
> > >
> > > I don't see where i_security is set to NULL.
> > 
> > The function header comments are known to be a bit suspect, a side
> > effect of being detached from the functions for many years, this may
> > be one of those cases.  I tried to fix up the really awful ones when I
> > moved the comments back, back I didn't have time to go through each
> > one in detail.  Patches to correct the function header comments are
> > welcome and encouraged! :)
> > 
> > > >  */
> > > > void security_inode_free(struct inode *inode)
> > > > {
> > >
> > > Shouldn't we add this check here?
> > > if (!inode->i_security)
> > >         return;
> > 
> > Unless I'm remembering something wrong, I believe we *should* always
> > have a valid i_security pointer each time we are called, if not
> > something has gone wrong, e.g. the security_inode_free() hook is no
> > longer being called from the right place.  If we add a NULL check, we
> > should probably have a WARN_ON(), pr_err(), or something similar to
> > put some spew on the console/logs.
> > 
> > All that said, it would be good to hear some confirmation from the VFS
> > folks that the security_inode_free() hook is located in a spot such
> > that once it exits it's current RCU critical section it is safe to
> > release the associated LSM state.
> > 
> > It's also worth mentioning that while we always allocate i_security in
> > security_inode_alloc() right now, I can see a world where we allocate
> > the i_security field based on need using the lsm_blob_size info (maybe
> > that works today?  not sure how kmem_cache handled 0 length blobs?).
> > The result is that there might be a legitimate case where i_security
> > is NULL, yet we still want to call into the LSM using the
> > inode_free_security() implementation hook.
> 
> Looking at existing LSM implementations, even if some helpers (e.g.
> selinux_inode) return NULL if inode->i_security is NULL, this may not be
> handled by the callers.  For instance, SELinux always dereferences the
> blob pointer in the security_inode_permission() hook.  EVM seems to be
> the only one properly handling this case.
> 
> Shouldn't we remove all inode->i_security checks and assume it is always
> set?  This is currently the case anyway, but it would be clearer this
> way and avoid false sense of security (with useless checks).

A patch was sent to do this kind of check:
https://lore.kernel.org/r/20140109101932.0508dec7@gandalf.local.home
but the applied commit 3dc91d4338d6 ("SELinux: Fix possible NULL pointer
dereference in selinux_inode_permission()") didn't include the
i_security check.

Since this commit, the security_inode_free()'s header comment is no
longer correct because i_security is no longer set to NULL.

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [syzbot] [lsm?] general protection fault in hook_inode_free_security
  2024-07-10 12:23           ` Mickaël Salaün
  2024-07-10 13:53             ` Mickaël Salaün
@ 2024-07-10 21:22             ` Paul Moore
  1 sibling, 0 replies; 24+ messages in thread
From: Paul Moore @ 2024-07-10 21:22 UTC (permalink / raw)
  To: Mickaël Salaün
  Cc: Jann Horn, Christian Brauner, Paul E. McKenney, Casey Schaufler,
	Kees Cook, syzbot, jmorris, linux-kernel, linux-security-module,
	serge, syzkaller-bugs, linux-fsdevel, Mimi Zohar, Roberto Sassu

On Wed, Jul 10, 2024 at 8:23 AM Mickaël Salaün <mic@digikod.net> wrote:
> On Thu, Jun 27, 2024 at 02:28:03PM -0400, Paul Moore wrote:
> > On Thu, Jun 27, 2024 at 9:34 AM Mickaël Salaün <mic@digikod.net> wrote:
> > >
> > > I didn't find specific issues with Landlock's code except the extra
> > > check in hook_inode_free_security().  It looks like inode->i_security is
> > > a dangling pointer, leading to UAF.
> > >
> > > Reading security_inode_free() comments, two things looks weird to me:
> > >
> > > > /**
> > > >  * security_inode_free() - Free an inode's LSM blob
> > > >  * @inode: the inode
> > > >  *
> > > >  * Deallocate the inode security structure and set @inode->i_security to NULL.
> > >
> > > I don't see where i_security is set to NULL.
> >
> > The function header comments are known to be a bit suspect, a side
> > effect of being detached from the functions for many years, this may
> > be one of those cases.  I tried to fix up the really awful ones when I
> > moved the comments back, back I didn't have time to go through each
> > one in detail.  Patches to correct the function header comments are
> > welcome and encouraged! :)
> >
> > > >  */
> > > > void security_inode_free(struct inode *inode)
> > > > {
> > >
> > > Shouldn't we add this check here?
> > > if (!inode->i_security)
> > >         return;
> >
> > Unless I'm remembering something wrong, I believe we *should* always
> > have a valid i_security pointer each time we are called, if not
> > something has gone wrong, e.g. the security_inode_free() hook is no
> > longer being called from the right place.  If we add a NULL check, we
> > should probably have a WARN_ON(), pr_err(), or something similar to
> > put some spew on the console/logs.
> >
> > All that said, it would be good to hear some confirmation from the VFS
> > folks that the security_inode_free() hook is located in a spot such
> > that once it exits it's current RCU critical section it is safe to
> > release the associated LSM state.
> >
> > It's also worth mentioning that while we always allocate i_security in
> > security_inode_alloc() right now, I can see a world where we allocate
> > the i_security field based on need using the lsm_blob_size info (maybe
> > that works today?  not sure how kmem_cache handled 0 length blobs?).
> > The result is that there might be a legitimate case where i_security
> > is NULL, yet we still want to call into the LSM using the
> > inode_free_security() implementation hook.
>
> Looking at existing LSM implementations, even if some helpers (e.g.
> selinux_inode) return NULL if inode->i_security is NULL, this may not be
> handled by the callers.  For instance, SELinux always dereferences the
> blob pointer in the security_inode_permission() hook.

Since SELinux requires space in inode->i_security there should never
be a case where SELinux is enabled and we don't allocate a blob for
inode->i_security.

> Shouldn't we remove all inode->i_security checks and assume it is always
> set?  This is currently the case anyway, but it would be clearer this
> way and avoid false sense of security (with useless checks).

It would be interesting to draft a patch to do that and make sure
everything still works; it *should*, but I'd want to see that change
get some good testing :)

Keep in mind, this still doesn't mean that an LSM is required to use
any space in inode->i_security if it wants to use the inode hooks.

-- 
paul-moore.com

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [syzbot] [lsm?] general protection fault in hook_inode_free_security
  2024-06-27 18:28         ` Paul Moore
  2024-07-08 14:11           ` Mickaël Salaün
  2024-07-10 12:23           ` Mickaël Salaün
@ 2024-07-11  0:30           ` Tetsuo Handa
  2024-07-11 14:06             ` Paul Moore
  2 siblings, 1 reply; 24+ messages in thread
From: Tetsuo Handa @ 2024-07-11  0:30 UTC (permalink / raw)
  To: Paul Moore, Mickaël Salaün
  Cc: Jann Horn, Christian Brauner, Paul E. McKenney, Casey Schaufler,
	Kees Cook, syzbot, jmorris, linux-kernel, linux-security-module,
	serge, syzkaller-bugs, linux-fsdevel

On 2024/06/28 3:28, Paul Moore wrote:
> It's also worth mentioning that while we always allocate i_security in
> security_inode_alloc() right now, I can see a world where we allocate
> the i_security field based on need using the lsm_blob_size info (maybe
> that works today?  not sure how kmem_cache handled 0 length blobs?).
> The result is that there might be a legitimate case where i_security
> is NULL, yet we still want to call into the LSM using the
> inode_free_security() implementation hook.

As a LKM-based LSM user, I don't like dependency on the lsm_blob_size info.

Since LKM-based LSM users cannot use lsm_blob_size due to __ro_after_init,
LKM-based LSM users depend on individual LSM hooks being called even if
i_security is NULL. How do we provide hooks for AV/EDR which cannot be 
built into vmlinux (due to distributor's support policy) ? They cannot be
benefited from infrastructure-managed security blobs.


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [syzbot] [lsm?] general protection fault in hook_inode_free_security
  2024-07-11  0:30           ` Tetsuo Handa
@ 2024-07-11 14:06             ` Paul Moore
  0 siblings, 0 replies; 24+ messages in thread
From: Paul Moore @ 2024-07-11 14:06 UTC (permalink / raw)
  To: Tetsuo Handa
  Cc: Mickaël Salaün, Jann Horn, Christian Brauner,
	Paul E. McKenney, Casey Schaufler, Kees Cook, syzbot, jmorris,
	linux-kernel, linux-security-module, serge, syzkaller-bugs,
	linux-fsdevel

On Wed, Jul 10, 2024 at 8:32 PM Tetsuo Handa
<penguin-kernel@i-love.sakura.ne.jp> wrote:
> On 2024/06/28 3:28, Paul Moore wrote:
> > It's also worth mentioning that while we always allocate i_security in
> > security_inode_alloc() right now, I can see a world where we allocate
> > the i_security field based on need using the lsm_blob_size info (maybe
> > that works today?  not sure how kmem_cache handled 0 length blobs?).
> > The result is that there might be a legitimate case where i_security
> > is NULL, yet we still want to call into the LSM using the
> > inode_free_security() implementation hook.
>
> As a LKM-based LSM user, I don't like dependency on the lsm_blob_size info.
>
> Since LKM-based LSM users cannot use lsm_blob_size due to __ro_after_init,
> LKM-based LSM users depend on individual LSM hooks being called even if
> i_security is NULL. How do we provide hooks for AV/EDR which cannot be
> built into vmlinux (due to distributor's support policy) ? They cannot be
> benefited from infrastructure-managed security blobs.

As stated many times in the past, the LSM framework as well as the
Linux kernel in general, does not provide the same level of
consideration to out-of-tree code that it does to upstream, mainline
code.  My policy on this remains the same as last time we talked:
while I have no goal to make things difficult for out-of-tree code, I
will not sacrifice the continued development and maintenance of
existing upstream code in favor of out-of-tree code.

-- 
paul-moore.com

^ permalink raw reply	[flat|nested] 24+ messages in thread

end of thread, other threads:[~2024-07-11 14:06 UTC | newest]

Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-05-08 19:32 [syzbot] [lsm?] general protection fault in hook_inode_free_security syzbot
2024-05-10  0:01 ` Paul Moore
2024-05-15 15:12   ` Mickaël Salaün
2024-05-16  7:31     ` Mickaël Salaün
2024-05-16 13:07       ` Mathieu Desnoyers
2024-06-27 13:33         ` Mickaël Salaün
2024-06-27 13:34       ` Mickaël Salaün
2024-06-27 18:12         ` Kees Cook
2024-06-27 18:45           ` Paul E. McKenney
2024-06-27 19:29           ` Paul Moore
2024-07-08 14:02           ` Mickaël Salaün
2024-07-08 19:25             ` Kees Cook
2024-07-09  5:12             ` Christian Brauner
2024-06-27 18:28         ` Paul Moore
2024-07-08 14:11           ` Mickaël Salaün
2024-07-08 22:32             ` Paul Moore
2024-07-09  5:46             ` Christian Brauner
2024-07-09 23:13               ` Paul Moore
2024-07-10  5:52             ` Christian Brauner
2024-07-10 12:23           ` Mickaël Salaün
2024-07-10 13:53             ` Mickaël Salaün
2024-07-10 21:22             ` Paul Moore
2024-07-11  0:30           ` Tetsuo Handa
2024-07-11 14:06             ` Paul Moore

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.