From: Jann Horn <jannh@google.com> To: Oleg Nesterov <oleg@redhat.com> Cc: Amol Grover <frextrite@gmail.com>, David Howells <dhowells@redhat.com>, Shakeel Butt <shakeelb@google.com>, James Morris <jamorris@linux.microsoft.com>, Kees Cook <keescook@chromium.org>, Thomas Gleixner <tglx@linutronix.de>, kernel list <linux-kernel@vger.kernel.org>, linux-kernel-mentees@lists.linuxfoundation.org, Joel Fernandes <joel@joelfernandes.org>, Madhuparna Bhowmik <madhuparnabhowmik04@gmail.com>, "Paul E . McKenney" <paulmck@kernel.org>, Casey Schaufler <casey@schaufler-ca.com> Subject: Re: [PATCH] cred: Use RCU primitives to access RCU pointers Date: Tue, 28 Jan 2020 13:19:56 +0100 [thread overview] Message-ID: <CAG48ez2rJZGQyhOcgWe7NwLOxk-CBJugUFMM0Oa9JyuPamRwCg@mail.gmail.com> (raw) In-Reply-To: <20200128114818.GA17943@redhat.com> On Tue, Jan 28, 2020 at 12:48 PM Oleg Nesterov <oleg@redhat.com> wrote: > On 01/28, Jann Horn wrote: > > On Tue, Jan 28, 2020 at 8:28 AM Amol Grover <frextrite@gmail.com> wrote: > > > task_struct.cred and task_struct.real_cred are annotated by __rcu, > > > > task_struct.cred doesn't actually have RCU semantics though, see > > commit d7852fbd0f0423937fa287a598bfde188bb68c22. For task_struct.cred, > > it would probably be more correct to remove the __rcu annotation? > > Yes, but get_task_cred() assumes it has RCU semantics... Oh, huh. AFAICS get_task_cred() makes no sense semantically, and I think it ought to be deleted. There are the following users at the moment: rdtgroup_task_write_permission() - uses it for acting on a task as object, should be using objective credentials instead. __cgroup1_procs_write() - same thing. task_state() - should also be using objective credentials instead, you wouldn't want a task to show up as belonging to root in there just because it's in the middle of some overlayfs filesystem method or something like that. apparmor_getprocattr() - same thing as for task_state() rpcauth_bind_root_cred() - should also be using objective credentials instead, if init is in overlayfs or whatever, you probably wouldn't want that to have an effect here prepare_kernel_cred() - most callers pass NULL and don't hit this codepath, and those that don't pass NULL use `current`, so there can be no concurrent modification. Maybe this could be rewritten to something like this: if (daemon) { BUG_ON(daemon != current); old = get_current_cred(); } else { ... } or maybe it could just use the objective creds, it shouldn't matter here, I think. > To be honest I didn't know we have this helper. Same here. > Can't it race with revert_creds() in > do_faccessat() ? Yeah, I think you can probably trigger use-after-free reads with that. I also remember seeing something fishy in Smack at some point that had similar issues... smack_file_send_sigiotask() does smk_of_task(smack_cred(tsk->cred)), which looks very wrong.
WARNING: multiple messages have this Message-ID (diff)
From: Jann Horn via Linux-kernel-mentees <linux-kernel-mentees@lists.linuxfoundation.org> To: Oleg Nesterov <oleg@redhat.com> Cc: Kees Cook <keescook@chromium.org>, "Paul E . McKenney" <paulmck@kernel.org>, kernel list <linux-kernel@vger.kernel.org>, James Morris <jamorris@linux.microsoft.com>, David Howells <dhowells@redhat.com>, Casey Schaufler <casey@schaufler-ca.com>, Shakeel Butt <shakeelb@google.com>, Joel Fernandes <joel@joelfernandes.org>, Thomas Gleixner <tglx@linutronix.de>, linux-kernel-mentees@lists.linuxfoundation.org Subject: Re: [Linux-kernel-mentees] [PATCH] cred: Use RCU primitives to access RCU pointers Date: Tue, 28 Jan 2020 13:19:56 +0100 [thread overview] Message-ID: <CAG48ez2rJZGQyhOcgWe7NwLOxk-CBJugUFMM0Oa9JyuPamRwCg@mail.gmail.com> (raw) In-Reply-To: <20200128114818.GA17943@redhat.com> On Tue, Jan 28, 2020 at 12:48 PM Oleg Nesterov <oleg@redhat.com> wrote: > On 01/28, Jann Horn wrote: > > On Tue, Jan 28, 2020 at 8:28 AM Amol Grover <frextrite@gmail.com> wrote: > > > task_struct.cred and task_struct.real_cred are annotated by __rcu, > > > > task_struct.cred doesn't actually have RCU semantics though, see > > commit d7852fbd0f0423937fa287a598bfde188bb68c22. For task_struct.cred, > > it would probably be more correct to remove the __rcu annotation? > > Yes, but get_task_cred() assumes it has RCU semantics... Oh, huh. AFAICS get_task_cred() makes no sense semantically, and I think it ought to be deleted. There are the following users at the moment: rdtgroup_task_write_permission() - uses it for acting on a task as object, should be using objective credentials instead. __cgroup1_procs_write() - same thing. task_state() - should also be using objective credentials instead, you wouldn't want a task to show up as belonging to root in there just because it's in the middle of some overlayfs filesystem method or something like that. apparmor_getprocattr() - same thing as for task_state() rpcauth_bind_root_cred() - should also be using objective credentials instead, if init is in overlayfs or whatever, you probably wouldn't want that to have an effect here prepare_kernel_cred() - most callers pass NULL and don't hit this codepath, and those that don't pass NULL use `current`, so there can be no concurrent modification. Maybe this could be rewritten to something like this: if (daemon) { BUG_ON(daemon != current); old = get_current_cred(); } else { ... } or maybe it could just use the objective creds, it shouldn't matter here, I think. > To be honest I didn't know we have this helper. Same here. > Can't it race with revert_creds() in > do_faccessat() ? Yeah, I think you can probably trigger use-after-free reads with that. I also remember seeing something fishy in Smack at some point that had similar issues... smack_file_send_sigiotask() does smk_of_task(smack_cred(tsk->cred)), which looks very wrong. _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
next prev parent reply other threads:[~2020-01-28 12:20 UTC|newest] Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-01-28 7:27 [PATCH] cred: Use RCU primitives to access RCU pointers Amol Grover 2020-01-28 7:27 ` [Linux-kernel-mentees] " Amol Grover 2020-01-28 9:30 ` Jann Horn 2020-01-28 9:30 ` [Linux-kernel-mentees] " Jann Horn via Linux-kernel-mentees 2020-01-28 11:48 ` Oleg Nesterov 2020-01-28 11:48 ` [Linux-kernel-mentees] " Oleg Nesterov 2020-01-28 12:19 ` Jann Horn [this message] 2020-01-28 12:19 ` Jann Horn via Linux-kernel-mentees 2020-01-28 12:38 ` Oleg Nesterov 2020-01-28 12:38 ` [Linux-kernel-mentees] " Oleg Nesterov 2020-01-28 17:04 ` Amol Grover 2020-01-28 17:04 ` [Linux-kernel-mentees] " Amol Grover 2020-01-28 19:09 ` Jann Horn 2020-01-28 19:09 ` [Linux-kernel-mentees] " Jann Horn via Linux-kernel-mentees 2020-01-29 6:57 ` Amol Grover 2020-01-29 6:57 ` [Linux-kernel-mentees] " Amol Grover 2020-01-29 14:14 ` Jann Horn 2020-01-29 14:14 ` [Linux-kernel-mentees] " Jann Horn via Linux-kernel-mentees 2020-02-06 1:32 ` Joel Fernandes 2020-02-06 1:32 ` [Linux-kernel-mentees] " Joel Fernandes 2020-02-06 11:28 ` Jann Horn 2020-02-06 11:28 ` [Linux-kernel-mentees] " Jann Horn via Linux-kernel-mentees 2020-02-06 16:49 ` Joel Fernandes 2020-02-06 16:49 ` [Linux-kernel-mentees] " Joel Fernandes 2020-02-06 17:15 ` Jann Horn 2020-02-06 17:15 ` [Linux-kernel-mentees] " Jann Horn via Linux-kernel-mentees 2020-02-06 18:08 ` Joel Fernandes 2020-02-06 18:08 ` [Linux-kernel-mentees] " Joel Fernandes 2020-02-06 13:09 ` Amol Grover 2020-02-06 13:09 ` [Linux-kernel-mentees] " Amol Grover 2020-01-28 15:00 ` David Howells 2020-01-28 15:00 ` [Linux-kernel-mentees] " David Howells 2020-01-31 17:49 ` David Howells 2020-01-31 17:49 ` [Linux-kernel-mentees] " David Howells
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=CAG48ez2rJZGQyhOcgWe7NwLOxk-CBJugUFMM0Oa9JyuPamRwCg@mail.gmail.com \ --to=jannh@google.com \ --cc=casey@schaufler-ca.com \ --cc=dhowells@redhat.com \ --cc=frextrite@gmail.com \ --cc=jamorris@linux.microsoft.com \ --cc=joel@joelfernandes.org \ --cc=keescook@chromium.org \ --cc=linux-kernel-mentees@lists.linuxfoundation.org \ --cc=linux-kernel@vger.kernel.org \ --cc=madhuparnabhowmik04@gmail.com \ --cc=oleg@redhat.com \ --cc=paulmck@kernel.org \ --cc=shakeelb@google.com \ --cc=tglx@linutronix.de \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.