Re: [PATCH] proc: maps protection

Re: [PATCH] proc: maps protection

[Kees Cook]

On Tue, Mar 06, 2007 at 06:59:42PM -0800, Andrew Morton wrote:
> On Tue, 6 Mar 2007 18:13:35 -0800 Kees Cook <kees@outflux.net> wrote:
> 
> > On Tue, Mar 06, 2007 at 05:56:09PM -0800, Andrew Morton wrote:
> > > On Tue, 6 Mar 2007 17:22:34 -0800
> > > Kees Cook <kees@outflux.net> wrote:
> > > 
> > > > This is a continuation of a much earlier discussion[1].  As I 
> > > > understand, the problem is:
> > > 
> > > This sounds like a really good way of breaking lots and lots of people's
> > > expensively-developed stuff.  In ways which we won't discover until a year
> > > after we shipped it.
> > > 
> > > So nope, sorry.  Need to find a compatible way of doing this.  Perhaps a
> > > kernel boot parameter or a /proc knob.
> > 
> > Do you have examples of things in the kernel that I can use as a 
> > starting point?
> 
> No, I don't think this has precedent.
> 
> >  Would something like /proc/sys/kernel/maps_protect be 
> > reasonable?
> 
> Yes, that sounds reasonable.
> 
> An alternative is to do it with elf headers, perhaps - let the process
> specify what protections it wants in some manner.
> 
> > If an acceptable toggle is made, would you consider it being enabled by 
> > default (i.e. "tighter security by default")?
> 
> Again, that sounds risky.

[Adding Cc:lkml]

How about using a reduced check, as is done for fd and environ?  This 
would allow root-running system monitors to still do their job.  
Effectively, this changes the test from "is ptracing" to just "can 
ptrace".

If this still isn't considered safe, I'll add the maps_protect file...

--- 
task_mmu.c   |   16 +++++++++++++++-
 task_nommu.c |    6 ++++++
 2 files changed, 21 insertions(+), 1 deletion(-)
---
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index 7445980..7c9aad3 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -134,6 +134,9 @@ static int show_map_internal(struct seq_file *m, void *v, struct mem_size_stats
 	dev_t dev = 0;
 	int len;
 
+	if (!ptrace_may_attach(task))
+		return -EACCES;
+
 	if (file) {
 		struct inode *inode = vma->vm_file->f_path.dentry->d_inode;
 		dev = inode->i_sb->s_dev;
@@ -444,11 +447,22 @@ const struct file_operations proc_maps_operations = {
 #ifdef CONFIG_NUMA
 extern int show_numa_map(struct seq_file *m, void *v);
 
+static int show_numa_map_checked(struct seq_file *m, void *v)
+{
+	struct proc_maps_private *priv = m->private;
+	struct task_struct *task = priv->task;
+
+	if (!ptrace_may_attach(task))
+		return -EACCES;
+	
+	return show_numa_map(m, v);
+}
+
 static struct seq_operations proc_pid_numa_maps_op = {
         .start  = m_start,
         .next   = m_next,
         .stop   = m_stop,
-        .show   = show_numa_map
+        .show   = show_numa_map_checked
 };
 
 static int numa_maps_open(struct inode *inode, struct file *file)
diff --git a/fs/proc/task_nommu.c b/fs/proc/task_nommu.c
index 7cddf6b..c5783b7 100644
--- a/fs/proc/task_nommu.c
+++ b/fs/proc/task_nommu.c
@@ -143,6 +143,12 @@ out:
 static int show_map(struct seq_file *m, void *_vml)
 {
 	struct vm_list_struct *vml = _vml;
+	struct proc_maps_private *priv = m->private;
+	struct task_struct *task = priv->task;
+	
+	if (!ptrace_may_attach(task))
+		return -EACCES;
+
 	return nommu_vma_show(m, vml->vma);
 }
 



-- 
Kees Cook                                            @outflux.net

Re: [PATCH] proc: maps protection

[Arjan van de Ven]

> [Adding Cc:lkml]

> How about using a reduced check, as is done for fd and environ?  This 
> would allow root-running system monitors to still do their job.  
> Effectively, this changes the test from "is ptracing" to just "can 
> ptrace".
> 
> If this still isn't considered safe, I'll add the maps_protect file...


btw I consider it an information leak that any user can see which
files/libraries any other user and root has mmap'd. (and with glibc's
stdio mmap feature that goes even beyond direct mmap to fopen()'d).

If root or some other user wants to watch
hillary-vs-obama-in-the-mud.avi, no other user has ANY business even
seeing that. So at minimum it's a privacy issue showing the filenames...

Re: [PATCH] proc: maps protection

[Kees Cook]

On Tue, Mar 06, 2007 at 08:22:11PM -0800, Arjan van de Ven wrote:
> > [Adding Cc:lkml]
> 
> > How about using a reduced check, as is done for fd and environ?  This 
> > would allow root-running system monitors to still do their job.  
> > Effectively, this changes the test from "is ptracing" to just "can 
> > ptrace".
> > 
> > If this still isn't considered safe, I'll add the maps_protect file...
> 
> btw I consider it an information leak that any user can see which
> files/libraries any other user and root has mmap'd. (and with glibc's
> stdio mmap feature that goes even beyond direct mmap to fopen()'d).
> 
> If root or some other user wants to watch
> hillary-vs-obama-in-the-mud.avi, no other user has ANY business even
> seeing that. So at minimum it's a privacy issue showing the filenames...

So, what's the state of this?  Is the reduced "allowed to ptrace" check 
good enough for inclusion?  What is needed for some form of this patch 
to be included?  I'm happy to try new approaches if I can get some 
further input.

Another suggestion was to make this an ELF header toggle, and I'm 
generally against that idea.  If a toggle is needed, I'd rather it be a 
system-wide one.

-- 
Kees Cook

Re: [PATCH] proc: maps protection

[Andrew Morton]

> On Thu, 8 Mar 2007 12:55:25 -0800 Kees Cook <kees@outflux.net> wrote:
> On Tue, Mar 06, 2007 at 08:22:11PM -0800, Arjan van de Ven wrote:
> > > [Adding Cc:lkml]
> > 
> > > How about using a reduced check, as is done for fd and environ?  This 
> > > would allow root-running system monitors to still do their job.  
> > > Effectively, this changes the test from "is ptracing" to just "can 
> > > ptrace".
> > > 
> > > If this still isn't considered safe, I'll add the maps_protect file...
> > 
> > btw I consider it an information leak that any user can see which
> > files/libraries any other user and root has mmap'd. (and with glibc's
> > stdio mmap feature that goes even beyond direct mmap to fopen()'d).
> > 
> > If root or some other user wants to watch
> > hillary-vs-obama-in-the-mud.avi, no other user has ANY business even
> > seeing that. So at minimum it's a privacy issue showing the filenames...
> 
> So, what's the state of this?  Is the reduced "allowed to ptrace" check 
> good enough for inclusion?  What is needed for some form of this patch 
> to be included?  I'm happy to try new approaches if I can get some 
> further input.

I just don't know what it will break - we're changing things so that user A
cannot monitor user B's memory maps.  I feel that it's sure to break
various people's fancy custom system activity monitoring/logging setups,
and the sort of users who will be affected are, alas, the sort of people
who won't run a kernel with this change in it for another couple of years
yet.

> Another suggestion was to make this an ELF header toggle, and I'm 
> generally against that idea.  If a toggle is needed, I'd rather it be a 
> system-wide one.

A global /proc knob probably isn't too bad.

Do we actually need to disable the whole interface?  If all you're
concerned about is the pathname then perhaps the knob could cause that
pathname to be replaced with "<hidden>".  That'll cause things to
break less seriously and still allows somewhat useful info to be gathered.

Re: [PATCH] proc: maps protection

[Arjan van de Ven]

Andrew Morton wrote:
>> On Thu, 8 Mar 2007 12:55:25 -0800 Kees Cook <kees@outflux.net> wrote:
>> On Tue, Mar 06, 2007 at 08:22:11PM -0800, Arjan van de Ven wrote:
>>>> [Adding Cc:lkml]
>>>> How about using a reduced check, as is done for fd and environ?  This 
>>>> would allow root-running system monitors to still do their job.  
>>>> Effectively, this changes the test from "is ptracing" to just "can 
>>>> ptrace".
>>>>
>>>> If this still isn't considered safe, I'll add the maps_protect file...
>>> btw I consider it an information leak that any user can see which
>>> files/libraries any other user and root has mmap'd. (and with glibc's
>>> stdio mmap feature that goes even beyond direct mmap to fopen()'d).
>>>
>>> If root or some other user wants to watch
>>> hillary-vs-obama-in-the-mud.avi, no other user has ANY business even
>>> seeing that. So at minimum it's a privacy issue showing the filenames...
>> So, what's the state of this?  Is the reduced "allowed to ptrace" check 
>> good enough for inclusion?  What is needed for some form of this patch 
>> to be included?  I'm happy to try new approaches if I can get some 
>> further input.
> 
> I just don't know what it will break - we're changing things so that user A
> cannot monitor user B's memory maps.  I feel that it's sure to break
> various people's fancy custom system activity monitoring/logging setups,
> and the sort of users who will be affected are, alas, the sort of people
> who won't run a kernel with this change in it for another couple of years
> yet.

except if they run RHEL or FC kernels, in which case they already have 
that change

> Do we actually need to disable the whole interface?  If all you're
> concerned about is the pathname then perhaps the knob could cause that
> pathname to be replaced with "<hidden>".  That'll cause things to
> break less seriously and still allows somewhat useful info to be gathered.

the problem part is that you can see EXACLTY where glibc is loaded in 
memory, which effectively defeats address space randomization for 
local users....

Re: [PATCH] proc: maps protection

[Kees Cook]

On Fri, Mar 09, 2007 at 09:01:41PM -0800, Arjan van de Ven wrote:
> >I just don't know what it will break - we're changing things so that user A
> >cannot monitor user B's memory maps.  I feel that it's sure to break
> >various people's fancy custom system activity monitoring/logging setups,
> >and the sort of users who will be affected are, alas, the sort of people
> >who won't run a kernel with this change in it for another couple of years
> >yet.
> 
> except if they run RHEL or FC kernels, in which case they already have 
> that change

Right, this is a "gentler" version of just slapping 0400 perms on the 
maps files, which already has the same effect.  I think tightening this 
bit of security is worth some possible breakage.

> >Do we actually need to disable the whole interface?  If all you're
> >concerned about is the pathname then perhaps the knob could cause that
> >pathname to be replaced with "<hidden>".  That'll cause things to
> >break less seriously and still allows somewhat useful info to be gathered.
> 
> the problem part is that you can see EXACLTY where glibc is loaded in 
> memory, which effectively defeats address space randomization for 
> local users....

Right; and since the maps are loaded in a specific order, just dropping 
the filename isn't going to help in protecting the ASLR.  e.g. I can get 
apache's library list from its ELF, and it's very easy to line that up 
with the maps file even if the filenames are missing.

Here's another revision, with both the "can ptrace" and the global /proc 
knob; and I'll beg for defaulting the knob to "on".  :)

Signed-off-by: Kees Cook <kees@outflux.net>

---
 fs/proc/base.c         |    3 +++
 fs/proc/internal.h     |    2 ++
 fs/proc/task_mmu.c     |   16 +++++++++++++++-
 fs/proc/task_nommu.c   |    6 ++++++
 include/linux/sysctl.h |    1 +
 kernel/sysctl.c        |    9 +++++++++
 6 files changed, 36 insertions(+), 1 deletion(-)
---
diff --git a/fs/proc/base.c b/fs/proc/base.c
index 1a979ea..9b6e731 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -123,6 +123,9 @@ struct pid_entry {
 		NULL, &proc_info_file_operations,	\
 		{ .proc_read = &proc_##OTYPE } )
 
+int maps_protect = 1;
+EXPORT_SYMBOL(maps_protect);
+
 static struct fs_struct *get_fs_struct(struct task_struct *task)
 {
 	struct fs_struct *fs;
diff --git a/fs/proc/internal.h b/fs/proc/internal.h
index 987c773..596d925 100644
--- a/fs/proc/internal.h
+++ b/fs/proc/internal.h
@@ -31,6 +31,8 @@ do {						\
 extern int nommu_vma_show(struct seq_file *, struct vm_area_struct *);
 #endif
 
+extern int maps_protect;
+
 extern void create_seq_entry(char *name, mode_t mode, const struct file_operations *f);
 extern int proc_exe_link(struct inode *, struct dentry **, struct vfsmount **);
 extern int proc_tid_stat(struct task_struct *,  char *);
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index 55ade0d..2e43c12 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -134,6 +134,9 @@ static int show_map_internal(struct seq_file *m, void *v, struct mem_size_stats
 	dev_t dev = 0;
 	int len;
 
+	if (maps_protect && !ptrace_may_attach(task))
+		return -EACCES;
+
 	if (file) {
 		struct inode *inode = vma->vm_file->f_path.dentry->d_inode;
 		dev = inode->i_sb->s_dev;
@@ -444,11 +447,22 @@ struct file_operations proc_maps_operations = {
 #ifdef CONFIG_NUMA
 extern int show_numa_map(struct seq_file *m, void *v);
 
+static int show_numa_map_checked(struct seq_file *m, void *v)
+{
+	struct proc_maps_private *priv = m->private;
+	struct task_struct *task = priv->task;
+
+	if (maps_protect && !ptrace_may_attach(task))
+		return -EACCES;
+	
+	return show_numa_map(m, v);
+}
+
 static struct seq_operations proc_pid_numa_maps_op = {
         .start  = m_start,
         .next   = m_next,
         .stop   = m_stop,
-        .show   = show_numa_map
+        .show   = show_numa_map_checked
 };
 
 static int numa_maps_open(struct inode *inode, struct file *file)
diff --git a/fs/proc/task_nommu.c b/fs/proc/task_nommu.c
index fcc5caf..fb36c6a 100644
--- a/fs/proc/task_nommu.c
+++ b/fs/proc/task_nommu.c
@@ -143,6 +143,12 @@ out:
 static int show_map(struct seq_file *m, void *_vml)
 {
 	struct vm_list_struct *vml = _vml;
+	struct proc_maps_private *priv = m->private;
+	struct task_struct *task = priv->task;
+	
+	if (maps_protect && !ptrace_may_attach(task))
+		return -EACCES;
+
 	return nommu_vma_show(m, vml->vma);
 }
 
diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
index 81480e6..743d63d 100644
--- a/include/linux/sysctl.h
+++ b/include/linux/sysctl.h
@@ -160,6 +160,7 @@ enum
 	KERN_MAX_LOCK_DEPTH=74,
 	KERN_NMI_WATCHDOG=75, /* int: enable/disable nmi watchdog */
 	KERN_PANIC_ON_NMI=76, /* int: whether we will panic on an unrecovered */
+	KERN_MAPS_PROTECT=77, /* int: whether we protect maps from public visibility */
 };
 
 
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index b01e767..653a6ad 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -76,6 +76,7 @@ extern int pid_max_min, pid_max_max;
 extern int sysctl_drop_caches;
 extern int percpu_pagelist_fraction;
 extern int compat_log;
+extern int maps_protect;
 
 /* this is needed for the proc_dointvec_minmax for [fs_]overflow UID and GID */
 static int maxolduid = 65535;
@@ -782,6 +783,14 @@ static ctl_table kern_table[] = {
 		.proc_handler	= &proc_dointvec,
 	},
 #endif
+	{
+		.ctl_name       = KERN_MAPS_PROTECT,
+		.procname       = "maps_protect",
+		.data           = &maps_protect,
+		.maxlen         = sizeof(int),
+		.mode           = 0644,
+		.proc_handler   = &proc_dointvec,
+	},
 
 	{ .ctl_name = 0 }
 };


-- 
Kees Cook

Re: [PATCH] proc: maps protection

[Andrew Morton]

> On Sat, 10 Mar 2007 10:33:41 -0800 Kees Cook <kees@outflux.net> wrote:
> Here's another revision, with both the "can ptrace" and the global /proc 
> knob;

We'd be needing a changelog for that.

Please update the procfs documentation.

Does the patch also cover /proc/pid/smaps?

Re: [PATCH] proc: maps protection

[Matt Mackall]

On Sat, Mar 10, 2007 at 04:21:01PM -0800, Andrew Morton wrote:
> > On Sat, 10 Mar 2007 10:33:41 -0800 Kees Cook <kees@outflux.net> wrote:
> > Here's another revision, with both the "can ptrace" and the global /proc 
> > knob;
> 
> We'd be needing a changelog for that.
> 
> Please update the procfs documentation.
> 
> Does the patch also cover /proc/pid/smaps?

Also, we ought to revisit /proc/pid/mem write, which is currently disabled.
Either drop the code, fix it, or make it root only.

-- 
Mathematics is the supreme nostalgia of our time.

Re: [PATCH] proc: maps protection

[Kees Cook]

The /proc/pid/ "maps", "smaps", and "numa_maps" files contain sensitive 
information about the memory location and usage of processes.  Issues:

- maps should not be world-readable, especially if programs expect any 
  kind of ASLR protection from local attackers.
- maps cannot just be 0400 because "-D_FORTIFY_SOURCE=2 -O2" makes glibc
  check the maps when %n is in a *printf call, and a setuid(getuid()) 
  process wouldn't be able to read its own maps file.  (For reference
  see http://lkml.org/lkml/2006/1/22/150)
- a system-wide toggle is needed to allow prior behavior in the case of
  non-root applications that depend on access to the maps contents.

This change implements a check using "ptrace_may_attach" before allowing 
access to read the maps contents.  To control this protection, the new 
knob /proc/sys/kernel/maps_protect has been added, with corresponding 
updates to the procfs documentation.

Signed-off-by: Kees Cook <kees@outflux.net>
---
 CREDITS                            |    2 +-
 Documentation/filesystems/proc.txt |    7 +++++++
 fs/proc/base.c                     |    3 +++
 fs/proc/internal.h                 |    2 ++
 fs/proc/task_mmu.c                 |   16 +++++++++++++++-
 fs/proc/task_nommu.c               |    6 ++++++
 include/linux/sysctl.h             |    1 +
 kernel/sysctl.c                    |    9 +++++++++
 8 files changed, 44 insertions(+), 2 deletions(-)
---
diff --git a/CREDITS b/CREDITS
index 6bd8ab8..38c3ada 100644
--- a/CREDITS
+++ b/CREDITS
@@ -655,7 +655,7 @@ N: Kees Cook
 E: kees@outflux.net
 W: http://outflux.net/
 P: 1024D/17063E6D 9FA3 C49C 23C9 D1BC 2E30  1975 1FFF 4BA9 1706 3E6D
-D: Minor updates to SCSI code for the Communications type
+D: Minor updates to SCSI types, added /proc/pid/maps protection
 S: (ask for current address)
 S: USA
 
diff --git a/Documentation/filesystems/proc.txt b/Documentation/filesystems/proc.txt
index 5484ab5..d9b06b5 100644
--- a/Documentation/filesystems/proc.txt
+++ b/Documentation/filesystems/proc.txt
@@ -1137,6 +1137,13 @@ determine whether or not they are still functioning properly.
 Because the NMI watchdog shares registers with oprofile, by disabling the NMI
 watchdog, oprofile may have more registers to utilize.
 
+maps_protect
+------------
+
+Enables/Disables the protection of the per-process proc entries "maps" and
+"smaps".  When enabled, the contents of these files are visible only to
+readers that are allowed to ptrace() the given process.
+
 
 2.4 /proc/sys/vm - The virtual memory subsystem
 -----------------------------------------------
diff --git a/fs/proc/base.c b/fs/proc/base.c
index 01f7769..6feccbc 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -123,6 +123,9 @@ struct pid_entry {
 		NULL, &proc_info_file_operations,	\
 		{ .proc_read = &proc_##OTYPE } )
 
+int maps_protect = 0;
+EXPORT_SYMBOL(maps_protect);
+
 static struct fs_struct *get_fs_struct(struct task_struct *task)
 {
 	struct fs_struct *fs;
diff --git a/fs/proc/internal.h b/fs/proc/internal.h
index c932aa6..2c65b6e 100644
--- a/fs/proc/internal.h
+++ b/fs/proc/internal.h
@@ -33,6 +33,8 @@ do {						\
 extern int nommu_vma_show(struct seq_file *, struct vm_area_struct *);
 #endif
 
+extern int maps_protect;
+
 extern void create_seq_entry(char *name, mode_t mode, const struct file_operations *f);
 extern int proc_exe_link(struct inode *, struct dentry **, struct vfsmount **);
 extern int proc_tid_stat(struct task_struct *,  char *);
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index 7445980..45a0f3e 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -134,6 +134,9 @@ static int show_map_internal(struct seq_file *m, void *v, struct mem_size_stats
 	dev_t dev = 0;
 	int len;
 
+	if (maps_protect && !ptrace_may_attach(task))
+		return -EACCES;
+
 	if (file) {
 		struct inode *inode = vma->vm_file->f_path.dentry->d_inode;
 		dev = inode->i_sb->s_dev;
@@ -444,11 +447,22 @@ const struct file_operations proc_maps_operations = {
 #ifdef CONFIG_NUMA
 extern int show_numa_map(struct seq_file *m, void *v);
 
+static int show_numa_map_checked(struct seq_file *m, void *v)
+{
+	struct proc_maps_private *priv = m->private;
+	struct task_struct *task = priv->task;
+
+	if (maps_protect && !ptrace_may_attach(task))
+		return -EACCES;
+	
+	return show_numa_map(m, v);
+}
+
 static struct seq_operations proc_pid_numa_maps_op = {
         .start  = m_start,
         .next   = m_next,
         .stop   = m_stop,
-        .show   = show_numa_map
+        .show   = show_numa_map_checked
 };
 
 static int numa_maps_open(struct inode *inode, struct file *file)
diff --git a/fs/proc/task_nommu.c b/fs/proc/task_nommu.c
index 7cddf6b..c2747c9 100644
--- a/fs/proc/task_nommu.c
+++ b/fs/proc/task_nommu.c
@@ -143,6 +143,12 @@ out:
 static int show_map(struct seq_file *m, void *_vml)
 {
 	struct vm_list_struct *vml = _vml;
+	struct proc_maps_private *priv = m->private;
+	struct task_struct *task = priv->task;
+	
+	if (maps_protect && !ptrace_may_attach(task))
+		return -EACCES;
+
 	return nommu_vma_show(m, vml->vma);
 }
 
diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
index 2c5fb38..608a331 100644
--- a/include/linux/sysctl.h
+++ b/include/linux/sysctl.h
@@ -165,6 +165,7 @@ enum
 	KERN_MAX_LOCK_DEPTH=74,
 	KERN_NMI_WATCHDOG=75, /* int: enable/disable nmi watchdog */
 	KERN_PANIC_ON_NMI=76, /* int: whether we will panic on an unrecovered */
+	KERN_MAPS_PROTECT=77, /* int: whether we protect maps from public visibility */
 };
 
 
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 1b255df..ca4d69f 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -76,6 +76,7 @@ extern int pid_max_min, pid_max_max;
 extern int sysctl_drop_caches;
 extern int percpu_pagelist_fraction;
 extern int compat_log;
+extern int maps_protect;
 
 /* this is needed for the proc_dointvec_minmax for [fs_]overflow UID and GID */
 static int maxolduid = 65535;
@@ -603,6 +604,14 @@ static ctl_table kern_table[] = {
 		.proc_handler	= &proc_dointvec,
 	},
 #endif
+	{,
+		.ctl_name       = KERN_MAPS_PROTECT,
+		.procname       = "maps_protect",
+		.data           = &maps_protect,
+		.maxlen         = sizeof(int),
+		.mode           = 0644,
+		.proc_handler   = &proc_dointvec,
+	},
 
 	{ .ctl_name = 0 }
 };



-- 
Kees Cook

Re: [PATCH] proc: maps protection

[Kees Cook]

On Sat, Mar 10, 2007 at 04:21:01PM -0800, Andrew Morton wrote:
> We'd be needing a changelog for that.

Done; sent separately from this email.

> Please update the procfs documentation.

Done.

> Does the patch also cover /proc/pid/smaps?

Yes, and numa_maps.

Thanks!

-- 
Kees Cook

[PATCH] getrusage() : Fill ru_inblock and ru_oublock fields if possible

[Eric Dumazet]

[-- Attachment #1: Type: text/plain, Size: 1105 bytes --]

if CONFIG_TASK_IO_ACCOUNTING is defined, we update io accounting counters for 
each task.

This patch permits reporting of values using the well known getrusage() 
syscall, filling ru_inblock and ru_oublock instead of null values.

As TASK_IO_ACCOUNTING currently counts bytes counts, we approximate blocks 
count doing : nr_blocks = nr_bytes / 512

Example of use :
----------------------
After patch is applied, /usr/bin/time command can now give a good 
approximation of IO that the process had to do.

$ /usr/bin/time grep tototo /usr/include/*
Command exited with non-zero status 1
0.00user 0.02system 0:02.11elapsed 1%CPU (0avgtext+0avgdata 0maxresident)k
24288inputs+0outputs (0major+259minor)pagefaults 0swaps

$ /usr/bin/time dd if=/dev/zero of=/tmp/testfile count=1000
1000+0 enregistrements lus
1000+0 enregistrements écrits
512000 octets (512 kB) copiés, 0,00326601 seconde, 157 MB/s
0.00user 0.00system 0:00.00elapsed 80%CPU (0avgtext+0avgdata 0maxresident)k
0inputs+3000outputs (0major+299minor)pagefaults 0swaps

Signed-off-by: Eric Dumazet <dada1@cosmosbay.com>

[-- Attachment #2: getrusage_add_inblock_and_oublock.patch --]
[-- Type: text/plain, Size: 2673 bytes --]

--- linux-2.6.21-rc3/kernel/sys.c	2007-03-12 17:30:34.000000000 +0100
+++ linux-2.6.21-rc3-ed/kernel/sys.c	2007-03-12 17:50:15.000000000 +0100
@@ -29,6 +29,7 @@
 #include <linux/signal.h>
 #include <linux/cn_proc.h>
 #include <linux/getcpu.h>
+#include <linux/task_io_accounting_ops.h>
 
 #include <linux/compat.h>
 #include <linux/syscalls.h>
@@ -2021,6 +2022,8 @@ static void k_getrusage(struct task_stru
 			r->ru_nivcsw = p->signal->cnivcsw;
 			r->ru_minflt = p->signal->cmin_flt;
 			r->ru_majflt = p->signal->cmaj_flt;
+			r->ru_inblock = task_io_get_inblock(p);
+			r->ru_oublock = task_io_get_oublock(p);
 
 			if (who == RUSAGE_CHILDREN)
 				break;
@@ -2032,6 +2035,8 @@ static void k_getrusage(struct task_stru
 			r->ru_nivcsw += p->signal->nivcsw;
 			r->ru_minflt += p->signal->min_flt;
 			r->ru_majflt += p->signal->maj_flt;
+			r->ru_inblock += task_io_get_inblock(p);
+			r->ru_oublock += task_io_get_oublock(p);
 			t = p;
 			do {
 				utime = cputime_add(utime, t->utime);
@@ -2040,6 +2045,8 @@ static void k_getrusage(struct task_stru
 				r->ru_nivcsw += t->nivcsw;
 				r->ru_minflt += t->min_flt;
 				r->ru_majflt += t->maj_flt;
+				r->ru_inblock += task_io_get_inblock(t);
+				r->ru_oublock += task_io_get_oublock(t);
 				t = next_thread(t);
 			} while (t != p);
 			break;
--- linux-2.6.21-rc3/include/linux/task_io_accounting_ops.h	2007-03-12 17:30:34.000000000 +0100
+++ linux-2.6.21-rc3-ed/include/linux/task_io_accounting_ops.h	2007-03-12 17:50:15.000000000 +0100
@@ -10,11 +10,29 @@ static inline void task_io_account_read(
 	current->ioac.read_bytes += bytes;
 }
 
+/*
+ * We approximate number of blocks, because we account bytes only.
+ * A 'block' is 512 bytes
+ */
+static inline unsigned long task_io_get_inblock(const struct task_struct *p)
+{
+	return p->ioac.read_bytes >> 9;
+}
+
 static inline void task_io_account_write(size_t bytes)
 {
 	current->ioac.write_bytes += bytes;
 }
 
+/*
+ * We approximate number of blocks, because we account bytes only.
+ * A 'block' is 512 bytes
+ */
+static inline unsigned long task_io_get_oublock(const struct task_struct *p)
+{
+	return p->ioac.write_bytes >> 9;
+}
+
 static inline void task_io_account_cancelled_write(size_t bytes)
 {
 	current->ioac.cancelled_write_bytes += bytes;
@@ -31,10 +49,20 @@ static inline void task_io_account_read(
 {
 }
 
+static inline unsigned long task_io_get_inblock(const struct task_struct *p)
+{
+	return 0;
+}
+
 static inline void task_io_account_write(size_t bytes)
 {
 }
 
+static inline unsigned long task_io_get_oublock(const struct task_struct *p)
+{
+	return 0;
+}
+
 static inline void task_io_account_cancelled_write(size_t bytes)
 {
 }

Re: [PATCH] proc: maps protection

[Andy Isaacson]

On Tue, Mar 06, 2007 at 08:22:11PM -0800, Arjan van de Ven wrote:
> > How about using a reduced check, as is done for fd and environ?  This 
> > would allow root-running system monitors to still do their job.  
> > Effectively, this changes the test from "is ptracing" to just "can 
> > ptrace".
> > 
> > If this still isn't considered safe, I'll add the maps_protect file...
> 
> btw I consider it an information leak that any user can see which
> files/libraries any other user and root has mmap'd. (and with glibc's
> stdio mmap feature that goes even beyond direct mmap to fopen()'d).
> 
> If root or some other user wants to watch
> hillary-vs-obama-in-the-mud.avi, no other user has ANY business even
> seeing that. So at minimum it's a privacy issue showing the filenames...

Sure, I would be fine with making /proc/<pid>/maps mode 0400 (so long as
root can still read it).  But please don't take away my ability to debug
applications using pmap(1).  It's so incredibly useful to be able to say
"ah, look, he mapped nss_nis.so rather than nss_nisplus.so".

-andy