* [PATCH v2] debugfs: inode: debugfs_create_dir uses mode permission from parent @ 2018-04-27 12:35 Thomas Richter 2018-04-27 13:49 ` [PATCH v2] " Greg KH 0 siblings, 1 reply; 7+ messages in thread From: Thomas Richter @ 2018-04-27 12:35 UTC (permalink / raw) To: gregkh Cc: brueckner, schwidefsky, heiko.carstens, linux-kernel, 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] inode: debugfs_create_dir uses mode permission from parent 2018-04-27 12:35 [PATCH v2] debugfs: inode: debugfs_create_dir uses mode permission from parent Thomas Richter @ 2018-04-27 13:49 ` Greg KH 2018-04-27 14:58 ` Kees Cook 2018-04-30 14:15 ` Jann Horn 0 siblings, 2 replies; 7+ messages in thread From: Greg KH @ 2018-04-27 13:49 UTC (permalink / raw) To: Kees Cook, Thomas Richter Cc: kernel-hardening, brueckner, schwidefsky, heiko.carstens, linux-kernel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] inode: debugfs_create_dir uses mode permission from parent 2018-04-27 13:49 ` [PATCH v2] " Greg KH @ 2018-04-27 14:58 ` Kees Cook 2018-05-02 7:16 ` Thomas-Mich Richter 2018-04-30 14:15 ` Jann Horn 1 sibling, 1 reply; 7+ messages in thread From: Kees Cook @ 2018-04-27 14:58 UTC (permalink / raw) To: Greg KH Cc: Thomas Richter, Kernel Hardening, brueckner, Martin Schwidefsky, Heiko Carstens, LKML 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] inode: debugfs_create_dir uses mode permission from parent 2018-04-27 14:58 ` Kees Cook @ 2018-05-02 7:16 ` Thomas-Mich Richter 2018-05-02 14:29 ` Kees Cook 0 siblings, 1 reply; 7+ messages in thread From: Thomas-Mich Richter @ 2018-05-02 7:16 UTC (permalink / raw) To: Kees Cook, Greg KH Cc: Kernel Hardening, brueckner, Martin Schwidefsky, Heiko Carstens, LKML 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] inode: debugfs_create_dir uses mode permission from parent 2018-05-02 7:16 ` Thomas-Mich Richter @ 2018-05-02 14:29 ` Kees Cook 0 siblings, 0 replies; 7+ messages in thread From: Kees Cook @ 2018-05-02 14:29 UTC (permalink / raw) To: Thomas-Mich Richter Cc: Greg KH, Kernel Hardening, brueckner, Martin Schwidefsky, Heiko Carstens, LKML 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] inode: debugfs_create_dir uses mode permission from parent 2018-04-27 13:49 ` [PATCH v2] " Greg KH 2018-04-27 14:58 ` Kees Cook @ 2018-04-30 14:15 ` Jann Horn 2018-04-30 14:26 ` Greg KH 1 sibling, 1 reply; 7+ messages in thread From: Jann Horn @ 2018-04-30 14:15 UTC (permalink / raw) To: Greg KH Cc: Kees Cook, Thomas Richter, Kernel Hardening, brueckner, Martin Schwidefsky, Heiko Carstens, kernel list 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? ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] inode: debugfs_create_dir uses mode permission from parent 2018-04-30 14:15 ` Jann Horn @ 2018-04-30 14:26 ` Greg KH 0 siblings, 0 replies; 7+ messages in thread From: Greg KH @ 2018-04-30 14:26 UTC (permalink / raw) To: Jann Horn Cc: Kees Cook, Thomas Richter, Kernel Hardening, brueckner, Martin Schwidefsky, Heiko Carstens, kernel list 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 :) ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-05-02 14:29 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-04-27 12:35 [PATCH v2] debugfs: inode: debugfs_create_dir uses mode permission from parent Thomas Richter 2018-04-27 13:49 ` [PATCH v2] " Greg KH 2018-04-27 14:58 ` Kees Cook 2018-05-02 7:16 ` Thomas-Mich Richter 2018-05-02 14:29 ` Kees Cook 2018-04-30 14:15 ` Jann Horn 2018-04-30 14:26 ` Greg KH
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).