Message ID | 20131024191724.GB19904@ac100 |
---|---|
State | New |
Headers | show |
On 10/24/2013 12:17 PM, Serge Hallyn wrote: > [ Hi - the following patch has been applied to Tejun's > cgroup/for-3.13 branch. It's needed for non-root users to be > able to create containers ] > > It is really only wanting to duplicate a check which is already done by the > cgroup subsystem. > > With this patch, user jdoe still cannot move pid 1 into a devices cgroup > he owns, but now he can move his own other tasks into devices cgroups. > > Signed-off-by: Serge Hallyn <serge.hallyn@ubuntu.com> > Signed-off-by: Tejun Heo <tj@kernel.org> > Cc: Aristeu Rozanski <aris@redhat.com> > --- > security/device_cgroup.c | 11 ----------- > 1 file changed, 11 deletions(-) > > diff --git a/security/device_cgroup.c b/security/device_cgroup.c > index c123628..7c2a0a7 100644 > --- a/security/device_cgroup.c > +++ b/security/device_cgroup.c > @@ -63,16 +63,6 @@ static inline struct dev_cgroup *task_devcgroup(struct task_struct *task) > > struct cgroup_subsys devices_subsys; > > -static int devcgroup_can_attach(struct cgroup_subsys_state *new_css, > - struct cgroup_taskset *set) > -{ > - struct task_struct *task = cgroup_taskset_first(set); > - > - if (current != task && !capable(CAP_SYS_ADMIN)) > - return -EPERM; > - return 0; > -} > - > /* > * called under devcgroup_mutex > */ > @@ -697,7 +687,6 @@ static struct cftype dev_cgroup_files[] = { > > struct cgroup_subsys devices_subsys = { > .name = "devices", > - .can_attach = devcgroup_can_attach, > .css_alloc = devcgroup_css_alloc, > .css_free = devcgroup_css_free, > .css_online = devcgroup_online, > This is kind of a fundamental change. Can you write a test case ? rtg
Quoting Tim Gardner (tim.gardner@canonical.com): > On 10/24/2013 12:17 PM, Serge Hallyn wrote: > >[ Hi - the following patch has been applied to Tejun's > >cgroup/for-3.13 branch. It's needed for non-root users to be > >able to create containers ] > > > >It is really only wanting to duplicate a check which is already done by the > >cgroup subsystem. > > > >With this patch, user jdoe still cannot move pid 1 into a devices cgroup > >he owns, but now he can move his own other tasks into devices cgroups. > > > >Signed-off-by: Serge Hallyn <serge.hallyn@ubuntu.com> > >Signed-off-by: Tejun Heo <tj@kernel.org> > >Cc: Aristeu Rozanski <aris@redhat.com> > >--- > > security/device_cgroup.c | 11 ----------- > > 1 file changed, 11 deletions(-) > > > >diff --git a/security/device_cgroup.c b/security/device_cgroup.c > >index c123628..7c2a0a7 100644 > >--- a/security/device_cgroup.c > >+++ b/security/device_cgroup.c > >@@ -63,16 +63,6 @@ static inline struct dev_cgroup *task_devcgroup(struct task_struct *task) > > > > struct cgroup_subsys devices_subsys; > > > >-static int devcgroup_can_attach(struct cgroup_subsys_state *new_css, > >- struct cgroup_taskset *set) > >-{ > >- struct task_struct *task = cgroup_taskset_first(set); > >- > >- if (current != task && !capable(CAP_SYS_ADMIN)) > >- return -EPERM; > >- return 0; > >-} > >- > > /* > > * called under devcgroup_mutex > > */ > >@@ -697,7 +687,6 @@ static struct cftype dev_cgroup_files[] = { > > > > struct cgroup_subsys devices_subsys = { > > .name = "devices", > >- .can_attach = devcgroup_can_attach, > > .css_alloc = devcgroup_css_alloc, > > .css_free = devcgroup_css_free, > > .css_online = devcgroup_online, > > > > This is kind of a fundamental change. Can you write a test case ? This test case checks whether the patch is present (and makes sure it doesn't let non-root users move root owned tasks). #!/bin/bash die() { echo $1 exit 1 } create_chown_cgroup() { echo "Creating a cgroup to use" sudo mkdir -p /sys/fs/cgroup/devices/xxx sudo chown -R $1 /sys/fs/cgroup/devices/xxx || return 1 return 0 } test_move_init() { echo "Testing whether we can change init's cgroup as non-root (should fail)" echo 1 | /sys/fs/cgroup/devices/xxx/tasks && return 1 return 0 } test_move_child() { echo "Testing whether we can change our child's cgroup as non-root (should succeed)" sleep 100 & echo $! > /sys/fs/cgroup/devices/xxx/tasks || return 1 kill $! return 0 } myid=$(id -u) if [ $myid -eq 0 ]; then die "don't run this as root" exit 1 fi create_chown_cgroup $myid || die "failed to create test cgroup" test_move_init || die "was able to move init to new cgroup" test_move_child || die "was unable to move my child to new cgroup" echo "Success" exit 0
I would not have deeply suspicious of this patch had you mentioned it was for Trusty.
diff --git a/security/device_cgroup.c b/security/device_cgroup.c index c123628..7c2a0a7 100644 --- a/security/device_cgroup.c +++ b/security/device_cgroup.c @@ -63,16 +63,6 @@ static inline struct dev_cgroup *task_devcgroup(struct task_struct *task) struct cgroup_subsys devices_subsys; -static int devcgroup_can_attach(struct cgroup_subsys_state *new_css, - struct cgroup_taskset *set) -{ - struct task_struct *task = cgroup_taskset_first(set); - - if (current != task && !capable(CAP_SYS_ADMIN)) - return -EPERM; - return 0; -} - /* * called under devcgroup_mutex */ @@ -697,7 +687,6 @@ static struct cftype dev_cgroup_files[] = { struct cgroup_subsys devices_subsys = { .name = "devices", - .can_attach = devcgroup_can_attach, .css_alloc = devcgroup_css_alloc, .css_free = devcgroup_css_free, .css_online = devcgroup_online,