| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <20070920164632.fd440b78.akpm@linux-foundation.org>
Date: Thu, 20 Sep 2007 16:46:32 -0700
From: Andrew Morton <akpm@...ux-foundation.org>
To: "James Pearson" <james-p@...ing-picture.com>
Cc: linux-kernel@...r.kernel.org, aarapov@...hat.com, hpa@...or.com,
Mel Gorman <mel@....ul.ie>
Subject: Re: [PATCH -mm] Don't truncate /proc/PID/environ at 4096 characters
On Wed, 19 Sep 2007 14:35:29 +0100
"James Pearson" <james-p@...ing-picture.com> wrote:
>
> From: James Pearson <james-p@...ing-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@...ing-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@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists