linux-kernel - [PATCH] x86/uaccess: Zero the 8-byte 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-next>] [day] [month] [year] [list]

Message-ID: <20240731073031.4045579-1-davidgow@google.com>
Date: Wed, 31 Jul 2024 15:30:29 +0800
From: David Gow <davidgow@...gle.com>
To: Kees Cook <kees@...nel.org>, Linus Torvalds <torvalds@...ux-foundation.org>, 
	Borislav Petkov <bp@...en8.de>
Cc: David Gow <davidgow@...gle.com>, 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: [PATCH] x86/uaccess: Zero the 8-byte get_range case on failure

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@...gle.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