From: James Pearson <[email protected]>
/proc/PID/environ currently truncates at 4096 characters, patch based on
the /proc/PID/mem code.
Signed-off-by: James Pearson <[email protected]>
---
Patch against 2.6.23-rc6-mm1
--- ./fs/proc/base.c.dist 2007-09-19 12:29:46.244929651 +0100
+++ ./fs/proc/base.c 2007-09-19 12:36:18.155648760 +0100
@@ -202,27 +202,6 @@ static int proc_root_link(struct inode *
(task->state == TASK_STOPPED || task->state == TASK_TRACED) && \
security_ptrace(current,task) == 0))
-static int proc_pid_environ(struct task_struct *task, char * buffer)
-{
- int res = 0;
- struct mm_struct *mm = get_task_mm(task);
- if (mm) {
- 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);
-out:
- mmput(mm);
- }
- return res;
-}
-
static int proc_pid_cmdline(struct task_struct *task, char * buffer)
{
int res = 0;
@@ -740,6 +719,79 @@ static const struct file_operations proc
.open = mem_open,
};
+static ssize_t environ_read(struct file *file, char __user *buf,
+ size_t count, loff_t *ppos)
+{
+ struct task_struct *task = get_proc_task(file->f_dentry->d_inode);
+ char *page;
+ unsigned long src = *ppos;
+ int ret = -ESRCH;
+ struct mm_struct *mm;
+ size_t max_len;
+
+ if (!task)
+ goto out_no_task;
+
+ if (!ptrace_may_attach(task))
+ goto out;
+
+ ret = -ENOMEM;
+ page = (char *)__get_free_page(GFP_TEMPORARY);
+ if (!page)
+ goto out;
+
+ ret = 0;
+
+ mm = get_task_mm(task);
+ if (!mm)
+ goto out_free;
+
+ max_len = (count > PAGE_SIZE) ? PAGE_SIZE : count;
+
+ while (count > 0) {
+ int this_len, retval;
+
+ this_len = mm->env_end - (mm->env_start + src);
+
+ if (this_len <= 0)
+ break;
+
+ if (this_len > max_len)
+ this_len = max_len;
+
+ retval = access_process_vm(task, (mm->env_start + src),
+ page, this_len, 0);
+
+ if (retval <= 0) {
+ ret = retval;
+ break;
+ }
+
+ if (copy_to_user(buf, page, retval)) {
+ ret = -EFAULT;
+ break;
+ }
+
+ ret += retval;
+ src += retval;
+ buf += retval;
+ count -= retval;
+ }
+ *ppos = src;
+
+ mmput(mm);
+out_free:
+ free_page((unsigned long) page);
+out:
+ put_task_struct(task);
+out_no_task:
+ return ret;
+}
+
+static const struct file_operations proc_environ_operations = {
+ .read = environ_read,
+};
+
static ssize_t oom_adjust_read(struct file *file, char __user *buf,
size_t count, loff_t *ppos)
{
@@ -2092,7 +2144,7 @@ static const struct pid_entry tgid_base_
DIR("task", S_IRUGO|S_IXUGO, task),
DIR("fd", S_IRUSR|S_IXUSR, fd),
DIR("fdinfo", S_IRUSR|S_IXUSR, fdinfo),
- INF("environ", S_IRUSR, pid_environ),
+ REG("environ", S_IRUSR, environ),
INF("auxv", S_IRUSR, pid_auxv),
INF("status", S_IRUGO, pid_status),
INF("limits", S_IRUSR, pid_limits),
@@ -2421,7 +2473,7 @@ out_no_task:
static const struct pid_entry tid_base_stuff[] = {
DIR("fd", S_IRUSR|S_IXUSR, fd),
DIR("fdinfo", S_IRUSR|S_IXUSR, fdinfo),
- INF("environ", S_IRUSR, pid_environ),
+ REG("environ", S_IRUSR, environ),
INF("auxv", S_IRUSR, pid_auxv),
INF("status", S_IRUGO, pid_status),
INF("limits", S_IRUSR, pid_limits),
On Wed, 19 Sep 2007 14:35:29 +0100, James Pearson wrote:
> /proc/PID/environ currently truncates at 4096 characters, patch based on
> the /proc/PID/mem code.
Does /proc/PID/mem even work? If I do `strace cat /proc/PID/mem > /dev/null'
for a known good PID, the first read() from /proc/PID/mem fails with ESRCH,
which is bullocks. cat /proc/PID/environ does work. I'm seeing this on
2.6.22 vanilla, 2.6.20 FC6, and 2.6.18 RHEL5 kernels.
/Mikael
Mikael Pettersson wrote:
> On Wed, 19 Sep 2007 14:35:29 +0100, James Pearson wrote:
>> /proc/PID/environ currently truncates at 4096 characters, patch based on
>> the /proc/PID/mem code.
>
> Does /proc/PID/mem even work? If I do `strace cat /proc/PID/mem > /dev/null'
> for a known good PID, the first read() from /proc/PID/mem fails with ESRCH,
Of course it does. Address zero isn't typically mapped.
-hpa
H. Peter Anvin writes:
> Mikael Pettersson wrote:
> > On Wed, 19 Sep 2007 14:35:29 +0100, James Pearson wrote:
> >> /proc/PID/environ currently truncates at 4096 characters, patch based on
> >> the /proc/PID/mem code.
> >
> > Does /proc/PID/mem even work? If I do `strace cat /proc/PID/mem > /dev/null'
> > for a known good PID, the first read() from /proc/PID/mem fails with ESRCH,
>
> Of course it does. Address zero isn't typically mapped.
Indeed. My bad :-(
On Wed, 19 Sep 2007, Mikael Pettersson wrote:
> H. Peter Anvin writes:
> > Mikael Pettersson wrote:
> > > Does /proc/PID/mem even work? If I do `strace cat /proc/PID/mem > /dev/null'
> > > for a known good PID, the first read() from /proc/PID/mem fails with ESRCH,
> >
> > Of course it does. Address zero isn't typically mapped.
>
> Indeed. My bad :-(
No, not quite. Peter explains why "cat /proc/self/mem" gets EIO,
but you were seeing "cat /proc/other/mem" get ESRCH: that's from
the stringent !MAY_PTRACE || !ptrace_may_attach checks.
Hugh
On Wed, 19 Sep 2007 14:35:29 +0100
"James Pearson" <[email protected]> wrote:
>
> From: James Pearson <[email protected]>
>
> /proc/PID/environ currently truncates at 4096 characters, patch based on
> the /proc/PID/mem code.
patch needs to be carefully reviewed from the security POV (ie: permissions)
as well as for correctness. Does anyone have time to do that?
> Signed-off-by: James Pearson <[email protected]>
>
> --- ./fs/proc/base.c.dist 2007-09-19 12:29:46.244929651 +0100
> +++ ./fs/proc/base.c 2007-09-19 12:36:18.155648760 +0100
> @@ -202,27 +202,6 @@ static int proc_root_link(struct inode *
> (task->state == TASK_STOPPED || task->state == TASK_TRACED) && \
> security_ptrace(current,task) == 0))
>
> -static int proc_pid_environ(struct task_struct *task, char * buffer)
> -{
> - int res = 0;
> - struct mm_struct *mm = get_task_mm(task);
> - if (mm) {
> - 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);
> -out:
> - mmput(mm);
> - }
> - return res;
> -}
> -
> static int proc_pid_cmdline(struct task_struct *task, char * buffer)
> {
> int res = 0;
> @@ -740,6 +719,79 @@ static const struct file_operations proc
> .open = mem_open,
> };
>
> +static ssize_t environ_read(struct file *file, char __user *buf,
> + size_t count, loff_t *ppos)
> +{
> + struct task_struct *task = get_proc_task(file->f_dentry->d_inode);
> + char *page;
> + unsigned long src = *ppos;
> + int ret = -ESRCH;
> + struct mm_struct *mm;
> + size_t max_len;
> +
> + if (!task)
> + goto out_no_task;
> +
> + if (!ptrace_may_attach(task))
> + goto out;
> +
> + ret = -ENOMEM;
> + page = (char *)__get_free_page(GFP_TEMPORARY);
Now I wonder what inspired you to reach for GFP_TEMPORARY? Perhaps the
fact that it is crappily named and undocumented.
This should be GFP_KERNEL - the page you're allocating here is not
reclaimable by the VM.
> + if (!page)
> + goto out;
> +
> + ret = 0;
> +
> + mm = get_task_mm(task);
> + if (!mm)
> + goto out_free;
> +
> + max_len = (count > PAGE_SIZE) ? PAGE_SIZE : count;
> +
> + while (count > 0) {
> + int this_len, retval;
> +
> + this_len = mm->env_end - (mm->env_start + src);
> +
> + if (this_len <= 0)
> + break;
> +
> + if (this_len > max_len)
> + this_len = max_len;
> +
> + retval = access_process_vm(task, (mm->env_start + src),
> + page, this_len, 0);
> +
> + if (retval <= 0) {
> + ret = retval;
> + break;
> + }
> +
> + if (copy_to_user(buf, page, retval)) {
> + ret = -EFAULT;
> + break;
> + }
> +
> + ret += retval;
> + src += retval;
> + buf += retval;
> + count -= retval;
> + }
Now that's a funky loop. Someone please convince me that there is no way
in which `count - retval' can ever go negative (ie: huge positive).
> + *ppos = src;
> +
> + mmput(mm);
> +out_free:
> + free_page((unsigned long) page);
> +out:
> + put_task_struct(task);
> +out_no_task:
> + return ret;
> +}
> +
> +static const struct file_operations proc_environ_operations = {
> + .read = environ_read,
> +};
> +
> static ssize_t oom_adjust_read(struct file *file, char __user *buf,
> size_t count, loff_t *ppos)
> {
> @@ -2092,7 +2144,7 @@ static const struct pid_entry tgid_base_
> DIR("task", S_IRUGO|S_IXUGO, task),
> DIR("fd", S_IRUSR|S_IXUSR, fd),
> DIR("fdinfo", S_IRUSR|S_IXUSR, fdinfo),
> - INF("environ", S_IRUSR, pid_environ),
> + REG("environ", S_IRUSR, environ),
> INF("auxv", S_IRUSR, pid_auxv),
> INF("status", S_IRUGO, pid_status),
> INF("limits", S_IRUSR, pid_limits),
> @@ -2421,7 +2473,7 @@ out_no_task:
> static const struct pid_entry tid_base_stuff[] = {
> DIR("fd", S_IRUSR|S_IXUSR, fd),
> DIR("fdinfo", S_IRUSR|S_IXUSR, fdinfo),
> - INF("environ", S_IRUSR, pid_environ),
> + REG("environ", S_IRUSR, environ),
> INF("auxv", S_IRUSR, pid_auxv),
> INF("status", S_IRUGO, pid_status),
> INF("limits", S_IRUSR, pid_limits),
2007/9/19, James Pearson <[email protected]>:
> + while (count > 0) {
> + int this_len, retval;
> +
> + this_len = mm->env_end - (mm->env_start + src);
> +
> + if (this_len <= 0)
> + break;
> +
> + if (this_len > max_len)
> + this_len = max_len;
> +
> + retval = access_process_vm(task, (mm->env_start + src),
> + page, this_len, 0);
> +
> + if (retval <= 0) {
> + ret = retval;
> + break;
> + }
> +
> + if (copy_to_user(buf, page, retval)) {
^^^^
shouldn't you only copy min(count,retval) bytes? otherwise you could
write beyond the users buffer "buf", right?
Arvin
Andrew Morton wrote:
> On Wed, 19 Sep 2007 14:35:29 +0100
> "James Pearson" <[email protected]> wrote:
>
>
>>From: James Pearson <[email protected]>
>>
>>/proc/PID/environ currently truncates at 4096 characters, patch based on
>>the /proc/PID/mem code.
>
>
> patch needs to be carefully reviewed from the security POV (ie: permissions)
> as well as for correctness. Does anyone have time to do that?
>
>
>>Signed-off-by: James Pearson <[email protected]>
>>
>>--- ./fs/proc/base.c.dist 2007-09-19 12:29:46.244929651 +0100
>>+++ ./fs/proc/base.c 2007-09-19 12:36:18.155648760 +0100
>>@@ -202,27 +202,6 @@ static int proc_root_link(struct inode *
>> (task->state == TASK_STOPPED || task->state == TASK_TRACED) && \
>> security_ptrace(current,task) == 0))
>>
>>-static int proc_pid_environ(struct task_struct *task, char * buffer)
>>-{
>>- int res = 0;
>>- struct mm_struct *mm = get_task_mm(task);
>>- if (mm) {
>>- 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);
>>-out:
>>- mmput(mm);
>>- }
>>- return res;
>>-}
>>-
>> static int proc_pid_cmdline(struct task_struct *task, char * buffer)
>> {
>> int res = 0;
>>@@ -740,6 +719,79 @@ static const struct file_operations proc
>> .open = mem_open,
>> };
>>
>>+static ssize_t environ_read(struct file *file, char __user *buf,
>>+ size_t count, loff_t *ppos)
>>+{
>>+ struct task_struct *task = get_proc_task(file->f_dentry->d_inode);
>>+ char *page;
>>+ unsigned long src = *ppos;
>>+ int ret = -ESRCH;
>>+ struct mm_struct *mm;
>>+ size_t max_len;
>>+
>>+ if (!task)
>>+ goto out_no_task;
>>+
>>+ if (!ptrace_may_attach(task))
>>+ goto out;
>>+
>>+ ret = -ENOMEM;
>>+ page = (char *)__get_free_page(GFP_TEMPORARY);
>
>
> Now I wonder what inspired you to reach for GFP_TEMPORARY? Perhaps the
> fact that it is crappily named and undocumented.
>
> This should be GFP_KERNEL - the page you're allocating here is not
> reclaimable by the VM.
The code is based on mem_read() - and that is what mem_read() does in
2.6.23rc6-mm1 - my previous patch for 2.6.23rc5 used GFP_USER, as that
is what mem_read() does in 2.6.23rc5.
>>+ if (!page)
>>+ goto out;
>>+
>>+ ret = 0;
>>+
>>+ mm = get_task_mm(task);
>>+ if (!mm)
>>+ goto out_free;
>>+
>>+ max_len = (count > PAGE_SIZE) ? PAGE_SIZE : count;
>>+
>>+ while (count > 0) {
>>+ int this_len, retval;
>>+
>>+ this_len = mm->env_end - (mm->env_start + src);
>>+
>>+ if (this_len <= 0)
>>+ break;
>>+
>>+ if (this_len > max_len)
>>+ this_len = max_len;
>>+
>>+ retval = access_process_vm(task, (mm->env_start + src),
>>+ page, this_len, 0);
>>+
>>+ if (retval <= 0) {
>>+ ret = retval;
>>+ break;
>>+ }
>>+
>>+ if (copy_to_user(buf, page, retval)) {
>>+ ret = -EFAULT;
>>+ break;
>>+ }
>>+
>>+ ret += retval;
>>+ src += retval;
>>+ buf += retval;
>>+ count -= retval;
>>+ }
>
>
> Now that's a funky loop. Someone please convince me that there is no way
> in which `count - retval' can ever go negative (ie: huge positive).
Again, this is exactly the same as in mem_read()
James Pearson
Arvin Moezzi wrote:
> 2007/9/19, James Pearson <[email protected]>:
>
>>+ while (count > 0) {
>>+ int this_len, retval;
>>+
>>+ this_len = mm->env_end - (mm->env_start + src);
>>+
>>+ if (this_len <= 0)
>>+ break;
>>+
>>+ if (this_len > max_len)
>>+ this_len = max_len;
>>+
>>+ retval = access_process_vm(task, (mm->env_start + src),
>>+ page, this_len, 0);
>>+
>>+ if (retval <= 0) {
>>+ ret = retval;
>>+ break;
>>+ }
>>+
>>+ if (copy_to_user(buf, page, retval)) {
>
> ^^^^
> shouldn't you only copy min(count,retval) bytes? otherwise you could
> write beyond the users buffer "buf", right?
AFAIK, 'retval' can never be greater than 'this_len', which can never be
greater than 'max_len', which can never be greater than 'count'
James Pearson
On (20/09/07 16:46), Andrew Morton didst pronounce:
> On Wed, 19 Sep 2007 14:35:29 +0100
> "James Pearson" <[email protected]> wrote:
>
> >
> > From: James Pearson <[email protected]>
> >
> > /proc/PID/environ currently truncates at 4096 characters, patch based on
> > the /proc/PID/mem code.
>
> patch needs to be carefully reviewed from the security POV (ie: permissions)
> as well as for correctness. Does anyone have time to do that?
>
> > Signed-off-by: James Pearson <[email protected]>
> >
> > --- ./fs/proc/base.c.dist 2007-09-19 12:29:46.244929651 +0100
> > +++ ./fs/proc/base.c 2007-09-19 12:36:18.155648760 +0100
> > @@ -202,27 +202,6 @@ static int proc_root_link(struct inode *
> > (task->state == TASK_STOPPED || task->state == TASK_TRACED) && \
> > security_ptrace(current,task) == 0))
> >
> > -static int proc_pid_environ(struct task_struct *task, char * buffer)
> > -{
> > - int res = 0;
> > - struct mm_struct *mm = get_task_mm(task);
> > - if (mm) {
> > - 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);
> > -out:
> > - mmput(mm);
> > - }
> > - return res;
> > -}
> > -
> > static int proc_pid_cmdline(struct task_struct *task, char * buffer)
> > {
> > int res = 0;
> > @@ -740,6 +719,79 @@ static const struct file_operations proc
> > .open = mem_open,
> > };
> >
> > +static ssize_t environ_read(struct file *file, char __user *buf,
> > + size_t count, loff_t *ppos)
> > +{
> > + struct task_struct *task = get_proc_task(file->f_dentry->d_inode);
> > + char *page;
> > + unsigned long src = *ppos;
> > + int ret = -ESRCH;
> > + struct mm_struct *mm;
> > + size_t max_len;
> > +
> > + if (!task)
> > + goto out_no_task;
> > +
> > + if (!ptrace_may_attach(task))
> > + goto out;
> > +
> > + ret = -ENOMEM;
> > + page = (char *)__get_free_page(GFP_TEMPORARY);
>
> Now I wonder what inspired you to reach for GFP_TEMPORARY? Perhaps the
> fact that it is crappily named and undocumented.
>
Because it's a very short lived allocation? He allocates it here and
frees it at the end of the function again.
> This should be GFP_KERNEL - the page you're allocating here is not
> reclaimable by the VM.
>
The patch leader documented how this works although you're right in that the
code doesn't explain it adequately. Temporary and short-lived allocations
are grouped with kernel-reclaimable pages such as inode and dcache pages
intentionally.
> > + if (!page)
> > + goto out;
> > +
> > + ret = 0;
> > +
> > + mm = get_task_mm(task);
> > + if (!mm)
> > + goto out_free;
> > +
> > + max_len = (count > PAGE_SIZE) ? PAGE_SIZE : count;
> > +
> > + while (count > 0) {
> > + int this_len, retval;
> > +
> > + this_len = mm->env_end - (mm->env_start + src);
> > +
> > + if (this_len <= 0)
> > + break;
> > +
> > + if (this_len > max_len)
> > + this_len = max_len;
> > +
> > + retval = access_process_vm(task, (mm->env_start + src),
> > + page, this_len, 0);
> > +
> > + if (retval <= 0) {
> > + ret = retval;
> > + break;
> > + }
> > +
> > + if (copy_to_user(buf, page, retval)) {
> > + ret = -EFAULT;
> > + break;
> > + }
> > +
> > + ret += retval;
> > + src += retval;
> > + buf += retval;
> > + count -= retval;
> > + }
>
> Now that's a funky loop. Someone please convince me that there is no way
> in which `count - retval' can ever go negative (ie: huge positive).
>
>
> > + *ppos = src;
> > +
> > + mmput(mm);
> > +out_free:
> > + free_page((unsigned long) page);
> > +out:
> > + put_task_struct(task);
> > +out_no_task:
> > + return ret;
> > +}
> > +
> > +static const struct file_operations proc_environ_operations = {
> > + .read = environ_read,
> > +};
> > +
> > static ssize_t oom_adjust_read(struct file *file, char __user *buf,
> > size_t count, loff_t *ppos)
> > {
> > @@ -2092,7 +2144,7 @@ static const struct pid_entry tgid_base_
> > DIR("task", S_IRUGO|S_IXUGO, task),
> > DIR("fd", S_IRUSR|S_IXUSR, fd),
> > DIR("fdinfo", S_IRUSR|S_IXUSR, fdinfo),
> > - INF("environ", S_IRUSR, pid_environ),
> > + REG("environ", S_IRUSR, environ),
> > INF("auxv", S_IRUSR, pid_auxv),
> > INF("status", S_IRUGO, pid_status),
> > INF("limits", S_IRUSR, pid_limits),
> > @@ -2421,7 +2473,7 @@ out_no_task:
> > static const struct pid_entry tid_base_stuff[] = {
> > DIR("fd", S_IRUSR|S_IXUSR, fd),
> > DIR("fdinfo", S_IRUSR|S_IXUSR, fdinfo),
> > - INF("environ", S_IRUSR, pid_environ),
> > + REG("environ", S_IRUSR, environ),
> > INF("auxv", S_IRUSR, pid_auxv),
> > INF("status", S_IRUGO, pid_status),
> > INF("limits", S_IRUSR, pid_limits),
>
--
--
Mel Gorman
Part-time Phd Student Linux Technology Center
University of Limerick IBM Dublin Software Lab
> >>+
> >>+ if (copy_to_user(buf, page, retval)) {
> >
> > ^^^^
> > shouldn't you only copy min(count,retval) bytes? otherwise you could
> > write beyond the users buffer "buf", right?
>
> AFAIK, 'retval' can never be greater than 'this_len', which can never be
> greater than 'max_len', which can never be greater than 'count'
I think that's not true. 'count' is changing through the iteration.
The difference in the mem_read():
* while (count > 0) {
* int this_len, retval;
*
* this_len = (count > PAGE_SIZE) ? PAGE_SIZE : count;
* retval = access_process_vm(task, src, page, this_len, 0);
*
* ...
* }
is the fact, that this_len = min(PAGE_SIZE, count) is in the
iteration block, hence retval <= this_len <= count in each iteration
step. So this is ok. But IMHO in your code 'retval' may be bigger than
'count' in the last iteration of the block, because 'max_len' is fix
through your iteration but 'count' is changing. Or am i missing
something?
> James Pearson
Arvin
Arvin Moezzi wrote:
> I think that's not true. 'count' is changing through the iteration.
> The difference in the mem_read():
>
> * while (count > 0) {
> * int this_len, retval;
> *
> * this_len = (count > PAGE_SIZE) ? PAGE_SIZE : count;
> * retval = access_process_vm(task, src, page, this_len, 0);
> *
> * ...
> * }
>
> is the fact, that this_len = min(PAGE_SIZE, count) is in the
> iteration block, hence retval <= this_len <= count in each iteration
> step. So this is ok. But IMHO in your code 'retval' may be bigger than
> 'count' in the last iteration of the block, because 'max_len' is fix
> through your iteration but 'count' is changing. Or am i missing
> something?
Yes, you are correct ...
Thanks
James Pearson
James Pearson wrote:
> Arvin Moezzi wrote:
>
>> I think that's not true. 'count' is changing through the iteration.
>> The difference in the mem_read():
>>
>> * while (count > 0) {
>> * int this_len, retval;
>> *
>> * this_len = (count > PAGE_SIZE) ? PAGE_SIZE : count;
>> * retval = access_process_vm(task, src, page, this_len, 0);
>> *
>> * ...
>> * }
>>
>> is the fact, that this_len = min(PAGE_SIZE, count) is in the
>> iteration block, hence retval <= this_len <= count in each iteration
>> step. So this is ok. But IMHO in your code 'retval' may be bigger than
>> 'count' in the last iteration of the block, because 'max_len' is fix
>> through your iteration but 'count' is changing. Or am i missing
>> something?
>
>
> Yes, you are correct ...
Here is a new patch that fixes the above issue ...
However, I'm not sure if I should be using GFP_TEMPORARY, GFP_KERNEL or
GFP_USER ?
Thanks
James Pearson
Patch against 2.6.23-rc6-mm1
---
/proc/PID/environ currently truncates at 4096 characters, patch based on
the /proc/PID/mem code.
Signed-off-by: James Pearson <[email protected]>
--- ./fs/proc/base.c.dist 2007-09-19 12:29:46.244929651 +0100
+++ ./fs/proc/base.c 2007-09-25 12:40:53.194497911 +0100
@@ -202,27 +202,6 @@ static int proc_root_link(struct inode *
(task->state == TASK_STOPPED || task->state == TASK_TRACED) && \
security_ptrace(current,task) == 0))
-static int proc_pid_environ(struct task_struct *task, char * buffer)
-{
- int res = 0;
- struct mm_struct *mm = get_task_mm(task);
- if (mm) {
- 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);
-out:
- mmput(mm);
- }
- return res;
-}
-
static int proc_pid_cmdline(struct task_struct *task, char * buffer)
{
int res = 0;
@@ -740,6 +719,76 @@ static const struct file_operations proc
.open = mem_open,
};
+static ssize_t environ_read(struct file *file, char __user *buf,
+ size_t count, loff_t *ppos)
+{
+ struct task_struct *task = get_proc_task(file->f_dentry->d_inode);
+ char *page;
+ unsigned long src = *ppos;
+ int ret = -ESRCH;
+ struct mm_struct *mm;
+
+ if (!task)
+ goto out_no_task;
+
+ if (!ptrace_may_attach(task))
+ goto out;
+
+ ret = -ENOMEM;
+ page = (char *)__get_free_page(GFP_TEMPORARY);
+ if (!page)
+ goto out;
+
+ ret = 0;
+
+ mm = get_task_mm(task);
+ if (!mm)
+ goto out_free;
+
+ while (count > 0) {
+ int this_len, retval, max_len;
+
+ this_len = mm->env_end - (mm->env_start + src);
+
+ if (this_len <= 0)
+ break;
+
+ max_len = (count > PAGE_SIZE) ? PAGE_SIZE : count;
+ this_len = (this_len > max_len) ? max_len : this_len;
+
+ retval = access_process_vm(task, (mm->env_start + src),
+ page, this_len, 0);
+
+ if (retval <= 0) {
+ ret = retval;
+ break;
+ }
+
+ if (copy_to_user(buf, page, retval)) {
+ ret = -EFAULT;
+ break;
+ }
+
+ ret += retval;
+ src += retval;
+ buf += retval;
+ count -= retval;
+ }
+ *ppos = src;
+
+ mmput(mm);
+out_free:
+ free_page((unsigned long) page);
+out:
+ put_task_struct(task);
+out_no_task:
+ return ret;
+}
+
+static const struct file_operations proc_environ_operations = {
+ .read = environ_read,
+};
+
static ssize_t oom_adjust_read(struct file *file, char __user *buf,
size_t count, loff_t *ppos)
{
@@ -2092,7 +2141,7 @@ static const struct pid_entry tgid_base_
DIR("task", S_IRUGO|S_IXUGO, task),
DIR("fd", S_IRUSR|S_IXUSR, fd),
DIR("fdinfo", S_IRUSR|S_IXUSR, fdinfo),
- INF("environ", S_IRUSR, pid_environ),
+ REG("environ", S_IRUSR, environ),
INF("auxv", S_IRUSR, pid_auxv),
INF("status", S_IRUGO, pid_status),
INF("limits", S_IRUSR, pid_limits),
@@ -2421,7 +2470,7 @@ out_no_task:
static const struct pid_entry tid_base_stuff[] = {
DIR("fd", S_IRUSR|S_IXUSR, fd),
DIR("fdinfo", S_IRUSR|S_IXUSR, fdinfo),
- INF("environ", S_IRUSR, pid_environ),
+ REG("environ", S_IRUSR, environ),
INF("auxv", S_IRUSR, pid_auxv),
INF("status", S_IRUGO, pid_status),
INF("limits", S_IRUSR, pid_limits),