Date: Thu, 20 Sep 2007 16:46:32 -0700
From: Andrew Morton <akpm@linux-foundation.org>
To: "James Pearson" <james-p@moving-picture.com>
Cc: linux-kernel@vger.kernel.org, aarapov@redhat.com, hpa@zytor.com,
       Mel Gorman <mel@csn.ul.ie>
Subject: Re: [PATCH -mm] Don't truncate /proc/PID/environ at 4096 characters
Message-Id: <20070920164632.fd440b78.akpm@linux-foundation.org>
In-Reply-To: <E1IXziD-0003YJ-0w@moving-picture.com>
References: <E1IXziD-0003YJ-0w@moving-picture.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4385
Lines: 159

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),
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/