Re: [PATCH] Consolidate CONFIG_DEBUG_STRICT_USER_COPY_CHECKS

From: Helge Deller
Date: Wed Feb 27 2013 - 17:42:07 EST

Next message: Stephen Rothwell: "Re: [PATCH] Consolidate CONFIG_DEBUG_STRICT_USER_COPY_CHECK"
Previous message: H. Peter Anvin: "Re: [PATCH] mm/x86: Flush lazy MMU when DEBUG_PAGEALLOC is set"
In reply to: H. Peter Anvin: "Re: [PATCH] Consolidate CONFIG_DEBUG_STRICT_USER_COPY_CHECK"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 02/27/2013 04:00 AM, Stephen Boyd wrote:
> The help text for this config is duplicated across the x86,
> parisc, and s390 Kconfig.debug files. Arnd Bergman noted that the
> help text was slightly misleading and should be fixed to state
> that enabling this option isn't a problem when using pre 4.4 gcc.
>
> To simplify the rewording, consolidate the text into
> lib/Kconfig.debug and modify it there to be more explicit about
> when you should say N to this config.
>
> Also, make the text a bit more generic by stating that this
> option enables compile time checks so we can cover architectures
> which emit warnings vs. ones which emit errors. The details of
> how an architecture decided to implement the checks isn't as
> important as the concept of compile time checking of
> copy_from_user() calls.
>
> While we're doing this, remove all the copy_from_user_overflow()
> code that's duplicated many times and place it into lib/ so that
> any architecture supporting this option can get the function for
> free.
>
> Cc: Arnd Bergmann <arnd@xxxxxxxx>
> Cc: Ingo Molnar <mingo@xxxxxxx>
> Cc: H. Peter Anvin <hpa@xxxxxxxxx>
> Cc: linux-parisc@xxxxxxxxxxxxxxx
> Cc: linux-s390@xxxxxxxxxxxxxxx
> Cc: Arjan van de Ven <arjan@xxxxxxxxxxxxxxx>
> Cc: Helge Deller <deller@xxxxxx>
> Cc: Heiko Carstens <heiko.carstens@xxxxxxxxxx>
> Cc: Stephen Rothwell <sfr@xxxxxxxxxxxxxxxx>
> Cc: Chris Metcalf <cmetcalf@xxxxxxxxxx>
> Signed-off-by: Stephen Boyd <sboyd@xxxxxxxxxxxxxx>
> ---
>
> Per Helge Deller prodding me, I'm resending just this patch.

Yes, I found it in kernel patchwork and I think it's a good cleanup.
Tested OK on parisc:

Acked-by: Helge Deller <deller@xxxxxx>

> https://patchwork.kernel.org/patch/833182/
>
> arch/parisc/Kconfig | 1 +
> arch/parisc/Kconfig.debug | 14 --------------
> arch/s390/Kconfig | 1 +
> arch/s390/Kconfig.debug | 14 --------------
> arch/s390/lib/Makefile | 1 -
> arch/s390/lib/usercopy.c | 8 --------
> arch/sparc/lib/Makefile | 1 -
> arch/sparc/lib/usercopy.c | 9 ---------
> arch/tile/Kconfig | 8 +-------
> arch/tile/include/asm/uaccess.h | 7 ++++++-
> arch/tile/lib/uaccess.c | 8 --------
> arch/x86/Kconfig | 1 +
> arch/x86/Kconfig.debug | 14 --------------
> arch/x86/lib/usercopy_32.c | 6 ------
> lib/Kconfig.debug | 18 ++++++++++++++++++
> lib/Makefile | 1 +
> lib/usercopy.c | 9 +++++++++
> 17 files changed, 38 insertions(+), 83 deletions(-)
> delete mode 100644 arch/s390/lib/usercopy.c
> delete mode 100644 arch/sparc/lib/usercopy.c
> create mode 100644 lib/usercopy.c
>
> diff --git a/arch/parisc/Kconfig b/arch/parisc/Kconfig
> index a2a47d9..96be92f 100644
> --- a/arch/parisc/Kconfig
> +++ b/arch/parisc/Kconfig
> @@ -1,5 +1,6 @@
> config PARISC
> def_bool y
> + select ARCH_HAS_DEBUG_STRICT_USER_COPY_CHECKS
> select HAVE_IDE
> select HAVE_OPROFILE
> select HAVE_FUNCTION_TRACER if 64BIT
> diff --git a/arch/parisc/Kconfig.debug b/arch/parisc/Kconfig.debug
> index 7305ac8..bc989e5 100644
> --- a/arch/parisc/Kconfig.debug
> +++ b/arch/parisc/Kconfig.debug
> @@ -12,18 +12,4 @@ config DEBUG_RODATA
> portion of the kernel code won't be covered by a TLB anymore.
> If in doubt, say "N".
>
> -config DEBUG_STRICT_USER_COPY_CHECKS
> - bool "Strict copy size checks"
> - depends on DEBUG_KERNEL && !TRACE_BRANCH_PROFILING
> - ---help---
> - Enabling this option turns a certain set of sanity checks for user
> - copy operations into compile time failures.
> -
> - The copy_from_user() etc checks are there to help test if there
> - are sufficient security checks on the length argument of
> - the copy operation, by having gcc prove that the argument is
> - within bounds.
> -
> - If unsure, or if you run an older (pre 4.4) gcc, say N.
> -
> endmenu
> diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
> index 4b50537..516621f 100644
> --- a/arch/s390/Kconfig
> +++ b/arch/s390/Kconfig
> @@ -91,6 +91,7 @@ config S390
> select ARCH_INLINE_WRITE_UNLOCK_BH
> select ARCH_INLINE_WRITE_UNLOCK_IRQ
> select ARCH_INLINE_WRITE_UNLOCK_IRQRESTORE
> + select ARCH_HAS_DEBUG_STRICT_USER_COPY_CHECKS
> select ARCH_SAVE_PAGE_KEYS if HIBERNATION
> select ARCH_WANT_IPC_PARSE_VERSION
> select BUILDTIME_EXTABLE_SORT
> diff --git a/arch/s390/Kconfig.debug b/arch/s390/Kconfig.debug
> index fc32a2d..c56878e 100644
> --- a/arch/s390/Kconfig.debug
> +++ b/arch/s390/Kconfig.debug
> @@ -17,20 +17,6 @@ config STRICT_DEVMEM
>
> If you are unsure, say Y.
>
> -config DEBUG_STRICT_USER_COPY_CHECKS
> - def_bool n
> - prompt "Strict user copy size checks"
> - ---help---
> - Enabling this option turns a certain set of sanity checks for user
> - copy operations into compile time warnings.
> -
> - The copy_from_user() etc checks are there to help test if there
> - are sufficient security checks on the length argument of
> - the copy operation, by having gcc prove that the argument is
> - within bounds.
> -
> - If unsure, or if you run an older (pre 4.4) gcc, say N.
> -
> config S390_PTDUMP
> bool "Export kernel pagetable layout to userspace via debugfs"
> depends on DEBUG_KERNEL
> diff --git a/arch/s390/lib/Makefile b/arch/s390/lib/Makefile
> index 6ab0d0b..20b0e97 100644
> --- a/arch/s390/lib/Makefile
> +++ b/arch/s390/lib/Makefile
> @@ -3,7 +3,6 @@
> #
>
> lib-y += delay.o string.o uaccess_std.o uaccess_pt.o
> -obj-y += usercopy.o
> obj-$(CONFIG_32BIT) += div64.o qrnnd.o ucmpdi2.o mem32.o
> obj-$(CONFIG_64BIT) += mem64.o
> lib-$(CONFIG_64BIT) += uaccess_mvcos.o
> diff --git a/arch/s390/lib/usercopy.c b/arch/s390/lib/usercopy.c
> deleted file mode 100644
> index 14b363f..0000000
> --- a/arch/s390/lib/usercopy.c
> +++ /dev/null
> @@ -1,8 +0,0 @@
> -#include <linux/module.h>
> -#include <linux/bug.h>
> -
> -void copy_from_user_overflow(void)
> -{
> - WARN(1, "Buffer overflow detected!\n");
> -}
> -EXPORT_SYMBOL(copy_from_user_overflow);
> diff --git a/arch/sparc/lib/Makefile b/arch/sparc/lib/Makefile
> index 8410065f2..dbe119b 100644
> --- a/arch/sparc/lib/Makefile
> +++ b/arch/sparc/lib/Makefile
> @@ -45,4 +45,3 @@ obj-y += iomap.o
> obj-$(CONFIG_SPARC32) += atomic32.o ucmpdi2.o
> obj-y += ksyms.o
> obj-$(CONFIG_SPARC64) += PeeCeeI.o
> -obj-y += usercopy.o
> diff --git a/arch/sparc/lib/usercopy.c b/arch/sparc/lib/usercopy.c
> deleted file mode 100644
> index 5c4284c..0000000
> --- a/arch/sparc/lib/usercopy.c
> +++ /dev/null
> @@ -1,9 +0,0 @@
> -#include <linux/module.h>
> -#include <linux/kernel.h>
> -#include <linux/bug.h>
> -
> -void copy_from_user_overflow(void)
> -{
> - WARN(1, "Buffer overflow detected!\n");
> -}
> -EXPORT_SYMBOL(copy_from_user_overflow);
> diff --git a/arch/tile/Kconfig b/arch/tile/Kconfig
> index 1404497..9d65a48 100644
> --- a/arch/tile/Kconfig
> +++ b/arch/tile/Kconfig
> @@ -19,6 +19,7 @@ config TILE
> select HAVE_SYSCALL_WRAPPERS if TILEGX
> select HAVE_VIRT_TO_BUS
> select SYS_HYPERVISOR
> + select ARCH_HAS_DEBUG_STRICT_USER_COPY_CHECKS
> select ARCH_HAVE_NMI_SAFE_CMPXCHG
> select GENERIC_CLOCKEVENTS
> select MODULES_USE_ELF_RELA
> @@ -117,13 +118,6 @@ config STRICT_DEVMEM
> config SMP
> def_bool y
>
> -# Allow checking for compile-time determined overflow errors in
> -# copy_from_user(). There are still unprovable places in the
> -# generic code as of 2.6.34, so this option is not really compatible
> -# with -Werror, which is more useful in general.
> -config DEBUG_COPY_FROM_USER
> - def_bool n
> -
> config HVC_TILE
> depends on TTY
> select HVC_DRIVER
> diff --git a/arch/tile/include/asm/uaccess.h b/arch/tile/include/asm/uaccess.h
> index 9ab078a..8a082bc 100644
> --- a/arch/tile/include/asm/uaccess.h
> +++ b/arch/tile/include/asm/uaccess.h
> @@ -395,7 +395,12 @@ _copy_from_user(void *to, const void __user *from, unsigned long n)
> return n;
> }
>
> -#ifdef CONFIG_DEBUG_COPY_FROM_USER
> +#ifdef CONFIG_DEBUG_STRICT_USER_COPY_CHECKS
> +/*
> + * There are still unprovable places in the generic code as of 2.6.34, so this
> + * option is not really compatible with -Werror, which is more useful in
> + * general.
> + */
> extern void copy_from_user_overflow(void)
> __compiletime_warning("copy_from_user() size is not provably correct");
>
> diff --git a/arch/tile/lib/uaccess.c b/arch/tile/lib/uaccess.c
> index f8d398c..030abe3 100644
> --- a/arch/tile/lib/uaccess.c
> +++ b/arch/tile/lib/uaccess.c
> @@ -22,11 +22,3 @@ int __range_ok(unsigned long addr, unsigned long size)
> is_arch_mappable_range(addr, size));
> }
> EXPORT_SYMBOL(__range_ok);
> -
> -#ifdef CONFIG_DEBUG_COPY_FROM_USER
> -void copy_from_user_overflow(void)
> -{
> - WARN(1, "Buffer overflow detected!\n");
> -}
> -EXPORT_SYMBOL(copy_from_user_overflow);
> -#endif
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index a4f24f5..f045e0a 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -20,6 +20,7 @@ config X86_64
> ### Arch settings
> config X86
> def_bool y
> + select ARCH_HAS_DEBUG_STRICT_USER_COPY_CHECKS
> select HAVE_AOUT if X86_32
> select HAVE_UNSTABLE_SCHED_CLOCK
> select ARCH_SUPPORTS_NUMA_BALANCING
> diff --git a/arch/x86/Kconfig.debug b/arch/x86/Kconfig.debug
> index b322f12..dea0da5 100644
> --- a/arch/x86/Kconfig.debug
> +++ b/arch/x86/Kconfig.debug
> @@ -292,20 +292,6 @@ config OPTIMIZE_INLINING
>
> If unsure, say N.
>
> -config DEBUG_STRICT_USER_COPY_CHECKS
> - bool "Strict copy size checks"
> - depends on DEBUG_KERNEL && !TRACE_BRANCH_PROFILING
> - ---help---
> - Enabling this option turns a certain set of sanity checks for user
> - copy operations into compile time failures.
> -
> - The copy_from_user() etc checks are there to help test if there
> - are sufficient security checks on the length argument of
> - the copy operation, by having gcc prove that the argument is
> - within bounds.
> -
> - If unsure, or if you run an older (pre 4.4) gcc, say N.
> -
> config DEBUG_NMI_SELFTEST
> bool "NMI Selftest"
> depends on DEBUG_KERNEL && X86_LOCAL_APIC
> diff --git a/arch/x86/lib/usercopy_32.c b/arch/x86/lib/usercopy_32.c
> index f0312d7..3eb18ac 100644
> --- a/arch/x86/lib/usercopy_32.c
> +++ b/arch/x86/lib/usercopy_32.c
> @@ -689,9 +689,3 @@ _copy_from_user(void *to, const void __user *from, unsigned long n)
> return n;
> }
> EXPORT_SYMBOL(_copy_from_user);
> -
> -void copy_from_user_overflow(void)
> -{
> - WARN(1, "Buffer overflow detected!\n");
> -}
> -EXPORT_SYMBOL(copy_from_user_overflow);
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index 28be08c..ae80518 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -1292,6 +1292,24 @@ config LATENCYTOP
> Enable this option if you want to use the LatencyTOP tool
> to find out which userspace is blocking on what kernel operations.
>
> +config ARCH_HAS_DEBUG_STRICT_USER_COPY_CHECKS
> + bool
> +
> +config DEBUG_STRICT_USER_COPY_CHECKS
> + bool "Strict user copy size checks"
> + depends on ARCH_HAS_DEBUG_STRICT_USER_COPY_CHECKS
> + depends on DEBUG_KERNEL && !TRACE_BRANCH_PROFILING
> + help
> + Enabling this option turns a certain set of sanity checks for user
> + copy operations into compile time failures.
> +
> + The copy_from_user() etc checks are there to help test if there
> + are sufficient security checks on the length argument of
> + the copy operation, by having gcc prove that the argument is
> + within bounds.
> +
> + If unsure, say N.
> +
> source mm/Kconfig.debug
> source kernel/trace/Kconfig
>
> diff --git a/lib/Makefile b/lib/Makefile
> index 32f4455..59fabd0 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -15,6 +15,7 @@ lib-y := ctype.o string.o vsprintf.o cmdline.o \
> is_single_threaded.o plist.o decompress.o kobject_uevent.o \
> earlycpio.o percpu-refcount.o
>
> +lib-$(CONFIG_ARCH_HAS_DEBUG_STRICT_USER_COPY_CHECKS) += usercopy.o
> lib-$(CONFIG_MMU) += ioremap.o
> lib-$(CONFIG_SMP) += cpumask.o
>
> diff --git a/lib/usercopy.c b/lib/usercopy.c
> new file mode 100644
> index 0000000..4f5b1dd
> --- /dev/null
> +++ b/lib/usercopy.c
> @@ -0,0 +1,9 @@
> +#include <linux/export.h>
> +#include <linux/bug.h>
> +#include <linux/uaccess.h>
> +
> +void copy_from_user_overflow(void)
> +{
> + WARN(1, "Buffer overflow detected!\n");
> +}
> +EXPORT_SYMBOL(copy_from_user_overflow);
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Stephen Rothwell: "Re: [PATCH] Consolidate CONFIG_DEBUG_STRICT_USER_COPY_CHECK"
Previous message: H. Peter Anvin: "Re: [PATCH] mm/x86: Flush lazy MMU when DEBUG_PAGEALLOC is set"
In reply to: H. Peter Anvin: "Re: [PATCH] Consolidate CONFIG_DEBUG_STRICT_USER_COPY_CHECK"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]