Re: [patch 1/2] x86/process: Add proper bound checks in 64bit get_wchan()

[Dmitry Vyukov]

On Sat, Oct 3, 2015 at 1:31 PM, Andrey Ryabinin <ryabinin.a.a@xxxxxxxxx> wrote:
> 2015-10-03 13:54 GMT+03:00 Thomas Gleixner <tglx@xxxxxxxxxxxxx>:
>> On Fri, 2 Oct 2015, Sasha Levin wrote:
>>> I'm seeing a different issue with this patch:
>>>
>>> [ 5228.736320] BUG: KASAN: out-of-bounds in get_wchan+0xf9/0x1b0 at addr ffff88049d2b7c50
>>> [ 5228.737560] Read of size 8 by task killall/22177
>>> [ 5228.738304] page:ffffea001274adc0 count:0 mapcount:0 mapping:          (null) index:0x0
>>> [ 5228.739374] flags: 0x6fffff80000000()
>>> [ 5228.739862] page dumped because: kasan: bad access detected
>>> [ 5228.741764] CPU: 8 PID: 22177 Comm: killall Not tainted 4.3.0-rc3-next-20151002-sasha-00076-gde7fa56-dirty #2590
>>> [ 5228.743337]  ffff882c80967828 000000007a901a83 ffff882c80967790 ffffffffacd2c8c8
>>> [ 5228.744409]  ffff88049d2b7c50 ffff882c80967818 ffffffffab74befb ffff882c8bd00000
>>> [ 5228.745436]  0000000000000002 0000000000000282 ffff882c8bd00cf8 0000000000000001
>>> [ 5228.746446] Call Trace:
>>> [ 5228.746881] dump_stack (lib/dump_stack.c:52)
>>> [ 5228.747720] kasan_report_error (include/linux/kasan.h:28 mm/kasan/report.c:170 mm/kasan/report.c:237)
>>> [ 5228.748670] __asan_report_load8_noabort (mm/kasan/report.c:279)
>>> [ 5228.750563] get_wchan (arch/x86/kernel/process.c:561)
>>> [ 5228.751378] do_task_stat (fs/proc/array.c:458)
>>> [ 5228.755912] proc_tgid_stat (fs/proc/array.c:565)
>>> [ 5228.756770] proc_single_show (./arch/x86/include/asm/atomic.h:118 include/linux/sched.h:2012 fs/proc/base.c:789)
>>> [ 5228.759066] seq_read (fs/seq_file.c:238)
>>> [ 5228.762360] __vfs_read (fs/read_write.c:432)
>>> [ 5228.767957] vfs_read (fs/read_write.c:454)
>>> [ 5228.769368] SyS_read (fs/read_write.c:570 fs/read_write.c:562)
>>> [ 5228.778344] entry_SYSCALL_64_fastpath (arch/x86/entry/entry_64.S:186)
>>> [ 5228.779272] Memory state around the buggy address:
>>> [ 5228.779971]  ffff88049d2b7b00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>>> [ 5228.780992]  ffff88049d2b7b80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>>> [ 5228.782021] >ffff88049d2b7c00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>>> [ 5228.783066]                                                     ^
>>> [ 5228.783936]  ffff88049d2b7c80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>>> [ 5228.784994]  ffff88049d2b7d00: 00 00 00 00 00 f1 f1 f1 f1 00 f4 f4 f4 f3 f3 f3
>>>
>>>         fp = READ_ONCE(*(unsigned long *)sp);
>>>         do {
>>>                 if (fp < bottom || fp > top)
>>>                         return 0;
>>>                 ip = READ_ONCE(*(unsigned long *)(fp + sizeof(unsigned long)));
>>>                 if (!in_sched_functions(ip))
>>>                         return ip;
>>>                 fp = READ_ONCE(*(unsigned long *)fp); <=== Here
>>
>> Weird, we accessed
>>
>>      *(unsigned long *)(fp + sizeof(unsigned long))
>>
>> a few lines above, i.e. ffff88049d2b7c58
>>
>> But what's more weird is that the memory dump does not really look
>> like a stack at all.
>>
>> ffff88049d2b7c50 is stored on the stack:
>>
>>> [ 5228.744409]  ffff88049d2b7c50 ffff882c80967818 ffffffffab74befb ffff882c8bd00000
>>
>> But if it is not inside the stack bounds, how do we end up
>> dereferencing it. Confused....
>>
>
> I'm sure that we in stack bounds here.
> But we are not inside bounds of some stack variable
> and KASAN doesn't like it.

Agree.

> KASAN changes stack frame of each function, e.g.
>     void foo() {
>             int a;
>     }
>
> transformed to:
>     void foo() {
>             char redzone1[32];
>             int a;
>             char redzone2[28];
>             char redzone3[32];
>     }
>
> So any access to redzones will be reported.
>
> I could make a patch which will tell KASAN to ignore get_wchan(),
> but if there are any real bugs left in get_wchan() they will be ignored too.

Yes, we need KASAN to ignore it. False positives are not acceptable. I
wanted to mail a fix after the fix for the real bug. I am thinking
about kasan_disable_current/kasan_enable_current around the region
that does the stack accesses; the reports are rare enough, so it
should not have any significant performance effect. I don't mind if
you send the patch.
--
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/