[PATCH] proc: advertise new restrictions on /proc/*/maps & /proc/*/smaps

[PATCH] proc: advertise new restrictions on /proc/*/maps & /proc/*/smaps

[Guillaume Chazarain]

Now that strangers are kept out of /proc/<pid>/maps, let's welcome them
with -EPERM instead of a blank file.

Signed-off-by: Guillaume Chazarain <guichaz@yahoo.fr>
---

 fs/proc/base.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)


diff --git a/fs/proc/base.c b/fs/proc/base.c
index 7411bfb..c824b23 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -2207,7 +2207,7 @@ static const struct pid_entry tgid_base_stuff[] = {
 	INF("cmdline",    S_IRUGO, pid_cmdline),
 	INF("stat",       S_IRUGO, tgid_stat),
 	INF("statm",      S_IRUGO, pid_statm),
-	REG("maps",       S_IRUGO, maps),
+	REG("maps",       S_IRUSR, maps),
 #ifdef CONFIG_NUMA
 	REG("numa_maps",  S_IRUGO, numa_maps),
 #endif
@@ -2219,7 +2219,7 @@ static const struct pid_entry tgid_base_stuff[] = {
 	REG("mountstats", S_IRUSR, mountstats),
 #ifdef CONFIG_MMU
 	REG("clear_refs", S_IWUSR, clear_refs),
-	REG("smaps",      S_IRUGO, smaps),
+	REG("smaps",      S_IRUSR, smaps),
 #endif
 #ifdef CONFIG_SECURITY
 	DIR("attr",       S_IRUGO|S_IXUGO, attr_dir),
@@ -2533,7 +2533,7 @@ static const struct pid_entry tid_base_stuff[] = {
 	INF("cmdline",   S_IRUGO, pid_cmdline),
 	INF("stat",      S_IRUGO, tid_stat),
 	INF("statm",     S_IRUGO, pid_statm),
-	REG("maps",      S_IRUGO, maps),
+	REG("maps",      S_IRUSR, maps),
 #ifdef CONFIG_NUMA
 	REG("numa_maps", S_IRUGO, numa_maps),
 #endif
@@ -2544,7 +2544,7 @@ static const struct pid_entry tid_base_stuff[] = {
 	REG("mounts",    S_IRUGO, mounts),
 #ifdef CONFIG_MMU
 	REG("clear_refs", S_IWUSR, clear_refs),
-	REG("smaps",     S_IRUGO, smaps),
+	REG("smaps",     S_IRUSR, smaps),
 #endif
 #ifdef CONFIG_SECURITY
 	DIR("attr",      S_IRUGO|S_IXUGO, attr_dir),

Re: [PATCH] proc: advertise new restrictions on /proc/*/maps & /proc/*/smaps

[Al Viro]

On Fri, Jan 04, 2008 at 12:51:50AM +0100, Guillaume Chazarain wrote:
> Now that strangers are kept out of /proc/<pid>/maps, let's welcome them
> with -EPERM instead of a blank file.

NAK

The whole point is that we have to reject it at read() time, not open()
time.  Checks in open() are
	a) useless (since conditions can change later)
and
	b) actually broken, since CAP_SYS_PTRACE != CAP_DAC_OVERRIDE

Re: [PATCH] proc: advertise new restrictions on /proc/*/maps & /proc/*/smaps

[Guillaume Chazarain]

Al Viro <viro@ZenIV.linux.org.uk> wrote:

> The whole point is that we have to reject it at read() time, not open()
> time.

Yes, my patch was a complement to yours to propagate the -EPERM in easy
cases. As you noted it added restrictions on reading /proc/*/maps, even
though I found them acceptable.

How about this instead?

Maybe you'd prefer to propagate the actual -EPERM from
__ptrace_may_attach but that would be more invasive.

Sidenote: do you think a sparse annotation to check IS_ERR/PTR_ERR
usage would make sense?

proc: return -EPERM when preventing read of /proc/*/maps

Return an error instead of successfully reading an empty file.

Signed-off-by: Guillaume Chazarain <guichaz@yahoo.fr>
---

 fs/proc/base.c       |    2 +-
 fs/proc/task_mmu.c   |    8 +++++---
 fs/proc/task_nommu.c |    4 ++--
 3 files changed, 8 insertions(+), 6 deletions(-)


diff --git a/fs/proc/base.c b/fs/proc/base.c
index 7411bfb..3aebc85 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -219,7 +219,7 @@ out:
 	task_unlock(task);
 	up_read(&mm->mmap_sem);
 	mmput(mm);
-	return NULL;
+	return ERR_PTR(-EPERM);
 }
 
 static int proc_pid_cmdline(struct task_struct *task, char * buffer)
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index 8043a3e..db57e65 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -398,8 +398,8 @@ static void *m_start(struct seq_file *m, loff_t
*pos) return NULL;
 
 	mm = mm_for_maps(priv->task);
-	if (!mm)
-		return NULL;
+	if (IS_ERR(mm) || !mm)
+		return mm;
 
 	priv->tail_vma = tail_vma = get_gate_vma(priv->task);
 
@@ -437,7 +437,7 @@ out:
 
 static void vma_stop(struct proc_maps_private *priv, struct
vm_area_struct *vma) {
-	if (vma && vma != priv->tail_vma) {
+	if (vma && !IS_ERR(vma) && vma != priv->tail_vma) {
 		struct mm_struct *mm = vma->vm_mm;
 		up_read(&mm->mmap_sem);
 		mmput(mm);
@@ -451,6 +451,8 @@ static void *m_next(struct seq_file *m, void *v,
loff_t *pos) struct vm_area_struct *tail_vma = priv->tail_vma;
 
 	(*pos)++;
+	if (IS_ERR(vma))
+		return vma;
 	if (vma && (vma != tail_vma) && vma->vm_next)
 		return vma->vm_next;
 	vma_stop(priv, vma);
diff --git a/fs/proc/task_nommu.c b/fs/proc/task_nommu.c
index 1932c2c..53cb062 100644
--- a/fs/proc/task_nommu.c
+++ b/fs/proc/task_nommu.c
@@ -166,10 +166,10 @@ static void *m_start(struct seq_file *m, loff_t
*pos) return NULL;
 
 	mm = mm_for_maps(priv->task);
-	if (!mm) {
+	if (IS_ERR(mm) || !mm) {
 		put_task_struct(priv->task);
 		priv->task = NULL;
-		return NULL;
+		return mm;
 	}
 
 	/* start from the Nth VMA */


-- 
Guillaume

Re: [PATCH] proc: advertise new restrictions on /proc/*/maps & /proc/*/smaps

[Al Viro]

On Fri, Jan 04, 2008 at 12:15:02PM +0100, Guillaume Chazarain wrote:
> Al Viro <viro@ZenIV.linux.org.uk> wrote:
> 
> > The whole point is that we have to reject it at read() time, not open()
> > time.
> 
> Yes, my patch was a complement to yours to propagate the -EPERM in easy
> cases. As you noted it added restrictions on reading /proc/*/maps, even
> though I found them acceptable.
> 
> How about this instead?
> 
> Maybe you'd prefer to propagate the actual -EPERM from
> __ptrace_may_attach but that would be more invasive.
> 
> Sidenote: do you think a sparse annotation to check IS_ERR/PTR_ERR
> usage would make sense?
> 
> proc: return -EPERM when preventing read of /proc/*/maps
> 
> Return an error instead of successfully reading an empty file.

You are overcomplicating it - if ->start() returns ERR_PTR(), it's over;
read() will fail with that error and that's it.  No need to mess with
->next(), etc. - it'll never see that ERR_PTR(-EPERM).  Drop these chunks
and you've got an ACK...