lkml.org 
[lkml]   [2020]   [Jun]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH v3] debugfs: Add access restriction option
    Date
    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 <peter.enderborg@sony.com>
    >> ---
    >> 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

    \
     
     \ /
      Last update: 2020-06-17 16:56    [W:2.404 / U:0.464 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site