Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH] Fix prctl(PR_GET_NAME) to not leak random trailing bytes
To:     Rasmus Villemoes <linux@rasmusvillemoes.dk>,
        Linus Torvalds <torvalds@linux-foundation.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        linux-kernel@vger.kernel.org
Cc:     linux-parisc@vger.kernel.org,
        James Bottomley <James.Bottomley@hansenpartnership.com>,
        John David Anglin <dave.anglin@bell.net>
References: <YSiwLuE17BwRlI/d@ls3530>
 <aa0006e7-58b8-ab05-0599-5609e900ba4d@rasmusvillemoes.dk>
From:   Helge Deller <deller@gmx.de>
Message-ID: <97a0ddcf-243d-f312-8291-01d6595260bf@gmx.de>
Date:   Fri, 27 Aug 2021 14:18:17 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.11.0
MIME-Version: 1.0
In-Reply-To: <aa0006e7-58b8-ab05-0599-5609e900ba4d@rasmusvillemoes.dk>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On 8/27/21 12:31 PM, Rasmus Villemoes wrote:
> On 27/08/2021 11.28, Helge Deller wrote:
>> The prctl(PR_GET_NAME) and prctl(PR_SET_NAME) syscalls are used to set =
and
>> retrieve the process name. Those kernel functions are currently impleme=
nted to
>> always copy the full array of 16-bytes back and forth between kernel an=
d
>> userspace instead of just copying the relevant bytes of the string.
>>
>> This patch changes the prctl(PR_GET_NAME) to only copy back the null-te=
rminated
>> string (with max. up to 16 chars including the trailing zero) to usersp=
ace and
>> thus avoids copying and leaking random trailing chars behind the proces=
s name.
>>
>> Background:
>> The newest glibc testsuite includes a test which is implemented similia=
r to
>> this:
>>     prctl(PR_SET_NAME, "thread name", 0, 0, 0);
>>     char buffer[16] =3D { 0, };
>>     prctl(PR_GET_NAME, buffer, 0, 0, 0);
>>     char expected[16] =3D "thread name";
>>     fail if memcmp(buffer, expected, 16) !=3D 0;
>>
>> The compiler may put the "thread name" string given in the PR_SET_NAME =
call
>> somewhere into memory and it's not guaranteed that trailing (up to a to=
tal of
>> 16) chars behind that string has zeroes.
>> As such on the parisc architecture I've seen that the buffer[] array ge=
ts
>> filled on return of prctl(PR_GET_NAME) with such additional random byte=
s, e.g.:
>>         "thread name\000@\032i\000"
>>         74 68 72 65 61 64 20 6E 61 6D 65 00 40 1A 69 00
>>
>> Unfortunatly the glibc testuite tests the full memory block of 16 bytes
>> and fails because it expects zeroed characters behind the process name.
>>
>> In addition to fix the glibc testsuite, I suggest to fix the kernel fun=
ction of
>> prctl(PR_GET_NAME) to just return the null-terminated process name.
>>
>> Signed-off-by: Helge Deller <deller@gmx.de>
>> Cc: stable@vger.kernel.org
>>
>> diff --git a/kernel/sys.c b/kernel/sys.c
>> index ef1a78f5d71c..af71412760be 100644
>> --- a/kernel/sys.c
>> +++ b/kernel/sys.c
>> @@ -2367,7 +2367,7 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long=
, arg2, unsigned long, arg3,
>>   		break;
>>   	case PR_GET_NAME:
>>   		get_task_comm(comm, me);
>> -		if (copy_to_user((char __user *)arg2, comm, sizeof(comm)))
>> +		if (copy_to_user((char __user *)arg2, comm, strlen(comm) + 1))
>>   			return -EFAULT;
>>   		break;
>
> I don't understand. get_task_comm() is
>
> extern char *__get_task_comm(char *to, size_t len, struct task_struct *t=
sk);
> #define get_task_comm(buf, tsk) ({                      \
>          BUILD_BUG_ON(sizeof(buf) !=3D TASK_COMM_LEN);     \
>          __get_task_comm(buf, sizeof(buf), tsk);         \
> })
>
> and __get_task_comm() is
>
> char *__get_task_comm(char *buf, size_t buf_size, struct task_struct *ts=
k)
> {
>          task_lock(tsk);
>          strncpy(buf, tsk->comm, buf_size);
>          task_unlock(tsk);
>          return buf;
> }
>
> so the strncpy should ensure that the caller's buffer after the string's
> terminator gets zero-filled. I can see that parisc has its own
> strncpy(), but I can't read that asm, so I can't see if it actually does
> that mandated-by-C-standard zero-filling.

Oh, the parisc strncpy() asm does NOT zero-fill the target address !!
That's the bug.
I thought strncpy would just copy up to given number of chars.

> It would surprise me if it
> didn't (I'd expect lots of other breakage), but OTOH it is the only way
> I can explain what you've seen.

Interestingly the kernel runs quite well and we don't see any bigger break=
age.
Anyway, the function needs fixing.

Please ignore this patch and thanks for pointing to the real bug.

Helge