From: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
To: Ingo Molnar <mingo@elte.hu>
Subject: Re: [PATCH v6] Added PR_SET_PROCTITLE_AREA option for prctl()
Cc: kosaki.motohiro@jp.fujitsu.com, Bryan Donlan <bdonlan@gmail.com>,
       Ulrich Drepper <drepper@redhat.com>, Timo Sirainen <tss@iki.fi>,
       WANG Cong <xiyou.wangcong@gmail.com>, Oleg Nesterov <oleg@redhat.com>,
       LKML <linux-kernel@vger.kernel.org>,
       Andrew Morton <akpm@linux-foundation.org>
In-Reply-To: <20091208044605.GA32669@elte.hu>
References: <20091208121145.B58D.A69D9226@jp.fujitsu.com> <20091208044605.GA32669@elte.hu>
Message-Id: <20091208135010.B5A8.A69D9226@jp.fujitsu.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="US-ASCII"
Content-Transfer-Encoding: 7bit
Date: Tue,  8 Dec 2009 14:10:19 +0900 (JST)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2346
Lines: 79

> 
> * 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/

Thanks. I'll fix.

> 
> > +		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'.

Will fix.

> 
> > +			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.

ok.

> > +
> > +			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.

I'll do.


> 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.

No. It's impossible.
/proc/{pid}/cmdline read user process's memory. iow, this prctl() don't
receive string, it receive virtual address itself. I don't want any task
allow to change another task's memory except ptrace.

Plus, glibc don't need such capability. setproctitle() can only cahnge
own task's title.

Of cource, we can change argv is held in kernel structure (e.g. task_struct or
mm_struct). but it increase kernel memory footprint and I'm not sure its worth.


--
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/