All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/2] objtool uaccess fixes
@ 2019-04-24  7:19 Peter Zijlstra
  2019-04-24  7:19 ` [PATCH 1/2] x86/uaccess: Dont leak the AC flag into __put_user() argument evaluation Peter Zijlstra
  2019-04-24  7:19 ` [PATCH 2/2] mm/uaccess: Use unsigned long Peter Zijlstra
  0 siblings, 2 replies; 5+ messages in thread
From: Peter Zijlstra @ 2019-04-24  7:19 UTC (permalink / raw)
  To: mingo, tglx, x86; +Cc: linux-kernel, luto, peterz

Hi,

Here the two patches required to build a clean x86_64-allyesconfig with older GCC (<8).



^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH 1/2] x86/uaccess: Dont leak the AC flag into __put_user() argument evaluation
  2019-04-24  7:19 [PATCH 0/2] objtool uaccess fixes Peter Zijlstra
@ 2019-04-24  7:19 ` Peter Zijlstra
  2019-04-24 11:10   ` [tip:core/objtool] " tip-bot for Peter Zijlstra
  2019-04-24  7:19 ` [PATCH 2/2] mm/uaccess: Use unsigned long Peter Zijlstra
  1 sibling, 1 reply; 5+ messages in thread
From: Peter Zijlstra @ 2019-04-24  7:19 UTC (permalink / raw)
  To: mingo, tglx, x86; +Cc: linux-kernel, luto, peterz, Randy Dunlap, Linus Torvalds

The __put_user() macro evaluates it's @ptr argument inside the
__uaccess_begin() / __uaccess_end() region. While this would normally
not be expected to be an issue, an UBSAN bug (it ignored -fwrapv,
fixed in GCC 8+) would transform the @ptr evaluation for:

  drivers/gpu/drm/i915/i915_gem_execbuffer.c: if (unlikely(__put_user(offset, &urelocs[r-stack].presumed_offset))) {

into a signed-overflow-UB check and trigger the objtool AC validation.
Finish commit 2a418cf3f5f1 ("x86/uaccess: Don't leak the AC flag into
__put_user() value evaluation") and explicitly evaluate all 3 argument
early.

Reported-by: Randy Dunlap <rdunlap@infradead.org>
Acked-by: Randy Dunlap <rdunlap@infradead.org> # build-tested
Acked-by: Linus Torvalds <torvalds@linux-foundation.org>
Fixes: 2a418cf3f5f1 ("x86/uaccess: Don't leak the AC flag into __put_user() value evaluation")
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/include/asm/uaccess.h |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

--- a/arch/x86/include/asm/uaccess.h
+++ b/arch/x86/include/asm/uaccess.h
@@ -427,10 +427,11 @@ do {									\
 ({								\
 	__label__ __pu_label;					\
 	int __pu_err = -EFAULT;					\
-	__typeof__(*(ptr)) __pu_val;				\
-	__pu_val = x;						\
+	__typeof__(*(ptr)) __pu_val = (x);			\
+	__typeof__(ptr) __pu_ptr = (ptr);			\
+	__typeof__(size) __pu_size = (size);			\
 	__uaccess_begin();					\
-	__put_user_size(__pu_val, (ptr), (size), __pu_label);	\
+	__put_user_size(__pu_val, __pu_ptr, __pu_size, __pu_label);	\
 	__pu_err = 0;						\
 __pu_label:							\
 	__uaccess_end();					\



^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH 2/2] mm/uaccess: Use unsigned long
  2019-04-24  7:19 [PATCH 0/2] objtool uaccess fixes Peter Zijlstra
  2019-04-24  7:19 ` [PATCH 1/2] x86/uaccess: Dont leak the AC flag into __put_user() argument evaluation Peter Zijlstra
@ 2019-04-24  7:19 ` Peter Zijlstra
  2019-04-24 11:10   ` [tip:core/objtool] mm/uaccess: Use 'unsigned long' to placate UBSAN warnings on older GCC versions tip-bot for Peter Zijlstra
  1 sibling, 1 reply; 5+ messages in thread
From: Peter Zijlstra @ 2019-04-24  7:19 UTC (permalink / raw)
  To: mingo, tglx, x86; +Cc: linux-kernel, luto, peterz, Randy Dunlap, Linus Torvalds

Randy reported objtool triggered on his (GCC-7.4) build:

  lib/strncpy_from_user.o: warning: objtool: strncpy_from_user()+0x315: call to __ubsan_handle_add_overflow() with UACCESS enabled
  lib/strnlen_user.o: warning: objtool: strnlen_user()+0x337: call to __ubsan_handle_sub_overflow() with UACCESS enabled

This is due to UBSAN generating signed-overflow-UB warnings where it
should not. Prior to GCC-8 UBSAN ignored -fwrapv (which the kernel
uses through -fno-strict-overflow).

Make the functions use 'unsigned long' throughout.

Reported-by: Randy Dunlap <rdunlap@infradead.org>
Acked-by: Randy Dunlap <rdunlap@infradead.org> # build-tested
Acked-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 lib/strncpy_from_user.c |    5 +++--
 lib/strnlen_user.c      |    4 ++--
 2 files changed, 5 insertions(+), 4 deletions(-)

--- a/lib/strncpy_from_user.c
+++ b/lib/strncpy_from_user.c
@@ -23,10 +23,11 @@
  * hit it), 'max' is the address space maximum (and we return
  * -EFAULT if we hit it).
  */
-static inline long do_strncpy_from_user(char *dst, const char __user *src, long count, unsigned long max)
+static inline long do_strncpy_from_user(char *dst, const char __user *src,
+					unsigned long count, unsigned long max)
 {
 	const struct word_at_a_time constants = WORD_AT_A_TIME_CONSTANTS;
-	long res = 0;
+	unsigned long res = 0;
 
 	/*
 	 * Truncate 'max' to the user-specified limit, so that
--- a/lib/strnlen_user.c
+++ b/lib/strnlen_user.c
@@ -28,7 +28,7 @@
 static inline long do_strnlen_user(const char __user *src, unsigned long count, unsigned long max)
 {
 	const struct word_at_a_time constants = WORD_AT_A_TIME_CONSTANTS;
-	long align, res = 0;
+	unsigned long align, res = 0;
 	unsigned long c;
 
 	/*
@@ -42,7 +42,7 @@ static inline long do_strnlen_user(const
 	 * Do everything aligned. But that means that we
 	 * need to also expand the maximum..
 	 */
-	align = (sizeof(long) - 1) & (unsigned long)src;
+	align = (sizeof(unsigned long) - 1) & (unsigned long)src;
 	src -= align;
 	max += align;
 



^ permalink raw reply	[flat|nested] 5+ messages in thread

* [tip:core/objtool] x86/uaccess: Dont leak the AC flag into __put_user() argument evaluation
  2019-04-24  7:19 ` [PATCH 1/2] x86/uaccess: Dont leak the AC flag into __put_user() argument evaluation Peter Zijlstra
@ 2019-04-24 11:10   ` tip-bot for Peter Zijlstra
  0 siblings, 0 replies; 5+ messages in thread
From: tip-bot for Peter Zijlstra @ 2019-04-24 11:10 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: rdunlap, mingo, torvalds, linux-kernel, peterz, hpa, tglx

Commit-ID:  6ae865615fc43d014da2fd1f1bba7e81ee622d1b
Gitweb:     https://git.kernel.org/tip/6ae865615fc43d014da2fd1f1bba7e81ee622d1b
Author:     Peter Zijlstra <peterz@infradead.org>
AuthorDate: Wed, 24 Apr 2019 09:19:24 +0200
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Wed, 24 Apr 2019 12:19:45 +0200

x86/uaccess: Dont leak the AC flag into __put_user() argument evaluation

The __put_user() macro evaluates it's @ptr argument inside the
__uaccess_begin() / __uaccess_end() region. While this would normally
not be expected to be an issue, an UBSAN bug (it ignored -fwrapv,
fixed in GCC 8+) would transform the @ptr evaluation for:

  drivers/gpu/drm/i915/i915_gem_execbuffer.c: if (unlikely(__put_user(offset, &urelocs[r-stack].presumed_offset))) {

into a signed-overflow-UB check and trigger the objtool AC validation.

Finish this commit:

  2a418cf3f5f1 ("x86/uaccess: Don't leak the AC flag into __put_user() value evaluation")

and explicitly evaluate all 3 arguments early.

Reported-by: Randy Dunlap <rdunlap@infradead.org>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Acked-by: Randy Dunlap <rdunlap@infradead.org> # build-tested
Acked-by: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: luto@kernel.org
Fixes: 2a418cf3f5f1 ("x86/uaccess: Don't leak the AC flag into __put_user() value evaluation")
Link: http://lkml.kernel.org/r/20190424072208.695962771@infradead.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/include/asm/uaccess.h | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
index 5ca7b91faf67..bb21913885a3 100644
--- a/arch/x86/include/asm/uaccess.h
+++ b/arch/x86/include/asm/uaccess.h
@@ -427,10 +427,11 @@ do {									\
 ({								\
 	__label__ __pu_label;					\
 	int __pu_err = -EFAULT;					\
-	__typeof__(*(ptr)) __pu_val;				\
-	__pu_val = x;						\
+	__typeof__(*(ptr)) __pu_val = (x);			\
+	__typeof__(ptr) __pu_ptr = (ptr);			\
+	__typeof__(size) __pu_size = (size);			\
 	__uaccess_begin();					\
-	__put_user_size(__pu_val, (ptr), (size), __pu_label);	\
+	__put_user_size(__pu_val, __pu_ptr, __pu_size, __pu_label);	\
 	__pu_err = 0;						\
 __pu_label:							\
 	__uaccess_end();					\

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [tip:core/objtool] mm/uaccess: Use 'unsigned long' to placate UBSAN warnings on older GCC versions
  2019-04-24  7:19 ` [PATCH 2/2] mm/uaccess: Use unsigned long Peter Zijlstra
@ 2019-04-24 11:10   ` tip-bot for Peter Zijlstra
  0 siblings, 0 replies; 5+ messages in thread
From: tip-bot for Peter Zijlstra @ 2019-04-24 11:10 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: rdunlap, peterz, linux-kernel, torvalds, hpa, tglx, mingo

Commit-ID:  29da93fea3ea39ab9b12270cc6be1b70ef201c9e
Gitweb:     https://git.kernel.org/tip/29da93fea3ea39ab9b12270cc6be1b70ef201c9e
Author:     Peter Zijlstra <peterz@infradead.org>
AuthorDate: Wed, 24 Apr 2019 09:19:25 +0200
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Wed, 24 Apr 2019 12:19:45 +0200

mm/uaccess: Use 'unsigned long' to placate UBSAN warnings on older GCC versions

Randy reported objtool triggered on his (GCC-7.4) build:

  lib/strncpy_from_user.o: warning: objtool: strncpy_from_user()+0x315: call to __ubsan_handle_add_overflow() with UACCESS enabled
  lib/strnlen_user.o: warning: objtool: strnlen_user()+0x337: call to __ubsan_handle_sub_overflow() with UACCESS enabled

This is due to UBSAN generating signed-overflow-UB warnings where it
should not. Prior to GCC-8 UBSAN ignored -fwrapv (which the kernel
uses through -fno-strict-overflow).

Make the functions use 'unsigned long' throughout.

Reported-by: Randy Dunlap <rdunlap@infradead.org>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Acked-by: Randy Dunlap <rdunlap@infradead.org> # build-tested
Acked-by: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: luto@kernel.org
Link: http://lkml.kernel.org/r/20190424072208.754094071@infradead.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 lib/strncpy_from_user.c | 5 +++--
 lib/strnlen_user.c      | 4 ++--
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/lib/strncpy_from_user.c b/lib/strncpy_from_user.c
index 58eacd41526c..023ba9f3b99f 100644
--- a/lib/strncpy_from_user.c
+++ b/lib/strncpy_from_user.c
@@ -23,10 +23,11 @@
  * hit it), 'max' is the address space maximum (and we return
  * -EFAULT if we hit it).
  */
-static inline long do_strncpy_from_user(char *dst, const char __user *src, long count, unsigned long max)
+static inline long do_strncpy_from_user(char *dst, const char __user *src,
+					unsigned long count, unsigned long max)
 {
 	const struct word_at_a_time constants = WORD_AT_A_TIME_CONSTANTS;
-	long res = 0;
+	unsigned long res = 0;
 
 	/*
 	 * Truncate 'max' to the user-specified limit, so that
diff --git a/lib/strnlen_user.c b/lib/strnlen_user.c
index 1c1a1b0e38a5..7f2db3fe311f 100644
--- a/lib/strnlen_user.c
+++ b/lib/strnlen_user.c
@@ -28,7 +28,7 @@
 static inline long do_strnlen_user(const char __user *src, unsigned long count, unsigned long max)
 {
 	const struct word_at_a_time constants = WORD_AT_A_TIME_CONSTANTS;
-	long align, res = 0;
+	unsigned long align, res = 0;
 	unsigned long c;
 
 	/*
@@ -42,7 +42,7 @@ static inline long do_strnlen_user(const char __user *src, unsigned long count,
 	 * Do everything aligned. But that means that we
 	 * need to also expand the maximum..
 	 */
-	align = (sizeof(long) - 1) & (unsigned long)src;
+	align = (sizeof(unsigned long) - 1) & (unsigned long)src;
 	src -= align;
 	max += align;
 

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2019-04-24 11:11 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-24  7:19 [PATCH 0/2] objtool uaccess fixes Peter Zijlstra
2019-04-24  7:19 ` [PATCH 1/2] x86/uaccess: Dont leak the AC flag into __put_user() argument evaluation Peter Zijlstra
2019-04-24 11:10   ` [tip:core/objtool] " tip-bot for Peter Zijlstra
2019-04-24  7:19 ` [PATCH 2/2] mm/uaccess: Use unsigned long Peter Zijlstra
2019-04-24 11:10   ` [tip:core/objtool] mm/uaccess: Use 'unsigned long' to placate UBSAN warnings on older GCC versions tip-bot for Peter Zijlstra

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.