debugfs: inode: debugfs_create_dir uses mode permission from parent

PATCH In review Thomas Richter 4 participants 7 messages Apr 27, 2018 – May 2, 2018 Driver core

Share & tools

Thomas Richter

author

2018-04-27 12:35 UTC #0 lore raw

PATCH v2 1/1 debugfs: inode: debugfs_create_dir uses mode permission from parent

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 <[email protected]>

Reviewed-by Kees Cook <[email protected]>

---
 fs/debugfs/inode.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

fs/debugfs/inode.c +3 -1

index 13b0135..a913b12 100644
@@ -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;

Signature · 1 line

2.9.3

Greg KH

maintainer

2018-04-27 13:49 UTC #1 lore raw

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

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

Quoted 185 lines · 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 <[email protected]>

Cc Greg Kroah-Hartman <[email protected]>

---
 fs/debugfs/inode.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

fs/debugfs/inode.c +3 -1

index 13b0135..a913b12 100644
@@ -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;

Signature · 1 line

2.9.3

Kees Cook

2018-04-27 14:58 UTC #2 lore raw

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

Quoted 7 lines · 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?

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

Quoted 191 lines

thanks,

greg k-h

Quoted 185 lines · 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 <[email protected]>

Cc Greg Kroah-Hartman <[email protected]>

---
 fs/debugfs/inode.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

fs/debugfs/inode.c +3 -2

index 13b0135..a913b12 100644
@@ -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

Signature · 2 lines

Kees Cook
Pixel Security

Thomas-Mich Richter

author

2018-05-02 07:16 UTC #3 lore raw

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

Quoted 13 lines · Kees Cook

Quoted 7 lines · 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?
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. :)

Quoted 9 lines

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.

Signature · 5 lines

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

Kees Cook

Reviewed-by

2018-05-02 14:29 UTC #4 lore raw

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

Quoted 30 lines · Thomas-Mich Richter

Quoted 13 lines · Kees Cook
Quoted 7 lines · 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?
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. :)
Quoted 9 lines
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

Quoted 7 lines

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. :)

Quoted 1 line

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 <[email protected]>

-Kees

Signature · 2 lines

Kees Cook
Pixel Security

Jann Horn

2018-04-30 14:15 UTC #5 lore raw

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

In reply to Greg KH

Quoted 8 lines · 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?

Quoted 10 lines

thanks,
greg k-h
Quoted 5 lines · 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.

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

Greg KH

maintainer

2018-04-30 14:26 UTC #6 lore raw

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

Quoted 18 lines · Jann Horn

Quoted 17 lines · 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
Quoted 4 lines · 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.

Quoted 5 lines

Quoted 1 line
Quoted 1 line
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 :)