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

Groups > linux.kernel > #1596231 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  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

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

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] | [next] | [standalone]

#1596265

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

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] | [standalone]

Back to top | Article view | linux.kernel

csiph-web