On Mon, Jun 22, 2020 at 10:30:19AM +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 <[email protected]>
> ---
> .../admin-guide/kernel-parameters.txt | 12 ++++++
> fs/debugfs/inode.c | 37 +++++++++++++++++++
> fs/debugfs/internal.h | 14 +++++++
> lib/Kconfig.debug | 32 ++++++++++++++++
> 4 files changed, 95 insertions(+)
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt
> b/Documentation/admin-guide/kernel-parameters.txt
> index fb95fad81c79..236aacaceaf5 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -827,6 +827,18 @@
> 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.
Can you add "This is equilivant of the runtime functionality if debugfs
was not enabled in the kernel at all." to the "off" option?
> + Default value is set in build-time with a kernel
> configuration.
> +
> debugpat [X86] Enable PAT debugging
>
> decnet.addr= [HW,NET]
> diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
> index b7f2e971ecbc..a4a1c92ae478 100644
> --- a/fs/debugfs/inode.c
> +++ b/fs/debugfs/inode.c
> @@ -35,6 +35,7 @@
> static struct vfsmount *debugfs_mount;
> static int debugfs_mount_count;
> static bool debugfs_registered;
> +static unsigned int debugfs_allow = DEFAULT_DEBUGFS_ACCESS_BITS;
>
> /*
> * Don't allow access attributes to be changed whilst the kernel is locked
> down
> @@ -266,6 +267,9 @@ static struct dentry *debug_mount(struct file_system_type
> *fs_type,
> int flags, const char *dev_name,
> void *data)
> {
> + if (!(debugfs_allow & DEBUGFS_ALLOW_API_BIT))
> + return ERR_CAST(ERR_PTR(-EPERM));
Shouldn't ERR_PTR() be all that is needed here? I don't see any
ERR_CAST() usages in this format in the kernel today.
> +
> return mount_single(fs_type, flags, data, debug_fill_super);
> }
>
> @@ -311,6 +315,9 @@ static struct dentry *start_creating(const char *name,
> struct dentry *parent)
> struct dentry *dentry;
> int error;
>
> + if (!(debugfs_allow & DEBUGFS_ALLOW_API_BIT))
> + return ERR_PTR(-EPERM);
See, you don't use it here :)
> +
> pr_debug("creating file '%s'\n", name);
>
> if (IS_ERR(parent))
> @@ -385,6 +392,11 @@ static struct dentry *__debugfs_create_file(const char
> *name, umode_t mode,
> if (IS_ERR(dentry))
> return dentry;
>
> + if (!(debugfs_allow & DEBUGFS_ALLOW_API_BIT)) {
> + failed_creating(dentry);
> + return ERR_PTR(-EPERM);
> + }
> +
> inode = debugfs_get_inode(dentry->d_sb);
> if (unlikely(!inode)) {
> pr_err("out of free dentries, can not create file '%s'\n",
> @@ -541,6 +553,11 @@ struct dentry *debugfs_create_dir(const char *name,
> struct dentry *parent)
> if (IS_ERR(dentry))
> return dentry;
>
> + if (!(debugfs_allow & DEBUGFS_ALLOW_API_BIT)) {
> + failed_creating(dentry);
> + return ERR_PTR(-EPERM);
> + }
> +
> inode = debugfs_get_inode(dentry->d_sb);
> if (unlikely(!inode)) {
> pr_err("out of free dentries, can not create directory '%s'\n",
> @@ -583,6 +600,11 @@ struct dentry *debugfs_create_automount(const char *name,
> if (IS_ERR(dentry))
> return dentry;
>
> + if (!(debugfs_allow & DEBUGFS_ALLOW_API_BIT)) {
> + failed_creating(dentry);
> + return ERR_PTR(-EPERM);
> + }
> +
> inode = debugfs_get_inode(dentry->d_sb);
> if (unlikely(!inode)) {
> pr_err("out of free dentries, can not create automount '%s'\n",
> @@ -786,10 +808,25 @@ bool debugfs_initialized(void)
> }
> EXPORT_SYMBOL_GPL(debugfs_initialized);
>
> +static int __init debugfs_kernel(char *str)
> +{
> + if (str && !strcmp(str, "on"))
> + debugfs_allow = DEBUGFS_ALLOW_API_BIT | DEBUGFS_ALLOW_FS_BIT;
> + if (str && !strcmp(str, "no-fs"))
> + debugfs_allow = DEBUGFS_ALLOW_API_BIT;
> + if (str && !strcmp(str, "off"))
> + debugfs_allow = 0;
> +
> + return 0;
> +}
> +early_param("debugfs", debugfs_kernel);
> static int __init debugfs_init(void)
> {
> int retval;
>
> + if (!(debugfs_allow & DEBUGFS_ALLOW_FS_BIT))
> + return -EPERM;
> +
> retval = sysfs_create_mount_point(kernel_kobj, "debug");
> if (retval)
> return retval;
> diff --git a/fs/debugfs/internal.h b/fs/debugfs/internal.h
> index 034e6973cead..dba138f8d418 100644
> --- a/fs/debugfs/internal.h
> +++ b/fs/debugfs/internal.h
> @@ -29,4 +29,18 @@ struct debugfs_fsdata {
> */
> #define DEBUGFS_FSDATA_IS_REAL_FOPS_BIT BIT(0)
>
> +/* Access BITS */
> +#define DEBUGFS_ALLOW_API_BIT BIT(0)
> +#define DEBUGFS_ALLOW_FS_BIT BIT(1)
tabs please.
And no need for _BIT in the string, right?
> +
> +#ifdef CONFIG_DEBUG_FS_ACCESS_ALL
> +#define DEFAULT_DEBUGFS_ACCESS_BITS
> (DEBUGFS_ALLOW_FS_BIT|DEBUGFS_ALLOW_API_BIT)
' ' is your friend :)
> +#endif
> +#ifdef CONFIG_DEBUG_FS_ACCESS_NO_FS
> +#define DEFAULT_DEBUGFS_ACCESS_BITS (DEBUGFS_ALLOW_API_BIT)
> +#endif
> +#ifdef CONFIG_DEBUG_FS_ACCESS_NONE
> +#define DEFAULT_DEBUGFS_ACCESS_BITS (0)
> +#endif
> +
> #endif /* _DEBUGFS_INTERNAL_H_ */
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index d74ac0fd6b2d..4c699ffad1fb 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -477,6 +477,38 @@ config DEBUG_FS
>
> If unsure, say N.
>
> +choice
> + prompt "Debugfs default access"
> + depends on DEBUG_FS
> + default DEBUG_FS_ACCESS_ALL
> + help
> + This select the default access restricions for debugfs.
> + It can be overridden with kernel command line option
> + debugfs=[on,no-fs,off] The restrictions apply for API access
> + and filesystem registration. .
> +
> +config DEBUG_FS_ACCESS_ALL
> + bool "Access all"
"Normal access" as this is what we have today, "Access all" doesn't
really explain what this is.
> + help
> + No restrictions applies. Both API and filesystem registration
> + is on.
Add:
"This is the normal default operation"
Or something like that?
And you mix tabs and spaces a bunch in this patch for this file, did
checkpatch not complain?
thanks,
greg k-h
