2020-06-17 13:42:10

by peter enderborg

[permalink] [raw]
Subject: [PATCH v3] debugfs: Add access restriction option

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.

When enabled it is needed a kernel command line parameter to be activated.

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]>
---
v2. Removed MOUNT as part of restrictions. Added API's restrictions as
separate restriction.
v3 Updated Documentation after Randy Dunlap reviews and suggestions.

.../admin-guide/kernel-parameters.txt | 11 +++++
fs/debugfs/inode.c | 47 +++++++++++++++++++
lib/Kconfig.debug | 10 ++++
3 files changed, 68 insertions(+)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index fb95fad81c79..249c86e53bb7 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -827,6 +827,17 @@
useful to also enable the page_owner functionality.
on: enable the feature

+ debugfs= [KNL] When CONFIG_DEBUG_FS_RESTRICTED is set, this parameter
+ enables what is exposed to userspace.
+ 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
+ it's content. There its nothing to mount.
+ off: (default) Filesystem is not registered and clients
+ get a -EPERM as result when trying to register files
+ or directories within debugfs.
+
debugpat [X86] Enable PAT debugging

decnet.addr= [HW,NET]
diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
index b7f2e971ecbc..2bd80a932ae1 100644
--- a/fs/debugfs/inode.c
+++ b/fs/debugfs/inode.c
@@ -31,10 +31,17 @@
#include "internal.h"

#define DEBUGFS_DEFAULT_MODE 0700
+#ifdef CONFIG_DEBUG_FS_RESTRICTED
+#define DEBUGFS_ALLOW_API 0x2
+#define DEBUGFS_ALLOW_FS 0x1
+#endif

static struct vfsmount *debugfs_mount;
static int debugfs_mount_count;
static bool debugfs_registered;
+#ifdef CONFIG_DEBUG_FS_RESTRICTED
+static unsigned int debugfs_allow;
+#endif

/*
* Don't allow access attributes to be changed whilst the kernel is locked down
@@ -266,6 +273,10 @@ static struct dentry *debug_mount(struct file_system_type *fs_type,
int flags, const char *dev_name,
void *data)
{
+#ifdef CONFIG_DEBUG_FS_RESTRICTED
+ if (!(debugfs_allow & DEBUGFS_ALLOW_API))
+ return ERR_PTR(-EPERM);
+#endif
return mount_single(fs_type, flags, data, debug_fill_super);
}

@@ -385,6 +396,12 @@ static struct dentry *__debugfs_create_file(const char *name, umode_t mode,
if (IS_ERR(dentry))
return dentry;

+#ifdef CONFIG_DEBUG_FS_RESTRICTED
+ if (!(debugfs_allow & DEBUGFS_ALLOW_API)) {
+ failed_creating(dentry);
+ return ERR_PTR(-EPERM);
+ }
+#endif
inode = debugfs_get_inode(dentry->d_sb);
if (unlikely(!inode)) {
pr_err("out of free dentries, can not create file '%s'\n",
@@ -541,6 +558,12 @@ struct dentry *debugfs_create_dir(const char *name, struct dentry *parent)
if (IS_ERR(dentry))
return dentry;

+#ifdef CONFIG_DEBUG_FS_RESTRICTED
+ if (!(debugfs_allow & DEBUGFS_ALLOW_API)) {
+ failed_creating(dentry);
+ return ERR_PTR(-EPERM);
+ }
+#endif
inode = debugfs_get_inode(dentry->d_sb);
if (unlikely(!inode)) {
pr_err("out of free dentries, can not create directory '%s'\n",
@@ -583,6 +606,12 @@ struct dentry *debugfs_create_automount(const char *name,
if (IS_ERR(dentry))
return dentry;

+#ifdef CONFIG_DEBUG_FS_RESTRICTED
+ if (!(debugfs_allow & DEBUGFS_ALLOW_API)) {
+ failed_creating(dentry);
+ return ERR_PTR(-EPERM);
+ }
+#endif
inode = debugfs_get_inode(dentry->d_sb);
if (unlikely(!inode)) {
pr_err("out of free dentries, can not create automount '%s'\n",
@@ -786,10 +815,28 @@ bool debugfs_initialized(void)
}
EXPORT_SYMBOL_GPL(debugfs_initialized);

+static int __init debugfs_kernel(char *str)
+{
+#ifdef CONFIG_DEBUG_FS_RESTRICTED
+ if (str && !strcmp(str, "on"))
+ debugfs_allow = DEBUGFS_ALLOW_API | DEBUGFS_ALLOW_FS;
+ if (str && !strcmp(str, "no-fs"))
+ debugfs_allow |= DEBUGFS_ALLOW_API;
+ if (str && !strcmp(str, "off"))
+ debugfs_allow = 0;
+#endif
+ return 0;
+
+}
+early_param("debugfs", debugfs_kernel);
static int __init debugfs_init(void)
{
int retval;

+#ifdef CONFIG_DEBUG_FS_RESTRICTED
+ if (!(debugfs_allow & DEBUGFS_ALLOW_FS))
+ return -EPERM;
+#endif
retval = sysfs_create_mount_point(kernel_kobj, "debug");
if (retval)
return retval;
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index d74ac0fd6b2d..19fdaae14e36 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -477,6 +477,16 @@ config DEBUG_FS

If unsure, say N.

+config DEBUG_FS_RESTRICTED
+ bool "Debug Filesystem restricted"
+ depends on DEBUG_FS
+ help
+ This is an additional restriction for mounting debugfs. It allows
+ the kernel to have debugfs compiled, but requires that kernel command
+ line has a debugfs parameter to register as a filesystem.
+
+ If unsure, say N.
+
source "lib/Kconfig.kgdb"

source "lib/Kconfig.ubsan"
--
2.17.1


2020-06-17 14:18:49

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [PATCH v3] debugfs: Add access restriction option

On Wed, Jun 17, 2020 at 03:37:38PM +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.
>
> When enabled it is needed a kernel command line parameter to be activated.
>
> 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]>
> ---
> v2. Removed MOUNT as part of restrictions. Added API's restrictions as
> separate restriction.
> v3 Updated Documentation after Randy Dunlap reviews and suggestions.
>
> .../admin-guide/kernel-parameters.txt | 11 +++++
> fs/debugfs/inode.c | 47 +++++++++++++++++++
> lib/Kconfig.debug | 10 ++++
> 3 files changed, 68 insertions(+)
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index fb95fad81c79..249c86e53bb7 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -827,6 +827,17 @@
> useful to also enable the page_owner functionality.
> on: enable the feature
>
> + debugfs= [KNL] When CONFIG_DEBUG_FS_RESTRICTED is set, this parameter
> + enables what is exposed to userspace.
> + 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
> + it's content. There its nothing to mount.
> + off: (default) Filesystem is not registered and clients
> + get a -EPERM as result when trying to register files
> + or directories within debugfs.
> +
> debugpat [X86] Enable PAT debugging
>
> decnet.addr= [HW,NET]
> diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
> index b7f2e971ecbc..2bd80a932ae1 100644
> --- a/fs/debugfs/inode.c
> +++ b/fs/debugfs/inode.c
> @@ -31,10 +31,17 @@
> #include "internal.h"
>
> #define DEBUGFS_DEFAULT_MODE 0700
> +#ifdef CONFIG_DEBUG_FS_RESTRICTED
> +#define DEBUGFS_ALLOW_API 0x2
> +#define DEBUGFS_ALLOW_FS 0x1

BIT()?

And a tab?

And why a #ifdef?

> +#endif
>
> static struct vfsmount *debugfs_mount;
> static int debugfs_mount_count;
> static bool debugfs_registered;
> +#ifdef CONFIG_DEBUG_FS_RESTRICTED
> +static unsigned int debugfs_allow;
> +#endif

Why #ifdef?

>
> /*
> * Don't allow access attributes to be changed whilst the kernel is locked down
> @@ -266,6 +273,10 @@ static struct dentry *debug_mount(struct file_system_type *fs_type,
> int flags, const char *dev_name,
> void *data)
> {
> +#ifdef CONFIG_DEBUG_FS_RESTRICTED
> + if (!(debugfs_allow & DEBUGFS_ALLOW_API))
> + return ERR_PTR(-EPERM);
> +#endif

Ick, all of this #ifdef is a mess, and can be totally avoided if you do
the logic right here. Please make it so that the functions and almost
all of the .c code does not have #ifdef CONFIG_DEBUG_FS_RESTRICTED at
all.


> return mount_single(fs_type, flags, data, debug_fill_super);
> }
>
> @@ -385,6 +396,12 @@ static struct dentry *__debugfs_create_file(const char *name, umode_t mode,
> if (IS_ERR(dentry))
> return dentry;
>
> +#ifdef CONFIG_DEBUG_FS_RESTRICTED
> + if (!(debugfs_allow & DEBUGFS_ALLOW_API)) {
> + failed_creating(dentry);
> + return ERR_PTR(-EPERM);
> + }
> +#endif
> inode = debugfs_get_inode(dentry->d_sb);
> if (unlikely(!inode)) {
> pr_err("out of free dentries, can not create file '%s'\n",
> @@ -541,6 +558,12 @@ struct dentry *debugfs_create_dir(const char *name, struct dentry *parent)
> if (IS_ERR(dentry))
> return dentry;
>
> +#ifdef CONFIG_DEBUG_FS_RESTRICTED
> + if (!(debugfs_allow & DEBUGFS_ALLOW_API)) {
> + failed_creating(dentry);
> + return ERR_PTR(-EPERM);
> + }
> +#endif
> inode = debugfs_get_inode(dentry->d_sb);
> if (unlikely(!inode)) {
> pr_err("out of free dentries, can not create directory '%s'\n",
> @@ -583,6 +606,12 @@ struct dentry *debugfs_create_automount(const char *name,
> if (IS_ERR(dentry))
> return dentry;
>
> +#ifdef CONFIG_DEBUG_FS_RESTRICTED
> + if (!(debugfs_allow & DEBUGFS_ALLOW_API)) {
> + failed_creating(dentry);
> + return ERR_PTR(-EPERM);
> + }
> +#endif
> inode = debugfs_get_inode(dentry->d_sb);
> if (unlikely(!inode)) {
> pr_err("out of free dentries, can not create automount '%s'\n",
> @@ -786,10 +815,28 @@ bool debugfs_initialized(void)
> }
> EXPORT_SYMBOL_GPL(debugfs_initialized);
>
> +static int __init debugfs_kernel(char *str)
> +{
> +#ifdef CONFIG_DEBUG_FS_RESTRICTED
> + if (str && !strcmp(str, "on"))
> + debugfs_allow = DEBUGFS_ALLOW_API | DEBUGFS_ALLOW_FS;
> + if (str && !strcmp(str, "no-fs"))
> + debugfs_allow |= DEBUGFS_ALLOW_API;
> + if (str && !strcmp(str, "off"))
> + debugfs_allow = 0;

It's set to 0 by default, no need to set it again, right?

> +#endif
> + return 0;
> +
> +}
> +early_param("debugfs", debugfs_kernel);

Why is this a valid parm even if the option is not enabled? Do you mean
to do that? Why?

thanks,

greg k-h

2020-06-17 15:00:49

by peter enderborg

[permalink] [raw]
Subject: Re: [PATCH v3] debugfs: Add access restriction option

On 6/17/20 4:15 PM, Greg Kroah-Hartman wrote:
> On Wed, Jun 17, 2020 at 03:37:38PM +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.
>>
>> When enabled it is needed a kernel command line parameter to be activated.
>>
>> 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]>
>> ---
>> v2. Removed MOUNT as part of restrictions. Added API's restrictions as
>> separate restriction.
>> v3 Updated Documentation after Randy Dunlap reviews and suggestions.
>>
>> .../admin-guide/kernel-parameters.txt | 11 +++++
>> fs/debugfs/inode.c | 47 +++++++++++++++++++
>> lib/Kconfig.debug | 10 ++++
>> 3 files changed, 68 insertions(+)
>>
>> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
>> index fb95fad81c79..249c86e53bb7 100644
>> --- a/Documentation/admin-guide/kernel-parameters.txt
>> +++ b/Documentation/admin-guide/kernel-parameters.txt
>> @@ -827,6 +827,17 @@
>> useful to also enable the page_owner functionality.
>> on: enable the feature
>>
>> + debugfs= [KNL] When CONFIG_DEBUG_FS_RESTRICTED is set, this parameter
>> + enables what is exposed to userspace.
>> + 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
>> + it's content. There its nothing to mount.
>> + off: (default) Filesystem is not registered and clients
>> + get a -EPERM as result when trying to register files
>> + or directories within debugfs.
>> +
>> debugpat [X86] Enable PAT debugging
>>
>> decnet.addr= [HW,NET]
>> diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
>> index b7f2e971ecbc..2bd80a932ae1 100644
>> --- a/fs/debugfs/inode.c
>> +++ b/fs/debugfs/inode.c
>> @@ -31,10 +31,17 @@
>> #include "internal.h"
>>
>> #define DEBUGFS_DEFAULT_MODE 0700
>> +#ifdef CONFIG_DEBUG_FS_RESTRICTED
>> +#define DEBUGFS_ALLOW_API 0x2
>> +#define DEBUGFS_ALLOW_FS 0x1
> BIT()?
>
> And a tab?
>
> And why a #ifdef?
To get it as least intrusive as possible. A solid Opt-In.
>
>> +#endif
>>
>> static struct vfsmount *debugfs_mount;
>> static int debugfs_mount_count;
>> static bool debugfs_registered;
>> +#ifdef CONFIG_DEBUG_FS_RESTRICTED
>> +static unsigned int debugfs_allow;
>> +#endif
> Why #ifdef?
>
>>
>> /*
>> * Don't allow access attributes to be changed whilst the kernel is locked down
>> @@ -266,6 +273,10 @@ static struct dentry *debug_mount(struct file_system_type *fs_type,
>> int flags, const char *dev_name,
>> void *data)
>> {
>> +#ifdef CONFIG_DEBUG_FS_RESTRICTED
>> + if (!(debugfs_allow & DEBUGFS_ALLOW_API))
>> + return ERR_PTR(-EPERM);
>> +#endif
> Ick, all of this #ifdef is a mess, and can be totally avoided if you do
> the logic right here. Please make it so that the functions and almost
> all of the .c code does not have #ifdef CONFIG_DEBUG_FS_RESTRICTED at
> all.
>
Is it ok to remove the #ifdefs and let code always be there and let the config set the default value?


>> return mount_single(fs_type, flags, data, debug_fill_super);
>> }
>>
>> @@ -385,6 +396,12 @@ static struct dentry *__debugfs_create_file(const char *name, umode_t mode,
>> if (IS_ERR(dentry))
>> return dentry;
>>
>> +#ifdef CONFIG_DEBUG_FS_RESTRICTED
>> + if (!(debugfs_allow & DEBUGFS_ALLOW_API)) {
>> + failed_creating(dentry);
>> + return ERR_PTR(-EPERM);
>> + }
>> +#endif
>> inode = debugfs_get_inode(dentry->d_sb);
>> if (unlikely(!inode)) {
>> pr_err("out of free dentries, can not create file '%s'\n",
>> @@ -541,6 +558,12 @@ struct dentry *debugfs_create_dir(const char *name, struct dentry *parent)
>> if (IS_ERR(dentry))
>> return dentry;
>>
>> +#ifdef CONFIG_DEBUG_FS_RESTRICTED
>> + if (!(debugfs_allow & DEBUGFS_ALLOW_API)) {
>> + failed_creating(dentry);
>> + return ERR_PTR(-EPERM);
>> + }
>> +#endif
>> inode = debugfs_get_inode(dentry->d_sb);
>> if (unlikely(!inode)) {
>> pr_err("out of free dentries, can not create directory '%s'\n",
>> @@ -583,6 +606,12 @@ struct dentry *debugfs_create_automount(const char *name,
>> if (IS_ERR(dentry))
>> return dentry;
>>
>> +#ifdef CONFIG_DEBUG_FS_RESTRICTED
>> + if (!(debugfs_allow & DEBUGFS_ALLOW_API)) {
>> + failed_creating(dentry);
>> + return ERR_PTR(-EPERM);
>> + }
>> +#endif
>> inode = debugfs_get_inode(dentry->d_sb);
>> if (unlikely(!inode)) {
>> pr_err("out of free dentries, can not create automount '%s'\n",
>> @@ -786,10 +815,28 @@ bool debugfs_initialized(void)
>> }
>> EXPORT_SYMBOL_GPL(debugfs_initialized);
>>
>> +static int __init debugfs_kernel(char *str)
>> +{
>> +#ifdef CONFIG_DEBUG_FS_RESTRICTED
>> + if (str && !strcmp(str, "on"))
>> + debugfs_allow = DEBUGFS_ALLOW_API | DEBUGFS_ALLOW_FS;
>> + if (str && !strcmp(str, "no-fs"))
>> + debugfs_allow |= DEBUGFS_ALLOW_API;
>> + if (str && !strcmp(str, "off"))
>> + debugfs_allow = 0;
> It's set to 0 by default, no need to set it again, right?
I think there have been some issues with the same parameter more than once.
>
>> +#endif
>> + return 0;
>> +
>> +}
>> +early_param("debugfs", debugfs_kernel);
> Why is this a valid parm even if the option is not enabled? Do you mean
> to do that? Why?

I did not find any good usage where it was config dependent, when it is there; it isĀ  "reserve" the name.

It will always be there if we remove the #ifdefs.

> thanks,
>
> greg k-h

2020-06-17 15:15:38

by Randy Dunlap

[permalink] [raw]
Subject: Re: [PATCH v3] debugfs: Add access restriction option

On 6/17/20 6:37 AM, Peter Enderborg wrote:
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index fb95fad81c79..249c86e53bb7 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -827,6 +827,17 @@
> useful to also enable the page_owner functionality.
> on: enable the feature
>
> + debugfs= [KNL] When CONFIG_DEBUG_FS_RESTRICTED is set, this parameter
> + enables what is exposed to userspace.
> + 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
> + it's content. There its nothing to mount.

its content. There is nothing to mount.


> + off: (default) Filesystem is not registered and clients
> + get a -EPERM as result when trying to register files
> + or directories within debugfs.
> +
> debugpat [X86] Enable PAT debugging
>
> decnet.addr= [HW,NET]


--
~Randy

2020-06-22 08:33:14

by peter enderborg

[permalink] [raw]
Subject: [PATCH v4 0/2] debugfs: Add access restriction option

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.

v2. Removed MOUNT as part of restrictions. Added API's restrictions as
separate restriction.
v3 Updated Documentation after Randy Dunlap reviews and suggestions.
v4 Removed #ifdefs from inode.c and using internal.h for configuration
and now using BIT() for that. Function is now always on, and are
instead selected by a built in default or command line parameter.
Added preparation patch that removes check debugfs is initialised.
Reported-by: kernel test robot <[email protected]>

2020-07-15 09:14:23

by peter enderborg

[permalink] [raw]
Subject: [PATCH v5 0/2] debugfs: Add access restriction option


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.

v2. Removed MOUNT as part of restrictions. Added API's restrictions as
separate restriction.
v3 Updated Documentation after Randy Dunlap reviews and suggestions.
v4 Removed #ifdefs from inode.c and using internal.h for configuration
and now using BIT() for that. Function is now always on, and are
instead selected by a built in default or command line parameter.
Changed return value on debug_mount
Reported-by: kernel test robot <[email protected]>
Im not sure about that it is right
v5 Added notes to config help suggested by GregKH.
Removed _BIT from names, white-space and tab.
(checkpatch did not complain).


2020-07-15 15:27:11

by peter enderborg

[permalink] [raw]
Subject: [PATCH v6 0/2] debugfs: Add access restriction option

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.

v2. Removed MOUNT as part of restrictions. Added API's restrictions as
separate restriction.
v3 Updated Documentation after Randy Dunlap reviews and suggestions.
v4 Removed #ifdefs from inode.c and using internal.h for configuration
and now using BIT() for that. Function is now always on, and are
instead selected by a built in default or command line parameter.
Changed return value on debug_mount
Reported-by: kernel test robot <[email protected]>
Im not sure about that it is right
v5 Added notes to config help suggested by GregKH.
Removed _BIT from names, white-space and tab.
(checkpatch did not complain).
v6 Using ALLOW instead of ACCESS as name on BIT's. Change the fs to
mount to make it clear and easy to understand.


2020-07-15 15:28:14

by peter enderborg

[permalink] [raw]
Subject: [PATCH 1/2] tracefs: Remove unnecessary debug_fs checks.

This is a preparation for debugfs restricted mode.
We don't need debugfs to trace, the removed check stop tracefs to work
if debugfs is not initialised. We instead tries to automount within
debugfs and relay on it's handling. The code path is to create a
backward compatibility from when tracefs was part of debugfs, it is now
standalone and does not need debugfs. When debugfs is in restricted
it is compiled in but not active and return EPERM to clients and
tracefs wont work if it assumes it is active it is compiled in
kernel.

Reported-by: kernel test robot <[email protected]>
Signed-off-by: Peter Enderborg <[email protected]>
Reviewed-by: Greg Kroah-Hartman <[email protected]>
Acked-by: Steven Rostedt (VMware) <[email protected]>
---
kernel/trace/trace.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index bb62269724d5..848f67a5f16d 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -8945,9 +8945,7 @@ struct dentry *tracing_init_dentry(void)
if (tr->dir)
return NULL;

- if (WARN_ON(!tracefs_initialized()) ||
- (IS_ENABLED(CONFIG_DEBUG_FS) &&
- WARN_ON(!debugfs_initialized())))
+ if (WARN_ON(!tracefs_initialized()))
return ERR_PTR(-ENODEV);

/*
--
2.17.1

2020-07-16 04:57:35

by peter enderborg

[permalink] [raw]
Subject: [PATCH v7 0/2] debugfs: Add access restriction option

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.

v2. Removed MOUNT as part of restrictions. Added API's restrictions as
separate restriction.
v3 Updated Documentation after Randy Dunlap reviews and suggestions.
v4 Removed #ifdefs from inode.c and using internal.h for configuration
and now using BIT() for that. Function is now always on, and are
instead selected by a built in default or command line parameter.
Changed return value on debug_mount
Reported-by: kernel test robot <[email protected]>
Im not sure about that it is right
v5 Added notes to config help suggested by GregKH.
Removed _BIT from names, white-space and tab.
(checkpatch did not complain).
v6 Using ALLOW instead of ACCESS as name on BIT's. Change the fs to
mount to make it clear and easy to understand.
v7 Updated Kconfig.debug with Randy Dunlap corrections.


2020-07-16 07:18:55

by peter enderborg

[permalink] [raw]
Subject: [PATCH v8 0/2] debugfs: Add access restriction option

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.

v2. Removed MOUNT as part of restrictions. Added API's restrictions as
separate restriction.
v3 Updated Documentation after Randy Dunlap reviews and suggestions.
v4 Removed #ifdefs from inode.c and using internal.h for configuration
and now using BIT() for that. Function is now always on, and are
instead selected by a built in default or command line parameter.
Changed return value on debug_mount
Reported-by: kernel test robot <[email protected]>
Im not sure about that it is right
v5 Added notes to config help suggested by GregKH.
Removed _BIT from names, white-space and tab.
(checkpatch did not complain).
v6 Using ALLOW instead of ACCESS as name on BIT's. Change the fs to
mount to make it clear and easy to understand.
v7 Updated Kconfig.debug with Randy Dunlap corrections.
v8 Spell fixes from Randy and using else-if for command argument
parser.


2020-07-23 15:13:58

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [PATCH v8 0/2] debugfs: Add access restriction option

On Thu, Jul 16, 2020 at 09:15:09AM +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.
>
> v2. Removed MOUNT as part of restrictions. Added API's restrictions as
> separate restriction.
> v3 Updated Documentation after Randy Dunlap reviews and suggestions.
> v4 Removed #ifdefs from inode.c and using internal.h for configuration
> and now using BIT() for that. Function is now always on, and are
> instead selected by a built in default or command line parameter.
> Changed return value on debug_mount
> Reported-by: kernel test robot <[email protected]>
> Im not sure about that it is right
> v5 Added notes to config help suggested by GregKH.
> Removed _BIT from names, white-space and tab.
> (checkpatch did not complain).
> v6 Using ALLOW instead of ACCESS as name on BIT's. Change the fs to
> mount to make it clear and easy to understand.
> v7 Updated Kconfig.debug with Randy Dunlap corrections.
> v8 Spell fixes from Randy and using else-if for command argument
> parser.
>
>

Thanks for sticking with this, now queued up!

greg k-h