LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Kees Cook <keescook@chromium.org>
To: Greg KH <gregkh@linuxfoundation.org>
Cc: Thomas Richter <tmricht@linux.ibm.com>,
	Kernel Hardening <kernel-hardening@lists.openwall.com>,
	brueckner@linux.vnet.ibm.com,
	Martin Schwidefsky <schwidefsky@de.ibm.com>,
	Heiko Carstens <heiko.carstens@de.ibm.com>,
	LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2] inode: debugfs_create_dir uses mode permission from parent
Date: Fri, 27 Apr 2018 07:58:35 -0700	[thread overview]
Message-ID: <CAGXu5j+LK8-g9bg+RNn+-v1LpRC32sTCAUNZr1v0HXquiG9fQw@mail.gmail.com> (raw)
In-Reply-To: <20180427134936.GA31171@kroah.com>

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

  reply	other threads:[~2018-04-27 14:58 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-04-27 12:35 [PATCH v2] debugfs: " Thomas Richter
2018-04-27 13:49 ` [PATCH v2] " Greg KH
2018-04-27 14:58   ` Kees Cook [this message]
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

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAGXu5j+LK8-g9bg+RNn+-v1LpRC32sTCAUNZr1v0HXquiG9fQw@mail.gmail.com \
    --to=keescook@chromium.org \
    --cc=brueckner@linux.vnet.ibm.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=heiko.carstens@de.ibm.com \
    --cc=kernel-hardening@lists.openwall.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=schwidefsky@de.ibm.com \
    --cc=tmricht@linux.ibm.com \
    --subject='Re: [PATCH v2] inode: debugfs_create_dir uses mode permission from parent' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

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