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 <[email protected]>
---
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
On Fri, Oct 2, 2009 at 5:23 PM, Timo Sirainen <[email protected]> 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.
On Oct 2, 2009, at 10:01 PM, Bryan Donlan wrote:
> On Fri, Oct 2, 2009 at 5:23 PM, Timo Sirainen <[email protected]> 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
On Fri, Oct 2, 2009 at 10:47 PM, Timo Sirainen <[email protected]> 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...
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.
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
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;
>> 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 [email protected]
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at ?http://www.tux.org/lkml/
>
On Sun, Oct 4, 2009 at 10:44 AM, KOSAKI Motohiro
<[email protected]> 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.
> On Sun, Oct 4, 2009 at 10:44 AM, KOSAKI Motohiro
> <[email protected]> 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 <[email protected]>
Cc: Ulrich Drepper <[email protected]>
Cc: Timo Sirainen <[email protected]>
Signed-off-by: KOSAKI Motohiro <[email protected]>
---
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
> 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.
On Sun, Oct 4, 2009 at 9:38 PM, KOSAKI Motohiro
<[email protected]> 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.
> On Sun, Oct 4, 2009 at 9:38 PM, KOSAKI Motohiro
> <[email protected]> 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.
On Sun, Oct 4, 2009 at 9:59 PM, KOSAKI Motohiro
<[email protected]> wrote:
>> On Sun, Oct 4, 2009 at 9:38 PM, KOSAKI Motohiro
>> <[email protected]> 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 :)
> On Sun, Oct 4, 2009 at 9:59 PM, KOSAKI Motohiro
> <[email protected]> wrote:
> >> On Sun, Oct 4, 2009 at 9:38 PM, KOSAKI Motohiro
> >> <[email protected]> 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 :)
> > 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 <[email protected]>
Cc: Ulrich Drepper <[email protected]>
Cc: Timo Sirainen <[email protected]>
Signed-off-by: KOSAKI Motohiro <[email protected]>
---
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
On Sun, Oct 4, 2009 at 10:48 PM, KOSAKI Motohiro
<[email protected]> 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.
> On Sun, Oct 4, 2009 at 10:48 PM, KOSAKI Motohiro
> <[email protected]> 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;
}
On Sun, Oct 4, 2009 at 11:29 PM, KOSAKI Motohiro
<[email protected]> wrote:
>> On Sun, Oct 4, 2009 at 10:48 PM, KOSAKI Motohiro
>> <[email protected]> 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.
> On Sun, Oct 4, 2009 at 11:29 PM, KOSAKI Motohiro
> <[email protected]> wrote:
> >> On Sun, Oct 4, 2009 at 10:48 PM, KOSAKI Motohiro
> >> <[email protected]> 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 :)
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 <[email protected]>
Cc: Ulrich Drepper <[email protected]>
Signed-off-by: KOSAKI Motohiro <[email protected]>
Signed-off-by: Timo Sirainen <[email protected]>
---
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