/proc/*/environ: wrong placing of ptrace_may_attach() check
PATCH 1/1 /proc/*/environ: wrong placing of ptrace_may_attach() check
---
fs/proc/base.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)
fs/proc/base.c +8 -3
@@ -204,12 +204,17 @@ static int proc_pid_environ(struct task_int res =0 ;struct mm_struct *mm =get_task_mm (task);if (mm) { -unsigned int len = mm->env_end - mm->env_start; +unsigned int len; + + res = -ESRCH ; +if (!ptrace_may_attach (task)) + goto out; + + len = mm->env_end - mm->env_start;if (len >PAGE_SIZE ) len =PAGE_SIZE ; res =access_process_vm (task, mm->env_start, buffer, len,0 ); -if (!ptrace_may_attach (task)) - res = -ESRCH ; +out:mmput (mm); }return res;
Quoted 1 line · On Mon, 28 May 2007 17:41:57 +0400 Alexey Dobriyan <[email protected]> wrote:
Signed-off-by Alexey Dobriyan <[email protected]>
Better changelogs, please.
Quoted 13 lines
fs/proc/base.c +6
-1@@ -204,12 +204,17 @@ static int proc_pid_environ(struct task_int res =0 ;struct mm_struct *mm =get_task_mm (task);if (mm) { -unsigned int len = mm->env_end - mm->env_start; +unsigned int len; + + res = -ESRCH ; +if (!ptrace_may_attach (task)) + goto out; +
fs/proc/base.c static int proc_pid_environ(struct task_ · line 220
Quoted 10 lines
fs/proc/base.c +2
-2+ len = mm->env_end - mm->env_start;if (len >PAGE_SIZE ) len =PAGE_SIZE ; res =access_process_vm (task, mm->env_start, buffer, len,0 ); -if (!ptrace_may_attach (task)) - res = -ESRCH ; +out:mmput (mm); }return res;
What's wrong with the existing code? It's a bit dopey-looking and can, I guess, permit a task to cause a pagefault in an mm which it doesn't have permission to read from. But is there some more serious problem being fixed here?
I shouldn't have to ask this stuff.
Quoted 23 lines · Andrew Morton
Quoted 20 lines · On Mon, 28 May 2007 17:41:57 +0400 Alexey Dobriyan <[email protected]> wrote:
fs/proc/base.c +8
-3@@ -204,12 +204,17 @@ static int proc_pid_environ(struct task_int res =0 ;struct mm_struct *mm =get_task_mm (task);if (mm) { -unsigned int len = mm->env_end - mm->env_start; +unsigned int len; + + res = -ESRCH ; +if (!ptrace_may_attach (task)) + goto out; + + len = mm->env_end - mm->env_start;if (len >PAGE_SIZE ) len =PAGE_SIZE ; res =access_process_vm (task, mm->env_start, buffer, len,0 ); -if (!ptrace_may_attach (task)) - res = -ESRCH ; +out:
Quoted 8 lines
fs/proc/base.c static int proc_pid_environ(struct task_ · line 220
Quoted 3 lines
fs/proc/base.c +0
-0mmput (mm); }return res;What's wrong with the existing code? It's a bit dopey-looking and can, I guess, permit a task to cause a pagefault in an mm which it doesn't have permission to read from. But is there some more serious problem being fixed here?
I think not, because environment will be copied from target task, stay in kernel tmp buffer, but not copied to target buffer due to -ESRCH. But such code is asking for problems.