Re: BUG: KASAN: slab-out-of-bounds in perf_callchain_user+0x494/0x530

[Namhyung Kim]

On Wed, Dec 6, 2017 at 11:31 PM, Namhyung Kim <namhyung@xxxxxxxxxx> wrote:
> Hi Peter,
>
> On Wed, Dec 06, 2017 at 02:47:06PM +0100, Peter Zijlstra wrote:
>> On Tue, Dec 05, 2017 at 11:47:18PM +0900, Namhyung Kim wrote:
>> > Sure, I mean the following code:
>> >
>> >     mutex_lock(&callchain_mutex);
>> >
>> >     count = atomic_inc_return(&nr_callchain_events);
>> >     if (WARN_ON_ONCE(count < 1)) {
>> >             err = -EINVAL;
>> >             goto exit;
>> >     }
>> >
>> >     if (count > 1) {
>> >             /* If the allocation failed, give up */
>> >             if (!callchain_cpus_entries)
>> >                     err = -ENOMEM;
>> >
>> >             goto exit;
>> >     }
>> >
>> >     err = alloc_callchain_buffers();
>> > exit:
>> >     if (err)
>> >             atomic_dec(&nr_callchain_events);
>> >
>> >     mutex_unlock(&callchain_mutex);
>> >
>> >
>> > The callchain_cpus_entries is allocated in alloc_callchain_buffers()
>> > only when the count is 1.  But if it failed to allocate, it decrease
>> > the count so next event would try to allocate it again.  Thus it seems
>> > not possible to see the callchain_cpus_entries being NULL in the
>> > 'if (count > 1)' block.  If you want to make next event give up, it'd
>> > need to take an additional count IMHO.
>>
>> There's also a race against put_callchain_buffers() there, consider:
>>
>>
>>       get_callchain_buffers()         put_callchain_buffers()
>>         mutex_lock();
>>         inc()
>>                                         dec_and_test() // false
>>
>>         dec() // 0
>>
>>
>> And the buffers leak.
>
> Hmm.. did you mean that get_callchain_buffers() returns an error?
> AFAICS it cannot fail when it sees count > 1 (and callchain_cpus_
> entries is allocated).  So I think it won't decrease the count and
> should be fine.

Oh, I missed the max_stack case (since it was dropped in the above).
Sorry for the noise..

Thanks,
Namhyung