Messages in this thread |  | | From | Jann Horn <> | Date | Tue, 28 Jan 2020 13:19:56 +0100 | Subject | Re: [PATCH] cred: Use RCU primitives to access RCU pointers |
| |
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.
|  |