Date:	Sat, 11 Jan 2014 21:03:21 +0900
From:	Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
To:	geert@...ux-m68k.org, akpm@...ux-foundation.org
Cc:	pavel@....cz, joe@...ches.com, keescook@...omium.org,
	jkosina@...e.cz, viro@...iv.linux.org.uk, davem@...emloft.net,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] lib/vsprintf: add %pT format specifier

Geert Uytterhoeven wrote:
> On Sat, Jan 11, 2014 at 12:59 AM, Andrew Morton
> <akpm@...ux-foundation.org> wrote:
> >> +char *comm_name(char *buf, char *end, struct task_struct *tsk,
> >> +             struct printf_spec spec, const char *fmt)
> >> +{
> >> +     char name[TASK_COMM_LEN];
> >> +
> >> +     /* Caller can pass NULL instead of current. */
> >> +     if (!tsk)
> >> +             tsk = current;
> >> +     /* Not using get_task_comm() in case I'm in IRQ context. */
> >> +     memcpy(name, tsk->comm, TASK_COMM_LEN);
> 
> So this may copy more bytes than the actual string length of tsk->comm.
> As this is a temporary buffer, that just wastes cycles.

For example, strncpy() in arch/x86/lib/string_32.c is

char *strncpy(char *dest, const char *src, size_t count)
{
	int d0, d1, d2, d3;
        asm volatile("1:\tdecl %2\n\t"
                "js 2f\n\t"
                "lodsb\n\t"
                "stosb\n\t"
                "testb %%al,%%al\n\t"
                "jne 1b\n\t"
                "rep\n\t"
                "stosb\n"
                "2:"
                : "=&S" (d0), "=&D" (d1), "=&c" (d2), "=&a" (d3)
                : "" (src), "1" (dest), "2" (count) : "memory");
        return dest;
}

and strncpy() in lib/string.c is

char *strncpy(char *dest, const char *src, size_t count)
{
        char *tmp = dest;

        while (count) {
                if ((*tmp = *src) != 0)
                        src++;
                tmp++;
                count--;
        }
        return dest;
}

while memcpy(name, tsk->comm, TASK_COMM_LEN) is

  u64 *dest = (u64 *) name;
  u64 *src = (u64 *) tsk->comm;
  *dest++ = *src++;
  *dest = *src;

if sizeof(long) == 64. I can't understand why unconditionally copying 8 bytes *
2 consumes more cycles than conditionally copying up to 16 bytes...

Also, strncpy() in lib/string.c is not safe for copying task_struct->comm, for
task_struct->comm can change at any moment.

  Initial state:

    p->comm contains "secret_commname\0"

  A reader calls strncpy(buf, p->comm, 16)
  In strncpy() does

    char *dest = buf
    char *src = tsk->comm
    char *tmp = dest
    while (16)
      if ((buf[0] = 's') != 0)
        src++
      tmp++;
      15
    while (15)
      if ((buf[1] = 'e') != 0)
        src++
      tmp++
      14

  At this moment preemption happens, and a writer jumps in.
  The writer calls set_task_comm(p, "x").
  Now p->comm contains "x\0cret_commname\0".
  The preemption ends and the reader continues the loop.
  Now *src == '\0' but continues copying.

    while (14)
      if ((buf[2] = 'c') != 0)
        src++
      tmp++
      13
    (...snipped...)
    while (1)
      if ((buf[15] = '\0') != 0)
      tmp++
      0
    return dest

  and gets "xecret_commname\0" in the buf.
  The reader got some garbage bytes.

What is worse, there are some writers who changes p->comm without using
set_task_comm(). This means that we cannot prove that p->comm contains '\0',
making readers to get non-'\0' terminated comm name.

> And even if it wasn't, data between the string zero terminator and the end of
> the buffer wouild be leaked.
> 
> >> +     name[sizeof(name) - 1] = '\0';
> 
> You can use strlcpy() here instead of memcpy and clear.

For example, strlcpy() in lib/string.c is

size_t strlcpy(char *dest, const char *src, size_t size)
{
        size_t ret = strlen(src);

        if (size) {
                size_t len = (ret >= size) ? size - 1 : ret;
                memcpy(dest, src, len);
                dest[len] = '\0';
        }
        return ret;
}

and strlen(p->comm) can change after ret is calculated, leading to the result
as with strncpy(buf, p->comm, TASK_COMM_LEN).

> strncpy() fills the remaining of the buffer with zeroes, so it avoids leaking
> data.

I don't think that is true, as described above.

> 
> Note that strncpy() may leave the buffer non-zero-terminated if the source
> string is too long, but as set_task_comm() uses strlcpy(), this should never
> be the case:

I don't think that is true, as described above.

Trying to copy p->comm using strncpy() or strlcpy() is not safe. Copy 16 bytes
using memcpy(), and explicitly terminate with '\0' is the safer way, although
any approach may get some garbage bytes.
--
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/

Powered by blists - more mailing lists