linux-kernel - Re: [PATCH v3 2/6] x86/uaccess: Avoid barrier_nospec() in 64-bit __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: <20241122031115.5aasuktqrp2sidfj@jpoimboe>
Date: Thu, 21 Nov 2024 19:11:15 -0800
From: Josh Poimboeuf <jpoimboe@...nel.org>
To: Linus Torvalds <torvalds@...ux-foundation.org>
Cc: David Laight <David.Laight@...lab.com>,
	"x86@...nel.org" <x86@...nel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Borislav Petkov <bp@...en8.de>,
	Peter Zijlstra <peterz@...radead.org>,
	Pawan Gupta <pawan.kumar.gupta@...ux.intel.com>,
	Waiman Long <longman@...hat.com>,
	Dave Hansen <dave.hansen@...ux.intel.com>,
	Ingo Molnar <mingo@...hat.com>,
	Michael Ellerman <mpe@...erman.id.au>,
	"linuxppc-dev@...ts.ozlabs.org" <linuxppc-dev@...ts.ozlabs.org>,
	Andrew Cooper <andrew.cooper3@...rix.com>,
	Mark Rutland <mark.rutland@....com>,
	"Kirill A . Shutemov" <kirill@...temov.name>
Subject: Re: [PATCH v3 2/6] x86/uaccess: Avoid barrier_nospec() in 64-bit
 __get_user()

On Thu, Nov 21, 2024 at 05:02:06PM -0800, Linus Torvalds wrote:
> [ Time passes ]
> 
> Ugh. I tried it. It looks like this:
> 
> #define inlined_get_user(res, ptr) ({                           \
>         __label__ fail2, fail1;                                 \
>         __auto_type __up = (ptr);                               \
>         int __ret = 0;                                          \
>         if (can_do_masked_user_access())                        \
>                 __up = masked_user_access_begin(__up);          \
>         else if (!user_read_access_begin(__up, sizeof(*__up)))  \
>                 goto fail1;                                     \
>         unsafe_get_user(res, ptr, fail2);                       \
>         user_access_end();                                      \
>         if (0) {                                                \
> fail2:  user_access_end();                                      \
> fail1:  __ret = -EFAULT;                                        \
>         }                                                       \
>         __ret; })

That actually doesn't seem so bad, it's easy enough to follow the logic.
And it contains the ugly/fidgety all in one place so the callers' hands
don't have to get dirty.

We could easily use that macro in size-specific inline functions
selected by a macro with a sizeof(type) switch statement -- not so bad
IMO if they improve code usage and generation.

So all the user has to do is get_user_new_and_improved() -- resolving to
get_user_new_and_improved_x() -- and the compiler decides on the
inlining.  Which on average is hopefully better than Joe Developer's
inlining decisions?  Otherwise we've got bigger problems?

Then all the arches have to do is implement unsafe_*_user_{1,2,4,8} and
the "one good implementation" idea comes together?

BTW, looking at some other arches, I notice that get_user() is already
unconditionally inline for arm64, riscv, powerpc, and s390.

I also see that arm64 already defines get_user() to __get_user(), with
__get_user() having an access_ok().

It would be really nice to have the same behavior and shared code across
all the arches.

-- 
Josh