Skip to content

Commit 0d025d2

Browse files
jpoimboetorvalds
authored andcommitted
mm/usercopy: get rid of CONFIG_DEBUG_STRICT_USER_COPY_CHECKS
There are three usercopy warnings which are currently being silenced for gcc 4.6 and newer: 1) "copy_from_user() buffer size is too small" compile warning/error This is a static warning which happens when object size and copy size are both const, and copy size > object size. I didn't see any false positives for this one. So the function warning attribute seems to be working fine here. Note this scenario is always a bug and so I think it should be changed to *always* be an error, regardless of CONFIG_DEBUG_STRICT_USER_COPY_CHECKS. 2) "copy_from_user() buffer size is not provably correct" compile warning This is another static warning which happens when I enable __compiletime_object_size() for new compilers (and CONFIG_DEBUG_STRICT_USER_COPY_CHECKS). It happens when object size is const, but copy size is *not*. In this case there's no way to compare the two at build time, so it gives the warning. (Note the warning is a byproduct of the fact that gcc has no way of knowing whether the overflow function will be called, so the call isn't dead code and the warning attribute is activated.) So this warning seems to only indicate "this is an unusual pattern, maybe you should check it out" rather than "this is a bug". I get 102(!) of these warnings with allyesconfig and the __compiletime_object_size() gcc check removed. I don't know if there are any real bugs hiding in there, but from looking at a small sample, I didn't see any. According to Kees, it does sometimes find real bugs. But the false positive rate seems high. 3) "Buffer overflow detected" runtime warning This is a runtime warning where object size is const, and copy size > object size. All three warnings (both static and runtime) were completely disabled for gcc 4.6 with the following commit: 2fb0815 ("gcc4: disable __compiletime_object_size for GCC 4.6+") That commit mistakenly assumed that the false positives were caused by a gcc bug in __compiletime_object_size(). But in fact, __compiletime_object_size() seems to be working fine. The false positives were instead triggered by #2 above. (Though I don't have an explanation for why the warnings supposedly only started showing up in gcc 4.6.) So remove warning #2 to get rid of all the false positives, and re-enable warnings #1 and #3 by reverting the above commit. Furthermore, since #1 is a real bug which is detected at compile time, upgrade it to always be an error. Having done all that, CONFIG_DEBUG_STRICT_USER_COPY_CHECKS is no longer needed. Signed-off-by: Josh Poimboeuf <[email protected]> Cc: Kees Cook <[email protected]> Cc: Thomas Gleixner <[email protected]> Cc: Ingo Molnar <[email protected]> Cc: "H . Peter Anvin" <[email protected]> Cc: Andy Lutomirski <[email protected]> Cc: Steven Rostedt <[email protected]> Cc: Brian Gerst <[email protected]> Cc: Peter Zijlstra <[email protected]> Cc: Frederic Weisbecker <[email protected]> Cc: Byungchul Park <[email protected]> Cc: Nilay Vaish <[email protected]> Signed-off-by: Linus Torvalds <[email protected]>
1 parent d8dc020 commit 0d025d2

File tree

19 files changed

+45
-128
lines changed

19 files changed

+45
-128
lines changed

arch/parisc/Kconfig

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,5 @@
11
config PARISC
22
def_bool y
3-
select ARCH_HAS_DEBUG_STRICT_USER_COPY_CHECKS
43
select ARCH_MIGHT_HAVE_PC_PARPORT
54
select HAVE_IDE
65
select HAVE_OPROFILE

arch/parisc/configs/c8000_defconfig

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -245,7 +245,6 @@ CONFIG_DEBUG_RT_MUTEXES=y
245245
CONFIG_PROVE_RCU_DELAY=y
246246
CONFIG_DEBUG_BLOCK_EXT_DEVT=y
247247
CONFIG_LATENCYTOP=y
248-
CONFIG_DEBUG_STRICT_USER_COPY_CHECKS=y
249248
CONFIG_KEYS=y
250249
# CONFIG_CRYPTO_HW is not set
251250
CONFIG_FONTS=y

arch/parisc/configs/generic-64bit_defconfig

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -291,7 +291,6 @@ CONFIG_BOOTPARAM_SOFTLOCKUP_PANIC=y
291291
CONFIG_BOOTPARAM_HUNG_TASK_PANIC=y
292292
# CONFIG_SCHED_DEBUG is not set
293293
CONFIG_TIMER_STATS=y
294-
CONFIG_DEBUG_STRICT_USER_COPY_CHECKS=y
295294
CONFIG_CRYPTO_MANAGER=y
296295
CONFIG_CRYPTO_ECB=m
297296
CONFIG_CRYPTO_PCBC=m

arch/parisc/include/asm/uaccess.h

Lines changed: 12 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -208,13 +208,13 @@ unsigned long copy_in_user(void __user *dst, const void __user *src, unsigned lo
208208
#define __copy_to_user_inatomic __copy_to_user
209209
#define __copy_from_user_inatomic __copy_from_user
210210

211-
extern void copy_from_user_overflow(void)
212-
#ifdef CONFIG_DEBUG_STRICT_USER_COPY_CHECKS
213-
__compiletime_error("copy_from_user() buffer size is not provably correct")
214-
#else
215-
__compiletime_warning("copy_from_user() buffer size is not provably correct")
216-
#endif
217-
;
211+
extern void __compiletime_error("usercopy buffer size is too small")
212+
__bad_copy_user(void);
213+
214+
static inline void copy_user_overflow(int size, unsigned long count)
215+
{
216+
WARN(1, "Buffer overflow detected (%d < %lu)!\n", size, count);
217+
}
218218

219219
static inline unsigned long __must_check copy_from_user(void *to,
220220
const void __user *from,
@@ -223,10 +223,12 @@ static inline unsigned long __must_check copy_from_user(void *to,
223223
int sz = __compiletime_object_size(to);
224224
int ret = -EFAULT;
225225

226-
if (likely(sz == -1 || !__builtin_constant_p(n) || sz >= n))
226+
if (likely(sz == -1 || sz >= n))
227227
ret = __copy_from_user(to, from, n);
228-
else
229-
copy_from_user_overflow();
228+
else if (!__builtin_constant_p(n))
229+
copy_user_overflow(sz, n);
230+
else
231+
__bad_copy_user();
230232

231233
return ret;
232234
}

arch/s390/Kconfig

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,6 @@ config DEBUG_RODATA
6868
config S390
6969
def_bool y
7070
select ARCH_HAS_ATOMIC64_DEC_IF_POSITIVE
71-
select ARCH_HAS_DEBUG_STRICT_USER_COPY_CHECKS
7271
select ARCH_HAS_DEVMEM_IS_ALLOWED
7372
select ARCH_HAS_ELF_RANDOMIZE
7473
select ARCH_HAS_GCOV_PROFILE_ALL

arch/s390/configs/default_defconfig

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -602,7 +602,6 @@ CONFIG_FAIL_FUTEX=y
602602
CONFIG_FAULT_INJECTION_DEBUG_FS=y
603603
CONFIG_FAULT_INJECTION_STACKTRACE_FILTER=y
604604
CONFIG_LATENCYTOP=y
605-
CONFIG_DEBUG_STRICT_USER_COPY_CHECKS=y
606605
CONFIG_IRQSOFF_TRACER=y
607606
CONFIG_PREEMPT_TRACER=y
608607
CONFIG_SCHED_TRACER=y

arch/s390/configs/gcov_defconfig

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -552,7 +552,6 @@ CONFIG_NOTIFIER_ERROR_INJECTION=m
552552
CONFIG_CPU_NOTIFIER_ERROR_INJECT=m
553553
CONFIG_PM_NOTIFIER_ERROR_INJECT=m
554554
CONFIG_LATENCYTOP=y
555-
CONFIG_DEBUG_STRICT_USER_COPY_CHECKS=y
556555
CONFIG_BLK_DEV_IO_TRACE=y
557556
# CONFIG_KPROBE_EVENT is not set
558557
CONFIG_TRACE_ENUM_MAP_FILE=y

arch/s390/configs/performance_defconfig

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -549,7 +549,6 @@ CONFIG_TIMER_STATS=y
549549
CONFIG_RCU_TORTURE_TEST=m
550550
CONFIG_RCU_CPU_STALL_TIMEOUT=60
551551
CONFIG_LATENCYTOP=y
552-
CONFIG_DEBUG_STRICT_USER_COPY_CHECKS=y
553552
CONFIG_SCHED_TRACER=y
554553
CONFIG_FTRACE_SYSCALLS=y
555554
CONFIG_STACK_TRACER=y

arch/s390/defconfig

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -172,7 +172,6 @@ CONFIG_DEBUG_NOTIFIERS=y
172172
CONFIG_RCU_CPU_STALL_TIMEOUT=60
173173
CONFIG_RCU_TRACE=y
174174
CONFIG_LATENCYTOP=y
175-
CONFIG_DEBUG_STRICT_USER_COPY_CHECKS=y
176175
CONFIG_SCHED_TRACER=y
177176
CONFIG_FTRACE_SYSCALLS=y
178177
CONFIG_TRACER_SNAPSHOT_PER_CPU_SWAP=y

arch/s390/include/asm/uaccess.h

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -311,6 +311,14 @@ int __get_user_bad(void) __attribute__((noreturn));
311311
#define __put_user_unaligned __put_user
312312
#define __get_user_unaligned __get_user
313313

314+
extern void __compiletime_error("usercopy buffer size is too small")
315+
__bad_copy_user(void);
316+
317+
static inline void copy_user_overflow(int size, unsigned long count)
318+
{
319+
WARN(1, "Buffer overflow detected (%d < %lu)!\n", size, count);
320+
}
321+
314322
/**
315323
* copy_to_user: - Copy a block of data into user space.
316324
* @to: Destination address, in user space.
@@ -332,12 +340,6 @@ copy_to_user(void __user *to, const void *from, unsigned long n)
332340
return __copy_to_user(to, from, n);
333341
}
334342

335-
void copy_from_user_overflow(void)
336-
#ifdef CONFIG_DEBUG_STRICT_USER_COPY_CHECKS
337-
__compiletime_warning("copy_from_user() buffer size is not provably correct")
338-
#endif
339-
;
340-
341343
/**
342344
* copy_from_user: - Copy a block of data from user space.
343345
* @to: Destination address, in kernel space.
@@ -362,7 +364,10 @@ copy_from_user(void *to, const void __user *from, unsigned long n)
362364

363365
might_fault();
364366
if (unlikely(sz != -1 && sz < n)) {
365-
copy_from_user_overflow();
367+
if (!__builtin_constant_p(n))
368+
copy_user_overflow(sz, n);
369+
else
370+
__bad_copy_user();
366371
return n;
367372
}
368373
return __copy_from_user(to, from, n);

0 commit comments

Comments
 (0)