Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Fri, 27 Apr 2018 07:58:35 -0700
From: Kees Cook <keescook@...omium.org>
To: Greg KH <gregkh@...uxfoundation.org>
Cc: Thomas Richter <tmricht@...ux.ibm.com>, 
	Kernel Hardening <kernel-hardening@...ts.openwall.com>, brueckner@...ux.vnet.ibm.com, 
	Martin Schwidefsky <schwidefsky@...ibm.com>, Heiko Carstens <heiko.carstens@...ibm.com>, 
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] inode: debugfs_create_dir uses mode permission from parent

On Fri, Apr 27, 2018 at 6:49 AM, Greg KH <gregkh@...uxfoundation.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@...60047 ~]# 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@...60047 linux]#
>>
>> Output after the patch:
>> [root@...60047 ~]# 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@...60047 linux]#
>>
>> Here is the full diff output done with:
>> [root@...60047 ~]# 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@...ux.ibm.com>
>> Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.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

Powered by blists - more mailing lists

Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.