Re: [PATCH 2/2] debugfs: Add access restriction option

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Wed, Jul 15, 2020 at 10:03:19AM +0000, Enderborg, Peter wrote:
> On 7/15/20 11:39 AM, Greg Kroah-Hartman wrote:
> > On Wed, Jul 15, 2020 at 10:42:07AM +0200, Peter Enderborg wrote:
> >> Since debugfs include sensitive information it need to be treated
> >> carefully. But it also has many very useful debug functions for userspace.
> >> With this option we can have same configuration for system with
> >> need of debugfs and a way to turn it off. This gives a extra protection
> >> for exposure on systems where user-space services with system
> >> access are attacked.
> >>
> >> It is controlled by a configurable default value that can be override
> >> with a kernel command line parameter. (debugfs=)
> >>
> >> It can be on or off, but also internally on but not seen from user-space.
> >> This no-fs mode do not register a debugfs as filesystem, but client can
> >> register their parts in the internal structures. This data can be readed
> >> with a debugger or saved with a crashkernel. When it is off clients
> >> get EPERM error when accessing the functions for registering their
> >> components.
> >>
> >> Signed-off-by: Peter Enderborg <peter.enderborg@sony.com>
> >> ---
> >>  .../admin-guide/kernel-parameters.txt         | 14 +++++++
> >>  fs/debugfs/inode.c                            | 37 +++++++++++++++++++
> >>  fs/debugfs/internal.h                         | 14 +++++++
> >>  lib/Kconfig.debug                             | 32 ++++++++++++++++
> >>  4 files changed, 97 insertions(+)
> >>
> >> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> >> index fb95fad81c79..805aa2e58491 100644
> >> --- a/Documentation/admin-guide/kernel-parameters.txt
> >> +++ b/Documentation/admin-guide/kernel-parameters.txt
> >> @@ -827,6 +827,20 @@
> >>  			useful to also enable the page_owner functionality.
> >>  			on: enable the feature
> >>  
> >> +	debugfs=    	[KNL] This parameter enables what is exposed to userspace
> >> +			and debugfs internal clients.
> >> +			Format: { on, no-fs, off }
> >> +			on: 	All functions are enabled.
> >> +			no-fs: 	Filesystem is not registered but kernel clients can
> >> +			        access APIs and a crashkernel can be used to read
> >> +				its content. There is nothing to mount.
> >> +			off: 	Filesystem is not registered and clients
> >> +			        get a -EPERM as result when trying to register files
> >> +				or directories within debugfs.
> >> +				This is equilivant of the runtime functionality if
> >> +				debugfs was not enabled in the kernel at all.
> >> +			Default value is set in build-time with a kernel configuration.
> > Naming is hard.  In looking at this, should "no-fs" be called
> > "no-mount"?  That's a better description of what it does, right?
> 
> I think no-fs cover it better since it does not register a filesystem
> but provides the interfaces. Mounting is then indirectly stopped.

But "mounting" is the common term we all know.  "no-fs" doesn't really
describe what is happening here, right?  Everything works internally
just fine, but we just are forbidding the filesystem to be mounted.

> The idea start with a check for mounting but it is much more
> definitely stopped by prevention of register of the filesystem.
> I can imagine to have a forth "mode" where it register a fs but
> not allowing mounting. Such mode maybe useful for some runtime
> configuration. But this patch is about boot time configuration.

Preventing the registering of the filesystem does cut out the ability to
mount the thing quite well :)

We could change it to just prevent the superblock from mounting if you
want, as maybe we do need the fs to remain in the list of filesystems in
the kernel at that point in time?  Otherwise we are lying to userspace.

thanks,

greg k-h