LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] proc: return -EPERM when preventing read of /proc/*/maps
@ 2008-01-04 13:57 Guillaume Chazarain
  2008-01-04 14:14 ` Al Viro
  0 siblings, 1 reply; 6+ messages in thread
From: Guillaume Chazarain @ 2008-01-04 13:57 UTC (permalink / raw)
  To: Andrew Morton; +Cc: Al Viro, linux-kernel

Return an error instead of successfully reading an empty file.

Signed-off-by: Guillaume Chazarain <guichaz@yahoo.fr>
Acked-by: Al Viro <viro@ZenIV.linux.org.uk>
---

 fs/proc/base.c       |    2 +-
 fs/proc/task_mmu.c   |    6 +++---
 fs/proc/task_nommu.c |    4 ++--
 3 files changed, 6 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..74b4829 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);
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 */


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] proc: return -EPERM when preventing read of /proc/*/maps
  2008-01-04 13:57 [PATCH] proc: return -EPERM when preventing read of /proc/*/maps Guillaume Chazarain
@ 2008-01-04 14:14 ` Al Viro
  2008-01-04 14:35   ` Guillaume Chazarain
  0 siblings, 1 reply; 6+ messages in thread
From: Al Viro @ 2008-01-04 14:14 UTC (permalink / raw)
  To: Guillaume Chazarain; +Cc: Andrew Morton, linux-kernel

On Fri, Jan 04, 2008 at 02:57:31PM +0100, Guillaume Chazarain wrote:
> Return an error instead of successfully reading an empty file.

vma_stop() doesn't need changes either...

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] proc: return -EPERM when preventing read of /proc/*/maps
  2008-01-04 14:14 ` Al Viro
@ 2008-01-04 14:35   ` Guillaume Chazarain
  2008-01-04 15:19     ` Al Viro
  0 siblings, 1 reply; 6+ messages in thread
From: Guillaume Chazarain @ 2008-01-04 14:35 UTC (permalink / raw)
  To: Al Viro; +Cc: Andrew Morton, linux-kernel

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

> vma_stop() doesn't need changes either...

Hmmm, not sure ;-)

$ cat /proc/1/maps
Pid: 2282, comm: cat Not tainted (2.6.24-rc6-gc2 #185)
EIP: 0060:[<c01a4080>] EFLAGS: 00010286 CPU: 0
EIP is at vma_stop+0xd/0x21
EAX: f7c90360 EBX: f7c90360 ECX: c042b5f0 EDX: ffffffff
ESI: f62aa240 EDI: ffffffff EBP: f62daf24 ESP: f62daf20
 DS: 007b ES: 007b FS: 0000 GS: 0033 SS: 0068
Process cat (pid: 2282, ti=f62da000 task=f6264d20 task.ti=f62da000)
Stack: f7c90360 f62daf30 c01a40dc f62d0080 f62daf70 c018bdf1 00000400 0804f000 
       f62d0080 f62aa260 00000000 ffffffff 00000400 f62cc000 f62dafb0 00000000 
       00000000 f62d0080 c018bc9e 0804f000 f62daf90 c01751c5 f62daf9c 00000000 
Call Trace:
 [<c0104e4a>] show_trace_log_lvl+0x1a/0x2f
 [<c0104efc>] show_stack_log_lvl+0x9d/0xa5
 [<c0104fa6>] show_registers+0xa2/0x1b8
 [<c01051d9>] die+0x11d/0x202
 [<c03319f9>] do_general_protection+0x1f7/0x1ff
 [<c0331172>] error_code+0x6a/0x70
 [<c01a40dc>] m_stop+0xe/0x29
 [<c018bdf1>] seq_read+0x153/0x25a
 [<c01751c5>] vfs_read+0xa6/0x158
 [<c0175583>] sys_read+0x3d/0x61
 [<c0103ea2>] sysenter_past_esp+0x6b/0xa1
 =======================
Code: 89 50 18 31 d2 89 48 1c 83 c4 5c 89 d0 5b 5e 5f 5d c3 55 31 c9 89 e5 e8 80 fd ff ff 5d c3 55 85 d2 89 e5 53 74 16 3b 50 08 74 11 <8b> 1a 8d 43 34 e8 80 ea f8 ff 89 d8 e8 16 89 f7 ff 5b 5d c3 55 
EIP: [<c01a4080>] vma_stop+0xd/0x21 SS:ESP 0068:f62daf20
---[ end trace 297d07fbbfc82b7b ]---

This is an inconsistency in the handling of errors in m_start() between
fs/proc/task_mmu.c and fs/proc/task_nommu.c.

task_mmu.c:
        if (IS_ERR(mm) || !mm)
                return mm;

task_nommu.c:
        if (IS_ERR(mm) || !mm) {
                put_task_struct(priv->task);
                priv->task = NULL;
                return mm;
        }

task_nommu.c does the cleanup while task_mmu.c defers it to m_stop.

-- 
Guillaume

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] proc: return -EPERM when preventing read of /proc/*/maps
  2008-01-04 14:35   ` Guillaume Chazarain
@ 2008-01-04 15:19     ` Al Viro
  2008-01-04 16:15       ` Guillaume Chazarain
  2008-02-03 18:20       ` Guillaume Chazarain
  0 siblings, 2 replies; 6+ messages in thread
From: Al Viro @ 2008-01-04 15:19 UTC (permalink / raw)
  To: Guillaume Chazarain; +Cc: Andrew Morton, linux-kernel

On Fri, Jan 04, 2008 at 03:35:57PM +0100, Guillaume Chazarain wrote:
> Al Viro <viro@ZenIV.linux.org.uk> wrote:
> 
> > vma_stop() doesn't need changes either...
> 
> Hmmm, not sure ;-)

Umm...  Actually, m_next() and m_stop() both appear to be too convoluted.

* m_next() never gets v == NULL
* the only reason why we do that mmput et.al. both from ->next() and
  ->stop() is that we try to avoid having priv->mm; why bother?
* why the _hell_ is proc_maps_private defined in include/linux/proc_fs.h,
  of all places?
* while we are at it, why is it in any header at all?  Having that sucker
  in task_mmu.c and task_nommu.c would be more than enough (and we'd avoid
  that ifdef in definition, while we are at it).

How about this:

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..75bef20 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -131,12 +131,19 @@ struct pmd_walker {
 		       unsigned long, void *);
 };
 
+struct proc_maps_private {
+	struct pid *pid;
+	struct task_struct *task;
+	struct vm_area_struct *tail_vma;
+	struct mm_struct *mm;
+};
+
 static int show_map_internal(struct seq_file *m, void *v, struct mem_size_stats *mss)
 {
 	struct proc_maps_private *priv = m->private;
 	struct task_struct *task = priv->task;
+	struct mm_struct *mm = priv->mm;
 	struct vm_area_struct *vma = v;
-	struct mm_struct *mm = vma->vm_mm;
 	struct file *file = vma->vm_file;
 	int flags = vma->vm_flags;
 	unsigned long ino = 0;
@@ -381,6 +388,7 @@ static void *m_start(struct seq_file *m, loff_t *pos)
 
 	/* Clear the per syscall fields in priv */
 	priv->task = NULL;
+	priv->mm = NULL;
 	priv->tail_vma = NULL;
 
 	/*
@@ -398,10 +406,11 @@ static void *m_start(struct seq_file *m, loff_t *pos)
 		return NULL;
 
 	mm = mm_for_maps(priv->task);
-	if (!mm)
-		return NULL;
+	if (!mm || IS_ERR(mm))
+		return ERR_CAST(mm);
 
 	priv->tail_vma = tail_vma = get_gate_vma(priv->task);
+	priv->mm = mm;
 
 	/* Start with last addr hint */
 	if (last_addr && (vma = find_vma(mm, last_addr))) {
@@ -430,20 +439,9 @@ out:
 
 	/* End of vmas has been reached */
 	m->version = (tail_vma != NULL)? 0: -1UL;
-	up_read(&mm->mmap_sem);
-	mmput(mm);
 	return tail_vma;
 }
 
-static void vma_stop(struct proc_maps_private *priv, struct vm_area_struct *vma)
-{
-	if (vma && vma != priv->tail_vma) {
-		struct mm_struct *mm = vma->vm_mm;
-		up_read(&mm->mmap_sem);
-		mmput(mm);
-	}
-}
-
 static void *m_next(struct seq_file *m, void *v, loff_t *pos)
 {
 	struct proc_maps_private *priv = m->private;
@@ -451,18 +449,22 @@ static void *m_next(struct seq_file *m, void *v, loff_t *pos)
 	struct vm_area_struct *tail_vma = priv->tail_vma;
 
 	(*pos)++;
-	if (vma && (vma != tail_vma) && vma->vm_next)
+	if (vma == tail_vma)
+		return NULL;
+	else if (vma->vm_next)
 		return vma->vm_next;
-	vma_stop(priv, vma);
-	return (vma != tail_vma)? tail_vma: NULL;
+	else
+		return tail_vma;
 }
 
 static void m_stop(struct seq_file *m, void *v)
 {
 	struct proc_maps_private *priv = m->private;
-	struct vm_area_struct *vma = v;
 
-	vma_stop(priv, vma);
+	if (priv->mm) {
+		up_read(&priv->mm->mmap_sem);
+		mmput(priv->mm);
+	}
 	if (priv->task)
 		put_task_struct(priv->task);
 }
diff --git a/fs/proc/task_nommu.c b/fs/proc/task_nommu.c
index 1932c2c..a156b62 100644
--- a/fs/proc/task_nommu.c
+++ b/fs/proc/task_nommu.c
@@ -138,6 +138,12 @@ out:
 	return result;
 }
 
+struct proc_maps_private {
+	struct pid *pid;
+	struct task_struct *task;
+	struct mm_struct *mm;
+};
+
 /*
  * display mapping lines for a particular process's /proc/pid/maps
  */
@@ -161,16 +167,16 @@ static void *m_start(struct seq_file *m, loff_t *pos)
 	loff_t n = *pos;
 
 	/* pin the task and mm whilst we play with them */
+	priv->mm = NULL;
 	priv->task = get_pid_task(priv->pid, PIDTYPE_PID);
 	if (!priv->task)
 		return NULL;
 
 	mm = mm_for_maps(priv->task);
-	if (!mm) {
-		put_task_struct(priv->task);
-		priv->task = NULL;
-		return NULL;
-	}
+	if (!mm || IS_ERR(mm))
+		return ERR_CAST(mm);
+
+	priv->mm = mm;
 
 	/* start from the Nth VMA */
 	for (vml = mm->context.vmlist; vml; vml = vml->next)
@@ -183,12 +189,12 @@ static void m_stop(struct seq_file *m, void *_vml)
 {
 	struct proc_maps_private *priv = m->private;
 
-	if (priv->task) {
-		struct mm_struct *mm = priv->task->mm;
-		up_read(&mm->mmap_sem);
-		mmput(mm);
-		put_task_struct(priv->task);
+	if (priv->mm) {
+		up_read(&priv->mm->mmap_sem);
+		mmput(priv->mm);
 	}
+	if (priv->task)
+		put_task_struct(priv->task);
 }
 
 static void *m_next(struct seq_file *m, void *_vml, loff_t *pos)
diff --git a/include/linux/proc_fs.h b/include/linux/proc_fs.h
index a531682..192a5c4 100644
--- a/include/linux/proc_fs.h
+++ b/include/linux/proc_fs.h
@@ -286,12 +286,4 @@ static inline struct net *PDE_NET(struct proc_dir_entry *pde)
 
 struct net *get_proc_net(const struct inode *inode);
 
-struct proc_maps_private {
-	struct pid *pid;
-	struct task_struct *task;
-#ifdef CONFIG_MMU
-	struct vm_area_struct *tail_vma;
-#endif
-};
-
 #endif /* _LINUX_PROC_FS_H */

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] proc: return -EPERM when preventing read of /proc/*/maps
  2008-01-04 15:19     ` Al Viro
@ 2008-01-04 16:15       ` Guillaume Chazarain
  2008-02-03 18:20       ` Guillaume Chazarain
  1 sibling, 0 replies; 6+ messages in thread
From: Guillaume Chazarain @ 2008-01-04 16:15 UTC (permalink / raw)
  To: Al Viro; +Cc: Andrew Morton, linux-kernel

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

> How about this:

At least the task_mmu part works fine.

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

-- 
Guillaume

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] proc: return -EPERM when preventing read of /proc/*/maps
  2008-01-04 15:19     ` Al Viro
  2008-01-04 16:15       ` Guillaume Chazarain
@ 2008-02-03 18:20       ` Guillaume Chazarain
  1 sibling, 0 replies; 6+ messages in thread
From: Guillaume Chazarain @ 2008-02-03 18:20 UTC (permalink / raw)
  To: Al Viro; +Cc: Andrew Morton, linux-kernel

On Jan 4, 2008 4:19 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> Umm...  Actually, m_next() and m_stop() both appear to be too convoluted.
>
> * m_next() never gets v == NULL
> * the only reason why we do that mmput et.al. both from ->next() and
>   ->stop() is that we try to avoid having priv->mm; why bother?
> * why the _hell_ is proc_maps_private defined in include/linux/proc_fs.h,
>   of all places?
> * while we are at it, why is it in any header at all?  Having that sucker
>   in task_mmu.c and task_nommu.c would be more than enough (and we'd avoid
>   that ifdef in definition, while we are at it).
>
> How about this:

Hi Al,

Any update on this patch?
As you completely rewrote it, I thought you would take care of pushing
it forward.

Thanks.

-- 
Guillaume

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2008-02-03 18:20 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-01-04 13:57 [PATCH] proc: return -EPERM when preventing read of /proc/*/maps Guillaume Chazarain
2008-01-04 14:14 ` Al Viro
2008-01-04 14:35   ` Guillaume Chazarain
2008-01-04 15:19     ` Al Viro
2008-01-04 16:15       ` Guillaume Chazarain
2008-02-03 18:20       ` Guillaume Chazarain

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).