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

[Rasmus Villemoes]

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. 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.

[Also, the compiler most likely puts the "thread name" string literal
first into a .rodata.strX.Y section, only later to be merged into
.rodata by the linker, so anything after "thread name" I'd expect to
also be some readable string. The fact that you have a 0x1a byte in
there suggests that the garbage doesn't actually come from the original
PR_SET_NAME call, but is consistent with it being a stack leak from the
kernel. You could try running hexdump on the test binary to see if
there's any occurrence of "thread name" followed by those particular
garbage bytes.]

Rasmus