Date: Thu, 9 Mar 2017 18:28:15 +0100
From: Greg KH <greg@kroah.com>
To: Stephen Smalley <sds@tycho.nsa.gov>
Cc: Nick Kralevich <nnk@google.com>, Paul Moore <paul@paul-moore.com>,
        John Stultz <john.stultz@linaro.org>,
        Jeffrey Vander Stoep <jeffv@google.com>,
        Antonio Murdaca <amurdaca@redhat.com>,
        lkml <linux-kernel@vger.kernel.org>,
        Android Kernel Team <kernel-team@android.com>
Subject: Re: [Regression?] 1ea0ce4069 ("selinux: allow changing labels for
 cgroupfs") stops Android from booting
Message-ID: <20170309172815.GA25234@kroah.com>
References: <CALAqxLVzkW-8cFHxhO=mTiDW=5EAr3Gyzf+zczEwtdP4kPxX5g@mail.gmail.com>
 <CAHC9VhTjUuA1pdOQ4aHhLUiPHuyGDbZ19r_QUR-MyyTEgkO_nQ@mail.gmail.com>
 <1488224547.19819.32.camel@tycho.nsa.gov>
 <1488225201.19819.37.camel@tycho.nsa.gov>
 <CAFJ0LnHTmvBCaXKA1-j2ywHdASeHQzYS5bu0CHAzjSnXXK55fQ@mail.gmail.com>
 <1488230608.19819.51.camel@tycho.nsa.gov>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <1488230608.19819.51.camel@tycho.nsa.gov>
User-Agent: Mutt/1.8.0 (2017-02-23)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3106
Lines: 74

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