linux-kernel - Re: [PATCH 2/2] cgroup: Add a allow

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 6 Oct 2016 15:43:51 -0700
From:   Dmitry Torokhov <dmitry.torokhov@...il.com>
To:     John Stultz <john.stultz@...aro.org>
Cc:     lkml <linux-kernel@...r.kernel.org>,
        Rom Lemarchand <romlem@...roid.com>, Tejun Heo <tj@...nel.org>,
        Li Zefan <lizefan@...wei.com>,
        Jonathan Corbet <corbet@....net>, cgroups@...r.kernel.org,
        Android Kernel Team <kernel-team@...roid.com>,
        Colin Cross <ccross@...roid.com>,
        Dmitry Shmidt <dimitrysh@...gle.com>,
        Todd Kjos <tkjos@...gle.com>,
        Christian Poetzsch <christian.potzsch@...tec.com>,
        Amit Pundir <amit.pundir@...aro.org>,
        Ricky Zhou <rickyz@...omium.org>
Subject: Re: [PATCH 2/2] cgroup: Add a allow_attach policy for Android

On Wed, Oct 05, 2016 at 12:18:17PM -0700, John Stultz wrote:
> On Wed, Oct 5, 2016 at 12:10 PM, Dmitry Torokhov
> <dmitry.torokhov@...il.com> wrote:
> > On Mon, Oct 03, 2016 at 09:41:30PM -0700, John Stultz wrote:
> >> +#ifdef CONFIG_CGROUP_NICE_ATTACH
> >> +int cgroup_nice_allow_attach(struct cgroup_taskset *tset)
> >> +{
> >> +     const struct cred *cred = current_cred(), *tcred;
> >> +     struct task_struct *task;
> >> +     struct cgroup_subsys_state *css;
> >> +
> >> +     if (capable(CAP_SYS_NICE))
> >> +             return 0;
> >> +
> >> +     cgroup_taskset_for_each(task, css, tset) {
> >> +             tcred = __task_cred(task);
> >
> > __task_cred() requires RCU lock (courtesy Ricky Z).
> 
> Again, hopefully this isn't an issue with the new approach, but for
> the short term I'll see if we can get this fixed in the android tree.
> 

Actually, it should all be simply removed from there right away, as this
ends up being basically noop (but with all the locking violations and
races):

cgroup_taskset_for_each() needs tasks to be placed on cset->mg_tasks
list, but nobody does this in the ->allow_access() code path, so this
loops always executes exactly 0 times and the whole thing is exactly
equivalent of doing

	return capable(CAP_SYS_NICE) ? 0 : -EACESS;

which can be done right in cgroup_procs_write_permission().

Thanks.

-- 
Dmitry