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@xxxxxxxxxxxxxxxxxx> wrote:
>
>
> > From: James Pearson <james-p@xxxxxxxxxxxxxxxxxx>
> >
> > /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@xxxxxxxxxxxxxxxxxx>
> >
> > --- ./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

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/