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

[Bryan Donlan]

On Fri, Oct 2, 2009 at 10:47 PM, Timo Sirainen <tss@xxxxxx> 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...
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/