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@xxxxxxxxxxxx
> > 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.