There was no way to specify the mode of the root directory of filesystems
created with simple_fill_super.
Signed-off-by: Kees Cook <[email protected]>
---
drivers/infiniband/hw/ipath/ipath_fs.c | 3 ++-
drivers/infiniband/hw/qib/qib_fs.c | 3 ++-
drivers/xen/xenfs/super.c | 3 ++-
fs/binfmt_misc.c | 3 ++-
fs/debugfs/inode.c | 3 ++-
fs/fuse/control.c | 3 ++-
fs/libfs.c | 4 ++--
fs/nfsd/nfsctl.c | 3 ++-
include/linux/fs.h | 3 ++-
security/inode.c | 3 ++-
security/selinux/selinuxfs.c | 3 ++-
security/smack/smackfs.c | 3 ++-
12 files changed, 24 insertions(+), 13 deletions(-)
diff --git a/drivers/infiniband/hw/ipath/ipath_fs.c b/drivers/infiniband/hw/ipath/ipath_fs.c
index 31ae1b1..991aa4f 100644
--- a/drivers/infiniband/hw/ipath/ipath_fs.c
+++ b/drivers/infiniband/hw/ipath/ipath_fs.c
@@ -336,7 +336,8 @@ static int ipathfs_fill_super(struct super_block *sb, void *data,
{""},
};
- ret = simple_fill_super(sb, IPATHFS_MAGIC, files);
+ ret = simple_fill_super(sb, IPATHFS_MAGIC, files,
+ S_IWUSR | S_IRUGO | S_IXUGO);
if (ret) {
printk(KERN_ERR "simple_fill_super failed: %d\n", ret);
goto bail;
diff --git a/drivers/infiniband/hw/qib/qib_fs.c b/drivers/infiniband/hw/qib/qib_fs.c
index df7fa25..de01b23 100644
--- a/drivers/infiniband/hw/qib/qib_fs.c
+++ b/drivers/infiniband/hw/qib/qib_fs.c
@@ -530,7 +530,8 @@ static int qibfs_fill_super(struct super_block *sb, void *data, int silent)
{""},
};
- ret = simple_fill_super(sb, QIBFS_MAGIC, files);
+ ret = simple_fill_super(sb, QIBFS_MAGIC, files,
+ S_IWUSR | S_IRUGO | S_IXUGO);
if (ret) {
printk(KERN_ERR "simple_fill_super failed: %d\n", ret);
goto bail;
diff --git a/drivers/xen/xenfs/super.c b/drivers/xen/xenfs/super.c
index 1aa3897..d5d65cf 100644
--- a/drivers/xen/xenfs/super.c
+++ b/drivers/xen/xenfs/super.c
@@ -89,7 +89,8 @@ static int xenfs_fill_super(struct super_block *sb, void *data, int silent)
};
int rc;
- rc = simple_fill_super(sb, XENFS_SUPER_MAGIC, xenfs_files);
+ rc = simple_fill_super(sb, XENFS_SUPER_MAGIC, xenfs_files,
+ S_IWUSR | S_IRUGO | S_IXUGO);
if (rc < 0)
return rc;
diff --git a/fs/binfmt_misc.c b/fs/binfmt_misc.c
index 1befe2e..6ad4874 100644
--- a/fs/binfmt_misc.c
+++ b/fs/binfmt_misc.c
@@ -700,7 +700,8 @@ static int bm_fill_super(struct super_block * sb, void * data, int silent)
[3] = {"register", &bm_register_operations, S_IWUSR},
/* last one */ {""}
};
- int err = simple_fill_super(sb, 0x42494e4d, bm_files);
+ int err = simple_fill_super(sb, 0x42494e4d, bm_files,
+ S_IWUSR | S_IRUGO | S_IXUGO);
if (!err)
sb->s_op = &s_ops;
return err;
diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
index 37a8ca7..3cb33c3 100644
--- a/fs/debugfs/inode.c
+++ b/fs/debugfs/inode.c
@@ -132,7 +132,8 @@ static int debug_fill_super(struct super_block *sb, void *data, int silent)
{
static struct tree_descr debug_files[] = {{""}};
- return simple_fill_super(sb, DEBUGFS_MAGIC, debug_files);
+ return simple_fill_super(sb, DEBUGFS_MAGIC, debug_files,
+ S_IWUSR | S_IRUGO | S_IXUGO);
}
static struct dentry *debug_mount(struct file_system_type *fs_type,
diff --git a/fs/fuse/control.c b/fs/fuse/control.c
index 85542a7..80bbb66 100644
--- a/fs/fuse/control.c
+++ b/fs/fuse/control.c
@@ -302,7 +302,8 @@ static int fuse_ctl_fill_super(struct super_block *sb, void *data, int silent)
struct fuse_conn *fc;
int err;
- err = simple_fill_super(sb, FUSE_CTL_SUPER_MAGIC, &empty_descr);
+ err = simple_fill_super(sb, FUSE_CTL_SUPER_MAGIC, &empty_descr,
+ S_IWUSR | S_IRUGO | S_IXUGO);
if (err)
return err;
diff --git a/fs/libfs.c b/fs/libfs.c
index c88eab5..ea4d695 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -463,7 +463,7 @@ int simple_write_end(struct file *file, struct address_space *mapping,
* to pass it an appropriate max_reserved value to avoid collisions.
*/
int simple_fill_super(struct super_block *s, unsigned long magic,
- struct tree_descr *files)
+ struct tree_descr *files, umode_t mode)
{
struct inode *inode;
struct dentry *root;
@@ -484,7 +484,7 @@ int simple_fill_super(struct super_block *s, unsigned long magic,
* entry at index 1
*/
inode->i_ino = 1;
- inode->i_mode = S_IFDIR | 0755;
+ inode->i_mode = (mode & (S_IRWXUGO | S_ISVTX)) | S_IFDIR;
inode->i_atime = inode->i_mtime = inode->i_ctime = CURRENT_TIME;
inode->i_op = &simple_dir_inode_operations;
inode->i_fop = &simple_dir_operations;
diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
index 33b3e2b..709ca56 100644
--- a/fs/nfsd/nfsctl.c
+++ b/fs/nfsd/nfsctl.c
@@ -1404,7 +1404,8 @@ static int nfsd_fill_super(struct super_block * sb, void * data, int silent)
#endif
/* last one */ {""}
};
- return simple_fill_super(sb, 0x6e667364, nfsd_files);
+ return simple_fill_super(sb, 0x6e667364, nfsd_files,
+ S_IWUSR | S_IRUGO | S_IXUGO);
}
static struct dentry *nfsd_mount(struct file_system_type *fs_type,
diff --git a/include/linux/fs.h b/include/linux/fs.h
index bd32159..d4dd31e 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2435,7 +2435,8 @@ extern const struct file_operations simple_dir_operations;
extern const struct inode_operations simple_dir_inode_operations;
struct tree_descr { char *name; const struct file_operations *ops; int mode; };
struct dentry *d_alloc_name(struct dentry *, const char *);
-extern int simple_fill_super(struct super_block *, unsigned long, struct tree_descr *);
+extern int simple_fill_super(struct super_block *, unsigned long,
+ struct tree_descr *, umode_t mode);
extern int simple_pin_fs(struct file_system_type *, struct vfsmount **mount, int *count);
extern void simple_release_fs(struct vfsmount **mount, int *count);
diff --git a/security/inode.c b/security/inode.c
index c4df2fb..d85e416 100644
--- a/security/inode.c
+++ b/security/inode.c
@@ -128,7 +128,8 @@ static int fill_super(struct super_block *sb, void *data, int silent)
{
static struct tree_descr files[] = {{""}};
- return simple_fill_super(sb, SECURITYFS_MAGIC, files);
+ return simple_fill_super(sb, SECURITYFS_MAGIC, files,
+ S_IWUSR | S_IRUGO | S_IXUGO);
}
static struct dentry *get_sb(struct file_system_type *fs_type,
diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
index ea39cb7..26f9c025 100644
--- a/security/selinux/selinuxfs.c
+++ b/security/selinux/selinuxfs.c
@@ -1792,7 +1792,8 @@ static int sel_fill_super(struct super_block *sb, void *data, int silent)
[SEL_POLICY] = {"policy", &sel_policy_ops, S_IRUSR},
/* last one */ {""}
};
- ret = simple_fill_super(sb, SELINUX_MAGIC, selinux_files);
+ ret = simple_fill_super(sb, SELINUX_MAGIC, selinux_files,
+ S_IWUSR | S_IRUGO | S_IXUGO);
if (ret)
goto err;
diff --git a/security/smack/smackfs.c b/security/smack/smackfs.c
index 362d5ed..788fac4 100644
--- a/security/smack/smackfs.c
+++ b/security/smack/smackfs.c
@@ -1323,7 +1323,8 @@ static int smk_fill_super(struct super_block *sb, void *data, int silent)
/* last one */ {""}
};
- rc = simple_fill_super(sb, SMACK_MAGIC, smack_files);
+ rc = simple_fill_super(sb, SMACK_MAGIC, smack_files,
+ S_IWUSR | S_IRUGO | S_IXUGO);
if (rc != 0) {
printk(KERN_ERR "%s failed %d while creating inodes\n",
__func__, rc);
--
1.7.2.3
Block access to the potentially dangerous debugging interfaces in
the debugfs filesystem.
Signed-off-by: Kees Cook <[email protected]>
---
fs/debugfs/inode.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
index 3cb33c3..83c61a3 100644
--- a/fs/debugfs/inode.c
+++ b/fs/debugfs/inode.c
@@ -133,7 +133,7 @@ static int debug_fill_super(struct super_block *sb, void *data, int silent)
static struct tree_descr debug_files[] = {{""}};
return simple_fill_super(sb, DEBUGFS_MAGIC, debug_files,
- S_IWUSR | S_IRUGO | S_IXUGO);
+ S_IRWXU);
}
static struct dentry *debug_mount(struct file_system_type *fs_type,
--
1.7.2.3
Har har, I forgot --compose to "git send-email".
Anyway, with the continuing deluge of bugs in the "debug" filesystem, I
would like to make that filesystem's root directory mode 0700 by default
since it's filled with crazy stuff that regular users do not need to see.
Better to try to just close the door completely on all the stuff in there.
It is, after all, supposed to only be used for debugging, right?
-Kees
On Tue, Feb 22, 2011 at 10:09:58AM -0800, Kees Cook wrote:
> Block access to the potentially dangerous debugging interfaces in
> the debugfs filesystem.
>
> Signed-off-by: Kees Cook <[email protected]>
> ---
> fs/debugfs/inode.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
> index 3cb33c3..83c61a3 100644
> --- a/fs/debugfs/inode.c
> +++ b/fs/debugfs/inode.c
> @@ -133,7 +133,7 @@ static int debug_fill_super(struct super_block *sb, void *data, int silent)
> static struct tree_descr debug_files[] = {{""}};
>
> return simple_fill_super(sb, DEBUGFS_MAGIC, debug_files,
> - S_IWUSR | S_IRUGO | S_IXUGO);
> + S_IRWXU);
> }
>
> static struct dentry *debug_mount(struct file_system_type *fs_type,
> --
> 1.7.2.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
Kees Cook
Ubuntu Security Team
On 02/22/2011 10:16 AM, Kees Cook wrote:
> Har har, I forgot --compose to "git send-email".
>
> Anyway, with the continuing deluge of bugs in the "debug" filesystem, I
> would like to make that filesystem's root directory mode 0700 by default
> since it's filled with crazy stuff that regular users do not need to see.
>
> Better to try to just close the door completely on all the stuff in there.
> It is, after all, supposed to only be used for debugging, right?
>
It depends if you consider use of ftrace and kprobes 'debugging'. In
any event, you really have to be root to be able to manipulate them.
I can currently do 'cat /sys/kernel/debug/tracing/trace' as a normal
user. With your change I don't think it would be possible. This is not
something I often (ever) do, but it is a change.
David Daney
> -Kees
>
> On Tue, Feb 22, 2011 at 10:09:58AM -0800, Kees Cook wrote:
>> Block access to the potentially dangerous debugging interfaces in
>> the debugfs filesystem.
>>
>> Signed-off-by: Kees Cook<[email protected]>
>> ---
>> fs/debugfs/inode.c | 2 +-
>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
>> index 3cb33c3..83c61a3 100644
>> --- a/fs/debugfs/inode.c
>> +++ b/fs/debugfs/inode.c
>> @@ -133,7 +133,7 @@ static int debug_fill_super(struct super_block *sb, void *data, int silent)
>> static struct tree_descr debug_files[] = {{""}};
>>
>> return simple_fill_super(sb, DEBUGFS_MAGIC, debug_files,
>> - S_IWUSR | S_IRUGO | S_IXUGO);
>> + S_IRWXU);
>> }
>>
>> static struct dentry *debug_mount(struct file_system_type *fs_type,
>> --
>> 1.7.2.3
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at http://www.tux.org/lkml/
On Tue, Feb 22, 2011 at 10:32:19AM -0800, David Daney wrote:
> On 02/22/2011 10:16 AM, Kees Cook wrote:
> >Har har, I forgot --compose to "git send-email".
> >
> >Anyway, with the continuing deluge of bugs in the "debug" filesystem, I
> >would like to make that filesystem's root directory mode 0700 by default
> >since it's filled with crazy stuff that regular users do not need to see.
> >
> >Better to try to just close the door completely on all the stuff in there.
> >It is, after all, supposed to only be used for debugging, right?
> >
>
> It depends if you consider use of ftrace and kprobes 'debugging'.
> In any event, you really have to be root to be able to manipulate
> them.
>
> I can currently do 'cat /sys/kernel/debug/tracing/trace' as a normal
> user. With your change I don't think it would be possible. This is
> not something I often (ever) do, but it is a change.
Right, my thinking is that all the manipulations on this tree should be
root-only (non-root stuff shouldn't live in something named "debug"),
so we might as well lock it down a bit more so we can avoid all the CVEs[1]
being assigned for glitches in this tree.
-Kees
[1] http://seclists.org/oss-sec/2011/q1/229
http://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2010-4347
etc
--
Kees Cook
Ubuntu Security Team
On Tue, Feb 22, 2011 at 10:16:13AM -0800, Kees Cook wrote:
> Har har, I forgot --compose to "git send-email".
>
> Anyway, with the continuing deluge of bugs in the "debug" filesystem, I
> would like to make that filesystem's root directory mode 0700 by default
> since it's filled with crazy stuff that regular users do not need to see.
But that will break existing users of this interface, right?
> Better to try to just close the door completely on all the stuff in there.
> It is, after all, supposed to only be used for debugging, right?
No, not really, people use it for all sorts of crazy things.
Remember, the only rule in debugfs is:
There are no rules in debugfs.
thanks,
greg k-h
On Tue, Feb 22, 2011 at 10:47:26AM -0800, Kees Cook wrote:
> On Tue, Feb 22, 2011 at 10:32:19AM -0800, David Daney wrote:
> > On 02/22/2011 10:16 AM, Kees Cook wrote:
> > >Har har, I forgot --compose to "git send-email".
> > >
> > >Anyway, with the continuing deluge of bugs in the "debug" filesystem, I
> > >would like to make that filesystem's root directory mode 0700 by default
> > >since it's filled with crazy stuff that regular users do not need to see.
> > >
> > >Better to try to just close the door completely on all the stuff in there.
> > >It is, after all, supposed to only be used for debugging, right?
> > >
> >
> > It depends if you consider use of ftrace and kprobes 'debugging'.
> > In any event, you really have to be root to be able to manipulate
> > them.
> >
> > I can currently do 'cat /sys/kernel/debug/tracing/trace' as a normal
> > user. With your change I don't think it would be possible. This is
> > not something I often (ever) do, but it is a change.
>
> Right, my thinking is that all the manipulations on this tree should be
> root-only (non-root stuff shouldn't live in something named "debug"),
> so we might as well lock it down a bit more so we can avoid all the CVEs[1]
> being assigned for glitches in this tree.
The CVEs are for non-root writes to debugfs, same thing for sysfs. It's
just stupid mistakes being made here, don't try to lock down the whole
filesystem for just a handfull of bugs.
So I really can't accept these patches, sorry.
thanks,
greg k-h
Hi Greg,
On Tue, Feb 22, 2011 at 11:13:33AM -0800, Greg KH wrote:
> On Tue, Feb 22, 2011 at 10:16:13AM -0800, Kees Cook wrote:
> > Har har, I forgot --compose to "git send-email".
> >
> > Anyway, with the continuing deluge of bugs in the "debug" filesystem, I
> > would like to make that filesystem's root directory mode 0700 by default
> > since it's filled with crazy stuff that regular users do not need to see.
>
> But that will break existing users of this interface, right?
>
> > Better to try to just close the door completely on all the stuff in there.
> > It is, after all, supposed to only be used for debugging, right?
>
> No, not really, people use it for all sorts of crazy things.
>
> Remember, the only rule in debugfs is:
> There are no rules in debugfs.
Right, which seems extremely dangerous to me. I think debugfs should either
be:
a) used only by the root user (would prefer this was actually CAP_DEBUG or
something)
or
b) used by userspace tools
But not both. Creating interfaces with no rules for userspace consumption
is going to lead to disaster. You can't disallow "break[ing] existing
users of this interface" and say there are no rules at the same time. :)
-Kees
--
Kees Cook
Ubuntu Security Team
Hi Greg,
On Tue, Feb 22, 2011 at 11:14:54AM -0800, Greg KH wrote:
> On Tue, Feb 22, 2011 at 10:47:26AM -0800, Kees Cook wrote:
> > On Tue, Feb 22, 2011 at 10:32:19AM -0800, David Daney wrote:
> > > On 02/22/2011 10:16 AM, Kees Cook wrote:
> > > >Har har, I forgot --compose to "git send-email".
> > > >
> > > >Anyway, with the continuing deluge of bugs in the "debug" filesystem, I
> > > >would like to make that filesystem's root directory mode 0700 by default
> > > >since it's filled with crazy stuff that regular users do not need to see.
> > > >
> > > >Better to try to just close the door completely on all the stuff in there.
> > > >It is, after all, supposed to only be used for debugging, right?
> > > >
> > >
> > > It depends if you consider use of ftrace and kprobes 'debugging'.
> > > In any event, you really have to be root to be able to manipulate
> > > them.
> > >
> > > I can currently do 'cat /sys/kernel/debug/tracing/trace' as a normal
> > > user. With your change I don't think it would be possible. This is
> > > not something I often (ever) do, but it is a change.
> >
> > Right, my thinking is that all the manipulations on this tree should be
> > root-only (non-root stuff shouldn't live in something named "debug"),
> > so we might as well lock it down a bit more so we can avoid all the CVEs[1]
> > being assigned for glitches in this tree.
>
> The CVEs are for non-root writes to debugfs, same thing for sysfs. It's
> just stupid mistakes being made here, don't try to lock down the whole
> filesystem for just a handfull of bugs.
>
> So I really can't accept these patches, sorry.
What system do you proposed to keep these "stupid mistakes" from
continuing to happen? If debugfs had already been mode 0700, we could have
avoided all of these CVEs, including the full-blown local root escalation.
The "no rules" approach to debugfs is not a good idea, IMO.
-Kees
--
Kees Cook
Ubuntu Security Team
> What system do you proposed to keep these "stupid mistakes" from
> continuing to happen? If debugfs had already been mode 0700, we could have
> avoided all of these CVEs, including the full-blown local root escalation.
And all sorts of features would have put themselves in sysfs instead and
broken no doubt.
> The "no rules" approach to debugfs is not a good idea, IMO.
It's a debugging fs, it needs to be "no rules" other than the obvious
"don't mount it on production systems"
Anyway - we don't mostly have a "root/non-root" model - it went out
around Linux 1.2. So any model for dealing with should be respecting of
capabilities, SELinux and the like.
Or of course you could just chmod it 0700 in the distro !
On Tue, Feb 22, 2011 at 11:22:48AM -0800, Kees Cook wrote:
> On Tue, Feb 22, 2011 at 11:13:33AM -0800, Greg KH wrote:
> > On Tue, Feb 22, 2011 at 10:16:13AM -0800, Kees Cook wrote:
> > > Har har, I forgot --compose to "git send-email".
> > >
> > > Anyway, with the continuing deluge of bugs in the "debug" filesystem, I
> > > would like to make that filesystem's root directory mode 0700 by default
> > > since it's filled with crazy stuff that regular users do not need to see.
> >
> > But that will break existing users of this interface, right?
> >
> > > Better to try to just close the door completely on all the stuff in there.
> > > It is, after all, supposed to only be used for debugging, right?
> >
> > No, not really, people use it for all sorts of crazy things.
> >
> > Remember, the only rule in debugfs is:
> > There are no rules in debugfs.
>
> Right, which seems extremely dangerous to me. I think debugfs should either
> be:
> a) used only by the root user (would prefer this was actually CAP_DEBUG or
> something)
> or
> b) used by userspace tools
>
> But not both. Creating interfaces with no rules for userspace consumption
> is going to lead to disaster. You can't disallow "break[ing] existing
> users of this interface" and say there are no rules at the same time. :)
I'm not going to argue that the wisdom of putting userspace tool
interfaces in debugfs wasn't a bad idea, but that was not for me to
decide, it was the creators of that interface's decision. And even
then, it was something the evolved over time and I doubt that anyone
when it was first created, would have imagined the perf interface being
in debugfs today.
And even then, there's been a proposal to move that to a new filesystem
which would solve that issue.
I will say that you can not break existing users, as I want all users to
have access to the debugfs files I have created in there for a wide
range of valid reasons.
So no, I'm not going to change the whole filesystem to not allow all
users to access it, that is a per-file/directory decision by the creator
of that file/directory.
Again, let's fix the real problems here, world-writable debugfs files.
Nothing different here from sysfs which had the same issues, just that
no one thought to ask for zillions of CVE entries to panic the world
with when we resolved all of them...
thanks,
greg k-h
On Tue, Feb 22, 2011 at 11:25:33AM -0800, Kees Cook wrote:
> Hi Greg,
>
> On Tue, Feb 22, 2011 at 11:14:54AM -0800, Greg KH wrote:
> > On Tue, Feb 22, 2011 at 10:47:26AM -0800, Kees Cook wrote:
> > > On Tue, Feb 22, 2011 at 10:32:19AM -0800, David Daney wrote:
> > > > On 02/22/2011 10:16 AM, Kees Cook wrote:
> > > > >Har har, I forgot --compose to "git send-email".
> > > > >
> > > > >Anyway, with the continuing deluge of bugs in the "debug" filesystem, I
> > > > >would like to make that filesystem's root directory mode 0700 by default
> > > > >since it's filled with crazy stuff that regular users do not need to see.
> > > > >
> > > > >Better to try to just close the door completely on all the stuff in there.
> > > > >It is, after all, supposed to only be used for debugging, right?
> > > > >
> > > >
> > > > It depends if you consider use of ftrace and kprobes 'debugging'.
> > > > In any event, you really have to be root to be able to manipulate
> > > > them.
> > > >
> > > > I can currently do 'cat /sys/kernel/debug/tracing/trace' as a normal
> > > > user. With your change I don't think it would be possible. This is
> > > > not something I often (ever) do, but it is a change.
> > >
> > > Right, my thinking is that all the manipulations on this tree should be
> > > root-only (non-root stuff shouldn't live in something named "debug"),
> > > so we might as well lock it down a bit more so we can avoid all the CVEs[1]
> > > being assigned for glitches in this tree.
> >
> > The CVEs are for non-root writes to debugfs, same thing for sysfs. It's
> > just stupid mistakes being made here, don't try to lock down the whole
> > filesystem for just a handfull of bugs.
> >
> > So I really can't accept these patches, sorry.
>
> What system do you proposed to keep these "stupid mistakes" from
> continuing to happen?
The newly added rule to checkpatch.pl for sysfs files could be extended
to also check for debugfs files with these incorrect permissions.
That's the best thing for all new patches as lots of people run that
tool to find problems.
> If debugfs had already been mode 0700, we could have
> avoided all of these CVEs, including the full-blown local root escalation.
Same could be said for sysfs :)
> The "no rules" approach to debugfs is not a good idea, IMO.
Sorry you don't like it, but that's the way it is.
And as Alan said, you can convince your distro to just not enable it if
you really don't like it, or mount it with this permission.
thanks,
greg k-h
On Tue, Feb 22, 2011 at 07:34:18PM +0000, Alan Cox wrote:
> > What system do you proposed to keep these "stupid mistakes" from
> > continuing to happen? If debugfs had already been mode 0700, we could have
> > avoided all of these CVEs, including the full-blown local root escalation.
>
> And all sorts of features would have put themselves in sysfs instead and
> broken no doubt.
>
> > The "no rules" approach to debugfs is not a good idea, IMO.
>
> It's a debugging fs, it needs to be "no rules" other than the obvious
> "don't mount it on production systems"
Okay, so the debugfs is not supposed to be mounted on a production system.
This seems to be news to a lot of developers trying to use the interfaces
exposed there. It would be nice to say this more loudly. Basically,
a normal system should not depend on anything in the debugfs. I can get
behind that.
> Or of course you could just chmod it 0700 in the distro !
I'm trying to, but that involves a race condition since I can't control it
during the mount (nor set the default mode like I was trying to with the
patchset). tmpfs allows "mode=", but debugfs does not...
-Kees
--
Kees Cook
Ubuntu Security Team
On 02/22/2011 11:50 AM, Kees Cook wrote:
> On Tue, Feb 22, 2011 at 07:34:18PM +0000, Alan Cox wrote:
[...]
>
>> Or of course you could just chmod it 0700 in the distro !
>
> I'm trying to, but that involves a race condition since I can't control it
> during the mount (nor set the default mode like I was trying to with the
> patchset). tmpfs allows "mode=", but debugfs does not...
>
Someone skilled in the black arts of kernel programming might be able to
add 'mode=' support to debugfs.
David Daney
On Tue, Feb 22, 2011 at 07:34:18PM +0000, Alan Cox wrote:
> Anyway - we don't mostly have a "root/non-root" model - it went out
> around Linux 1.2. So any model for dealing with should be respecting of
> capabilities, SELinux and the like.
That reminds me; in this line of reasoning, would a patchset to update all
the /proc/sys, /sys, etc writing functions to take capability lists and check
them be accepted? Because right now, almost everything in /proc/sys uses
DAC instead of checking caps. I think the same is true of /sys and debugfs.
-Kees
--
Kees Cook
Ubuntu Security Team
On Tue, Feb 22, 2011 at 11:50:18AM -0800, Kees Cook wrote:
> On Tue, Feb 22, 2011 at 07:34:18PM +0000, Alan Cox wrote:
> > > What system do you proposed to keep these "stupid mistakes" from
> > > continuing to happen? If debugfs had already been mode 0700, we could have
> > > avoided all of these CVEs, including the full-blown local root escalation.
> >
> > And all sorts of features would have put themselves in sysfs instead and
> > broken no doubt.
> >
> > > The "no rules" approach to debugfs is not a good idea, IMO.
> >
> > It's a debugging fs, it needs to be "no rules" other than the obvious
> > "don't mount it on production systems"
>
> Okay, so the debugfs is not supposed to be mounted on a production system.
No, not true at all, the "enterprise" distros all mount debugfs for good
reason on their systems.
> This seems to be news to a lot of developers trying to use the interfaces
> exposed there. It would be nice to say this more loudly. Basically,
> a normal system should not depend on anything in the debugfs. I can get
> behind that.
Again, not true. Mostly all due to the perf interface, fix that to move
out of debugfs (patches have been proposed) and this problem will go
away.
thanks,
greg k-h
On Tue, Feb 22, 2011 at 12:16:10PM -0800, Greg KH wrote:
> On Tue, Feb 22, 2011 at 11:50:18AM -0800, Kees Cook wrote:
> > On Tue, Feb 22, 2011 at 07:34:18PM +0000, Alan Cox wrote:
> > > > What system do you proposed to keep these "stupid mistakes" from
> > > > continuing to happen? If debugfs had already been mode 0700, we could have
> > > > avoided all of these CVEs, including the full-blown local root escalation.
> > >
> > > And all sorts of features would have put themselves in sysfs instead and
> > > broken no doubt.
> > >
> > > > The "no rules" approach to debugfs is not a good idea, IMO.
> > >
> > > It's a debugging fs, it needs to be "no rules" other than the obvious
> > > "don't mount it on production systems"
> >
> > Okay, so the debugfs is not supposed to be mounted on a production system.
>
> No, not true at all, the "enterprise" distros all mount debugfs for good
> reason on their systems.
What reasons are those? Or better yet, why do you and Alan Cox disagree on
this point?
> > This seems to be news to a lot of developers trying to use the interfaces
> > exposed there. It would be nice to say this more loudly. Basically,
> > a normal system should not depend on anything in the debugfs. I can get
> > behind that.
>
> Again, not true. Mostly all due to the perf interface, fix that to move
> out of debugfs (patches have been proposed) and this problem will go
> away.
You can't have "no rules" and "all distros mount debugfs for good reason".
This is asking for (even more) trouble. If there is something universally
useful in debugfs (I do not count perf as universally useful -- my parents
do not use perf), then why is it living in a filesystem with no rules
(where "no rules" seems to also include "don't break interfaces").
-Kees
--
Kees Cook
Ubuntu Security Team
On Tue, Feb 22, 2011 at 11:33:14AM -0800, Greg KH wrote:
> Again, let's fix the real problems here, world-writable debugfs files.
We could just ban them?
diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
index e7a7a2f..03ae095 100644
--- a/fs/debugfs/inode.c
+++ b/fs/debugfs/inode.c
@@ -223,6 +223,9 @@ struct dentry *debugfs_create_file(const char *name, mode_t mode,
pr_debug("debugfs: creating file '%s'\n",name);
+ /* don't allow world writable files */
+ mode &= ~S_IWOTH;
+
error = simple_pin_fs(&debug_fs_type, &debugfs_mount,
&debugfs_mount_count);
if (error)
On Tue, Feb 22, 2011 at 11:29:15PM +0300, Dan Carpenter wrote:
> On Tue, Feb 22, 2011 at 11:33:14AM -0800, Greg KH wrote:
> > Again, let's fix the real problems here, world-writable debugfs files.
>
> We could just ban them?
That would be nice. This would be much better than relying on
check_patch.pl. Perhaps do this for /sys and /proc/sys too?
Acked-by: Kees Cook <[email protected]>
>
> diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
> index e7a7a2f..03ae095 100644
> --- a/fs/debugfs/inode.c
> +++ b/fs/debugfs/inode.c
> @@ -223,6 +223,9 @@ struct dentry *debugfs_create_file(const char *name, mode_t mode,
>
> pr_debug("debugfs: creating file '%s'\n",name);
>
> + /* don't allow world writable files */
> + mode &= ~S_IWOTH;
> +
> error = simple_pin_fs(&debug_fs_type, &debugfs_mount,
> &debugfs_mount_count);
> if (error)
--
Kees Cook
Ubuntu Security Team
On Tue, Feb 22, 2011 at 12:28:56PM -0800, Kees Cook wrote:
> On Tue, Feb 22, 2011 at 12:16:10PM -0800, Greg KH wrote:
> > On Tue, Feb 22, 2011 at 11:50:18AM -0800, Kees Cook wrote:
> > > On Tue, Feb 22, 2011 at 07:34:18PM +0000, Alan Cox wrote:
> > > > > What system do you proposed to keep these "stupid mistakes" from
> > > > > continuing to happen? If debugfs had already been mode 0700, we could have
> > > > > avoided all of these CVEs, including the full-blown local root escalation.
> > > >
> > > > And all sorts of features would have put themselves in sysfs instead and
> > > > broken no doubt.
> > > >
> > > > > The "no rules" approach to debugfs is not a good idea, IMO.
> > > >
> > > > It's a debugging fs, it needs to be "no rules" other than the obvious
> > > > "don't mount it on production systems"
> > >
> > > Okay, so the debugfs is not supposed to be mounted on a production system.
> >
> > No, not true at all, the "enterprise" distros all mount debugfs for good
> > reason on their systems.
>
> What reasons are those? Or better yet, why do you and Alan Cox disagree on
> this point?
These distros have made the decision to support the perf interface,
which lives in debugfs, for their customers. I'm not saying that I
disagree with Alan about this, just pointing out the reality of the
situation here.
> > > This seems to be news to a lot of developers trying to use the interfaces
> > > exposed there. It would be nice to say this more loudly. Basically,
> > > a normal system should not depend on anything in the debugfs. I can get
> > > behind that.
> >
> > Again, not true. Mostly all due to the perf interface, fix that to move
> > out of debugfs (patches have been proposed) and this problem will go
> > away.
>
> You can't have "no rules" and "all distros mount debugfs for good reason".
> This is asking for (even more) trouble. If there is something universally
> useful in debugfs (I do not count perf as universally useful -- my parents
> do not use perf), then why is it living in a filesystem with no rules
> (where "no rules" seems to also include "don't break interfaces").
Again, "don't break interfaces" is just me saying "don't break the
interfaces I have created in debugfs as they are to be used by all
users." Don't take that as a set-in-stone rule of debugfs at all, it
isn't.
Again, you are trying to exclude a whole range of useful and valid files
from being used, when there was only a very very very small percentage
created incorrectly. They have now been fixed, and we have the
infrastructure to prevent future ones from being created as well, so I
don't see the issue here anymore.
thanks,
greg k-h
On Tue, Feb 22, 2011 at 12:37:04PM -0800, Greg KH wrote:
> On Tue, Feb 22, 2011 at 12:28:56PM -0800, Kees Cook wrote:
> > On Tue, Feb 22, 2011 at 12:16:10PM -0800, Greg KH wrote:
> > > On Tue, Feb 22, 2011 at 11:50:18AM -0800, Kees Cook wrote:
> > > > On Tue, Feb 22, 2011 at 07:34:18PM +0000, Alan Cox wrote:
> > > > > > What system do you proposed to keep these "stupid mistakes" from
> > > > > > continuing to happen? If debugfs had already been mode 0700, we could have
> > > > > > avoided all of these CVEs, including the full-blown local root escalation.
> > > > >
> > > > > And all sorts of features would have put themselves in sysfs instead and
> > > > > broken no doubt.
> > > > >
> > > > > > The "no rules" approach to debugfs is not a good idea, IMO.
> > > > >
> > > > > It's a debugging fs, it needs to be "no rules" other than the obvious
> > > > > "don't mount it on production systems"
> > > >
> > > > Okay, so the debugfs is not supposed to be mounted on a production system.
> > >
> > > No, not true at all, the "enterprise" distros all mount debugfs for good
> > > reason on their systems.
> >
> > What reasons are those? Or better yet, why do you and Alan Cox disagree on
> > this point?
>
> These distros have made the decision to support the perf interface,
> which lives in debugfs, for their customers. I'm not saying that I
> disagree with Alan about this, just pointing out the reality of the
> situation here.
A tool used only by the root user, so the proposed mount mode of 0700
wouldn't break anything.
> > > > This seems to be news to a lot of developers trying to use the interfaces
> > > > exposed there. It would be nice to say this more loudly. Basically,
> > > > a normal system should not depend on anything in the debugfs. I can get
> > > > behind that.
> > >
> > > Again, not true. Mostly all due to the perf interface, fix that to move
> > > out of debugfs (patches have been proposed) and this problem will go
> > > away.
> >
> > You can't have "no rules" and "all distros mount debugfs for good reason".
> > This is asking for (even more) trouble. If there is something universally
> > useful in debugfs (I do not count perf as universally useful -- my parents
> > do not use perf), then why is it living in a filesystem with no rules
> > (where "no rules" seems to also include "don't break interfaces").
>
> Again, "don't break interfaces" is just me saying "don't break the
> interfaces I have created in debugfs as they are to be used by all
> users." Don't take that as a set-in-stone rule of debugfs at all, it
> isn't.
>
> Again, you are trying to exclude a whole range of useful and valid files
> from being used, when there was only a very very very small percentage
> created incorrectly. They have now been fixed, and we have the
> infrastructure to prevent future ones from being created as well, so I
> don't see the issue here anymore.
I'm trying to minimize exposure. So far, debugfs has proven itself to be
repeatedly dangerous/flawed. I would like to take preventative measures to
contain it. Everyone seems to agree that debugfs is useful for debugging,
and I don't doubt that. It may also be riddled with potential holes, so why
expose an entire tree of debugging interfaces to non-root users?
We have %pK to keep kernel addresses out of the hands of non-root users
when reading the debugging interfaces, and we have either my patchset or
Dan Carpenter's to keep non-root users from writing to these debugging
interfaces. There needs to be a way for system owners to be able to protect
themselves proactively from debugfs.
-Kees
--
Kees Cook
Ubuntu Security Team
On Tue, 22 Feb 2011, Dan Carpenter wrote:
> On Tue, Feb 22, 2011 at 11:33:14AM -0800, Greg KH wrote:
> > Again, let's fix the real problems here, world-writable debugfs files.
>
> We could just ban them?
Eh, if you're serious, maybe add an WARN_ONCE so that the source of the
unwanted DAC bits gets cleaned up?
I do wish this went in. debugfs is a hazard, both security-wise and
kernel-quality wise. Anything that has to end up enabled in a distro
kernel really does not belong in debugfs. In fact, if you would object
to a kernel taint if a feature is used, IMO it certainly don't belong on
debugfs.
> diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
> index e7a7a2f..03ae095 100644
> --- a/fs/debugfs/inode.c
> +++ b/fs/debugfs/inode.c
> @@ -223,6 +223,9 @@ struct dentry *debugfs_create_file(const char *name, mode_t mode,
>
> pr_debug("debugfs: creating file '%s'\n",name);
>
> + /* don't allow world writable files */
> + mode &= ~S_IWOTH;
> +
> error = simple_pin_fs(&debug_fs_type, &debugfs_mount,
> &debugfs_mount_count);
> if (error)
>
>
--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh
On Tue, Feb 22, 2011 at 05:58:47PM -0300, Henrique de Moraes Holschuh wrote:
>
> I do wish this went in. debugfs is a hazard, both security-wise and
> kernel-quality wise. Anything that has to end up enabled in a distro
> kernel really does not belong in debugfs. In fact, if you would object
> to a kernel taint if a feature is used, IMO it certainly don't belong on
> debugfs.
>
As I am one of the culprits of adding tool interfaces into debugfs, I'll
give my opinion too.
When I first started using debugfs, it was because it was so much easier
to add files to than /sys, and I remember /proc is something we do not
want to add more functionality to.
It was also because we had no idea how ftrace was going to be used and
what the final ABI was going to be. I was hoping that we can experiment
with the interface and after some time we could move it out of debugfs
with a stable ABI. But this, for various reasons, never materialized.
Currently things are still in a large flux, and I do not know how this
will play out. Although the changes in /debugfs/tracing has toned down a
lot, there's also the work going in on how to merge perf and ftrace.
This may start changes as well, and perhaps deprecate interfaces. I
don't know.
Having debugfs as the main interface was not the goal of all this, but
we seemed to have just gotten stuck with it.
-- Steve
On Thu, 24 Feb 2011, Steven Rostedt wrote:
> On Tue, Feb 22, 2011 at 05:58:47PM -0300, Henrique de Moraes Holschuh wrote:
> > I do wish this went in. debugfs is a hazard, both security-wise and
> > kernel-quality wise. Anything that has to end up enabled in a distro
> > kernel really does not belong in debugfs. In fact, if you would object
> > to a kernel taint if a feature is used, IMO it certainly don't belong on
> > debugfs.
>
> It was also because we had no idea how ftrace was going to be used and
> what the final ABI was going to be. I was hoping that we can experiment
> with the interface and after some time we could move it out of debugfs
> with a stable ABI. But this, for various reasons, never materialized.
>
> Currently things are still in a large flux, and I do not know how this
> will play out. Although the changes in /debugfs/tracing has toned down a
> lot, there's also the work going in on how to merge perf and ftrace.
> This may start changes as well, and perhaps deprecate interfaces. I
> don't know.
How will that play with Linus' ruling that "you shipped it, someone used it,
it becomes an stable ABI" ?
Some middle ground is clearly needed, here.
--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh
Hi Greg,
On Tue, Feb 22, 2011 at 12:54:13PM -0800, Kees Cook wrote:
> On Tue, Feb 22, 2011 at 12:37:04PM -0800, Greg KH wrote:
> > On Tue, Feb 22, 2011 at 12:28:56PM -0800, Kees Cook wrote:
> > > On Tue, Feb 22, 2011 at 12:16:10PM -0800, Greg KH wrote:
> > > > On Tue, Feb 22, 2011 at 11:50:18AM -0800, Kees Cook wrote:
> > > > > On Tue, Feb 22, 2011 at 07:34:18PM +0000, Alan Cox wrote:
> > > > > > > What system do you proposed to keep these "stupid mistakes" from
> > > > > > > continuing to happen? If debugfs had already been mode 0700, we could have
> > > > > > > avoided all of these CVEs, including the full-blown local root escalation.
> > > > > >
> > > > > > And all sorts of features would have put themselves in sysfs instead and
> > > > > > broken no doubt.
> > > > > >
> > > > > > > The "no rules" approach to debugfs is not a good idea, IMO.
> > > > > >
> > > > > > It's a debugging fs, it needs to be "no rules" other than the obvious
> > > > > > "don't mount it on production systems"
> > > > >
> > > > > Okay, so the debugfs is not supposed to be mounted on a production system.
> > > >
> > > > No, not true at all, the "enterprise" distros all mount debugfs for good
> > > > reason on their systems.
> > >
> > > What reasons are those? Or better yet, why do you and Alan Cox disagree on
> > > this point?
> >
> > These distros have made the decision to support the perf interface,
> > which lives in debugfs, for their customers. I'm not saying that I
> > disagree with Alan about this, just pointing out the reality of the
> > situation here.
>
> A tool used only by the root user, so the proposed mount mode of 0700
> wouldn't break anything.
The summary is this:
- debugfs has been demonstrably dangerous to have available
- Alan Cox says that debugfs should not be used on production systems
- Greg KH does not disagree
- however, pref needs it, and this is used by some root users
- perf will likely move out of debugfs as some point
What is the objection, then, to making the root of debugfs mode 0600? All
the tools I reviewed that need it run as root (e.g. powertop). I've
already written, tested, and sent the patches -- they would not break
the requirements above.
-Kees
--
Kees Cook
Ubuntu Security Team
On Thu, Feb 24, 2011 at 04:22:14PM -0800, Kees Cook wrote:
> Hi Greg,
>
> On Tue, Feb 22, 2011 at 12:54:13PM -0800, Kees Cook wrote:
> > On Tue, Feb 22, 2011 at 12:37:04PM -0800, Greg KH wrote:
> > > On Tue, Feb 22, 2011 at 12:28:56PM -0800, Kees Cook wrote:
> > > > On Tue, Feb 22, 2011 at 12:16:10PM -0800, Greg KH wrote:
> > > > > On Tue, Feb 22, 2011 at 11:50:18AM -0800, Kees Cook wrote:
> > > > > > On Tue, Feb 22, 2011 at 07:34:18PM +0000, Alan Cox wrote:
> > > > > > > > What system do you proposed to keep these "stupid mistakes" from
> > > > > > > > continuing to happen? If debugfs had already been mode 0700, we could have
> > > > > > > > avoided all of these CVEs, including the full-blown local root escalation.
> > > > > > >
> > > > > > > And all sorts of features would have put themselves in sysfs instead and
> > > > > > > broken no doubt.
> > > > > > >
> > > > > > > > The "no rules" approach to debugfs is not a good idea, IMO.
> > > > > > >
> > > > > > > It's a debugging fs, it needs to be "no rules" other than the obvious
> > > > > > > "don't mount it on production systems"
> > > > > >
> > > > > > Okay, so the debugfs is not supposed to be mounted on a production system.
> > > > >
> > > > > No, not true at all, the "enterprise" distros all mount debugfs for good
> > > > > reason on their systems.
> > > >
> > > > What reasons are those? Or better yet, why do you and Alan Cox disagree on
> > > > this point?
> > >
> > > These distros have made the decision to support the perf interface,
> > > which lives in debugfs, for their customers. I'm not saying that I
> > > disagree with Alan about this, just pointing out the reality of the
> > > situation here.
> >
> > A tool used only by the root user, so the proposed mount mode of 0700
> > wouldn't break anything.
>
> The summary is this:
> - debugfs has been demonstrably dangerous to have available
Wait, I do not believe this statement at all.
It's like saying "sysfs and proc are demonstrably dangerous to have
available" because there were some bugs with some implementations of
sysfs and proc files in the past.
> - Alan Cox says that debugfs should not be used on production systems
> - Greg KH does not disagree
I also don't agree, as my day-job entails supporting a wide range of
production systems with this filesystem mounted and enabled.
> - however, pref needs it, and this is used by some root users
> - perf will likely move out of debugfs as some point
>
> What is the objection, then, to making the root of debugfs mode 0600? All
> the tools I reviewed that need it run as root (e.g. powertop). I've
> already written, tested, and sent the patches -- they would not break
> the requirements above.
There are a wide range of other files that can be safely read as a
normal user in debugfs. For example, the usb debugging files which we
use to help debug hardware controller issues. Now yes, we could ask the
user to become root first, but is that really necessary?
Again, I feel these were just a few bugs that do not reflect the much
larger and benificial use of this filesystem. We now have a set of
checks in place to prevent this type of error from occuring again, why
not rely on that instead of just removing the whole filesystem from
normal users entirely?
thanks,
greg k-h
On Thu, Feb 24, 2011 at 04:35:08PM -0800, Greg KH wrote:
> On Thu, Feb 24, 2011 at 04:22:14PM -0800, Kees Cook wrote:
> > On Tue, Feb 22, 2011 at 12:54:13PM -0800, Kees Cook wrote:
> > > On Tue, Feb 22, 2011 at 12:37:04PM -0800, Greg KH wrote:
> > > > On Tue, Feb 22, 2011 at 12:28:56PM -0800, Kees Cook wrote:
> > > > > On Tue, Feb 22, 2011 at 12:16:10PM -0800, Greg KH wrote:
> > > > > > On Tue, Feb 22, 2011 at 11:50:18AM -0800, Kees Cook wrote:
> > > > > > > On Tue, Feb 22, 2011 at 07:34:18PM +0000, Alan Cox wrote:
> > > > > > > > > What system do you proposed to keep these "stupid mistakes" from
> > > > > > > > > continuing to happen? If debugfs had already been mode 0700, we could have
> > > > > > > > > avoided all of these CVEs, including the full-blown local root escalation.
> > > > > > > >
> > > > > > > > And all sorts of features would have put themselves in sysfs instead and
> > > > > > > > broken no doubt.
> > > > > > > >
> > > > > > > > > The "no rules" approach to debugfs is not a good idea, IMO.
> > > > > > > >
> > > > > > > > It's a debugging fs, it needs to be "no rules" other than the obvious
> > > > > > > > "don't mount it on production systems"
> > > > > > >
> > > > > > > Okay, so the debugfs is not supposed to be mounted on a production system.
> > > > > >
> > > > > > No, not true at all, the "enterprise" distros all mount debugfs for good
> > > > > > reason on their systems.
> > > > >
> > > > > What reasons are those? Or better yet, why do you and Alan Cox disagree on
> > > > > this point?
> > > >
> > > > These distros have made the decision to support the perf interface,
> > > > which lives in debugfs, for their customers. I'm not saying that I
> > > > disagree with Alan about this, just pointing out the reality of the
> > > > situation here.
> > >
> > > A tool used only by the root user, so the proposed mount mode of 0700
> > > wouldn't break anything.
> >
> > The summary is this:
> > - debugfs has been demonstrably dangerous to have available
>
> Wait, I do not believe this statement at all.
>
> It's like saying "sysfs and proc are demonstrably dangerous to have
> available" because there were some bugs with some implementations of
> sysfs and proc files in the past.
Since sysfs and proc have "rules", it discourages bad code more than
debugfs does.
> > - Alan Cox says that debugfs should not be used on production systems
> > - Greg KH does not disagree
>
> I also don't agree, as my day-job entails supporting a wide range of
> production systems with this filesystem mounted and enabled.
I was careful in reproducing your earlier statement about not disagreeing.
:)
> > - however, pref needs it, and this is used by some root users
> > - perf will likely move out of debugfs as some point
> >
> > What is the objection, then, to making the root of debugfs mode 0600? All
> > the tools I reviewed that need it run as root (e.g. powertop). I've
> > already written, tested, and sent the patches -- they would not break
> > the requirements above.
>
> There are a wide range of other files that can be safely read as a
> normal user in debugfs. For example, the usb debugging files which we
> use to help debug hardware controller issues. Now yes, we could ask the
> user to become root first, but is that really necessary?
If production systems should not have debugfs mounted, and the file is
universally useful to non-root users, it should move like the perf
interfaces, right?
> Again, I feel these were just a few bugs that do not reflect the much
> larger and benificial use of this filesystem. We now have a set of
> checks in place to prevent this type of error from occuring again, why
> not rely on that instead of just removing the whole filesystem from
> normal users entirely?
I don't feel that a test in checkpatch is sufficient to prevent future
problems. What about Dan Carpenter's patch?
-Kees
--
Kees Cook
Ubuntu Security Team
On Thu, Feb 24, 2011 at 05:12:42PM -0800, Kees Cook wrote:
> On Thu, Feb 24, 2011 at 04:35:08PM -0800, Greg KH wrote:
> > On Thu, Feb 24, 2011 at 04:22:14PM -0800, Kees Cook wrote:
> > > On Tue, Feb 22, 2011 at 12:54:13PM -0800, Kees Cook wrote:
> > > > On Tue, Feb 22, 2011 at 12:37:04PM -0800, Greg KH wrote:
> > > > > On Tue, Feb 22, 2011 at 12:28:56PM -0800, Kees Cook wrote:
> > > > > > On Tue, Feb 22, 2011 at 12:16:10PM -0800, Greg KH wrote:
> > > > > > > On Tue, Feb 22, 2011 at 11:50:18AM -0800, Kees Cook wrote:
> > > > > > > > On Tue, Feb 22, 2011 at 07:34:18PM +0000, Alan Cox wrote:
> > > > > > > > > > What system do you proposed to keep these "stupid mistakes" from
> > > > > > > > > > continuing to happen? If debugfs had already been mode 0700, we could have
> > > > > > > > > > avoided all of these CVEs, including the full-blown local root escalation.
> > > > > > > > >
> > > > > > > > > And all sorts of features would have put themselves in sysfs instead and
> > > > > > > > > broken no doubt.
> > > > > > > > >
> > > > > > > > > > The "no rules" approach to debugfs is not a good idea, IMO.
> > > > > > > > >
> > > > > > > > > It's a debugging fs, it needs to be "no rules" other than the obvious
> > > > > > > > > "don't mount it on production systems"
> > > > > > > >
> > > > > > > > Okay, so the debugfs is not supposed to be mounted on a production system.
> > > > > > >
> > > > > > > No, not true at all, the "enterprise" distros all mount debugfs for good
> > > > > > > reason on their systems.
> > > > > >
> > > > > > What reasons are those? Or better yet, why do you and Alan Cox disagree on
> > > > > > this point?
> > > > >
> > > > > These distros have made the decision to support the perf interface,
> > > > > which lives in debugfs, for their customers. I'm not saying that I
> > > > > disagree with Alan about this, just pointing out the reality of the
> > > > > situation here.
> > > >
> > > > A tool used only by the root user, so the proposed mount mode of 0700
> > > > wouldn't break anything.
> > >
> > > The summary is this:
> > > - debugfs has been demonstrably dangerous to have available
> >
> > Wait, I do not believe this statement at all.
> >
> > It's like saying "sysfs and proc are demonstrably dangerous to have
> > available" because there were some bugs with some implementations of
> > sysfs and proc files in the past.
>
> Since sysfs and proc have "rules", it discourages bad code more than
> debugfs does.
Oh yeah? You should see the crap in proc :)
And I activly fight to keep the sysfs code sane, no reason why we can't
pay attention to debugfs as well, I just haven't been.
> > > - Alan Cox says that debugfs should not be used on production systems
> > > - Greg KH does not disagree
> >
> > I also don't agree, as my day-job entails supporting a wide range of
> > production systems with this filesystem mounted and enabled.
>
> I was careful in reproducing your earlier statement about not disagreeing.
> :)
>
> > > - however, pref needs it, and this is used by some root users
> > > - perf will likely move out of debugfs as some point
> > >
> > > What is the objection, then, to making the root of debugfs mode 0600? All
> > > the tools I reviewed that need it run as root (e.g. powertop). I've
> > > already written, tested, and sent the patches -- they would not break
> > > the requirements above.
> >
> > There are a wide range of other files that can be safely read as a
> > normal user in debugfs. For example, the usb debugging files which we
> > use to help debug hardware controller issues. Now yes, we could ask the
> > user to become root first, but is that really necessary?
>
> If production systems should not have debugfs mounted, and the file is
> universally useful to non-root users, it should move like the perf
> interfaces, right?
No, the usb files have nothing to do with perf.
I do strongly feel that the perf files need to move out of debugfs and
have proposed perffs and tracefs numerous times in the past, patches
included, but it's up to the maintainers and developers of that code to
make that change, not me.
And if you do that, I see no reason to mount debugfs on any "enterprise"
system, which would be a great thing.
> > Again, I feel these were just a few bugs that do not reflect the much
> > larger and benificial use of this filesystem. We now have a set of
> > checks in place to prevent this type of error from occuring again, why
> > not rely on that instead of just removing the whole filesystem from
> > normal users entirely?
>
> I don't feel that a test in checkpatch is sufficient to prevent future
> problems. What about Dan Carpenter's patch?
I have no objection to Dan's patch, it's in my "to-apply" queue and I
should get to it in a day or so.
thanks,
greg k-h
On Thu, Feb 24, 2011 at 07:31:46PM -0800, Greg KH wrote:
> On Thu, Feb 24, 2011 at 05:12:42PM -0800, Kees Cook wrote:
> > On Thu, Feb 24, 2011 at 04:35:08PM -0800, Greg KH wrote:
> > > On Thu, Feb 24, 2011 at 04:22:14PM -0800, Kees Cook wrote:
> > > > On Tue, Feb 22, 2011 at 12:54:13PM -0800, Kees Cook wrote:
> > > > > On Tue, Feb 22, 2011 at 12:37:04PM -0800, Greg KH wrote:
> > > > > > On Tue, Feb 22, 2011 at 12:28:56PM -0800, Kees Cook wrote:
> > > > > > > On Tue, Feb 22, 2011 at 12:16:10PM -0800, Greg KH wrote:
> > > > > > > > On Tue, Feb 22, 2011 at 11:50:18AM -0800, Kees Cook wrote:
> > > > > > > > > On Tue, Feb 22, 2011 at 07:34:18PM +0000, Alan Cox wrote:
> > > > > > > > > > > What system do you proposed to keep these "stupid mistakes" from
Gentlement, I must congratulate you on the perfect match of form and contents,
but could you please take that to the place where it belongs? alt.cascade
is -> that way...
On Tue, Feb 22, 2011 at 11:29:15PM +0300, Dan Carpenter wrote:
> On Tue, Feb 22, 2011 at 11:33:14AM -0800, Greg KH wrote:
> > Again, let's fix the real problems here, world-writable debugfs files.
>
> We could just ban them?
>
> diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
> index e7a7a2f..03ae095 100644
> --- a/fs/debugfs/inode.c
> +++ b/fs/debugfs/inode.c
> @@ -223,6 +223,9 @@ struct dentry *debugfs_create_file(const char *name, mode_t mode,
>
> pr_debug("debugfs: creating file '%s'\n",name);
>
> + /* don't allow world writable files */
> + mode &= ~S_IWOTH;
> +
> error = simple_pin_fs(&debug_fs_type, &debugfs_mount,
> &debugfs_mount_count);
> if (error)
>
I have no objection to this patch, care to resend it with a
signed-off-by: so that I can apply it?
thanks,
greg k-h
On Fri, 25 Feb 2011, Greg KH wrote:
> On Tue, Feb 22, 2011 at 11:29:15PM +0300, Dan Carpenter wrote:
> > On Tue, Feb 22, 2011 at 11:33:14AM -0800, Greg KH wrote:
> > > Again, let's fix the real problems here, world-writable debugfs files.
> >
> > We could just ban them?
> >
> > diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
> > index e7a7a2f..03ae095 100644
> > --- a/fs/debugfs/inode.c
> > +++ b/fs/debugfs/inode.c
> > @@ -223,6 +223,9 @@ struct dentry *debugfs_create_file(const char *name, mode_t mode,
> >
> > pr_debug("debugfs: creating file '%s'\n",name);
> >
> > + /* don't allow world writable files */
> > + mode &= ~S_IWOTH;
> > +
> > error = simple_pin_fs(&debug_fs_type, &debugfs_mount,
> > &debugfs_mount_count);
> > if (error)
> >
>
> I have no objection to this patch, care to resend it with a
> signed-off-by: so that I can apply it?
That's funny, I wrote something in debugfs a few months ago which was
very deliberately rw--w--w-: a kind of circular trace buffer into which
any userspace could easily echo its progress for debugging.
Probably won't be upstreamed in that form, nor use debugfs if it is.
But I mention it as an example of why any such limitation on debugfs
seems inappropriate to me.
Hugh
On Fri, Feb 25, 2011 at 12:40:43PM -0800, Hugh Dickins wrote:
> On Fri, 25 Feb 2011, Greg KH wrote:
> > On Tue, Feb 22, 2011 at 11:29:15PM +0300, Dan Carpenter wrote:
> > > On Tue, Feb 22, 2011 at 11:33:14AM -0800, Greg KH wrote:
> > > > Again, let's fix the real problems here, world-writable debugfs files.
> > >
> > > We could just ban them?
> > >
> > > diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
> > > index e7a7a2f..03ae095 100644
> > > --- a/fs/debugfs/inode.c
> > > +++ b/fs/debugfs/inode.c
> > > @@ -223,6 +223,9 @@ struct dentry *debugfs_create_file(const char *name, mode_t mode,
> > >
> > > pr_debug("debugfs: creating file '%s'\n",name);
> > >
> > > + /* don't allow world writable files */
> > > + mode &= ~S_IWOTH;
> > > +
> > > error = simple_pin_fs(&debug_fs_type, &debugfs_mount,
> > > &debugfs_mount_count);
> > > if (error)
> > >
> >
> > I have no objection to this patch, care to resend it with a
> > signed-off-by: so that I can apply it?
>
> That's funny, I wrote something in debugfs a few months ago which was
> very deliberately rw--w--w-: a kind of circular trace buffer into which
> any userspace could easily echo its progress for debugging.
>
> Probably won't be upstreamed in that form, nor use debugfs if it is.
> But I mention it as an example of why any such limitation on debugfs
> seems inappropriate to me.
Ok, fair enough, then I'll not add this patch, as you just proved it is
a valuable thing to have world writable debugfs files.
thanks,
greg k-h
On Thursday 24 February 2011, Steven Rostedt wrote:
> Currently things are still in a large flux, and I do not know how this
> will play out. Although the changes in /debugfs/tracing has toned down a
> lot, there's also the work going in on how to merge perf and ftrace.
> This may start changes as well, and perhaps deprecate interfaces. I
> don't know.
>
> Having debugfs as the main interface was not the goal of all this, but
> we seemed to have just gotten stuck with it.
Some time ago, I started a patch set to make it possible to unify a
lot of the pseudo file systems like debugfs, configfs and others, and
to also make it possible to trivially create another file_system_type,
basically creating a new root inode in the same way that you'd create a new
debugfs directory.
The patch set is probably quite outdated now, and I currently don't have
time to finish it, but I'd be happy to help someone else refresh it
to get it upstream.
Once that is done, you could move the tracing files and any other
parts of debugfs that have evolved into a proper API into their own
file systems without changing much code.
This would in particular make it possible to mount the main debugfs
root-only while you can have other rules for parts of it that have
become real interfaces.
Arnd