linux-kernel - Re: [PATCH v6] Added PR_SET_PROCTITLE

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue,  8 Dec 2009 14:10:19 +0900 (JST)
From:	KOSAKI Motohiro <kosaki.motohiro@...fujitsu.com>
To:	Ingo Molnar <mingo@...e.hu>
Cc:	kosaki.motohiro@...fujitsu.com, Bryan Donlan <bdonlan@...il.com>,
	Ulrich Drepper <drepper@...hat.com>,
	Timo Sirainen <tss@....fi>,
	WANG Cong <xiyou.wangcong@...il.com>,
	Oleg Nesterov <oleg@...hat.com>,
	LKML <linux-kernel@...r.kernel.org>,
	Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH v6] Added PR_SET_PROCTITLE_AREA option for prctl()

> 
> * KOSAKI Motohiro <kosaki.motohiro@...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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/