Re: 4.3-rc7: kmemleak BUG: Redzone overwritten

[Linus Torvalds]

On Wed, Oct 28, 2015 at 12:46 AM, Aaro Koskinen <aaro.koskinen@xxxxxxxxx> wrote:
>
> With 4.3-rc7 and slub_debug=FZUP, I get the below when reading
> /sys/kernel/debug/kmemleak with a large number of reported entries.
> It's pretty repeatable. HW is MIPS64.
>
> With the SLUB debugging disabled, box crashes randomly in kmem_cache_free
> or kmem_cache_alloc when the kmemleak file is read on a running system.
>
> Seems to start with 6fc37c490076 ("kmemleak: use seq_hex_dump() to
> dump buffers").

Well, so that commit itself looks fine - it just uses the seq accessor
functions to print things out, instead of doing it by hand.

So if that commit causes problems, then I suspect that the real issue
is that seq_hex_dump() itself is buggered, and that the commit just
exposed it by adding new use-cases. It looks like the hexdump wrote
one byte (the terminating NUL) past the end of the buffer:

> [   77.706871] BUG kmalloc-4096 (Not tainted): Redzone overwritten
> [   77.706877]
> [   77.706894] INFO: 0x800000002e939000-0x800000002e939000. First byte 0x0 instead of 0xcc
> [   77.706914] INFO: Allocated in seq_buf_alloc+0x24/0x58 age=452 cpu=2 pid=587
> [   77.706928]  __slab_alloc.isra.72.constprop.75+0x4a4/0x508
> [   77.706938]  __kmalloc+0x30c/0x3f0
> [   77.706947]  seq_buf_alloc+0x24/0x58
> [   77.706956]  seq_read+0x304/0x4a0
> [   77.706968]  __vfs_read+0x3c/0x100
> [   77.706977]  vfs_read+0x8c/0x138
> [   77.706987]  SyS_read+0x64/0xe8
> [   77.707000]  syscall_common+0x34/0x58
> [   77.707012] INFO: Freed in seq_release+0x24/0x40 age=3450 cpu=3 pid=584
> [   77.707023]  __slab_free+0x340/0x4f0
> [   77.707032]  seq_release+0x24/0x40
> [   77.707044]  kernfs_fop_release+0x50/0x80
> [   77.707055]  __fput+0xa4/0x218
> [   77.707066]  task_work_run+0xb0/0x108
> [   77.707078]  work_notifysig+0x10/0x18
> [   77.707087] INFO: Slab 0x8000000003ec4440 objects=7 used=1 fp=0x800000002e93e7b0 flags=0x200000004081
> [   77.707095] INFO: Object 0x800000002e938000 @offset=0 fp=0x800000002e939148
> [   77.707095]
> [   77.707108] Object 800000002e938000: 75 6e 72 65 66 65 72 65 6e 63 65 64 20 6f 62 6a  unreferenced obj
> [   77.707118] Object 800000002e938010: 65 63 74 20 30 78 38 30 30 30 30 30 30 30 32 66  ect 0x800000002f
...
> [   77.709583] Object 800000002e938f90: 6d 6d 20 22 73 77 61 70 70 65 72 2f 30 22 2c 20  mm "swapper/0",
> [   77.709593] Object 800000002e938fa0: 70 69 64 20 31 2c 20 6a 69 66 66 69 65 73 20 34  pid 1, jiffies 4
> [   77.709603] Object 800000002e938fb0: 32 39 34 39 33 38 30 35 31 20 28 61 67 65 20 34  294938051 (age 4
> [   77.709613] Object 800000002e938fc0: 31 2e 35 37 30 73 29 0a 20 20 68 65 78 20 64 75  1.570s).  hex du
> [   77.709623] Object 800000002e938fd0: 6d 70 20 28 66 69 72 73 74 20 33 32 20 62 79 74  mp (first 32 byt
> [   77.709633] Object 800000002e938fe0: 65 73 29 3a 0a 20 20 20 20 36 62 20 36 62 20 36  es):.    6b 6b 6
> [   77.709643] Object 800000002e938ff0: 62 20 36 62 20 36 62 20 36 62 20 36 62 20 00 20  b 6b 6b 6b 6b .
> [   77.709653] Redzone 800000002e939000: 00 cc cc cc cc cc cc cc                          ........

So I suspect that some seq function ends up adding a terminating NUL
character too much when the buffer overflows.

The obvious suspect would be the "hex_dump_to_buffer()" call in
seq_hex_dump(). It's the only thing that doesn't use really common
core helpers, though.

Looking at "hex_dump_to_buffer()", code like this strikes me as
particularly dangerous:

                        if (linebuflen < lx + 3)
                                goto overflow2;
     ...
    overflow2:
            linebuf[lx++] = '\0';
    overflow1:
            return ascii ? ascii_column + len : (groupsize * 2 + 1) *
ngroups - 1;

because what if lx == linebuflen in the overflow condition.

But the non-overflow condition looks a bit scary too: the
"non-overflow" case checks that there is room for three characters,
and then adds those three characters (and possible removes the last
one). Fine - but what if the three characters *exactly* filled the
buffer, and we think we haven't overflowed, and now we just do

    nil:
            linebuf[lx] = '\0';
            return lx;

there as the "success" case.

So without trying to really analyze this, I do suspect that the
problem is in either of those cases.

I would suggest the "nil:" case do

    nil:
            if (lx < linebuflen)
                    linebuf[lx] = 0;
            return lx;

and add something similar to overflow2 too.

Hmm? Does that fix your test-case? Added Al Viro as seq_file
maintainer to the cc.

              Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/