[PATCH v2] debugfs: inode: debugfs_create_dir uses mode permission from parent

[PATCH v2] debugfs: inode: debugfs_create_dir uses mode permission from parent

[Thomas Richter]

Currently function debugfs_create_dir() creates a new
directory in the debugfs (usually mounted /sys/kernel/debug)
with permission rwxr-xr-x. This is hard coded.

Change this to use the parent directory permission.

Output before the patch:
root@s8360047 ~]# tree -dp -L 1 /sys/kernel/debug/
/sys/kernel/debug/
├── [drwxr-xr-x]  bdi
├── [drwxr-xr-x]  block
├── [drwxr-xr-x]  dasd
├── [drwxr-xr-x]  device_component
├── [drwxr-xr-x]  extfrag
├── [drwxr-xr-x]  hid
├── [drwxr-xr-x]  kprobes
├── [drwxr-xr-x]  kvm
├── [drwxr-xr-x]  memblock
├── [drwxr-xr-x]  pm_qos
├── [drwxr-xr-x]  qdio
├── [drwxr-xr-x]  s390
├── [drwxr-xr-x]  s390dbf
└── [drwx------]  tracing

14 directories
[root@s8360047 linux]#

Output after the patch:
[root@s8360047 ~]# tree -dp -L 1 /sys/kernel/debug/
sys/kernel/debug/
├── [drwx------]  bdi
├── [drwx------]  block
├── [drwx------]  dasd
├── [drwx------]  device_component
├── [drwx------]  extfrag
├── [drwx------]  hid
├── [drwx------]  kprobes
├── [drwx------]  kvm
├── [drwx------]  memblock
├── [drwx------]  pm_qos
├── [drwx------]  qdio
├── [drwx------]  s390
├── [drwx------]  s390dbf
└── [drwx------]  tracing

14 directories
[root@s8360047 linux]#

Here is the full diff output done with:
[root@s8360047 ~]# diff -u treefull.before treefull.after |
	sed 's-^- # -' > treefull.diff
 # --- treefull.before	2018-04-27 13:22:04.532824564 +0200
 # +++ treefull.after	2018-04-27 13:24:12.106182062 +0200
 # @@ -1,55 +1,55 @@
 #  /sys/kernel/debug/
 # -├── [drwxr-xr-x]  bdi
 # -│   ├── [drwxr-xr-x]  1:0
 # -│   ├── [drwxr-xr-x]  1:1
 # -│   ├── [drwxr-xr-x]  1:10
 # -│   ├── [drwxr-xr-x]  1:11
 # -│   ├── [drwxr-xr-x]  1:12
 # -│   ├── [drwxr-xr-x]  1:13
 # -│   ├── [drwxr-xr-x]  1:14
 # -│   ├── [drwxr-xr-x]  1:15
 # -│   ├── [drwxr-xr-x]  1:2
 # -│   ├── [drwxr-xr-x]  1:3
 # -│   ├── [drwxr-xr-x]  1:4
 # -│   ├── [drwxr-xr-x]  1:5
 # -│   ├── [drwxr-xr-x]  1:6
 # -│   ├── [drwxr-xr-x]  1:7
 # -│   ├── [drwxr-xr-x]  1:8
 # -│   ├── [drwxr-xr-x]  1:9
 # -│   └── [drwxr-xr-x]  94:0
 # -├── [drwxr-xr-x]  block
 # -├── [drwxr-xr-x]  dasd
 # -│   ├── [drwxr-xr-x]  0.0.e18a
 # -│   ├── [drwxr-xr-x]  dasda
 # -│   └── [drwxr-xr-x]  global
 # -├── [drwxr-xr-x]  device_component
 # -├── [drwxr-xr-x]  extfrag
 # -├── [drwxr-xr-x]  hid
 # -├── [drwxr-xr-x]  kprobes
 # -├── [drwxr-xr-x]  kvm
 # -├── [drwxr-xr-x]  memblock
 # -├── [drwxr-xr-x]  pm_qos
 # -├── [drwxr-xr-x]  qdio
 # -│   └── [drwxr-xr-x]  0.0.f5f2
 # -├── [drwxr-xr-x]  s390
 # -│   └── [drwxr-xr-x]  stsi
 # -├── [drwxr-xr-x]  s390dbf
 # -│   ├── [drwxr-xr-x]  0.0.e18a
 # -│   ├── [drwxr-xr-x]  cio_crw
 # -│   ├── [drwxr-xr-x]  cio_msg
 # -│   ├── [drwxr-xr-x]  cio_trace
 # -│   ├── [drwxr-xr-x]  dasd
 # -│   ├── [drwxr-xr-x]  kvm-trace
 # -│   ├── [drwxr-xr-x]  lgr
 # -│   ├── [drwxr-xr-x]  qdio_0.0.f5f2
 # -│   ├── [drwxr-xr-x]  qdio_error
 # -│   ├── [drwxr-xr-x]  qdio_setup
 # -│   ├── [drwxr-xr-x]  qeth_card_0.0.f5f0
 # -│   ├── [drwxr-xr-x]  qeth_control
 # -│   ├── [drwxr-xr-x]  qeth_msg
 # -│   ├── [drwxr-xr-x]  qeth_setup
 # -│   ├── [drwxr-xr-x]  vmcp
 # -│   └── [drwxr-xr-x]  vmur
 # +├── [drwx------]  bdi
 # +│   ├── [drwx------]  1:0
 # +│   ├── [drwx------]  1:1
 # +│   ├── [drwx------]  1:10
 # +│   ├── [drwx------]  1:11
 # +│   ├── [drwx------]  1:12
 # +│   ├── [drwx------]  1:13
 # +│   ├── [drwx------]  1:14
 # +│   ├── [drwx------]  1:15
 # +│   ├── [drwx------]  1:2
 # +│   ├── [drwx------]  1:3
 # +│   ├── [drwx------]  1:4
 # +│   ├── [drwx------]  1:5
 # +│   ├── [drwx------]  1:6
 # +│   ├── [drwx------]  1:7
 # +│   ├── [drwx------]  1:8
 # +│   ├── [drwx------]  1:9
 # +│   └── [drwx------]  94:0
 # +├── [drwx------]  block
 # +├── [drwx------]  dasd
 # +│   ├── [drwx------]  0.0.e18a
 # +│   ├── [drwx------]  dasda
 # +│   └── [drwx------]  global
 # +├── [drwx------]  device_component
 # +├── [drwx------]  extfrag
 # +├── [drwx------]  hid
 # +├── [drwx------]  kprobes
 # +├── [drwx------]  kvm
 # +├── [drwx------]  memblock
 # +├── [drwx------]  pm_qos
 # +├── [drwx------]  qdio
 # +│   └── [drwx------]  0.0.f5f2
 # +├── [drwx------]  s390
 # +│   └── [drwx------]  stsi
 # +├── [drwx------]  s390dbf
 # +│   ├── [drwx------]  0.0.e18a
 # +│   ├── [drwx------]  cio_crw
 # +│   ├── [drwx------]  cio_msg
 # +│   ├── [drwx------]  cio_trace
 # +│   ├── [drwx------]  dasd
 # +│   ├── [drwx------]  kvm-trace
 # +│   ├── [drwx------]  lgr
 # +│   ├── [drwx------]  qdio_0.0.f5f2
 # +│   ├── [drwx------]  qdio_error
 # +│   ├── [drwx------]  qdio_setup
 # +│   ├── [drwx------]  qeth_card_0.0.f5f0
 # +│   ├── [drwx------]  qeth_control
 # +│   ├── [drwx------]  qeth_msg
 # +│   ├── [drwx------]  qeth_setup
 # +│   ├── [drwx------]  vmcp
 # +│   └── [drwx------]  vmur
 #  └── [drwx------]  tracing
 #      ├── [drwxr-xr-x]  events
 #      │   ├── [drwxr-xr-x]  alarmtimer

Fixes: edac65eaf8d5c ("debugfs: take mode-dependent parts of debugfs_get_inode() into callers")
Signed-off-by: Thomas Richter <tmricht@linux.ibm.com>
Reviewed-by: Kees Cook <keescook@chromium.org>
---
 fs/debugfs/inode.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
index 13b0135..a913b12 100644
--- a/fs/debugfs/inode.c
+++ b/fs/debugfs/inode.c
@@ -512,7 +512,9 @@ struct dentry *debugfs_create_dir(const char *name, struct dentry *parent)
 	if (unlikely(!inode))
 		return failed_creating(dentry);
 
-	inode->i_mode = S_IFDIR | S_IRWXU | S_IRUGO | S_IXUGO;
+	if (!parent)
+		parent = debugfs_mount->mnt_root;
+	inode->i_mode = S_IFDIR | ((d_inode(parent)->i_mode & 0770));
 	inode->i_op = &simple_dir_inode_operations;
 	inode->i_fop = &simple_dir_operations;
 
-- 
2.9.3

Re: [PATCH v2] inode: debugfs_create_dir uses mode permission from parent

[Greg KH]

I'm going to add Kees and the kernel-hardning list here, as I'd like
their opinions for the patch below.

Kees, do you have any problems with this patch?  I know you worked on
making debugfs more "secure" from non-root users, this should still keep
the intial mount permissions all fine, right?  Anything I'm not
considering here?

thanks,

greg k-h

On Fri, Apr 27, 2018 at 02:35:47PM +0200, Thomas Richter wrote:
> Currently function debugfs_create_dir() creates a new
> directory in the debugfs (usually mounted /sys/kernel/debug)
> with permission rwxr-xr-x. This is hard coded.
> 
> Change this to use the parent directory permission.
> 
> Output before the patch:
> root@s8360047 ~]# tree -dp -L 1 /sys/kernel/debug/
> /sys/kernel/debug/
> ├── [drwxr-xr-x]  bdi
> ├── [drwxr-xr-x]  block
> ├── [drwxr-xr-x]  dasd
> ├── [drwxr-xr-x]  device_component
> ├── [drwxr-xr-x]  extfrag
> ├── [drwxr-xr-x]  hid
> ├── [drwxr-xr-x]  kprobes
> ├── [drwxr-xr-x]  kvm
> ├── [drwxr-xr-x]  memblock
> ├── [drwxr-xr-x]  pm_qos
> ├── [drwxr-xr-x]  qdio
> ├── [drwxr-xr-x]  s390
> ├── [drwxr-xr-x]  s390dbf
> └── [drwx------]  tracing
> 
> 14 directories
> [root@s8360047 linux]#
> 
> Output after the patch:
> [root@s8360047 ~]# tree -dp -L 1 /sys/kernel/debug/
> sys/kernel/debug/
> ├── [drwx------]  bdi
> ├── [drwx------]  block
> ├── [drwx------]  dasd
> ├── [drwx------]  device_component
> ├── [drwx------]  extfrag
> ├── [drwx------]  hid
> ├── [drwx------]  kprobes
> ├── [drwx------]  kvm
> ├── [drwx------]  memblock
> ├── [drwx------]  pm_qos
> ├── [drwx------]  qdio
> ├── [drwx------]  s390
> ├── [drwx------]  s390dbf
> └── [drwx------]  tracing
> 
> 14 directories
> [root@s8360047 linux]#
> 
> Here is the full diff output done with:
> [root@s8360047 ~]# diff -u treefull.before treefull.after |
> 	sed 's-^- # -' > treefull.diff
>  # --- treefull.before	2018-04-27 13:22:04.532824564 +0200
>  # +++ treefull.after	2018-04-27 13:24:12.106182062 +0200
>  # @@ -1,55 +1,55 @@
>  #  /sys/kernel/debug/
>  # -├── [drwxr-xr-x]  bdi
>  # -│   ├── [drwxr-xr-x]  1:0
>  # -│   ├── [drwxr-xr-x]  1:1
>  # -│   ├── [drwxr-xr-x]  1:10
>  # -│   ├── [drwxr-xr-x]  1:11
>  # -│   ├── [drwxr-xr-x]  1:12
>  # -│   ├── [drwxr-xr-x]  1:13
>  # -│   ├── [drwxr-xr-x]  1:14
>  # -│   ├── [drwxr-xr-x]  1:15
>  # -│   ├── [drwxr-xr-x]  1:2
>  # -│   ├── [drwxr-xr-x]  1:3
>  # -│   ├── [drwxr-xr-x]  1:4
>  # -│   ├── [drwxr-xr-x]  1:5
>  # -│   ├── [drwxr-xr-x]  1:6
>  # -│   ├── [drwxr-xr-x]  1:7
>  # -│   ├── [drwxr-xr-x]  1:8
>  # -│   ├── [drwxr-xr-x]  1:9
>  # -│   └── [drwxr-xr-x]  94:0
>  # -├── [drwxr-xr-x]  block
>  # -├── [drwxr-xr-x]  dasd
>  # -│   ├── [drwxr-xr-x]  0.0.e18a
>  # -│   ├── [drwxr-xr-x]  dasda
>  # -│   └── [drwxr-xr-x]  global
>  # -├── [drwxr-xr-x]  device_component
>  # -├── [drwxr-xr-x]  extfrag
>  # -├── [drwxr-xr-x]  hid
>  # -├── [drwxr-xr-x]  kprobes
>  # -├── [drwxr-xr-x]  kvm
>  # -├── [drwxr-xr-x]  memblock
>  # -├── [drwxr-xr-x]  pm_qos
>  # -├── [drwxr-xr-x]  qdio
>  # -│   └── [drwxr-xr-x]  0.0.f5f2
>  # -├── [drwxr-xr-x]  s390
>  # -│   └── [drwxr-xr-x]  stsi
>  # -├── [drwxr-xr-x]  s390dbf
>  # -│   ├── [drwxr-xr-x]  0.0.e18a
>  # -│   ├── [drwxr-xr-x]  cio_crw
>  # -│   ├── [drwxr-xr-x]  cio_msg
>  # -│   ├── [drwxr-xr-x]  cio_trace
>  # -│   ├── [drwxr-xr-x]  dasd
>  # -│   ├── [drwxr-xr-x]  kvm-trace
>  # -│   ├── [drwxr-xr-x]  lgr
>  # -│   ├── [drwxr-xr-x]  qdio_0.0.f5f2
>  # -│   ├── [drwxr-xr-x]  qdio_error
>  # -│   ├── [drwxr-xr-x]  qdio_setup
>  # -│   ├── [drwxr-xr-x]  qeth_card_0.0.f5f0
>  # -│   ├── [drwxr-xr-x]  qeth_control
>  # -│   ├── [drwxr-xr-x]  qeth_msg
>  # -│   ├── [drwxr-xr-x]  qeth_setup
>  # -│   ├── [drwxr-xr-x]  vmcp
>  # -│   └── [drwxr-xr-x]  vmur
>  # +├── [drwx------]  bdi
>  # +│   ├── [drwx------]  1:0
>  # +│   ├── [drwx------]  1:1
>  # +│   ├── [drwx------]  1:10
>  # +│   ├── [drwx------]  1:11
>  # +│   ├── [drwx------]  1:12
>  # +│   ├── [drwx------]  1:13
>  # +│   ├── [drwx------]  1:14
>  # +│   ├── [drwx------]  1:15
>  # +│   ├── [drwx------]  1:2
>  # +│   ├── [drwx------]  1:3
>  # +│   ├── [drwx------]  1:4
>  # +│   ├── [drwx------]  1:5
>  # +│   ├── [drwx------]  1:6
>  # +│   ├── [drwx------]  1:7
>  # +│   ├── [drwx------]  1:8
>  # +│   ├── [drwx------]  1:9
>  # +│   └── [drwx------]  94:0
>  # +├── [drwx------]  block
>  # +├── [drwx------]  dasd
>  # +│   ├── [drwx------]  0.0.e18a
>  # +│   ├── [drwx------]  dasda
>  # +│   └── [drwx------]  global
>  # +├── [drwx------]  device_component
>  # +├── [drwx------]  extfrag
>  # +├── [drwx------]  hid
>  # +├── [drwx------]  kprobes
>  # +├── [drwx------]  kvm
>  # +├── [drwx------]  memblock
>  # +├── [drwx------]  pm_qos
>  # +├── [drwx------]  qdio
>  # +│   └── [drwx------]  0.0.f5f2
>  # +├── [drwx------]  s390
>  # +│   └── [drwx------]  stsi
>  # +├── [drwx------]  s390dbf
>  # +│   ├── [drwx------]  0.0.e18a
>  # +│   ├── [drwx------]  cio_crw
>  # +│   ├── [drwx------]  cio_msg
>  # +│   ├── [drwx------]  cio_trace
>  # +│   ├── [drwx------]  dasd
>  # +│   ├── [drwx------]  kvm-trace
>  # +│   ├── [drwx------]  lgr
>  # +│   ├── [drwx------]  qdio_0.0.f5f2
>  # +│   ├── [drwx------]  qdio_error
>  # +│   ├── [drwx------]  qdio_setup
>  # +│   ├── [drwx------]  qeth_card_0.0.f5f0
>  # +│   ├── [drwx------]  qeth_control
>  # +│   ├── [drwx------]  qeth_msg
>  # +│   ├── [drwx------]  qeth_setup
>  # +│   ├── [drwx------]  vmcp
>  # +│   └── [drwx------]  vmur
>  #  └── [drwx------]  tracing
>  #      ├── [drwxr-xr-x]  events
>  #      │   ├── [drwxr-xr-x]  alarmtimer
> 
> Fixes: edac65eaf8d5c ("debugfs: take mode-dependent parts of debugfs_get_inode() into callers")
> Signed-off-by: Thomas Richter <tmricht@linux.ibm.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>  fs/debugfs/inode.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
> index 13b0135..a913b12 100644
> --- a/fs/debugfs/inode.c
> +++ b/fs/debugfs/inode.c
> @@ -512,7 +512,9 @@ struct dentry *debugfs_create_dir(const char *name, struct dentry *parent)
>  	if (unlikely(!inode))
>  		return failed_creating(dentry);
>  
> -	inode->i_mode = S_IFDIR | S_IRWXU | S_IRUGO | S_IXUGO;
> +	if (!parent)
> +		parent = debugfs_mount->mnt_root;
> +	inode->i_mode = S_IFDIR | ((d_inode(parent)->i_mode & 0770));
>  	inode->i_op = &simple_dir_inode_operations;
>  	inode->i_fop = &simple_dir_operations;
>  
> -- 
> 2.9.3

Re: [PATCH v2] inode: debugfs_create_dir uses mode permission from parent

[Kees Cook]

On Fri, Apr 27, 2018 at 6:49 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> I'm going to add Kees and the kernel-hardning list here, as I'd like
> their opinions for the patch below.
>
> Kees, do you have any problems with this patch?  I know you worked on
> making debugfs more "secure" from non-root users, this should still keep
> the intial mount permissions all fine, right?  Anything I'm not
> considering here?

This appears correct to me. I'd like to see some stronger rationale
for why this is needed, just so I have a "design" to compare the
implementation against. :)

Normally, the top-level directory permissions should block all the
subdirectories too. The only time I think of this being needed is if
someone is explicitly bind-mounting a subdirectory to another location
(e.g. Chrome OS does this for the i915 subdirectory). In that case,
I'd expect them to tweak permissions too. Thomas, what's your
use-case?

-Kees

>
> thanks,
>
> greg k-h
>
> On Fri, Apr 27, 2018 at 02:35:47PM +0200, Thomas Richter wrote:
>> Currently function debugfs_create_dir() creates a new
>> directory in the debugfs (usually mounted /sys/kernel/debug)
>> with permission rwxr-xr-x. This is hard coded.
>>
>> Change this to use the parent directory permission.
>>
>> Output before the patch:
>> root@s8360047 ~]# tree -dp -L 1 /sys/kernel/debug/
>> /sys/kernel/debug/
>> ├── [drwxr-xr-x]  bdi
>> ├── [drwxr-xr-x]  block
>> ├── [drwxr-xr-x]  dasd
>> ├── [drwxr-xr-x]  device_component
>> ├── [drwxr-xr-x]  extfrag
>> ├── [drwxr-xr-x]  hid
>> ├── [drwxr-xr-x]  kprobes
>> ├── [drwxr-xr-x]  kvm
>> ├── [drwxr-xr-x]  memblock
>> ├── [drwxr-xr-x]  pm_qos
>> ├── [drwxr-xr-x]  qdio
>> ├── [drwxr-xr-x]  s390
>> ├── [drwxr-xr-x]  s390dbf
>> └── [drwx------]  tracing
>>
>> 14 directories
>> [root@s8360047 linux]#
>>
>> Output after the patch:
>> [root@s8360047 ~]# tree -dp -L 1 /sys/kernel/debug/
>> sys/kernel/debug/
>> ├── [drwx------]  bdi
>> ├── [drwx------]  block
>> ├── [drwx------]  dasd
>> ├── [drwx------]  device_component
>> ├── [drwx------]  extfrag
>> ├── [drwx------]  hid
>> ├── [drwx------]  kprobes
>> ├── [drwx------]  kvm
>> ├── [drwx------]  memblock
>> ├── [drwx------]  pm_qos
>> ├── [drwx------]  qdio
>> ├── [drwx------]  s390
>> ├── [drwx------]  s390dbf
>> └── [drwx------]  tracing
>>
>> 14 directories
>> [root@s8360047 linux]#
>>
>> Here is the full diff output done with:
>> [root@s8360047 ~]# diff -u treefull.before treefull.after |
>>       sed 's-^- # -' > treefull.diff
>>  # --- treefull.before        2018-04-27 13:22:04.532824564 +0200
>>  # +++ treefull.after 2018-04-27 13:24:12.106182062 +0200
>>  # @@ -1,55 +1,55 @@
>>  #  /sys/kernel/debug/
>>  # -├── [drwxr-xr-x]  bdi
>>  # -│   ├── [drwxr-xr-x]  1:0
>>  # -│   ├── [drwxr-xr-x]  1:1
>>  # -│   ├── [drwxr-xr-x]  1:10
>>  # -│   ├── [drwxr-xr-x]  1:11
>>  # -│   ├── [drwxr-xr-x]  1:12
>>  # -│   ├── [drwxr-xr-x]  1:13
>>  # -│   ├── [drwxr-xr-x]  1:14
>>  # -│   ├── [drwxr-xr-x]  1:15
>>  # -│   ├── [drwxr-xr-x]  1:2
>>  # -│   ├── [drwxr-xr-x]  1:3
>>  # -│   ├── [drwxr-xr-x]  1:4
>>  # -│   ├── [drwxr-xr-x]  1:5
>>  # -│   ├── [drwxr-xr-x]  1:6
>>  # -│   ├── [drwxr-xr-x]  1:7
>>  # -│   ├── [drwxr-xr-x]  1:8
>>  # -│   ├── [drwxr-xr-x]  1:9
>>  # -│   └── [drwxr-xr-x]  94:0
>>  # -├── [drwxr-xr-x]  block
>>  # -├── [drwxr-xr-x]  dasd
>>  # -│   ├── [drwxr-xr-x]  0.0.e18a
>>  # -│   ├── [drwxr-xr-x]  dasda
>>  # -│   └── [drwxr-xr-x]  global
>>  # -├── [drwxr-xr-x]  device_component
>>  # -├── [drwxr-xr-x]  extfrag
>>  # -├── [drwxr-xr-x]  hid
>>  # -├── [drwxr-xr-x]  kprobes
>>  # -├── [drwxr-xr-x]  kvm
>>  # -├── [drwxr-xr-x]  memblock
>>  # -├── [drwxr-xr-x]  pm_qos
>>  # -├── [drwxr-xr-x]  qdio
>>  # -│   └── [drwxr-xr-x]  0.0.f5f2
>>  # -├── [drwxr-xr-x]  s390
>>  # -│   └── [drwxr-xr-x]  stsi
>>  # -├── [drwxr-xr-x]  s390dbf
>>  # -│   ├── [drwxr-xr-x]  0.0.e18a
>>  # -│   ├── [drwxr-xr-x]  cio_crw
>>  # -│   ├── [drwxr-xr-x]  cio_msg
>>  # -│   ├── [drwxr-xr-x]  cio_trace
>>  # -│   ├── [drwxr-xr-x]  dasd
>>  # -│   ├── [drwxr-xr-x]  kvm-trace
>>  # -│   ├── [drwxr-xr-x]  lgr
>>  # -│   ├── [drwxr-xr-x]  qdio_0.0.f5f2
>>  # -│   ├── [drwxr-xr-x]  qdio_error
>>  # -│   ├── [drwxr-xr-x]  qdio_setup
>>  # -│   ├── [drwxr-xr-x]  qeth_card_0.0.f5f0
>>  # -│   ├── [drwxr-xr-x]  qeth_control
>>  # -│   ├── [drwxr-xr-x]  qeth_msg
>>  # -│   ├── [drwxr-xr-x]  qeth_setup
>>  # -│   ├── [drwxr-xr-x]  vmcp
>>  # -│   └── [drwxr-xr-x]  vmur
>>  # +├── [drwx------]  bdi
>>  # +│   ├── [drwx------]  1:0
>>  # +│   ├── [drwx------]  1:1
>>  # +│   ├── [drwx------]  1:10
>>  # +│   ├── [drwx------]  1:11
>>  # +│   ├── [drwx------]  1:12
>>  # +│   ├── [drwx------]  1:13
>>  # +│   ├── [drwx------]  1:14
>>  # +│   ├── [drwx------]  1:15
>>  # +│   ├── [drwx------]  1:2
>>  # +│   ├── [drwx------]  1:3
>>  # +│   ├── [drwx------]  1:4
>>  # +│   ├── [drwx------]  1:5
>>  # +│   ├── [drwx------]  1:6
>>  # +│   ├── [drwx------]  1:7
>>  # +│   ├── [drwx------]  1:8
>>  # +│   ├── [drwx------]  1:9
>>  # +│   └── [drwx------]  94:0
>>  # +├── [drwx------]  block
>>  # +├── [drwx------]  dasd
>>  # +│   ├── [drwx------]  0.0.e18a
>>  # +│   ├── [drwx------]  dasda
>>  # +│   └── [drwx------]  global
>>  # +├── [drwx------]  device_component
>>  # +├── [drwx------]  extfrag
>>  # +├── [drwx------]  hid
>>  # +├── [drwx------]  kprobes
>>  # +├── [drwx------]  kvm
>>  # +├── [drwx------]  memblock
>>  # +├── [drwx------]  pm_qos
>>  # +├── [drwx------]  qdio
>>  # +│   └── [drwx------]  0.0.f5f2
>>  # +├── [drwx------]  s390
>>  # +│   └── [drwx------]  stsi
>>  # +├── [drwx------]  s390dbf
>>  # +│   ├── [drwx------]  0.0.e18a
>>  # +│   ├── [drwx------]  cio_crw
>>  # +│   ├── [drwx------]  cio_msg
>>  # +│   ├── [drwx------]  cio_trace
>>  # +│   ├── [drwx------]  dasd
>>  # +│   ├── [drwx------]  kvm-trace
>>  # +│   ├── [drwx------]  lgr
>>  # +│   ├── [drwx------]  qdio_0.0.f5f2
>>  # +│   ├── [drwx------]  qdio_error
>>  # +│   ├── [drwx------]  qdio_setup
>>  # +│   ├── [drwx------]  qeth_card_0.0.f5f0
>>  # +│   ├── [drwx------]  qeth_control
>>  # +│   ├── [drwx------]  qeth_msg
>>  # +│   ├── [drwx------]  qeth_setup
>>  # +│   ├── [drwx------]  vmcp
>>  # +│   └── [drwx------]  vmur
>>  #  └── [drwx------]  tracing
>>  #      ├── [drwxr-xr-x]  events
>>  #      │   ├── [drwxr-xr-x]  alarmtimer
>>
>> Fixes: edac65eaf8d5c ("debugfs: take mode-dependent parts of debugfs_get_inode() into callers")
>> Signed-off-by: Thomas Richter <tmricht@linux.ibm.com>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> ---
>>  fs/debugfs/inode.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
>> index 13b0135..a913b12 100644
>> --- a/fs/debugfs/inode.c
>> +++ b/fs/debugfs/inode.c
>> @@ -512,7 +512,9 @@ struct dentry *debugfs_create_dir(const char *name, struct dentry *parent)
>>       if (unlikely(!inode))
>>               return failed_creating(dentry);
>>
>> -     inode->i_mode = S_IFDIR | S_IRWXU | S_IRUGO | S_IXUGO;
>> +     if (!parent)
>> +             parent = debugfs_mount->mnt_root;
>> +     inode->i_mode = S_IFDIR | ((d_inode(parent)->i_mode & 0770));
>>       inode->i_op = &simple_dir_inode_operations;
>>       inode->i_fop = &simple_dir_operations;
>>
>> --
>> 2.9.3



-- 
Kees Cook
Pixel Security

Re: [PATCH v2] inode: debugfs_create_dir uses mode permission from parent

[Jann Horn]

On Fri, Apr 27, 2018 at 3:49 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> I'm going to add Kees and the kernel-hardning list here, as I'd like
> their opinions for the patch below.
>
> Kees, do you have any problems with this patch?  I know you worked on
> making debugfs more "secure" from non-root users, this should still keep
> the intial mount permissions all fine, right?  Anything I'm not
> considering here?
>
> thanks,
>
> greg k-h
>
> On Fri, Apr 27, 2018 at 02:35:47PM +0200, Thomas Richter wrote:
>> Currently function debugfs_create_dir() creates a new
>> directory in the debugfs (usually mounted /sys/kernel/debug)
>> with permission rwxr-xr-x. This is hard coded.
>>
>> Change this to use the parent directory permission.

AFAICS no inodes in debugfs have handlers for the ->rename, ->mkdir,
->create inode ops. What is write permission on debugfs directories
useful for?

Re: [PATCH v2] inode: debugfs_create_dir uses mode permission from parent

[Greg KH]

On Mon, Apr 30, 2018 at 04:15:58PM +0200, Jann Horn wrote:
> On Fri, Apr 27, 2018 at 3:49 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> > I'm going to add Kees and the kernel-hardning list here, as I'd like
> > their opinions for the patch below.
> >
> > Kees, do you have any problems with this patch?  I know you worked on
> > making debugfs more "secure" from non-root users, this should still keep
> > the intial mount permissions all fine, right?  Anything I'm not
> > considering here?
> >
> > thanks,
> >
> > greg k-h
> >
> > On Fri, Apr 27, 2018 at 02:35:47PM +0200, Thomas Richter wrote:
> >> Currently function debugfs_create_dir() creates a new
> >> directory in the debugfs (usually mounted /sys/kernel/debug)
> >> with permission rwxr-xr-x. This is hard coded.
> >>
> >> Change this to use the parent directory permission.
> 
> AFAICS no inodes in debugfs have handlers for the ->rename, ->mkdir,
> ->create inode ops. What is write permission on debugfs directories
> useful for?

I doubt anything :)

Re: [PATCH v2] inode: debugfs_create_dir uses mode permission from parent

[Thomas-Mich Richter]

On 04/27/2018 04:58 PM, Kees Cook wrote:
> On Fri, Apr 27, 2018 at 6:49 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
>> I'm going to add Kees and the kernel-hardning list here, as I'd like
>> their opinions for the patch below.
>>
>> Kees, do you have any problems with this patch?  I know you worked on
>> making debugfs more "secure" from non-root users, this should still keep
>> the intial mount permissions all fine, right?  Anything I'm not
>> considering here?
> 
> This appears correct to me. I'd like to see some stronger rationale
> for why this is needed, just so I have a "design" to compare the
> implementation against. :)
> 
> Normally, the top-level directory permissions should block all the
> subdirectories too. The only time I think of this being needed is if
> someone is explicitly bind-mounting a subdirectory to another location
> (e.g. Chrome OS does this for the i915 subdirectory). In that case,
> I'd expect them to tweak permissions too. Thomas, what's your
> use-case?
> 
> -Kees
> 

There is no 'real use case'. I wrote the patch because of discussions
regarding file permissions for files located deeply in the
directory tree, for example

  -r--r--r-- 1 root root 0 Apr 27 14:23 /sys/kernel/debug/kprobes/blacklist

which gives the impression it is world readable.
This happened to me in recent discussions when I wrote patches to fix some
of the address randomized output of /sys files which broke the perf tool.

During discussion people often forgot that the root /sys/kernel/debug is rwx for
root only and blocks non root access to subdirectories and files. They simply
looked at the file permissions.

I have not thougth about the bind-mount case nor did I test this scenario.

-- 
Thomas Richter, Dept 3303, IBM s390 Linux Development, Boeblingen, Germany
--
Vorsitzende des Aufsichtsrats: Martina Koederitz 
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen / Registergericht: Amtsgericht Stuttgart, HRB 243294

Re: [PATCH v2] inode: debugfs_create_dir uses mode permission from parent

[Kees Cook]

On Wed, May 2, 2018 at 12:16 AM, Thomas-Mich Richter
<tmricht@linux.ibm.com> wrote:
> On 04/27/2018 04:58 PM, Kees Cook wrote:
>> On Fri, Apr 27, 2018 at 6:49 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
>>> I'm going to add Kees and the kernel-hardning list here, as I'd like
>>> their opinions for the patch below.
>>>
>>> Kees, do you have any problems with this patch?  I know you worked on
>>> making debugfs more "secure" from non-root users, this should still keep
>>> the intial mount permissions all fine, right?  Anything I'm not
>>> considering here?
>>
>> This appears correct to me. I'd like to see some stronger rationale
>> for why this is needed, just so I have a "design" to compare the
>> implementation against. :)
>>
>> Normally, the top-level directory permissions should block all the
>> subdirectories too. The only time I think of this being needed is if
>> someone is explicitly bind-mounting a subdirectory to another location
>> (e.g. Chrome OS does this for the i915 subdirectory). In that case,
>> I'd expect them to tweak permissions too. Thomas, what's your
>> use-case?
>>
>> -Kees
>>
>
> There is no 'real use case'. I wrote the patch because of discussions
> regarding file permissions for files located deeply in the
> directory tree, for example
>
>   -r--r--r-- 1 root root 0 Apr 27 14:23 /sys/kernel/debug/kprobes/blacklist
>
> which gives the impression it is world readable.
> This happened to me in recent discussions when I wrote patches to fix some
> of the address randomized output of /sys files which broke the perf tool.
>
> During discussion people often forgot that the root /sys/kernel/debug is rwx for
> root only and blocks non root access to subdirectories and files. They simply
> looked at the file permissions.

Okay, sounds good. "Make permissions less surprising" is a perfectly
good reason to make the change. :)

> I have not thought about the bind-mount case nor did I test this scenario.

I think this case is fine too. Anyone doing this is likely doing some
pretty special things with permissions already.

So, FWIW:

Reviewed-by: Kees Cook <keescook@chromium.org>

-Kees

-- 
Kees Cook
Pixel Security