Re: [PATCH] x86/uaccess: Zero the 8-byte get_range case on failure

[David Gow <davidgow@google.com>]

On Wed, 31 Jul 2024 09:38:15 -0700 Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Wed, 31 Jul 2024 at 09:24, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > My bad. My mental model these days is the 64-bit case, where the whole
> > 'check_range' thing is about address masking tricks, not the actual
> > conditional. So I didn't think of the "access_ok fails" case at all.
> 
> Actually, now that I said that out loud, it made me go "why aren't we
> doing that on 32-bit too?"
> 
> IOW, an alternative would be to just unify things more. Something like this?
> 
> *ENTIRELY UNTESTED*.

I tested this on everything I had on hand, and it passes the usercopy
KUnit tests on:
- QEMU, in every config I tried
- A Core i7-4770K (Haswell), under KVM on a 64-bit host kernel
- A Core 2 Duo E6600, bare metal
- A 486/DX2, bare metal

The 486 seemed to treat the wraparound a bit differently: it's triggering
a General Protection Fault, and so giving the WARN() normally reserved
for non-canonical addresses.

> 
> And no, this is not a NAK of David's patch. Last time I said "let's
> unify things", it caused this bug.
> 
> I'm claiming that finishing that unification will fix the bug again,
> and I *think* we leave that top address unmapped on 32-bit x86 too,
> but this whole trick does very much depend on that "access to all-one
> address is guaranteed to fail".
> 
> So this is the "maybe cleaner, but somebody really needs to
> double-check me" patch.
> 
>              Linus
>

I think this is right (there's definitely an unmapped page at the top),
but I'd want someone who knows better to verify that this won't do
something weird with segmentation and/or speculation with the 8-byte case
(if vm.mmap_min_addr is 0 and someone's mapped page 0).

The Intel manuals are very cagey about what happens with wraparound and
segmentation, and definitely seem to suggest that it's implementation
dependent:

"When the effective limit is FFFFFFFFH (4 GBytes), these accesses may or may not
cause the indicated exceptions.  Behavior is implementation-specific and may
vary from one execution to another."

So I'm a little worried that there might be more cases where this works
differently. Does anyone know for sure if it's worth risking it?

Regardless, I tried making the same changes to put_user, and those work
equally well and pass the same tests (including with the WARN() on 486).
Combined patch below.

Cheers,
-- David

---
From 3fd12432efee3bbed26abb347244c5378b7bf7e9 Mon Sep 17 00:00:00 2001
From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Thu, 1 Aug 2024 09:30:39 +0800
Subject: [PATCH] x86/uaccess: Use address masking for get_size on ia32

On x86_64 get_user and put_user rely on using address masking to force
any invalid addresses to the top of kernel address space, which is
unmapped, and then will trap. The 32-bit case has thus far just used a
comparison and a jump.

Use the address masking technique on ia32 as well (as the top page is
guaranteed to be unmapped here as well), to bring it into alignment with
the x86_64 implementation.

This also fixes the previous cleanup, which didn't zero the high bits if
a 64-bit get_user() was attempted with an invalid address, as in the
usercopy.usercopy_test_invalid KUnit test.

Fixes: 8a2462df1547 ("x86/uaccess: Improve the 8-byte getuser() case")
Co-developed-by: David Gow <davidgow@google.com>
Signed-off-by: David Gow <davidgow@google.com>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---
 arch/x86/lib/getuser.S | 5 ++---
 arch/x86/lib/putuser.S | 5 +++--
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/x86/lib/getuser.S b/arch/x86/lib/getuser.S
index a314622aa093..3ee80b9c4f78 100644
--- a/arch/x86/lib/getuser.S
+++ b/arch/x86/lib/getuser.S
@@ -44,9 +44,9 @@
 	or %rdx, %rax
 .else
 	cmp $TASK_SIZE_MAX-\size+1, %eax
-	jae .Lbad_get_user
 	sbb %edx, %edx		/* array_index_mask_nospec() */
-	and %edx, %eax
+	not %edx
+	or %edx, %eax
 .endif
 .endm
 
@@ -153,7 +153,6 @@ EXPORT_SYMBOL(__get_user_nocheck_8)
 
 SYM_CODE_START_LOCAL(__get_user_handle_exception)
 	ASM_CLAC
-.Lbad_get_user:
 	xor %edx,%edx
 	mov $(-EFAULT),%_ASM_AX
 	RET
diff --git a/arch/x86/lib/putuser.S b/arch/x86/lib/putuser.S
index 975c9c18263d..8896f6bcbf9c 100644
--- a/arch/x86/lib/putuser.S
+++ b/arch/x86/lib/putuser.S
@@ -39,7 +39,9 @@
 	or %rbx, %rcx
 .else
 	cmp $TASK_SIZE_MAX-\size+1, %ecx
-	jae .Lbad_put_user
+	sbb %ebx, %ebx
+	not %ebx
+	or %ebx, %ecx
 .endif
 .endm
 
@@ -128,7 +130,6 @@ EXPORT_SYMBOL(__put_user_nocheck_8)
 
 SYM_CODE_START_LOCAL(__put_user_handle_exception)
 	ASM_CLAC
-.Lbad_put_user:
 	movl $-EFAULT,%ecx
 	RET
 SYM_CODE_END(__put_user_handle_exception)
-- 
2.46.0.rc1.232.g9752f9e123-goog