| 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: <20070921100055.GA3562@skynet.ie>
Date: Fri, 21 Sep 2007 11:00:56 +0100
From: mel@...net.ie (Mel Gorman)
To: Andrew Morton <akpm@...ux-foundation.org>
Cc: James Pearson <james-p@...ing-picture.com>,
linux-kernel@...r.kernel.org, aarapov@...hat.com, hpa@...or.com
Subject: Re: [PATCH -mm] Don't truncate /proc/PID/environ at 4096 characters
On (20/09/07 16:46), Andrew Morton didst pronounce:
> 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.
>
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
-
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