Message-ID: <20110225033146.GA8526@suse.de>
Date:	Thu, 24 Feb 2011 19:31:46 -0800
From:	Greg KH <gregkh@...e.de>
To:	Kees Cook <kees.cook@...onical.com>
Cc:	Alan Cox <alan@...rguk.ukuu.org.uk>,
	David Daney <ddaney@...iumnetworks.com>,
	linux-kernel@...r.kernel.org, Eugene Teo <eugeneteo@...nel.sg>,
	Ralph Campbell <infinipath@...gic.com>,
	Roland Dreier <roland@...nel.org>,
	Sean Hefty <sean.hefty@...el.com>,
	Hal Rosenstock <hal.rosenstock@...il.com>,
	Jeremy Fitzhardinge <jeremy.fitzhardinge@...rix.com>,
	Konrad Rzeszutek Wilk <konrad.wilk@...cle.com>,
	Alexander Viro <viro@...iv.linux.org.uk>,
	Miklos Szeredi <miklos@...redi.hu>,
	"J. Bruce Fields" <bfields@...ldses.org>,
	Neil Brown <neilb@...e.de>, Matthew Wilcox <matthew@....cx>,
	James Morris <jmorris@...ei.org>,
	Stephen Smalley <sds@...ho.nsa.gov>,
	Eric Paris <eparis@...isplace.org>,
	Nick Piggin <npiggin@...nel.dk>, Arnd Bergmann <arnd@...db.de>,
	Ian Campbell <ian.campbell@...rix.com>,
	Jarkko Sakkinen <ext-jarkko.2.sakkinen@...ia.com>,
	Tejun Heo <tj@...nel.org>,
	Casey Schaufler <casey@...aufler-ca.com>
Subject: Re: [PATCH 2/2] debugfs: only allow root access to debugging
 interfaces

On Thu, Feb 24, 2011 at 05:12:42PM -0800, Kees Cook wrote:
> On Thu, Feb 24, 2011 at 04:35:08PM -0800, Greg KH wrote:
> > On Thu, Feb 24, 2011 at 04:22:14PM -0800, Kees Cook wrote:
> > > On Tue, Feb 22, 2011 at 12:54:13PM -0800, Kees Cook wrote:
> > > > On Tue, Feb 22, 2011 at 12:37:04PM -0800, Greg KH wrote:
> > > > > On Tue, Feb 22, 2011 at 12:28:56PM -0800, Kees Cook wrote:
> > > > > > On Tue, Feb 22, 2011 at 12:16:10PM -0800, Greg KH wrote:
> > > > > > > On Tue, Feb 22, 2011 at 11:50:18AM -0800, Kees Cook wrote:
> > > > > > > > On Tue, Feb 22, 2011 at 07:34:18PM +0000, Alan Cox wrote:
> > > > > > > > > > What system do you proposed to keep these "stupid mistakes" from
> > > > > > > > > > continuing to happen? If debugfs had already been mode 0700, we could have
> > > > > > > > > > avoided all of these CVEs, including the full-blown local root escalation.
> > > > > > > > > 
> > > > > > > > > And all sorts of features would have put themselves in sysfs instead and
> > > > > > > > > broken no doubt.
> > > > > > > > > 
> > > > > > > > > > The "no rules" approach to debugfs is not a good idea, IMO.
> > > > > > > > > 
> > > > > > > > > It's a debugging fs, it needs to be "no rules" other than the obvious
> > > > > > > > > "don't mount it on production systems"
> > > > > > > > 
> > > > > > > > Okay, so the debugfs is not supposed to be mounted on a production system.
> > > > > > > 
> > > > > > > No, not true at all, the "enterprise" distros all mount debugfs for good
> > > > > > > reason on their systems.
> > > > > > 
> > > > > > What reasons are those? Or better yet, why do you and Alan Cox disagree on
> > > > > > this point?
> > > > > 
> > > > > These distros have made the decision to support the perf interface,
> > > > > which lives in debugfs, for their customers.  I'm not saying that I
> > > > > disagree with Alan about this, just pointing out the reality of the
> > > > > situation here.
> > > > 
> > > > A tool used only by the root user, so the proposed mount mode of 0700
> > > > wouldn't break anything.
> > > 
> > > The summary is this:
> > >  - debugfs has been demonstrably dangerous to have available
> > 
> > Wait, I do not believe this statement at all.
> > 
> > It's like saying "sysfs and proc are demonstrably dangerous to have
> > available" because there were some bugs with some implementations of
> > sysfs and proc files in the past.
> 
> Since sysfs and proc have "rules", it discourages bad code more than
> debugfs does.

Oh yeah?  You should see the crap in proc :)

And I activly fight to keep the sysfs code sane, no reason why we can't
pay attention to debugfs as well, I just haven't been.

> > >  - Alan Cox says that debugfs should not be used on production systems
> > >  - Greg KH does not disagree
> > 
> > I also don't agree, as my day-job entails supporting a wide range of
> > production systems with this filesystem mounted and enabled.
> 
> I was careful in reproducing your earlier statement about not disagreeing.
> :)
> 
> > >  - however, pref needs it, and this is used by some root users
> > >  - perf will likely move out of debugfs as some point
> > > 
> > > What is the objection, then, to making the root of debugfs mode 0600? All
> > > the tools I reviewed that need it run as root (e.g. powertop). I've
> > > already written, tested, and sent the patches -- they would not break
> > > the requirements above.
> > 
> > There are a wide range of other files that can be safely read as a
> > normal user in debugfs.  For example, the usb debugging files which we
> > use to help debug hardware controller issues.  Now yes, we could ask the
> > user to become root first, but is that really necessary?
> 
> If production systems should not have debugfs mounted, and the file is
> universally useful to non-root users, it should move like the perf
> interfaces, right?

No, the usb files have nothing to do with perf.

I do strongly feel that the perf files need to move out of debugfs and
have proposed perffs and tracefs numerous times in the past, patches
included, but it's up to the maintainers and developers of that code to
make that change, not me.

And if you do that, I see no reason to mount debugfs on any "enterprise"
system, which would be a great thing.

> > Again, I feel these were just a few bugs that do not reflect the much
> > larger and benificial use of this filesystem.  We now have a set of
> > checks in place to prevent this type of error from occuring again, why
> > not rely on that instead of just removing the whole filesystem from
> > normal users entirely?
> 
> I don't feel that a test in checkpatch is sufficient to prevent future
> problems. What about Dan Carpenter's patch?

I have no objection to Dan's patch, it's in my "to-apply" queue and I
should get to it in a day or so.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists