| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 28 Oct 2015 00:39:07 +0200
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Linus Torvalds <torvalds@...ux-foundation.org>
Cc: Aaro Koskinen <aaro.koskinen@...ia.com>,
Al Viro <viro@...iv.linux.org.uk>,
Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
Catalin Marinas <catalin.marinas@....com>,
Andrew Morton <akpm@...ux-foundation.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: 4.3-rc7: kmemleak BUG: Redzone overwritten
On Wed, Oct 28, 2015 at 12:16 AM, Linus Torvalds
<torvalds@...ux-foundation.org> wrote:
> On Wed, Oct 28, 2015 at 12:46 AM, Aaro Koskinen <aaro.koskinen@...ia.com> 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@...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