linux-kernel - Re: [PATCH] Added PR_SET_PROCTITLE

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

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

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

Message-ID: <3e8340490910041105p327396e8v39b1a3454c0922bf@mail.gmail.com>
Date:	Sun, 4 Oct 2009 14:05:59 -0400
From:	Bryan Donlan <bdonlan@...il.com>
To:	KOSAKI Motohiro <kosaki.motohiro@...il.com>
Cc:	Timo Sirainen <tss@....fi>, linux-kernel@...r.kernel.org,
	Ulrich Drepper <drepper@...hat.com>,
	KOSAKI Motohiro <kosaki.motohiro@...fujitsu.com>
Subject: Re: [PATCH] Added PR_SET_PROCTITLE_AREA option for prctl()

On Sun, Oct 4, 2009 at 10:44 AM, KOSAKI Motohiro
<kosaki.motohiro@...il.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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/