linux-kernel - Re: [PATCH v2] perf: Fix refcount warning on event->mmap

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 linux-cve-announce 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]

Message-ID: <20260106093431.GA3707837@noisy.programming.kicks-ass.net>
Date: Tue, 6 Jan 2026 10:34:31 +0100
From: Peter Zijlstra <peterz@...radead.org>
To: Will Rosenberg <whrosenb@....edu>
Cc: yi1.lai@...ux.intel.com, Ingo Molnar <mingo@...hat.com>,
	Arnaldo Carvalho de Melo <acme@...nel.org>,
	Namhyung Kim <namhyung@...nel.org>,
	Mark Rutland <mark.rutland@....com>,
	Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
	Jiri Olsa <jolsa@...nel.org>, Ian Rogers <irogers@...gle.com>,
	Adrian Hunter <adrian.hunter@...el.com>,
	James Clark <james.clark@...aro.org>,
	Lorenzo Stoakes <lorenzo.stoakes@...cle.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	"open list:PERFORMANCE EVENTS SUBSYSTEM" <linux-perf-users@...r.kernel.org>,
	"open list:PERFORMANCE EVENTS SUBSYSTEM" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] perf: Fix refcount warning on event->mmap_count
 increment

On Mon, Jan 05, 2026 at 09:51:49AM -0700, Will Rosenberg wrote:
> When calling refcount_inc(&event->mmap_count) inside perf_mmap_rb(), the
> following warning is triggered:
> 
> 	refcount_t: addition on 0; use-after-free.
> 	WARNING: lib/refcount.c:25
> 
> PoC:
> 
>     struct perf_event_attr attr = {0};
>     int fd = syscall(__NR_perf_event_open, &attr, 0, -1, -1, 0);
>     mmap(NULL, 0x3000, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0);
>     int victim = syscall(__NR_perf_event_open, &attr, 0, -1, fd,
>                          PERF_FLAG_FD_OUTPUT);
>     mmap(NULL, 0x3000, PROT_READ | PROT_WRITE, MAP_SHARED, victim, 0);
> 
> This occurs when creating a group member event with the flag
> PERF_FLAG_FD_OUTPUT. The group leader should be mmap-ed and then mmap-ing
> the event triggers the warning.
> 
> Since the event has copied the output_event in perf_event_set_output(),
> event->rb is set. As a result, perf_mmap_rb() calls
> refcount_inc(&event->mmap_count) when event->mmap_count = 0.
> 
> Account for the case when event->mmap_count = 0. This patch goes against
> the design philosophy of the refcount library by re-enabling an empty
> refcount, but the patch remains inline with the current treatment of
> mmap_count.
> 
> Fixes: 448f97fba901 ("perf: Convert mmap() refcounts to refcount_t")
> Signed-off-by: Will Rosenberg <whrosenb@....edu>
> ---
> 
> Notes:
>     v1 -> v2: Add Fixes tag
>     
>     I also have a related concern about code that handles the mmap_count.
>     In perf_mmap_close(), if refcount_dec_and_mutex_lock() decrements
>     event->mmap_count to zero, then event->rb is set to NULL. This
>     effectively undos our ring buffer copy. However, is this desired
>     behavior? Should event->rb remain unchanged since it may still be
>     mmap-ed by other events and can still be used?
> 
>  kernel/events/core.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 376fb07d869b..49709b627b1f 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -7279,7 +7279,8 @@ static int perf_mmap_rb(struct vm_area_struct *vma, struct perf_event *event,
>  			 * multiple times.
>  			 */
>  			perf_mmap_account(vma, user_extra, extra);
> -			refcount_inc(&event->mmap_count);
> +			if (!refcount_inc_not_zero(&event->mmap_count))
> +				refcount_set(&event->mmap_count, 1);

So this pattern was an instant red flag; this cannot be right.

Yes, this makes the error go away, but I think the result is bad. 

The sequence as provided will create a mapping for event fd, and create
victim such that its events are redirected to this buffer.

So far so good.

However, the mmap() of victim will create an alias of the earlier buffer
(which is pointless but isn't a problem per-se), but by setting
event->mmap_count, you're saying this second event should also update
the user_page (struct perf_event_mmap_page at offset +0).

This means that if you make this succeed you end up with both events
(fd, victim) writing to the same page (which is mapped twice). And that
is broken.

I'm thinking we should dis-allow this mmap()... something like so, hmm?

---
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 3c2a491200c6..ccf3aecbfff5 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -7273,6 +7273,15 @@ static int perf_mmap_rb(struct vm_area_struct *vma, struct perf_event *event,
 		if (data_page_nr(event->rb) != nr_pages)
 			return -EINVAL;
 
+		/*
+		 * If this event doesn't have mmap_count, we're attempting to
+		 * create an alias of another event's mmap(); this would mean
+		 * both events will end up scribbling the same user_page;
+		 * which makes no sense.
+		 */
+		if (refcount_read(&event->mmap_count))
+			return -EBUSY;
+
 		if (refcount_inc_not_zero(&event->rb->mmap_count)) {
 			/*
 			 * Success -- managed to mmap() the same buffer