[PATCH] cgroup: Add new capability to allow a process to migrate other tasks between cgroups

[PATCH] cgroup: Add new capability to allow a process to migrate other tasks between cgroups

[John Stultz]

This patch adds CAP_GROUP_MIGRATE and logic to allows a process
to migrate other tasks between cgroups.

In Android (where this feature originated), the ActivityManager tracks
various application states (TOP_APP, FOREGROUND, BACKGROUND, SYSTEM,
etc), and then as applications change states, the SchedPolicy logic
will migrate the application tasks between different cgroups used
to control the different application states (for example, there is a
background cpuset cgroup which can limit background tasks to stay
on one low-power cpu, and the bg_non_interactive cpuctrl cgroup can
then further limit those background tasks to a small percentage of
that one cpu's cpu time).

However, for security reasons, Android doesn't want to make the
system_server (the process that runs the ActivityManager and
SchedPolicy logic), run as root. So in the Android common.git
kernel, they have some logic to allow cgroups to loosen their
permissions so CAP_SYS_NICE tasks can migrate other tasks between
cgroups.

The approach taken there overloads CAP_SYS_NICE a bit much, and
is maybe more complicated then needed.

So this patch, as suggested by Tejun,  simply adds a new process
capability flag (CAP_CGROUP_MIGRATE), and uses it when checking
if a task can migrate other tasks between cgroups.

I've tested this with AOSP master (though its a bit hacked in as I
still need to properly get the selinux bits aware of the new
capability bit) with selinux set to permissive and it seems to be
working well.

Thoughts and feedback would be appreciated!

Cc: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Li Zefan <lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
Cc: Jonathan Corbet <corbet-T1hC0tSOHrs@public.gmane.org>
Cc: cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: Android Kernel Team <kernel-team-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org>
Cc: Rom Lemarchand <romlem-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org>
Cc: Colin Cross <ccross-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org>
Cc: Dmitry Shmidt <dimitrysh-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
Cc: Ricky Zhou <rickyz-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Cc: Dmitry Torokhov <dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Todd Kjos <tkjos-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
Cc: Christian Poetzsch <christian.potzsch-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
Cc: Amit Pundir <amit.pundir-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Cc: Serge E. Hallyn <serge-A9i7LUbDfNHQT0dZR+AlfA@public.gmane.org>
Cc: linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Signed-off-by: John Stultz <john.stultz-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
v2: Renamed to just CAP_CGROUP_MIGRATE as reccomended by Tejun
---
 include/uapi/linux/capability.h | 5 ++++-
 kernel/cgroup.c                 | 3 ++-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/include/uapi/linux/capability.h b/include/uapi/linux/capability.h
index 49bc062..44d7ff4 100644
--- a/include/uapi/linux/capability.h
+++ b/include/uapi/linux/capability.h
@@ -349,8 +349,11 @@ struct vfs_cap_data {
 
 #define CAP_AUDIT_READ		37
 
+/* Allow migrating tasks between cgroups */
 
-#define CAP_LAST_CAP         CAP_AUDIT_READ
+#define CAP_CGROUP_MIGRATE	38
+
+#define CAP_LAST_CAP         CAP_CGROUP_MIGRATE
 
 #define cap_valid(x) ((x) >= 0 && (x) <= CAP_LAST_CAP)
 
diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 85bc9be..09f84d2 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -2856,7 +2856,8 @@ static int cgroup_procs_write_permission(struct task_struct *task,
 	 */
 	if (!uid_eq(cred->euid, GLOBAL_ROOT_UID) &&
 	    !uid_eq(cred->euid, tcred->uid) &&
-	    !uid_eq(cred->euid, tcred->suid))
+	    !uid_eq(cred->euid, tcred->suid) &&
+	    !ns_capable(tcred->user_ns, CAP_CGROUP_MIGRATE))
 		ret = -EACCES;
 
 	if (!ret && cgroup_on_dfl(dst_cgrp)) {
-- 
2.7.4

Re: [PATCH] cgroup: Add new capability to allow a process to migrate other tasks between cgroups

[Andy Lutomirski]

On Mon, Oct 17, 2016 at 3:35 PM, John Stultz <john.stultz-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> This patch adds CAP_GROUP_MIGRATE and logic to allows a process
> to migrate other tasks between cgroups.
>
> In Android (where this feature originated), the ActivityManager tracks
> various application states (TOP_APP, FOREGROUND, BACKGROUND, SYSTEM,
> etc), and then as applications change states, the SchedPolicy logic
> will migrate the application tasks between different cgroups used
> to control the different application states (for example, there is a
> background cpuset cgroup which can limit background tasks to stay
> on one low-power cpu, and the bg_non_interactive cpuctrl cgroup can
> then further limit those background tasks to a small percentage of
> that one cpu's cpu time).
>
> However, for security reasons, Android doesn't want to make the
> system_server (the process that runs the ActivityManager and
> SchedPolicy logic), run as root. So in the Android common.git
> kernel, they have some logic to allow cgroups to loosen their
> permissions so CAP_SYS_NICE tasks can migrate other tasks between
> cgroups.
>
> The approach taken there overloads CAP_SYS_NICE a bit much, and
> is maybe more complicated then needed.
>
> So this patch, as suggested by Tejun,  simply adds a new process
> capability flag (CAP_CGROUP_MIGRATE), and uses it when checking
> if a task can migrate other tasks between cgroups.
>
> I've tested this with AOSP master (though its a bit hacked in as I
> still need to properly get the selinux bits aware of the new
> capability bit) with selinux set to permissive and it seems to be
> working well.
>
> Thoughts and feedback would be appreciated!
>
> Cc: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: Li Zefan <lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> Cc: Jonathan Corbet <corbet-T1hC0tSOHrs@public.gmane.org>
> Cc: cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Cc: Android Kernel Team <kernel-team-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org>
> Cc: Rom Lemarchand <romlem-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org>
> Cc: Colin Cross <ccross-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org>
> Cc: Dmitry Shmidt <dimitrysh-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> Cc: Ricky Zhou <rickyz-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> Cc: Dmitry Torokhov <dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Cc: Todd Kjos <tkjos-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> Cc: Christian Poetzsch <christian.potzsch-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
> Cc: Amit Pundir <amit.pundir-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Cc: Serge E. Hallyn <serge-A9i7LUbDfNHQT0dZR+AlfA@public.gmane.org>
> Cc: linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Signed-off-by: John Stultz <john.stultz-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
> v2: Renamed to just CAP_CGROUP_MIGRATE as reccomended by Tejun
> ---
>  include/uapi/linux/capability.h | 5 ++++-
>  kernel/cgroup.c                 | 3 ++-
>  2 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/include/uapi/linux/capability.h b/include/uapi/linux/capability.h
> index 49bc062..44d7ff4 100644
> --- a/include/uapi/linux/capability.h
> +++ b/include/uapi/linux/capability.h
> @@ -349,8 +349,11 @@ struct vfs_cap_data {
>
>  #define CAP_AUDIT_READ         37
>
> +/* Allow migrating tasks between cgroups */
>
> -#define CAP_LAST_CAP         CAP_AUDIT_READ
> +#define CAP_CGROUP_MIGRATE     38
> +
> +#define CAP_LAST_CAP         CAP_CGROUP_MIGRATE
>
>  #define cap_valid(x) ((x) >= 0 && (x) <= CAP_LAST_CAP)
>
> diff --git a/kernel/cgroup.c b/kernel/cgroup.c
> index 85bc9be..09f84d2 100644
> --- a/kernel/cgroup.c
> +++ b/kernel/cgroup.c
> @@ -2856,7 +2856,8 @@ static int cgroup_procs_write_permission(struct task_struct *task,
>          */
>         if (!uid_eq(cred->euid, GLOBAL_ROOT_UID) &&
>             !uid_eq(cred->euid, tcred->uid) &&
> -           !uid_eq(cred->euid, tcred->suid))
> +           !uid_eq(cred->euid, tcred->suid) &&
> +           !ns_capable(tcred->user_ns, CAP_CGROUP_MIGRATE))
>                 ret = -EACCES;

This logic seems rather confused to me.  Without this patch, a user
can write to procs if it's root *or* it matches the target uid *or* it
matches the target suid.  How does this make sense?  How about
ptrace_may_access(...) || ns_capable(tcred->user_ns,
CAP_CGROUP_MIGRATE)?

--Andy

Re: [PATCH] cgroup: Add new capability to allow a process to migrate other tasks between cgroups

[John Stultz]

On Mon, Oct 17, 2016 at 3:40 PM, Andy Lutomirski <luto@amacapital.net> wrote:
> On Mon, Oct 17, 2016 at 3:35 PM, John Stultz <john.stultz@linaro.org> wrote:
>> This patch adds CAP_GROUP_MIGRATE and logic to allows a process
>> to migrate other tasks between cgroups.
>>
>> In Android (where this feature originated), the ActivityManager tracks
>> various application states (TOP_APP, FOREGROUND, BACKGROUND, SYSTEM,
>> etc), and then as applications change states, the SchedPolicy logic
>> will migrate the application tasks between different cgroups used
>> to control the different application states (for example, there is a
>> background cpuset cgroup which can limit background tasks to stay
>> on one low-power cpu, and the bg_non_interactive cpuctrl cgroup can
>> then further limit those background tasks to a small percentage of
>> that one cpu's cpu time).
>>
>> However, for security reasons, Android doesn't want to make the
>> system_server (the process that runs the ActivityManager and
>> SchedPolicy logic), run as root. So in the Android common.git
>> kernel, they have some logic to allow cgroups to loosen their
>> permissions so CAP_SYS_NICE tasks can migrate other tasks between
>> cgroups.
>>
>> The approach taken there overloads CAP_SYS_NICE a bit much, and
>> is maybe more complicated then needed.
>>
>> So this patch, as suggested by Tejun,  simply adds a new process
>> capability flag (CAP_CGROUP_MIGRATE), and uses it when checking
>> if a task can migrate other tasks between cgroups.
>>
>> I've tested this with AOSP master (though its a bit hacked in as I
>> still need to properly get the selinux bits aware of the new
>> capability bit) with selinux set to permissive and it seems to be
>> working well.
>>
>> Thoughts and feedback would be appreciated!
>>
>> Cc: Tejun Heo <tj@kernel.org>
>> Cc: Li Zefan <lizefan@huawei.com>
>> Cc: Jonathan Corbet <corbet@lwn.net>
>> Cc: cgroups@vger.kernel.org
>> Cc: Android Kernel Team <kernel-team@android.com>
>> Cc: Rom Lemarchand <romlem@android.com>
>> Cc: Colin Cross <ccross@android.com>
>> Cc: Dmitry Shmidt <dimitrysh@google.com>
>> Cc: Ricky Zhou <rickyz@chromium.org>
>> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>> Cc: Todd Kjos <tkjos@google.com>
>> Cc: Christian Poetzsch <christian.potzsch@imgtec.com>
>> Cc: Amit Pundir <amit.pundir@linaro.org>
>> Cc: Serge E. Hallyn <serge@hallyn.com>
>> Cc: linux-api@vger.kernel.org
>> Signed-off-by: John Stultz <john.stultz@linaro.org>
>> ---
>> v2: Renamed to just CAP_CGROUP_MIGRATE as reccomended by Tejun
>> ---
>>  include/uapi/linux/capability.h | 5 ++++-
>>  kernel/cgroup.c                 | 3 ++-
>>  2 files changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/uapi/linux/capability.h b/include/uapi/linux/capability.h
>> index 49bc062..44d7ff4 100644
>> --- a/include/uapi/linux/capability.h
>> +++ b/include/uapi/linux/capability.h
>> @@ -349,8 +349,11 @@ struct vfs_cap_data {
>>
>>  #define CAP_AUDIT_READ         37
>>
>> +/* Allow migrating tasks between cgroups */
>>
>> -#define CAP_LAST_CAP         CAP_AUDIT_READ
>> +#define CAP_CGROUP_MIGRATE     38
>> +
>> +#define CAP_LAST_CAP         CAP_CGROUP_MIGRATE
>>
>>  #define cap_valid(x) ((x) >= 0 && (x) <= CAP_LAST_CAP)
>>
>> diff --git a/kernel/cgroup.c b/kernel/cgroup.c
>> index 85bc9be..09f84d2 100644
>> --- a/kernel/cgroup.c
>> +++ b/kernel/cgroup.c
>> @@ -2856,7 +2856,8 @@ static int cgroup_procs_write_permission(struct task_struct *task,
>>          */
>>         if (!uid_eq(cred->euid, GLOBAL_ROOT_UID) &&
>>             !uid_eq(cred->euid, tcred->uid) &&
>> -           !uid_eq(cred->euid, tcred->suid))
>> +           !uid_eq(cred->euid, tcred->suid) &&
>> +           !ns_capable(tcred->user_ns, CAP_CGROUP_MIGRATE))
>>                 ret = -EACCES;
>
> This logic seems rather confused to me.  Without this patch, a user
> can write to procs if it's root *or* it matches the target uid *or* it
> matches the target suid.  How does this make sense?  How about
> ptrace_may_access(...) || ns_capable(tcred->user_ns,
> CAP_CGROUP_MIGRATE)?

Though ptrace_may_access would open it also to apps with
CAP_SYS_PTRACE as well, no?

Would pulling out from __ptrace_may_access the:
 if (uid_eq(caller_uid, tcred->euid) &&
            uid_eq(caller_uid, tcred->suid) &&
            uid_eq(caller_uid, tcred->uid)  &&
            gid_eq(caller_gid, tcred->egid) &&
            gid_eq(caller_gid, tcred->sgid) &&
            gid_eq(caller_gid, tcred->gid))
                goto ok;

check and creating a new helper that could be shared between them be
the right approach?

thanks
-john

Re: [PATCH] cgroup: Add new capability to allow a process to migrate other tasks between cgroups

[Michael Kerrisk (man-pages)]

Hi John,

On 18 October 2016 at 01:35, John Stultz <john.stultz-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> On Mon, Oct 17, 2016 at 3:40 PM, Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> wrote:
>> On Mon, Oct 17, 2016 at 3:35 PM, John Stultz <john.stultz-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>>> This patch adds CAP_GROUP_MIGRATE and logic to allows a process
>>> to migrate other tasks between cgroups.
>>>
>>> In Android (where this feature originated), the ActivityManager tracks
>>> various application states (TOP_APP, FOREGROUND, BACKGROUND, SYSTEM,
>>> etc), and then as applications change states, the SchedPolicy logic
>>> will migrate the application tasks between different cgroups used
>>> to control the different application states (for example, there is a
>>> background cpuset cgroup which can limit background tasks to stay
>>> on one low-power cpu, and the bg_non_interactive cpuctrl cgroup can
>>> then further limit those background tasks to a small percentage of
>>> that one cpu's cpu time).
>>>
>>> However, for security reasons, Android doesn't want to make the
>>> system_server (the process that runs the ActivityManager and
>>> SchedPolicy logic), run as root. So in the Android common.git
>>> kernel, they have some logic to allow cgroups to loosen their
>>> permissions so CAP_SYS_NICE tasks can migrate other tasks between
>>> cgroups.
>>>
>>> The approach taken there overloads CAP_SYS_NICE a bit much, and
>>> is maybe more complicated then needed.
>>>
>>> So this patch, as suggested by Tejun,  simply adds a new process
>>> capability flag (CAP_CGROUP_MIGRATE), and uses it when checking
>>> if a task can migrate other tasks between cgroups.
>>>
>>> I've tested this with AOSP master (though its a bit hacked in as I
>>> still need to properly get the selinux bits aware of the new
>>> capability bit) with selinux set to permissive and it seems to be
>>> working well.
>>>
>>> Thoughts and feedback would be appreciated!
>>>
>>> Cc: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>>> Cc: Li Zefan <lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
>>> Cc: Jonathan Corbet <corbet-T1hC0tSOHrs@public.gmane.org>
>>> Cc: cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>>> Cc: Android Kernel Team <kernel-team-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org>
>>> Cc: Rom Lemarchand <romlem-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org>
>>> Cc: Colin Cross <ccross-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org>
>>> Cc: Dmitry Shmidt <dimitrysh-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
>>> Cc: Ricky Zhou <rickyz-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>>> Cc: Dmitry Torokhov <dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>> Cc: Todd Kjos <tkjos-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
>>> Cc: Christian Poetzsch <christian.potzsch-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
>>> Cc: Amit Pundir <amit.pundir-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>>> Cc: Serge E. Hallyn <serge-A9i7LUbDfNHQT0dZR+AlfA@public.gmane.org>
>>> Cc: linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>>> Signed-off-by: John Stultz <john.stultz-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>>> ---
>>> v2: Renamed to just CAP_CGROUP_MIGRATE as reccomended by Tejun
>>> ---
>>>  include/uapi/linux/capability.h | 5 ++++-
>>>  kernel/cgroup.c                 | 3 ++-
>>>  2 files changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/include/uapi/linux/capability.h b/include/uapi/linux/capability.h
>>> index 49bc062..44d7ff4 100644
>>> --- a/include/uapi/linux/capability.h
>>> +++ b/include/uapi/linux/capability.h
>>> @@ -349,8 +349,11 @@ struct vfs_cap_data {
>>>
>>>  #define CAP_AUDIT_READ         37
>>>
>>> +/* Allow migrating tasks between cgroups */
>>>
>>> -#define CAP_LAST_CAP         CAP_AUDIT_READ
>>> +#define CAP_CGROUP_MIGRATE     38
>>> +
>>> +#define CAP_LAST_CAP         CAP_CGROUP_MIGRATE
>>>
>>>  #define cap_valid(x) ((x) >= 0 && (x) <= CAP_LAST_CAP)
>>>
>>> diff --git a/kernel/cgroup.c b/kernel/cgroup.c
>>> index 85bc9be..09f84d2 100644
>>> --- a/kernel/cgroup.c
>>> +++ b/kernel/cgroup.c
>>> @@ -2856,7 +2856,8 @@ static int cgroup_procs_write_permission(struct task_struct *task,
>>>          */
>>>         if (!uid_eq(cred->euid, GLOBAL_ROOT_UID) &&
>>>             !uid_eq(cred->euid, tcred->uid) &&
>>> -           !uid_eq(cred->euid, tcred->suid))
>>> +           !uid_eq(cred->euid, tcred->suid) &&
>>> +           !ns_capable(tcred->user_ns, CAP_CGROUP_MIGRATE))
>>>                 ret = -EACCES;
>>
>> This logic seems rather confused to me.  Without this patch, a user
>> can write to procs if it's root *or* it matches the target uid *or* it
>> matches the target suid.  How does this make sense?  How about
>> ptrace_may_access(...) || ns_capable(tcred->user_ns,
>> CAP_CGROUP_MIGRATE)?
>
> Though ptrace_may_access would open it also to apps with
> CAP_SYS_PTRACE as well, no?
>
> Would pulling out from __ptrace_may_access the:
>  if (uid_eq(caller_uid, tcred->euid) &&
>             uid_eq(caller_uid, tcred->suid) &&
>             uid_eq(caller_uid, tcred->uid)  &&
>             gid_eq(caller_gid, tcred->egid) &&
>             gid_eq(caller_gid, tcred->sgid) &&
>             gid_eq(caller_gid, tcred->gid))
>                 goto ok;
>
> check and creating a new helper that could be shared between them be
> the right approach?

So, is creating a new capability here necessarily the right approach?
Is this operation so unique, or is there an existing silo (not
CAP_SYS_ADMIN) that we can re-use? I ask, because we currently use 38
silos out of a possible 64 capabilities, and when everyone chooses
single-use capabilities, we will quickly exhaust the silos.

I'm not saying that creating a new capability here is wrong, but it is
worth further considering the existing silos to see if there is one
that is a suitable match.

Looking at http://man7.org/linux/man-pages/man7/capabilities.7.html
throws up the following possibilities:

CAP_SYS_NICE
CAP_SYS_PTRACE
CAP_SYS_RESOURCE

I'm aware that you said above that use CAP_SYS_NICE overloads that
capability a bit too much. Maybe it's true, but on the other hand, by
my count from dome rough grepping of the kernel source, there are a
total of 14 capable() checks for CAP_SYS_NICE, out of a total of
around 1256 capable() checks altogether. So, I think this does need to
be balanced against the limited number of silos.

Also, CAP_SYS_RESOURCE deserves consideration (34 uses in capable()
checks). I'd say, since cgroups are about resources, so there's
something of a match there., so it's also worth considering.

Cheers,

Michael


-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/

Re: [PATCH] cgroup: Add new capability to allow a process to migrate other tasks between cgroups

[John Stultz]

On Tue, Oct 18, 2016 at 1:17 AM, Michael Kerrisk (man-pages)
<mtk.manpages-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> Hi John,
>
> On 18 October 2016 at 01:35, John Stultz <john.stultz-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>> On Mon, Oct 17, 2016 at 3:40 PM, Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> wrote:
>>> On Mon, Oct 17, 2016 at 3:35 PM, John Stultz <john.stultz-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>>>> This patch adds CAP_GROUP_MIGRATE and logic to allows a process
>>>> to migrate other tasks between cgroups.
>>>>
>>>> In Android (where this feature originated), the ActivityManager tracks
>>>> various application states (TOP_APP, FOREGROUND, BACKGROUND, SYSTEM,
>>>> etc), and then as applications change states, the SchedPolicy logic
>>>> will migrate the application tasks between different cgroups used
>>>> to control the different application states (for example, there is a
>>>> background cpuset cgroup which can limit background tasks to stay
>>>> on one low-power cpu, and the bg_non_interactive cpuctrl cgroup can
>>>> then further limit those background tasks to a small percentage of
>>>> that one cpu's cpu time).
>>>>
>>>> However, for security reasons, Android doesn't want to make the
>>>> system_server (the process that runs the ActivityManager and
>>>> SchedPolicy logic), run as root. So in the Android common.git
>>>> kernel, they have some logic to allow cgroups to loosen their
>>>> permissions so CAP_SYS_NICE tasks can migrate other tasks between
>>>> cgroups.
>>>>
>>>> The approach taken there overloads CAP_SYS_NICE a bit much, and
>>>> is maybe more complicated then needed.
>>>>
>>>> So this patch, as suggested by Tejun,  simply adds a new process
>>>> capability flag (CAP_CGROUP_MIGRATE), and uses it when checking
>>>> if a task can migrate other tasks between cgroups.
>>>>
>>>> I've tested this with AOSP master (though its a bit hacked in as I
>>>> still need to properly get the selinux bits aware of the new
>>>> capability bit) with selinux set to permissive and it seems to be
>>>> working well.
>>>>
>>>> Thoughts and feedback would be appreciated!
>>>>
>>>> Cc: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>>>> Cc: Li Zefan <lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
>>>> Cc: Jonathan Corbet <corbet-T1hC0tSOHrs@public.gmane.org>
>>>> Cc: cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>>>> Cc: Android Kernel Team <kernel-team-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org>
>>>> Cc: Rom Lemarchand <romlem-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org>
>>>> Cc: Colin Cross <ccross-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org>
>>>> Cc: Dmitry Shmidt <dimitrysh-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
>>>> Cc: Ricky Zhou <rickyz-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>>>> Cc: Dmitry Torokhov <dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>>> Cc: Todd Kjos <tkjos-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
>>>> Cc: Christian Poetzsch <christian.potzsch-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
>>>> Cc: Amit Pundir <amit.pundir-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>>>> Cc: Serge E. Hallyn <serge-A9i7LUbDfNHQT0dZR+AlfA@public.gmane.org>
>>>> Cc: linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>>>> Signed-off-by: John Stultz <john.stultz-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>>>> ---
>>>> v2: Renamed to just CAP_CGROUP_MIGRATE as reccomended by Tejun
>>>> ---
>>>>  include/uapi/linux/capability.h | 5 ++++-
>>>>  kernel/cgroup.c                 | 3 ++-
>>>>  2 files changed, 6 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/include/uapi/linux/capability.h b/include/uapi/linux/capability.h
>>>> index 49bc062..44d7ff4 100644
>>>> --- a/include/uapi/linux/capability.h
>>>> +++ b/include/uapi/linux/capability.h
>>>> @@ -349,8 +349,11 @@ struct vfs_cap_data {
>>>>
>>>>  #define CAP_AUDIT_READ         37
>>>>
>>>> +/* Allow migrating tasks between cgroups */
>>>>
>>>> -#define CAP_LAST_CAP         CAP_AUDIT_READ
>>>> +#define CAP_CGROUP_MIGRATE     38
>>>> +
>>>> +#define CAP_LAST_CAP         CAP_CGROUP_MIGRATE
>>>>
>>>>  #define cap_valid(x) ((x) >= 0 && (x) <= CAP_LAST_CAP)
>>>>
>>>> diff --git a/kernel/cgroup.c b/kernel/cgroup.c
>>>> index 85bc9be..09f84d2 100644
>>>> --- a/kernel/cgroup.c
>>>> +++ b/kernel/cgroup.c
>>>> @@ -2856,7 +2856,8 @@ static int cgroup_procs_write_permission(struct task_struct *task,
>>>>          */
>>>>         if (!uid_eq(cred->euid, GLOBAL_ROOT_UID) &&
>>>>             !uid_eq(cred->euid, tcred->uid) &&
>>>> -           !uid_eq(cred->euid, tcred->suid))
>>>> +           !uid_eq(cred->euid, tcred->suid) &&
>>>> +           !ns_capable(tcred->user_ns, CAP_CGROUP_MIGRATE))
>>>>                 ret = -EACCES;
>>>
>>> This logic seems rather confused to me.  Without this patch, a user
>>> can write to procs if it's root *or* it matches the target uid *or* it
>>> matches the target suid.  How does this make sense?  How about
>>> ptrace_may_access(...) || ns_capable(tcred->user_ns,
>>> CAP_CGROUP_MIGRATE)?
>>
>> Though ptrace_may_access would open it also to apps with
>> CAP_SYS_PTRACE as well, no?
>>
>> Would pulling out from __ptrace_may_access the:
>>  if (uid_eq(caller_uid, tcred->euid) &&
>>             uid_eq(caller_uid, tcred->suid) &&
>>             uid_eq(caller_uid, tcred->uid)  &&
>>             gid_eq(caller_gid, tcred->egid) &&
>>             gid_eq(caller_gid, tcred->sgid) &&
>>             gid_eq(caller_gid, tcred->gid))
>>                 goto ok;
>>
>> check and creating a new helper that could be shared between them be
>> the right approach?
>
> So, is creating a new capability here necessarily the right approach?
> Is this operation so unique, or is there an existing silo (not
> CAP_SYS_ADMIN) that we can re-use? I ask, because we currently use 38
> silos out of a possible 64 capabilities, and when everyone chooses
> single-use capabilities, we will quickly exhaust the silos.

Agreed this is a concern, and CGROUP_MIGRATE is maybe too narrow of a
specification for something so limited.

> I'm not saying that creating a new capability here is wrong, but it is
> worth further considering the existing silos to see if there is one
> that is a suitable match.
>
> Looking at http://man7.org/linux/man-pages/man7/capabilities.7.html
> throws up the following possibilities:
>
> CAP_SYS_NICE

Again, for Android uses, CAP_SYS_NICE would be fine (ideal really),
but I worry that it might be too commonly given in other systems to
allow a task to migrate potential cgroup restrictions in container
focused environments.

> CAP_SYS_PTRACE

For Android, PTRACE requires too much privilege given to the
controlling task, as that would allow the system_server to also be
able to inspect memory of all other tasks, which raises security
concerns.  (We already went through this with the
proc/<tid>/timerslack_ns interface, and had to move back to
CAP_SYS_NICE there).


> CAP_SYS_RESOURCE
>
> I'm aware that you said above that use CAP_SYS_NICE overloads that
> capability a bit too much. Maybe it's true, but on the other hand, by
> my count from dome rough grepping of the kernel source, there are a
> total of 14 capable() checks for CAP_SYS_NICE, out of a total of
> around 1256 capable() checks altogether. So, I think this does need to
> be balanced against the limited number of silos.
>
> Also, CAP_SYS_RESOURCE deserves consideration (34 uses in capable()
> checks). I'd say, since cgroups are about resources, so there's
> something of a match there., so it's also worth considering.

I'll try to look into CAP_SYS_RESOURCE.

Colin/Todd: Any objection from the Android side on CAP_SYS_RESOURCE?

(Or we could just create a new 512bit CAP2_ capabilities interface! :P)

thanks
-john

Re: [PATCH] cgroup: Add new capability to allow a process to migrate other tasks between cgroups

[Michael Kerrisk (man-pages)]

Hi John,

On 10/18/2016 06:54 PM, John Stultz wrote:
> On Tue, Oct 18, 2016 at 1:17 AM, Michael Kerrisk (man-pages)
> <mtk.manpages@gmail.com> wrote:
>> Hi John,
>>
>> On 18 October 2016 at 01:35, John Stultz <john.stultz@linaro.org> wrote:
>>> On Mon, Oct 17, 2016 at 3:40 PM, Andy Lutomirski <luto@amacapital.net> wrote:
>>>> On Mon, Oct 17, 2016 at 3:35 PM, John Stultz <john.stultz@linaro.org> wrote:
>>>>> This patch adds CAP_GROUP_MIGRATE and logic to allows a process
>>>>> to migrate other tasks between cgroups.
>>>>>
>>>>> In Android (where this feature originated), the ActivityManager tracks
>>>>> various application states (TOP_APP, FOREGROUND, BACKGROUND, SYSTEM,
>>>>> etc), and then as applications change states, the SchedPolicy logic
>>>>> will migrate the application tasks between different cgroups used
>>>>> to control the different application states (for example, there is a
>>>>> background cpuset cgroup which can limit background tasks to stay
>>>>> on one low-power cpu, and the bg_non_interactive cpuctrl cgroup can
>>>>> then further limit those background tasks to a small percentage of
>>>>> that one cpu's cpu time).
>>>>>
>>>>> However, for security reasons, Android doesn't want to make the
>>>>> system_server (the process that runs the ActivityManager and
>>>>> SchedPolicy logic), run as root. So in the Android common.git
>>>>> kernel, they have some logic to allow cgroups to loosen their
>>>>> permissions so CAP_SYS_NICE tasks can migrate other tasks between
>>>>> cgroups.
>>>>>
>>>>> The approach taken there overloads CAP_SYS_NICE a bit much, and
>>>>> is maybe more complicated then needed.
>>>>>
>>>>> So this patch, as suggested by Tejun,  simply adds a new process
>>>>> capability flag (CAP_CGROUP_MIGRATE), and uses it when checking
>>>>> if a task can migrate other tasks between cgroups.
>>>>>
>>>>> I've tested this with AOSP master (though its a bit hacked in as I
>>>>> still need to properly get the selinux bits aware of the new
>>>>> capability bit) with selinux set to permissive and it seems to be
>>>>> working well.
>>>>>
>>>>> Thoughts and feedback would be appreciated!
>>>>>
>>>>> Cc: Tejun Heo <tj@kernel.org>
>>>>> Cc: Li Zefan <lizefan@huawei.com>
>>>>> Cc: Jonathan Corbet <corbet@lwn.net>
>>>>> Cc: cgroups@vger.kernel.org
>>>>> Cc: Android Kernel Team <kernel-team@android.com>
>>>>> Cc: Rom Lemarchand <romlem@android.com>
>>>>> Cc: Colin Cross <ccross@android.com>
>>>>> Cc: Dmitry Shmidt <dimitrysh@google.com>
>>>>> Cc: Ricky Zhou <rickyz@chromium.org>
>>>>> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>>>>> Cc: Todd Kjos <tkjos@google.com>
>>>>> Cc: Christian Poetzsch <christian.potzsch@imgtec.com>
>>>>> Cc: Amit Pundir <amit.pundir@linaro.org>
>>>>> Cc: Serge E. Hallyn <serge@hallyn.com>
>>>>> Cc: linux-api@vger.kernel.org
>>>>> Signed-off-by: John Stultz <john.stultz@linaro.org>
>>>>> ---
>>>>> v2: Renamed to just CAP_CGROUP_MIGRATE as reccomended by Tejun
>>>>> ---
>>>>>  include/uapi/linux/capability.h | 5 ++++-
>>>>>  kernel/cgroup.c                 | 3 ++-
>>>>>  2 files changed, 6 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/include/uapi/linux/capability.h b/include/uapi/linux/capability.h
>>>>> index 49bc062..44d7ff4 100644
>>>>> --- a/include/uapi/linux/capability.h
>>>>> +++ b/include/uapi/linux/capability.h
>>>>> @@ -349,8 +349,11 @@ struct vfs_cap_data {
>>>>>
>>>>>  #define CAP_AUDIT_READ         37
>>>>>
>>>>> +/* Allow migrating tasks between cgroups */
>>>>>
>>>>> -#define CAP_LAST_CAP         CAP_AUDIT_READ
>>>>> +#define CAP_CGROUP_MIGRATE     38
>>>>> +
>>>>> +#define CAP_LAST_CAP         CAP_CGROUP_MIGRATE
>>>>>
>>>>>  #define cap_valid(x) ((x) >= 0 && (x) <= CAP_LAST_CAP)
>>>>>
>>>>> diff --git a/kernel/cgroup.c b/kernel/cgroup.c
>>>>> index 85bc9be..09f84d2 100644
>>>>> --- a/kernel/cgroup.c
>>>>> +++ b/kernel/cgroup.c
>>>>> @@ -2856,7 +2856,8 @@ static int cgroup_procs_write_permission(struct task_struct *task,
>>>>>          */
>>>>>         if (!uid_eq(cred->euid, GLOBAL_ROOT_UID) &&
>>>>>             !uid_eq(cred->euid, tcred->uid) &&
>>>>> -           !uid_eq(cred->euid, tcred->suid))
>>>>> +           !uid_eq(cred->euid, tcred->suid) &&
>>>>> +           !ns_capable(tcred->user_ns, CAP_CGROUP_MIGRATE))
>>>>>                 ret = -EACCES;
>>>>
>>>> This logic seems rather confused to me.  Without this patch, a user
>>>> can write to procs if it's root *or* it matches the target uid *or* it
>>>> matches the target suid.  How does this make sense?  How about
>>>> ptrace_may_access(...) || ns_capable(tcred->user_ns,
>>>> CAP_CGROUP_MIGRATE)?
>>>
>>> Though ptrace_may_access would open it also to apps with
>>> CAP_SYS_PTRACE as well, no?
>>>
>>> Would pulling out from __ptrace_may_access the:
>>>  if (uid_eq(caller_uid, tcred->euid) &&
>>>             uid_eq(caller_uid, tcred->suid) &&
>>>             uid_eq(caller_uid, tcred->uid)  &&
>>>             gid_eq(caller_gid, tcred->egid) &&
>>>             gid_eq(caller_gid, tcred->sgid) &&
>>>             gid_eq(caller_gid, tcred->gid))
>>>                 goto ok;
>>>
>>> check and creating a new helper that could be shared between them be
>>> the right approach?
>>
>> So, is creating a new capability here necessarily the right approach?
>> Is this operation so unique, or is there an existing silo (not
>> CAP_SYS_ADMIN) that we can re-use? I ask, because we currently use 38
>> silos out of a possible 64 capabilities, and when everyone chooses
>> single-use capabilities, we will quickly exhaust the silos.
> 
> Agreed this is a concern, and CGROUP_MIGRATE is maybe too narrow of a
> specification for something so limited.
> 
>> I'm not saying that creating a new capability here is wrong, but it is
>> worth further considering the existing silos to see if there is one
>> that is a suitable match.
>>
>> Looking at http://man7.org/linux/man-pages/man7/capabilities.7.html
>> throws up the following possibilities:
>>
>> CAP_SYS_NICE
> 
> Again, for Android uses, CAP_SYS_NICE would be fine (ideal really),
> but I worry that it might be too commonly given in other systems to
> allow a task to migrate potential cgroup restrictions in container
> focused environments.
> 
>> CAP_SYS_PTRACE
> 
> For Android, PTRACE requires too much privilege given to the
> controlling task, as that would allow the system_server to also be
> able to inspect memory of all other tasks, which raises security
> concerns.  (We already went through this with the
> proc/<tid>/timerslack_ns interface, and had to move back to
> CAP_SYS_NICE there).
> 
> 
>> CAP_SYS_RESOURCE
>>
>> I'm aware that you said above that use CAP_SYS_NICE overloads that
>> capability a bit too much. Maybe it's true, but on the other hand, by
>> my count from dome rough grepping of the kernel source, there are a
>> total of 14 capable() checks for CAP_SYS_NICE, out of a total of
>> around 1256 capable() checks altogether. So, I think this does need to
>> be balanced against the limited number of silos.
>>
>> Also, CAP_SYS_RESOURCE deserves consideration (34 uses in capable()
>> checks). I'd say, since cgroups are about resources, so there's
>> something of a match there., so it's also worth considering.
> 
> I'll try to look into CAP_SYS_RESOURCE.
> 
> Colin/Todd: Any objection from the Android side on CAP_SYS_RESOURCE?

Just to reiterate my perspective: I'm suggesting that one
of the existing silos be considered only. It may be that because
of the smearing issues you allude to (where the fact that a process
may have the capability for another purpose that inadvertently
allows it also to cgroup migration), that a new capability
is in order. I just want to make sure that the issue is considered
(and--importantly--that the rationale for the eventual decision is
documented in the commit message!).

Cheers,

Michael


-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/

Re: [PATCH] cgroup: Add new capability to allow a process to migrate other tasks between cgroups

[Tejun Heo]

Hello, Andy.

On Mon, Oct 17, 2016 at 03:40:37PM -0700, Andy Lutomirski wrote:
> > @@ -2856,7 +2856,8 @@ static int cgroup_procs_write_permission(struct task_struct *task,
> >          */
> >         if (!uid_eq(cred->euid, GLOBAL_ROOT_UID) &&
> >             !uid_eq(cred->euid, tcred->uid) &&
> > -           !uid_eq(cred->euid, tcred->suid))
> > +           !uid_eq(cred->euid, tcred->suid) &&
> > +           !ns_capable(tcred->user_ns, CAP_CGROUP_MIGRATE))
> >                 ret = -EACCES;
> 
> This logic seems rather confused to me.  Without this patch, a user
> can write to procs if it's root *or* it matches the target uid *or* it
> matches the target suid.  How does this make sense?  How about
> ptrace_may_access(...) || ns_capable(tcred->user_ns,
> CAP_CGROUP_MIGRATE)?

Yeah, it's weird.  The problem is that there was no delegation model
defined on v1 and it used a hybrid of file + ptracey access checks.
The goal, I think, was disallowing !root user from pulling in random
tasks into a cgroup it has write access to, which was possible because
there was no isolation on the delegation boundary.

Given how long it has been out in the wild, I don't think changing the
logic is a good idea.  We should simply replace GLOBAL_ROOT_UID test
with CAT_WHATEVER_WE_PICK test and just ignore the whole thing on v2.

Thanks.

-- 
tejun

Re: [PATCH] cgroup: Add new capability to allow a process to migrate other tasks between cgroups

[Tejun Heo]

Hello,

On Tue, Oct 18, 2016 at 09:54:37AM -0700, John Stultz wrote:
> > Also, CAP_SYS_RESOURCE deserves consideration (34 uses in capable()
> > checks). I'd say, since cgroups are about resources, so there's
> > something of a match there., so it's also worth considering.
> 
> I'll try to look into CAP_SYS_RESOURCE.
> 
> Colin/Todd: Any objection from the Android side on CAP_SYS_RESOURCE?
> 
> (Or we could just create a new 512bit CAP2_ capabilities interface! :P)

FWIW, if CAP_SYS_RESOURCE works, I'd be happy with that.

Thanks.

-- 
tejun

Re: [PATCH] cgroup: Add new capability to allow a process to migrate other tasks between cgroups

[John Stultz]

On Wed, Oct 19, 2016 at 1:52 PM, Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> Hello,
>
> On Tue, Oct 18, 2016 at 09:54:37AM -0700, John Stultz wrote:
>> > Also, CAP_SYS_RESOURCE deserves consideration (34 uses in capable()
>> > checks). I'd say, since cgroups are about resources, so there's
>> > something of a match there., so it's also worth considering.
>>
>> I'll try to look into CAP_SYS_RESOURCE.
>>
>> Colin/Todd: Any objection from the Android side on CAP_SYS_RESOURCE?
>>
>> (Or we could just create a new 512bit CAP2_ capabilities interface! :P)
>
> FWIW, if CAP_SYS_RESOURCE works, I'd be happy with that.

CAP_SYS_RESOURCE would work for Android right now (system_server
already has CAP_SYS_RESOURCE), so I'm optimistic this will be the best
approach (I've got a newer, much simpler patch queued for sending out
here).

But I'm waiting to hear back from folks on the Android side to make
sure they aren't planning on removing that CAP from system_server any
time soon.

thanks
-john

[PATCH] cgroup: Add new capability to allow a process to migrate other tasks between cgroups

[John Stultz]

This patch adds CAP_GROUP and logic to allows a process to
migrate other tasks between cgroups.

In Android (where this feature originated), the ActivityManager
tracks various application states (TOP_APP, FOREGROUND,
BACKGROUND, SYSTEM, etc), and then as applications change
states, the SchedPolicy logic will migrate the application tasks
between different cgroups used to control the different
application states (for example, there is a background cpuset
cgroup which can limit background tasks to stay on one low-power
cpu, and the bg_non_interactive cpuctrl cgroup can then further
limit those background tasks to a small percentage of that one
cpu's cpu time).

However, for security reasons, Android doesn't want to make the
system_server (the process that runs the ActivityManager and
SchedPolicy logic), run as root. So in the Android common.git
kernel, they have some logic to allow cgroups to loosen their
permissions so CAP_SYS_NICE tasks can migrate other tasks between
cgroups.

I feel the approach taken there overloads CAP_SYS_NICE a bit much
for non-android environments. Efforts to re-use CAP_SYS_RESOURCE
for this purpose (which Android has since adopted) was also
stymied by concerns about risks from future cgroups that could be
considered "dangerous" by how they might change system semantics.

So to avoid overlapping usage, this patch adds a brand new
process capability flag (CAP_CGROUP), and uses it when checking
if a task can migrate other tasks between cgroups.

I've tested this with AOSP master (though its a bit hacked in as
I still need to properly get the selinux userspace bits aware of
the new capability bit) with selinux set to permissive and it
seems to be working well.

Thoughts and feedback would be appreciated!

(Note, I'm going on holiday break after today, so I may not
respond to feedback immediately, but I figured it would be
better to give folks the chance to review this rather then sit
it for two weeks. I'll resend after the new-year, addressing any
feedback I do get.)

Cc: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Li Zefan <lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
Cc: Jonathan Corbet <corbet-T1hC0tSOHrs@public.gmane.org>
Cc: cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: Android Kernel Team <kernel-team-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org>
Cc: Rom Lemarchand <romlem-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org>
Cc: Colin Cross <ccross-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org>
Cc: Dmitry Shmidt <dimitrysh-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
Cc: Todd Kjos <tkjos-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
Cc: Christian Poetzsch <christian.potzsch-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
Cc: Amit Pundir <amit.pundir-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Cc: Dmitry Torokhov <dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Kees Cook <keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Cc: Serge E. Hallyn <serge-A9i7LUbDfNHQT0dZR+AlfA@public.gmane.org>
Cc: Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org>
Cc: linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: Paul Moore <paul-r2n+y4ga6xFZroRs9YW3xA@public.gmane.org>
Cc: Stephen Smalley <sds-+05T5uksL2qpZYMLLGbcSA@public.gmane.org>
Cc: Eric Paris <eparis-FjpueFixGhCM4zKIHC2jIg@public.gmane.org>
Cc: James Morris <james.l.morris-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
Cc: Jeff Vander Stoep <jeffv-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
Cc: linux-security-module-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: selinux-+05T5uksL2qpZYMLLGbcSA@public.gmane.org
Signed-off-by: John Stultz <john.stultz-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
v2: Renamed to just CAP_CGROUP_MIGRATE as reccomended by Tejun
v3: Switched to just using CAP_SYS_RESOURCE as suggested by Michael
v4: Send out properly folded down version of the patch. :P
v5: Switch back to CAP_CGROUP_MIGRATE due to concerns from Andy
v6: Rename to CAP_CGROUP, as it might be used for other purposes
    in the future. Also added selinux mappings for the new cap.
---
 include/uapi/linux/capability.h     | 5 ++++-
 kernel/cgroup.c                     | 3 ++-
 security/selinux/include/classmap.h | 4 ++--
 3 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/include/uapi/linux/capability.h b/include/uapi/linux/capability.h
index 49bc062..726f767 100644
--- a/include/uapi/linux/capability.h
+++ b/include/uapi/linux/capability.h
@@ -349,8 +349,11 @@ struct vfs_cap_data {
 
 #define CAP_AUDIT_READ		37
 
+/* Allow migration of other tasks between cgroups */
 
-#define CAP_LAST_CAP         CAP_AUDIT_READ
+#define CAP_CGROUP		38
+
+#define CAP_LAST_CAP         CAP_CGROUP
 
 #define cap_valid(x) ((x) >= 0 && (x) <= CAP_LAST_CAP)
 
diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 2ee9ec3..8b42ae3 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -2856,7 +2856,8 @@ static int cgroup_procs_write_permission(struct task_struct *task,
 	 */
 	if (!uid_eq(cred->euid, GLOBAL_ROOT_UID) &&
 	    !uid_eq(cred->euid, tcred->uid) &&
-	    !uid_eq(cred->euid, tcred->suid))
+	    !uid_eq(cred->euid, tcred->suid) &&
+	    !ns_capable(tcred->user_ns, CAP_CGROUP))
 		ret = -EACCES;
 
 	if (!ret && cgroup_on_dfl(dst_cgrp)) {
diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
index e2d4ad3a..ee8c1ed 100644
--- a/security/selinux/include/classmap.h
+++ b/security/selinux/include/classmap.h
@@ -22,9 +22,9 @@
 	    "audit_control", "setfcap"
 
 #define COMMON_CAP2_PERMS  "mac_override", "mac_admin", "syslog", \
-		"wake_alarm", "block_suspend", "audit_read"
+		"wake_alarm", "block_suspend", "audit_read", "cgroup"
 
-#if CAP_LAST_CAP > CAP_AUDIT_READ
+#if CAP_LAST_CAP > CAP_CGROUP
 #error New capability defined, please update COMMON_CAP2_PERMS.
 #endif
 
-- 
2.7.4

Re: [PATCH] cgroup: Add new capability to allow a process to migrate other tasks between cgroups

[Mickaël Salaün]

[-- Attachment #1.1: Type: text/plain, Size: 8499 bytes --]

Hi,

If I understand correctly, this patch is intended to add a delegation
feature to cgroup v1, which does not really make sense for the v2
because of the clean cgroup-v2 delegation design. However, this new
capability impact both versions.

As Michael said, capabilities are a limited numbers of silos and we
should try to use existing ones as much as possible but without falling
into the trap of using the same capability for everything (e.g.
CAP_SYS_ADMIN) [1]. The CAP_CGROUP looks a lot like another
CAP_SYS_ADMIN. It is not tied to any particular privilege.

Cgroups are not about resource limitation or any particular access
control, they are just about managing a set of processes, which may then
be subject to resource limitation or access control. This possible
limitations only depend on cgroup controllers or netfilter rules or BPF
programs.

To avoid the current capability issue [1], I think it should be a good
idea to reuse an existing capability (if one make sense) for each class
of constraints a controller/eBPF program/netfilter rule can enforce.
This may looks like CAP_NET_ADMIN (with network namespace handling) for
netfilter and eBPF *socket* programs but CAP_SYS_RESOURCE for the CPU,
memory and IO controllers.
As Tejun said, it will be more complicated to handle such a case, but I
don't see any other solution to keep a meaningful use of capabilities.

However, even if a cgroup does not directly involve a limitation, it may
be used to identify a group of processes for a security critical purpose
(e.g. kill a group of process). It can then make sense to have a
dedicated capability CAP_CGROUP to allow a process *without the right to
write in cgroup.procs* to be allowed to move a process out of its
current cgroup. This is similar to CAP_DAC_OVERRIDE but only for
cgroup/controllers files (but not necessarily sufficient to modify all
cgroups). This does not means that CAP_CGROUP should allow to move any
process from any cgroup. The cgroup_procs_write_permission() should
compose the checks for CAP_CGROUP and/or CAP_SYS_RESOURCE and/or
CAP_SYS_ADMIN depending on the current use of the cgroup (i.e. cgroup
controller, BPF program type, netfilter).

Regards,
 Mickaël


[1] https://forums.grsecurity.net/viewtopic.php?f=7&t=2522


On 17/12/2016 05:43, John Stultz wrote:
> This patch adds CAP_GROUP and logic to allows a process to
> migrate other tasks between cgroups.
> 
> In Android (where this feature originated), the ActivityManager
> tracks various application states (TOP_APP, FOREGROUND,
> BACKGROUND, SYSTEM, etc), and then as applications change
> states, the SchedPolicy logic will migrate the application tasks
> between different cgroups used to control the different
> application states (for example, there is a background cpuset
> cgroup which can limit background tasks to stay on one low-power
> cpu, and the bg_non_interactive cpuctrl cgroup can then further
> limit those background tasks to a small percentage of that one
> cpu's cpu time).
> 
> However, for security reasons, Android doesn't want to make the
> system_server (the process that runs the ActivityManager and
> SchedPolicy logic), run as root. So in the Android common.git
> kernel, they have some logic to allow cgroups to loosen their
> permissions so CAP_SYS_NICE tasks can migrate other tasks between
> cgroups.
> 
> I feel the approach taken there overloads CAP_SYS_NICE a bit much
> for non-android environments. Efforts to re-use CAP_SYS_RESOURCE
> for this purpose (which Android has since adopted) was also
> stymied by concerns about risks from future cgroups that could be
> considered "dangerous" by how they might change system semantics.
> 
> So to avoid overlapping usage, this patch adds a brand new
> process capability flag (CAP_CGROUP), and uses it when checking
> if a task can migrate other tasks between cgroups.
> 
> I've tested this with AOSP master (though its a bit hacked in as
> I still need to properly get the selinux userspace bits aware of
> the new capability bit) with selinux set to permissive and it
> seems to be working well.
> 
> Thoughts and feedback would be appreciated!
> 
> (Note, I'm going on holiday break after today, so I may not
> respond to feedback immediately, but I figured it would be
> better to give folks the chance to review this rather then sit
> it for two weeks. I'll resend after the new-year, addressing any
> feedback I do get.)
> 
> Cc: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: Li Zefan <lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> Cc: Jonathan Corbet <corbet-T1hC0tSOHrs@public.gmane.org>
> Cc: cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Cc: Android Kernel Team <kernel-team-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org>
> Cc: Rom Lemarchand <romlem-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org>
> Cc: Colin Cross <ccross-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org>
> Cc: Dmitry Shmidt <dimitrysh-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> Cc: Todd Kjos <tkjos-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> Cc: Christian Poetzsch <christian.potzsch-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
> Cc: Amit Pundir <amit.pundir-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Cc: Dmitry Torokhov <dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Cc: Kees Cook <keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> Cc: Serge E. Hallyn <serge-A9i7LUbDfNHQT0dZR+AlfA@public.gmane.org>
> Cc: Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org>
> Cc: linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Cc: Paul Moore <paul-r2n+y4ga6xFZroRs9YW3xA@public.gmane.org>
> Cc: Stephen Smalley <sds-+05T5uksL2qpZYMLLGbcSA@public.gmane.org>
> Cc: Eric Paris <eparis-FjpueFixGhCM4zKIHC2jIg@public.gmane.org>
> Cc: James Morris <james.l.morris-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> Cc: Jeff Vander Stoep <jeffv-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> Cc: linux-security-module-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Cc: selinux-+05T5uksL2qpZYMLLGbcSA@public.gmane.org
> Signed-off-by: John Stultz <john.stultz-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
> v2: Renamed to just CAP_CGROUP_MIGRATE as reccomended by Tejun
> v3: Switched to just using CAP_SYS_RESOURCE as suggested by Michael
> v4: Send out properly folded down version of the patch. :P
> v5: Switch back to CAP_CGROUP_MIGRATE due to concerns from Andy
> v6: Rename to CAP_CGROUP, as it might be used for other purposes
>     in the future. Also added selinux mappings for the new cap.
> ---
>  include/uapi/linux/capability.h     | 5 ++++-
>  kernel/cgroup.c                     | 3 ++-
>  security/selinux/include/classmap.h | 4 ++--
>  3 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/include/uapi/linux/capability.h b/include/uapi/linux/capability.h
> index 49bc062..726f767 100644
> --- a/include/uapi/linux/capability.h
> +++ b/include/uapi/linux/capability.h
> @@ -349,8 +349,11 @@ struct vfs_cap_data {
>  
>  #define CAP_AUDIT_READ		37
>  
> +/* Allow migration of other tasks between cgroups */
>  
> -#define CAP_LAST_CAP         CAP_AUDIT_READ
> +#define CAP_CGROUP		38
> +
> +#define CAP_LAST_CAP         CAP_CGROUP
>  
>  #define cap_valid(x) ((x) >= 0 && (x) <= CAP_LAST_CAP)
>  
> diff --git a/kernel/cgroup.c b/kernel/cgroup.c
> index 2ee9ec3..8b42ae3 100644
> --- a/kernel/cgroup.c
> +++ b/kernel/cgroup.c
> @@ -2856,7 +2856,8 @@ static int cgroup_procs_write_permission(struct task_struct *task,
>  	 */
>  	if (!uid_eq(cred->euid, GLOBAL_ROOT_UID) &&
>  	    !uid_eq(cred->euid, tcred->uid) &&
> -	    !uid_eq(cred->euid, tcred->suid))
> +	    !uid_eq(cred->euid, tcred->suid) &&
> +	    !ns_capable(tcred->user_ns, CAP_CGROUP))
>  		ret = -EACCES;
>  
>  	if (!ret && cgroup_on_dfl(dst_cgrp)) {
> diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
> index e2d4ad3a..ee8c1ed 100644
> --- a/security/selinux/include/classmap.h
> +++ b/security/selinux/include/classmap.h
> @@ -22,9 +22,9 @@
>  	    "audit_control", "setfcap"
>  
>  #define COMMON_CAP2_PERMS  "mac_override", "mac_admin", "syslog", \
> -		"wake_alarm", "block_suspend", "audit_read"
> +		"wake_alarm", "block_suspend", "audit_read", "cgroup"
>  
> -#if CAP_LAST_CAP > CAP_AUDIT_READ
> +#if CAP_LAST_CAP > CAP_CGROUP
>  #error New capability defined, please update COMMON_CAP2_PERMS.
>  #endif
>  
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] cgroup: Add new capability to allow a process to migrate other tasks between cgroups

[Tejun Heo]

Hello,

On Sat, Dec 17, 2016 at 10:06:51PM +0100, Mickaël Salaün wrote:
> If I understand correctly, this patch is intended to add a delegation
> feature to cgroup v1, which does not really make sense for the v2

It's more about upstreaming a workaround for android somewhat like
including binder into kernel.  It isn't adding actual cgroup
delegation to v1.  It's just splitting a small piece of CAP_SYS_ADMIN
to accomodate what android has been doing.

> because of the clean cgroup-v2 delegation design. However, this new
> capability impact both versions.

In the same way but it's not about cgroup delegation.  It's just
allowing splitting up CAP_SYS_ADMIN so that "no extra restrictions on
cgroup" can be given away in a safer way.

> However, even if a cgroup does not directly involve a limitation, it may
> be used to identify a group of processes for a security critical purpose
> (e.g. kill a group of process). It can then make sense to have a
> dedicated capability CAP_CGROUP to allow a process *without the right to
> write in cgroup.procs* to be allowed to move a process out of its
> current cgroup. This is similar to CAP_DAC_OVERRIDE but only for
> cgroup/controllers files (but not necessarily sufficient to modify all
> cgroups). This does not means that CAP_CGROUP should allow to move any
> process from any cgroup. The cgroup_procs_write_permission() should
> compose the checks for CAP_CGROUP and/or CAP_SYS_RESOURCE and/or
> CAP_SYS_ADMIN depending on the current use of the cgroup (i.e. cgroup
> controller, BPF program type, netfilter).

There's no reason to invent a whole new set of security policies for
cgroup.  It already got one which follows the filesystem permissions
with some extra restrictions.  The CAP split is purely to accomodate
android and that's it.  If that isn't good enough a reason, then
android should just keep carrying the patches it needs.  This doesn't
justify bolting on another permission model on cgroup in any way.

Thanks.

-- 
tejun