Message-ID: <20240712090008.GA19796@noisy.programming.kicks-ass.net>
Date: Fri, 12 Jul 2024 11:00:08 +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 Thu, Jul 11, 2024 at 04:10:43PM -0700, Kees Cook wrote:

> The long answer is long, and comes in two halves: the language half and
> the fortify half.
> 
> First, the C standard requires that all function argument arrays be
> decayed to pointers, so your prototype is semantically handled as if
> it were:
> 
> void syscall_get_arguments(struct pt_regs *regs, unsigned long *args)
> 
> The "6" is just totally thrown away by the language. :(

Bah. I mean, I understand why, they *are* pointers after all. But to
then also discard the object size is just silly. I mean, traditionally C
doesn't give a crap about object size -- it is irrelevant.

But with alias analysis (which we switch off) and doubly so with this
fortify stuff, you do care about it.

So why throw it out?

> *However* the compilers _will_ do things with it while generating
> diagnostics, but only from the caller's perspective (code _inside_
> has no awareness of the "6" unless the function has been inlined, sort
> of). For example:
> 
> 	unsigned long toosmall[4];
> 	...
> 	syscall_get_arguments(regs, toosmall);
> 
> Produces:
> 
> <source>:60:5: warning: 'syscall_get_arguments' accessing 48 bytes in a region of size 32 [-Wstringop-overflow=]
>    60 |     syscall_get_arguments(regs, toosmall);
>       |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Makes sense.

> But syscall_get_arguments() internally has no idea what size "args" is:
> 
> void noinline syscall_get_arguments(struct pt_regs *regs, unsigned long args[6])
> {
> 	report(sizeof(args));
> 	report(__builtin_object_size(args, 1));
> 	report(__builtin_dynamic_object_size(args, 1));
>         memcpy(args, (typeof(args))&regs->bx, 6*sizeof(args[0]));
> }
> 
> Which reports 8, SIZE_MAX (unknown), and SIZE_MAX (unknown) respectively.

And that's just daft :/

> And the language is so busted about this that there is actually a
> diagnostic for "don't do that" that shows up with this code:
> 
> <source>: In function 'syscall_get_arguments':
> <source>:53:22: warning: 'sizeof' on array function parameter 'args' will return size of 'long unsigned int *' [-Wsizeof-array-argument]
>    53 |         report(sizeof(args));
>       |                      ^

:-( This just doesn't make sense. How can you be so inconsistent about
things.

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.

> _However_, if we _inline_ the function, suddenly _bos and _bdos know
> what's going on since they have visibility into the actual objection
> definition:
> 
> void inline syscall_get_arguments(struct pt_regs *regs, unsigned long args[6])

*sigh* yes ofcourse, inlineing in C is a mess, it really does change the
meaning of what you've written.

Combine this with the fact that 'inline' doesn't actually mean anything
much these days because the compiler will do whatever it feels like
anyway, and you've got a language that's like *really* hard to use.

Hence our noinline and __always_inline attributes to force it where we
need to.

> Now it reports 8, 32 (8 * the "toosmall" array size 4), 32 (same: _bdos
> falls back to _bos if there is no dynamic component). Note this is _not_
> 6, but 4, the actual object size.
> 
> Using the newer arg-sized array prototypes using a named argument _does_
> inform the internals, but that requires changing the calling convention
> for what is supposed to be a fixed size, and only behaves at runtime,
> which is just silly for compile-time fixed sizes. For example:
> 
> void noinline syscall_get_arguments(struct pt_regs *regs, int n, unsigned long args[n])
> ...
> 	syscall_get_arguments(regs, 6, toosmall);
> 
> 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?

> For the patch in this thread, the W=1 case was reported (overread of
> "bx"), because normally fortify would just ignore the overread of
> the source.

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.

> Finally to answer your question about working around this case, _bos/_bdos
> will see right through the casting because it operates on the actual
> object behind it. (And casts to an array type are illegal too.)
> 
>     unsigned long args[6];
> 
>     report(__builtin_object_size((typeof(args))&regs->bx, 1));
> 
> <source>: In function 'show':
> <source>:76:34: error: cast specifies array type
>    76 |     report(__builtin_object_size((typeof(args))&regs->bx, 1));
>       |                                  ^
> 
> And a (char *) cast doesn't work: _bos(1) reports 8, the size of bx. Using
> locals doesn't help either:
> 
>     void *ptr = (void *)&regs->bx;
> 
>     report(__builtin_object_size(ptr, 1));
> 
> Still 8. And ultimately this is good, since fortify will see through to
> the actual object that could get overflowed, etc. It's the behavior we
> want for the overflow defense.

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.

Powered by blists - more mailing lists