Messages in this thread | | | Date | Tue, 8 Dec 2009 05:46:05 +0100 | From | Ingo Molnar <> | Subject | Re: [PATCH v6] Added PR_SET_PROCTITLE_AREA option for prctl() |
| |
* KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com> wrote:
> + /* > + * If argv and environ aren't continuous (i.e. the process used > + * prctl(PR_SET_PROCTITLE_AREA)), we don't care environ override.
s/dont't care environ override/don't care about the evironment override/
> + case PR_SET_PROCTITLE_AREA: { > + struct mm_struct *mm = current->mm; > + unsigned long addr = arg2; > + unsigned long len = arg3; > + unsigned long end = arg2 + arg3;
would be cleaner to write the latter as 'addr + len'.
> + if (len > PAGE_SIZE) > + return -EINVAL; > + > + if (addr >= end) > + return -EINVAL; > + > + /* > + * If the process pass broken pointer, EFAULT is might better > + * than ps output zero-length proctitle. Plus if > + * the process pass kernel address (or something-else), > + * We have to block it. Oherwise, strange exploit > + * chance is there. > + */ > + if (!access_ok(VERIFY_READ, addr, len)) > + return -EFAULT;
the addr >= end check looks (partly) duplicative of the access_ok() check.
> + > + down_write(&mm->mmap_sem); > + mm->arg_start = addr; > + mm->arg_end = end; > + up_write(&mm->mmap_sem);
well we might as well name 'addr' as 'start' and have a match then here too.
The feature looks useful, but the choice of a prctl as an API is strange - it limits us to the current task only - while the ability to set arguments for another task looks a more generic (and potentially more useful) solution.
Ingo
| |