Skip to content

Commit

Permalink
perf/core: Fix race in the perf_mmap_close() function
Browse files Browse the repository at this point in the history
to #31887426

commit f91072e upstream.

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.

Tested-by: Michael Petlan <[email protected]>
Signed-off-by: Jiri Olsa <[email protected]>
Signed-off-by: Ingo Molnar <[email protected]>
Acked-by: Peter Zijlstra <[email protected]>
Acked-by: Namhyung Kim <[email protected]>
Acked-by: Wade Mealing <[email protected]>
Fixes: 9bb5d40 ("perf: Fix mmap() accounting hole");
Link: https://lore.kernel.org/r/20200916115311.GE2301783@krava
[sudip: used ring_buffer]
Signed-off-by: Sudip Mukherjee <[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>

Fixes: CVE-2020-14351
Signed-off-by: Shile Zhang <[email protected]>
Acked-by: Michael Wang <[email protected]>
  • Loading branch information
Jiri Olsa authored and shiloong committed Dec 17, 2020
1 parent 3e66ef4 commit a6b7a72
Showing 1 changed file with 4 additions and 3 deletions.
7 changes: 4 additions & 3 deletions kernel/events/core.c
Original file line number Diff line number Diff line change
Expand Up @@ -5498,11 +5498,11 @@ static void perf_pmu_output_stop(struct perf_event *event);
static void perf_mmap_close(struct vm_area_struct *vma)
{
struct perf_event *event = vma->vm_file->private_data;

struct ring_buffer *rb = ring_buffer_get(event);
struct user_struct *mmap_user = rb->mmap_user;
int mmap_locked = rb->mmap_locked;
unsigned long size = perf_data_size(rb);
bool detach_rest = false;

if (event->pmu->event_unmapped)
event->pmu->event_unmapped(event, vma->vm_mm);
Expand Down Expand Up @@ -5533,7 +5533,8 @@ static void perf_mmap_close(struct vm_area_struct *vma)
mutex_unlock(&event->mmap_mutex);
}

atomic_dec(&rb->mmap_count);
if (atomic_dec_and_test(&rb->mmap_count))
detach_rest = true;

if (!atomic_dec_and_mutex_lock(&event->mmap_count, &event->mmap_mutex))
goto out_put;
Expand All @@ -5542,7 +5543,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 (!detach_rest)
goto out_put;

/*
Expand Down

0 comments on commit a6b7a72

Please sign in to comment.