Messages in this thread | | | From | Namhyung Kim <> | Date | Thu, 10 Sep 2020 22:48:02 +0900 | Subject | Re: [PATCH] perf: Fix race in perf_mmap_close function |
| |
On Thu, Sep 10, 2020 at 7:42 PM Jiri Olsa <jolsa@kernel.org> wrote: > > There's a possible race in perf_mmap_close when checking ring buffer's > mmap_count refcount value. The problem is that the mmap_count check is > not atomic because we call atomic_dec and atomic_read separately. > > perf_mmap_close: > ... > atomic_dec(&rb->mmap_count); > ... > if (atomic_read(&rb->mmap_count)) > goto out_put; > > <ring buffer detach> > free_uid > > out_put: > ring_buffer_put(rb); /* could be last */ > > The race can happen when we have two (or more) events sharing same ring > buffer and they go through atomic_dec and then they both see 0 as refcount > value later in atomic_read. Then both will go on and execute code which > is meant to be run just once. > > The code that detaches ring buffer is probably fine to be executed more > than once, but the problem is in calling free_uid, which will later on > demonstrate in related crashes and refcount warnings, like: > > refcount_t: addition on 0; use-after-free. > ... > RIP: 0010:refcount_warn_saturate+0x6d/0xf > ... > Call Trace: > prepare_creds+0x190/0x1e0 > copy_creds+0x35/0x172 > copy_process+0x471/0x1a80 > _do_fork+0x83/0x3a0 > __do_sys_wait4+0x83/0x90 > __do_sys_clone+0x85/0xa0 > do_syscall_64+0x5b/0x1e0 > entry_SYSCALL_64_after_hwframe+0x44/0xa9 > > Using atomic decrease and check instead of separated calls. > This fixes CVE-2020-14351. > > Signed-off-by: Jiri Olsa <jolsa@kernel.org> > --- > kernel/events/core.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/kernel/events/core.c b/kernel/events/core.c > index 7ed5248f0445..29313cc54d9e 100644 > --- a/kernel/events/core.c > +++ b/kernel/events/core.c > @@ -5903,8 +5903,6 @@ static void perf_mmap_close(struct vm_area_struct *vma) > mutex_unlock(&event->mmap_mutex); > } > > - atomic_dec(&rb->mmap_count); > - > if (!atomic_dec_and_mutex_lock(&event->mmap_count, &event->mmap_mutex)) > goto out_put;
But when it takes the goto, rb->mmap_count won't decrement anymore..
Thanks Namhyung
> > @@ -5912,7 +5910,7 @@ static void perf_mmap_close(struct vm_area_struct *vma) > mutex_unlock(&event->mmap_mutex); > > /* If there's still other mmap()s of this buffer, we're done. */ > - if (atomic_read(&rb->mmap_count)) > + if (!atomic_dec_and_test(&rb->mmap_count)) > goto out_put; > > /* > -- > 2.26.2 >
| |