Re: 4.3-rc7: kmemleak BUG: Redzone overwritten

[Andy Shevchenko]

On Wed, Oct 28, 2015 at 12:16 AM, Linus Torvalds
<torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
> 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;


Just send couple of minutes before a message (pity it was in html, due
to was sent from phone). Similar suspicion.



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

I don't think it should be fixed like this.
All other cases are checking for room correctly (if I didn't miss anything).

Here I would like to repeat the snprintf() behaviour, i.e. print each
symbol separately

if (linwbuflen < lx + 2)
 goto overflow;
linebuf[lx++] = hi_byte;
...


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


-- 
With Best Regards,
Andy Shevchenko
--
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/