Re: [PATCH 24/24] debugfs: Restrict debugfs when the kernel is locked down

From: David Howells <dhowells@redhat.com>
To: Pavel Machek <pavel@ucw.cz>
Cc: dhowells@redhat.com, torvalds@linux-foundation.org,
	linux-man@vger.kernel.org, linux-api@vger.kernel.org,
	jmorris@namei.org, linux-kernel@vger.kernel.org,
	linux-security-module@vger.kernel.org
Subject: Re: [PATCH 24/24] debugfs: Restrict debugfs when the kernel is locked down
Date: Thu, 19 Apr 2018 15:35:47 +0100	[thread overview]
Message-ID: <27818.1524148547@warthog.procyon.org.uk> (raw)
In-Reply-To: <20180413202241.GB4484@amd>

Pavel Machek <pavel@ucw.cz> wrote:

> >  (1) chmod and chown are disallowed on debugfs objects (though the root dir
> >      can be modified by mount and remount, but I'm not worried about that).
> 
> This has nothing to do with the lockdown goals, right? I find chown of
> such files quite nice, to allow debugging without doing sudo all the time.

It allows someone to give everyone access to files that should perhaps only be
accessible by root.  Besides, if you disable lockdown then you can do this if
you want.

> >  (2) When the kernel is locked down, only files with the following criteria
> >      are permitted to be opened:
> > 
> > 	- The file must have mode 00444
> > 	- The file must not have ioctl methods
> > 	- The file must not have mmap
> 
> Dunno. Would not it be nicer to go through the debugfs files and split
> them into safe/unsafe varieties?

Whilst that is a laudable idea, there are at least a couple of thousand files
to analyse, some of them doing weird stuff in drivers that aren't easy to
understand, and some of them with lists of files, some of which might be safe
and some of which aren't safe.  Even some reads that look like they ought to
be safe have side effects (such as read-and-reset counters).

Besides, define 'safe'.  Is reading a particular reg on a device and returning
the value through a debugfs read safe, for example?  What about files where
reading is harmless, but writing needs to be disallowed?

I did try initially passing in a flag to say whether something was safe or
not, abusing an inode flag to store it since debugfs uses a plain inode
struct, but then I realised just how many ways there are to create debugfs
files and it started to get out of hand.  My initial idea also included
locking everything down by default and marking things unlocked on an as-needed
basis.

If you have the time to audit all these files, then that would be great.

I lean more towards the lock debugfs down entirely side.

David