linux-kernel - Re: [PATCH v2] perf/ring_buffer: Prefer struct

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: <202405060856.53AFAE4F22@keescook>
Date: Mon, 6 May 2024 09:23:15 -0700
From: Kees Cook <keescook@...omium.org>
To: Erick Archer <erick.archer@...look.com>
Cc: Christophe JAILLET <christophe.jaillet@...adoo.fr>,
	Peter Zijlstra <peterz@...radead.org>,
	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>,
	"Liang, Kan" <kan.liang@...ux.intel.com>,
	"Gustavo A. R. Silva" <gustavoars@...nel.org>,
	Nathan Chancellor <nathan@...nel.org>,
	Nick Desaulniers <ndesaulniers@...gle.com>,
	Bill Wendling <morbo@...gle.com>,
	Justin Stitt <justinstitt@...gle.com>,
	linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-hardening@...r.kernel.org, llvm@...ts.linux.dev
Subject: Re: [PATCH v2] perf/ring_buffer: Prefer struct_size over open coded
 arithmetic

On Sun, May 05, 2024 at 07:31:24PM +0200, Erick Archer wrote:
> On Sun, May 05, 2024 at 05:24:55PM +0200, Christophe JAILLET wrote:
> > Le 05/05/2024 à 16:15, Erick Archer a écrit :
> > > diff --git a/kernel/events/ring_buffer.c b/kernel/events/ring_buffer.c
> > > index 4013408ce012..080537eff69f 100644
> > > --- a/kernel/events/ring_buffer.c
> > > +++ b/kernel/events/ring_buffer.c
> > > @@ -822,9 +822,7 @@ struct perf_buffer *rb_alloc(int nr_pages, long watermark, int cpu, int flags)
> > >   	unsigned long size;
> > 
> > Hi,
> > 
> > Should size be size_t?
> 
> I'm sorry, but I don't have enough knowledge to answer this question.
> The "size" variable is used as a return value by struct_size and as
> a parameter to the order_base_2() and kzalloc_node() functions.

For Linux, size_t and unsigned long are the same (currently).
Pedantically, yes, this should be size_t, but it's the same.

> [...]
> > 	all_buf = vmalloc_user((nr_pages + 1) * PAGE_SIZE);
> >	if (!all_buf)
> >		goto fail_all_buf;
> >
> >	rb->user_page = all_buf;
> >	rb->data_pages[0] = all_buf + PAGE_SIZE;
> >	if (nr_pages) {					<--- here
> >		rb->nr_pages = 1;			<---
> >		rb->page_order = ilog2(nr_pages);
> >	}
> [...]
> I think that we don't need to deal with the "nr_pages = 0" case
> since the flex array will always have a length of one.
> 
> Kees, can you help us with this?

Agh, this code hurt my head for a while.

all_buf contains "nr_pages + 1" pages. all_buf gets attached to
rb->user_page, and then rb->data_pages[0] points to the second page in
all_buf... which means, I guess, that rb->data_pages does only have 1
entry.

However, the nr_pages == 0 case is weird. Currently, data_pages[0] will
still get set (which points ... off the end of all_buf). If we
unconditionally set rb->nr_pages to 1, we're changing the behavior. If
we _don't_ set rb->data_pages[0], we're changing the behavior, but I
think it's an invalid pointer anyway, so this is the safer change to
make. I suspect the right replacement is:


diff --git a/kernel/events/ring_buffer.c b/kernel/events/ring_buffer.c
index 4013408ce012..7d638ce76799 100644
--- a/kernel/events/ring_buffer.c
+++ b/kernel/events/ring_buffer.c
@@ -916,15 +916,11 @@ void rb_free(struct perf_buffer *rb)
 struct perf_buffer *rb_alloc(int nr_pages, long watermark, int cpu, int flags)
 {
 	struct perf_buffer *rb;
-	unsigned long size;
 	void *all_buf;
 	int node;
 
-	size = sizeof(struct perf_buffer);
-	size += sizeof(void *);
-
 	node = (cpu == -1) ? cpu : cpu_to_node(cpu);
-	rb = kzalloc_node(size, GFP_KERNEL, node);
+	rb = kzalloc_node(struct_size(rb, nr_pages, 1), GFP_KERNEL, node);
 	if (!rb)
 		goto fail;
 
@@ -935,9 +931,9 @@ struct perf_buffer *rb_alloc(int nr_pages, long watermark, int cpu, int flags)
 		goto fail_all_buf;
 
 	rb->user_page = all_buf;
-	rb->data_pages[0] = all_buf + PAGE_SIZE;
 	if (nr_pages) {
 		rb->nr_pages = 1;
+		rb->data_pages[0] = all_buf + PAGE_SIZE;
 		rb->page_order = ilog2(nr_pages);
 	}
 


Also, why does rb_alloc() take an "int" nr_pages? The only caller has an
unsigned long argument for nr_pages. Nothing checks for >INT_MAX that I
can find.

-- 
Kees Cook