| Message ID | 1361934016-22630-1-git-send-email-sboyd@codeaurora.org (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
On Wednesday 27 February 2013, Stephen Boyd wrote: > 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. > + Is there actually any architecture dependency left after this? I wonder if we actually need the ARCH_HAS_DEBUG_STRICT_USER_COPY_CHECKS symbol, or could just show the DEBUG_STRICT_USER_COPY_CHECKS option on all architectures. It's fine to do your patch as a first step though, which would not change the behavior. > 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 > I think this should instead be +lib-$(DEBUG_STRICT_USER_COPY_CHECKS) += usercopy.o No point building that file if we are not using it. Other than that, Acked-by: Arnd Bergmann <arnd@arndb.de> -- To unsubscribe from this list: send the line "unsubscribe linux-parisc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/27/13 12:32, Arnd Bergmann wrote: > On Wednesday 27 February 2013, Stephen Boyd wrote: >> 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. >> + > Is there actually any architecture dependency left after this? > I wonder if we actually need the ARCH_HAS_DEBUG_STRICT_USER_COPY_CHECKS > symbol, or could just show the DEBUG_STRICT_USER_COPY_CHECKS option > on all architectures. > > It's fine to do your patch as a first step though, which would not > change the behavior. A lot of arches seem to not want to enable it because false positives are everywhere. It really depends on how good the compiler is at doing constant propagation and dead code removal. > >> 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 >> > I think this should instead be > > +lib-$(DEBUG_STRICT_USER_COPY_CHECKS) += usercopy.o > > No point building that file if we are not using it. We still need it to link the kernel because the callers of the function don't have ifdefs. Also, all arches were doing an obj-y before, so this is equivalent.
On Wednesday 27 February 2013, Stephen Boyd wrote: > On 02/27/13 12:32, Arnd Bergmann wrote: > > On Wednesday 27 February 2013, Stephen Boyd wrote: > >> 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. > >> + > > Is there actually any architecture dependency left after this? > > I wonder if we actually need the ARCH_HAS_DEBUG_STRICT_USER_COPY_CHECKS > > symbol, or could just show the DEBUG_STRICT_USER_COPY_CHECKS option > > on all architectures. > > > > It's fine to do your patch as a first step though, which would not > > change the behavior. > > A lot of arches seem to not want to enable it because false positives > are everywhere. It really depends on how good the compiler is at doing > constant propagation and dead code removal. Ok, I see. Of course they would not need to enable that option, but I guess if we know that enabling it doesn't work, there is no point in providing the option. > >> > >> +lib-$(CONFIG_ARCH_HAS_DEBUG_STRICT_USER_COPY_CHECKS) += usercopy.o > >> lib-$(CONFIG_MMU) += ioremap.o > >> lib-$(CONFIG_SMP) += cpumask.o > >> > > I think this should instead be > > > > +lib-$(DEBUG_STRICT_USER_COPY_CHECKS) += usercopy.o > > > > No point building that file if we are not using it. > > We still need it to link the kernel because the callers of the function > don't have ifdefs. Also, all arches were doing an obj-y before, so this > is equivalent. Ok. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-parisc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/27/2013 12:42 PM, Stephen Boyd wrote: >> >> It's fine to do your patch as a first step though, which would not >> change the behavior. > > A lot of arches seem to not want to enable it because false positives > are everywhere. It really depends on how good the compiler is at doing > constant propagation and dead code removal. > Although some of the cases I have seen being flagged as "false positives" have been real bugs. -hpa -- To unsubscribe from this list: send the line "unsubscribe linux-parisc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@arndb.de> > Cc: Ingo Molnar <mingo@elte.hu> > Cc: H. Peter Anvin <hpa@zytor.com> > Cc: linux-parisc@vger.kernel.org > Cc: linux-s390@vger.kernel.org > Cc: Arjan van de Ven <arjan@linux.intel.com> > Cc: Helge Deller <deller@gmx.de> > Cc: Heiko Carstens <heiko.carstens@de.ibm.com> > Cc: Stephen Rothwell <sfr@canb.auug.org.au> > Cc: Chris Metcalf <cmetcalf@tilera.com> > Signed-off-by: Stephen Boyd <sboyd@codeaurora.org> > --- > > 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@gmx.de> > 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-parisc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi all, On Wed, 27 Feb 2013 14:19:16 -0800 "H. Peter Anvin" <hpa@zytor.com> wrote: > > Although some of the cases I have seen being flagged as "false > positives" have been real bugs. [hijacking the thread :-)] I have been getting this warning for a very long time ( which would be an error if CONFIG_DEBUG_STRICT_USER_COPY_CHECK was set): i386 defconfig i386-linux-gcc (GCC) 4.6.3 In file included from arch/x86/include/asm/uaccess.h:537:0, from include/linux/uaccess.h:5, from include/linux/highmem.h:8, from include/linux/pagemap.h:10, from fs/binfmt_misc.c:27: arch/x86/include/asm/uaccess_32.h: In function 'parse_command.part.2': arch/x86/include/asm/uaccess_32.h:211:26: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct [enabled by default]
On 02/27/13 14:19, H. Peter Anvin wrote: > On 02/27/2013 12:42 PM, Stephen Boyd wrote: >>> It's fine to do your patch as a first step though, which would not >>> change the behavior. >> A lot of arches seem to not want to enable it because false positives >> are everywhere. It really depends on how good the compiler is at doing >> constant propagation and dead code removal. >> > Although some of the cases I have seen being flagged as "false > positives" have been real bugs. There were so many false-positives on x86_64 that Andrew eventually dropped my patch to add support for this option to the copy_from_user() function there.
On 02/27/2013 02:45 PM, Stephen Rothwell wrote: > Hi all, > > On Wed, 27 Feb 2013 14:19:16 -0800 "H. Peter Anvin" <hpa@zytor.com> > wrote: >> >> Although some of the cases I have seen being flagged as "false >> positives" have been real bugs. > > [hijacking the thread :-)] > > I have been getting this warning for a very long time ( which would > be an error if CONFIG_DEBUG_STRICT_USER_COPY_CHECK was set): > > i386 defconfig i386-linux-gcc (GCC) 4.6.3 > > In file included from arch/x86/include/asm/uaccess.h:537:0, from > include/linux/uaccess.h:5, from include/linux/highmem.h:8, from > include/linux/pagemap.h:10, from fs/binfmt_misc.c:27: > arch/x86/include/asm/uaccess_32.h: In function > 'parse_command.part.2': arch/x86/include/asm/uaccess_32.h:211:26: > warning: call to 'copy_from_user_overflow' declared with attribute > warning: copy_from_user() buffer size is not provably correct > [enabled by default] > OK, that is surprising, because that copy is very clearly properly guarded: static int parse_command(const char __user *buffer, size_t count) { char s[4]; if (!count) return 0; if (count > 3) return -EINVAL; if (copy_from_user(s, buffer, count)) return -EFAULT; It isn't possible for count to be anything other than 1, 2 or 3 there, and it is very surprising that gcc can't see it. This might be worth filing a gcc bug for. -hpa -- To unsubscribe from this list: send the line "unsubscribe linux-parisc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/27/2013 02:52 PM, Stephen Boyd wrote: > On 02/27/13 14:19, H. Peter Anvin wrote: >> On 02/27/2013 12:42 PM, Stephen Boyd wrote: >>>> It's fine to do your patch as a first step though, which would not >>>> change the behavior. >>> A lot of arches seem to not want to enable it because false positives >>> are everywhere. It really depends on how good the compiler is at doing >>> constant propagation and dead code removal. >>> >> Although some of the cases I have seen being flagged as "false >> positives" have been real bugs. > > There were so many false-positives on x86_64 that Andrew eventually > dropped my patch to add support for this option to the copy_from_user() > function there. > I would probably have taken it, especially if it came with more x86-64 to i386 unification. It's an option, though. -hpa -- To unsubscribe from this list: send the line "unsubscribe linux-parisc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2/27/2013 2:45 PM, Stephen Rothwell wrote: > Hi all, > > On Wed, 27 Feb 2013 14:19:16 -0800 "H. Peter Anvin" <hpa@zytor.com> wrote: >> >> Although some of the cases I have seen being flagged as "false >> positives" have been real bugs. > > [hijacking the thread :-)] > > I have been getting this warning for a very long time ( which would be an > error if CONFIG_DEBUG_STRICT_USER_COPY_CHECK was set): > > i386 defconfig > i386-linux-gcc (GCC) 4.6.3 > > In file included from arch/x86/include/asm/uaccess.h:537:0, > from include/linux/uaccess.h:5, > from include/linux/highmem.h:8, > from include/linux/pagemap.h:10, > from fs/binfmt_misc.c:27: > arch/x86/include/asm/uaccess_32.h: In function 'parse_command.part.2': > arch/x86/include/asm/uaccess_32.h:211:26: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct [enabled by default] > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=35392 the gcc folks finally fixed that one it seems (but while there are some false positives, esp with older gcc, the majority were real originally.. just those all got fixed) -- To unsubscribe from this list: send the line "unsubscribe linux-parisc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/27/13 14:55, H. Peter Anvin wrote: > On 02/27/2013 02:52 PM, Stephen Boyd wrote: >> On 02/27/13 14:19, H. Peter Anvin wrote: >>> On 02/27/2013 12:42 PM, Stephen Boyd wrote: >>>>> It's fine to do your patch as a first step though, which would not >>>>> change the behavior. >>>> A lot of arches seem to not want to enable it because false positives >>>> are everywhere. It really depends on how good the compiler is at doing >>>> constant propagation and dead code removal. >>>> >>> Although some of the cases I have seen being flagged as "false >>> positives" have been real bugs. >> There were so many false-positives on x86_64 that Andrew eventually >> dropped my patch to add support for this option to the copy_from_user() >> function there. >> > I would probably have taken it, especially if it came with more x86-64 > to i386 unification. > > It's an option, though. You acked the patch[1]. Will you pick it up? [1] https://patchwork.kernel.org/patch/833192/
On 02/27/2013 03:19 PM, Stephen Boyd wrote: > On 02/27/13 14:55, H. Peter Anvin wrote: >> On 02/27/2013 02:52 PM, Stephen Boyd wrote: >>> On 02/27/13 14:19, H. Peter Anvin wrote: >>>> On 02/27/2013 12:42 PM, Stephen Boyd wrote: >>>>>> It's fine to do your patch as a first step though, which would not >>>>>> change the behavior. >>>>> A lot of arches seem to not want to enable it because false positives >>>>> are everywhere. It really depends on how good the compiler is at doing >>>>> constant propagation and dead code removal. >>>>> >>>> Although some of the cases I have seen being flagged as "false >>>> positives" have been real bugs. >>> There were so many false-positives on x86_64 that Andrew eventually >>> dropped my patch to add support for this option to the copy_from_user() >>> function there. >>> >> I would probably have taken it, especially if it came with more x86-64 >> to i386 unification. >> >> It's an option, though. > > You acked the patch[1]. Will you pick it up? > > [1] https://patchwork.kernel.org/patch/833192/ > I can, although I would prefer if it came with the uaccess/usercopy unification. -hpa -- To unsubscribe from this list: send the line "unsubscribe linux-parisc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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);
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@arndb.de> Cc: Ingo Molnar <mingo@elte.hu> Cc: H. Peter Anvin <hpa@zytor.com> Cc: linux-parisc@vger.kernel.org Cc: linux-s390@vger.kernel.org Cc: Arjan van de Ven <arjan@linux.intel.com> Cc: Helge Deller <deller@gmx.de> Cc: Heiko Carstens <heiko.carstens@de.ibm.com> Cc: Stephen Rothwell <sfr@canb.auug.org.au> Cc: Chris Metcalf <cmetcalf@tilera.com> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org> --- Per Helge Deller prodding me, I'm resending just this patch. 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