Re: kernel BUG at kernel/cred.c:434!

Re: kernel BUG at kernel/cred.c:434!

[Casey Schaufler]

On 4/11/2019 11:21 PM, chengjian (D) wrote:

Added LSM and SELinux lists.


> Hi.
>
>
> syzkaller reported the following BUG:
>
> [   73.146973] kernel BUG at kernel/cred.c:434!
> [   73.150231] invalid opcode: 0000 [#1] SMP KASAN PTI
> [   73.151928] CPU: 2 PID: 4058 Comm: syz-executor.6 Not tainted 
> 5.1.0-rc4-00062-g2d06b235815e-dirty #2
> [   73.155174] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), 
> BIOS rel-1.12.1-0-ga5cab58e9a3f-prebuilt.qemu.org 04/01/2014
> [   73.159798] RIP: 0010:commit_creds+0xadb/0xe50
> [   73.161426] Code: 8b 5b 20 48 c1 ea 03 0f b6 04 02 84 c0 74 08 3c 
> 03 0f 8e 06 03 00 00 39 5d 20 0f 85 ff fa ff ff e9 0c fb ff ff e8 05 
> a2 25 00 <0f> 0b 48 c7 c7 80 56 c0 83 e8 95 22 b1 00 e8 f2 a1 25 00 0f 
> 0b 48
> [   73.167852] RSP: 0000:ffff88836e65f5d0 EFLAGS: 00010293
> [   73.169636] RAX: ffff8883767b0000 RBX: ffff88837f111300 RCX: 
> ffffffff8124b5db
> [   73.171962] RDX: 0000000000000000 RSI: ffffffff83c9b140 RDI: 
> ffff88837f111300
> [   73.174310] RBP: ffff888376610400 R08: 0000000000000000 R09: 
> 0000000000000004
> [   73.176646] R10: 0000000000000001 R11: ffffed107c655acf R12: 
> ffff8883767b0000
> [   73.178527] Process accounting resumed
> [   73.179021] R13: ffff88837f111900 R14: ffff88837f111300 R15: 
> ffff8883767b0ac0
> [   73.179029] FS:  00007f2d207f9700(0000) GS:ffff8883e3280000(0000) 
> knlGS:0000000000000000
> [   73.179034] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [   73.179039] CR2: 00007f1500bd36c0 CR3: 00000003df304003 CR4: 
> 00000000000206e0
> [   73.179047] Call Trace:
> [   73.190461]  selinux_setprocattr+0x2ea/0x8f0
> [   73.191925]  ? ptrace_parent_sid+0x530/0x530
> [   73.193436]  ? proc_pid_attr_write+0x185/0x5a0
> [   73.194967]  security_setprocattr+0xa1/0x100
> [   73.196408]  proc_pid_attr_write+0x307/0x5a0
> [   73.197869]  ? mem_read+0x40/0x40
> [   73.199013]  __vfs_write+0x81/0x100
> [   73.200222]  __kernel_write+0xf8/0x330
> [   73.201562]  do_acct_process+0xca5/0x1340
> [   73.202969]  ? __ia32_sys_acct+0x1e0/0x1e0
> [   73.204498]  ? find_held_lock+0x2f/0x1e0
> [   73.205857]  ? rcu_irq_exit+0xec/0x2c0
> [   73.207160]  ? lock_downgrade+0x630/0x630
> [   73.208541]  acct_pin_kill+0x63/0x150
> [   73.209816]  pin_kill+0x16d/0x7c0
> [   73.210934]  ? lockdep_hardirqs_on+0x5e0/0x5e0
> [   73.212452]  ? xas_start+0x155/0x510
> [   73.213705]  ? pin_insert+0x50/0x50
> [   73.214903]  ? finish_wait+0x270/0x270
> [   73.216213]  ? cpumask_next+0x57/0x90
> [   73.217442]  ? mnt_pin_kill+0x68/0x1d0
> [   73.218851]  mnt_pin_kill+0x68/0x1d0
> [   73.220398]  cleanup_mnt+0x11b/0x150
> [   73.221970]  task_work_run+0x136/0x1b0
> [   73.223427]  do_exit+0x830/0x2ca0
> [   73.224586]  ? trace_hardirqs_off+0x3b/0x180
> [   73.226088]  ? mm_update_next_owner+0x6a0/0x6a0
> [   73.227622]  ? find_held_lock+0x2f/0x1e0
> [   73.228954]  ? get_signal+0x2cf/0x1c00
> [   73.230236]  ? lock_downgrade+0x630/0x630
> [   73.231628]  ? rwlock_bug.part.0+0x90/0x90
> [   73.233020]  do_group_exit+0x106/0x2f0
> [   73.234330]  get_signal+0x325/0x1c00
> [   73.235571]  do_signal+0x97/0x1670
> [   73.236739]  ? do_send_specific+0x12d/0x220
> [   73.238213]  ? lock_downgrade+0x630/0x630
> [   73.239566]  ? setup_sigcontext+0x820/0x820
> [   73.240982]  ? check_kill_permission+0x4a/0x510
> [   73.242509]  ? do_send_specific+0x156/0x220
> [   73.243905]  ? do_tkill+0x1c4/0x260
> [   73.245081]  ? do_send_specific+0x220/0x220
> [   73.246514]  ? trace_hardirqs_on_thunk+0x1a/0x1c
> [   73.248061]  ? exit_to_usermode_loop+0x97/0x1d0
> [   73.249619]  exit_to_usermode_loop+0x108/0x1d0
> [   73.251129]  do_syscall_64+0x461/0x580
> [   73.252454]  entry_SYSCALL_64_after_hwframe+0x49/0xbe
> [   73.254219] RIP: 0033:0x462eb9
> [   73.255327] Code: Bad RIP value.
> [   73.256539] RSP: 002b:00007f2d207f8c58 EFLAGS: 00000246 ORIG_RAX: 
> 00000000000000c8
> [   73.259454] RAX: 0000000000000000 RBX: 000000000073bf00 RCX: 
> 0000000000462eb9
> [   73.262309] RDX: 0000000000000000 RSI: 000000000000001e RDI: 
> 0000000000000005
> [   73.265064] RBP: 0000000000000002 R08: 0000000000000000 R09: 
> 0000000000000000
> [   73.267774] R10: 0000000000000000 R11: 0000000000000246 R12: 
> 00007f2d207f96bc
> [   73.270546] R13: 00000000004c5509 R14: 00000000007042f0 R15: 
> 00000000ffffffff
> [   73.273542] Modules linked in:
> [   73.274670] Dumping ftrace buffer:
> [   73.275852]    (ftrace buffer empty)
> [   73.277187] ---[ end trace dde36a95f458175d ]---
> [   73.278834] RIP: 0010:commit_creds+0xadb/0xe50
> [   73.280549] Code: 8b 5b 20 48 c1 ea 03 0f b6 04 02 84 c0 74 08 3c 
> 03 0f 8e 06 03 00 00 39 5d 20 0f 85 ff fa ff ff e9 0c fb ff ff e8 05 
> a2 25 00 <0f> 0b 48 c7 c7 80 56 c0 83 e8 95 22 b1 00 e8 f2 a1 25 00 0f 
> 0b 48
> [   73.287090] RSP: 0000:ffff88836e65f5d0 EFLAGS: 00010293
> [   73.288917] RAX: ffff8883767b0000 RBX: ffff88837f111300 RCX: 
> ffffffff8124b5db
> [   73.291390] RDX: 0000000000000000 RSI: ffffffff83c9b140 RDI: 
> ffff88837f111300
> [   73.293864] RBP: ffff888376610400 R08: 0000000000000000 R09: 
> 0000000000000004
> [   73.296370] R10: 0000000000000001 R11: ffffed107c655acf R12: 
> ffff8883767b0000
> [   73.299275] R13: ffff88837f111900 R14: ffff88837f111300 R15: 
> ffff8883767b0ac0
> [   73.301351] Process accounting resumed
> [   73.301822] FS:  00007f2d207f9700(0000) GS:ffff8883e3280000(0000) 
> knlGS:0000000000000000
> [   73.301827] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [   73.301832] CR2: 0000000000462e8f CR3: 0000000003a18002 CR4: 
> 00000000000206e0
> [   73.301850] Kernel panic - not syncing: Fatal exception
> [   73.310719] Process accounting resumed
> [   73.311515] Process accounting resumed
> [   73.318916] Dumping ftrace buffer:
> [   73.318921]    (ftrace buffer empty)
> [   73.318945] Kernel Offset: disabled
> [   73.328061] Rebooting in 10 seconds..
>
>
> 425 int commit_creds(struct cred *new)
> 426 {
> 427         struct task_struct *task = current;
> 428         const struct cred *old = task->real_cred;
> 429
> 430         kdebug("commit_creds(%p{%d,%d})", new,
> 431                atomic_read(&new->usage),
> 432                read_cred_subscribers(new));
> 433
> 434         BUG_ON(task->cred != old);  // BUG here
>
>
> I find that the call chain which triggered the BUG is :
>
> do_exit
>     |-=> acct_process
>     |    -=> do_acct_process
>     |        -=> orig_cred = override_creds(file->f_cred); // cred = 
> ffff8883c1878900/real_cred = ffff8883c1878900
>     |        -=>  if (file_start_write_trylock(file)) 
> {__kernel_write(file, &ac, sizeof(acct_t), &pos);}
>     |               -=> __kernel_write+0xf8/0x330
>     |                    -=> __vfs_write+0x81/0x100
>     |                           -=> proc_pid_attr_write+0x307/0x5a0
>     |                                -=> security_setprocattr+0xa1/0x100
>     | -=>selinux_setprocattr+0x2ea/0x8f0
>     | -=>commit_creds+0xd97/0x1080  // cred = 
> ffff888379a79c00/real_cred = ffff888379a79c00
>     |       -=> revert_creds(orig_cred);   // cred = 
> ffff8883c1878900/real_cred = ffff888379a79c00
>     |-=> task_work_run
>            -=> cleanup_mnt+0x11b/0x150
>                 -=> mnt_pin_kill+0x68/0x1d0
>                     -=> pin_kill+0x16d/0x7c0
>                     -=> acct_pin_kill+0x2e/0x100
>                     -=> do_acct_process+0x1a0/0x1340
>                           -=> override_creds+0x18a/0x1c0   // cred = 
> ffff8883c1878900/real_cred = ffff888379a79c00
>                           -=>  if (file_start_write_trylock(file)) 
> {__kernel_write(file, &ac, sizeof(acct_t), &pos);}
>                                     -=>   ...... 
> -=>commit_creds+0xd97/0x1080   // new = ffff888379a6bb00, cred = 
> ffff8883c1878900, real_ceed = ffff888379a79c00 BUG here
>                           -=> revert_creds(orig_cred);
>
>
> Syzkaller Report Testcase:
>
> cat crash.log
>
> 04:05:28 executing program 3:
> clone(0x24100, 0x0, 0x0, 0x0, 0x0)
> r0 = gettid()
> perf_event_open(&(0x7f00000000c0)={0x2, 0x70, 0x4, 0x2, 0x0, 0x0, 0x0, 
> 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 
> 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 
> 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, @perf_bp={0x0}}, 0x0, 
> 0xffffffffffffffff, 0xffffffffffffffff, 0x0)
> acct(&(0x7f0000000000)='./file0\x00')
> r1 = openat$smack_thread_current(0xffffffffffffff9c, 
> &(0x7f0000000000)='/proc/thread-self/attr/current\x00', 0x2, 0x0)
> fcntl$setown(r1, 0x8, r0)
> rt_tgsigqueueinfo(r0, r0, 0x1e, &(0x7f00000002c0))
> tkill(r0, 0x1e)
>
>
> Reproduce this BUG:
> ./syz-execprog -executor=./syz-executor -repeat=0 -procs=16 -cover=0 
> ./crash.log
>
>
> Can anyone help me ?
>
>
> Thanks,
>
>         Cheng Jian.
>
>

Re: kernel BUG at kernel/cred.c:434!

[Oleg Nesterov]

Well, acct("/proc/self/attr/current") doesn't look like a good idea, but I do
not know where should we put the additional check... And probably
"echo /proc/self/attr/current > /proc/sys/kernel/core_pattern" can hit the
same problem, do_coredump() does override_creds() too.

May be just add

	if (current->cred != current->real_cred)
		return -EACCES;

into proc_pid_attr_write(), I dunno.



On 04/12, Casey Schaufler wrote:
>
> On 4/11/2019 11:21 PM, chengjian (D) wrote:
> 
> Added LSM and SELinux lists.
> 
> 
> >Hi.
> >
> >
> >syzkaller reported the following BUG:
> >
> >[   73.146973] kernel BUG at kernel/cred.c:434!
> >[   73.150231] invalid opcode: 0000 [#1] SMP KASAN PTI
> >[   73.151928] CPU: 2 PID: 4058 Comm: syz-executor.6 Not tainted
> >5.1.0-rc4-00062-g2d06b235815e-dirty #2
> >[   73.155174] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
> >rel-1.12.1-0-ga5cab58e9a3f-prebuilt.qemu.org 04/01/2014
> >[   73.159798] RIP: 0010:commit_creds+0xadb/0xe50
> >[   73.161426] Code: 8b 5b 20 48 c1 ea 03 0f b6 04 02 84 c0 74 08 3c 03 0f
> >8e 06 03 00 00 39 5d 20 0f 85 ff fa ff ff e9 0c fb ff ff e8 05 a2 25 00
> ><0f> 0b 48 c7 c7 80 56 c0 83 e8 95 22 b1 00 e8 f2 a1 25 00 0f 0b 48
> >[   73.167852] RSP: 0000:ffff88836e65f5d0 EFLAGS: 00010293
> >[   73.169636] RAX: ffff8883767b0000 RBX: ffff88837f111300 RCX:
> >ffffffff8124b5db
> >[   73.171962] RDX: 0000000000000000 RSI: ffffffff83c9b140 RDI:
> >ffff88837f111300
> >[   73.174310] RBP: ffff888376610400 R08: 0000000000000000 R09:
> >0000000000000004
> >[   73.176646] R10: 0000000000000001 R11: ffffed107c655acf R12:
> >ffff8883767b0000
> >[   73.178527] Process accounting resumed
> >[   73.179021] R13: ffff88837f111900 R14: ffff88837f111300 R15:
> >ffff8883767b0ac0
> >[   73.179029] FS:  00007f2d207f9700(0000) GS:ffff8883e3280000(0000)
> >knlGS:0000000000000000
> >[   73.179034] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> >[   73.179039] CR2: 00007f1500bd36c0 CR3: 00000003df304003 CR4:
> >00000000000206e0
> >[   73.179047] Call Trace:
> >[   73.190461]  selinux_setprocattr+0x2ea/0x8f0
> >[   73.191925]  ? ptrace_parent_sid+0x530/0x530
> >[   73.193436]  ? proc_pid_attr_write+0x185/0x5a0
> >[   73.194967]  security_setprocattr+0xa1/0x100
> >[   73.196408]  proc_pid_attr_write+0x307/0x5a0
> >[   73.197869]  ? mem_read+0x40/0x40
> >[   73.199013]  __vfs_write+0x81/0x100
> >[   73.200222]  __kernel_write+0xf8/0x330
> >[   73.201562]  do_acct_process+0xca5/0x1340
> >[   73.202969]  ? __ia32_sys_acct+0x1e0/0x1e0
> >[   73.204498]  ? find_held_lock+0x2f/0x1e0
> >[   73.205857]  ? rcu_irq_exit+0xec/0x2c0
> >[   73.207160]  ? lock_downgrade+0x630/0x630
> >[   73.208541]  acct_pin_kill+0x63/0x150
> >[   73.209816]  pin_kill+0x16d/0x7c0
> >[   73.210934]  ? lockdep_hardirqs_on+0x5e0/0x5e0
> >[   73.212452]  ? xas_start+0x155/0x510
> >[   73.213705]  ? pin_insert+0x50/0x50
> >[   73.214903]  ? finish_wait+0x270/0x270
> >[   73.216213]  ? cpumask_next+0x57/0x90
> >[   73.217442]  ? mnt_pin_kill+0x68/0x1d0
> >[   73.218851]  mnt_pin_kill+0x68/0x1d0
> >[   73.220398]  cleanup_mnt+0x11b/0x150
> >[   73.221970]  task_work_run+0x136/0x1b0
> >[   73.223427]  do_exit+0x830/0x2ca0
> >[   73.224586]  ? trace_hardirqs_off+0x3b/0x180
> >[   73.226088]  ? mm_update_next_owner+0x6a0/0x6a0
> >[   73.227622]  ? find_held_lock+0x2f/0x1e0
> >[   73.228954]  ? get_signal+0x2cf/0x1c00
> >[   73.230236]  ? lock_downgrade+0x630/0x630
> >[   73.231628]  ? rwlock_bug.part.0+0x90/0x90
> >[   73.233020]  do_group_exit+0x106/0x2f0
> >[   73.234330]  get_signal+0x325/0x1c00
> >[   73.235571]  do_signal+0x97/0x1670
> >[   73.236739]  ? do_send_specific+0x12d/0x220
> >[   73.238213]  ? lock_downgrade+0x630/0x630
> >[   73.239566]  ? setup_sigcontext+0x820/0x820
> >[   73.240982]  ? check_kill_permission+0x4a/0x510
> >[   73.242509]  ? do_send_specific+0x156/0x220
> >[   73.243905]  ? do_tkill+0x1c4/0x260
> >[   73.245081]  ? do_send_specific+0x220/0x220
> >[   73.246514]  ? trace_hardirqs_on_thunk+0x1a/0x1c
> >[   73.248061]  ? exit_to_usermode_loop+0x97/0x1d0
> >[   73.249619]  exit_to_usermode_loop+0x108/0x1d0
> >[   73.251129]  do_syscall_64+0x461/0x580
> >[   73.252454]  entry_SYSCALL_64_after_hwframe+0x49/0xbe
> >[   73.254219] RIP: 0033:0x462eb9
> >[   73.255327] Code: Bad RIP value.
> >[   73.256539] RSP: 002b:00007f2d207f8c58 EFLAGS: 00000246 ORIG_RAX:
> >00000000000000c8
> >[   73.259454] RAX: 0000000000000000 RBX: 000000000073bf00 RCX:
> >0000000000462eb9
> >[   73.262309] RDX: 0000000000000000 RSI: 000000000000001e RDI:
> >0000000000000005
> >[   73.265064] RBP: 0000000000000002 R08: 0000000000000000 R09:
> >0000000000000000
> >[   73.267774] R10: 0000000000000000 R11: 0000000000000246 R12:
> >00007f2d207f96bc
> >[   73.270546] R13: 00000000004c5509 R14: 00000000007042f0 R15:
> >00000000ffffffff
> >[   73.273542] Modules linked in:
> >[   73.274670] Dumping ftrace buffer:
> >[   73.275852]    (ftrace buffer empty)
> >[   73.277187] ---[ end trace dde36a95f458175d ]---
> >[   73.278834] RIP: 0010:commit_creds+0xadb/0xe50
> >[   73.280549] Code: 8b 5b 20 48 c1 ea 03 0f b6 04 02 84 c0 74 08 3c 03 0f
> >8e 06 03 00 00 39 5d 20 0f 85 ff fa ff ff e9 0c fb ff ff e8 05 a2 25 00
> ><0f> 0b 48 c7 c7 80 56 c0 83 e8 95 22 b1 00 e8 f2 a1 25 00 0f 0b 48
> >[   73.287090] RSP: 0000:ffff88836e65f5d0 EFLAGS: 00010293
> >[   73.288917] RAX: ffff8883767b0000 RBX: ffff88837f111300 RCX:
> >ffffffff8124b5db
> >[   73.291390] RDX: 0000000000000000 RSI: ffffffff83c9b140 RDI:
> >ffff88837f111300
> >[   73.293864] RBP: ffff888376610400 R08: 0000000000000000 R09:
> >0000000000000004
> >[   73.296370] R10: 0000000000000001 R11: ffffed107c655acf R12:
> >ffff8883767b0000
> >[   73.299275] R13: ffff88837f111900 R14: ffff88837f111300 R15:
> >ffff8883767b0ac0
> >[   73.301351] Process accounting resumed
> >[   73.301822] FS:  00007f2d207f9700(0000) GS:ffff8883e3280000(0000)
> >knlGS:0000000000000000
> >[   73.301827] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> >[   73.301832] CR2: 0000000000462e8f CR3: 0000000003a18002 CR4:
> >00000000000206e0
> >[   73.301850] Kernel panic - not syncing: Fatal exception
> >[   73.310719] Process accounting resumed
> >[   73.311515] Process accounting resumed
> >[   73.318916] Dumping ftrace buffer:
> >[   73.318921]    (ftrace buffer empty)
> >[   73.318945] Kernel Offset: disabled
> >[   73.328061] Rebooting in 10 seconds..
> >
> >
> >425 int commit_creds(struct cred *new)
> >426 {
> >427         struct task_struct *task = current;
> >428         const struct cred *old = task->real_cred;
> >429
> >430         kdebug("commit_creds(%p{%d,%d})", new,
> >431                atomic_read(&new->usage),
> >432                read_cred_subscribers(new));
> >433
> >434         BUG_ON(task->cred != old);  // BUG here
> >
> >
> >I find that the call chain which triggered the BUG is :
> >
> >do_exit
> >    |-=> acct_process
> >    |    -=> do_acct_process
> >    |        -=> orig_cred = override_creds(file->f_cred); // cred =
> >ffff8883c1878900/real_cred = ffff8883c1878900
> >    |        -=>  if (file_start_write_trylock(file))
> >{__kernel_write(file, &ac, sizeof(acct_t), &pos);}
> >    |               -=> __kernel_write+0xf8/0x330
> >    |                    -=> __vfs_write+0x81/0x100
> >    |                           -=> proc_pid_attr_write+0x307/0x5a0
> >    |                                -=> security_setprocattr+0xa1/0x100
> >    | -=>selinux_setprocattr+0x2ea/0x8f0
> >    | -=>commit_creds+0xd97/0x1080  // cred = ffff888379a79c00/real_cred =
> >ffff888379a79c00
> >    |       -=> revert_creds(orig_cred);   // cred =
> >ffff8883c1878900/real_cred = ffff888379a79c00
> >    |-=> task_work_run
> >           -=> cleanup_mnt+0x11b/0x150
> >                -=> mnt_pin_kill+0x68/0x1d0
> >                    -=> pin_kill+0x16d/0x7c0
> >                    -=> acct_pin_kill+0x2e/0x100
> >                    -=> do_acct_process+0x1a0/0x1340
> >                          -=> override_creds+0x18a/0x1c0   // cred =
> >ffff8883c1878900/real_cred = ffff888379a79c00
> >                          -=>  if (file_start_write_trylock(file))
> >{__kernel_write(file, &ac, sizeof(acct_t), &pos);}
> >                                    -=>   ......
> >-=>commit_creds+0xd97/0x1080   // new = ffff888379a6bb00, cred =
> >ffff8883c1878900, real_ceed = ffff888379a79c00 BUG here
> >                          -=> revert_creds(orig_cred);
> >
> >
> >Syzkaller Report Testcase:
> >
> >cat crash.log
> >
> >04:05:28 executing program 3:
> >clone(0x24100, 0x0, 0x0, 0x0, 0x0)
> >r0 = gettid()
> >perf_event_open(&(0x7f00000000c0)={0x2, 0x70, 0x4, 0x2, 0x0, 0x0, 0x0,
> >0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0,
> >0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0,
> >0x0, 0x0, 0x0, 0x0, @perf_bp={0x0}}, 0x0, 0xffffffffffffffff,
> >0xffffffffffffffff, 0x0)
> >acct(&(0x7f0000000000)='./file0\x00')
> >r1 = openat$smack_thread_current(0xffffffffffffff9c,
> >&(0x7f0000000000)='/proc/thread-self/attr/current\x00', 0x2, 0x0)
> >fcntl$setown(r1, 0x8, r0)
> >rt_tgsigqueueinfo(r0, r0, 0x1e, &(0x7f00000002c0))
> >tkill(r0, 0x1e)
> >
> >
> >Reproduce this BUG:
> >./syz-execprog -executor=./syz-executor -repeat=0 -procs=16 -cover=0
> >./crash.log
> >
> >
> >Can anyone help me ?
> >
> >
> >Thanks,
> >
> >        Cheng Jian.
> >
> >

Re: kernel BUG at kernel/cred.c:434!

[Paul Moore]

On Mon, Apr 15, 2019 at 9:43 AM Oleg Nesterov <oleg@redhat.com> wrote:
> Well, acct("/proc/self/attr/current") doesn't look like a good idea, but I do
> not know where should we put the additional check... And probably
> "echo /proc/self/attr/current > /proc/sys/kernel/core_pattern" can hit the
> same problem, do_coredump() does override_creds() too.
>
> May be just add
>
>         if (current->cred != current->real_cred)
>                 return -EACCES;
>
> into proc_pid_attr_write(), I dunno.

Is the problem that do_acct_process() is calling override_creds() and
the returned/old credentials are being freed before do_acct_process()
can reinstall the creds via revert_creds()?  Presumably because the
process accounting is causing the credentials to be replaced?

> On 04/12, Casey Schaufler wrote:
> >
> > On 4/11/2019 11:21 PM, chengjian (D) wrote:
> >
> > Added LSM and SELinux lists.
> >
> >
> > >Hi.
> > >
> > >
> > >syzkaller reported the following BUG:
> > >
> > >[   73.146973] kernel BUG at kernel/cred.c:434!
> > >[   73.150231] invalid opcode: 0000 [#1] SMP KASAN PTI
> > >[   73.151928] CPU: 2 PID: 4058 Comm: syz-executor.6 Not tainted
> > >5.1.0-rc4-00062-g2d06b235815e-dirty #2
> > >[   73.155174] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
> > >rel-1.12.1-0-ga5cab58e9a3f-prebuilt.qemu.org 04/01/2014
> > >[   73.159798] RIP: 0010:commit_creds+0xadb/0xe50
> > >[   73.161426] Code: 8b 5b 20 48 c1 ea 03 0f b6 04 02 84 c0 74 08 3c 03 0f
> > >8e 06 03 00 00 39 5d 20 0f 85 ff fa ff ff e9 0c fb ff ff e8 05 a2 25 00
> > ><0f> 0b 48 c7 c7 80 56 c0 83 e8 95 22 b1 00 e8 f2 a1 25 00 0f 0b 48
> > >[   73.167852] RSP: 0000:ffff88836e65f5d0 EFLAGS: 00010293
> > >[   73.169636] RAX: ffff8883767b0000 RBX: ffff88837f111300 RCX:
> > >ffffffff8124b5db
> > >[   73.171962] RDX: 0000000000000000 RSI: ffffffff83c9b140 RDI:
> > >ffff88837f111300
> > >[   73.174310] RBP: ffff888376610400 R08: 0000000000000000 R09:
> > >0000000000000004
> > >[   73.176646] R10: 0000000000000001 R11: ffffed107c655acf R12:
> > >ffff8883767b0000
> > >[   73.178527] Process accounting resumed
> > >[   73.179021] R13: ffff88837f111900 R14: ffff88837f111300 R15:
> > >ffff8883767b0ac0
> > >[   73.179029] FS:  00007f2d207f9700(0000) GS:ffff8883e3280000(0000)
> > >knlGS:0000000000000000
> > >[   73.179034] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > >[   73.179039] CR2: 00007f1500bd36c0 CR3: 00000003df304003 CR4:
> > >00000000000206e0
> > >[   73.179047] Call Trace:
> > >[   73.190461]  selinux_setprocattr+0x2ea/0x8f0
> > >[   73.191925]  ? ptrace_parent_sid+0x530/0x530
> > >[   73.193436]  ? proc_pid_attr_write+0x185/0x5a0
> > >[   73.194967]  security_setprocattr+0xa1/0x100
> > >[   73.196408]  proc_pid_attr_write+0x307/0x5a0
> > >[   73.197869]  ? mem_read+0x40/0x40
> > >[   73.199013]  __vfs_write+0x81/0x100
> > >[   73.200222]  __kernel_write+0xf8/0x330
> > >[   73.201562]  do_acct_process+0xca5/0x1340
> > >[   73.202969]  ? __ia32_sys_acct+0x1e0/0x1e0
> > >[   73.204498]  ? find_held_lock+0x2f/0x1e0
> > >[   73.205857]  ? rcu_irq_exit+0xec/0x2c0
> > >[   73.207160]  ? lock_downgrade+0x630/0x630
> > >[   73.208541]  acct_pin_kill+0x63/0x150
> > >[   73.209816]  pin_kill+0x16d/0x7c0
> > >[   73.210934]  ? lockdep_hardirqs_on+0x5e0/0x5e0
> > >[   73.212452]  ? xas_start+0x155/0x510
> > >[   73.213705]  ? pin_insert+0x50/0x50
> > >[   73.214903]  ? finish_wait+0x270/0x270
> > >[   73.216213]  ? cpumask_next+0x57/0x90
> > >[   73.217442]  ? mnt_pin_kill+0x68/0x1d0
> > >[   73.218851]  mnt_pin_kill+0x68/0x1d0
> > >[   73.220398]  cleanup_mnt+0x11b/0x150
> > >[   73.221970]  task_work_run+0x136/0x1b0
> > >[   73.223427]  do_exit+0x830/0x2ca0
> > >[   73.224586]  ? trace_hardirqs_off+0x3b/0x180
> > >[   73.226088]  ? mm_update_next_owner+0x6a0/0x6a0
> > >[   73.227622]  ? find_held_lock+0x2f/0x1e0
> > >[   73.228954]  ? get_signal+0x2cf/0x1c00
> > >[   73.230236]  ? lock_downgrade+0x630/0x630
> > >[   73.231628]  ? rwlock_bug.part.0+0x90/0x90
> > >[   73.233020]  do_group_exit+0x106/0x2f0
> > >[   73.234330]  get_signal+0x325/0x1c00
> > >[   73.235571]  do_signal+0x97/0x1670
> > >[   73.236739]  ? do_send_specific+0x12d/0x220
> > >[   73.238213]  ? lock_downgrade+0x630/0x630
> > >[   73.239566]  ? setup_sigcontext+0x820/0x820
> > >[   73.240982]  ? check_kill_permission+0x4a/0x510
> > >[   73.242509]  ? do_send_specific+0x156/0x220
> > >[   73.243905]  ? do_tkill+0x1c4/0x260
> > >[   73.245081]  ? do_send_specific+0x220/0x220
> > >[   73.246514]  ? trace_hardirqs_on_thunk+0x1a/0x1c
> > >[   73.248061]  ? exit_to_usermode_loop+0x97/0x1d0
> > >[   73.249619]  exit_to_usermode_loop+0x108/0x1d0
> > >[   73.251129]  do_syscall_64+0x461/0x580
> > >[   73.252454]  entry_SYSCALL_64_after_hwframe+0x49/0xbe
> > >[   73.254219] RIP: 0033:0x462eb9
> > >[   73.255327] Code: Bad RIP value.
> > >[   73.256539] RSP: 002b:00007f2d207f8c58 EFLAGS: 00000246 ORIG_RAX:
> > >00000000000000c8
> > >[   73.259454] RAX: 0000000000000000 RBX: 000000000073bf00 RCX:
> > >0000000000462eb9
> > >[   73.262309] RDX: 0000000000000000 RSI: 000000000000001e RDI:
> > >0000000000000005
> > >[   73.265064] RBP: 0000000000000002 R08: 0000000000000000 R09:
> > >0000000000000000
> > >[   73.267774] R10: 0000000000000000 R11: 0000000000000246 R12:
> > >00007f2d207f96bc
> > >[   73.270546] R13: 00000000004c5509 R14: 00000000007042f0 R15:
> > >00000000ffffffff
> > >[   73.273542] Modules linked in:
> > >[   73.274670] Dumping ftrace buffer:
> > >[   73.275852]    (ftrace buffer empty)
> > >[   73.277187] ---[ end trace dde36a95f458175d ]---
> > >[   73.278834] RIP: 0010:commit_creds+0xadb/0xe50
> > >[   73.280549] Code: 8b 5b 20 48 c1 ea 03 0f b6 04 02 84 c0 74 08 3c 03 0f
> > >8e 06 03 00 00 39 5d 20 0f 85 ff fa ff ff e9 0c fb ff ff e8 05 a2 25 00
> > ><0f> 0b 48 c7 c7 80 56 c0 83 e8 95 22 b1 00 e8 f2 a1 25 00 0f 0b 48
> > >[   73.287090] RSP: 0000:ffff88836e65f5d0 EFLAGS: 00010293
> > >[   73.288917] RAX: ffff8883767b0000 RBX: ffff88837f111300 RCX:
> > >ffffffff8124b5db
> > >[   73.291390] RDX: 0000000000000000 RSI: ffffffff83c9b140 RDI:
> > >ffff88837f111300
> > >[   73.293864] RBP: ffff888376610400 R08: 0000000000000000 R09:
> > >0000000000000004
> > >[   73.296370] R10: 0000000000000001 R11: ffffed107c655acf R12:
> > >ffff8883767b0000
> > >[   73.299275] R13: ffff88837f111900 R14: ffff88837f111300 R15:
> > >ffff8883767b0ac0
> > >[   73.301351] Process accounting resumed
> > >[   73.301822] FS:  00007f2d207f9700(0000) GS:ffff8883e3280000(0000)
> > >knlGS:0000000000000000
> > >[   73.301827] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > >[   73.301832] CR2: 0000000000462e8f CR3: 0000000003a18002 CR4:
> > >00000000000206e0
> > >[   73.301850] Kernel panic - not syncing: Fatal exception
> > >[   73.310719] Process accounting resumed
> > >[   73.311515] Process accounting resumed
> > >[   73.318916] Dumping ftrace buffer:
> > >[   73.318921]    (ftrace buffer empty)
> > >[   73.318945] Kernel Offset: disabled
> > >[   73.328061] Rebooting in 10 seconds..
> > >
> > >
> > >425 int commit_creds(struct cred *new)
> > >426 {
> > >427         struct task_struct *task = current;
> > >428         const struct cred *old = task->real_cred;
> > >429
> > >430         kdebug("commit_creds(%p{%d,%d})", new,
> > >431                atomic_read(&new->usage),
> > >432                read_cred_subscribers(new));
> > >433
> > >434         BUG_ON(task->cred != old);  // BUG here
> > >
> > >
> > >I find that the call chain which triggered the BUG is :
> > >
> > >do_exit
> > >    |-=> acct_process
> > >    |    -=> do_acct_process
> > >    |        -=> orig_cred = override_creds(file->f_cred); // cred =
> > >ffff8883c1878900/real_cred = ffff8883c1878900
> > >    |        -=>  if (file_start_write_trylock(file))
> > >{__kernel_write(file, &ac, sizeof(acct_t), &pos);}
> > >    |               -=> __kernel_write+0xf8/0x330
> > >    |                    -=> __vfs_write+0x81/0x100
> > >    |                           -=> proc_pid_attr_write+0x307/0x5a0
> > >    |                                -=> security_setprocattr+0xa1/0x100
> > >    | -=>selinux_setprocattr+0x2ea/0x8f0
> > >    | -=>commit_creds+0xd97/0x1080  // cred = ffff888379a79c00/real_cred =
> > >ffff888379a79c00
> > >    |       -=> revert_creds(orig_cred);   // cred =
> > >ffff8883c1878900/real_cred = ffff888379a79c00
> > >    |-=> task_work_run
> > >           -=> cleanup_mnt+0x11b/0x150
> > >                -=> mnt_pin_kill+0x68/0x1d0
> > >                    -=> pin_kill+0x16d/0x7c0
> > >                    -=> acct_pin_kill+0x2e/0x100
> > >                    -=> do_acct_process+0x1a0/0x1340
> > >                          -=> override_creds+0x18a/0x1c0   // cred =
> > >ffff8883c1878900/real_cred = ffff888379a79c00
> > >                          -=>  if (file_start_write_trylock(file))
> > >{__kernel_write(file, &ac, sizeof(acct_t), &pos);}
> > >                                    -=>   ......
> > >-=>commit_creds+0xd97/0x1080   // new = ffff888379a6bb00, cred =
> > >ffff8883c1878900, real_ceed = ffff888379a79c00 BUG here
> > >                          -=> revert_creds(orig_cred);
> > >
> > >
> > >Syzkaller Report Testcase:
> > >
> > >cat crash.log
> > >
> > >04:05:28 executing program 3:
> > >clone(0x24100, 0x0, 0x0, 0x0, 0x0)
> > >r0 = gettid()
> > >perf_event_open(&(0x7f00000000c0)={0x2, 0x70, 0x4, 0x2, 0x0, 0x0, 0x0,
> > >0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0,
> > >0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0,
> > >0x0, 0x0, 0x0, 0x0, @perf_bp={0x0}}, 0x0, 0xffffffffffffffff,
> > >0xffffffffffffffff, 0x0)
> > >acct(&(0x7f0000000000)='./file0\x00')
> > >r1 = openat$smack_thread_current(0xffffffffffffff9c,
> > >&(0x7f0000000000)='/proc/thread-self/attr/current\x00', 0x2, 0x0)
> > >fcntl$setown(r1, 0x8, r0)
> > >rt_tgsigqueueinfo(r0, r0, 0x1e, &(0x7f00000002c0))
> > >tkill(r0, 0x1e)
> > >
> > >
> > >Reproduce this BUG:
> > >./syz-execprog -executor=./syz-executor -repeat=0 -procs=16 -cover=0
> > >./crash.log
> > >
> > >
> > >Can anyone help me ?
> > >
> > >
> > >Thanks,
> > >
> > >        Cheng Jian.
> > >
> > >
>


-- 
paul moore
www.paul-moore.com

Re: kernel BUG at kernel/cred.c:434!

[Oleg Nesterov]

On 04/15, Paul Moore wrote:
>
> On Mon, Apr 15, 2019 at 9:43 AM Oleg Nesterov <oleg@redhat.com> wrote:
> > Well, acct("/proc/self/attr/current") doesn't look like a good idea, but I do
> > not know where should we put the additional check... And probably
> > "echo /proc/self/attr/current > /proc/sys/kernel/core_pattern" can hit the
> > same problem, do_coredump() does override_creds() too.
> >
> > May be just add
> >
> >         if (current->cred != current->real_cred)
> >                 return -EACCES;
> >
> > into proc_pid_attr_write(), I dunno.
>
> Is the problem that do_acct_process() is calling override_creds() and
> the returned/old credentials are being freed before do_acct_process()
> can reinstall the creds via revert_creds()?  Presumably because the
> process accounting is causing the credentials to be replaced?

Afaics, the problem is that do_acct_process() does override_creds() and
then __kernel_write(). Which calls proc_pid_attr_write(), which in turn calls
selinux_setprocattr(), which does another prepare_creds() + commit_creds();
and commit_creds() hits

	BUG_ON(task->cred != old);

Oleg.

Re: kernel BUG at kernel/cred.c:434!

[Paul Moore]

On Mon, Apr 15, 2019 at 11:05 AM Oleg Nesterov <oleg@redhat.com> wrote:
> On 04/15, Paul Moore wrote:
> >
> > On Mon, Apr 15, 2019 at 9:43 AM Oleg Nesterov <oleg@redhat.com> wrote:
> > > Well, acct("/proc/self/attr/current") doesn't look like a good idea, but I do
> > > not know where should we put the additional check... And probably
> > > "echo /proc/self/attr/current > /proc/sys/kernel/core_pattern" can hit the
> > > same problem, do_coredump() does override_creds() too.
> > >
> > > May be just add
> > >
> > >         if (current->cred != current->real_cred)
> > >                 return -EACCES;
> > >
> > > into proc_pid_attr_write(), I dunno.
> >
> > Is the problem that do_acct_process() is calling override_creds() and
> > the returned/old credentials are being freed before do_acct_process()
> > can reinstall the creds via revert_creds()?  Presumably because the
> > process accounting is causing the credentials to be replaced?
>
> Afaics, the problem is that do_acct_process() does override_creds() and
> then __kernel_write(). Which calls proc_pid_attr_write(), which in turn calls
> selinux_setprocattr(), which does another prepare_creds() + commit_creds();
> and commit_creds() hits
>
>         BUG_ON(task->cred != old);

Gotcha.  In the process of looking at the backtrace I forgot about the
BUG_ON() at the top of the oops message.

I wonder what terrible things would happen if we changed the BUG_ON()
in commit_creds to simple returning an error an error code to the
caller.  There is a warning/requirement in commit_creds() function
header comment that it should always return 0.

-- 
paul moore
www.paul-moore.com

Re: kernel BUG at kernel/cred.c:434!

[Kees Cook]

On Mon, Apr 15, 2019 at 11:20 AM Paul Moore <paul@paul-moore.com> wrote:
>
> On Mon, Apr 15, 2019 at 11:05 AM Oleg Nesterov <oleg@redhat.com> wrote:
> > On 04/15, Paul Moore wrote:
> > >
> > > On Mon, Apr 15, 2019 at 9:43 AM Oleg Nesterov <oleg@redhat.com> wrote:
> > > > Well, acct("/proc/self/attr/current") doesn't look like a good idea, but I do
> > > > not know where should we put the additional check... And probably
> > > > "echo /proc/self/attr/current > /proc/sys/kernel/core_pattern" can hit the
> > > > same problem, do_coredump() does override_creds() too.
> > > >
> > > > May be just add
> > > >
> > > >         if (current->cred != current->real_cred)
> > > >                 return -EACCES;
> > > >
> > > > into proc_pid_attr_write(), I dunno.
> > >
> > > Is the problem that do_acct_process() is calling override_creds() and
> > > the returned/old credentials are being freed before do_acct_process()
> > > can reinstall the creds via revert_creds()?  Presumably because the
> > > process accounting is causing the credentials to be replaced?
> >
> > Afaics, the problem is that do_acct_process() does override_creds() and
> > then __kernel_write(). Which calls proc_pid_attr_write(), which in turn calls
> > selinux_setprocattr(), which does another prepare_creds() + commit_creds();
> > and commit_creds() hits
> >
> >         BUG_ON(task->cred != old);
>
> Gotcha.  In the process of looking at the backtrace I forgot about the
> BUG_ON() at the top of the oops message.
>
> I wonder what terrible things would happen if we changed the BUG_ON()
> in commit_creds to simple returning an error an error code to the
> caller.  There is a warning/requirement in commit_creds() function
> header comment that it should always return 0.

Would callers be expected to call abort_creds() on failure? There are
a number of places where it'd need fixing up. And would likely be best
with a __must_check marking.

It seems like avoiding the pathological case might be simpler?

-- 
Kees Cook

Re: kernel BUG at kernel/cred.c:434!

[chengjian (D)]

On 2019/4/16 11:40, Kees Cook wrote:
> On Mon, Apr 15, 2019 at 11:20 AM Paul Moore <paul@paul-moore.com> wrote:
>> On Mon, Apr 15, 2019 at 11:05 AM Oleg Nesterov <oleg@redhat.com> wrote:
>>> On 04/15, Paul Moore wrote:
>>>> On Mon, Apr 15, 2019 at 9:43 AM Oleg Nesterov <oleg@redhat.com> wrote:
>>>>> Well, acct("/proc/self/attr/current") doesn't look like a good idea, but I do
>>>>> not know where should we put the additional check... And probably
>>>>> "echo /proc/self/attr/current > /proc/sys/kernel/core_pattern" can hit the
>>>>> same problem, do_coredump() does override_creds() too.
>>>>>
>>>>> May be just add
>>>>>
>>>>>          if (current->cred != current->real_cred)
>>>>>                  return -EACCES;
>>>>>
>>>>> into proc_pid_attr_write(), I dunno.
>>>> Is the problem that do_acct_process() is calling override_creds() and
>>>> the returned/old credentials are being freed before do_acct_process()
>>>> can reinstall the creds via revert_creds()?  Presumably because the
>>>> process accounting is causing the credentials to be replaced?
>>> Afaics, the problem is that do_acct_process() does override_creds() and
>>> then __kernel_write(). Which calls proc_pid_attr_write(), which in turn calls
>>> selinux_setprocattr(), which does another prepare_creds() + commit_creds();
>>> and commit_creds() hits
>>>
>>>          BUG_ON(task->cred != old);
>> Gotcha.  In the process of looking at the backtrace I forgot about the
>> BUG_ON() at the top of the oops message.
>>
>> I wonder what terrible things would happen if we changed the BUG_ON()
>> in commit_creds to simple returning an error an error code to the
>> caller.  There is a warning/requirement in commit_creds() function
>> header comment that it should always return 0.
> Would callers be expected to call abort_creds() on failure? There are
> a number of places where it'd need fixing up. And would likely be best
> with a __must_check marking.
>
> It seems like avoiding the pathological case might be simpler?


Yeah, Avoiding this pathological case is a better solution.

It seems like that we can't commit_creds() during

override_creds() and revert_creds().

So how about just put commit_creds outside !


just like:

     override_creds()   // cred  -=> new

     // may BUG_ON if commit_creds done.

     revert_creds()     //  cred -=> old                         
<-----------|

     commit_creds   //  cred = real_cred = new                |

[revert_creds]--------------------------------------------------|


[1]--Before we call commit_creds in selinux_setprocattr(),

if we find that cred != real_cred, it may have been overridden

before, we should revert it.

[2]--The same to revert_creds, when we found someone have committed,

orig_cred != current->real_cred may hits, this means that

we have reverted before(see [1]).

[3]--Sometimes new and old are the same, then we need to consider this

situation specially.


The code just like:


From: Yang Yingliang <yangyingliang@huawei.com>
Date: Sat, 13 Apr 2019 21:56:01 +0800
Subject: [PATCH] fix cred bug_on

Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
---
  kernel/acct.c            | 3 ++-
  kernel/cred.c            | 6 ++++++
  security/selinux/hooks.c | 2 ++
  3 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/kernel/acct.c b/kernel/acct.c
index addf7732fb56..f2065f899eee 100644
--- a/kernel/acct.c
+++ b/kernel/acct.c
@@ -522,7 +522,8 @@ static void do_acct_process(struct bsd_acct_struct 
*acct)
      }
  out:
      current->signal->rlim[RLIMIT_FSIZE].rlim_cur = flim;
-    revert_creds(orig_cred);
+    if (orig_cred == current->real_cred)    // [2]
+        revert_creds(orig_cred);
  }

  /**
diff --git a/kernel/cred.c b/kernel/cred.c
index ecf03657e71c..c4d5ba92fb9b 100644
--- a/kernel/cred.c
+++ b/kernel/cred.c
@@ -522,6 +522,9 @@ const struct cred *override_creds(const struct cred 
*new)
  {
      const struct cred *old = current->cred;

+    if (old == new)    //  [3]
+        return old;
+
      kdebug("override_creds(%p{%d,%d})", new,
             atomic_read(&new->usage),
             read_cred_subscribers(new));
@@ -551,6 +554,9 @@ void revert_creds(const struct cred *old)
  {
      const struct cred *override = current->cred;

+    if (override == old)    // [3]
+        return;
+
      kdebug("revert_creds(%p{%d,%d})", old,
             atomic_read(&old->usage),
             read_cred_subscribers(old));
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index b5017beb4ef7..bc8108e4e90f 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -6590,6 +6590,8 @@ static int selinux_setprocattr(const char *name, 
void *value, size_t size)
          goto abort_change;
      }

+    if (current->cred != current->real_cred)    // [1]
+        revert_creds(current->real_cred);
      commit_creds(new);
      return size;

-- 
2.17.1


We have tested this patch for 3 days and it works well.

Are there any cases that are not covered here ?


Thanks.

     Cheng Jian

Re: kernel BUG at kernel/cred.c:434!

[Paul Moore]

On Tue, Apr 16, 2019 at 10:46 AM chengjian (D) <cj.chengjian@huawei.com> wrote:
> On 2019/4/16 11:40, Kees Cook wrote:
> > On Mon, Apr 15, 2019 at 11:20 AM Paul Moore <paul@paul-moore.com> wrote:
> >> On Mon, Apr 15, 2019 at 11:05 AM Oleg Nesterov <oleg@redhat.com> wrote:
> >>> On 04/15, Paul Moore wrote:
> >>>> On Mon, Apr 15, 2019 at 9:43 AM Oleg Nesterov <oleg@redhat.com> wrote:
> >>>>> Well, acct("/proc/self/attr/current") doesn't look like a good idea, but I do
> >>>>> not know where should we put the additional check... And probably
> >>>>> "echo /proc/self/attr/current > /proc/sys/kernel/core_pattern" can hit the
> >>>>> same problem, do_coredump() does override_creds() too.
> >>>>>
> >>>>> May be just add
> >>>>>
> >>>>>          if (current->cred != current->real_cred)
> >>>>>                  return -EACCES;
> >>>>>
> >>>>> into proc_pid_attr_write(), I dunno.
> >>>> Is the problem that do_acct_process() is calling override_creds() and
> >>>> the returned/old credentials are being freed before do_acct_process()
> >>>> can reinstall the creds via revert_creds()?  Presumably because the
> >>>> process accounting is causing the credentials to be replaced?
> >>> Afaics, the problem is that do_acct_process() does override_creds() and
> >>> then __kernel_write(). Which calls proc_pid_attr_write(), which in turn calls
> >>> selinux_setprocattr(), which does another prepare_creds() + commit_creds();
> >>> and commit_creds() hits
> >>>
> >>>          BUG_ON(task->cred != old);
> >> Gotcha.  In the process of looking at the backtrace I forgot about the
> >> BUG_ON() at the top of the oops message.
> >>
> >> I wonder what terrible things would happen if we changed the BUG_ON()
> >> in commit_creds to simple returning an error an error code to the
> >> caller.  There is a warning/requirement in commit_creds() function
> >> header comment that it should always return 0.
> > Would callers be expected to call abort_creds() on failure? There are
> > a number of places where it'd need fixing up. And would likely be best
> > with a __must_check marking.
> >
> > It seems like avoiding the pathological case might be simpler?
>
> Yeah, Avoiding this pathological case is a better solution.

No arguments that this is particularly messed up scenario, I'm just
trying to arrive at a solution that isn't too ugly.

> From: Yang Yingliang <yangyingliang@huawei.com>
> Date: Sat, 13 Apr 2019 21:56:01 +0800
> Subject: [PATCH] fix cred bug_on
>
> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
> ---
>   kernel/acct.c            | 3 ++-
>   kernel/cred.c            | 6 ++++++
>   security/selinux/hooks.c | 2 ++
>   3 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/acct.c b/kernel/acct.c
> index addf7732fb56..f2065f899eee 100644
> --- a/kernel/acct.c
> +++ b/kernel/acct.c
> @@ -522,7 +522,8 @@ static void do_acct_process(struct bsd_acct_struct
> *acct)
>       }
>   out:
>       current->signal->rlim[RLIMIT_FSIZE].rlim_cur = flim;
> -    revert_creds(orig_cred);
> +    if (orig_cred == current->real_cred)    // [2]
> +        revert_creds(orig_cred);
>   }
>
>   /**
> diff --git a/kernel/cred.c b/kernel/cred.c
> index ecf03657e71c..c4d5ba92fb9b 100644
> --- a/kernel/cred.c
> +++ b/kernel/cred.c
> @@ -522,6 +522,9 @@ const struct cred *override_creds(const struct cred
> *new)
>   {
>       const struct cred *old = current->cred;
>
> +    if (old == new)    //  [3]
> +        return old;
> +
>       kdebug("override_creds(%p{%d,%d})", new,
>              atomic_read(&new->usage),
>              read_cred_subscribers(new));
> @@ -551,6 +554,9 @@ void revert_creds(const struct cred *old)
>   {
>       const struct cred *override = current->cred;
>
> +    if (override == old)    // [3]
> +        return;
> +
>       kdebug("revert_creds(%p{%d,%d})", old,
>              atomic_read(&old->usage),
>              read_cred_subscribers(old));
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index b5017beb4ef7..bc8108e4e90f 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -6590,6 +6590,8 @@ static int selinux_setprocattr(const char *name,
> void *value, size_t size)
>           goto abort_change;
>       }
>
> +    if (current->cred != current->real_cred)    // [1]
> +        revert_creds(current->real_cred);
>       commit_creds(new);
>       return size;

Doing the revert only to then commit the creds seems really ugly to
me.  I'm tempted to simply return an error in selinux_setprocattr() if
the task's credentials are not the same as its real_cred; if we do
that I believe we should resolve this problem.  The accounting write
to the SELinux file in /proc would fail of course, but I think we can
all consider that as a positive side-effect.

While I don't think this should have a negative impact on anything
else, I haven't convinced myself of that just yet.

-- 
paul moore
www.paul-moore.com

Re: kernel BUG at kernel/cred.c:434!

[Oleg Nesterov]

On 04/17, Paul Moore wrote:
>
> I'm tempted to simply return an error in selinux_setprocattr() if
> the task's credentials are not the same as its real_cred;

What about other modules? I have no idea what smack_setprocattr() is,
but it too does prepare_creds/commit creds.

it seems that the simplest workaround should simply add the additional
cred == real_cred into proc_pid_attr_write().

Oleg.

Re: kernel BUG at kernel/cred.c:434!

[Casey Schaufler]

On 4/17/2019 7:57 AM, Oleg Nesterov wrote:
> On 04/17, Paul Moore wrote:
>> I'm tempted to simply return an error in selinux_setprocattr() if
>> the task's credentials are not the same as its real_cred;
> What about other modules? I have no idea what smack_setprocattr() is,
> but it too does prepare_creds/commit creds.

For what it's worth, my test for Smack does not reproduce
the problem.

>
> it seems that the simplest workaround should simply add the additional
> cred == real_cred into proc_pid_attr_write().
>
> Oleg.
>

Re: kernel BUG at kernel/cred.c:434!

[Paul Moore]

On Wed, Apr 17, 2019 at 10:57 AM Oleg Nesterov <oleg@redhat.com> wrote:
> On 04/17, Paul Moore wrote:
> >
> > I'm tempted to simply return an error in selinux_setprocattr() if
> > the task's credentials are not the same as its real_cred;
>
> What about other modules? I have no idea what smack_setprocattr() is,
> but it too does prepare_creds/commit creds.
>
> it seems that the simplest workaround should simply add the additional
> cred == real_cred into proc_pid_attr_write().

Yes, that is simple, but I worry about what other LSMs might want to
do.  While I believe failing if the effective creds are not the same
as the real_creds is okay for SELinux (possibly Smack too), I worry
about what other LSMs may want to do.  After all,
proc_pid_attr_write() doesn't change the the creds itself, that is
something the specific LSMs do.

-- 
paul moore
www.paul-moore.com

Re: kernel BUG at kernel/cred.c:434!

[Oleg Nesterov]

On 04/17, Paul Moore wrote:
>
> On Wed, Apr 17, 2019 at 10:57 AM Oleg Nesterov <oleg@redhat.com> wrote:
> > On 04/17, Paul Moore wrote:
> > >
> > > I'm tempted to simply return an error in selinux_setprocattr() if
> > > the task's credentials are not the same as its real_cred;
> >
> > What about other modules? I have no idea what smack_setprocattr() is,
> > but it too does prepare_creds/commit creds.
> >
> > it seems that the simplest workaround should simply add the additional
> > cred == real_cred into proc_pid_attr_write().
>
> Yes, that is simple, but I worry about what other LSMs might want to
> do.  While I believe failing if the effective creds are not the same
> as the real_creds is okay for SELinux (possibly Smack too), I worry
> about what other LSMs may want to do.  After all,
> proc_pid_attr_write() doesn't change the the creds itself, that is
> something the specific LSMs do.

Yes, but if proc_pid_attr_write() is called with cred != real_cred then
something is already wrong?

In fact, I think that something is already wrong if it is not called by
user-space directly. Too late to ask, but why is this /proc/self/attr/
magic not implemented via syscall(s) ?

But, Paul, this is up to you. I don't understand this all even remotely.

Oleg.

Re: kernel BUG at kernel/cred.c:434!

[Casey Schaufler]

On 4/17/2019 9:27 AM, Oleg Nesterov wrote:
> On 04/17, Paul Moore wrote:
>> On Wed, Apr 17, 2019 at 10:57 AM Oleg Nesterov <oleg@redhat.com> wrote:
>>> On 04/17, Paul Moore wrote:
>>>> I'm tempted to simply return an error in selinux_setprocattr() if
>>>> the task's credentials are not the same as its real_cred;
>>> What about other modules? I have no idea what smack_setprocattr() is,
>>> but it too does prepare_creds/commit creds.
>>>
>>> it seems that the simplest workaround should simply add the additional
>>> cred == real_cred into proc_pid_attr_write().
>> Yes, that is simple, but I worry about what other LSMs might want to
>> do.  While I believe failing if the effective creds are not the same
>> as the real_creds is okay for SELinux (possibly Smack too), I worry
>> about what other LSMs may want to do.  After all,
>> proc_pid_attr_write() doesn't change the the creds itself, that is
>> something the specific LSMs do.
> Yes, but if proc_pid_attr_write() is called with cred != real_cred then
> something is already wrong?
>
> In fact, I think that something is already wrong if it is not called by
> user-space directly. Too late to ask, but why is this /proc/self/attr/
> magic not implemented via syscall(s) ?

Shell scripts, for one thing. It's a straightforward and appropriate
use of the /proc interface. System calls would require additional change
to existing programs, whereas using the /proc interface allows a good
deal to be done in the containing scripts.
  

Re: kernel BUG at kernel/cred.c:434!

[Paul Moore]

On Wed, Apr 17, 2019 at 12:27 PM Oleg Nesterov <oleg@redhat.com> wrote:
> On 04/17, Paul Moore wrote:
> >
> > On Wed, Apr 17, 2019 at 10:57 AM Oleg Nesterov <oleg@redhat.com> wrote:
> > > On 04/17, Paul Moore wrote:
> > > >
> > > > I'm tempted to simply return an error in selinux_setprocattr() if
> > > > the task's credentials are not the same as its real_cred;
> > >
> > > What about other modules? I have no idea what smack_setprocattr() is,
> > > but it too does prepare_creds/commit creds.
> > >
> > > it seems that the simplest workaround should simply add the additional
> > > cred == real_cred into proc_pid_attr_write().
> >
> > Yes, that is simple, but I worry about what other LSMs might want to
> > do.  While I believe failing if the effective creds are not the same
> > as the real_creds is okay for SELinux (possibly Smack too), I worry
> > about what other LSMs may want to do.  After all,
> > proc_pid_attr_write() doesn't change the the creds itself, that is
> > something the specific LSMs do.
>
> Yes, but if proc_pid_attr_write() is called with cred != real_cred then
> something is already wrong?

True, or at least I would think so.

Looking at the current tree there are three LSMs which implement
setprocattr hooks: SELinux, Smack, and AppArmor.  I know Casey has
already mentioned that he wasn't able to trigger the problem in Smack,
but looking at smack_setprocattr() I see the similar commit_creds()
usage so I would expect the same problem in Smack; what say you Casey?
 Looking at apparmor_setprocattr(), it appears that it too could end
up calling commit_creds() via aa_set_current_hat().

Since it looks like all three LSMs which implement the setprocattr
hook are vulnerable I'm open to the idea that proc_pid_attr_write() is
a better choice for the cred != read_cred check, but I would want to
make sure John and Casey are okay with that.

John?

Casey?

-- 
paul moore
www.paul-moore.com

Re: kernel BUG at kernel/cred.c:434!

[John Johansen]

On 4/17/19 4:39 PM, Paul Moore wrote:
> On Wed, Apr 17, 2019 at 12:27 PM Oleg Nesterov <oleg@redhat.com> wrote:
>> On 04/17, Paul Moore wrote:
>>>
>>> On Wed, Apr 17, 2019 at 10:57 AM Oleg Nesterov <oleg@redhat.com> wrote:
>>>> On 04/17, Paul Moore wrote:
>>>>>
>>>>> I'm tempted to simply return an error in selinux_setprocattr() if
>>>>> the task's credentials are not the same as its real_cred;
>>>>
>>>> What about other modules? I have no idea what smack_setprocattr() is,
>>>> but it too does prepare_creds/commit creds.
>>>>
>>>> it seems that the simplest workaround should simply add the additional
>>>> cred == real_cred into proc_pid_attr_write().
>>>
>>> Yes, that is simple, but I worry about what other LSMs might want to
>>> do.  While I believe failing if the effective creds are not the same
>>> as the real_creds is okay for SELinux (possibly Smack too), I worry
>>> about what other LSMs may want to do.  After all,
>>> proc_pid_attr_write() doesn't change the the creds itself, that is
>>> something the specific LSMs do.
>>
>> Yes, but if proc_pid_attr_write() is called with cred != real_cred then
>> something is already wrong?
> 
> True, or at least I would think so.
> 
> Looking at the current tree there are three LSMs which implement
> setprocattr hooks: SELinux, Smack, and AppArmor.  I know Casey has
> already mentioned that he wasn't able to trigger the problem in Smack,
> but looking at smack_setprocattr() I see the similar commit_creds()
> usage so I would expect the same problem in Smack; what say you Casey?
>  Looking at apparmor_setprocattr(), it appears that it too could end
> up calling commit_creds() via aa_set_current_hat().
> 
> Since it looks like all three LSMs which implement the setprocattr
> hook are vulnerable I'm open to the idea that proc_pid_attr_write() is
> a better choice for the cred != read_cred check, but I would want to
> make sure John and Casey are okay with that.
> 
> John?

heh yeah,

seems I messed up our check, we actually have

	if (current_cred() != current_real_cred())
		return -EBUSY;

on the change_profile path and missed it on the change_hat
one.

It makes sense to lift the check up earlier

Re: kernel BUG at kernel/cred.c:434!

[Casey Schaufler]

On 4/17/2019 4:39 PM, Paul Moore wrote:
> On Wed, Apr 17, 2019 at 12:27 PM Oleg Nesterov <oleg@redhat.com> wrote:
>> On 04/17, Paul Moore wrote:
>>> On Wed, Apr 17, 2019 at 10:57 AM Oleg Nesterov <oleg@redhat.com> wrote:
>>>> On 04/17, Paul Moore wrote:
>>>>> I'm tempted to simply return an error in selinux_setprocattr() if
>>>>> the task's credentials are not the same as its real_cred;
>>>> What about other modules? I have no idea what smack_setprocattr() is,
>>>> but it too does prepare_creds/commit creds.
>>>>
>>>> it seems that the simplest workaround should simply add the additional
>>>> cred == real_cred into proc_pid_attr_write().
>>> Yes, that is simple, but I worry about what other LSMs might want to
>>> do.  While I believe failing if the effective creds are not the same
>>> as the real_creds is okay for SELinux (possibly Smack too), I worry
>>> about what other LSMs may want to do.  After all,
>>> proc_pid_attr_write() doesn't change the the creds itself, that is
>>> something the specific LSMs do.
>> Yes, but if proc_pid_attr_write() is called with cred != real_cred then
>> something is already wrong?
> True, or at least I would think so.
>
> Looking at the current tree there are three LSMs which implement
> setprocattr hooks: SELinux, Smack, and AppArmor.  I know Casey has
> already mentioned that he wasn't able to trigger the problem in Smack,
> but looking at smack_setprocattr() I see the similar commit_creds()
> usage so I would expect the same problem in Smack; what say you Casey?

I say that my test program runs without ill effect. I call acct()
with "/proc/self/attr/current", which succeeds and enables accounting
just like it is supposed to. I then have the program open
"/proc/self/attr/current" and read it, all of which goes swimmingly.
When Smack frees a cred it usually does not free any memory of its
own, so it is conceivable that I'm just getting lucky. Or, I may not
have sufficient debug enabled.

>   Looking at apparmor_setprocattr(), it appears that it too could end
> up calling commit_creds() via aa_set_current_hat().
>
> Since it looks like all three LSMs which implement the setprocattr
> hook are vulnerable I'm open to the idea that proc_pid_attr_write() is
> a better choice for the cred != read_cred check, but I would want to
> make sure John and Casey are okay with that.
>
> John?
>
> Casey?

I'm fine with the change going into proc_pid_attr_write().

Re: kernel BUG at kernel/cred.c:434!

[Yang Yingliang]

Hi, Casey

On 2019/4/18 8:24, Casey Schaufler wrote:
> On 4/17/2019 4:39 PM, Paul Moore wrote:
>> On Wed, Apr 17, 2019 at 12:27 PM Oleg Nesterov <oleg@redhat.com> wrote:
>>> On 04/17, Paul Moore wrote:
>>>> On Wed, Apr 17, 2019 at 10:57 AM Oleg Nesterov <oleg@redhat.com> 
>>>> wrote:
>>>>> On 04/17, Paul Moore wrote:
>>>>>> I'm tempted to simply return an error in selinux_setprocattr() if
>>>>>> the task's credentials are not the same as its real_cred;
>>>>> What about other modules? I have no idea what smack_setprocattr() is,
>>>>> but it too does prepare_creds/commit creds.
>>>>>
>>>>> it seems that the simplest workaround should simply add the 
>>>>> additional
>>>>> cred == real_cred into proc_pid_attr_write().
>>>> Yes, that is simple, but I worry about what other LSMs might want to
>>>> do.  While I believe failing if the effective creds are not the same
>>>> as the real_creds is okay for SELinux (possibly Smack too), I worry
>>>> about what other LSMs may want to do.  After all,
>>>> proc_pid_attr_write() doesn't change the the creds itself, that is
>>>> something the specific LSMs do.
>>> Yes, but if proc_pid_attr_write() is called with cred != real_cred then
>>> something is already wrong?
>> True, or at least I would think so.
>>
>> Looking at the current tree there are three LSMs which implement
>> setprocattr hooks: SELinux, Smack, and AppArmor.  I know Casey has
>> already mentioned that he wasn't able to trigger the problem in Smack,
>> but looking at smack_setprocattr() I see the similar commit_creds()
>> usage so I would expect the same problem in Smack; what say you Casey?
>
> I say that my test program runs without ill effect. I call acct()
> with "/proc/self/attr/current", which succeeds and enables accounting
> just like it is supposed to. I then have the program open
> "/proc/self/attr/current" and read it, all of which goes swimmingly.
> When Smack frees a cred it usually does not free any memory of its
> own, so it is conceivable that I'm just getting lucky. Or, I may not
> have sufficient debug enabled.
>
>>   Looking at apparmor_setprocattr(), it appears that it too could end
>> up calling commit_creds() via aa_set_current_hat().
>>
>> Since it looks like all three LSMs which implement the setprocattr
>> hook are vulnerable I'm open to the idea that proc_pid_attr_write() is
>> a better choice for the cred != read_cred check, but I would want to
>> make sure John and Casey are okay with that.
>>
>> John?
>>
>> Casey?
>
> I'm fine with the change going into proc_pid_attr_write().
The cred != real_cred checking is not enough.

Consider this situation, when doing override, cred, real_cred and 
new_cred are all same:

after override_creds()    cred == real_cred == new1_cred
after prepare_creds()     new2_cred
after commit_creds()     becasue the check is false, so cred == 
real_cred == new2_cred
after revert_creds()        cred == new1_cred, real_cred == new2_cred

It will cause cred != real_cred finally.


Regards,
Yang

Re: kernel BUG at kernel/cred.c:434!

[Stephen Smalley]

On 4/17/19 12:42 PM, Casey Schaufler wrote:
> On 4/17/2019 9:27 AM, Oleg Nesterov wrote:
>> On 04/17, Paul Moore wrote:
>>> On Wed, Apr 17, 2019 at 10:57 AM Oleg Nesterov <oleg@redhat.com> wrote:
>>>> On 04/17, Paul Moore wrote:
>>>>> I'm tempted to simply return an error in selinux_setprocattr() if
>>>>> the task's credentials are not the same as its real_cred;
>>>> What about other modules? I have no idea what smack_setprocattr() is,
>>>> but it too does prepare_creds/commit creds.
>>>>
>>>> it seems that the simplest workaround should simply add the additional
>>>> cred == real_cred into proc_pid_attr_write().
>>> Yes, that is simple, but I worry about what other LSMs might want to
>>> do.  While I believe failing if the effective creds are not the same
>>> as the real_creds is okay for SELinux (possibly Smack too), I worry
>>> about what other LSMs may want to do.  After all,
>>> proc_pid_attr_write() doesn't change the the creds itself, that is
>>> something the specific LSMs do.
>> Yes, but if proc_pid_attr_write() is called with cred != real_cred then
>> something is already wrong?
>>
>> In fact, I think that something is already wrong if it is not called by
>> user-space directly. Too late to ask, but why is this /proc/self/attr/
>> magic not implemented via syscall(s) ?
> 
> Shell scripts, for one thing. It's a straightforward and appropriate
> use of the /proc interface. System calls would require additional change
> to existing programs, whereas using the /proc interface allows a good
> deal to be done in the containing scripts.

It is fairly awkward/fragile to use these interfaces from shell scripts 
due to limitations of the interface (e.g. no partial writes, thereby 
breaking if the shell splits up the data into multiple write() calls) 
and due to the fact that most of the attributes (except for current) are 
typically reset/cleared upon exec to prevent undue caller/callee 
influence.  It works well enough for echo -n "somelabel" > 
/proc/self/attr/current but not much else, and even that doesn't work 
without the -n due to multiple write() calls.

Just for the record, this functionality was originally implemented via 
separate system calls at least for SELinux (in its original kernel 
patch), then multiplexed through the single LSM security system call 
upon porting to LSM (before merging to mainline), but viro and others 
strongly favored using /proc as the interface for getting/setting any 
new process attributes.  Understandable, but it does impose some 
limitations, including a dependency on having a writable proc mount in 
the namespace of any process that needs to set any of these attributes,

Re: kernel BUG at kernel/cred.c:434!

[Paul Moore]

On Wed, Apr 17, 2019 at 10:50 PM Yang Yingliang
<yangyingliang@huawei.com> wrote:
> On 2019/4/18 8:24, Casey Schaufler wrote:
> > On 4/17/2019 4:39 PM, Paul Moore wrote:
> >>
> >> Since it looks like all three LSMs which implement the setprocattr
> >> hook are vulnerable I'm open to the idea that proc_pid_attr_write() is
> >> a better choice for the cred != read_cred check, but I would want to
> >> make sure John and Casey are okay with that.
> >>
> >> John?
> >>
> >> Casey?
> >
> > I'm fine with the change going into proc_pid_attr_write().
>
> The cred != real_cred checking is not enough.
>
> Consider this situation, when doing override, cred, real_cred and
> new_cred are all same:
>
> after override_creds()    cred == real_cred == new1_cred

I'm sorry, you've lost me.  After override_creds() returns
current->cred and current->real_cred are not going to be the same,
yes?

> after prepare_creds()     new2_cred
> after commit_creds()     becasue the check is false, so cred ==
> real_cred == new2_cred
> after revert_creds()        cred == new1_cred, real_cred == new2_cred
>
> It will cause cred != real_cred finally.

-- 
paul moore
www.paul-moore.com

Re: kernel BUG at kernel/cred.c:434!

[Yang Yingliang]

On 2019/4/19 10:04, Paul Moore wrote:
> On Wed, Apr 17, 2019 at 10:50 PM Yang Yingliang
> <yangyingliang@huawei.com> wrote:
>> On 2019/4/18 8:24, Casey Schaufler wrote:
>>> On 4/17/2019 4:39 PM, Paul Moore wrote:
>>>> Since it looks like all three LSMs which implement the setprocattr
>>>> hook are vulnerable I'm open to the idea that proc_pid_attr_write() is
>>>> a better choice for the cred != read_cred check, but I would want to
>>>> make sure John and Casey are okay with that.
>>>>
>>>> John?
>>>>
>>>> Casey?
>>> I'm fine with the change going into proc_pid_attr_write().
>> The cred != real_cred checking is not enough.
>>
>> Consider this situation, when doing override, cred, real_cred and
>> new_cred are all same:
>>
>> after override_creds()    cred == real_cred == new1_cred
> I'm sorry, you've lost me.  After override_creds() returns
> current->cred and current->real_cred are not going to be the same,
> yes?
It's possible the new  cred is equal to current->real_cred and 
current->cred,
so after overrides_creds(), they have the same value.
>
>> after prepare_creds()     new2_cred
>> after commit_creds()     becasue the check is false, so cred ==
>> real_cred == new2_cred
>> after revert_creds()        cred == new1_cred, real_cred == new2_cred
>>
>> It will cause cred != real_cred finally.

Re: kernel BUG at kernel/cred.c:434!

[Paul Moore]

On Thu, Apr 18, 2019 at 10:42 PM Yang Yingliang
<yangyingliang@huawei.com> wrote:
> On 2019/4/19 10:04, Paul Moore wrote:
> > On Wed, Apr 17, 2019 at 10:50 PM Yang Yingliang
> > <yangyingliang@huawei.com> wrote:
> >> On 2019/4/18 8:24, Casey Schaufler wrote:
> >>> On 4/17/2019 4:39 PM, Paul Moore wrote:
> >>>> Since it looks like all three LSMs which implement the setprocattr
> >>>> hook are vulnerable I'm open to the idea that proc_pid_attr_write() is
> >>>> a better choice for the cred != read_cred check, but I would want to
> >>>> make sure John and Casey are okay with that.
> >>>>
> >>>> John?
> >>>>
> >>>> Casey?
> >>> I'm fine with the change going into proc_pid_attr_write().
> >> The cred != real_cred checking is not enough.
> >>
> >> Consider this situation, when doing override, cred, real_cred and
> >> new_cred are all same:
> >>
> >> after override_creds()    cred == real_cred == new1_cred
> > I'm sorry, you've lost me.  After override_creds() returns
> > current->cred and current->real_cred are not going to be the same,
> > yes?
>
> It's possible the new  cred is equal to current->real_cred and
> current->cred,
> so after overrides_creds(), they have the same value.

Both task_struct.cred and task_struct.real_cred are pointer values,
assuming that one uses prepare_creds() to allocate/initialize a new
cred struct for use with override_creds() then the newly created cred
should never be equal to task_struct.real_cred.  Am I missing
something, or are you thinking of something else?

-- 
paul moore
www.paul-moore.com

Re: kernel BUG at kernel/cred.c:434!

[Yang Yingliang]

On 2019/4/19 21:24, Paul Moore wrote:
> On Thu, Apr 18, 2019 at 10:42 PM Yang Yingliang
> <yangyingliang@huawei.com> wrote:
>> On 2019/4/19 10:04, Paul Moore wrote:
>>> On Wed, Apr 17, 2019 at 10:50 PM Yang Yingliang
>>> <yangyingliang@huawei.com> wrote:
>>>> On 2019/4/18 8:24, Casey Schaufler wrote:
>>>>> On 4/17/2019 4:39 PM, Paul Moore wrote:
>>>>>> Since it looks like all three LSMs which implement the setprocattr
>>>>>> hook are vulnerable I'm open to the idea that proc_pid_attr_write() is
>>>>>> a better choice for the cred != read_cred check, but I would want to
>>>>>> make sure John and Casey are okay with that.
>>>>>>
>>>>>> John?
>>>>>>
>>>>>> Casey?
>>>>> I'm fine with the change going into proc_pid_attr_write().
>>>> The cred != real_cred checking is not enough.
>>>>
>>>> Consider this situation, when doing override, cred, real_cred and
>>>> new_cred are all same:
>>>>
>>>> after override_creds()    cred == real_cred == new1_cred
>>> I'm sorry, you've lost me.  After override_creds() returns
>>> current->cred and current->real_cred are not going to be the same,
>>> yes?
>> It's possible the new  cred is equal to current->real_cred and
>> current->cred,
>> so after overrides_creds(), they have the same value.
> Both task_struct.cred and task_struct.real_cred are pointer values,
> assuming that one uses prepare_creds() to allocate/initialize a new
> cred struct for use with override_creds() then the newly created cred
> should never be equal to task_struct.real_cred.  Am I missing
> something, or are you thinking of something else?
In do_acct_process(), file->f_cred may equal to current->real_cred, I 
confirm
it by adding some debug message in do_acct_process() like this:

--- a/kernel/acct.c
+++ b/kernel/acct.c
@@ -481,6 +481,7 @@ static void do_acct_process(struct bsd_acct_struct 
*acct)
         flim = current->signal->rlim[RLIMIT_FSIZE].rlim_cur;
         current->signal->rlim[RLIMIT_FSIZE].rlim_cur = RLIM_INFINITY;
         /* Perform file operations on behalf of whoever enabled 
accounting */
+       pr_info("task:%px new cred:%px real cred:%px cred:%px\n", 
current, file->f_cred, current->real_cred, current->cred);
         orig_cred = override_creds(file->f_cred);



Messages:
[   56.643298] task:ffff88841a9595c0 new cred:ffff88841ae450c0 real 
cred:ffff88841ae450c0 cred:ffff88841ae450c0    //They are same.
[   56.646609] Process accounting resumed
[   56.649943] task:ffff88841a9595c0 new cred:ffff88841ae450c0 real 
cred:ffff88841c96c300 cred:ffff88841ae450c0
[   56.653565] ------------[ cut here ]------------
[   56.655119] kernel BUG at kernel/cred.c:434!
[   56.656590] invalid opcode: 0000 [#1] SMP PTI
[   56.658033] CPU: 2 PID: 4169 Comm: syz-executor.15 Not tainted 
5.1.0-rc4-00034-g869e3305f23d-dirty #143
[   56.661077] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), 
BIOS rel-1.12.1-0-ga5cab58e9a3f-prebuilt.qemu.org 04/01/2014
[   56.664895] RIP: 0010:commit_creds+0x1eb/0x230
[   56.666344] Code: 43 1c 0f 85 08 ff ff ff e9 10 ff ff ff 8b 45 10 39 
43 10 0f 85 18 ff ff ff 8b 43 20 39 45 20 0f 85 0c ff ff ff e9 14 ff ff 
ff <0f> 0b 48 c7 c7 d0 d2 49 82 e8 17 3b 3e 00 0f 0b 48 c7 c7 c0 d2 49
[   56.672410] RSP: 0018:ffffc90003a17b20 EFLAGS: 00010287
[   56.674098] RAX: ffff88841a9595c0 RBX: ffff88841ae450c0 RCX: 
0000000000000000
[   56.676410] RDX: 0000000000000001 RSI: 0000000000000020 RDI: 
ffff88841c96ce40
[   56.678691] RBP: 0000000000000001 R08: 0000000000800000 R09: 
0000000000000000
[   56.680997] R10: ffff88841c9265a0 R11: ffffffff810d6940 R12: 
ffff88841a9595c0
[   56.681198] task:ffff88841a9195c0 new cred:ffff88841aeaa0c0 real 
cred:ffff88841aeaa0c0 cred:ffff88841aeaa0c0
[   56.683293] R13: 0000000000000040 R14: ffff88841c96ce40 R15: 
0000000000000040
[   56.683296] FS:  00007f5969a5c700(0000) GS:ffff88842fa80000(0000) 
knlGS:0000000000000000
[   56.683297] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   56.683299] CR2: 00007f82742214f0 CR3: 000000041cbc0005 CR4: 
00000000000206e0
[   56.683305] Call Trace:
[   56.683340]  selinux_setprocattr+0x17b/0x480
[   56.686513] Process accounting resumed
[   56.688849]  proc_pid_attr_write+0xc0/0xf0
[   56.688857]  __kernel_write+0x4f/0xf0
[   56.688866]  do_acct_process+0x538/0x750
[   56.703090]  ? __schedule+0x290/0x960
[   56.704311]  ? __queue_work+0x160/0x5c0
[   56.705571]  acct_pin_kill+0x1e/0x70
[   56.706743]  pin_kill+0x81/0x150
[   56.707813]  ? finish_wait+0x80/0x80
[   56.708985]  mnt_pin_kill+0x1e/0x30
[   56.710127]  cleanup_mnt+0x6e/0x70
[   56.711247]  task_work_run+0x8a/0xb0
[   56.712453]  do_exit+0x2e0/0xc80
[   56.713525]  do_group_exit+0x33/0xb0
[   56.714701]  get_signal+0x143/0x810
[   56.715865]  do_signal+0x36/0x610
[   56.716962]  ? __x64_sys_futex+0x134/0x180
[   56.718307]  ? _copy_to_user+0x22/0x30
[   56.719606]  exit_to_usermode_loop+0x80/0xe0
[   56.721003]  do_syscall_64+0x16c/0x180
[   56.722242]  entry_SYSCALL_64_after_hwframe+0x44/0xa9

Re: kernel BUG at kernel/cred.c:434!

[Paul Moore]

On Fri, Apr 19, 2019 at 10:34 AM Yang Yingliang
<yangyingliang@huawei.com> wrote:
> On 2019/4/19 21:24, Paul Moore wrote:
> > On Thu, Apr 18, 2019 at 10:42 PM Yang Yingliang
> > <yangyingliang@huawei.com> wrote:
> >> On 2019/4/19 10:04, Paul Moore wrote:
> >>> On Wed, Apr 17, 2019 at 10:50 PM Yang Yingliang
> >>> <yangyingliang@huawei.com> wrote:
> >>>> On 2019/4/18 8:24, Casey Schaufler wrote:
> >>>>> On 4/17/2019 4:39 PM, Paul Moore wrote:
> >>>>>> Since it looks like all three LSMs which implement the setprocattr
> >>>>>> hook are vulnerable I'm open to the idea that proc_pid_attr_write() is
> >>>>>> a better choice for the cred != read_cred check, but I would want to
> >>>>>> make sure John and Casey are okay with that.
> >>>>>>
> >>>>>> John?
> >>>>>>
> >>>>>> Casey?
> >>>>> I'm fine with the change going into proc_pid_attr_write().
> >>>> The cred != real_cred checking is not enough.
> >>>>
> >>>> Consider this situation, when doing override, cred, real_cred and
> >>>> new_cred are all same:
> >>>>
> >>>> after override_creds()    cred == real_cred == new1_cred
> >>> I'm sorry, you've lost me.  After override_creds() returns
> >>> current->cred and current->real_cred are not going to be the same,
> >>> yes?
> >> It's possible the new  cred is equal to current->real_cred and
> >> current->cred,
> >> so after overrides_creds(), they have the same value.
> > Both task_struct.cred and task_struct.real_cred are pointer values,
> > assuming that one uses prepare_creds() to allocate/initialize a new
> > cred struct for use with override_creds() then the newly created cred
> > should never be equal to task_struct.real_cred.  Am I missing
> > something, or are you thinking of something else?
>
> In do_acct_process(), file->f_cred may equal to current->real_cred, I
> confirm
> it by adding some debug message in do_acct_process() like this:

I would expect that; real_cred is the task's objective DAC
credentials, so using it for f_cred makes sense.

What we are now talking about is the task's subjective credentials,
which can be overridden via override_creds(), and are what the LSMs
change via proc_pid_attr_write().

> --- a/kernel/acct.c
> +++ b/kernel/acct.c
> @@ -481,6 +481,7 @@ static void do_acct_process(struct bsd_acct_struct
> *acct)
>          flim = current->signal->rlim[RLIMIT_FSIZE].rlim_cur;
>          current->signal->rlim[RLIMIT_FSIZE].rlim_cur = RLIM_INFINITY;
>          /* Perform file operations on behalf of whoever enabled
> accounting */
> +       pr_info("task:%px new cred:%px real cred:%px cred:%px\n",
> current, file->f_cred, current->real_cred, current->cred);
>          orig_cred = override_creds(file->f_cred);

-- 
paul moore
www.paul-moore.com

Re: kernel BUG at kernel/cred.c:434!

[Yang Yingliang]

On 2019/4/20 0:13, Paul Moore wrote:
> On Fri, Apr 19, 2019 at 10:34 AM Yang Yingliang
> <yangyingliang@huawei.com> wrote:
>> On 2019/4/19 21:24, Paul Moore wrote:
>>> On Thu, Apr 18, 2019 at 10:42 PM Yang Yingliang
>>> <yangyingliang@huawei.com> wrote:
>>>> On 2019/4/19 10:04, Paul Moore wrote:
>>>>> On Wed, Apr 17, 2019 at 10:50 PM Yang Yingliang
>>>>> <yangyingliang@huawei.com> wrote:
>>>>>> On 2019/4/18 8:24, Casey Schaufler wrote:
>>>>>>> On 4/17/2019 4:39 PM, Paul Moore wrote:
>>>>>>>> Since it looks like all three LSMs which implement the setprocattr
>>>>>>>> hook are vulnerable I'm open to the idea that proc_pid_attr_write() is
>>>>>>>> a better choice for the cred != read_cred check, but I would want to
>>>>>>>> make sure John and Casey are okay with that.
>>>>>>>>
>>>>>>>> John?
>>>>>>>>
>>>>>>>> Casey?
>>>>>>> I'm fine with the change going into proc_pid_attr_write().
>>>>>> The cred != real_cred checking is not enough.
>>>>>>
>>>>>> Consider this situation, when doing override, cred, real_cred and
>>>>>> new_cred are all same:
>>>>>>
>>>>>> after override_creds()    cred == real_cred == new1_cred
>>>>> I'm sorry, you've lost me.  After override_creds() returns
>>>>> current->cred and current->real_cred are not going to be the same,
>>>>> yes?
>>>> It's possible the new  cred is equal to current->real_cred and
>>>> current->cred,
>>>> so after overrides_creds(), they have the same value.
>>> Both task_struct.cred and task_struct.real_cred are pointer values,
>>> assuming that one uses prepare_creds() to allocate/initialize a new
>>> cred struct for use with override_creds() then the newly created cred
>>> should never be equal to task_struct.real_cred.  Am I missing
>>> something, or are you thinking of something else?
>> In do_acct_process(), file->f_cred may equal to current->real_cred, I
>> confirm
>> it by adding some debug message in do_acct_process() like this:
> I would expect that; real_cred is the task's objective DAC
> credentials, so using it for f_cred makes sense.
>
> What we are now talking about is the task's subjective credentials,
> which can be overridden via override_creds(), and are what the LSMs
> change via proc_pid_attr_write().
I'm not sure you got my point.

I was saying cred != real_cred check is not quite right, because the 
cred can
be overridden by a same pointer as my print messages showing.

"cred != real_cred" means override_creds() is called, but "cred == 
real_cred"
doesn't mean override_creds() is not called.

When we use "cred != real_cred" check, we may lost the situation that cred
is overridden by a same pointer. In this case, we will do 
override_creds() =>
commit_creds() => revert_creds(), this make cred != real_cred, when a new
commit_creds() is called, it also will trigger a BUG_ON().
>
>> --- a/kernel/acct.c
>> +++ b/kernel/acct.c
>> @@ -481,6 +481,7 @@ static void do_acct_process(struct bsd_acct_struct
>> *acct)
>>           flim = current->signal->rlim[RLIMIT_FSIZE].rlim_cur;
>>           current->signal->rlim[RLIMIT_FSIZE].rlim_cur = RLIM_INFINITY;
>>           /* Perform file operations on behalf of whoever enabled
>> accounting */
>> +       pr_info("task:%px new cred:%px real cred:%px cred:%px\n",
>> current, file->f_cred, current->real_cred, current->cred);
>>           orig_cred = override_creds(file->f_cred);

Re: kernel BUG at kernel/cred.c:434!

[Paul Moore]

On Sat, Apr 20, 2019 at 3:39 AM Yang Yingliang <yangyingliang@huawei.com> wrote:
> I'm not sure you got my point.

I went back and looked at your previous emails again to try and
understand what you are talking about, and I'm a little confused by
some of the output ...

> --- a/kernel/acct.c
> +++ b/kernel/acct.c
> @@ -481,6 +481,7 @@ static void do_acct_process(struct bsd_acct_struct
> *acct)
>          flim = current->signal->rlim[RLIMIT_FSIZE].rlim_cur;
>          current->signal->rlim[RLIMIT_FSIZE].rlim_cur = RLIM_INFINITY;
>          /* Perform file operations on behalf of whoever enabled
> accounting */
> +       pr_info("task:%px new cred:%px real cred:%px cred:%px\n",
> current, file->f_cred, current->real_cred, current->cred);
>          orig_cred = override_creds(file->f_cred);

Okay, with this patch applied we should the task/cred info when
do_acct_process is called.  Got it.

> Messages:
> [   56.643298] task:ffff88841a9595c0 new cred:ffff88841ae450c0 real
> cred:ffff88841ae450c0 cred:ffff88841ae450c0    //They are same.

Okay, it looks like do_acct_process() was called and f_cred,
real_cred, and cred are all the same.

> [   56.646609] Process accounting resumed

It looks like do_acct_process() has called check_free_space() now.  So
far so good.

> [   56.649943] task:ffff88841a9595c0 new cred:ffff88841ae450c0 real
> cred:ffff88841c96c300 cred:ffff88841ae450c0

Wait a minute ... why are we seeing this again?  Looking at the task
pointer and the timestamp, this is the same task exiting and trying to
write to the accounting file, yes?  This output is particularly
curious since it appears that real_cred has changed; where is this
happening?

> [   56.653565] ------------[ cut here ]------------
> [   56.655119] kernel BUG at kernel/cred.c:434!
> [   56.656590] invalid opcode: 0000 [#1] SMP PTI
> [   56.658033] CPU: 2 PID: 4169 Comm: syz-executor.15 Not tainted
> 5.1.0-rc4-00034-g869e3305f23d-dirty #143
> [   56.661077] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
> BIOS rel-1.12.1-0-ga5cab58e9a3f-prebuilt.qemu.org 04/01/2014
> [   56.664895] RIP: 0010:commit_creds+0x1eb/0x230
> [   56.666344] Code: 43 1c 0f 85 08 ff ff ff e9 10 ff ff ff 8b 45 10 39
> 43 10 0f 85 18 ff ff ff 8b 43 20 39 45 20 0f 85 0c ff ff ff e9 14 ff ff
> ff <0f> 0b 48 c7 c7 d0 d2 49 82 e8 17 3b 3e 00 0f 0b 48 c7 c7 c0 d2 49
> [   56.672410] RSP: 0018:ffffc90003a17b20 EFLAGS: 00010287
> [   56.674098] RAX: ffff88841a9595c0 RBX: ffff88841ae450c0 RCX:
> 0000000000000000
> [   56.676410] RDX: 0000000000000001 RSI: 0000000000000020 RDI:
> ffff88841c96ce40
> [   56.678691] RBP: 0000000000000001 R08: 0000000000800000 R09:
> 0000000000000000
> [   56.680997] R10: ffff88841c9265a0 R11: ffffffff810d6940 R12:
> ffff88841a9595c0
> [   56.681198] task:ffff88841a9195c0 new cred:ffff88841aeaa0c0 real
> cred:ffff88841aeaa0c0 cred:ffff88841aeaa0c0
> [   56.683293] R13: 0000000000000040 R14: ffff88841c96ce40 R15:
> 0000000000000040
> [   56.683296] FS:  00007f5969a5c700(0000) GS:ffff88842fa80000(0000)
> knlGS:0000000000000000
> [   56.683297] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [   56.683299] CR2: 00007f82742214f0 CR3: 000000041cbc0005 CR4:
> 00000000000206e0
> [   56.683305] Call Trace:
> [   56.683340]  selinux_setprocattr+0x17b/0x480
> [   56.686513] Process accounting resumed
> [   56.688849]  proc_pid_attr_write+0xc0/0xf0
> [   56.688857]  __kernel_write+0x4f/0xf0
> [   56.688866]  do_acct_process+0x538/0x750
> [   56.703090]  ? __schedule+0x290/0x960
> [   56.704311]  ? __queue_work+0x160/0x5c0
> [   56.705571]  acct_pin_kill+0x1e/0x70
> [   56.706743]  pin_kill+0x81/0x150
> [   56.707813]  ? finish_wait+0x80/0x80
> [   56.708985]  mnt_pin_kill+0x1e/0x30
> [   56.710127]  cleanup_mnt+0x6e/0x70
> [   56.711247]  task_work_run+0x8a/0xb0
> [   56.712453]  do_exit+0x2e0/0xc80
> [   56.713525]  do_group_exit+0x33/0xb0
> [   56.714701]  get_signal+0x143/0x810
> [   56.715865]  do_signal+0x36/0x610
> [   56.716962]  ? __x64_sys_futex+0x134/0x180
> [   56.718307]  ? _copy_to_user+0x22/0x30
> [   56.719606]  exit_to_usermode_loop+0x80/0xe0
> [   56.721003]  do_syscall_64+0x16c/0x180
> [   56.722242]  entry_SYSCALL_64_after_hwframe+0x44/0xa9

-- 
paul moore
www.paul-moore.com

Re: kernel BUG at kernel/cred.c:434!

[Yang Yingliang]

On 2019/4/23 3:48, Paul Moore wrote:
> On Sat, Apr 20, 2019 at 3:39 AM Yang Yingliang <yangyingliang@huawei.com> wrote:
>> I'm not sure you got my point.
> I went back and looked at your previous emails again to try and
> understand what you are talking about, and I'm a little confused by
> some of the output ...
>
>> --- a/kernel/acct.c
>> +++ b/kernel/acct.c
>> @@ -481,6 +481,7 @@ static void do_acct_process(struct bsd_acct_struct
>> *acct)
>>           flim = current->signal->rlim[RLIMIT_FSIZE].rlim_cur;
>>           current->signal->rlim[RLIMIT_FSIZE].rlim_cur = RLIM_INFINITY;
>>           /* Perform file operations on behalf of whoever enabled
>> accounting */
>> +       pr_info("task:%px new cred:%px real cred:%px cred:%px\n",
>> current, file->f_cred, current->real_cred, current->cred);
>>           orig_cred = override_creds(file->f_cred);
> Okay, with this patch applied we should the task/cred info when
> do_acct_process is called.  Got it.
>
>> Messages:
>> [   56.643298] task:ffff88841a9595c0 new cred:ffff88841ae450c0 real
>> cred:ffff88841ae450c0 cred:ffff88841ae450c0    //They are same.
> Okay, it looks like do_acct_process() was called and f_cred,
> real_cred, and cred are all the same.
This is a original message, without patch applied.
>
>> [   56.646609] Process accounting resumed
> It looks like do_acct_process() has called check_free_space() now.  So
> far so good.
>
>> [   56.649943] task:ffff88841a9595c0 new cred:ffff88841ae450c0 real
>> cred:ffff88841c96c300 cred:ffff88841ae450c0
> Wait a minute ... why are we seeing this again?  Looking at the task
> pointer and the timestamp, this is the same task exiting and trying to
> write to the accounting file, yes?  This output is particularly
> curious since it appears that real_cred has changed; where is this
> happening?
This is the message when the BUG_ON was triggered without applying any
fix patch.


If we apply this patch "proc: prevent changes to overridden 
credentials", program
runs like this:

1. As print message shows, before overriden, the pointer has the 
following value:
     real_cread=cred=0xffff88841ae450c0, f_cred=0xffff88841ae450c0
     override_creds() is called in do_acct_process():
     ...
     /* Perform file operations on behalf of whoever enabled accounting */
     orig_cred = override_creds(file->f_cred);
     ...


2. After override_creds(), if (current_cred() != current_real_cred()) is 
not work here,
we will call commit_creds()  in security_setprocattr().
     ...
     /* Prevent changes to overridden credentials. */
         if (current_cred() != current_real_cred()) {
             rcu_read_unlock();
         return -EBUSY;
     }
     ...


3. After commit_creds(), we have new cred and real_cred.
     security_setprocattr()    //commit_creds is called here

4. revert_creds() is called in in do_acct_process(), the cred
is reverted to the old value(0xffff88841ae450c0)
     ...
     current->signal->rlim[RLIMIT_FSIZE].rlim_cur = flim;
     revert_creds(orig_cred);

5. After reverting, cred and real_cred are not equal.
If it has a risk to trigger the BUG_ON, when doing another
commit_creds() ?

Re: kernel BUG at kernel/cred.c:434!

[Paul Moore]

On Tue, Apr 23, 2019 at 12:08 AM Yang Yingliang
<yangyingliang@huawei.com> wrote:
> On 2019/4/23 3:48, Paul Moore wrote:
> > On Sat, Apr 20, 2019 at 3:39 AM Yang Yingliang <yangyingliang@huawei.com> wrote:
> >> I'm not sure you got my point.
> > I went back and looked at your previous emails again to try and
> > understand what you are talking about, and I'm a little confused by
> > some of the output ...
> >
> >> --- a/kernel/acct.c
> >> +++ b/kernel/acct.c
> >> @@ -481,6 +481,7 @@ static void do_acct_process(struct bsd_acct_struct
> >> *acct)
> >>           flim = current->signal->rlim[RLIMIT_FSIZE].rlim_cur;
> >>           current->signal->rlim[RLIMIT_FSIZE].rlim_cur = RLIM_INFINITY;
> >>           /* Perform file operations on behalf of whoever enabled
> >> accounting */
> >> +       pr_info("task:%px new cred:%px real cred:%px cred:%px\n",
> >> current, file->f_cred, current->real_cred, current->cred);
> >>           orig_cred = override_creds(file->f_cred);
> > Okay, with this patch applied we should the task/cred info when
> > do_acct_process is called.  Got it.
> >
> >> Messages:
> >> [   56.643298] task:ffff88841a9595c0 new cred:ffff88841ae450c0 real
> >> cred:ffff88841ae450c0 cred:ffff88841ae450c0    //They are same.
> > Okay, it looks like do_acct_process() was called and f_cred,
> > real_cred, and cred are all the same.
>
> This is a original message, without patch applied.

The patch I am referring to is your pr_info patch (above).  I'm not
talking about any other patches at the moment; I just want to
understand the example dmesg output you copied into your email.

With that in mind, the message above seems to indicate that
do_acct_process() has been invoked with f_cred, real_cred, and cred
all pointing to the same credentials struct, yes?

> >> [   56.646609] Process accounting resumed
> > It looks like do_acct_process() has called check_free_space() now.  So
> > far so good.

...

> >> [   56.649943] task:ffff88841a9595c0 new cred:ffff88841ae450c0 real
> >> cred:ffff88841c96c300 cred:ffff88841ae450c0
> > Wait a minute ... why are we seeing this again?  Looking at the task
> > pointer and the timestamp, this is the same task exiting and trying to
> > write to the accounting file, yes?  This output is particularly
> > curious since it appears that real_cred has changed; where is this
> > happening?
>
> This is the message when the BUG_ON was triggered without applying any
> fix patch.

The only place in the code that generates this message is the bit of
code that you patches in using the pr_info() patch (above), yes?  If
so, that would seem to indicate that the same task is calling
do_acct_process() twice, yes?  I may be fundamentally misunderstanding
something about process accounting, but I though do_acct_process()
would only be called once for a given task - while it was exiting.
Yes, no?

> If we apply this patch "proc: prevent changes to overridden
> credentials", program
> runs like this:

I'd like to focus on understanding the dmesg output you shared first,
because it doesn't seem to make sense to me.

-- 
paul moore
www.paul-moore.com