Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]


Groups > linux.kernel > #1587090 > unrolled thread

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

Started byJohn Stultz <john.stultz@linaro.org>
First post2017-02-23 20:00 +0100
Last post2017-02-28 02:40 +0100
Articles 16 — 5 participants

Back to article view | Back to linux.kernel


Contents

  [Regression?] 1ea0ce4069 ("selinux: allow changing labels for  cgroupfs") stops Android from booting John Stultz <john.stultz@linaro.org> - 2017-02-23 20:00 +0100
    Re: [Regression?] 1ea0ce4069 ("selinux: allow changing labels for  cgroupfs") stops Android from booting Paul Moore <paul@paul-moore.com> - 2017-02-24 01:10 +0100
      Re: [Regression?] 1ea0ce4069 ("selinux: allow changing labels for  cgroupfs") stops Android from booting John Stultz <john.stultz@linaro.org> - 2017-02-25 03:10 +0100
        Re: [Regression?] 1ea0ce4069 ("selinux: allow changing labels for  cgroupfs") stops Android from booting Nick Kralevich <nnk@google.com> - 2017-02-25 04:50 +0100
        Re: [Regression?] 1ea0ce4069 ("selinux: allow changing labels for  cgroupfs") stops Android from booting John Stultz <john.stultz@linaro.org> - 2017-02-25 05:40 +0100
      Re: [Regression?] 1ea0ce4069 ("selinux: allow changing labels for  cgroupfs") stops Android from booting Stephen Smalley <sds@tycho.nsa.gov> - 2017-02-27 20:50 +0100
        Re: [Regression?] 1ea0ce4069 ("selinux: allow changing labels for  cgroupfs") stops Android from booting Stephen Smalley <sds@tycho.nsa.gov> - 2017-02-27 22:40 +0100
        Re: [Regression?] 1ea0ce4069 ("selinux: allow changing labels for  cgroupfs") stops Android from booting Nick Kralevich <nnk@google.com> - 2017-02-28 02:00 +0100
          Re: [Regression?] 1ea0ce4069 ("selinux: allow changing labels for  cgroupfs") stops Android from booting Stephen Smalley <sds@tycho.nsa.gov> - 2017-02-28 02:10 +0100
            Re: [Regression?] 1ea0ce4069 ("selinux: allow changing labels for  cgroupfs") stops Android from booting Paul Moore <paul@paul-moore.com> - 2017-02-28 04:30 +0100
              Re: [Regression?] 1ea0ce4069 ("selinux: allow changing labels for  cgroupfs") stops Android from booting Stephen Smalley <sds@tycho.nsa.gov> - 2017-02-28 16:30 +0100
                Re: [Regression?] 1ea0ce4069 ("selinux: allow changing labels for  cgroupfs") stops Android from booting Paul Moore <paul@paul-moore.com> - 2017-02-28 18:30 +0100
            Re: [Regression?] 1ea0ce4069 ("selinux: allow changing labels for  cgroupfs") stops Android from booting Greg KH <greg@kroah.com> - 2017-03-09 18:30 +0100
              Re: [Regression?] 1ea0ce4069 ("selinux: allow changing labels for  cgroupfs") stops Android from booting Stephen Smalley <sds@tycho.nsa.gov> - 2017-03-09 19:00 +0100
                Re: [Regression?] 1ea0ce4069 ("selinux: allow changing labels for  cgroupfs") stops Android from booting Greg KH <greg@kroah.com> - 2017-03-09 19:40 +0100
        Re: [Regression?] 1ea0ce4069 ("selinux: allow changing labels for  cgroupfs") stops Android from booting Stephen Smalley <sds@tycho.nsa.gov> - 2017-02-28 02:40 +0100

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

FromJohn Stultz <john.stultz@linaro.org>
Date2017-02-23 20:00 +0100
Subject[Regression?] 1ea0ce4069 ("selinux: allow changing labels for cgroupfs") stops Android from booting
Message-ID<te6Mp-58d-19@gated-at.bofh.it>
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

[toc] | [next] | [standalone]


#1587188

FromPaul Moore <paul@paul-moore.com>
Date2017-02-24 01:10 +0100
Message-ID<tebCp-hQ-11@gated-at.bofh.it>
In reply to#1587090
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

[toc] | [prev] | [next] | [standalone]


#1588040

FromJohn Stultz <john.stultz@linaro.org>
Date2017-02-25 03:10 +0100
Message-ID<tezY5-P0-1@gated-at.bofh.it>
In reply to#1587188
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

[toc] | [prev] | [next] | [standalone]


#1588054

FromNick Kralevich <nnk@google.com>
Date2017-02-25 04:50 +0100
Message-ID<teBwR-1TQ-3@gated-at.bofh.it>
In reply to#1588040
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

[toc] | [prev] | [next] | [standalone]


#1588063

FromJohn Stultz <john.stultz@linaro.org>
Date2017-02-25 05:40 +0100
Message-ID<teCjg-2r5-5@gated-at.bofh.it>
In reply to#1588040
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

[toc] | [prev] | [next] | [standalone]


#1588893

FromStephen Smalley <sds@tycho.nsa.gov>
Date2017-02-27 20:50 +0100
Message-ID<tfzsZ-1N7-13@gated-at.bofh.it>
In reply to#1587188
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.

[toc] | [prev] | [next] | [standalone]


#1588953

FromStephen Smalley <sds@tycho.nsa.gov>
Date2017-02-27 22:40 +0100
Message-ID<tfBbr-31m-1@gated-at.bofh.it>
In reply to#1588893
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.

[toc] | [prev] | [next] | [standalone]


#1589076

FromNick Kralevich <nnk@google.com>
Date2017-02-28 02:00 +0100
Message-ID<tfBbr-31m-5@gated-at.bofh.it>
In reply to#1588893
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

[toc] | [prev] | [next] | [standalone]


#1589077

FromStephen Smalley <sds@tycho.nsa.gov>
Date2017-02-28 02:10 +0100
Message-ID<tfBbr-31m-7@gated-at.bofh.it>
In reply to#1589076
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.

[toc] | [prev] | [next] | [standalone]


#1589145

FromPaul Moore <paul@paul-moore.com>
Date2017-02-28 04:30 +0100
Message-ID<tfGE9-6YX-7@gated-at.bofh.it>
In reply to#1589077
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

[toc] | [prev] | [next] | [standalone]


#1589561

FromStephen Smalley <sds@tycho.nsa.gov>
Date2017-02-28 16:30 +0100
Message-ID<tfRSV-6gQ-1@gated-at.bofh.it>
In reply to#1589145
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.

[toc] | [prev] | [next] | [standalone]


#1589643

FromPaul Moore <paul@paul-moore.com>
Date2017-02-28 18:30 +0100
Message-ID<tfTL4-7Al-9@gated-at.bofh.it>
In reply to#1589561
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

[toc] | [prev] | [next] | [standalone]


#1596231

FromGreg KH <greg@kroah.com>
Date2017-03-09 18:30 +0100
Message-ID<tja30-8iG-11@gated-at.bofh.it>
In reply to#1589077
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

[toc] | [prev] | [next] | [standalone]


#1596265

FromStephen Smalley <sds@tycho.nsa.gov>
Date2017-03-09 19:00 +0100
Message-ID<tjaw2-8vb-11@gated-at.bofh.it>
In reply to#1596231
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").

[toc] | [prev] | [next] | [standalone]


#1596304

FromGreg KH <greg@kroah.com>
Date2017-03-09 19:40 +0100
Message-ID<tjb8J-yF-7@gated-at.bofh.it>
In reply to#1596265
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

[toc] | [prev] | [next] | [standalone]


#1589099

FromStephen Smalley <sds@tycho.nsa.gov>
Date2017-02-28 02:40 +0100
Message-ID<tfBbr-31m-3@gated-at.bofh.it>
In reply to#1588893
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).

[toc] | [prev] | [standalone]


Back to top | Article view | linux.kernel


csiph-web