Re: [PATCH] Fix prctl(PR_GET_NAME) to not leak random trailing bytes

[Helge Deller]

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 implemented to
> > always copy the full array of 16-bytes back and forth between kernel and
> > 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-terminated
> > string (with max. up to 16 chars including the trailing zero) to userspace and
> > thus avoids copying and leaking random trailing chars behind the process name.
> >
> > Background:
> > The newest glibc testsuite includes a test which is implemented similiar to
> > this:
> >     prctl(PR_SET_NAME, "thread name", 0, 0, 0);
> >     char buffer[16] = { 0, };
> >     prctl(PR_GET_NAME, buffer, 0, 0, 0);
> >     char expected[16] = "thread name";
> >     fail if memcmp(buffer, expected, 16) != 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 total of
> > 16) chars behind that string has zeroes.
> > As such on the parisc architecture I've seen that the buffer[] array gets
> > filled on return of prctl(PR_GET_NAME) with such additional random bytes, 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 function of
> > prctl(PR_GET_NAME) to just return the null-terminated process name.
> >
> > Signed-off-by: Helge Deller <deller@xxxxxx>
> > Cc: stable@xxxxxxxxxxxxxxx
> >
> > 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 *tsk);
> #define get_task_comm(buf, tsk) ({                      \
>          BUILD_BUG_ON(sizeof(buf) != 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 *tsk)
> {
>          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 breakage.
Anyway, the function needs fixing.

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

Helge