Re: [PATCH v4 0/3] Hardening perf subsystem

[Peter Zijlstra]

On Wed, Jun 12, 2024 at 12:01:19PM -0700, Kees Cook wrote:
> 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. ;)

Yeah, but in those cases the macro actually saves a lot of typing. A
mult and add is something you shouldn't be needing a macro for.
	
> But that's why we write kern-doc:

Right, but reading comments is asking for trouble. That is, I really
don't even see comments -- I have a built-in pre-processor that filters
them out. I've been burned too many times by misleading or flat out
wrong comments.

> > #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)

Fine I suppose, but what if we want something not size_t? Are we waiting
for the type system extension?

Anyway, I'll take the patch with the above. That at least is readable.

The saturating thing is relying in the allocators never granting INT_MAX
(or whatever size_t actually is) bytes?

> 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.

Yeah, those are a sodding trainwreck, much like the
__builtin_*_overflow() garbage. Can't we simply have them trap on
overflow and do away with that most terrible calling convention?

If we want to be really fancy we can have a #UD instruction that encodes
a destination register to clear/saturate/whatever.

> > 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

Right, I got that far. I also read most of:

  https://discourse.llvm.org/t/rfc-enforcing-bounds-safety-in-c-fbounds-safety/70854

But none of that is showing me generated asm for the various cases. As
such, I don't consider myself informed enough.

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

Separate it from the struct_size() nonsense and Cc the author of that
code (Sandipan IIRC) and I might just apply it.