linux-kernel - Re: [PATCH] x86: fix user address masking non-canonical speculation issue

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: <CAHk-=wgynHGhG9dzwRdySJSHZTOCp9jBHChomEF-mERJmsUeQg@mail.gmail.com>
Date: Thu, 24 Oct 2024 10:53:35 -0700
From: Linus Torvalds <torvalds@...ux-foundation.org>
To: Borislav Petkov <bp@...en8.de>
Cc: Linux Kernel Mailing List <linux-kernel@...r.kernel.org>, x86@...nel.org, 
	Andrew Cooper <andrew.cooper3@...rix.com>, Josh Poimboeuf <jpoimboe@...nel.org>
Subject: Re: [PATCH] x86: fix user address masking non-canonical speculation issue

On Thu, 24 Oct 2024 at 04:08, Borislav Petkov <bp@...en8.de> wrote:
>
> Can we make this more readable pls?
>
> Something like this:
>
> static inline void __user *mask_user_address(const void __user *ptr)
> {
>         void __user *ret;
>
>         asm("cmp %[ptr],%[ret]\n\t"
>             "sbb %[ret],%[ret]\n\t"
>             "or  %[ptr],%[ret]"
>                 : [ret] "=r" (ret)
>                 : [ptr] "r" (ptr),
>                   "0" (runtime_const_ptr(USER_PTR_MAX)));

Will do at least for the newlines.

I actually find the '%[ret]' kind of verbosity *less* readable than
using the numbers that are needed anyway to disambiguate the
input/output thing.

Yes, named arguments are good when there's enough of them to be a big
deal, but in this case the variable naming ends up just becoming line
noise and overwhelming the actual code in question.

> This way the compiler-generated asm is more readable too due to the newlines
> and tabs.

Ack. I don't find the compiler-generated asm very readable due to all
the magic section tricks, but certainly that can be avoided for the
cmp/sbb/or sequence.

> In any case, it looks good, I single-stepped strnlen_user() and I got:

Thanks, looks good.

> Looking at Documentation/arch/x86/x86_64/mm.rst, 5 level page tables define
> USR_PTR_MAX as 0x00ffffffffffffff, i.e., bits [55:0].
>
> So I guess that USER_PTR_MAX needs to look at X86_FEATURE_LA57, no?

The *page tables* only cover bits [55:0] of user space, yes.

But the user *pointers* are bigger. That's the point of LAM. The upper
bits are masked off by hardware.

So a valid user pointer with LAM57 looks like:

 [63: MBZ] [62-57: masked off] [56: MBZ] [55-0: used for TLB lookup]

LAM48 is the same, except obviously [62-48] are masked off, and bit 47
is the one that must-be-zero.

So as far as the kernel is concerned, any address with the top bit
clear is a user address.

So with LAM, you basically end up with the sign bit test again (except
that we still don't allow the last page of user space to be mapped).

We could also take the whole "bit 56 must be zero too" thing into
account and further tighten it, but it really doesn't change the
situation in any material ways, since we have that page-sized hole at
the end of the user address space anyway even without it (and with
LAM, the upper bit masking just means that special guard page is
repeated in the linear address space).

                   Linus