Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <b4a3fc9d-bd15-0682-2c56-4e63e0fb30cd@bytedance.com>
Date:   Fri, 23 Dec 2022 18:08:03 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.6.0
Subject: Re: [syzbot] KASAN: use-after-free Read in put_pmu_ctx
Content-Language: en-US
To:     Peter Zijlstra <peterz@infradead.org>
Cc:     syzbot <syzbot+b8e8c01c8ade4fe6e48f@syzkaller.appspotmail.com>,
        acme@kernel.org, alexander.shishkin@linux.intel.com,
        bpf@vger.kernel.org, jolsa@kernel.org,
        linux-kernel@vger.kernel.org, linux-perf-users@vger.kernel.org,
        mark.rutland@arm.com, mingo@redhat.com, namhyung@kernel.org,
        netdev@vger.kernel.org, syzkaller-bugs@googlegroups.com
References: <000000000000a20a2e05f029c577@google.com>
 <Y6B3xEgkbmFUCeni@hirez.programming.kicks-ass.net>
 <3a5a4738-2868-8f2f-f8b2-a28c10fbe25b@linux.dev>
 <Y6TFKdVJ9BY56fkI@hirez.programming.kicks-ass.net>
From:   Chengming Zhou <zhouchengming@bytedance.com>
In-Reply-To: <Y6TFKdVJ9BY56fkI@hirez.programming.kicks-ass.net>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 2022/12/23 04:59, Peter Zijlstra wrote:
> On Wed, Dec 21, 2022 at 10:42:39AM +0800, Chengming Zhou wrote:
> 
>>> Does this help?
>>>
>>> diff --git a/kernel/events/core.c b/kernel/events/core.c
>>> index e47914ac8732..bbff551783e1 100644
>>> --- a/kernel/events/core.c
>>> +++ b/kernel/events/core.c
>>> @@ -12689,7 +12689,8 @@ SYSCALL_DEFINE5(perf_event_open,
>>>  	return event_fd;
>>>  
>>>  err_context:
>>> -	/* event->pmu_ctx freed by free_event() */
>>> +	put_pmu_ctx(event->pmu_ctx);
>>> +	event->pmu_ctx = NULL; /* _free_event() */
>>>  err_locked:
>>>  	mutex_unlock(&ctx->mutex);
>>>  	perf_unpin_context(ctx);
>>
>> Tested-by: Chengming Zhou <zhouchengming@bytedance.com>
>>
>> While reviewing the code, I found perf_event_create_kernel_counter()
>> has the similar problem in the "err_pmu_ctx" error handling path:
> 
> Right you are, updated the patch, thanks!
> 
>> CPU0					CPU1
>> perf_event_create_kernel_counter()
>>   // inc ctx refcnt
>>   find_get_context(task, event) (1)
>>
>>   // inc pmu_ctx refcnt
>>   pmu_ctx = find_get_pmu_context()
>>
>>   event->pmu_ctx = pmu_ctx
>>   ...
>>   goto err_pmu_ctx:
>>     // dec pmu_ctx refcnt
>>     put_pmu_ctx(pmu_ctx) (2)
>>
>>     mutex_unlock(&ctx->mutex)
>>     // dec ctx refcnt
>>     put_ctx(ctx)
>> 					perf_event_exit_task_context()
>> 					  mutex_lock()
>> 					  mutex_unlock()
>> 					  // last refcnt put
>> 					  put_ctx()
>>     free_event(event)
>>       if (event->pmu_ctx) // True
>>         put_pmu_ctx() (3)
>>           // will access freed pmu_ctx or ctx
>>
>>       if (event->ctx) // False
>>         put_ctx()
> 
> This doesn't look right; iirc you can hit this without concurrency,
> something like so:

Right, pmu_ctx UaF can hit without concurrency.

But ctx has been created with refcnt == 1, which referenced by the task,
so the last refcnt put must be in perf_event_exit_task_context().

Maybe we can improve this, don't let ctx referenced by the task? Then ctx
can be freed when all perf_events are removed, instead of having to wait
for the task to exit. Maybe I missed something...

> 
> 
> 	// note that when getting here, we've not passed
> 	// perf_install_in_context() and event->ctx == NULL.
> err_pmu_ctx:
> 	put_pmu_ctx();
> 	put_ctx(); // last, actually frees ctx

This put_ctx() dec refcnt from 2 to 1, perf_event_exit_task_context()
will put the last refcnt and free it.

> 	..
> err_alloc:
> 	free_event()
> 	  _free_event()
> 	    if (event->pmu_ctx) // true, because we forgot to clear
> 	      put_pmu_ctx() // hits 0 because double put
> 	        // goes and touch epc->ctx and UaF
> 
>