From mboxrd@z Thu Jan 1 00:00:00 1970 Date: Fri, 27 Apr 2018 15:49:37 +0200 From: Greg KH Subject: Re: [PATCH v2] inode: debugfs_create_dir uses mode permission from parent Message-ID: <20180427134936.GA31171@kroah.com> References: <20180427123547.15727-1-tmricht@linux.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20180427123547.15727-1-tmricht@linux.ibm.com> To: Kees Cook , Thomas Richter Cc: kernel-hardening@lists.openwall.com, brueckner@linux.vnet.ibm.com, schwidefsky@de.ibm.com, heiko.carstens@de.ibm.com, linux-kernel@vger.kernel.org List-ID: 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 > Cc: Greg Kroah-Hartman > --- > 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