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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux SoC]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux