Message-ID: <20240715083713.GX27299@noisy.programming.kicks-ass.net>
Date: Mon, 15 Jul 2024 10:37:13 +0200
From: Peter Zijlstra <peterz@...radead.org>
To: Kees Cook <kees@...nel.org>
Cc: Thomas Gleixner <tglx@...utronix.de>,
	Mirsad Todorovac <mtodorovac69@...il.com>,
	Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
	Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org,
	"H. Peter Anvin" <hpa@...or.com>,
	Daniel Sneddon <daniel.sneddon@...ux.intel.com>,
	Arnd Bergmann <arnd@...db.de>, Brian Gerst <brgerst@...il.com>,
	Josh Poimboeuf <jpoimboe@...nel.org>,
	Pawan Gupta <pawan.kumar.gupta@...ux.intel.com>,
	Peter Collingbourne <pcc@...gle.com>, linux-kernel@...r.kernel.org,
	linux-hardening@...r.kernel.org
Subject: Re: [PATCH] x86/syscall: Avoid memcpy() for ia32
 syscall_get_arguments()

On Fri, Jul 12, 2024 at 10:55:16AM -0700, Kees Cook wrote:

> > What will actually break if you 'fix' this? Given that inlining (see
> > below) changes the rules willy nilly, I feel we can (and should!) just
> > fix this.
> 
> I'm not sure -- I have kind of given up on "standard" C helping with any
> of this. I look to consistent language extensions now, and where there
> isn't any, we've been trying to create them. :P 

Yeah, arguing a committee is mostly a waste of time, also, they
typically listen a lot more when you say, here these two compilers have
implemented it and this Linux thing uses it.

So yeah, language extensions are it.

> And we're not alone:
> Apple's -fbounds-safety stuff[1] looks good too, and overlaps with what
> we were already designing with the "counted_by" attribute:
> https://discourse.llvm.org/t/rfc-enforcing-bounds-safety-in-c-fbounds-safety/
> (We borrowed the "counted_by" name, which is better than what we were
> calling it: "element_count".)

Yep, I read that a while back. I think you referenced it in one of them
other threads where we disagreed over struct_size() :-)

> > > Does report the expected things for _bdos internally (48), but not for
> > > sizeof (8) nor _bos (SIZE_MAX). Of course if we inline it, _bos starts
> > > working and, along with _bdos, realizes it was lied to, and reports
> > > 32 again.
> > 
> > WTF ?!?! How can all this be so inconsistent and why are people okay
> > with that?
> 
> This. A thousands times, this. I'm really not okay with it, and we've
> been working to get rid of every ambiguity we trip over. It's made sane
> bounds checking in Linux extremely hard to get right.

Yeah, not just Linux I imagine. The rules are so insane it's near
useless. I'd say press onwards with the language extension, it's not
like Linux kernel is written in ANSI/ISO C anyway :-)

> For more fun with array bounds, the one that absolutely floored me was
> the exception over trailing arrays:
> 
> struct middle_t {
> 	u8 array[6];
> 	int foo;
> } *middle;
> 
> __builtin_object_size(middle->array, 1)  ==  6
> 
> struct trailing_t {
> 	int foo;
> 	u8 array[6];
> } *trailing;
> 
> __builtin_object_size(trailing->array, 1)  ==  SIZE_MAX ("unknown")

WTF :-)

> > So I'm not entirely sure I agree with that argument. Yes, &regs->bx is
> > 'unsigned long *' and sizeof(unsigned long) is 8 (if we assume 64bit).
> > However, you can also read it as the point of pt_regs where bx sits --
> > which is a far more sensible interpretation IMO.
> > 
> > Because then we're looking at struct pt_regs and an offset therein.
> 
> Right -- the way to make this unambiguous has been to make sure there
> is an addressable object which contains the elements in question. For
> the least disruption, the best we were able to do is introduce the
> struct_group() helper. It's internally ugly, but it works.

That macro is fairly trivial, nowhere near as ugly as struct_size() :-)
But urgh... can't we do something like:

void *memcpy_off(void *dst, const void *src, size_t off, size_t n)
{
	memcpu(dst, src+off, n);
	return dst;
}

And then you can write:

  memcpy_off(args, regs, offsetof(*regs, bx), 6);

I mean, that sucks, but possilby less than struct_group() does.

[ also, we should probably do:
  #defime offsetof(t, m) __builtin_offsetof(typeof(t), m) ]

> > So really pt_regs *is* an array of unsigned long, and I feel it is
> > really unfortunate we cannot express this in a way that is more concise.
> 
> A way to do this would be:
> 
> struct pt_regs {
> 	union {
> 		struct {
> 			unsigned long bx;
> 			unsigned long cx;
> 			unsigned long dx;
> 			unsigned long si;
> 			unsigned long di;
> 			unsigned long bp;
> 		};
> 		unsigned long syscall_regs[6];
> 	};
> 	unsigned long ax;
> 	...
> };
> 
> Now regs->syscall_regs is addressable, sized, etc. "bx" means just bx,
> and "syscall_regs" means all 6.

In this case I would just make all of pt_regs a union with one giant
array (much like some archs already have IIRC).

> I wrote up a bunch of notes about much of this horror last year here:
> https://people.kernel.org/kees/bounded-flexible-arrays-in-c

Oh, yeah, I think I saw that fly by on hackernews a while ago.

Powered by blists - more mailing lists