[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240801063442.553090-2-davidgow@google.com>
Date: Thu, 1 Aug 2024 14:34:35 +0800
From: David Gow <davidgow@...gle.com>
To: Linus Torvalds <torvalds@...ux-foundation.org>
Cc: David Gow <davidgow@...gle.com>, Kees Cook <kees@...nel.org>, Borislav Petkov <bp@...en8.de>,
Thomas Gleixner <tglx@...utronix.de>, Ingo Molnar <mingo@...hat.com>,
Dave Hansen <dave.hansen@...ux.intel.com>, Peter Zijlstra <peterz@...radead.org>,
Andrew Morton <akpm@...ux-foundation.org>, "H . Peter Anvin" <hpa@...or.com>, x86@...nel.org,
kunit-dev@...glegroups.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] x86/uaccess: Zero the 8-byte get_range case on failure
On Wed, 31 Jul 2024 09:38:15 -0700 Linus Torvalds <torvalds@...ux-foundation.org> wrote:
> On Wed, 31 Jul 2024 at 09:24, Linus Torvalds
> <torvalds@...ux-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@...ux-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@...gle.com>
Signed-off-by: David Gow <davidgow@...gle.com>
Signed-off-by: Linus Torvalds <torvalds@...ux-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
Powered by blists - more mailing lists