netdev - Re: [syzbot] KASAN: use-after-free Read in put_pmu

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 PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 22 Dec 2022 21:59:21 +0100
From:   Peter Zijlstra <peterz@...radead.org>
To:     Chengming Zhou <zhouchengming@...edance.com>
Cc:     syzbot <syzbot+b8e8c01c8ade4fe6e48f@...kaller.appspotmail.com>,
        acme@...nel.org, alexander.shishkin@...ux.intel.com,
        bpf@...r.kernel.org, jolsa@...nel.org,
        linux-kernel@...r.kernel.org, linux-perf-users@...r.kernel.org,
        mark.rutland@....com, mingo@...hat.com, namhyung@...nel.org,
        netdev@...r.kernel.org, syzkaller-bugs@...glegroups.com
Subject: Re: [syzbot] KASAN: use-after-free Read in put_pmu_ctx

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@...edance.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:


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