linux-kernel - Re: [PATCH] x86/syscall: Avoid memcpy() for ia32 syscall_get

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: <202407150947.92A48C959@keescook>
Date: Mon, 15 Jul 2024 10:01:56 -0700
From: Kees Cook <kees@...nel.org>
To: Peter Zijlstra <peterz@...radead.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 Mon, Jul 15, 2024 at 10:37:13AM +0200, Peter Zijlstra wrote:
> 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.

Precisely.

> So yeah, language extensions are it.

The one I may try to point out to the committee is flexible arrays in
unions. "array[1]" and "array[0]" are allowed in unions, but "array[]"
wasn't. This totally wrecks attempts to modernize a codebase that
depends on such union uses. We worked around it but finally got the
language extension, er, extended, recently:

https://github.com/llvm/llvm-project/commit/14ba782a87e16e9e15460a51f50e67e2744c26d9
https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=adb1c8a0f167c3a1f7593d75f5a10eb07a5d741a

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

Yup. Between the above flex arrays in unions fix and -fstrict-flex-arrays=3,
a C codebase can actually get unambiguous array bounds handling. And now
with the "counted_by" attribute, we can cover _dynamic_ arrays too.

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

Yeah, that would be possible, but I wanted something that the compiler
could reason about for a given identifier since it's not just fortify
that cares about object bounds. Being able to declare the layouts so
that the bounds sanitizer instrumentation wouldn't get confused was
important too. That is more related to arrays than integral members, but
separating those quickly became confusing to declare easily/correctly. So
struct_group() ended up being the best direction in the general case.

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

Yup, that works too. (Though pt_regs is relatively unique in this "the
whole thing is expected to be an array" characteristic.)

-Kees

-- 
Kees Cook