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

[Dmitry Vyukov]

On Sun, Oct 4, 2015 at 7:10 PM, 'Kostya Serebryany' via kasan-dev
<kasan-dev@xxxxxxxxxxxxxxxx> wrote:
>
>
> On Sun, Oct 4, 2015 at 5:14 AM, Dmitry Vyukov <dvyukov@xxxxxxxxxx> wrote:
>>
>> 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
>
> This is very risky w/o RAII -- we can forget to do kasan_enable_current on
> one of the paths.
> And we can't do RAII in C.

Not more risky than thousands of other resource allocations in kernel
and other C programs that power the world.

In [k]tsan we check that such counters are 0 on thread end. This is
practically enough mitigate potential risks.
As far as I remember we don't do it in kasan yet.


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