Re: [PATCH] proc: advertise new restrictions on /proc/*/maps & /proc/*/smaps
On Fri, Jan 04, 2008 at 12:15:02PM +0100, Guillaume Chazarain wrote: > Al Viro <[EMAIL PROTECTED]> 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... -- 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: advertise new restrictions on /proc/*/maps & /proc/*/smaps
Al Viro <[EMAIL PROTECTED]> 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 <[EMAIL PROTECTED]> --- 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 -- 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: advertise new restrictions on /proc/*/maps & /proc/*/smaps
On Fri, Jan 04, 2008 at 12:51:50AM +0100, Guillaume Chazarain wrote: > Now that strangers are kept out of /proc//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 -- 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: advertise new restrictions on /proc/*/maps & /proc/*/smaps
Now that strangers are kept out of /proc//maps, let's welcome them with -EPERM instead of a blank file. Signed-off-by: Guillaume Chazarain <[EMAIL PROTECTED]> --- 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), -- 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/