On Wed, Oct 23, 2019 at 7:06 PM Dmitry Vyukov <dvyukov@xxxxxxxxxx> wrote: > > On Thu, Oct 17, 2019 at 8:33 PM 'Marco Elver' via syzkaller-bugs > <syzkaller-bugs@xxxxxxxxxxxxxxxx> wrote: > > > > On Thu, 17 Oct 2019 at 20:17, Alexey Dobriyan <adobriyan@xxxxxxxxx> wrote: > > > > > > On Thu, Oct 17, 2019 at 02:56:47PM +0200, Marco Elver wrote: > > > > Hi, > > > > > > > > On Thu, 17 Oct 2019 at 14:36, syzbot > > > > <syzbot+e392f8008a294fdf8891@xxxxxxxxxxxxxxxxxxxxxxxxx> wrote: > > > > > > > > > > Hello, > > > > > > > > > > syzbot found the following crash on: > > > > > > > > > > HEAD commit: d724f94f x86, kcsan: Enable KCSAN for x86 > > > > > git tree: https://github.com/google/ktsan.git kcsan > > > > > console output: https://syzkaller.appspot.com/x/log.txt?x=17884db3600000 > > > > > kernel config: https://syzkaller.appspot.com/x/.config?x=c0906aa620713d80 > > > > > dashboard link: https://syzkaller.appspot.com/bug?extid=e392f8008a294fdf8891 > > > > > compiler: gcc (GCC) 9.0.0 20181231 (experimental) > > > > > > > > > > Unfortunately, I don't have any reproducer for this crash yet. > > > > > > > > > > IMPORTANT: if you fix the bug, please add the following tag to the commit: > > > > > Reported-by: syzbot+e392f8008a294fdf8891@xxxxxxxxxxxxxxxxxxxxxxxxx > > > > > > > > > > ================================================================== > > > > > BUG: KCSAN: data-race in task_dump_owner / task_dump_owner > > > > > > > > > > write to 0xffff8881255bb7fc of 4 bytes by task 7804 on cpu 0: > > > > > task_dump_owner+0xd8/0x260 fs/proc/base.c:1742 > > > > > pid_update_inode+0x3c/0x70 fs/proc/base.c:1818 > > > > > pid_revalidate+0x91/0xd0 fs/proc/base.c:1841 > > > > > d_revalidate fs/namei.c:765 [inline] > > > > > d_revalidate fs/namei.c:762 [inline] > > > > > lookup_fast+0x7cb/0x7e0 fs/namei.c:1613 > > > > > walk_component+0x6d/0xe80 fs/namei.c:1804 > > > > > link_path_walk.part.0+0x5d3/0xa90 fs/namei.c:2139 > > > > > link_path_walk fs/namei.c:2070 [inline] > > > > > path_openat+0x14f/0x3530 fs/namei.c:3532 > > > > > do_filp_open+0x11e/0x1b0 fs/namei.c:3563 > > > > > do_sys_open+0x3b3/0x4f0 fs/open.c:1089 > > > > > __do_sys_open fs/open.c:1107 [inline] > > > > > __se_sys_open fs/open.c:1102 [inline] > > > > > __x64_sys_open+0x55/0x70 fs/open.c:1102 > > > > > do_syscall_64+0xcf/0x2f0 arch/x86/entry/common.c:296 > > > > > entry_SYSCALL_64_after_hwframe+0x44/0xa9 > > > > > > > > > > write to 0xffff8881255bb7fc of 4 bytes by task 7813 on cpu 1: > > > > > task_dump_owner+0xd8/0x260 fs/proc/base.c:1742 > > > > > pid_update_inode+0x3c/0x70 fs/proc/base.c:1818 > > > > > pid_revalidate+0x91/0xd0 fs/proc/base.c:1841 > > > > > d_revalidate fs/namei.c:765 [inline] > > > > > d_revalidate fs/namei.c:762 [inline] > > > > > lookup_fast+0x7cb/0x7e0 fs/namei.c:1613 > > > > > walk_component+0x6d/0xe80 fs/namei.c:1804 > > > > > lookup_last fs/namei.c:2271 [inline] > > > > > path_lookupat.isra.0+0x13a/0x5a0 fs/namei.c:2316 > > > > > filename_lookup+0x145/0x2d0 fs/namei.c:2346 > > > > > user_path_at_empty+0x4c/0x70 fs/namei.c:2606 > > > > > user_path_at include/linux/namei.h:60 [inline] > > > > > vfs_statx+0xd9/0x190 fs/stat.c:187 > > > > > vfs_stat include/linux/fs.h:3188 [inline] > > > > > __do_sys_newstat+0x51/0xb0 fs/stat.c:341 > > > > > __se_sys_newstat fs/stat.c:337 [inline] > > > > > __x64_sys_newstat+0x3a/0x50 fs/stat.c:337 > > > > > do_syscall_64+0xcf/0x2f0 arch/x86/entry/common.c:296 > > > > > entry_SYSCALL_64_after_hwframe+0x44/0xa9 > > > > > > > > > > Reported by Kernel Concurrency Sanitizer on: > > > > > CPU: 1 PID: 7813 Comm: ps Not tainted 5.3.0+ #0 > > > > > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS > > > > > Google 01/01/2011 > > > > > ================================================================== > > > > > > > > My understanding is, that for every access to /proc/<pid>, > > > > d_revalidate is called, and /proc-fs implementation simply says that > > > > pid_revalidate always revalidates by rewriting uid/gid because "owning > > > > task may have performed a setuid(), etc." presumably so every access > > > > to a /proc/<pid> entry always has the right uid/gid (in effect > > > > updating /proc/<pid> lazily via d_revalidate). > > > > > > > > Is it possible that one of the tasks above could be preempted after > > > > doing its writes to *ruid/*rgid, another thread writing some other > > > > values (after setuid / seteuid), and then the preempted thread seeing > > > > the other values? Assertion here should never fail: > > > > === TASK 1 === > > > > | seteuid(1000); > > > > | seteuid(0); > > > > | stat("/proc/<pid-of-task-1>", &fstat); > > > > | assert(fstat.st_uid == 0); > > > > === TASK 2 === > > > > | stat("/proc/<pid-of-task-1>", ...); > > > > > > Is it the same as > > > pid_revalidate() snapshots (uid,gid) correctly > > > but writeback is done in any order? > > > > Yes, I think so. Snapshot is done in RCU reader critical section, but > > the writes can race with another thread. Is there logic that ensures > > this doesn't lead to the observable outcome above? > > > I found the case where this leads to an observable bug. > common_perm_cond() in security/apparmor/lsm.c reads the inode uid and > uses it for the security check: > > static int common_perm_cond(const char *op, const struct path *path, u32 mask) > { > struct path_cond cond = { d_backing_inode(path->dentry)->i_uid, > > d_backing_inode(path->dentry)->i_mode > }; > > Now consider the following test program: > > #define _GNU_SOURCE > #include <stdio.h> > #include <sys/stat.h> > #include <sys/types.h> > #include <unistd.h> > #include <stdlib.h> > #include <pthread.h> > > void *thr(void *arg) > { > for (;;) { > struct stat file_stat; > stat((char*)arg, &file_stat); > } > return 0; > } > > int main(int argc, char *argv[]) > { > char proc[32]; > sprintf(proc, "/proc/%d", getpid()); > printf("%s\n", proc); > pthread_t th; > pthread_create(&th, 0, thr, proc); > for (;;) { > seteuid(1000); > usleep(1); > seteuid(0); > struct stat file_stat; > stat(proc, &file_stat); > } > return 0; > } > > Whenever the main thread does stat, it must observe inode.uid == 0 in > common_perm_cond(). > > But since task_dump_owner() does writeback out of order, it can lead > to non-linearizable executions and main thread observing inode.uid == > 1000. > This in turn can lead to both false negatives and false positives from > AppArmour (false denying access and falsely permitting access). > > I don't know how to setup actual AppArmour profile to do this, but I > see this guide mentions "owner @{PROC}/[0-9]*" in a policy, so I > assume it's possible: > https://gitlab.com/apparmor/apparmor/wikis/Profiling_by_hand > > Instead, I added the following check to common_perm_cond() (it's > dirty, but you get the idea): > > @@ -218,6 +218,15 @@ static int common_perm_cond(const char *op, const > struct path *path, u32 mask) > d_backing_inode(path->dentry)->i_mode > }; > + if (op == OP_GETATTR && mask == AA_MAY_GETATTR && cond.uid.val != 0) { > + char buf1[64], buf2[64]; > + char *str = d_path(path, buf1, sizeof(buf1)); > + sprintf(buf2, "/proc/%d", current->pid); > + if (!strcmp(str, buf2)) > + pr_err("common_perm_cond: path=%s pid=%d uid=%d\n", > + str, current->pid, cond.uid.val); > + } > > Now when I run the program, I see how it fires every few seconds: > > # ./a.out > /proc/1548 > [ 123.233107] common_perm_cond: path=/proc/1548 pid=1548 uid=1000 > [ 126.142869] common_perm_cond: path=/proc/1548 pid=1548 uid=1000 > [ 127.048353] common_perm_cond: path=/proc/1548 pid=1548 uid=1000 > [ 128.181873] common_perm_cond: path=/proc/1548 pid=1548 uid=1000 > [ 128.557104] common_perm_cond: path=/proc/1548 pid=1548 uid=1000 > [ 144.690774] common_perm_cond: path=/proc/1548 pid=1548 uid=1000 > > Which means AppArmour acts based on the wrong UID. Obviously can lead > to falsely denying access, but also falsely permitting access. > Consider the following scenario. > A process sets owner UID on a file so that a child process won't be > able to access it, after that it starts the child process. > common_perm_cond() in the child process should observe the new owner > UID. However, if there a random other process simply doing stat() or > something similar on the file, now the common_perm_cond() in the child > can suddenly observe the old UID, which will be permitted by > AppArmour. Boom! > > I've tried to apply "proc: fix inode uid/gid writeback race": > https://lore.kernel.org/lkml/20191020173010.GA14744@avx2/ > but it does _not_ help because it does not really resolve the > non-atomic snapshot and writeback of UID. FTR here is the corresponding race report: https://groups.google.com/d/msg/syzkaller-bugs/Cs07ly_Nmtg/avW672LrAQAJ
