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

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

[David Gow]

While zeroing the upper 32 bits of an 8-byte getuser on 32-bit x86 was
fixed by commit 8c860ed825cb ("x86/uaccess: Fix missed zeroing of ia32 u64 get_user() range checking")
it was broken again in commit 8a2462df1547 ("x86/uaccess: Improve the 8-byte getuser() case").

This is because the register which holds the upper 32 bits (%ecx) is
being cleared _after_ the check_range, so if the range check fails, %ecx
is never cleared.

This can be reproduced with:
./tools/testing/kunit/kunit.py run --arch i386 usercopy

Instead, clear %ecx _before_ check_range in the 8-byte case. This
reintroduces a bit of the ugliness we were trying to avoid by adding
another #ifndef CONFIG_X86_64, but at least keeps check_range from
needing a separate bad_get_user_8 jump.

Fixes: 8a2462df1547 ("x86/uaccess: Improve the 8-byte getuser() case")
Signed-off-by: David Gow <davidgow@google.com>
---

There are a few other ways we could fix this, but all of them seem to
increase the ugliness a bit. This seemed the best compromise, but if
you'd prefer to have the size=8 special case live in check_range, that's
fine, too.

See also:
https://lore.kernel.org/lkml/CAHk-=whYb2L_atsRk9pBiFiVLGe5wNZLHhRinA69yu6FiKvDsw@mail.gmail.com/

Cheers,
-- David

---
 arch/x86/lib/getuser.S | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/x86/lib/getuser.S b/arch/x86/lib/getuser.S
index a314622aa093..d066aecf8aeb 100644
--- a/arch/x86/lib/getuser.S
+++ b/arch/x86/lib/getuser.S
@@ -88,12 +88,14 @@ SYM_FUNC_END(__get_user_4)
 EXPORT_SYMBOL(__get_user_4)
 
 SYM_FUNC_START(__get_user_8)
+#ifndef CONFIG_X86_64
+	xor %ecx,%ecx
+#endif
 	check_range size=8
 	ASM_STAC
 #ifdef CONFIG_X86_64
 	UACCESS movq (%_ASM_AX),%rdx
 #else
-	xor %ecx,%ecx
 	UACCESS movl (%_ASM_AX),%edx
 	UACCESS movl 4(%_ASM_AX),%ecx
 #endif
-- 
2.46.0.rc1.232.g9752f9e123-goog

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

[Linus Torvalds]

On Wed, 31 Jul 2024 at 00:31, David Gow <davidgow@google.com> wrote:
>
> Fixes: 8a2462df1547 ("x86/uaccess: Improve the 8-byte getuser() case")
> Signed-off-by: David Gow <davidgow@google.com>

Ack.

I'd have liked to minimize the #ifdef's in code, but yeah, this fix is
obviously needed.

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.

               Linus

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

[Linus Torvalds]

[-- Attachment #1: Type: text/plain, Size: 947 bytes --]

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*.

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

[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 636 bytes --]

 arch/x86/lib/getuser.S | 5 ++---
 1 file changed, 2 insertions(+), 3 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

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

[David Gow]

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

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

[Linus Torvalds]

On Wed, 31 Jul 2024 at 23:35, David Gow <davidgow@google.com> wrote:
>
> 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.

Ahh, yes. Old i386 machines (that we no longer support) did the same.
You hit the segment limit, not the page fault.

And we had something very similar when we did the whole 64-bit address
range checking relaxation (to avoid all the crazy LAM noise in the
access_ok() code).

See commit 6014bc27561f ("x86-64: make access_ok() independent of
LAM") and the extable.c parts in particular

That wasn't because of segment limits, but the whole "non-canonical
address range" ends up having a very similar situation, and also
causes #GP before the page fault.

So yeah, the same way x86-64 no longer warns for #GP in the user
address range and the point where the sign wraps, the 32-bit warning
for this #GP would have to be suppressed.

In fact, it's the exact same gp_fault_address_ok() logic, it would
just get a 32-bit case for "#GP at the end of the address range is
ok".

> 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?

I don't think the address wrap-around is a risk per se.

As far as I know the "undefined behavior" is not some kind of subtle
thing where things have gone wrong in the past - it's just that later
microarchitectures just ended up special-casing the flat segment setup
and no longer do any segment limit checks (or base additions) at all.

But I'm also not going to argue that this is really worth pursuing on
32-bit if anybody is in the least worried.

It's on life support, not like it's actively maintained. Your original
patch may not be exactly "pretty", but it clearly fixes the problem
without playing any games.

          Linus

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

[Thomas Gleixner]

On Thu, Aug 01 2024 at 09:24, Linus Torvalds wrote:
> On Wed, 31 Jul 2024 at 23:35, David Gow <davidgow@google.com> wrote:
> But I'm also not going to argue that this is really worth pursuing on
> 32-bit if anybody is in the least worried.
>
> It's on life support, not like it's actively maintained. Your original
> patch may not be exactly "pretty", but it clearly fixes the problem
> without playing any games.

I queue that oe up and if someone really cares, then it can be updated,
but I doubt it's worth the trouble.

Thanks,

        tglx

[tip: x86/urgent] x86/uaccess: Zero the 8-byte get_range case on failure on 32-bit

[tip-bot2 for David Gow]

The following commit has been merged into the x86/urgent branch of tip:

Commit-ID:     dd35a0933269c636635b6af89dc6fa1782791e56
Gitweb:        https://git.kernel.org/tip/dd35a0933269c636635b6af89dc6fa1782791e56
Author:        David Gow <davidgow@google.com>
AuthorDate:    Wed, 31 Jul 2024 15:30:29 +08:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Thu, 01 Aug 2024 21:19:10 +02:00

x86/uaccess: Zero the 8-byte get_range case on failure on 32-bit

While zeroing the upper 32 bits of an 8-byte getuser on 32-bit x86 was
fixed by commit 8c860ed825cb ("x86/uaccess: Fix missed zeroing of ia32 u64
get_user() range checking") it was broken again in commit 8a2462df1547
("x86/uaccess: Improve the 8-byte getuser() case").

This is because the register which holds the upper 32 bits (%ecx) is being
cleared _after_ the check_range, so if the range check fails, %ecx is never
cleared.

This can be reproduced with:
./tools/testing/kunit/kunit.py run --arch i386 usercopy

Instead, clear %ecx _before_ check_range in the 8-byte case. This
reintroduces a bit of the ugliness we were trying to avoid by adding
another #ifndef CONFIG_X86_64, but at least keeps check_range from needing
a separate bad_get_user_8 jump.

Fixes: 8a2462df1547 ("x86/uaccess: Improve the 8-byte getuser() case")
Signed-off-by: David Gow <davidgow@google.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Linus Torvalds <torvalds@linux-foundation.org>
Link: https://lore.kernel.org/all/20240731073031.4045579-1-davidgow@google.com
---
 arch/x86/lib/getuser.S | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/x86/lib/getuser.S b/arch/x86/lib/getuser.S
index a314622..d066aec 100644
--- a/arch/x86/lib/getuser.S
+++ b/arch/x86/lib/getuser.S
@@ -88,12 +88,14 @@ SYM_FUNC_END(__get_user_4)
 EXPORT_SYMBOL(__get_user_4)
 
 SYM_FUNC_START(__get_user_8)
+#ifndef CONFIG_X86_64
+	xor %ecx,%ecx
+#endif
 	check_range size=8
 	ASM_STAC
 #ifdef CONFIG_X86_64
 	UACCESS movq (%_ASM_AX),%rdx
 #else
-	xor %ecx,%ecx
 	UACCESS movl (%_ASM_AX),%edx
 	UACCESS movl 4(%_ASM_AX),%ecx
 #endif