Received-SPF: pass (google.com: domain of linux-kernel+bounces-212131-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Wed, 12 Jun 2024 12:01:19 -0700
From: Kees Cook <kees@kernel.org>
To: Peter Zijlstra <peterz@infradead.org>
Cc: Erick Archer <erick.archer@outlook.com>, Ingo Molnar <mingo@redhat.com>,
	Arnaldo Carvalho de Melo <acme@kernel.org>,
	Namhyung Kim <namhyung@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Alexander Shishkin <alexander.shishkin@linux.intel.com>,
	Jiri Olsa <jolsa@kernel.org>, Ian Rogers <irogers@google.com>,
	Adrian Hunter <adrian.hunter@intel.com>,
	"Liang, Kan" <kan.liang@linux.intel.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	Borislav Petkov <bp@alien8.de>,
	Dave Hansen <dave.hansen@linux.intel.com>,
	"H. Peter Anvin" <hpa@zytor.com>,
	"Gustavo A. R. Silva" <gustavoars@kernel.org>,
	Nathan Chancellor <nathan@kernel.org>,
	Nick Desaulniers <ndesaulniers@google.com>,
	Bill Wendling <morbo@google.com>,
	Justin Stitt <justinstitt@google.com>,
	Christophe JAILLET <christophe.jaillet@wanadoo.fr>,
	Matthew Wilcox <mawilcox@microsoft.com>, x86@kernel.org,
	linux-perf-users@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-hardening@vger.kernel.org, llvm@lists.linux.dev
Subject: Re: [PATCH v4 0/3] Hardening perf subsystem
Message-ID: <202406121148.688240B@keescook>
References: <AS8PR02MB7237F5BFDAA793E15692B3998BFD2@AS8PR02MB7237.eurprd02.prod.outlook.com>
 <202406101010.E1C77AE9D@keescook>
 <20240610200544.GY8774@noisy.programming.kicks-ass.net>
 <202406101438.BC43514F@keescook>
 <20240611075542.GD8774@noisy.programming.kicks-ass.net>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240611075542.GD8774@noisy.programming.kicks-ass.net>

On Tue, Jun 11, 2024 at 09:55:42AM +0200, Peter Zijlstra wrote:
> On Mon, Jun 10, 2024 at 02:46:09PM -0700, Kees Cook wrote:
> 
> > > I really detest this thing because it makes what was trivially readable
> > > into something opaque. Get me that type qualifier that traps on overflow
> > > and write plain C. All this __builtin_overflow garbage is just that,
> > > unreadable nonsense.
> > 
> > It's more readable than container_of(), 
> 
> Yeah, no. container_of() is absolutely trivial and very readable.
> container_of_const() a lot less so.

I mean, we have complex macros in the kernel. This isn't uncommon. Look
at cleanup.h. ;)

But that's why we write kern-doc:

 * struct_size() - Calculate size of structure with trailing flexible
 * array.
 * @p: Pointer to the structure.
 * @member: Name of the array member.
 * @count: Number of elements in the array.

> #define struct_size(p, member, num) \
> 	mult_add_no_overflow(num, sizeof(p->member), sizeof(*p))
> 
> would be *FAR* more readable. And then I still think struct_size() is
> less readable than its expansion. ]]

I'm happy to take patches. And for this bikeshed, this would be better
named under the size_*() helpers which are trying to keep size_t
calculations from overflowing (by saturating). i.e.:

	size_add_mult(sizeof(*p), sizeof(*p->member), num)

Generalized overflow handing (check_add/sub/mul_overflow()) helpers
already exist and requires a destination variable to determine type
sizes. It is not safe to allow C semantics to perform
promotion/truncation, so we have to be explicit.

> Some day I'll have to look at this FORTIFY_SOURCE and see what it
> actually does I suppose :/

LOL. It's basically doing compile-time (__builtin_object_size) and
run-time (__builtin_dynamic_object_size) bounds checking on destination
(and source) object sizes, mainly driven by the mentioned builtins:
https://gcc.gnu.org/onlinedocs/gcc/Object-Size-Checking.html

> I coulnd't quickly find a single instance in the code I care about. So
> nothing is sailing afaict.

I have to care about all of Linux's code. :P We generally don't have to
defend the kernel from perf, so the hardening changes tend to end up in
core APIs along with compiler improvements.

Anyway! What about the patch that takes the 2 allocations down to 1?
That seems like an obvious improvement.

-Kees

-- 
Kees Cook