On 1/11/2019 2:43 PM, Casey Schaufler wrote: > On 1/11/2019 2:30 PM, John Johansen wrote: >> On 1/11/19 2:11 PM, Casey Schaufler wrote: >>> On 1/11/2019 1:43 AM, syzbot wrote: >>>> Hello, >>>> >>>> syzbot found the following crash on: >>>> >>>> HEAD commit: b808822a75a3 Add linux-next specific files for 20190111 >>>> git tree: linux-next >>>> console output: https://syzkaller.appspot.com/x/log.txt?x=179c22f7400000 >>>> kernel config: https://syzkaller.appspot.com/x/.config?x=c052ead0aed5001b >>>> dashboard link: >>>> https://syzkaller.appspot.com/bug?extid=69ca07954461f189e808 >>>> compiler: gcc (GCC) 9.0.0 20181231 (experimental) >>>> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=162d947f400000 >>>> C reproducer: https://syzkaller.appspot.com/x/repro.c?x=139f6c37400000 >>>> >>>> IMPORTANT: if you fix the bug, please add the following tag to the commit: >>>> Reported-by: [email protected] >>>> >>>> ------------[ cut here ]------------ >>>> AppArmor WARN cred_label: ((!blob)): >>>> WARNING: CPU: 0 PID: 0 at security/apparmor/include/cred.h:30 cred_label >>>> security/apparmor/include/cred.h:30 [inline] >>>> WARNING: CPU: 0 PID: 0 at security/apparmor/include/cred.h:30 >>>> apparmor_cred_free+0x12f/0x1a0 security/apparmor/lsm.c:62 >>>> Kernel panic - not syncing: panic_on_warn set ... >>>> CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.0.0-rc1-next-20190111 #10 >>>> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS >>>> Google 01/01/2011 >>>> Call Trace: >>>> <IRQ> >>>> __dump_stack lib/dump_stack.c:77 [inline] >>>> dump_stack+0x1db/0x2d0 lib/dump_stack.c:113 >>>> panic+0x2cb/0x65c kernel/panic.c:214 >>>> __warn.cold+0x20/0x48 kernel/panic.c:571 >>>> report_bug+0x263/0x2b0 lib/bug.c:186 >>>> fixup_bug arch/x86/kernel/traps.c:178 [inline] >>>> fixup_bug arch/x86/kernel/traps.c:173 [inline] >>>> do_error_trap+0x11b/0x200 arch/x86/kernel/traps.c:271 >>>> do_invalid_op+0x37/0x50 arch/x86/kernel/traps.c:290 >>>> invalid_op+0x14/0x20 arch/x86/entry/entry_64.S:973 >>>> RIP: 0010:cred_label security/apparmor/include/cred.h:30 [inline] >>>> RIP: 0010:apparmor_cred_free+0x12f/0x1a0 security/apparmor/lsm.c:62 >>>> Code: 7c 88 48 c7 c7 00 d0 7c 88 e8 fd 70 f2 fd 0f 0b eb a9 e8 54 3f 29 fe >>>> 48 c7 c6 c0 df 7c 88 48 c7 c7 00 d0 7c 88 e8 e1 70 f2 fd <0f> 0b 48 b8 00 >>>> 00 00 00 00 fc ff df 80 38 00 75 4a 4c 8b 2c 25 00 >>>> RSP: 0018:ffff8880ae6079f8 EFLAGS: 00010286 >>>> RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000 >>>> RDX: 0000000000000100 RSI: ffffffff81687fa6 RDI: 0000000000000006 >>>> RBP: ffff8880ae607a18 R08: ffffffff8987dec0 R09: 0000000000000000 >>>> R10: 0000000000000000 R11: 0000000000000000 R12: ffff8880a86b3100 >>>> R13: ffff8880a86b3100 R14: ffff8880a86b3188 R15: dffffc0000000000 >>>> security_cred_free+0x4b/0xf0 security/security.c:1490 >>> The obvious thing to do is put a check in security_cred_free >>> for a NULL cred->security, in which case the LSM hooks >>> wouldn't get called. >> Right, but the question is should we? To my thinking we shouldn't >> ever have a cred without cred->security, unless the cred was >> allocated but a later step in its construction, say allocating >> ->security failed. > If allocating ->security fails in security_cred_alloc_blank() > or security_prepare_creds() you don't have to do anything but > fail because the LSM hooks are not called before the allocation. > >> In which case I'd rather see the cred directly freed and not >> call into security_cred_free() as I like being able to detect >> corrupt creds. > I think we need to look for some bit of code that's setting > cred->security to NULL inappropriately.
If security_cred_alloc_blank() fails for lack of memory in cred_alloc_blank() abort_creds() will be called. This in turn calls put_cred() and put_cred_rcu(), which will call security_cred_free() with ->security set to NULL. put_cred_rcu() is the only caller of security_cred_free(). The ->security == NULL check can be in either put_cred_rcu() or in security_cred_free(). I suggest the latter as the cleanest option.
