Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1517644 > unrolled thread
| Started by | John Stultz <john.stultz@linaro.org> |
|---|---|
| First post | 2016-11-09 00:30 +0100 |
| Last post | 2016-11-09 01:20 +0100 |
| Articles | 5 — 5 participants |
Back to article view | Back to linux.kernel
[RESEND][PATCH v4] cgroup: Use CAP_SYS_RESOURCE to allow a process to migrate other tasks between cgroups John Stultz <john.stultz@linaro.org> - 2016-11-09 00:30 +0100
Re: [RESEND][PATCH v4] cgroup: Use CAP_SYS_RESOURCE to allow a process to migrate other tasks between cgroups Kees Cook <keescook@chromium.org> - 2016-11-09 00:50 +0100
Re: [RESEND][PATCH v4] cgroup: Use CAP_SYS_RESOURCE to allow a process to migrate other tasks between cgroups Andy Lutomirski <luto@kernel.org> - 2016-11-09 01:00 +0100
Re: [RESEND][PATCH v4] cgroup: Use CAP_SYS_RESOURCE to allow a process to migrate other tasks between cgroups Alexei Starovoitov <alexei.starovoitov@gmail.com> - 2016-11-09 01:10 +0100
Re: [RESEND][PATCH v4] cgroup: Use CAP_SYS_RESOURCE to allow a process to migrate other tasks between cgroups Andy Lutomirski <luto@amacapital.net> - 2016-11-09 01:20 +0100
| From | John Stultz <john.stultz@linaro.org> |
|---|---|
| Date | 2016-11-09 00:30 +0100 |
| Subject | [RESEND][PATCH v4] cgroup: Use CAP_SYS_RESOURCE to allow a process to migrate other tasks between cgroups |
| Message-ID | <sBo01-7H0-1@gated-at.bofh.it> |
This patch adds logic to allows a process to migrate other tasks
between cgroups if they have CAP_SYS_RESOURCE.
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.
So this patch, as suggested by Michael Kerrisk, simply adds a
check for CAP_SYS_RESOURCE.
I've tested this with AOSP master, and this seems to work well
as Zygote and system_server already use CAP_SYS_RESOURCE. I've
also submitted patches against the android-4.4 kernel to change
it to use CAP_SYS_RESOURCE, and the Android developers just merged
it.
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: Todd Kjos <tkjos@google.com>
Cc: Christian Poetzsch <christian.potzsch@imgtec.com>
Cc: Amit Pundir <amit.pundir@linaro.org>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Serge E. Hallyn <serge@hallyn.com>
Cc: linux-api@vger.kernel.org
Acked-by: Serge Hallyn <serge@hallyn.com>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
v2: Renamed to just CAP_CGROUP_MIGRATE as recommended 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
---
kernel/cgroup.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 85bc9be..866059a 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_SYS_RESOURCE))
ret = -EACCES;
if (!ret && cgroup_on_dfl(dst_cgrp)) {
--
2.7.4
[toc] | [next] | [standalone]
| From | Kees Cook <keescook@chromium.org> |
|---|---|
| Date | 2016-11-09 00:50 +0100 |
| Subject | Re: [RESEND][PATCH v4] cgroup: Use CAP_SYS_RESOURCE to allow a process to migrate other tasks between cgroups |
| Message-ID | <sBojo-7S6-5@gated-at.bofh.it> |
| In reply to | #1517644 |
On Tue, Nov 8, 2016 at 3:28 PM, John Stultz <john.stultz@linaro.org> wrote:
> This patch adds logic to allows a process to migrate other tasks
> between cgroups if they have CAP_SYS_RESOURCE.
>
> 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.
>
> So this patch, as suggested by Michael Kerrisk, simply adds a
> check for CAP_SYS_RESOURCE.
>
> I've tested this with AOSP master, and this seems to work well
> as Zygote and system_server already use CAP_SYS_RESOURCE. I've
> also submitted patches against the android-4.4 kernel to change
> it to use CAP_SYS_RESOURCE, and the Android developers just merged
> it.
>
> 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: Todd Kjos <tkjos@google.com>
> Cc: Christian Poetzsch <christian.potzsch@imgtec.com>
> Cc: Amit Pundir <amit.pundir@linaro.org>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Serge E. Hallyn <serge@hallyn.com>
> Cc: linux-api@vger.kernel.org
> Acked-by: Serge Hallyn <serge@hallyn.com>
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> ---
> v2: Renamed to just CAP_CGROUP_MIGRATE as recommended 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
> ---
> kernel/cgroup.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/cgroup.c b/kernel/cgroup.c
> index 85bc9be..866059a 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_SYS_RESOURCE))
> ret = -EACCES;
>
> if (!ret && cgroup_on_dfl(dst_cgrp)) {
> --
> 2.7.4
>
Reviewed-by: Kees Cook <keescook@chromium.org>
-Kees
--
Kees Cook
Nexus Security
[toc] | [prev] | [next] | [standalone]
| From | Andy Lutomirski <luto@kernel.org> |
|---|---|
| Date | 2016-11-09 01:00 +0100 |
| Subject | Re: [RESEND][PATCH v4] cgroup: Use CAP_SYS_RESOURCE to allow a process to migrate other tasks between cgroups |
| Message-ID | <sBot3-7V7-1@gated-at.bofh.it> |
| In reply to | #1517644 |
On Tue, Nov 8, 2016 at 3:28 PM, John Stultz <john.stultz@linaro.org> wrote: > This patch adds logic to allows a process to migrate other tasks > between cgroups if they have CAP_SYS_RESOURCE. > > 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. > > So this patch, as suggested by Michael Kerrisk, simply adds a > check for CAP_SYS_RESOURCE. > > I've tested this with AOSP master, and this seems to work well > as Zygote and system_server already use CAP_SYS_RESOURCE. I've > also submitted patches against the android-4.4 kernel to change > it to use CAP_SYS_RESOURCE, and the Android developers just merged > it. > I hate to say it, but I think I may see a problem. Current developments are afoot to make cgroups do more than resource control. For example, there's Landlock and there's Daniel's ingress/egress filter thing. Current cgroup controllers can mostly just DoS their controlled processes. These new controllers (or controller-like things) can exfiltrate data and change semantics. Does anyone have a security model in mind for these controllers and the cgroups that they're attached to? I'm reasonably confident that CAP_SYS_RESOURCE is not the answer...
[toc] | [prev] | [next] | [standalone]
| From | Alexei Starovoitov <alexei.starovoitov@gmail.com> |
|---|---|
| Date | 2016-11-09 01:10 +0100 |
| Subject | Re: [RESEND][PATCH v4] cgroup: Use CAP_SYS_RESOURCE to allow a process to migrate other tasks between cgroups |
| Message-ID | <sBoCJ-8eR-5@gated-at.bofh.it> |
| In reply to | #1517658 |
On Tue, Nov 08, 2016 at 03:51:40PM -0800, Andy Lutomirski wrote: > On Tue, Nov 8, 2016 at 3:28 PM, John Stultz <john.stultz@linaro.org> wrote: > > This patch adds logic to allows a process to migrate other tasks > > between cgroups if they have CAP_SYS_RESOURCE. > > > > 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. > > > > So this patch, as suggested by Michael Kerrisk, simply adds a > > check for CAP_SYS_RESOURCE. > > > > I've tested this with AOSP master, and this seems to work well > > as Zygote and system_server already use CAP_SYS_RESOURCE. I've > > also submitted patches against the android-4.4 kernel to change > > it to use CAP_SYS_RESOURCE, and the Android developers just merged > > it. > > > > I hate to say it, but I think I may see a problem. Current > developments are afoot to make cgroups do more than resource control. > For example, there's Landlock and there's Daniel's ingress/egress > filter thing. Current cgroup controllers can mostly just DoS their > controlled processes. These new controllers (or controller-like > things) can exfiltrate data and change semantics. > > Does anyone have a security model in mind for these controllers and > the cgroups that they're attached to? I'm reasonably confident that > CAP_SYS_RESOURCE is not the answer... and specifically the answer is... ? Also would be great if you start with specifying the question first and the problem you're trying to solve.
[toc] | [prev] | [next] | [standalone]
| From | Andy Lutomirski <luto@amacapital.net> |
|---|---|
| Date | 2016-11-09 01:20 +0100 |
| Subject | Re: [RESEND][PATCH v4] cgroup: Use CAP_SYS_RESOURCE to allow a process to migrate other tasks between cgroups |
| Message-ID | <sBoMp-8k7-3@gated-at.bofh.it> |
| In reply to | #1517663 |
On Tue, Nov 8, 2016 at 4:03 PM, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > On Tue, Nov 08, 2016 at 03:51:40PM -0800, Andy Lutomirski wrote: >> On Tue, Nov 8, 2016 at 3:28 PM, John Stultz <john.stultz@linaro.org> wrote: >> > This patch adds logic to allows a process to migrate other tasks >> > between cgroups if they have CAP_SYS_RESOURCE. >> > >> > 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. >> > >> > So this patch, as suggested by Michael Kerrisk, simply adds a >> > check for CAP_SYS_RESOURCE. >> > >> > I've tested this with AOSP master, and this seems to work well >> > as Zygote and system_server already use CAP_SYS_RESOURCE. I've >> > also submitted patches against the android-4.4 kernel to change >> > it to use CAP_SYS_RESOURCE, and the Android developers just merged >> > it. >> > >> >> I hate to say it, but I think I may see a problem. Current >> developments are afoot to make cgroups do more than resource control. >> For example, there's Landlock and there's Daniel's ingress/egress >> filter thing. Current cgroup controllers can mostly just DoS their >> controlled processes. These new controllers (or controller-like >> things) can exfiltrate data and change semantics. >> >> Does anyone have a security model in mind for these controllers and >> the cgroups that they're attached to? I'm reasonably confident that >> CAP_SYS_RESOURCE is not the answer... > > and specifically the answer is... ? > Also would be great if you start with specifying the question first > and the problem you're trying to solve. > I don't have a good answer right now. Here are some constraints, though: 1. An insufficiently privileged process should not be able to move a victim into a dangerous cgroup. 2. An insufficiently privileged process should not be able to move itself into a dangerous cgroup and then use execve to gain privilege such that the execve'd program can be compromised. 3. An insufficiently privileged process should not be able to make an existing cgroup dangerous in a way that could compromise a victim in that cgroup. 4. An insufficiently privileged process should not be able to make a cgroup dangerous in a way that bypasses protections that would otherwise protect execve() as used by itself or some other process in that cgroup. Keep in mind that "dangerous" may apply to a cgroup's descendents in addition to the cgroup being controlled.
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web