[PATCH] Added PR_SET_PROCTITLE_AREA option for prctl()

[PATCH] Added PR_SET_PROCTITLE_AREA option for prctl()

[Timo Sirainen]

PR_SET_PROCTITLE_AREA updates mm_struct->arg_start and arg_end to the
given pointers, which makes it possible for user space to implement
setproctitle(3) cleanly.


Signed-off-by: Timo Sirainen <tss@iki.fi>
---
 fs/proc/base.c        |    9 ++++++---
 include/linux/prctl.h |    2 ++
 kernel/sys.c          |   10 ++++++++++
 3 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 837469a..43ef276 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -267,9 +267,12 @@ static int proc_pid_cmdline(struct task_struct *task, char * buffer)
  
 	res = access_process_vm(task, mm->arg_start, buffer, len, 0);
 
-	// If the nul at the end of args has been overwritten, then
-	// assume application is using setproctitle(3).
-	if (res > 0 && buffer[res-1] != '\0' && len < PAGE_SIZE) {
+	if (mm->arg_end != mm->env_start) {
+		// PR_SET_PROCTITLE_AREA used
+		res = strnlen(buffer, res);
+	} else if (res > 0 && buffer[res-1] != '\0' && len < PAGE_SIZE) {
+		// If the nul at the end of args has been overwritten, then
+		// assume application is using old style setproctitle(3).
 		len = strnlen(buffer, res);
 		if (len < res) {
 		    res = len;
diff --git a/include/linux/prctl.h b/include/linux/prctl.h
index 9311505..c79ccc8 100644
--- a/include/linux/prctl.h
+++ b/include/linux/prctl.h
@@ -90,4 +90,6 @@
 
 #define PR_MCE_KILL	33
 
+#define PR_SET_PROCTITLE_AREA 34	/* Set process title memory area */
+
 #endif /* _LINUX_PRCTL_H */
diff --git a/kernel/sys.c b/kernel/sys.c
index 255475d..c216ae6 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -1564,6 +1564,16 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
 			error = 0;
 			break;
 
+		case PR_SET_PROCTITLE_AREA: {
+			struct mm_struct *mm = current->mm;
+
+			if (arg2 >= arg3)
+				return -EINVAL;
+
+			mm->arg_start = arg2;
+			mm->arg_end = arg3;
+			return 0;
+		}
 		default:
 			error = -EINVAL;
 			break;
-- 
1.6.3.3

Re: [PATCH] Added PR_SET_PROCTITLE_AREA option for prctl()

[Bryan Donlan]

On Fri, Oct 2, 2009 at 5:23 PM, Timo Sirainen <tss@iki.fi> wrote:
> PR_SET_PROCTITLE_AREA updates mm_struct->arg_start and arg_end to the
> given pointers, which makes it possible for user space to implement
> setproctitle(3) cleanly.


> @@ -267,9 +267,12 @@ static int proc_pid_cmdline(struct task_struct *task, char * buffer)
>
>        res = access_process_vm(task, mm->arg_start, buffer, len, 0);
>
> -       // If the nul at the end of args has been overwritten, then
> -       // assume application is using setproctitle(3).
> -       if (res > 0 && buffer[res-1] != '\0' && len < PAGE_SIZE) {
> +       if (mm->arg_end != mm->env_start) {
> +               // PR_SET_PROCTITLE_AREA used
> +               res = strnlen(buffer, res);

Is this check really needed? Surely it's enough to simply state that
behavior if the area isn't null-terminated is undefined.

> +       } else if (res > 0 && buffer[res-1] != '\0' && len < PAGE_SIZE) {
> +               // If the nul at the end of args has been overwritten, then
> +               // assume application is using old style setproctitle(3).
>                len = strnlen(buffer, res);
>                if (len < res) {
>                    res = len;

Might want to fix the bug later on in that function while you're in
here - the second access_process_vm call is never checked for errors,
but (from my reading) it's possible that the page that the environment
is on could be unmapped between those two calls. The result could
either be a short read (not the end of the world) or a negative value
(error code + small original argument length) passed to strnlen.

That said, come to think of it, I'm not actually sure if this prctl
stuff is strictly necessary. Wouldn't it be enough for glibc to copy
the environment somewhere safe, and then have the kernel guarantee a
full PAGE_SIZE between arg_start and env_end, even if this means
padding out the environment? The process could then measure to make
sure it has this much space (in case of running on an old kernel) by
testing the difference between arg_start and the top of the stack, or
an auxiliary vector could be passed down from the kernel with the
maximum proctitle length.

Re: [PATCH] Added PR_SET_PROCTITLE_AREA option for prctl()

[Timo Sirainen]

On Oct 2, 2009, at 10:01 PM, Bryan Donlan wrote:

> On Fri, Oct 2, 2009 at 5:23 PM, Timo Sirainen <tss@iki.fi> wrote:
>> PR_SET_PROCTITLE_AREA updates mm_struct->arg_start and arg_end to the
>> given pointers, which makes it possible for user space to implement
>> setproctitle(3) cleanly.
>
>
>> @@ -267,9 +267,12 @@ static int proc_pid_cmdline(struct task_struct  
>> *task, char * buffer)
>>
>>        res = access_process_vm(task, mm->arg_start, buffer, len, 0);
>>
>> -       // If the nul at the end of args has been overwritten, then
>> -       // assume application is using setproctitle(3).
>> -       if (res > 0 && buffer[res-1] != '\0' && len < PAGE_SIZE) {
>> +       if (mm->arg_end != mm->env_start) {
>> +               // PR_SET_PROCTITLE_AREA used
>> +               res = strnlen(buffer, res);
>
> Is this check really needed? Surely it's enough to simply state that
> behavior if the area isn't null-terminated is undefined.

Well, that depends. I was hoping to use the syscall only once per  
process. That would allow me to just update the process title whenever  
I feel like it, possibly hundreds of times per second. This is much  
cheaper if I don't have to use a syscall every time.

So if I'm setting the PR_SET_PROCTITLE_AREA initially to e.g. 1 kB  
memory area, without the above code ps will show it entirely  
regardless of any \0 characters (because parameters are separated by  
\0).

>> +       } else if (res > 0 && buffer[res-1] != '\0' && len <  
>> PAGE_SIZE) {
>> +               // If the nul at the end of args has been  
>> overwritten, then
>> +               // assume application is using old style  
>> setproctitle(3).
>>                len = strnlen(buffer, res);
>>                if (len < res) {
>>                    res = len;
>
> Might want to fix the bug later on in that function while you're in
> here - the second access_process_vm call is never checked for errors,
> but (from my reading) it's possible that the page that the environment
> is on could be unmapped between those two calls. The result could
> either be a short read (not the end of the world) or a negative value
> (error code + small original argument length) passed to strnlen.

Hmm. Originally I thought it would have returned only -1, but if it's - 
errno then I'm beginning to wonder if this is a security hole. If the  
original res is small enough, and it looks like it can be, that code  
could get res to negative, i.e. unlimited. But I can't follow the code  
right now if it also means that userspace can read tons of data or if  
it gets caught by some "< 0" check.

> That said, come to think of it, I'm not actually sure if this prctl
> stuff is strictly necessary. Wouldn't it be enough for glibc to copy
> the environment somewhere safe, and then have the kernel guarantee a
> full PAGE_SIZE between arg_start and env_end, even if this means
> padding out the environment? The process could then measure to make
> sure it has this much space (in case of running on an old kernel) by
> testing the difference between arg_start and the top of the stack, or
> an auxiliary vector could be passed down from the kernel with the
> maximum proctitle length.

This would get around the potential "not enough space" problem, but  
not really the ugliness. I can't really think of other potential  
problems with it right now, but my main concern is actually getting  
setproctitle() to glibc. Based on Ulrich's previous reply to me I  
don't know if he'd be willing to accept that solution: http://sources.redhat.com/ml/libc-alpha/2009-10/msg00000.html

Re: [PATCH] Added PR_SET_PROCTITLE_AREA option for prctl()

[Bryan Donlan]

On Fri, Oct 2, 2009 at 10:47 PM, Timo Sirainen <tss@iki.fi> wrote:

>>> -       // If the nul at the end of args has been overwritten, then
>>> -       // assume application is using setproctitle(3).
>>> -       if (res > 0 && buffer[res-1] != '\0' && len < PAGE_SIZE) {
>>> +       if (mm->arg_end != mm->env_start) {
>>> +               // PR_SET_PROCTITLE_AREA used
>>> +               res = strnlen(buffer, res);
>>
>> Is this check really needed? Surely it's enough to simply state that
>> behavior if the area isn't null-terminated is undefined.
>
> Well, that depends. I was hoping to use the syscall only once per process.
> That would allow me to just update the process title whenever I feel like
> it, possibly hundreds of times per second. This is much cheaper if I don't
> have to use a syscall every time.
>
> So if I'm setting the PR_SET_PROCTITLE_AREA initially to e.g. 1 kB memory
> area, without the above code ps will show it entirely regardless of any \0
> characters (because parameters are separated by \0).

That makes sense - but note that it's not completely atomic still -
with a syscall you could insert some kind of barrier (rwsem?) to
ensure other processes don't see a halfway updated process name. With
infrequent updates this isn't a problem, but if you're really
intending to update it at a rate where syscall overhead becomes a
problem, then you're also going to see these kinds of issues as well.

>
>>> +       } else if (res > 0 && buffer[res-1] != '\0' && len < PAGE_SIZE) {
>>> +               // If the nul at the end of args has been overwritten,
>>> then
>>> +               // assume application is using old style setproctitle(3).
>>>               len = strnlen(buffer, res);
>>>               if (len < res) {
>>>                   res = len;
>>
>> Might want to fix the bug later on in that function while you're in
>> here - the second access_process_vm call is never checked for errors,
>> but (from my reading) it's possible that the page that the environment
>> is on could be unmapped between those two calls. The result could
>> either be a short read (not the end of the world) or a negative value
>> (error code + small original argument length) passed to strnlen.
>
> Hmm. Originally I thought it would have returned only -1, but if it's -errno
> then I'm beginning to wonder if this is a security hole. If the original res
> is small enough, and it looks like it can be, that code could get res to
> negative, i.e. unlimited. But I can't follow the code right now if it also
> means that userspace can read tons of data or if it gets caught by some "<
> 0" check.

By the time we get to the second read, len is definitely between 0 and
PAGE_SIZE, so that's not a problem. What I'm worried about mostly is
whether strnlen would interpret its argument (negative error + small
positive value = negative value) as a large positive value, go running
off into the woods and cause an OOPS. Which I suppose is a denial of
service issue.

That said, I'm not really sure why it's written the way it is anyway.
Why not just unconditionally try to load all the way from arg_start to
env_end (or to arg_start + PAGE_SIZE), then just figure out where the
\0 is and you're done? It would seem to have the same effect...

>> That said, come to think of it, I'm not actually sure if this prctl
>> stuff is strictly necessary. Wouldn't it be enough for glibc to copy
>> the environment somewhere safe, and then have the kernel guarantee a
>> full PAGE_SIZE between arg_start and env_end, even if this means
>> padding out the environment? The process could then measure to make
>> sure it has this much space (in case of running on an old kernel) by
>> testing the difference between arg_start and the top of the stack, or
>> an auxiliary vector could be passed down from the kernel with the
>> maximum proctitle length.
>
> This would get around the potential "not enough space" problem, but not
> really the ugliness. I can't really think of other potential problems with
> it right now, but my main concern is actually getting setproctitle() to
> glibc. Based on Ulrich's previous reply to me I don't know if he'd be
> willing to accept that solution:
> http://sources.redhat.com/ml/libc-alpha/2009-10/msg00000.html

I don't think it's a particularly ugly solution - all it does is
canonizes what programs are _already doing_, while at the same time
giving more precise and useful guarantees. This means every old
program which might, in theory, have been unreliable previously (due
to assuming that its argument vector would be long enough) is now
magically safe. And since if all goes well it'll get wrapped up in a
nice C library wrapper eventually anyway, minor low-level ugliness
shouldn't be a problem...

Re: [PATCH] Added PR_SET_PROCTITLE_AREA option for prctl()

[Timo Sirainen]

On Oct 2, 2009, at 10:59 PM, Bryan Donlan wrote:

>> So if I'm setting the PR_SET_PROCTITLE_AREA initially to e.g. 1 kB  
>> memory
>> area, without the above code ps will show it entirely regardless of  
>> any \0
>> characters (because parameters are separated by \0).
>
> That makes sense - but note that it's not completely atomic still -
> with a syscall you could insert some kind of barrier (rwsem?) to
> ensure other processes don't see a halfway updated process name. With
> infrequent updates this isn't a problem, but if you're really
> intending to update it at a rate where syscall overhead becomes a
> problem, then you're also going to see these kinds of issues as well.

I'm not worried about atomicity. Typically programs (like mine) would  
still show their executable name first and then followed by human  
readable status text. If the human readable text is corrupted once in  
a while (and practically never), I doubt anyone cares. In my case it  
would be an IMAP server and I was just thinking of maybe updating the  
process title to show what command is being executed (or some other  
long running task is going on). Then if there's a load problem or  
something the admin could easily check what some process that's been  
stuck for several minutes is doing. So it's not important to show the  
process title for those that rapidly change it, but for those that  
have been stuck for a while.

>>> Might want to fix the bug later on in that function while you're in
>>> here - the second access_process_vm call is never checked for  
>>> errors,
>>> but (from my reading) it's possible that the page that the  
>>> environment
>>> is on could be unmapped between those two calls. The result could
>>> either be a short read (not the end of the world) or a negative  
>>> value
>>> (error code + small original argument length) passed to strnlen.
>>
>> Hmm. Originally I thought it would have returned only -1, but if  
>> it's -errno
>> then I'm beginning to wonder if this is a security hole. If the  
>> original res
>> is small enough, and it looks like it can be, that code could get  
>> res to
>> negative, i.e. unlimited. But I can't follow the code right now if  
>> it also
>> means that userspace can read tons of data or if it gets caught by  
>> some "<
>> 0" check.
>
> By the time we get to the second read, len is definitely between 0 and
> PAGE_SIZE, so that's not a problem. What I'm worried about mostly is
> whether strnlen would interpret its argument (negative error + small
> positive value = negative value) as a large positive value,

Right.

> go running
> off into the woods and cause an OOPS. Which I suppose is a denial of
> service issue.

I was more thinking that strnlen() would see \0, but not before it had  
returned some sensitive information that shouldn't have been returned  
to userspace.

> That said, I'm not really sure why it's written the way it is anyway.
> Why not just unconditionally try to load all the way from arg_start to
> env_end (or to arg_start + PAGE_SIZE), then just figure out where the
> \0 is and you're done? It would seem to have the same effect...

I guess some minor performance benefit, since typically programs won't  
change their process title so the second access is rare.

Re: [PATCH] Added PR_SET_PROCTITLE_AREA option for prctl()

[matthieu castet]

Timo Sirainen <tss <at> iki.fi> writes:

> 
> PR_SET_PROCTITLE_AREA updates mm_struct->arg_start and arg_end to the
> given pointers, which makes it possible for user space to implement
> setproctitle(3) cleanly.
> 
>
Why can't you use PR_SET_NAME ?

And with PR_SET_NAME, you can a name per thread, which you can't do by changing
arg[0] name (it should be shared across all thread).

Matthieu

Re: [PATCH] Added PR_SET_PROCTITLE_AREA option for prctl()

[KOSAKI Motohiro]

Hi


>>>> -       // If the nul at the end of args has been overwritten, then
>>>> -       // assume application is using setproctitle(3).
>>>> -       if (res > 0 && buffer[res-1] != '\0' && len < PAGE_SIZE) {
>>>> +       if (mm->arg_end != mm->env_start) {
>>>> +               // PR_SET_PROCTITLE_AREA used
>>>> +               res = strnlen(buffer, res);
>>>
>>> Is this check really needed? Surely it's enough to simply state that
>>> behavior if the area isn't null-terminated is undefined.
>>
>> Well, that depends. I was hoping to use the syscall only once per process.
>> That would allow me to just update the process title whenever I feel like
>> it, possibly hundreds of times per second. This is much cheaper if I don't
>> have to use a syscall every time.
>>
>> So if I'm setting the PR_SET_PROCTITLE_AREA initially to e.g. 1 kB memory
>> area, without the above code ps will show it entirely regardless of any \0
>> characters (because parameters are separated by \0).
>
> That makes sense - but note that it's not completely atomic still -
> with a syscall you could insert some kind of barrier (rwsem?) to
> ensure other processes don't see a halfway updated process name. With
> infrequent updates this isn't a problem, but if you're really
> intending to update it at a rate where syscall overhead becomes a
> problem, then you're also going to see these kinds of issues as well.


Yes.  this patch seems buggy. The lock is necessary.
following scenario makes disaster, I think.

CPU0 (prctl caller)                                CPU1 (ps)
-------------------------------------------------------------------------------------------
mm->arg_start = arg2;
                                                                len =
mm->arg_end - mm->arg_start; in proc_pid_cmdline()
mm->arg_end = arg3;

Re: [PATCH] Added PR_SET_PROCTITLE_AREA option for prctl()

[KOSAKI Motohiro]

>> PR_SET_PROCTITLE_AREA updates mm_struct->arg_start and arg_end to the
>> given pointers, which makes it possible for user space to implement
>> setproctitle(3) cleanly.
>>
>>
> Why can't you use PR_SET_NAME ?

example:

% ps -ef |grep sendmail
root      2444     1  0 Oct04 ?        00:00:00 sendmail: accepting connections
smmsp     2454     1  0 Oct04 ?        00:00:00 sendmail: Queue
runner@01:00:00 for /var/spool/clientmqueue
kosaki    3927  2907  0 00:02 pts/1    00:00:00 grep sendmail



Almost setproctitle() heavy user (e.g. sendmail, openssh and some bsd
style daemon programs) use long string rather than 16.



> And with PR_SET_NAME, you can a name per thread, which you can't do by changing
> arg[0] name (it should be shared across all thread).
>
> Matthieu
>
> --
> 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/
>

Re: [PATCH] Added PR_SET_PROCTITLE_AREA option for prctl()

[Bryan Donlan]

On Sun, Oct 4, 2009 at 10:44 AM, KOSAKI Motohiro
<kosaki.motohiro@gmail.com> wrote:
> Hi
>
>
>>>>> -       // If the nul at the end of args has been overwritten, then
>>>>> -       // assume application is using setproctitle(3).
>>>>> -       if (res > 0 && buffer[res-1] != '\0' && len < PAGE_SIZE) {
>>>>> +       if (mm->arg_end != mm->env_start) {
>>>>> +               // PR_SET_PROCTITLE_AREA used
>>>>> +               res = strnlen(buffer, res);
>>>>
>>>> Is this check really needed? Surely it's enough to simply state that
>>>> behavior if the area isn't null-terminated is undefined.
>>>
>>> Well, that depends. I was hoping to use the syscall only once per process.
>>> That would allow me to just update the process title whenever I feel like
>>> it, possibly hundreds of times per second. This is much cheaper if I don't
>>> have to use a syscall every time.
>>>
>>> So if I'm setting the PR_SET_PROCTITLE_AREA initially to e.g. 1 kB memory
>>> area, without the above code ps will show it entirely regardless of any \0
>>> characters (because parameters are separated by \0).
>>
>> That makes sense - but note that it's not completely atomic still -
>> with a syscall you could insert some kind of barrier (rwsem?) to
>> ensure other processes don't see a halfway updated process name. With
>> infrequent updates this isn't a problem, but if you're really
>> intending to update it at a rate where syscall overhead becomes a
>> problem, then you're also going to see these kinds of issues as well.
>
>
> Yes.  this patch seems buggy. The lock is necessary.
> following scenario makes disaster, I think.
>
> CPU0 (prctl caller)                                CPU1 (ps)
> -------------------------------------------------------------------------------------------
> mm->arg_start = arg2;
>                                                                len =
> mm->arg_end - mm->arg_start; in proc_pid_cmdline()
> mm->arg_end = arg3;
>

Since len is unsigned and clamped to PAGE_SIZE, the worst this will do
is expose one page of userspace memory starting at the new value of
arg_start. Note that this can also happen currently, exposing one env
variable, when all NULs in the argument array have been overwritten,
but before the new NUL has been written (that is, there is no NUL
terminator from arg_start until the end of envp[0]).

It is, however, still undesirable, of course, particularly if
userspace isn't prepared for this kind of thing to happen.

Re: [PATCH] Added PR_SET_PROCTITLE_AREA option for prctl()

[KOSAKI Motohiro]

> On Sun, Oct 4, 2009 at 10:44 AM, KOSAKI Motohiro
> <kosaki.motohiro@gmail.com> wrote:
> > Hi
> >
> >
> >>>>> -       // If the nul at the end of args has been overwritten, then
> >>>>> -       // assume application is using setproctitle(3).
> >>>>> -       if (res > 0 && buffer[res-1] != '\0' && len < PAGE_SIZE) {
> >>>>> +       if (mm->arg_end != mm->env_start) {
> >>>>> +               // PR_SET_PROCTITLE_AREA used
> >>>>> +               res = strnlen(buffer, res);
> >>>>
> >>>> Is this check really needed? Surely it's enough to simply state that
> >>>> behavior if the area isn't null-terminated is undefined.
> >>>
> >>> Well, that depends. I was hoping to use the syscall only once per process.
> >>> That would allow me to just update the process title whenever I feel like
> >>> it, possibly hundreds of times per second. This is much cheaper if I don't
> >>> have to use a syscall every time.
> >>>
> >>> So if I'm setting the PR_SET_PROCTITLE_AREA initially to e.g. 1 kB memory
> >>> area, without the above code ps will show it entirely regardless of any \0
> >>> characters (because parameters are separated by \0).
> >>
> >> That makes sense - but note that it's not completely atomic still -
> >> with a syscall you could insert some kind of barrier (rwsem?) to
> >> ensure other processes don't see a halfway updated process name. With
> >> infrequent updates this isn't a problem, but if you're really
> >> intending to update it at a rate where syscall overhead becomes a
> >> problem, then you're also going to see these kinds of issues as well.
> >
> >
> > Yes.  this patch seems buggy. The lock is necessary.
> > following scenario makes disaster, I think.
> >
> > CPU0 (prctl caller)                                CPU1 (ps)
> > -------------------------------------------------------------------------------------------
> > mm->arg_start = arg2;
> >                                                                len =
> > mm->arg_end - mm->arg_start; in proc_pid_cmdline()
> > mm->arg_end = arg3;
> >
> 
> Since len is unsigned and clamped to PAGE_SIZE, the worst this will do
> is expose one page of userspace memory starting at the new value of
> arg_start. Note that this can also happen currently, exposing one env
> variable, when all NULs in the argument array have been overwritten,
> but before the new NUL has been written (that is, there is no NUL
> terminator from arg_start until the end of envp[0]).
> 
> It is, however, still undesirable, of course, particularly if
> userspace isn't prepared for this kind of thing to happen.

The improvement idea is here.


Changelog
  - Added task_lock() to prctl(PR_SET_PROCTITLE_AREA)
 -  Added small input sanity check to prctl(PR_SET_PROCTITLE_AREA)

======================================================================
Subject: [PATCH] Added PR_SET_PROCTITLE_AREA option for prctl()

Currently, glibc2 doesn't have setproctitle(3). it lead userland daemon
 to use brutal stack modification.

In the first implession, It seems no problem at all. however, carefully
inspection spot the weaknes of the way.

example:

 % ps -ef |grep avahi-daemon
 avahi     1679     1  0 09:20 ?        00:00:00 avahi-daemon: running [kosadesk.local]
 avahi     1680  1679  0 09:20 ?        00:00:00 avahi-daemon: chroot helper

 # cat /proc/1679/cmdline
 avahi-daemon: running [kosadesk.local]

ok. seems good.
but...

 # cat /proc/1679/environ
 adesk.local]

Doh, this daemon has overwritten envrionment string area.

Recently, many security folks is thinking information leak is
security risk. then many administrator unset almost environment
variable before starting the daemon.
  e.g.
    env - MINIMUM-NEED-VAR=foo /path/to/daemon

it mean, we don't have enough string space now.

Thus, this patch implement new prctl. prctl(PR_SET_PROCTITLE_AREA)
updates mm_struct->arg_start and arg_end to the given pointers,
which makes it possible for user space to implement
setproctitle(3) cleanly.


 test program:
 ================================================
 #define ERR(str) (perror(str), exit(1))

 void settitle(char* title){
         int err;

         err = prctl(34, title, strlen(title)+1);
         if (err < 0)
                 ERR("prctl ");
 }

 void main(void){
         long i;
         char buf[1024];

         for (i = 0; i < 10000000000LL; i++){
                 sprintf(buf, "loooooooooooooooooooooooong string %d",i);
                 settitle(buf);
         }
 }
 ==================================================

Cc: Bryan Donlan <bdonlan@gmail.com>
Cc: Ulrich Drepper <drepper@redhat.com>
Cc: Timo Sirainen <tss@iki.fi>
Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
---
 fs/proc/base.c        |   46 +++++++++++++++++++++++++++++++++++-----------
 include/linux/prctl.h |    4 ++++
 kernel/sys.c          |   23 +++++++++++++++++++++++
 3 files changed, 62 insertions(+), 11 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 6f742f6..f266482 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -254,22 +254,46 @@ static int proc_pid_cmdline(struct task_struct *task, char * buffer)
 {
 	int res = 0;
 	unsigned int len;
-	struct mm_struct *mm = get_task_mm(task);
-	if (!mm)
+	struct mm_struct *mm;
+	unsigned long arg_start;
+	unsigned long arg_end;
+
+	task_lock(task);
+	mm = task->mm;
+	if (task->flags & PF_KTHREAD)
+		mm = NULL;
+	if (!mm) {
+		task_unlock(task);
 		goto out;
-	if (!mm->arg_end)
-		goto out_mm;	/* Shh! No looking before we're done */
+	}
+
+	atomic_inc(&mm->mm_users);
+
+	/* task_lock protect mm->arg_{start,end} too */
+	arg_start = mm->arg_start;
+	arg_end   = mm->arg_end;
+	len = arg_end - arg_start;
+
+	task_unlock(task);
+
+	/* The process was not constructed yet? */
+	if (!arg_end)
+		goto out_mm;
 
- 	len = mm->arg_end - mm->arg_start;
- 
 	if (len > PAGE_SIZE)
 		len = PAGE_SIZE;
- 
-	res = access_process_vm(task, mm->arg_start, buffer, len, 0);
 
-	// If the nul at the end of args has been overwritten, then
-	// assume application is using setproctitle(3).
-	if (res > 0 && buffer[res-1] != '\0' && len < PAGE_SIZE) {
+	res = access_process_vm(task, arg_start, buffer, len, 0);
+
+	if (arg_end != mm->env_start)
+		/* PR_SET_PROCTITLE_AREA used */
+		res = strnlen(buffer, res);
+	else if (res > 0 && buffer[res-1] != '\0' && len < PAGE_SIZE) {
+		/*
+		 * If the nul at the end of args has been overwritten, then
+		 * assume application is using sendmail's SPT_REUSEARGV style
+		 * argv override.
+		 */
 		len = strnlen(buffer, res);
 		if (len < res) {
 		    res = len;
diff --git a/include/linux/prctl.h b/include/linux/prctl.h
index b00df4c..feffb17 100644
--- a/include/linux/prctl.h
+++ b/include/linux/prctl.h
@@ -88,4 +88,8 @@
 #define PR_TASK_PERF_COUNTERS_DISABLE		31
 #define PR_TASK_PERF_COUNTERS_ENABLE		32
 
+
+/* Set process title memory area for setproctitle() */
+#define PR_SET_PROCTITLE_AREA 34
+
 #endif /* _LINUX_PRCTL_H */
diff --git a/kernel/sys.c b/kernel/sys.c
index b3f1097..fe4fbf1 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -1528,6 +1528,29 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
 				current->timer_slack_ns = arg2;
 			error = 0;
 			break;
+		case PR_SET_PROCTITLE_AREA: {
+			struct mm_struct *mm = current->mm;
+			unsigned long addr = arg2;
+			unsigned long len = arg3;
+			unsigned long end = arg2 + arg3;
+
+			if (len > PAGE_SIZE)
+				return -EINVAL;
+
+			if (addr >= end)
+				return -EINVAL;
+
+			if (!access_ok(VERIFY_READ, addr, len)) {
+				return -EFAULT;
+			}
+
+			task_lock(current);
+			mm->arg_start = addr;
+			mm->arg_end = addr + len;
+			task_unlock(current);
+
+			return 0;
+		}
 		default:
 			error = -EINVAL;
 			break;
-- 
1.6.2.5

Re: [PATCH] Added PR_SET_PROCTITLE_AREA option for prctl()

[KOSAKI Motohiro]

> The improvement idea is here.
> 
> Changelog
>   - Added task_lock() to prctl(PR_SET_PROCTITLE_AREA)
>  -  Added small input sanity check to prctl(PR_SET_PROCTITLE_AREA)

Doh, task_lock() is obviously wrong. please forget this.

Re: [PATCH] Added PR_SET_PROCTITLE_AREA option for prctl()

[Bryan Donlan]

On Sun, Oct 4, 2009 at 9:38 PM, KOSAKI Motohiro
<kosaki.motohiro@jp.fujitsu.com> wrote:
>> The improvement idea is here.
>>
>> Changelog
>>   - Added task_lock() to prctl(PR_SET_PROCTITLE_AREA)
>>  -  Added small input sanity check to prctl(PR_SET_PROCTITLE_AREA)
>
> Doh, task_lock() is obviously wrong. please forget this.

As another note, in general I think we'd need to hold a lock over the
entire operation. After all, if userspace changes its PROCTITLE_AREA,
and then reuses the memory for something else, we have an information
leak.

Perhaps a simpler approach would simply be to add a generation
counter. Read it once at the start, barrier, then grab the title. Then
at the end, read the generation counter again. If the value changed,
we need to start over. Also, in this case, an error when reading the
target process' memory should be ignored and retried, as we may have
hit a race in which the target process unmapped the proctitle area
after changing it.

Re: [PATCH] Added PR_SET_PROCTITLE_AREA option for prctl()

[KOSAKI Motohiro]

> On Sun, Oct 4, 2009 at 9:38 PM, KOSAKI Motohiro
> <kosaki.motohiro@jp.fujitsu.com> wrote:
> >> The improvement idea is here.
> >>
> >> Changelog
> >>   - Added task_lock() to prctl(PR_SET_PROCTITLE_AREA)
> >>  -  Added small input sanity check to prctl(PR_SET_PROCTITLE_AREA)
> >
> > Doh, task_lock() is obviously wrong. please forget this.
> 
> As another note, in general I think we'd need to hold a lock over the
> entire operation. After all, if userspace changes its PROCTITLE_AREA,
> and then reuses the memory for something else, we have an information
> leak.

if reusing occur, it's obviously userland fault. I don't think we need to care this.
because current kernel also can be information leak by strcpy(argv[0], mypassword).

I think they are userland bug both.

 
> Perhaps a simpler approach would simply be to add a generation
> counter. Read it once at the start, barrier, then grab the title. Then
> at the end, read the generation counter again. If the value changed,
> we need to start over. Also, in this case, an error when reading the
> target process' memory should be ignored and retried, as we may have
> hit a race in which the target process unmapped the proctitle area
> after changing it.

Re: [PATCH] Added PR_SET_PROCTITLE_AREA option for prctl()

[Bryan Donlan]

On Sun, Oct 4, 2009 at 9:59 PM, KOSAKI Motohiro
<kosaki.motohiro@jp.fujitsu.com> wrote:
>> On Sun, Oct 4, 2009 at 9:38 PM, KOSAKI Motohiro
>> <kosaki.motohiro@jp.fujitsu.com> wrote:
>> >> The improvement idea is here.
>> >>
>> >> Changelog
>> >>   - Added task_lock() to prctl(PR_SET_PROCTITLE_AREA)
>> >>  -  Added small input sanity check to prctl(PR_SET_PROCTITLE_AREA)
>> >
>> > Doh, task_lock() is obviously wrong. please forget this.
>>
>> As another note, in general I think we'd need to hold a lock over the
>> entire operation. After all, if userspace changes its PROCTITLE_AREA,
>> and then reuses the memory for something else, we have an information
>> leak.
>
> if reusing occur, it's obviously userland fault. I don't think we need to care this.
> because current kernel also can be information leak by strcpy(argv[0], mypassword).
>
> I think they are userland bug both.

No, the scenario is:

Process B: Enter proc_pid_cmdline(), read arg_start and arg_end into
CPU registers
Process A: prctl(PR_SET_PROCTITLE_AREA)....
Process A: free(old_arg_area);
Process A: char *foo = malloc(...);
Process A: strcpy(foo, super_secret_password);
Process B: access_process_vm - using an area overlapping foo

Process B now has process A's secrets. This cannot be avoided by
process A, as it cannot control when process B will complete
proc_pid_cmdline(), and so the kernel must protect against this
scenario.  The only way a userspace process could prevent this is by
only using PR_SET_PROCTITLE_AREA once, and never reusing that memory,
ever. This does not seem like an appropriate restriction to pass down
to userspace for me...

Anyway, I'm working on a patch that uses the generation-counter approach now :)

Re: [PATCH] Added PR_SET_PROCTITLE_AREA option for prctl()

[KOSAKI Motohiro]

> On Sun, Oct 4, 2009 at 9:59 PM, KOSAKI Motohiro
> <kosaki.motohiro@jp.fujitsu.com> wrote:
> >> On Sun, Oct 4, 2009 at 9:38 PM, KOSAKI Motohiro
> >> <kosaki.motohiro@jp.fujitsu.com> wrote:
> >> >> The improvement idea is here.
> >> >>
> >> >> Changelog
> >> >>   - Added task_lock() to prctl(PR_SET_PROCTITLE_AREA)
> >> >>  -  Added small input sanity check to prctl(PR_SET_PROCTITLE_AREA)
> >> >
> >> > Doh, task_lock() is obviously wrong. please forget this.
> >>
> >> As another note, in general I think we'd need to hold a lock over the
> >> entire operation. After all, if userspace changes its PROCTITLE_AREA,
> >> and then reuses the memory for something else, we have an information
> >> leak.
> >
> > if reusing occur, it's obviously userland fault. I don't think we need to care this.
> > because current kernel also can be information leak by strcpy(argv[0], mypassword).
> >
> > I think they are userland bug both.
> 
> No, the scenario is:
> 
> Process B: Enter proc_pid_cmdline(), read arg_start and arg_end into
> CPU registers
> Process A: prctl(PR_SET_PROCTITLE_AREA)....
> Process A: free(old_arg_area);
> Process A: char *foo = malloc(...);
> Process A: strcpy(foo, super_secret_password);
> Process B: access_process_vm - using an area overlapping foo
> 
> Process B now has process A's secrets. This cannot be avoided by
> process A, as it cannot control when process B will complete
> proc_pid_cmdline(), and so the kernel must protect against this
> scenario.  The only way a userspace process could prevent this is by
> only using PR_SET_PROCTITLE_AREA once, and never reusing that memory,
> ever. This does not seem like an appropriate restriction to pass down
> to userspace for me...
> 
> Anyway, I'm working on a patch that uses the generation-counter approach now :)

Ok, you are right.
Plus, I've finished to made generation-counter approach patch :)

Re: [PATCH] Added PR_SET_PROCTITLE_AREA option for prctl()

[KOSAKI Motohiro]

> > No, the scenario is:
> > 
> > Process B: Enter proc_pid_cmdline(), read arg_start and arg_end into
> > CPU registers
> > Process A: prctl(PR_SET_PROCTITLE_AREA)....
> > Process A: free(old_arg_area);
> > Process A: char *foo = malloc(...);
> > Process A: strcpy(foo, super_secret_password);
> > Process B: access_process_vm - using an area overlapping foo
> > 
> > Process B now has process A's secrets. This cannot be avoided by
> > process A, as it cannot control when process B will complete
> > proc_pid_cmdline(), and so the kernel must protect against this
> > scenario.  The only way a userspace process could prevent this is by
> > only using PR_SET_PROCTITLE_AREA once, and never reusing that memory,
> > ever. This does not seem like an appropriate restriction to pass down
> > to userspace for me...
> > 
> > Anyway, I'm working on a patch that uses the generation-counter approach now :)
> 
> Ok, you are right.
> Plus, I've finished to made generation-counter approach patch :)

Updated version is here.
Changelog
 - since v2
      - use seqlock instead tasklock
  - since Timo's original
      - Added task_lock() to prctl(PR_SET_PROCTITLE_AREA)
      -  Added small input sanity check to prctl(PR_SET_PROCTITLE_AREA)


==============================================
Subject: [PATCH] Added PR_SET_PROCTITLE_AREA option for prctl()

Currently, glibc2 doesn't have setproctitle(3). it lead userland daemon
to use brutal stack modification.

In the first implession, It seems no problem at all. however, carefully
inspection spot the weaknes of the way.

example:

 % ps -ef |grep avahi-daemon
 avahi     1679     1  0 09:20 ?        00:00:00 avahi-daemon: running [kosadesk.local]
 avahi     1680  1679  0 09:20 ?        00:00:00 avahi-daemon: chroot helper

 # cat /proc/1679/cmdline
 avahi-daemon: running [kosadesk.local]

ok. seems good.
but...

 # cat /proc/1679/environ
 adesk.local]

Doh, this daemon has overwritten envrionment string area.

Recently, many security folks is thinking information leak is
security risk. then many administrator unset almost environment
variable before starting the daemon.
  e.g.
    env - MINIMUM-NEED-VAR=foo /path/to/daemon

it mean, we don't have enough string space now.

Thus, this patch implement new prctl. prctl(PR_SET_PROCTITLE_AREA)
updates mm_struct->arg_start and arg_end to the given pointers,
which makes it possible for user space to implement
setproctitle(3) cleanly.

 test_setproctitle.c
 ================================================
 #define ERR(str) (perror(str), exit(1))

 void settitle(char* title){
         int err;

         err = prctl(34, title, strlen(title)+1);
         if (err < 0)
                 ERR("prctl ");
 }

 void main(void){
         long i;
         char buf[1024];

         for (i = 0; i < 10000000000LL; i++){
                 sprintf(buf, "loooooooooooooooooooooooong string %d",i);
                 settitle(buf);
         }
 }
 ==================================================

Cc: Bryan Donlan <bdonlan@gmail.com>
Cc: Ulrich Drepper <drepper@redhat.com>
Cc: Timo Sirainen <tss@iki.fi>
Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
---
 fs/proc/base.c           |   57 +++++++++++++++++++++++++++++-----------------
 include/linux/mm_types.h |    2 +
 include/linux/prctl.h    |    4 +++
 kernel/fork.c            |    1 +
 kernel/sys.c             |   23 ++++++++++++++++++
 5 files changed, 66 insertions(+), 21 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 6f742f6..2f48440 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -255,32 +255,47 @@ static int proc_pid_cmdline(struct task_struct *task, char * buffer)
 	int res = 0;
 	unsigned int len;
 	struct mm_struct *mm = get_task_mm(task);
+	unsigned seq;
+
 	if (!mm)
 		goto out;
+
+	/* The process was not constructed yet? */
 	if (!mm->arg_end)
-		goto out_mm;	/* Shh! No looking before we're done */
+		goto out_mm;
 
- 	len = mm->arg_end - mm->arg_start;
- 
-	if (len > PAGE_SIZE)
-		len = PAGE_SIZE;
- 
-	res = access_process_vm(task, mm->arg_start, buffer, len, 0);
-
-	// If the nul at the end of args has been overwritten, then
-	// assume application is using setproctitle(3).
-	if (res > 0 && buffer[res-1] != '\0' && len < PAGE_SIZE) {
-		len = strnlen(buffer, res);
-		if (len < res) {
-		    res = len;
-		} else {
-			len = mm->env_end - mm->env_start;
-			if (len > PAGE_SIZE - res)
-				len = PAGE_SIZE - res;
-			res += access_process_vm(task, mm->env_start, buffer+res, len, 0);
+	do {
+		seq = read_seqbegin(&mm->arg_lock);
+
+		len = mm->arg_end - mm->arg_start;
+		if (len > PAGE_SIZE)
+			len = PAGE_SIZE;
+
+		res = access_process_vm(task, mm->arg_start, buffer, len, 0);
+
+		if (mm->arg_end != mm->env_start)
+			/* PR_SET_PROCTITLE_AREA used */
 			res = strnlen(buffer, res);
+		else if (res > 0 && buffer[res-1] != '\0' && len < PAGE_SIZE) {
+			/*
+			 * If the nul at the end of args has been overwritten,
+			 * then assume application is using sendmail's
+			 * SPT_REUSEARGV style argv override.
+			 */
+			len = strnlen(buffer, res);
+			if (len < res) {
+				res = len;
+			} else {
+				len = mm->env_end - mm->env_start;
+				if (len > PAGE_SIZE - res)
+					len = PAGE_SIZE - res;
+				res += access_process_vm(task, mm->env_start,
+							 buffer+res, len, 0);
+				res = strnlen(buffer, res);
+			}
 		}
-	}
+	} while (read_seqretry(&mm->arg_lock, seq));
+
 out_mm:
 	mmput(mm);
 out:
diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
index 0042090..9daa1fa 100644
--- a/include/linux/mm_types.h
+++ b/include/linux/mm_types.h
@@ -12,6 +12,7 @@
 #include <linux/completion.h>
 #include <linux/cpumask.h>
 #include <linux/page-debug-flags.h>
+#include <linux/seqlock.h>
 #include <asm/page.h>
 #include <asm/mmu.h>
 
@@ -236,6 +237,7 @@ struct mm_struct {
 	unsigned long stack_vm, reserved_vm, def_flags, nr_ptes;
 	unsigned long start_code, end_code, start_data, end_data;
 	unsigned long start_brk, brk, start_stack;
+	seqlock_t arg_lock;
 	unsigned long arg_start, arg_end, env_start, env_end;
 
 	unsigned long saved_auxv[AT_VECTOR_SIZE]; /* for /proc/PID/auxv */
diff --git a/include/linux/prctl.h b/include/linux/prctl.h
index b00df4c..feffb17 100644
--- a/include/linux/prctl.h
+++ b/include/linux/prctl.h
@@ -88,4 +88,8 @@
 #define PR_TASK_PERF_COUNTERS_DISABLE		31
 #define PR_TASK_PERF_COUNTERS_ENABLE		32
 
+
+/* Set process title memory area for setproctitle() */
+#define PR_SET_PROCTITLE_AREA 34
+
 #endif /* _LINUX_PRCTL_H */
diff --git a/kernel/fork.c b/kernel/fork.c
index bfee931..9eaa1cb 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -435,6 +435,7 @@ static struct mm_struct * mm_init(struct mm_struct * mm, struct task_struct *p)
 	mm->free_area_cache = TASK_UNMAPPED_BASE;
 	mm->cached_hole_size = ~0UL;
 	mm_init_owner(mm, p);
+	seqlock_init(&mm->arg_lock);
 
 	if (likely(!mm_alloc_pgd(mm))) {
 		mm->def_flags = 0;
diff --git a/kernel/sys.c b/kernel/sys.c
index b3f1097..e26c687 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -1528,6 +1528,29 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
 				current->timer_slack_ns = arg2;
 			error = 0;
 			break;
+		case PR_SET_PROCTITLE_AREA: {
+			struct mm_struct *mm = current->mm;
+			unsigned long addr = arg2;
+			unsigned long len = arg3;
+			unsigned long end = arg2 + arg3;
+
+			if (len > PAGE_SIZE)
+				return -EINVAL;
+
+			if (addr >= end)
+				return -EINVAL;
+
+			if (!access_ok(VERIFY_READ, addr, len)) {
+				return -EFAULT;
+			}
+
+			write_seqlock(&mm->arg_lock);
+			mm->arg_start = addr;
+			mm->arg_end = addr + len;
+			write_sequnlock(&mm->arg_lock);
+
+			return 0;
+		}
 		default:
 			error = -EINVAL;
 			break;
-- 
1.6.2.5

Re: [PATCH] Added PR_SET_PROCTITLE_AREA option for prctl()

[Bryan Donlan]

On Sun, Oct 4, 2009 at 10:48 PM, KOSAKI Motohiro
<kosaki.motohiro@jp.fujitsu.com> wrote:

> +                       } else {
> +                               len = mm->env_end - mm->env_start;
> +                               if (len > PAGE_SIZE - res)
> +                                       len = PAGE_SIZE - res;
> +                               res += access_process_vm(task, mm->env_start,
> +                                                        buffer+res, len, 0);
> +                               res = strnlen(buffer, res);
> +                       }


This bug was in the original code, but since you're touching it
anyway, it should be fixed now; if this access_process_vm fails
(perhaps due to the target unmapping the page in question in between
the two calls), bad things might happen if (error code) + res < 0, as
then strnlen will get a huge value in its length (possibly leading to
OOPS etc). It should be changed to check for an error return here and
fail out properly if there is an error in this second check.

Re: [PATCH] Added PR_SET_PROCTITLE_AREA option for prctl()

[KOSAKI Motohiro]

> On Sun, Oct 4, 2009 at 10:48 PM, KOSAKI Motohiro
> <kosaki.motohiro@jp.fujitsu.com> wrote:
> 
> > +                       } else {
> > +                               len = mm->env_end - mm->env_start;
> > +                               if (len > PAGE_SIZE - res)
> > +                                       len = PAGE_SIZE - res;
> > +                               res += access_process_vm(task, mm->env_start,
> > +                                                        buffer+res, len, 0);
> > +                               res = strnlen(buffer, res);
> > +                       }
> 
> 
> This bug was in the original code, but since you're touching it
> anyway, it should be fixed now; if this access_process_vm fails
> (perhaps due to the target unmapping the page in question in between
> the two calls), bad things might happen if (error code) + res < 0, as
> then strnlen will get a huge value in its length (possibly leading to
> OOPS etc). It should be changed to check for an error return here and
> fail out properly if there is an error in this second check.

AFAIK, access_process_vm() never return negative value.

===================================================================
int access_process_vm(struct task_struct *tsk, unsigned long addr, void *buf, int len, int write)
{
        mm = get_task_mm(tsk);
        if (!mm)
                return 0;

        down_read(&mm->mmap_sem);
        /* ignore errors, just check how much was successfully transferred */
        while (len) {
(snip)

        }
        up_read(&mm->mmap_sem);
        mmput(mm);

        return buf - old_buf;
}

Re: [PATCH] Added PR_SET_PROCTITLE_AREA option for prctl()

[Bryan Donlan]

On Sun, Oct 4, 2009 at 11:29 PM, KOSAKI Motohiro
<kosaki.motohiro@jp.fujitsu.com> wrote:
>> On Sun, Oct 4, 2009 at 10:48 PM, KOSAKI Motohiro
>> <kosaki.motohiro@jp.fujitsu.com> wrote:
>>
>> > +                       } else {
>> > +                               len = mm->env_end - mm->env_start;
>> > +                               if (len > PAGE_SIZE - res)
>> > +                                       len = PAGE_SIZE - res;
>> > +                               res += access_process_vm(task, mm->env_start,
>> > +                                                        buffer+res, len, 0);
>> > +                               res = strnlen(buffer, res);
>> > +                       }
>>
>>
>> This bug was in the original code, but since you're touching it
>> anyway, it should be fixed now; if this access_process_vm fails
>> (perhaps due to the target unmapping the page in question in between
>> the two calls), bad things might happen if (error code) + res < 0, as
>> then strnlen will get a huge value in its length (possibly leading to
>> OOPS etc). It should be changed to check for an error return here and
>> fail out properly if there is an error in this second check.
>
> AFAIK, access_process_vm() never return negative value.

Ahh, okay, I had read the if (res > 0 && ...) bit in the original code
as an error test. Nevermind then.

Re: [PATCH] Added PR_SET_PROCTITLE_AREA option for prctl()

[KOSAKI Motohiro]

> On Sun, Oct 4, 2009 at 11:29 PM, KOSAKI Motohiro
> <kosaki.motohiro@jp.fujitsu.com> wrote:
> >> On Sun, Oct 4, 2009 at 10:48 PM, KOSAKI Motohiro
> >> <kosaki.motohiro@jp.fujitsu.com> wrote:
> >>
> >> > +                       } else {
> >> > +                               len = mm->env_end - mm->env_start;
> >> > +                               if (len > PAGE_SIZE - res)
> >> > +                                       len = PAGE_SIZE - res;
> >> > +                               res += access_process_vm(task, mm->env_start,
> >> > +                                                        buffer+res, len, 0);
> >> > +                               res = strnlen(buffer, res);
> >> > +                       }
> >>
> >>
> >> This bug was in the original code, but since you're touching it
> >> anyway, it should be fixed now; if this access_process_vm fails
> >> (perhaps due to the target unmapping the page in question in between
> >> the two calls), bad things might happen if (error code) + res < 0, as
> >> then strnlen will get a huge value in its length (possibly leading to
> >> OOPS etc). It should be changed to check for an error return here and
> >> fail out properly if there is an error in this second check.
> >
> > AFAIK, access_process_vm() never return negative value.
> 
> Ahh, okay, I had read the if (res > 0 && ...) bit in the original code
> as an error test. Nevermind then.

No problem. very thak you for your good reviewing :)

Re: [PATCH] Added PR_SET_PROCTITLE_AREA option for prctl()

[Timo Sirainen]

On Mon, 2009-10-05 at 11:48 +0900, KOSAKI Motohiro wrote:
> Updated version is here.
> Changelog
>  - since v2
>       - use seqlock instead tasklock
>   - since Timo's original
>       - Added task_lock() to prctl(PR_SET_PROCTITLE_AREA)
>       -  Added small input sanity check to prctl(PR_SET_PROCTITLE_AREA)

I cleaned up the description somewhat as per Motohiro's request. The
actual patch is identical.

==============================================
Subject: [PATCH] Added PR_SET_PROCTITLE_AREA option for prctl()

Currently glibc2 doesn't have setproctitle(3), so several userland 
daemons attempt to emulate it by doing some brutal stack modifications.
This works most of the time, but it has problems. For example:

 % ps -ef |grep avahi-daemon
 avahi     1679     1  0 09:20 ?        00:00:00 avahi-daemon: running [kosadesk.local]

 # cat /proc/1679/cmdline
 avahi-daemon: running [kosadesk.local]

This looks good, but the process has also overwritten its environment
area and made the environ file useless:

 # cat /proc/1679/environ
 adesk.local]

Another problem is that the process title length is limited by the size of
the environment. Security conscious people try to avoid potential information
leaks by clearing most of the environment before running a daemon:

 # env - MINIMUM_NEEDED_VAR=foo /path/to/daemon

The resulting environment size may be too small to fit the wanted process
titles.

This patch makes it possible for userspace to implement setproctitle()
cleanly. It adds a new PR_SET_PROCTITLE_AREA option for prctl(), which
updates task's mm_struct->arg_start and arg_end to the given area.

 test_setproctitle.c
 ================================================
 #define ERR(str) (perror(str), exit(1))

 void settitle(char* title){
         int err;

         err = prctl(34, title, strlen(title)+1);
         if (err < 0)
                 ERR("prctl ");
 }

 void main(void){
         long i;
         char buf[1024];

         for (i = 0; i < 10000000000LL; i++){
                 sprintf(buf, "loooooooooooooooooooooooong string %d",i);
                 settitle(buf);
         }
 }
 ==================================================

Cc: Bryan Donlan <bdonlan@gmail.com>
Cc: Ulrich Drepper <drepper@redhat.com>
Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
Signed-off-by: Timo Sirainen <tss@iki.fi>
---
 fs/proc/base.c           |   57 +++++++++++++++++++++++++++++-----------------
 include/linux/mm_types.h |    2 +
 include/linux/prctl.h    |    4 +++
 kernel/fork.c            |    1 +
 kernel/sys.c             |   23 ++++++++++++++++++
 5 files changed, 66 insertions(+), 21 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 6f742f6..2f48440 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -255,32 +255,47 @@ static int proc_pid_cmdline(struct task_struct *task, char * buffer)
 	int res = 0;
 	unsigned int len;
 	struct mm_struct *mm = get_task_mm(task);
+	unsigned seq;
+
 	if (!mm)
 		goto out;
+
+	/* The process was not constructed yet? */
 	if (!mm->arg_end)
-		goto out_mm;	/* Shh! No looking before we're done */
+		goto out_mm;
 
- 	len = mm->arg_end - mm->arg_start;
- 
-	if (len > PAGE_SIZE)
-		len = PAGE_SIZE;
- 
-	res = access_process_vm(task, mm->arg_start, buffer, len, 0);
-
-	// If the nul at the end of args has been overwritten, then
-	// assume application is using setproctitle(3).
-	if (res > 0 && buffer[res-1] != '\0' && len < PAGE_SIZE) {
-		len = strnlen(buffer, res);
-		if (len < res) {
-		    res = len;
-		} else {
-			len = mm->env_end - mm->env_start;
-			if (len > PAGE_SIZE - res)
-				len = PAGE_SIZE - res;
-			res += access_process_vm(task, mm->env_start, buffer+res, len, 0);
+	do {
+		seq = read_seqbegin(&mm->arg_lock);
+
+		len = mm->arg_end - mm->arg_start;
+		if (len > PAGE_SIZE)
+			len = PAGE_SIZE;
+
+		res = access_process_vm(task, mm->arg_start, buffer, len, 0);
+
+		if (mm->arg_end != mm->env_start)
+			/* PR_SET_PROCTITLE_AREA used */
 			res = strnlen(buffer, res);
+		else if (res > 0 && buffer[res-1] != '\0' && len < PAGE_SIZE) {
+			/*
+			 * If the nul at the end of args has been overwritten,
+			 * then assume application is using sendmail's
+			 * SPT_REUSEARGV style argv override.
+			 */
+			len = strnlen(buffer, res);
+			if (len < res) {
+				res = len;
+			} else {
+				len = mm->env_end - mm->env_start;
+				if (len > PAGE_SIZE - res)
+					len = PAGE_SIZE - res;
+				res += access_process_vm(task, mm->env_start,
+							 buffer+res, len, 0);
+				res = strnlen(buffer, res);
+			}
 		}
-	}
+	} while (read_seqretry(&mm->arg_lock, seq));
+
 out_mm:
 	mmput(mm);
 out:
diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
index 0042090..9daa1fa 100644
--- a/include/linux/mm_types.h
+++ b/include/linux/mm_types.h
@@ -12,6 +12,7 @@
 #include <linux/completion.h>
 #include <linux/cpumask.h>
 #include <linux/page-debug-flags.h>
+#include <linux/seqlock.h>
 #include <asm/page.h>
 #include <asm/mmu.h>
 
@@ -236,6 +237,7 @@ struct mm_struct {
 	unsigned long stack_vm, reserved_vm, def_flags, nr_ptes;
 	unsigned long start_code, end_code, start_data, end_data;
 	unsigned long start_brk, brk, start_stack;
+	seqlock_t arg_lock;
 	unsigned long arg_start, arg_end, env_start, env_end;
 
 	unsigned long saved_auxv[AT_VECTOR_SIZE]; /* for /proc/PID/auxv */
diff --git a/include/linux/prctl.h b/include/linux/prctl.h
index b00df4c..feffb17 100644
--- a/include/linux/prctl.h
+++ b/include/linux/prctl.h
@@ -88,4 +88,8 @@
 #define PR_TASK_PERF_COUNTERS_DISABLE		31
 #define PR_TASK_PERF_COUNTERS_ENABLE		32
 
+
+/* Set process title memory area for setproctitle() */
+#define PR_SET_PROCTITLE_AREA 34
+
 #endif /* _LINUX_PRCTL_H */
diff --git a/kernel/fork.c b/kernel/fork.c
index bfee931..9eaa1cb 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -435,6 +435,7 @@ static struct mm_struct * mm_init(struct mm_struct * mm, struct task_struct *p)
 	mm->free_area_cache = TASK_UNMAPPED_BASE;
 	mm->cached_hole_size = ~0UL;
 	mm_init_owner(mm, p);
+	seqlock_init(&mm->arg_lock);
 
 	if (likely(!mm_alloc_pgd(mm))) {
 		mm->def_flags = 0;
diff --git a/kernel/sys.c b/kernel/sys.c
index b3f1097..e26c687 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -1528,6 +1528,29 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
 				current->timer_slack_ns = arg2;
 			error = 0;
 			break;
+		case PR_SET_PROCTITLE_AREA: {
+			struct mm_struct *mm = current->mm;
+			unsigned long addr = arg2;
+			unsigned long len = arg3;
+			unsigned long end = arg2 + arg3;
+
+			if (len > PAGE_SIZE)
+				return -EINVAL;
+
+			if (addr >= end)
+				return -EINVAL;
+
+			if (!access_ok(VERIFY_READ, addr, len)) {
+				return -EFAULT;
+			}
+
+			write_seqlock(&mm->arg_lock);
+			mm->arg_start = addr;
+			mm->arg_end = addr + len;
+			write_sequnlock(&mm->arg_lock);
+
+			return 0;
+		}
 		default:
 			error = -EINVAL;
 			break;
-- 
1.6.2.5