Re: [PATCH] proc: return -EPERM when preventing read of /proc/*/maps
On Jan 4, 2008 4:19 PM, Al Viro <[EMAIL PROTECTED]> 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 -- 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/
Re: [PATCH] proc: return -EPERM when preventing read of /proc/*/maps
Al Viro <[EMAIL PROTECTED]> wrote: > How about this: At least the task_mmu part works fine. Tested-by: Guillaume Chazarain <[EMAIL PROTECTED]> -- Guillaume -- 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/
Re: [PATCH] proc: return -EPERM when preventing read of /proc/*/maps
On Fri, Jan 04, 2008 at 03:35:57PM +0100, Guillaume Chazarain wrote:
> Al Viro <[EMAIL PROTECTED]> 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 (vm
Re: [PATCH] proc: return -EPERM when preventing read of /proc/*/maps
Al Viro <[EMAIL PROTECTED]> 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:[] EFLAGS: 00010286 CPU: 0
EIP is at vma_stop+0xd/0x21
EAX: f7c90360 EBX: f7c90360 ECX: c042b5f0 EDX:
ESI: f62aa240 EDI: EBP: f62daf24 ESP: f62daf20
DS: 007b ES: 007b FS: GS: 0033 SS: 0068
Process cat (pid: 2282, ti=f62da000 task=f6264d20 task.ti=f62da000)
Stack: f7c90360 f62daf30 c01a40dc f62d0080 f62daf70 c018bdf1 0400 0804f000
f62d0080 f62aa260 0400 f62cc000 f62dafb0
f62d0080 c018bc9e 0804f000 f62daf90 c01751c5 f62daf9c
Call Trace:
[] show_trace_log_lvl+0x1a/0x2f
[] show_stack_log_lvl+0x9d/0xa5
[] show_registers+0xa2/0x1b8
[] die+0x11d/0x202
[] do_general_protection+0x1f7/0x1ff
[] error_code+0x6a/0x70
[] m_stop+0xe/0x29
[] seq_read+0x153/0x25a
[] vfs_read+0xa6/0x158
[] sys_read+0x3d/0x61
[] 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: [] 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
--
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/
Re: [PATCH] proc: return -EPERM when preventing read of /proc/*/maps
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... -- 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/
[PATCH] proc: return -EPERM when preventing read of /proc/*/maps
Return an error instead of successfully reading an empty file.
Signed-off-by: Guillaume Chazarain <[EMAIL PROTECTED]>
Acked-by: Al Viro <[EMAIL PROTECTED]>
---
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 */
--
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/
