* [PATCH] debugfs: Add mount restriction option @ 2020-05-28 8:00 Peter Enderborg 2020-05-28 8:27 ` Greg Kroah-Hartman 0 siblings, 1 reply; 7+ messages in thread From: Peter Enderborg @ 2020-05-28 8:00 UTC (permalink / raw) To: Greg Kroah-Hartman, linux-kernel, Rafael J . Wysocki, Andrew Morton Cc: Peter Enderborg 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. It is needed new kernel command line parameter to be activated. Signed-off-by: Peter Enderborg <peter.enderborg@sony.com> --- fs/debugfs/inode.c | 17 ++++++++++++++++- lib/Kconfig.debug | 10 ++++++++++ 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c index b7f2e971ecbc..bde37dab77e0 100644 --- a/fs/debugfs/inode.c +++ b/fs/debugfs/inode.c @@ -786,10 +786,25 @@ bool debugfs_initialized(void) } EXPORT_SYMBOL_GPL(debugfs_initialized); +static int allow_debugfs; + +static int __init debugfs_kernel(char *str) +{ + if (str && !strcmp(str, "true")) + allow_debugfs = true; + + return 0; + +} +early_param("debugfs", debugfs_kernel); + static int __init debugfs_init(void) { int retval; - +#ifdef CONFIG_DEBUG_FS_MOUNT_RESTRICTED + if (!allow_debugfs) + 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 21d9c5f6e7ec..d3a3338740d2 100644 --- a/lib/Kconfig.debug +++ b/lib/Kconfig.debug @@ -443,6 +443,16 @@ config DEBUG_FS If unsure, say N. +config DEBUG_FS_MOUNT_RESTRICTED + bool "Debug Filesystem mount 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.26.2 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] debugfs: Add mount restriction option 2020-05-28 8:00 [PATCH] debugfs: Add mount restriction option Peter Enderborg @ 2020-05-28 8:27 ` Greg Kroah-Hartman 2020-05-28 8:39 ` Enderborg, Peter 0 siblings, 1 reply; 7+ messages in thread From: Greg Kroah-Hartman @ 2020-05-28 8:27 UTC (permalink / raw) To: Peter Enderborg; +Cc: linux-kernel, Rafael J . Wysocki, Andrew Morton On Thu, May 28, 2020 at 10:00:31AM +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. It is needed new > kernel command line parameter to be activated. By "configuration" do you mean "kernel configuration"? What is wrong with relying on the build option like we do today? You might want to reword all of this to make more sense about the "problem" you are trying to solve here, as I don't really understand it, sorry. > > Signed-off-by: Peter Enderborg <peter.enderborg@sony.com> > --- > fs/debugfs/inode.c | 17 ++++++++++++++++- > lib/Kconfig.debug | 10 ++++++++++ > 2 files changed, 26 insertions(+), 1 deletion(-) No documentation update? That's not good :( > diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c > index b7f2e971ecbc..bde37dab77e0 100644 > --- a/fs/debugfs/inode.c > +++ b/fs/debugfs/inode.c > @@ -786,10 +786,25 @@ bool debugfs_initialized(void) > } > EXPORT_SYMBOL_GPL(debugfs_initialized); > > +static int allow_debugfs; > + > +static int __init debugfs_kernel(char *str) > +{ > + if (str && !strcmp(str, "true")) > + allow_debugfs = true; > + > + return 0; > + > +} > +early_param("debugfs", debugfs_kernel); > + > static int __init debugfs_init(void) > { > int retval; > - > +#ifdef CONFIG_DEBUG_FS_MOUNT_RESTRICTED > + if (!allow_debugfs) > + return -EPERM; > +#endif But you are not restricting the ability to mount it here, you are removing the ability for it to even start up at all. What does this break for code that thinks the filesystem is registered (i.e. the call to simple_pin_fs() in start_creating() in fs/debugfs/inode.c? > retval = sysfs_create_mount_point(kernel_kobj, "debug"); > if (retval) > return retval; > diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug > index 21d9c5f6e7ec..d3a3338740d2 100644 > --- a/lib/Kconfig.debug > +++ b/lib/Kconfig.debug > @@ -443,6 +443,16 @@ config DEBUG_FS > > If unsure, say N. > > +config DEBUG_FS_MOUNT_RESTRICTED > + bool "Debug Filesystem mount 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. No hint as to what the command line option is? And again, it's not a restriction for mounting it, you are preventing it from initializing entirely. Which, as I have found out over time, can had side affects for codepaths that try to check the return value of debugfs calls, and if they fail to succeed, abort the whole driver/subsystem/whatever initializaion path, which is not what I think you want to do here at all. thanks, greg k-h ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] debugfs: Add mount restriction option 2020-05-28 8:27 ` Greg Kroah-Hartman @ 2020-05-28 8:39 ` Enderborg, Peter 2020-05-28 9:27 ` Greg Kroah-Hartman 0 siblings, 1 reply; 7+ messages in thread From: Enderborg, Peter @ 2020-05-28 8:39 UTC (permalink / raw) To: Greg Kroah-Hartman; +Cc: linux-kernel, Rafael J . Wysocki, Andrew Morton On 5/28/20 10:27 AM, Greg Kroah-Hartman wrote: > On Thu, May 28, 2020 at 10:00:31AM +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. It is needed new >> kernel command line parameter to be activated. > By "configuration" do you mean "kernel configuration"? What is wrong > with relying on the build option like we do today? > > You might want to reword all of this to make more sense about the > "problem" you are trying to solve here, as I don't really understand it, > sorry. > > >> Signed-off-by: Peter Enderborg <peter.enderborg@sony.com> >> --- >> fs/debugfs/inode.c | 17 ++++++++++++++++- >> lib/Kconfig.debug | 10 ++++++++++ >> 2 files changed, 26 insertions(+), 1 deletion(-) > No documentation update? That's not good :( > > >> diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c >> index b7f2e971ecbc..bde37dab77e0 100644 >> --- a/fs/debugfs/inode.c >> +++ b/fs/debugfs/inode.c >> @@ -786,10 +786,25 @@ bool debugfs_initialized(void) >> } >> EXPORT_SYMBOL_GPL(debugfs_initialized); >> >> +static int allow_debugfs; >> + >> +static int __init debugfs_kernel(char *str) >> +{ >> + if (str && !strcmp(str, "true")) >> + allow_debugfs = true; >> + >> + return 0; >> + >> +} >> +early_param("debugfs", debugfs_kernel); >> + >> static int __init debugfs_init(void) >> { >> int retval; >> - >> +#ifdef CONFIG_DEBUG_FS_MOUNT_RESTRICTED >> + if (!allow_debugfs) >> + return -EPERM; >> +#endif > But you are not restricting the ability to mount it here, you are > removing the ability for it to even start up at all. What does this > break for code that thinks the filesystem is registered (i.e. the call > to simple_pin_fs() in start_creating() in fs/debugfs/inode.c? > If it does, the lines below is also cause the same problem. > >> retval = sysfs_create_mount_point(kernel_kobj, "debug"); >> if (retval) >> return retval; >> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug >> index 21d9c5f6e7ec..d3a3338740d2 100644 >> --- a/lib/Kconfig.debug >> +++ b/lib/Kconfig.debug >> @@ -443,6 +443,16 @@ config DEBUG_FS >> >> If unsure, say N. >> >> +config DEBUG_FS_MOUNT_RESTRICTED >> + bool "Debug Filesystem mount 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. > No hint as to what the command line option is? And again, it's not a > restriction for mounting it, you are preventing it from initializing > entirely. > > Which, as I have found out over time, can had side affects for codepaths > that try to check the return value of debugfs calls, and if they fail to > succeed, abort the whole driver/subsystem/whatever initializaion path, > which is not what I think you want to do here at all. > > thanks, > > greg k-h ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] debugfs: Add mount restriction option 2020-05-28 8:39 ` Enderborg, Peter @ 2020-05-28 9:27 ` Greg Kroah-Hartman 2020-05-28 12:04 ` Enderborg, Peter 0 siblings, 1 reply; 7+ messages in thread From: Greg Kroah-Hartman @ 2020-05-28 9:27 UTC (permalink / raw) To: Enderborg, Peter; +Cc: linux-kernel, Rafael J . Wysocki, Andrew Morton On Thu, May 28, 2020 at 08:39:02AM +0000, Enderborg, Peter wrote: > On 5/28/20 10:27 AM, Greg Kroah-Hartman wrote: > > On Thu, May 28, 2020 at 10:00:31AM +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. It is needed new > >> kernel command line parameter to be activated. > > By "configuration" do you mean "kernel configuration"? What is wrong > > with relying on the build option like we do today? > > > > You might want to reword all of this to make more sense about the > > "problem" you are trying to solve here, as I don't really understand it, > > sorry. > > > > > >> Signed-off-by: Peter Enderborg <peter.enderborg@sony.com> > >> --- > >> fs/debugfs/inode.c | 17 ++++++++++++++++- > >> lib/Kconfig.debug | 10 ++++++++++ > >> 2 files changed, 26 insertions(+), 1 deletion(-) > > No documentation update? That's not good :( > > > > > >> diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c > >> index b7f2e971ecbc..bde37dab77e0 100644 > >> --- a/fs/debugfs/inode.c > >> +++ b/fs/debugfs/inode.c > >> @@ -786,10 +786,25 @@ bool debugfs_initialized(void) > >> } > >> EXPORT_SYMBOL_GPL(debugfs_initialized); > >> > >> +static int allow_debugfs; > >> + > >> +static int __init debugfs_kernel(char *str) > >> +{ > >> + if (str && !strcmp(str, "true")) > >> + allow_debugfs = true; > >> + > >> + return 0; > >> + > >> +} > >> +early_param("debugfs", debugfs_kernel); > >> + > >> static int __init debugfs_init(void) > >> { > >> int retval; > >> - > >> +#ifdef CONFIG_DEBUG_FS_MOUNT_RESTRICTED > >> + if (!allow_debugfs) > >> + return -EPERM; > >> +#endif > > But you are not restricting the ability to mount it here, you are > > removing the ability for it to even start up at all. What does this > > break for code that thinks the filesystem is registered (i.e. the call > > to simple_pin_fs() in start_creating() in fs/debugfs/inode.c? > > > If it does, the lines below is also cause the same problem. In a working system, errors in the lines below will never happen :) thanks, greg k-h ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] debugfs: Add mount restriction option 2020-05-28 9:27 ` Greg Kroah-Hartman @ 2020-05-28 12:04 ` Enderborg, Peter 2020-05-28 12:23 ` Greg Kroah-Hartman 0 siblings, 1 reply; 7+ messages in thread From: Enderborg, Peter @ 2020-05-28 12:04 UTC (permalink / raw) To: Greg Kroah-Hartman; +Cc: linux-kernel, Rafael J . Wysocki, Andrew Morton On 5/28/20 11:27 AM, Greg Kroah-Hartman wrote: > On Thu, May 28, 2020 at 08:39:02AM +0000, Enderborg, Peter wrote: >> On 5/28/20 10:27 AM, Greg Kroah-Hartman wrote: >>> On Thu, May 28, 2020 at 10:00:31AM +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. It is needed new >>>> kernel command line parameter to be activated. >>> By "configuration" do you mean "kernel configuration"? What is wrong >>> with relying on the build option like we do today? >>> >>> You might want to reword all of this to make more sense about the >>> "problem" you are trying to solve here, as I don't really understand it, >>> sorry. >>> >>> >>>> Signed-off-by: Peter Enderborg <peter.enderborg@sony.com> >>>> --- >>>> fs/debugfs/inode.c | 17 ++++++++++++++++- >>>> lib/Kconfig.debug | 10 ++++++++++ >>>> 2 files changed, 26 insertions(+), 1 deletion(-) >>> No documentation update? That's not good :( >>> >>> >>>> diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c >>>> index b7f2e971ecbc..bde37dab77e0 100644 >>>> --- a/fs/debugfs/inode.c >>>> +++ b/fs/debugfs/inode.c >>>> @@ -786,10 +786,25 @@ bool debugfs_initialized(void) >>>> } >>>> EXPORT_SYMBOL_GPL(debugfs_initialized); >>>> >>>> +static int allow_debugfs; >>>> + >>>> +static int __init debugfs_kernel(char *str) >>>> +{ >>>> + if (str && !strcmp(str, "true")) >>>> + allow_debugfs = true; >>>> + >>>> + return 0; >>>> + >>>> +} >>>> +early_param("debugfs", debugfs_kernel); >>>> + >>>> static int __init debugfs_init(void) >>>> { >>>> int retval; >>>> - >>>> +#ifdef CONFIG_DEBUG_FS_MOUNT_RESTRICTED >>>> + if (!allow_debugfs) >>>> + return -EPERM; >>>> +#endif >>> But you are not restricting the ability to mount it here, you are >>> removing the ability for it to even start up at all. What does this >>> break for code that thinks the filesystem is registered (i.e. the call >>> to simple_pin_fs() in start_creating() in fs/debugfs/inode.c? >>> >> If it does, the lines below is also cause the same problem. > In a working system, errors in the lines below will never happen :) I think it is not unreasonable to expect that debug functionality are robust enough to handle system where not everything is working smoothly. > thanks, > > greg k-h ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] debugfs: Add mount restriction option 2020-05-28 12:04 ` Enderborg, Peter @ 2020-05-28 12:23 ` Greg Kroah-Hartman 2020-05-28 12:41 ` Enderborg, Peter 0 siblings, 1 reply; 7+ messages in thread From: Greg Kroah-Hartman @ 2020-05-28 12:23 UTC (permalink / raw) To: Enderborg, Peter; +Cc: linux-kernel, Rafael J . Wysocki, Andrew Morton On Thu, May 28, 2020 at 12:04:32PM +0000, Enderborg, Peter wrote: > On 5/28/20 11:27 AM, Greg Kroah-Hartman wrote: > > On Thu, May 28, 2020 at 08:39:02AM +0000, Enderborg, Peter wrote: > >> On 5/28/20 10:27 AM, Greg Kroah-Hartman wrote: > >>> On Thu, May 28, 2020 at 10:00:31AM +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. It is needed new > >>>> kernel command line parameter to be activated. > >>> By "configuration" do you mean "kernel configuration"? What is wrong > >>> with relying on the build option like we do today? > >>> > >>> You might want to reword all of this to make more sense about the > >>> "problem" you are trying to solve here, as I don't really understand it, > >>> sorry. > >>> > >>> > >>>> Signed-off-by: Peter Enderborg <peter.enderborg@sony.com> > >>>> --- > >>>> fs/debugfs/inode.c | 17 ++++++++++++++++- > >>>> lib/Kconfig.debug | 10 ++++++++++ > >>>> 2 files changed, 26 insertions(+), 1 deletion(-) > >>> No documentation update? That's not good :( > >>> > >>> > >>>> diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c > >>>> index b7f2e971ecbc..bde37dab77e0 100644 > >>>> --- a/fs/debugfs/inode.c > >>>> +++ b/fs/debugfs/inode.c > >>>> @@ -786,10 +786,25 @@ bool debugfs_initialized(void) > >>>> } > >>>> EXPORT_SYMBOL_GPL(debugfs_initialized); > >>>> > >>>> +static int allow_debugfs; > >>>> + > >>>> +static int __init debugfs_kernel(char *str) > >>>> +{ > >>>> + if (str && !strcmp(str, "true")) > >>>> + allow_debugfs = true; > >>>> + > >>>> + return 0; > >>>> + > >>>> +} > >>>> +early_param("debugfs", debugfs_kernel); > >>>> + > >>>> static int __init debugfs_init(void) > >>>> { > >>>> int retval; > >>>> - > >>>> +#ifdef CONFIG_DEBUG_FS_MOUNT_RESTRICTED > >>>> + if (!allow_debugfs) > >>>> + return -EPERM; > >>>> +#endif > >>> But you are not restricting the ability to mount it here, you are > >>> removing the ability for it to even start up at all. What does this > >>> break for code that thinks the filesystem is registered (i.e. the call > >>> to simple_pin_fs() in start_creating() in fs/debugfs/inode.c? > >>> > >> If it does, the lines below is also cause the same problem. > > In a working system, errors in the lines below will never happen :) > > I think it is not unreasonable to expect that debug functionality are robust enough to handle > system where not everything is working smoothly. I do not disagree, but have you tested this to ensure that your kernel still works when you disable debugfs in this manner? If so, great! Just trying to point out a potential problem... thanks, greg k-h ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] debugfs: Add mount restriction option 2020-05-28 12:23 ` Greg Kroah-Hartman @ 2020-05-28 12:41 ` Enderborg, Peter 0 siblings, 0 replies; 7+ messages in thread From: Enderborg, Peter @ 2020-05-28 12:41 UTC (permalink / raw) To: Greg Kroah-Hartman; +Cc: linux-kernel, Rafael J . Wysocki, Andrew Morton On 5/28/20 2:23 PM, Greg Kroah-Hartman wrote: > On Thu, May 28, 2020 at 12:04:32PM +0000, Enderborg, Peter wrote: >> On 5/28/20 11:27 AM, Greg Kroah-Hartman wrote: >>> On Thu, May 28, 2020 at 08:39:02AM +0000, Enderborg, Peter wrote: >>>> On 5/28/20 10:27 AM, Greg Kroah-Hartman wrote: >>>>> On Thu, May 28, 2020 at 10:00:31AM +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. It is needed new >>>>>> kernel command line parameter to be activated. >>>>> By "configuration" do you mean "kernel configuration"? What is wrong >>>>> with relying on the build option like we do today? >>>>> >>>>> You might want to reword all of this to make more sense about the >>>>> "problem" you are trying to solve here, as I don't really understand it, >>>>> sorry. >>>>> >>>>> >>>>>> Signed-off-by: Peter Enderborg <peter.enderborg@sony.com> >>>>>> --- >>>>>> fs/debugfs/inode.c | 17 ++++++++++++++++- >>>>>> lib/Kconfig.debug | 10 ++++++++++ >>>>>> 2 files changed, 26 insertions(+), 1 deletion(-) >>>>> No documentation update? That's not good :( >>>>> >>>>> >>>>>> diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c >>>>>> index b7f2e971ecbc..bde37dab77e0 100644 >>>>>> --- a/fs/debugfs/inode.c >>>>>> +++ b/fs/debugfs/inode.c >>>>>> @@ -786,10 +786,25 @@ bool debugfs_initialized(void) >>>>>> } >>>>>> EXPORT_SYMBOL_GPL(debugfs_initialized); >>>>>> >>>>>> +static int allow_debugfs; >>>>>> + >>>>>> +static int __init debugfs_kernel(char *str) >>>>>> +{ >>>>>> + if (str && !strcmp(str, "true")) >>>>>> + allow_debugfs = true; >>>>>> + >>>>>> + return 0; >>>>>> + >>>>>> +} >>>>>> +early_param("debugfs", debugfs_kernel); >>>>>> + >>>>>> static int __init debugfs_init(void) >>>>>> { >>>>>> int retval; >>>>>> - >>>>>> +#ifdef CONFIG_DEBUG_FS_MOUNT_RESTRICTED >>>>>> + if (!allow_debugfs) >>>>>> + return -EPERM; >>>>>> +#endif >>>>> But you are not restricting the ability to mount it here, you are >>>>> removing the ability for it to even start up at all. What does this >>>>> break for code that thinks the filesystem is registered (i.e. the call >>>>> to simple_pin_fs() in start_creating() in fs/debugfs/inode.c? >>>>> >>>> If it does, the lines below is also cause the same problem. >>> In a working system, errors in the lines below will never happen :) >> I think it is not unreasonable to expect that debug functionality are robust enough to handle >> system where not everything is working smoothly. > I do not disagree, but have you tested this to ensure that your kernel > still works when you disable debugfs in this manner? So far only tested on fedora f32 (x86_64) and older arm64 systems. So far only zswap that prints out that "debugfs initialization failed" and it still works fine. > If so, great! Just trying to point out a potential problem... Will do new patchset, im waiting a bit longer to see if there some other comments first. > thanks, > > greg k-h ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-05-28 12:41 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-05-28 8:00 [PATCH] debugfs: Add mount restriction option Peter Enderborg 2020-05-28 8:27 ` Greg Kroah-Hartman 2020-05-28 8:39 ` Enderborg, Peter 2020-05-28 9:27 ` Greg Kroah-Hartman 2020-05-28 12:04 ` Enderborg, Peter 2020-05-28 12:23 ` Greg Kroah-Hartman 2020-05-28 12:41 ` Enderborg, Peter
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.