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

[Bryan Donlan <bdonlan@gmail.com>]

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.