[Regression?] 1ea0ce4069 ("selinux: allow changing labels for cgroupfs") stops Android from booting

[Regression?] 1ea0ce4069 ("selinux: allow changing labels for cgroupfs") stops Android from booting

[John Stultz]

Hey folks,
   I've not been able to figure out why yet, but I wanted to raise the
issue that last night I found I couldn't boot Android on my Hikey
board with Linus' HEAD kernel. It seems to cause logd to crash
repeatedly so I'm not able to get debug info from logcat.

I do see the following over and over on the console:

[   12.505838] init: computing context for service 'logd'
[   12.506355] init: starting service 'logd'...
[   12.507683] init: property_set("ro.boottime.logd", "12500792498")
failed: property already set
[   12.508701] init: Created socket '/dev/socket/logd', mode 666, user
1036, group 1036
[   12.509294] init: Created socket '/dev/socket/logdr', mode 666,
user 1036, group 1036
[   12.509891] init: Created socket '/dev/socket/logdw', mode 222,
user 1036, group 1036
[   12.510132] init: Opened file '/proc/kmsg', flags 0
[   12.510187] init: Opened file '/dev/kmsg', flags 1
[   12.510353] init: couldn't write 1941 to
/dev/cpuset/system-background/tasks: No such file or directory
[   12.533046] init: Service 'logd' (pid 1941) exited with status 255


I did some bisection and narrowed it down to 1ea0ce4069 ("selinux:
allow changing labels for cgroupfs"), which was merged in yesterday.
I've not yet been able to figure out the root cause, but reverting
that patch makes things work again.

So I wanted to raise the issue here so folks were aware.

If there is anything folks want me to test or try, please let me know.

thanks
-john

Re: [Regression?] 1ea0ce4069 ("selinux: allow changing labels for cgroupfs") stops Android from booting

[Paul Moore]

On Thu, Feb 23, 2017 at 1:43 PM, John Stultz <john.stultz@linaro.org> wrote:
> Hey folks,
>    I've not been able to figure out why yet, but I wanted to raise the
> issue that last night I found I couldn't boot Android on my Hikey
> board with Linus' HEAD kernel. It seems to cause logd to crash
> repeatedly so I'm not able to get debug info from logcat.
>
> I do see the following over and over on the console:
>
> [   12.505838] init: computing context for service 'logd'
> [   12.506355] init: starting service 'logd'...
> [   12.507683] init: property_set("ro.boottime.logd", "12500792498")
> failed: property already set
> [   12.508701] init: Created socket '/dev/socket/logd', mode 666, user
> 1036, group 1036
> [   12.509294] init: Created socket '/dev/socket/logdr', mode 666,
> user 1036, group 1036
> [   12.509891] init: Created socket '/dev/socket/logdw', mode 222,
> user 1036, group 1036
> [   12.510132] init: Opened file '/proc/kmsg', flags 0
> [   12.510187] init: Opened file '/dev/kmsg', flags 1
> [   12.510353] init: couldn't write 1941 to
> /dev/cpuset/system-background/tasks: No such file or directory
> [   12.533046] init: Service 'logd' (pid 1941) exited with status 255
>
>
> I did some bisection and narrowed it down to 1ea0ce4069 ("selinux:
> allow changing labels for cgroupfs"), which was merged in yesterday.
> I've not yet been able to figure out the root cause, but reverting
> that patch makes things work again.
>
> So I wanted to raise the issue here so folks were aware.
>
> If there is anything folks want me to test or try, please let me know.

Unfortunately I don't have an Android test system to play with, have
any of the SEAndroid folks on the To/CC line seen a similar problem?

-- 
paul moore
www.paul-moore.com

Re: [Regression?] 1ea0ce4069 ("selinux: allow changing labels for cgroupfs") stops Android from booting

[John Stultz]

On Thu, Feb 23, 2017 at 4:01 PM, Paul Moore <paul@paul-moore.com> wrote:
> On Thu, Feb 23, 2017 at 1:43 PM, John Stultz <john.stultz@linaro.org> wrote:
>> Hey folks,
>>    I've not been able to figure out why yet, but I wanted to raise the
>> issue that last night I found I couldn't boot Android on my Hikey
>> board with Linus' HEAD kernel. It seems to cause logd to crash
>> repeatedly so I'm not able to get debug info from logcat.
>>
>> I do see the following over and over on the console:
>>
>> [   12.505838] init: computing context for service 'logd'
>> [   12.506355] init: starting service 'logd'...
>> [   12.507683] init: property_set("ro.boottime.logd", "12500792498")
>> failed: property already set
>> [   12.508701] init: Created socket '/dev/socket/logd', mode 666, user
>> 1036, group 1036
>> [   12.509294] init: Created socket '/dev/socket/logdr', mode 666,
>> user 1036, group 1036
>> [   12.509891] init: Created socket '/dev/socket/logdw', mode 222,
>> user 1036, group 1036
>> [   12.510132] init: Opened file '/proc/kmsg', flags 0
>> [   12.510187] init: Opened file '/dev/kmsg', flags 1
>> [   12.510353] init: couldn't write 1941 to
>> /dev/cpuset/system-background/tasks: No such file or directory
>> [   12.533046] init: Service 'logd' (pid 1941) exited with status 255
>>
>>
>> I did some bisection and narrowed it down to 1ea0ce4069 ("selinux:
>> allow changing labels for cgroupfs"), which was merged in yesterday.
>> I've not yet been able to figure out the root cause, but reverting
>> that patch makes things work again.
>>
>> So I wanted to raise the issue here so folks were aware.
>>
>> If there is anything folks want me to test or try, please let me know.
>
> Unfortunately I don't have an Android test system to play with, have
> any of the SEAndroid folks on the To/CC line seen a similar problem?

So from my very limited knowledge here, adding the patch in question
seems to make the cgroup mount get the SBLABEL_MNT flag?
Which I'm guessing this is causing additional selinux restrictions on
processes accessing cgroup mounts, which causes some of the early
initialization processes to fail?

Should this change mean the selinux policy needs to be updated?

thanks
-john

Re: [Regression?] 1ea0ce4069 ("selinux: allow changing labels for cgroupfs") stops Android from booting

[Nick Kralevich]

Can you try adding the androidboot.selinux=permissive line to the kernel
command line, to boot in permissive mode? I suspect the policy just needs
to be adjusted.

-- Nick

On Fri, Feb 24, 2017 at 6:01 PM, John Stultz <john.stultz@linaro.org> wrote:
> On Thu, Feb 23, 2017 at 4:01 PM, Paul Moore <paul@paul-moore.com> wrote:
>> On Thu, Feb 23, 2017 at 1:43 PM, John Stultz <john.stultz@linaro.org> wrote:
>>> Hey folks,
>>>    I've not been able to figure out why yet, but I wanted to raise the
>>> issue that last night I found I couldn't boot Android on my Hikey
>>> board with Linus' HEAD kernel. It seems to cause logd to crash
>>> repeatedly so I'm not able to get debug info from logcat.
>>>
>>> I do see the following over and over on the console:
>>>
>>> [   12.505838] init: computing context for service 'logd'
>>> [   12.506355] init: starting service 'logd'...
>>> [   12.507683] init: property_set("ro.boottime.logd", "12500792498")
>>> failed: property already set
>>> [   12.508701] init: Created socket '/dev/socket/logd', mode 666, user
>>> 1036, group 1036
>>> [   12.509294] init: Created socket '/dev/socket/logdr', mode 666,
>>> user 1036, group 1036
>>> [   12.509891] init: Created socket '/dev/socket/logdw', mode 222,
>>> user 1036, group 1036
>>> [   12.510132] init: Opened file '/proc/kmsg', flags 0
>>> [   12.510187] init: Opened file '/dev/kmsg', flags 1
>>> [   12.510353] init: couldn't write 1941 to
>>> /dev/cpuset/system-background/tasks: No such file or directory
>>> [   12.533046] init: Service 'logd' (pid 1941) exited with status 255
>>>
>>>
>>> I did some bisection and narrowed it down to 1ea0ce4069 ("selinux:
>>> allow changing labels for cgroupfs"), which was merged in yesterday.
>>> I've not yet been able to figure out the root cause, but reverting
>>> that patch makes things work again.
>>>
>>> So I wanted to raise the issue here so folks were aware.
>>>
>>> If there is anything folks want me to test or try, please let me know.
>>
>> Unfortunately I don't have an Android test system to play with, have
>> any of the SEAndroid folks on the To/CC line seen a similar problem?
>
> So from my very limited knowledge here, adding the patch in question
> seems to make the cgroup mount get the SBLABEL_MNT flag?
> Which I'm guessing this is causing additional selinux restrictions on
> processes accessing cgroup mounts, which causes some of the early
> initialization processes to fail?
>
> Should this change mean the selinux policy needs to be updated?
>
> thanks
> -john



-- 
Nick Kralevich | Android Security | nnk@google.com | 650.214.4037

Re: [Regression?] 1ea0ce4069 ("selinux: allow changing labels for cgroupfs") stops Android from booting

[John Stultz]

On Fri, Feb 24, 2017 at 7:39 PM, Nick Kralevich <nnk@google.com> wrote:
> Can you try adding the androidboot.selinux=permissive line to the kernel
> command line, to boot in permissive mode? I suspect the policy just needs to
> be adjusted.

Yep. It does seem to boot fine in permissive mode, just not in enforcing.

Any clues as to what might need to be tweaked policy-wise?

I know selinux is sort of special, as its all about restricting
functionality, but this still "feels" a little bit like a regression
though, as userspace that worked before suddenly stopped working. I
don't want to throw a wrench in things and am ok if we can sort out
the policy changes, but longer term, it makes it hard to advocate for
devices to update their kernel if new kernels aren't going to just
work.

thanks
-john

Re: [Regression?] 1ea0ce4069 ("selinux: allow changing labels for cgroupfs") stops Android from booting

[Stephen Smalley]

On Thu, 2017-02-23 at 19:01 -0500, Paul Moore wrote:
> On Thu, Feb 23, 2017 at 1:43 PM, John Stultz <john.stultz@linaro.org>
> wrote:
> > 
> > Hey folks,
> >    I've not been able to figure out why yet, but I wanted to raise
> > the
> > issue that last night I found I couldn't boot Android on my Hikey
> > board with Linus' HEAD kernel. It seems to cause logd to crash
> > repeatedly so I'm not able to get debug info from logcat.
> > 
> > I do see the following over and over on the console:
> > 
> > [   12.505838] init: computing context for service 'logd'
> > [   12.506355] init: starting service 'logd'...
> > [   12.507683] init: property_set("ro.boottime.logd",
> > "12500792498")
> > failed: property already set
> > [   12.508701] init: Created socket '/dev/socket/logd', mode 666,
> > user
> > 1036, group 1036
> > [   12.509294] init: Created socket '/dev/socket/logdr', mode 666,
> > user 1036, group 1036
> > [   12.509891] init: Created socket '/dev/socket/logdw', mode 222,
> > user 1036, group 1036
> > [   12.510132] init: Opened file '/proc/kmsg', flags 0
> > [   12.510187] init: Opened file '/dev/kmsg', flags 1
> > [   12.510353] init: couldn't write 1941 to
> > /dev/cpuset/system-background/tasks: No such file or directory
> > [   12.533046] init: Service 'logd' (pid 1941) exited with status
> > 255
> > 
> > 
> > I did some bisection and narrowed it down to 1ea0ce4069 ("selinux:
> > allow changing labels for cgroupfs"), which was merged in
> > yesterday.
> > I've not yet been able to figure out the root cause, but reverting
> > that patch makes things work again.
> > 
> > So I wanted to raise the issue here so folks were aware.
> > 
> > If there is anything folks want me to test or try, please let me
> > know.
> 
> Unfortunately I don't have an Android test system to play with, have
> any of the SEAndroid folks on the To/CC line seen a similar problem?

I can reproduce it on angler (with a back-port of just that patch),
although I am unclear on the cause.  The patch is only supposed to
enable explicit setting of security labels by userspace on cgroup
files, so it isn't supposed to cause any breakage under existing
policy.  Prior to the patch, the kernel would always just return -1
with errno EOPNOTSUPP upon attempts to set security labels on cgroup
files; with the patch, the kernel may instead return -1 with errno
EACCES if not allowed.  So I suppose if userspace was explicitly
testing for EOPNOTSUPP and not failing hard in that case, it might
cause breakage.  Not sure why existing userspace would be trying to
relabel cgroup files, unless it is just a recursive restorecon that
happens to traverse into a cgroup mount (and in that case, not sure why
it would be fatal).  Other possible interaction would be use of
setfscreatecon() prior to creating a file in cgroup.

Re: [Regression?] 1ea0ce4069 ("selinux: allow changing labels for cgroupfs") stops Android from booting

[Stephen Smalley]

On Mon, 2017-02-27 at 14:42 -0500, Stephen Smalley wrote:
> On Thu, 2017-02-23 at 19:01 -0500, Paul Moore wrote:
> > 
> > On Thu, Feb 23, 2017 at 1:43 PM, John Stultz <john.stultz@linaro.or
> > g>
> > wrote:
> > > 
> > > 
> > > Hey folks,
> > >    I've not been able to figure out why yet, but I wanted to
> > > raise
> > > the
> > > issue that last night I found I couldn't boot Android on my Hikey
> > > board with Linus' HEAD kernel. It seems to cause logd to crash
> > > repeatedly so I'm not able to get debug info from logcat.
> > > 
> > > I do see the following over and over on the console:
> > > 
> > > [   12.505838] init: computing context for service 'logd'
> > > [   12.506355] init: starting service 'logd'...
> > > [   12.507683] init: property_set("ro.boottime.logd",
> > > "12500792498")
> > > failed: property already set
> > > [   12.508701] init: Created socket '/dev/socket/logd', mode 666,
> > > user
> > > 1036, group 1036
> > > [   12.509294] init: Created socket '/dev/socket/logdr', mode
> > > 666,
> > > user 1036, group 1036
> > > [   12.509891] init: Created socket '/dev/socket/logdw', mode
> > > 222,
> > > user 1036, group 1036
> > > [   12.510132] init: Opened file '/proc/kmsg', flags 0
> > > [   12.510187] init: Opened file '/dev/kmsg', flags 1
> > > [   12.510353] init: couldn't write 1941 to
> > > /dev/cpuset/system-background/tasks: No such file or directory
> > > [   12.533046] init: Service 'logd' (pid 1941) exited with status
> > > 255
> > > 
> > > 
> > > I did some bisection and narrowed it down to 1ea0ce4069
> > > ("selinux:
> > > allow changing labels for cgroupfs"), which was merged in
> > > yesterday.
> > > I've not yet been able to figure out the root cause, but
> > > reverting
> > > that patch makes things work again.
> > > 
> > > So I wanted to raise the issue here so folks were aware.
> > > 
> > > If there is anything folks want me to test or try, please let me
> > > know.
> > 
> > Unfortunately I don't have an Android test system to play with,
> > have
> > any of the SEAndroid folks on the To/CC line seen a similar
> > problem?
> 
> I can reproduce it on angler (with a back-port of just that patch),
> although I am unclear on the cause.  The patch is only supposed to
> enable explicit setting of security labels by userspace on cgroup
> files, so it isn't supposed to cause any breakage under existing
> policy.  Prior to the patch, the kernel would always just return -1
> with errno EOPNOTSUPP upon attempts to set security labels on cgroup
> files; with the patch, the kernel may instead return -1 with errno
> EACCES if not allowed.  So I suppose if userspace was explicitly
> testing for EOPNOTSUPP and not failing hard in that case, it might
> cause breakage.  Not sure why existing userspace would be trying to
> relabel cgroup files, unless it is just a recursive restorecon that
> happens to traverse into a cgroup mount (and in that case, not sure
> why
> it would be fatal).  Other possible interaction would be use of
> setfscreatecon() prior to creating a file in cgroup.

Oh, I see - it is the latter.

For example, init.rc does mkdir /dev/cpuctl/bg_non_interactive, which
internally looks up the context for that directory from file_contexts
and does a setfscreatecon() followed by a mkdir().  Previously, that
was ignored because cgroup did not support anything other than the
policy-defined label.  But now it will try to use that label, which in
turn will trigger a denial in enforcing mode and the create will fail.

So this is an incompatible change and needs to be reverted.
We'll need to wrap it up with a policy capability or something to allow
it to be enabled only if the policy correctly supports it.  Even
better, we should instead just allow the policy to specify which
filesystems should support this behavior (already on the issues list).

Re: [Regression?] 1ea0ce4069 ("selinux: allow changing labels for cgroupfs") stops Android from booting

[Nick Kralevich]

On Mon, Feb 27, 2017 at 11:53 AM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
>> I can reproduce it on angler (with a back-port of just that patch),
>> although I am unclear on the cause.  The patch is only supposed to
>> enable explicit setting of security labels by userspace on cgroup
>> files, so it isn't supposed to cause any breakage under existing
>> policy.  Prior to the patch, the kernel would always just return -1
>> with errno EOPNOTSUPP upon attempts to set security labels on cgroup
>> files; with the patch, the kernel may instead return -1 with errno
>> EACCES if not allowed.  So I suppose if userspace was explicitly
>> testing for EOPNOTSUPP and not failing hard in that case, it might
>> cause breakage.  Not sure why existing userspace would be trying to
>> relabel cgroup files, unless it is just a recursive restorecon that
>> happens to traverse into a cgroup mount (and in that case, not sure
>> why
>> it would be fatal).  Other possible interaction would be use of
>> setfscreatecon() prior to creating a file in cgroup.
>
> Oh, I see - it is the latter.
>
> For example, init.rc does mkdir /dev/cpuctl/bg_non_interactive, which
> internally looks up the context for that directory from file_contexts
> and does a setfscreatecon() followed by a mkdir().  Previously, that
> was ignored because cgroup did not support anything other than the
> policy-defined label.  But now it will try to use that label, which in
> turn will trigger a denial in enforcing mode and the create will fail.
>
> So this is an incompatible change and needs to be reverted.
> We'll need to wrap it up with a policy capability or something to allow
> it to be enabled only if the policy correctly supports it.  Even
> better, we should instead just allow the policy to specify which
> filesystems should support this behavior (already on the issues list).
>

If Android is the only system affected by this bug, I would prefer to
just fix Android to allow for this patch, rather than having
additional kernel complexity.

-- 
Nick Kralevich | Android Security | nnk@google.com | 650.214.4037

Re: [Regression?] 1ea0ce4069 ("selinux: allow changing labels for cgroupfs") stops Android from booting

[Stephen Smalley]

On Mon, 2017-02-27 at 12:48 -0800, Nick Kralevich wrote:
> On Mon, Feb 27, 2017 at 11:53 AM, Stephen Smalley <sds@tycho.nsa.gov>
> wrote:
> > 
> > > 
> > > I can reproduce it on angler (with a back-port of just that
> > > patch),
> > > although I am unclear on the cause.  The patch is only supposed
> > > to
> > > enable explicit setting of security labels by userspace on cgroup
> > > files, so it isn't supposed to cause any breakage under existing
> > > policy.  Prior to the patch, the kernel would always just return
> > > -1
> > > with errno EOPNOTSUPP upon attempts to set security labels on
> > > cgroup
> > > files; with the patch, the kernel may instead return -1 with
> > > errno
> > > EACCES if not allowed.  So I suppose if userspace was explicitly
> > > testing for EOPNOTSUPP and not failing hard in that case, it
> > > might
> > > cause breakage.  Not sure why existing userspace would be trying
> > > to
> > > relabel cgroup files, unless it is just a recursive restorecon
> > > that
> > > happens to traverse into a cgroup mount (and in that case, not
> > > sure
> > > why
> > > it would be fatal).  Other possible interaction would be use of
> > > setfscreatecon() prior to creating a file in cgroup.
> > 
> > Oh, I see - it is the latter.
> > 
> > For example, init.rc does mkdir /dev/cpuctl/bg_non_interactive,
> > which
> > internally looks up the context for that directory from
> > file_contexts
> > and does a setfscreatecon() followed by a mkdir().  Previously,
> > that
> > was ignored because cgroup did not support anything other than the
> > policy-defined label.  But now it will try to use that label, which
> > in
> > turn will trigger a denial in enforcing mode and the create will
> > fail.
> > 
> > So this is an incompatible change and needs to be reverted.
> > We'll need to wrap it up with a policy capability or something to
> > allow
> > it to be enabled only if the policy correctly supports it.  Even
> > better, we should instead just allow the policy to specify which
> > filesystems should support this behavior (already on the issues
> > list).
> > 
> 
> If Android is the only system affected by this bug, I would prefer to
> just fix Android to allow for this patch, rather than having
> additional kernel complexity.

Well, it does break userspace (even if it happens to only affect
Android, which isn't clear, e.g. possibly a distribution would likewise
suffer breakage under a tighter policy), and we already have a long-
standing open issue to replace the current set of whitelisted
filesystem types with something configuration-driven.  So I'm ok with
reverting it and requiring it to be done in a more general way.  The
latter is something we want regardless.

Re: [Regression?] 1ea0ce4069 ("selinux: allow changing labels for cgroupfs") stops Android from booting

[Stephen Smalley]

On Mon, 2017-02-27 at 16:23 -0500, Stephen Smalley wrote:
> On Mon, 2017-02-27 at 12:48 -0800, Nick Kralevich wrote:
> > 
> > On Mon, Feb 27, 2017 at 11:53 AM, Stephen Smalley <sds@tycho.nsa.go
> > v>
> > wrote:
> > > 
> > > 
> > > > 
> > > > 
> > > > I can reproduce it on angler (with a back-port of just that
> > > > patch),
> > > > although I am unclear on the cause.  The patch is only supposed
> > > > to
> > > > enable explicit setting of security labels by userspace on
> > > > cgroup
> > > > files, so it isn't supposed to cause any breakage under
> > > > existing
> > > > policy.  Prior to the patch, the kernel would always just
> > > > return
> > > > -1
> > > > with errno EOPNOTSUPP upon attempts to set security labels on
> > > > cgroup
> > > > files; with the patch, the kernel may instead return -1 with
> > > > errno
> > > > EACCES if not allowed.  So I suppose if userspace was
> > > > explicitly
> > > > testing for EOPNOTSUPP and not failing hard in that case, it
> > > > might
> > > > cause breakage.  Not sure why existing userspace would be
> > > > trying
> > > > to
> > > > relabel cgroup files, unless it is just a recursive restorecon
> > > > that
> > > > happens to traverse into a cgroup mount (and in that case, not
> > > > sure
> > > > why
> > > > it would be fatal).  Other possible interaction would be use of
> > > > setfscreatecon() prior to creating a file in cgroup.
> > > 
> > > Oh, I see - it is the latter.
> > > 
> > > For example, init.rc does mkdir /dev/cpuctl/bg_non_interactive,
> > > which
> > > internally looks up the context for that directory from
> > > file_contexts
> > > and does a setfscreatecon() followed by a mkdir().  Previously,
> > > that
> > > was ignored because cgroup did not support anything other than
> > > the
> > > policy-defined label.  But now it will try to use that label,
> > > which
> > > in
> > > turn will trigger a denial in enforcing mode and the create will
> > > fail.
> > > 
> > > So this is an incompatible change and needs to be reverted.
> > > We'll need to wrap it up with a policy capability or something to
> > > allow
> > > it to be enabled only if the policy correctly supports it.  Even
> > > better, we should instead just allow the policy to specify which
> > > filesystems should support this behavior (already on the issues
> > > list).
> > > 
> > 
> > If Android is the only system affected by this bug, I would prefer
> > to
> > just fix Android to allow for this patch, rather than having
> > additional kernel complexity.
> 
> Well, it does break userspace (even if it happens to only affect
> Android, which isn't clear, e.g. possibly a distribution would
> likewise
> suffer breakage under a tighter policy), and we already have a long-
> standing open issue to replace the current set of whitelisted
> filesystem types with something configuration-driven.  So I'm ok with
> reverting it and requiring it to be done in a more general way.  The
> latter is something we want regardless.

Also, I'm not sure it can be fixed cleanly just by policy, or at least
not just via kernel policy.  You wouldn't want to allow creation of
cgroup files in the contexts presently specified via file_contexts; you
would need to modify file_contexts to correctly specify the cgroup file
contexts.  And that in turn raises another existing issue:
distinguishing the label for the mountpoint directory versus the
mounted directory.

Re: [Regression?] 1ea0ce4069 ("selinux: allow changing labels for cgroupfs") stops Android from booting

[Paul Moore]

On Mon, Feb 27, 2017 at 4:23 PM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
> On Mon, 2017-02-27 at 12:48 -0800, Nick Kralevich wrote:
>> On Mon, Feb 27, 2017 at 11:53 AM, Stephen Smalley <sds@tycho.nsa.gov>
>> wrote:
>> >
>> > >
>> > > I can reproduce it on angler (with a back-port of just that
>> > > patch),
>> > > although I am unclear on the cause.  The patch is only supposed
>> > > to
>> > > enable explicit setting of security labels by userspace on cgroup
>> > > files, so it isn't supposed to cause any breakage under existing
>> > > policy.  Prior to the patch, the kernel would always just return
>> > > -1
>> > > with errno EOPNOTSUPP upon attempts to set security labels on
>> > > cgroup
>> > > files; with the patch, the kernel may instead return -1 with
>> > > errno
>> > > EACCES if not allowed.  So I suppose if userspace was explicitly
>> > > testing for EOPNOTSUPP and not failing hard in that case, it
>> > > might
>> > > cause breakage.  Not sure why existing userspace would be trying
>> > > to
>> > > relabel cgroup files, unless it is just a recursive restorecon
>> > > that
>> > > happens to traverse into a cgroup mount (and in that case, not
>> > > sure
>> > > why
>> > > it would be fatal).  Other possible interaction would be use of
>> > > setfscreatecon() prior to creating a file in cgroup.
>> >
>> > Oh, I see - it is the latter.
>> >
>> > For example, init.rc does mkdir /dev/cpuctl/bg_non_interactive,
>> > which
>> > internally looks up the context for that directory from
>> > file_contexts
>> > and does a setfscreatecon() followed by a mkdir().  Previously,
>> > that
>> > was ignored because cgroup did not support anything other than the
>> > policy-defined label.  But now it will try to use that label, which
>> > in
>> > turn will trigger a denial in enforcing mode and the create will
>> > fail.
>> >
>> > So this is an incompatible change and needs to be reverted.
>> > We'll need to wrap it up with a policy capability or something to
>> > allow
>> > it to be enabled only if the policy correctly supports it.  Even
>> > better, we should instead just allow the policy to specify which
>> > filesystems should support this behavior (already on the issues
>> > list).
>> >
>>
>> If Android is the only system affected by this bug, I would prefer to
>> just fix Android to allow for this patch, rather than having
>> additional kernel complexity.
>
> Well, it does break userspace (even if it happens to only affect
> Android, which isn't clear, e.g. possibly a distribution would likewise
> suffer breakage under a tighter policy), and we already have a long-
> standing open issue to replace the current set of whitelisted
> filesystem types with something configuration-driven.  So I'm ok with
> reverting it and requiring it to be done in a more general way.  The
> latter is something we want regardless.

This went up to Linus during the current merge window via the
stable-4.11 branch and I know the container guys really want this so
I'd prefer to fix this up in 4.11 with a policy capability if possible
(and I believe it should be).  I agree with Stephen that we need a
better long term solution, but I think a policy capability should work
in the short term.

Who wants to send me a patch? ;)

-- 
paul moore
www.paul-moore.com

Re: [Regression?] 1ea0ce4069 ("selinux: allow changing labels for cgroupfs") stops Android from booting

[Stephen Smalley]

On Mon, 2017-02-27 at 19:18 -0500, Paul Moore wrote:
> On Mon, Feb 27, 2017 at 4:23 PM, Stephen Smalley <sds@tycho.nsa.gov>
> wrote:
> > 
> > On Mon, 2017-02-27 at 12:48 -0800, Nick Kralevich wrote:
> > > 
> > > On Mon, Feb 27, 2017 at 11:53 AM, Stephen Smalley <sds@tycho.nsa.
> > > gov>
> > > wrote:
> > > > 
> > > > 
> > > > > 
> > > > > 
> > > > > I can reproduce it on angler (with a back-port of just that
> > > > > patch),
> > > > > although I am unclear on the cause.  The patch is only
> > > > > supposed
> > > > > to
> > > > > enable explicit setting of security labels by userspace on
> > > > > cgroup
> > > > > files, so it isn't supposed to cause any breakage under
> > > > > existing
> > > > > policy.  Prior to the patch, the kernel would always just
> > > > > return
> > > > > -1
> > > > > with errno EOPNOTSUPP upon attempts to set security labels on
> > > > > cgroup
> > > > > files; with the patch, the kernel may instead return -1 with
> > > > > errno
> > > > > EACCES if not allowed.  So I suppose if userspace was
> > > > > explicitly
> > > > > testing for EOPNOTSUPP and not failing hard in that case, it
> > > > > might
> > > > > cause breakage.  Not sure why existing userspace would be
> > > > > trying
> > > > > to
> > > > > relabel cgroup files, unless it is just a recursive
> > > > > restorecon
> > > > > that
> > > > > happens to traverse into a cgroup mount (and in that case,
> > > > > not
> > > > > sure
> > > > > why
> > > > > it would be fatal).  Other possible interaction would be use
> > > > > of
> > > > > setfscreatecon() prior to creating a file in cgroup.
> > > > 
> > > > Oh, I see - it is the latter.
> > > > 
> > > > For example, init.rc does mkdir /dev/cpuctl/bg_non_interactive,
> > > > which
> > > > internally looks up the context for that directory from
> > > > file_contexts
> > > > and does a setfscreatecon() followed by a mkdir().  Previously,
> > > > that
> > > > was ignored because cgroup did not support anything other than
> > > > the
> > > > policy-defined label.  But now it will try to use that label,
> > > > which
> > > > in
> > > > turn will trigger a denial in enforcing mode and the create
> > > > will
> > > > fail.
> > > > 
> > > > So this is an incompatible change and needs to be reverted.
> > > > We'll need to wrap it up with a policy capability or something
> > > > to
> > > > allow
> > > > it to be enabled only if the policy correctly supports
> > > > it.  Even
> > > > better, we should instead just allow the policy to specify
> > > > which
> > > > filesystems should support this behavior (already on the issues
> > > > list).
> > > > 
> > > 
> > > If Android is the only system affected by this bug, I would
> > > prefer to
> > > just fix Android to allow for this patch, rather than having
> > > additional kernel complexity.
> > 
> > Well, it does break userspace (even if it happens to only affect
> > Android, which isn't clear, e.g. possibly a distribution would
> > likewise
> > suffer breakage under a tighter policy), and we already have a
> > long-
> > standing open issue to replace the current set of whitelisted
> > filesystem types with something configuration-driven.  So I'm ok
> > with
> > reverting it and requiring it to be done in a more general
> > way.  The
> > latter is something we want regardless.
> 
> This went up to Linus during the current merge window via the
> stable-4.11 branch and I know the container guys really want this so
> I'd prefer to fix this up in 4.11 with a policy capability if
> possible
> (and I believe it should be).  I agree with Stephen that we need a
> better long term solution, but I think a policy capability should
> work
> in the short term.
> 
> Who wants to send me a patch? ;)

So, there are a couple of caveats with doing that:

1) It still requires the container folks to update their kernel,
libsepol, and policy in order to make use of the new policy capability.

2) The determination of whether a given mount should be assigned this
flag is made at mount time, so you can't simply reload policy with a
policy that defines this capability and have it automatically applied
to existing cgroup mounts.  You'd have to unmount and re-mount them
(more likely reboot).

Not saying you can't do that, just understand what is required.

Re: [Regression?] 1ea0ce4069 ("selinux: allow changing labels for cgroupfs") stops Android from booting

[Paul Moore]

On Tue, Feb 28, 2017 at 10:29 AM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
> On Mon, 2017-02-27 at 19:18 -0500, Paul Moore wrote:
>> On Mon, Feb 27, 2017 at 4:23 PM, Stephen Smalley <sds@tycho.nsa.gov>
>> wrote:
>> >
>> > On Mon, 2017-02-27 at 12:48 -0800, Nick Kralevich wrote:
>> > >
>> > > On Mon, Feb 27, 2017 at 11:53 AM, Stephen Smalley <sds@tycho.nsa.
>> > > gov>
>> > > wrote:
>> > > >
>> > > >
>> > > > >
>> > > > >
>> > > > > I can reproduce it on angler (with a back-port of just that
>> > > > > patch),
>> > > > > although I am unclear on the cause.  The patch is only
>> > > > > supposed
>> > > > > to
>> > > > > enable explicit setting of security labels by userspace on
>> > > > > cgroup
>> > > > > files, so it isn't supposed to cause any breakage under
>> > > > > existing
>> > > > > policy.  Prior to the patch, the kernel would always just
>> > > > > return
>> > > > > -1
>> > > > > with errno EOPNOTSUPP upon attempts to set security labels on
>> > > > > cgroup
>> > > > > files; with the patch, the kernel may instead return -1 with
>> > > > > errno
>> > > > > EACCES if not allowed.  So I suppose if userspace was
>> > > > > explicitly
>> > > > > testing for EOPNOTSUPP and not failing hard in that case, it
>> > > > > might
>> > > > > cause breakage.  Not sure why existing userspace would be
>> > > > > trying
>> > > > > to
>> > > > > relabel cgroup files, unless it is just a recursive
>> > > > > restorecon
>> > > > > that
>> > > > > happens to traverse into a cgroup mount (and in that case,
>> > > > > not
>> > > > > sure
>> > > > > why
>> > > > > it would be fatal).  Other possible interaction would be use
>> > > > > of
>> > > > > setfscreatecon() prior to creating a file in cgroup.
>> > > >
>> > > > Oh, I see - it is the latter.
>> > > >
>> > > > For example, init.rc does mkdir /dev/cpuctl/bg_non_interactive,
>> > > > which
>> > > > internally looks up the context for that directory from
>> > > > file_contexts
>> > > > and does a setfscreatecon() followed by a mkdir().  Previously,
>> > > > that
>> > > > was ignored because cgroup did not support anything other than
>> > > > the
>> > > > policy-defined label.  But now it will try to use that label,
>> > > > which
>> > > > in
>> > > > turn will trigger a denial in enforcing mode and the create
>> > > > will
>> > > > fail.
>> > > >
>> > > > So this is an incompatible change and needs to be reverted.
>> > > > We'll need to wrap it up with a policy capability or something
>> > > > to
>> > > > allow
>> > > > it to be enabled only if the policy correctly supports
>> > > > it.  Even
>> > > > better, we should instead just allow the policy to specify
>> > > > which
>> > > > filesystems should support this behavior (already on the issues
>> > > > list).
>> > > >
>> > >
>> > > If Android is the only system affected by this bug, I would
>> > > prefer to
>> > > just fix Android to allow for this patch, rather than having
>> > > additional kernel complexity.
>> >
>> > Well, it does break userspace (even if it happens to only affect
>> > Android, which isn't clear, e.g. possibly a distribution would
>> > likewise
>> > suffer breakage under a tighter policy), and we already have a
>> > long-
>> > standing open issue to replace the current set of whitelisted
>> > filesystem types with something configuration-driven.  So I'm ok
>> > with
>> > reverting it and requiring it to be done in a more general
>> > way.  The
>> > latter is something we want regardless.
>>
>> This went up to Linus during the current merge window via the
>> stable-4.11 branch and I know the container guys really want this so
>> I'd prefer to fix this up in 4.11 with a policy capability if
>> possible
>> (and I believe it should be).  I agree with Stephen that we need a
>> better long term solution, but I think a policy capability should
>> work
>> in the short term.
>>
>> Who wants to send me a patch? ;)
>
> So, there are a couple of caveats with doing that:
>
> 1) It still requires the container folks to update their kernel,
> libsepol, and policy in order to make use of the new policy capability.

A kernel upgrade is going to be necessary regardless at this original
change just landed in Linus' tree this merge window.  As far the
userspace and policy updates, I think that is a reasonable
expectation.  I just want to protect from the new-kernel/old-userspace
case that appears to affect Android systems at a minimum.

> 2) The determination of whether a given mount should be assigned this
> flag is made at mount time, so you can't simply reload policy with a
> policy that defines this capability and have it automatically applied
> to existing cgroup mounts.  You'd have to unmount and re-mount them
> (more likely reboot).

Yes, but once again, I don't think that is an unreasonable requirement
for new functionality.

> Not saying you can't do that, just understand what is required.

-- 
paul moore
www.paul-moore.com

Re: [Regression?] 1ea0ce4069 ("selinux: allow changing labels for cgroupfs") stops Android from booting

[Greg KH]

On Mon, Feb 27, 2017 at 04:23:28PM -0500, Stephen Smalley wrote:
> On Mon, 2017-02-27 at 12:48 -0800, Nick Kralevich wrote:
> > On Mon, Feb 27, 2017 at 11:53 AM, Stephen Smalley <sds@tycho.nsa.gov>
> > wrote:
> > > 
> > > > 
> > > > I can reproduce it on angler (with a back-port of just that
> > > > patch),
> > > > although I am unclear on the cause.  The patch is only supposed
> > > > to
> > > > enable explicit setting of security labels by userspace on cgroup
> > > > files, so it isn't supposed to cause any breakage under existing
> > > > policy.  Prior to the patch, the kernel would always just return
> > > > -1
> > > > with errno EOPNOTSUPP upon attempts to set security labels on
> > > > cgroup
> > > > files; with the patch, the kernel may instead return -1 with
> > > > errno
> > > > EACCES if not allowed.  So I suppose if userspace was explicitly
> > > > testing for EOPNOTSUPP and not failing hard in that case, it
> > > > might
> > > > cause breakage.  Not sure why existing userspace would be trying
> > > > to
> > > > relabel cgroup files, unless it is just a recursive restorecon
> > > > that
> > > > happens to traverse into a cgroup mount (and in that case, not
> > > > sure
> > > > why
> > > > it would be fatal).  Other possible interaction would be use of
> > > > setfscreatecon() prior to creating a file in cgroup.
> > > 
> > > Oh, I see - it is the latter.
> > > 
> > > For example, init.rc does mkdir /dev/cpuctl/bg_non_interactive,
> > > which
> > > internally looks up the context for that directory from
> > > file_contexts
> > > and does a setfscreatecon() followed by a mkdir().  Previously,
> > > that
> > > was ignored because cgroup did not support anything other than the
> > > policy-defined label.  But now it will try to use that label, which
> > > in
> > > turn will trigger a denial in enforcing mode and the create will
> > > fail.
> > > 
> > > So this is an incompatible change and needs to be reverted.
> > > We'll need to wrap it up with a policy capability or something to
> > > allow
> > > it to be enabled only if the policy correctly supports it.  Even
> > > better, we should instead just allow the policy to specify which
> > > filesystems should support this behavior (already on the issues
> > > list).
> > > 
> > 
> > If Android is the only system affected by this bug, I would prefer to
> > just fix Android to allow for this patch, rather than having
> > additional kernel complexity.
> 
> Well, it does break userspace (even if it happens to only affect
> Android, which isn't clear, e.g. possibly a distribution would likewise
> suffer breakage under a tighter policy), and we already have a long-
> standing open issue to replace the current set of whitelisted
> filesystem types with something configuration-driven.  So I'm ok with
> reverting it and requiring it to be done in a more general way.  The
> latter is something we want regardless.
> 

Please revert this, it's not ok to break working userspace code.  I've
gotten a few off-line queries as to why this ended up being merged when
it was known to break Android.

thanks,

greg k-h

Re: [Regression?] 1ea0ce4069 ("selinux: allow changing labels for cgroupfs") stops Android from booting

[Stephen Smalley]

On Thu, 2017-03-09 at 18:28 +0100, Greg KH wrote:
> On Mon, Feb 27, 2017 at 04:23:28PM -0500, Stephen Smalley wrote:
> > 
> > On Mon, 2017-02-27 at 12:48 -0800, Nick Kralevich wrote:
> > > 
> > > On Mon, Feb 27, 2017 at 11:53 AM, Stephen Smalley <sds@tycho.nsa.
> > > gov>
> > > wrote:
> > > > 
> > > > 
> > > > > 
> > > > > 
> > > > > I can reproduce it on angler (with a back-port of just that
> > > > > patch),
> > > > > although I am unclear on the cause.  The patch is only
> > > > > supposed
> > > > > to
> > > > > enable explicit setting of security labels by userspace on
> > > > > cgroup
> > > > > files, so it isn't supposed to cause any breakage under
> > > > > existing
> > > > > policy.  Prior to the patch, the kernel would always just
> > > > > return
> > > > > -1
> > > > > with errno EOPNOTSUPP upon attempts to set security labels on
> > > > > cgroup
> > > > > files; with the patch, the kernel may instead return -1 with
> > > > > errno
> > > > > EACCES if not allowed.  So I suppose if userspace was
> > > > > explicitly
> > > > > testing for EOPNOTSUPP and not failing hard in that case, it
> > > > > might
> > > > > cause breakage.  Not sure why existing userspace would be
> > > > > trying
> > > > > to
> > > > > relabel cgroup files, unless it is just a recursive
> > > > > restorecon
> > > > > that
> > > > > happens to traverse into a cgroup mount (and in that case,
> > > > > not
> > > > > sure
> > > > > why
> > > > > it would be fatal).  Other possible interaction would be use
> > > > > of
> > > > > setfscreatecon() prior to creating a file in cgroup.
> > > > 
> > > > Oh, I see - it is the latter.
> > > > 
> > > > For example, init.rc does mkdir /dev/cpuctl/bg_non_interactive,
> > > > which
> > > > internally looks up the context for that directory from
> > > > file_contexts
> > > > and does a setfscreatecon() followed by a mkdir().  Previously,
> > > > that
> > > > was ignored because cgroup did not support anything other than
> > > > the
> > > > policy-defined label.  But now it will try to use that label,
> > > > which
> > > > in
> > > > turn will trigger a denial in enforcing mode and the create
> > > > will
> > > > fail.
> > > > 
> > > > So this is an incompatible change and needs to be reverted.
> > > > We'll need to wrap it up with a policy capability or something
> > > > to
> > > > allow
> > > > it to be enabled only if the policy correctly supports
> > > > it.  Even
> > > > better, we should instead just allow the policy to specify
> > > > which
> > > > filesystems should support this behavior (already on the issues
> > > > list).
> > > > 
> > > 
> > > If Android is the only system affected by this bug, I would
> > > prefer to
> > > just fix Android to allow for this patch, rather than having
> > > additional kernel complexity.
> > 
> > Well, it does break userspace (even if it happens to only affect
> > Android, which isn't clear, e.g. possibly a distribution would
> > likewise
> > suffer breakage under a tighter policy), and we already have a
> > long-
> > standing open issue to replace the current set of whitelisted
> > filesystem types with something configuration-driven.  So I'm ok
> > with
> > reverting it and requiring it to be done in a more general way.
> >  The
> > latter is something we want regardless.
> > 
> 
> Please revert this, it's not ok to break working userspace
> code.  I've
> gotten a few off-line queries as to why this ended up being merged
> when
> it was known to break Android.

It should be fixed by commit 2651225b5ebcdde60f684c4db8ec7e9e3800a74f
("selinux: wrap cgroup seclabel support with its own policy
capability").

Re: [Regression?] 1ea0ce4069 ("selinux: allow changing labels for cgroupfs") stops Android from booting

[Greg KH]

On Thu, Mar 09, 2017 at 12:57:14PM -0500, Stephen Smalley wrote:
> On Thu, 2017-03-09 at 18:28 +0100, Greg KH wrote:
> > On Mon, Feb 27, 2017 at 04:23:28PM -0500, Stephen Smalley wrote:
> > > 
> > > On Mon, 2017-02-27 at 12:48 -0800, Nick Kralevich wrote:
> > > > 
> > > > On Mon, Feb 27, 2017 at 11:53 AM, Stephen Smalley <sds@tycho.nsa.
> > > > gov>
> > > > wrote:
> > > > > 
> > > > > 
> > > > > > 
> > > > > > 
> > > > > > I can reproduce it on angler (with a back-port of just that
> > > > > > patch),
> > > > > > although I am unclear on the cause.  The patch is only
> > > > > > supposed
> > > > > > to
> > > > > > enable explicit setting of security labels by userspace on
> > > > > > cgroup
> > > > > > files, so it isn't supposed to cause any breakage under
> > > > > > existing
> > > > > > policy.  Prior to the patch, the kernel would always just
> > > > > > return
> > > > > > -1
> > > > > > with errno EOPNOTSUPP upon attempts to set security labels on
> > > > > > cgroup
> > > > > > files; with the patch, the kernel may instead return -1 with
> > > > > > errno
> > > > > > EACCES if not allowed.  So I suppose if userspace was
> > > > > > explicitly
> > > > > > testing for EOPNOTSUPP and not failing hard in that case, it
> > > > > > might
> > > > > > cause breakage.  Not sure why existing userspace would be
> > > > > > trying
> > > > > > to
> > > > > > relabel cgroup files, unless it is just a recursive
> > > > > > restorecon
> > > > > > that
> > > > > > happens to traverse into a cgroup mount (and in that case,
> > > > > > not
> > > > > > sure
> > > > > > why
> > > > > > it would be fatal).  Other possible interaction would be use
> > > > > > of
> > > > > > setfscreatecon() prior to creating a file in cgroup.
> > > > > 
> > > > > Oh, I see - it is the latter.
> > > > > 
> > > > > For example, init.rc does mkdir /dev/cpuctl/bg_non_interactive,
> > > > > which
> > > > > internally looks up the context for that directory from
> > > > > file_contexts
> > > > > and does a setfscreatecon() followed by a mkdir().  Previously,
> > > > > that
> > > > > was ignored because cgroup did not support anything other than
> > > > > the
> > > > > policy-defined label.  But now it will try to use that label,
> > > > > which
> > > > > in
> > > > > turn will trigger a denial in enforcing mode and the create
> > > > > will
> > > > > fail.
> > > > > 
> > > > > So this is an incompatible change and needs to be reverted.
> > > > > We'll need to wrap it up with a policy capability or something
> > > > > to
> > > > > allow
> > > > > it to be enabled only if the policy correctly supports
> > > > > it.  Even
> > > > > better, we should instead just allow the policy to specify
> > > > > which
> > > > > filesystems should support this behavior (already on the issues
> > > > > list).
> > > > > 
> > > > 
> > > > If Android is the only system affected by this bug, I would
> > > > prefer to
> > > > just fix Android to allow for this patch, rather than having
> > > > additional kernel complexity.
> > > 
> > > Well, it does break userspace (even if it happens to only affect
> > > Android, which isn't clear, e.g. possibly a distribution would
> > > likewise
> > > suffer breakage under a tighter policy), and we already have a
> > > long-
> > > standing open issue to replace the current set of whitelisted
> > > filesystem types with something configuration-driven.  So I'm ok
> > > with
> > > reverting it and requiring it to be done in a more general way.
> > >  The
> > > latter is something we want regardless.
> > > 
> > 
> > Please revert this, it's not ok to break working userspace
> > code.  I've
> > gotten a few off-line queries as to why this ended up being merged
> > when
> > it was known to break Android.
> 
> It should be fixed by commit 2651225b5ebcdde60f684c4db8ec7e9e3800a74f
> ("selinux: wrap cgroup seclabel support with its own policy
> capability").

Ah, so sorry about this, missed this commit.  Nevermind :)

greg k-h