perf: Fix mmap replacement ring lifetime race
PATCH v2 1/1 perf: Fix mmap replacement ring lifetime race
From: Kyle Zeng <[email protected]>
perf_mmap_close() drops the ring-local mmap_count before serializing with perf_mmap() through event->mmap_mutex. When the last mapping is being closed, a concurrent mmap can therefore observe a nonzero event->mmap_count and a zero rb->mmap_count.
In that case perf_mmap_rb() detaches the old ring, installs a replacement, and resets event->mmap_count to one. The old close then consumes that replacement count and detaches the new ring. Its pages can consequently be freed while the replacement VMA still maps their PFNs.
Take event->mmap_mutex before updating either count. This makes the ring-local and event-global count transitions atomic with respect to perf_mmap(), so a replacement cannot be installed until the old close has detached its ring.
---
Changes in v2:
- Restore Kyle Zeng as the patch author and correct the sign-off chain.
- Move the research credit below the commit-message separator.
v1: https://lore.kernel.org/all/[email protected]/
Bug found and triaged by OpenAI Security Research and validated by Trail of Bits.
Trail of Bits has a reproducer for this bug that triggers a kernel panic and can share if needed.
kernel/events/core.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)
kernel/events/core.c +9 -1
@@ -7053,11 +7053,19 @@ static void perf_mmap_close(struct vm_area_struct *vma)mutex_unlock (&rb->aux_mutex); } +/* +* Serialize both count updates with perf_mmap() so they cannot +* refer to different ring buffer generations. +*/ +mutex_lock (&event->mmap_mutex); +if (refcount_dec_and_test (&rb->mmap_count)) detach_rest = true; -if (!refcount_dec_and_mutex_lock (&event->mmap_count, &event->mmap_mutex)) +if (!refcount_dec_and_test (&event->mmap_count)) { +mutex_unlock (&event->mmap_mutex); goto out_put; + }ring_buffer_attach (event,NULL );mutex_unlock (&event->mmap_mutex);
Signature · 1 line
2.53.0
[email protected]
AI reviewThank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues:
- [High] Cross-mm pinned_vm and locked_vm accounting mismatch when main buffer and AUX buffer are mapped by different processes.
- [High] Error path in perf_mmap leaks ring buffer mmap counts and unaccounts wrong memory size for AUX mmaps on redirected events. --
--- Patch [1]: [PATCH v2] perf: Fix mmap replacement ring lifetime race --- commit 454633461f89e2fe1c245d071d92eff5dc95e16e
Author: Kyle Zeng <[email protected]>
Subject: perf: Fix mmap replacement ring lifetime race
perf_mmap_close() drops the ring-local mmap_count before serializing with perf_mmap() through event->mmap_mutex. This patch takes event->mmap_mutex before updating either count, ensuring the transitions are atomic with respect to perf_mmap().
Quoted 11 lines
kernel/events/core.c +3
-0@@ -7053,11 +7053,19 @@ static void perf_mmap_close(struct vm_area_struct *vma)mutex_unlock (&rb->aux_mutex); } +/* +* Serialize both count updates with perf_mmap() so they cannot +* refer to different ring buffer generations.
kernel/events/core.c static void perf_mmap_close(struct vm_area_struct *vma) · line 7067
Quoted 10 lines
kernel/events/core.c +5
-1+*/ +mutex_lock (&event->mmap_mutex); +if (refcount_dec_and_test (&rb->mmap_count)) detach_rest = true; -if (!refcount_dec_and_mutex_lock (&event->mmap_count, &event->mmap_mutex)) +if (!refcount_dec_and_test (&event->mmap_count)) { +mutex_unlock (&event->mmap_mutex); goto out_put;
[Severity: High]
This is a pre-existing issue, but does this mmap_count logic lead to a cross-mm pinned_vm and locked_vm accounting mismatch when the main buffer and AUX buffer are mapped by different processes?
If Process A maps the main ring buffer of an event, charging its VMA's pinned_vm, and the event fd is shared with Process B which maps the AUX buffer, rb->mmap_count tracks both mappings globally.
If Process A unmaps the main buffer first, the following evaluates to false:
if (refcount_dec_and_test(&rb->mmap_count)) detach_rest = true;
The mmap_count decrements but does not reach zero, skipping the later call to perf_mmap_unaccount().
When Process B later unmaps the AUX buffer, rb->mmap_count reaches zero and perf_mmap_unaccount() is called with Process B's VMA near the end of perf_mmap_close():
/* * Aside from that, this buffer is 'fully' detached and unmapped, * undo the VM accounting. */ perf_mmap_unaccount(vma, rb);
Does this unaccount the main buffer's size from Process B's limits, underflowing Process B's pinned_vm while permanently leaking the limits for Process A?
kernel/events/core.c static void perf_mmap_close(struct vm_area_struct *vma) · line 7071
Quoted 4 lines
kernel/events/core.c +1
-0+ }ring_buffer_attach (event,NULL );mutex_unlock (&event->mmap_mutex);
[Severity: High]
This is also a pre-existing issue, but looking at the related error path in perf_mmap(), could it leak ring buffer mmap counts and unaccount the wrong memory size for AUX mmaps on redirected events?
When mmap() is called for the AUX buffer of a redirected event, perf_mmap_aux() increments rb->mmap_count, rb->aux_mmap_count, and sets event->mmap_count to 1.
If map_range() fails due to an error, the cleanup path in perf_mmap() evaluates this:
if (refcount_read(&event->mmap_count) == 1) {
/*
* Minimal perf_mmap_close(); there can't be AUX or
* other events on account of this being the first.
*/
mapped = get_mapped(event, event_unmapped);
if (mapped)
mapped(event, vma->vm_mm);
perf_mmap_unaccount(vma, event->rb);
ring_buffer_attach(event, NULL);
refcount_set(&event->mmap_count, 0);
return ret;
}Because event->mmap_count is 1, it assumes this is an exclusive main buffer mmap. It then calls perf_mmap_unaccount(), which unaccounts the main buffer's size instead of the AUX buffer's size, potentially underflowing the caller's pinned_vm.
Additionally, it skips decrementing rb->mmap_count and rb->aux_mmap_count while detaching the event from the ring buffer. Could this cause a persistent memory leak of ring buffer pages and break event redirection?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1