[PATCH -mm] Don't truncate /proc/PID/environ at 4096 characters

[PATCH -mm] Don't truncate /proc/PID/environ at 4096 characters

[James Pearson]

From: James Pearson <james-p@moving-picture.com>

/proc/PID/environ currently truncates at 4096 characters, patch based on 
the /proc/PID/mem code.

Signed-off-by: James Pearson <james-p@moving-picture.com>

---
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),

Re: [PATCH -mm] Don't truncate /proc/PID/environ at 4096 characters

[Mikael Pettersson]

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

Re: [PATCH -mm] Don't truncate /proc/PID/environ at 4096 characters

[H. Peter Anvin]

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

Re: [PATCH -mm] Don't truncate /proc/PID/environ at 4096 characters

[Mikael Pettersson]

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 :-(

Re: [PATCH -mm] Don't truncate /proc/PID/environ at 4096 characters

[Hugh Dickins]

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

Re: [PATCH -mm] Don't truncate /proc/PID/environ at 4096 characters

[Andrew Morton]

On Wed, 19 Sep 2007 14:35:29 +0100
"James Pearson" <james-p@moving-picture.com> wrote:

> 
> From: James Pearson <james-p@moving-picture.com>
> 
> /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 <james-p@moving-picture.com>
> 
> --- ./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),

Re: [PATCH -mm] Don't truncate /proc/PID/environ at 4096 characters

[Arvin Moezzi]

2007/9/19, James Pearson <james-p@moving-picture.com>:
> +       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

Re: [PATCH -mm] Don't truncate /proc/PID/environ at 4096 characters

[James Pearson]

Andrew Morton wrote:
> On Wed, 19 Sep 2007 14:35:29 +0100
> "James Pearson" <james-p@moving-picture.com> wrote:
> 
> 
>>From: James Pearson <james-p@moving-picture.com>
>>
>>/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 <james-p@moving-picture.com>
>>
>>--- ./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

Re: [PATCH -mm] Don't truncate /proc/PID/environ at 4096 characters

[James Pearson]

Arvin Moezzi wrote:
> 2007/9/19, James Pearson <james-p@moving-picture.com>:
> 
>>+       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

Re: [PATCH -mm] Don't truncate /proc/PID/environ at 4096 characters

[Mel Gorman]

On (20/09/07 16:46), Andrew Morton didst pronounce:
> On Wed, 19 Sep 2007 14:35:29 +0100
> "James Pearson" <james-p@moving-picture.com> wrote:
> 
> > 
> > From: James Pearson <james-p@moving-picture.com>
> > 
> > /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 <james-p@moving-picture.com>
> > 
> > --- ./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

Re: [PATCH -mm] Don't truncate /proc/PID/environ at 4096 characters

[Arvin Moezzi]

> >>+
> >>+               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

Re: [PATCH -mm] Don't truncate /proc/PID/environ at 4096 characters

[James Pearson]

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

Re: [PATCH -mm] Don't truncate /proc/PID/environ at 4096 characters

[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 <james-p@moving-picture.com>

--- ./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),