[PATCH] ARM: uaccess: Implement strict user copy checks

[PATCH] ARM: uaccess: Implement strict user copy checks

[Stephen Boyd]

This is mostly a copy from the s390 implementation (which copied
from x86 and sparc), except we print a warning if the Kconfig
option is disabled.

Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
---
 arch/arm/Kconfig.debug         |   14 ++++++++++++++
 arch/arm/include/asm/uaccess.h |   14 ++++++++++++++
 arch/arm/lib/Makefile          |    3 ++-
 arch/arm/lib/usercopy.c        |   25 +++++++++++++++++++++++++
 4 files changed, 55 insertions(+), 1 deletions(-)
 create mode 100644 arch/arm/lib/usercopy.c

diff --git a/arch/arm/Kconfig.debug b/arch/arm/Kconfig.debug
index 91344af..2cc0cdc 100644
--- a/arch/arm/Kconfig.debug
+++ b/arch/arm/Kconfig.debug
@@ -128,4 +128,18 @@ config DEBUG_S3C_UART
 	  The uncompressor code port configuration is now handled
 	  by CONFIG_S3C_LOWLEVEL_UART_PORT.
 
+config DEBUG_STRICT_USER_COPY_CHECKS
+	bool "Strict user copy size checks"
+	depends on DEBUG_KERNEL
+	help
+	  Enabling this option turns a certain set of sanity checks for user
+	  copy operations into compile time errors.
+
+	  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/arm/include/asm/uaccess.h b/arch/arm/include/asm/uaccess.h
index 33e4a48..3153e1a 100644
--- a/arch/arm/include/asm/uaccess.h
+++ b/arch/arm/include/asm/uaccess.h
@@ -401,8 +401,22 @@ extern unsigned long __must_check __clear_user_std(void __user *addr, unsigned l
 extern unsigned long __must_check __strncpy_from_user(char *to, const char __user *from, unsigned long count);
 extern unsigned long __must_check __strnlen_user(const char __user *s, long n);
 
+extern void copy_from_user_overflow(void)
+#ifdef CONFIG_DEBUG_STRICT_USER_COPY_CHECKS
+	__compiletime_error("copy_from_user() buffer size is not provably correct")
+#else
+	__compiletime_warning("copy_from_user() buffer size is not provably correct")
+#endif
+;
+
 static inline unsigned long __must_check copy_from_user(void *to, const void __user *from, unsigned long n)
 {
+	unsigned int sz = __compiletime_object_size(to);
+
+	if (unlikely(sz != -1 && sz < n)) {
+		copy_from_user_overflow();
+		return n;
+	}
 	if (access_ok(VERIFY_READ, from, n))
 		n = __copy_from_user(to, from, n);
 	else /* security hole - plug it */
diff --git a/arch/arm/lib/Makefile b/arch/arm/lib/Makefile
index 59ff42d..561cf3d 100644
--- a/arch/arm/lib/Makefile
+++ b/arch/arm/lib/Makefile
@@ -13,7 +13,8 @@ lib-y		:= backtrace.o changebit.o csumipv6.o csumpartial.o   \
 		   testchangebit.o testclearbit.o testsetbit.o        \
 		   ashldi3.o ashrdi3.o lshrdi3.o muldi3.o             \
 		   ucmpdi2.o lib1funcs.o div64.o sha1.o               \
-		   io-readsb.o io-writesb.o io-readsl.o io-writesl.o
+		   io-readsb.o io-writesb.o io-readsl.o io-writesl.o \
+		   usercopy.o
 
 mmu-y	:= clear_user.o copy_page.o getuser.o putuser.o
 
diff --git a/arch/arm/lib/usercopy.c b/arch/arm/lib/usercopy.c
new file mode 100644
index 0000000..e57e6e2
--- /dev/null
+++ b/arch/arm/lib/usercopy.c
@@ -0,0 +1,25 @@
+/*
+ * Copyright (c) 2009-2010, Code Aurora Forum. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
+ * 02110-1301, USA.
+ */
+#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);
-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH] ARM: uaccess: Implement strict user copy checks

[Stephen Boyd]

This is mostly a copy from the s390 implementation (which copied
from x86 and sparc), except we print a warning if the Kconfig
option is disabled.

Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
---
 arch/arm/Kconfig.debug         |   14 ++++++++++++++
 arch/arm/include/asm/uaccess.h |   14 ++++++++++++++
 arch/arm/lib/Makefile          |    3 ++-
 arch/arm/lib/usercopy.c        |   25 +++++++++++++++++++++++++
 4 files changed, 55 insertions(+), 1 deletions(-)
 create mode 100644 arch/arm/lib/usercopy.c

diff --git a/arch/arm/Kconfig.debug b/arch/arm/Kconfig.debug
index 91344af..2cc0cdc 100644
--- a/arch/arm/Kconfig.debug
+++ b/arch/arm/Kconfig.debug
@@ -128,4 +128,18 @@ config DEBUG_S3C_UART
 	  The uncompressor code port configuration is now handled
 	  by CONFIG_S3C_LOWLEVEL_UART_PORT.
 
+config DEBUG_STRICT_USER_COPY_CHECKS
+	bool "Strict user copy size checks"
+	depends on DEBUG_KERNEL
+	help
+	  Enabling this option turns a certain set of sanity checks for user
+	  copy operations into compile time errors.
+
+	  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/arm/include/asm/uaccess.h b/arch/arm/include/asm/uaccess.h
index 33e4a48..3153e1a 100644
--- a/arch/arm/include/asm/uaccess.h
+++ b/arch/arm/include/asm/uaccess.h
@@ -401,8 +401,22 @@ extern unsigned long __must_check __clear_user_std(void __user *addr, unsigned l
 extern unsigned long __must_check __strncpy_from_user(char *to, const char __user *from, unsigned long count);
 extern unsigned long __must_check __strnlen_user(const char __user *s, long n);
 
+extern void copy_from_user_overflow(void)
+#ifdef CONFIG_DEBUG_STRICT_USER_COPY_CHECKS
+	__compiletime_error("copy_from_user() buffer size is not provably correct")
+#else
+	__compiletime_warning("copy_from_user() buffer size is not provably correct")
+#endif
+;
+
 static inline unsigned long __must_check copy_from_user(void *to, const void __user *from, unsigned long n)
 {
+	unsigned int sz = __compiletime_object_size(to);
+
+	if (unlikely(sz != -1 && sz < n)) {
+		copy_from_user_overflow();
+		return n;
+	}
 	if (access_ok(VERIFY_READ, from, n))
 		n = __copy_from_user(to, from, n);
 	else /* security hole - plug it */
diff --git a/arch/arm/lib/Makefile b/arch/arm/lib/Makefile
index 59ff42d..561cf3d 100644
--- a/arch/arm/lib/Makefile
+++ b/arch/arm/lib/Makefile
@@ -13,7 +13,8 @@ lib-y		:= backtrace.o changebit.o csumipv6.o csumpartial.o   \
 		   testchangebit.o testclearbit.o testsetbit.o        \
 		   ashldi3.o ashrdi3.o lshrdi3.o muldi3.o             \
 		   ucmpdi2.o lib1funcs.o div64.o sha1.o               \
-		   io-readsb.o io-writesb.o io-readsl.o io-writesl.o
+		   io-readsb.o io-writesb.o io-readsl.o io-writesl.o \
+		   usercopy.o
 
 mmu-y	:= clear_user.o copy_page.o getuser.o putuser.o
 
diff --git a/arch/arm/lib/usercopy.c b/arch/arm/lib/usercopy.c
new file mode 100644
index 0000000..e57e6e2
--- /dev/null
+++ b/arch/arm/lib/usercopy.c
@@ -0,0 +1,25 @@
+/*
+ * Copyright (c) 2009-2010, Code Aurora Forum. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
+ * 02110-1301, USA.
+ */
+#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);
-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH] ARM: uaccess: Implement strict user copy checks

[Stephen Boyd]

On 08/03/2010 08:02 PM, Stephen Boyd wrote:
> This is mostly a copy from the s390 implementation (which copied
> from x86 and sparc), except we print a warning if the Kconfig
> option is disabled.
>
> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
>

Ping?

Should I submit this to the patch system?

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH] ARM: uaccess: Implement strict user copy checks

[Stephen Boyd]

On 08/03/2010 08:02 PM, Stephen Boyd wrote:
> This is mostly a copy from the s390 implementation (which copied
> from x86 and sparc), except we print a warning if the Kconfig
> option is disabled.
>
> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
>

Ping?

Should I submit this to the patch system?

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH] ARM: uaccess: Implement strict user copy checks

[Russell King - ARM Linux]

On Tue, Aug 10, 2010 at 03:46:59PM -0700, Stephen Boyd wrote:
> On 08/03/2010 08:02 PM, Stephen Boyd wrote:
>> This is mostly a copy from the s390 implementation (which copied
>> from x86 and sparc), except we print a warning if the Kconfig
>> option is disabled.
>>
>> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
>>
>
> Ping?
>
> Should I submit this to the patch system?

Unfortunately, there's quite a dearth of information on this patch,
so I can't say.  I think it needs some testing before a decision can
be made.

What compilers have been tested with this?

As the help comments intimate that it needs at least gcc 4.4, and
you've changed it to produce a compile time warning if the option is
disabled, what's the implications for older compilers?

[PATCH] ARM: uaccess: Implement strict user copy checks

[Russell King - ARM Linux]

On Tue, Aug 10, 2010 at 03:46:59PM -0700, Stephen Boyd wrote:
> On 08/03/2010 08:02 PM, Stephen Boyd wrote:
>> This is mostly a copy from the s390 implementation (which copied
>> from x86 and sparc), except we print a warning if the Kconfig
>> option is disabled.
>>
>> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
>>
>
> Ping?
>
> Should I submit this to the patch system?

Unfortunately, there's quite a dearth of information on this patch,
so I can't say.  I think it needs some testing before a decision can
be made.

What compilers have been tested with this?

As the help comments intimate that it needs at least gcc 4.4, and
you've changed it to produce a compile time warning if the option is
disabled, what's the implications for older compilers?

Re: [PATCH] ARM: uaccess: Implement strict user copy checks

[Stephen Boyd]

On 08/10/2010 03:55 PM, Russell King - ARM Linux wrote:
>
> Unfortunately, there's quite a dearth of information on this patch,
> so I can't say.  I think it needs some testing before a decision can
> be made.

Ok. I'll add more info and resend. Do you mind testing ;-)

>
> What compilers have been tested with this?

So far I've tested it with gcc-4.4.0 and 4.3.1

>
> As the help comments intimate that it needs at least gcc 4.4, and
> you've changed it to produce a compile time warning if the option is
> disabled, what's the implications for older compilers?

With older compilers (pre 4.4) __compiletime_object_size() will be 
replaced with -1 causing this code to be optimized away. Also, 
__compiletime_warning() and __compiletime_error() aren't defined to be 
anything except in include/linux/compiler-gcc4.h so users of older 
compilers shouldn't see any warnings or errors regardless of the config 
option being enabled.

People will start seeing warnings if they use gcc 4.4 or later though.
It's debatable whether or not to have both the warning and the error 
when you consider -Werror. I went this way since x86 and parisc opted 
for warnings and errors. Furthermore, I don't see any warnings or errors 
with this patch in my builds.

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH] ARM: uaccess: Implement strict user copy checks

[Stephen Boyd]

On 08/10/2010 03:55 PM, Russell King - ARM Linux wrote:
>
> Unfortunately, there's quite a dearth of information on this patch,
> so I can't say.  I think it needs some testing before a decision can
> be made.

Ok. I'll add more info and resend. Do you mind testing ;-)

>
> What compilers have been tested with this?

So far I've tested it with gcc-4.4.0 and 4.3.1

>
> As the help comments intimate that it needs at least gcc 4.4, and
> you've changed it to produce a compile time warning if the option is
> disabled, what's the implications for older compilers?

With older compilers (pre 4.4) __compiletime_object_size() will be 
replaced with -1 causing this code to be optimized away. Also, 
__compiletime_warning() and __compiletime_error() aren't defined to be 
anything except in include/linux/compiler-gcc4.h so users of older 
compilers shouldn't see any warnings or errors regardless of the config 
option being enabled.

People will start seeing warnings if they use gcc 4.4 or later though.
It's debatable whether or not to have both the warning and the error 
when you consider -Werror. I went this way since x86 and parisc opted 
for warnings and errors. Furthermore, I don't see any warnings or errors 
with this patch in my builds.

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH v2] ARM: uaccess: Implement strict user copy checks

[Stephen Boyd]

Programmers can easily forget to ensure their buffer size is
large enough to receive all the user data when calling
copy_from_user(). Add some sanity checking to copy_from_user() by
using the builtin __builtin_object_size() provided by newer GCC's
(4.4 and greater) to prove at compile time that the kernel buffer
won't overflow. This should catch some security holes earlier
since at compile time we'll either see a warning stating that
GCC can't prove the buffer size is large enough, or an error to
the same effect if CONFIG_DEBUG_STRICT_USER_COPY_CHECKS=y.

These checks are optimized away when GCC can't prove the buffer
will overflow and when it can prove the buffer won't overflow.

This patch is inspired by 9f0cf4a (x86: Use
__builtin_object_size() to validate the buffer size for
copy_from_user(), 2009-09-26).

Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Russell King <linux@arm.linux.org.uk>
---

Arnd,

I'm unsure what needs to be done for the follow up patch. Shouldn't
it be multiple patches sent to each arch maintainer to fix the
wording?

v2:
 * More descriptive commit text
 * dependency on !TRACE_BRANCH_PROFILING (see ad8f435 (x86: Don't use
 	the strict copy checks when branch profiling is in use, 2009-10-06))

 arch/arm/Kconfig.debug         |   14 ++++++++++++++
 arch/arm/include/asm/uaccess.h |   14 ++++++++++++++
 arch/arm/lib/Makefile          |    3 ++-
 arch/arm/lib/usercopy.c        |   25 +++++++++++++++++++++++++
 4 files changed, 55 insertions(+), 1 deletions(-)
 create mode 100644 arch/arm/lib/usercopy.c

diff --git a/arch/arm/Kconfig.debug b/arch/arm/Kconfig.debug
index 91344af..64e33b8 100644
--- a/arch/arm/Kconfig.debug
+++ b/arch/arm/Kconfig.debug
@@ -128,4 +128,18 @@ config DEBUG_S3C_UART
 	  The uncompressor code port configuration is now handled
 	  by CONFIG_S3C_LOWLEVEL_UART_PORT.
 
+config DEBUG_STRICT_USER_COPY_CHECKS
+	bool "Strict user 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 errors.
+
+	  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/arm/include/asm/uaccess.h b/arch/arm/include/asm/uaccess.h
index 33e4a48..3153e1a 100644
--- a/arch/arm/include/asm/uaccess.h
+++ b/arch/arm/include/asm/uaccess.h
@@ -401,8 +401,22 @@ extern unsigned long __must_check __clear_user_std(void __user *addr, unsigned l
 extern unsigned long __must_check __strncpy_from_user(char *to, const char __user *from, unsigned long count);
 extern unsigned long __must_check __strnlen_user(const char __user *s, long n);
 
+extern void copy_from_user_overflow(void)
+#ifdef CONFIG_DEBUG_STRICT_USER_COPY_CHECKS
+	__compiletime_error("copy_from_user() buffer size is not provably correct")
+#else
+	__compiletime_warning("copy_from_user() buffer size is not provably correct")
+#endif
+;
+
 static inline unsigned long __must_check copy_from_user(void *to, const void __user *from, unsigned long n)
 {
+	unsigned int sz = __compiletime_object_size(to);
+
+	if (unlikely(sz != -1 && sz < n)) {
+		copy_from_user_overflow();
+		return n;
+	}
 	if (access_ok(VERIFY_READ, from, n))
 		n = __copy_from_user(to, from, n);
 	else /* security hole - plug it */
diff --git a/arch/arm/lib/Makefile b/arch/arm/lib/Makefile
index 59ff42d..561cf3d 100644
--- a/arch/arm/lib/Makefile
+++ b/arch/arm/lib/Makefile
@@ -13,7 +13,8 @@ lib-y		:= backtrace.o changebit.o csumipv6.o csumpartial.o   \
 		   testchangebit.o testclearbit.o testsetbit.o        \
 		   ashldi3.o ashrdi3.o lshrdi3.o muldi3.o             \
 		   ucmpdi2.o lib1funcs.o div64.o sha1.o               \
-		   io-readsb.o io-writesb.o io-readsl.o io-writesl.o
+		   io-readsb.o io-writesb.o io-readsl.o io-writesl.o \
+		   usercopy.o
 
 mmu-y	:= clear_user.o copy_page.o getuser.o putuser.o
 
diff --git a/arch/arm/lib/usercopy.c b/arch/arm/lib/usercopy.c
new file mode 100644
index 0000000..e57e6e2
--- /dev/null
+++ b/arch/arm/lib/usercopy.c
@@ -0,0 +1,25 @@
+/*
+ * Copyright (c) 2009-2010, Code Aurora Forum. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
+ * 02110-1301, USA.
+ */
+#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);
-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH v2] ARM: uaccess: Implement strict user copy checks

[Stephen Boyd]

Programmers can easily forget to ensure their buffer size is
large enough to receive all the user data when calling
copy_from_user(). Add some sanity checking to copy_from_user() by
using the builtin __builtin_object_size() provided by newer GCC's
(4.4 and greater) to prove at compile time that the kernel buffer
won't overflow. This should catch some security holes earlier
since at compile time we'll either see a warning stating that
GCC can't prove the buffer size is large enough, or an error to
the same effect if CONFIG_DEBUG_STRICT_USER_COPY_CHECKS=y.

These checks are optimized away when GCC can't prove the buffer
will overflow and when it can prove the buffer won't overflow.

This patch is inspired by 9f0cf4a (x86: Use
__builtin_object_size() to validate the buffer size for
copy_from_user(), 2009-09-26).

Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Russell King <linux@arm.linux.org.uk>
---

Arnd,

I'm unsure what needs to be done for the follow up patch. Shouldn't
it be multiple patches sent to each arch maintainer to fix the
wording?

v2:
 * More descriptive commit text
 * dependency on !TRACE_BRANCH_PROFILING (see ad8f435 (x86: Don't use
 	the strict copy checks when branch profiling is in use, 2009-10-06))

 arch/arm/Kconfig.debug         |   14 ++++++++++++++
 arch/arm/include/asm/uaccess.h |   14 ++++++++++++++
 arch/arm/lib/Makefile          |    3 ++-
 arch/arm/lib/usercopy.c        |   25 +++++++++++++++++++++++++
 4 files changed, 55 insertions(+), 1 deletions(-)
 create mode 100644 arch/arm/lib/usercopy.c

diff --git a/arch/arm/Kconfig.debug b/arch/arm/Kconfig.debug
index 91344af..64e33b8 100644
--- a/arch/arm/Kconfig.debug
+++ b/arch/arm/Kconfig.debug
@@ -128,4 +128,18 @@ config DEBUG_S3C_UART
 	  The uncompressor code port configuration is now handled
 	  by CONFIG_S3C_LOWLEVEL_UART_PORT.
 
+config DEBUG_STRICT_USER_COPY_CHECKS
+	bool "Strict user 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 errors.
+
+	  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/arm/include/asm/uaccess.h b/arch/arm/include/asm/uaccess.h
index 33e4a48..3153e1a 100644
--- a/arch/arm/include/asm/uaccess.h
+++ b/arch/arm/include/asm/uaccess.h
@@ -401,8 +401,22 @@ extern unsigned long __must_check __clear_user_std(void __user *addr, unsigned l
 extern unsigned long __must_check __strncpy_from_user(char *to, const char __user *from, unsigned long count);
 extern unsigned long __must_check __strnlen_user(const char __user *s, long n);
 
+extern void copy_from_user_overflow(void)
+#ifdef CONFIG_DEBUG_STRICT_USER_COPY_CHECKS
+	__compiletime_error("copy_from_user() buffer size is not provably correct")
+#else
+	__compiletime_warning("copy_from_user() buffer size is not provably correct")
+#endif
+;
+
 static inline unsigned long __must_check copy_from_user(void *to, const void __user *from, unsigned long n)
 {
+	unsigned int sz = __compiletime_object_size(to);
+
+	if (unlikely(sz != -1 && sz < n)) {
+		copy_from_user_overflow();
+		return n;
+	}
 	if (access_ok(VERIFY_READ, from, n))
 		n = __copy_from_user(to, from, n);
 	else /* security hole - plug it */
diff --git a/arch/arm/lib/Makefile b/arch/arm/lib/Makefile
index 59ff42d..561cf3d 100644
--- a/arch/arm/lib/Makefile
+++ b/arch/arm/lib/Makefile
@@ -13,7 +13,8 @@ lib-y		:= backtrace.o changebit.o csumipv6.o csumpartial.o   \
 		   testchangebit.o testclearbit.o testsetbit.o        \
 		   ashldi3.o ashrdi3.o lshrdi3.o muldi3.o             \
 		   ucmpdi2.o lib1funcs.o div64.o sha1.o               \
-		   io-readsb.o io-writesb.o io-readsl.o io-writesl.o
+		   io-readsb.o io-writesb.o io-readsl.o io-writesl.o \
+		   usercopy.o
 
 mmu-y	:= clear_user.o copy_page.o getuser.o putuser.o
 
diff --git a/arch/arm/lib/usercopy.c b/arch/arm/lib/usercopy.c
new file mode 100644
index 0000000..e57e6e2
--- /dev/null
+++ b/arch/arm/lib/usercopy.c
@@ -0,0 +1,25 @@
+/*
+ * Copyright (c) 2009-2010, Code Aurora Forum. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
+ * 02110-1301, USA.
+ */
+#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);
-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH v2] ARM: uaccess: Implement strict user copy checks

[Arnd Bergmann]

On Wednesday 18 August 2010, Stephen Boyd wrote:
> 
> I'm unsure what needs to be done for the follow up patch. Shouldn't
> it be multiple patches sent to each arch maintainer to fix the
> wording?

No, that will just result in half of them applying it, best make 
a single patch and send it to linux-arch@vger.kernel.org for review.

It's probably best to move the config option to lib/Kconfig.debug
so it only appears once, and make it depend on DEBUG_USER_COPY_CHECKS,
which is then unconditionally defined by the architectures that want it.

	Arnd

[PATCH v2] ARM: uaccess: Implement strict user copy checks

[Arnd Bergmann]

On Wednesday 18 August 2010, Stephen Boyd wrote:
> 
> I'm unsure what needs to be done for the follow up patch. Shouldn't
> it be multiple patches sent to each arch maintainer to fix the
> wording?

No, that will just result in half of them applying it, best make 
a single patch and send it to linux-arch at vger.kernel.org for review.

It's probably best to move the config option to lib/Kconfig.debug
so it only appears once, and make it depend on DEBUG_USER_COPY_CHECKS,
which is then unconditionally defined by the architectures that want it.

	Arnd

Re: [PATCH v2] ARM: uaccess: Implement strict user copy checks

[Stephen Boyd]

On 08/18/2010 05:28 AM, Arnd Bergmann wrote:
> On Wednesday 18 August 2010, Stephen Boyd wrote:
>>
>> I'm unsure what needs to be done for the follow up patch. Shouldn't
>> it be multiple patches sent to each arch maintainer to fix the
>> wording?
>
> No, that will just result in half of them applying it, best make
> a single patch and send it to linux-arch@vger.kernel.org for review.
>
> It's probably best to move the config option to lib/Kconfig.debug
> so it only appears once, and make it depend on DEBUG_USER_COPY_CHECKS,
> which is then unconditionally defined by the architectures that want it.

Ok.

So the only sticking point now is that x86, parisc, and arm use warnings 
and errors but s390 only uses warnings. I guess I'll reword it to be:

	Enabling this option turns a certain set of sanity checks for
	user copy operations into compile time warnings/errors.

	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 where this
	option is a no-op, say N.

or I'll add a patch to make s390 trigger an error when this is enabled?

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH v2] ARM: uaccess: Implement strict user copy checks

[Stephen Boyd]

On 08/18/2010 05:28 AM, Arnd Bergmann wrote:
> On Wednesday 18 August 2010, Stephen Boyd wrote:
>>
>> I'm unsure what needs to be done for the follow up patch. Shouldn't
>> it be multiple patches sent to each arch maintainer to fix the
>> wording?
>
> No, that will just result in half of them applying it, best make
> a single patch and send it to linux-arch at vger.kernel.org for review.
>
> It's probably best to move the config option to lib/Kconfig.debug
> so it only appears once, and make it depend on DEBUG_USER_COPY_CHECKS,
> which is then unconditionally defined by the architectures that want it.

Ok.

So the only sticking point now is that x86, parisc, and arm use warnings 
and errors but s390 only uses warnings. I guess I'll reword it to be:

	Enabling this option turns a certain set of sanity checks for
	user copy operations into compile time warnings/errors.

	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 where this
	option is a no-op, say N.

or I'll add a patch to make s390 trigger an error when this is enabled?

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH v2] ARM: uaccess: Implement strict user copy checks

[Arnd Bergmann]

On Wednesday 18 August 2010, Stephen Boyd wrote:
> So the only sticking point now is that x86, parisc, and arm use warnings 
> and errors but s390 only uses warnings. I guess I'll reword it to be:
> 
>         Enabling this option turns a certain set of sanity checks for
>         user copy operations into compile time warnings/errors.
> 
>         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 where this
>         option is a no-op, say N.
> 
> or I'll add a patch to make s390 trigger an error when this is enabled?

(Taking Martin and Heiko on Cc for s390)

I'd strongly suggest making the behavior the same for everyone. It should
be fairly easy to make sure none of these warnings ever triggers
on s390, because most of the Linux device driver code does not get build
there anyway.

I'd also drop the part about old compilers and just say
"If unsure, say N".

	Arnd

[PATCH v2] ARM: uaccess: Implement strict user copy checks

[Arnd Bergmann]

On Wednesday 18 August 2010, Stephen Boyd wrote:
> So the only sticking point now is that x86, parisc, and arm use warnings 
> and errors but s390 only uses warnings. I guess I'll reword it to be:
> 
>         Enabling this option turns a certain set of sanity checks for
>         user copy operations into compile time warnings/errors.
> 
>         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 where this
>         option is a no-op, say N.
> 
> or I'll add a patch to make s390 trigger an error when this is enabled?

(Taking Martin and Heiko on Cc for s390)

I'd strongly suggest making the behavior the same for everyone. It should
be fairly easy to make sure none of these warnings ever triggers
on s390, because most of the Linux device driver code does not get build
there anyway.

I'd also drop the part about old compilers and just say
"If unsure, say N".

	Arnd

Re: [PATCH v2] ARM: uaccess: Implement strict user copy checks

[Heiko Carstens]

On Thu, Aug 19, 2010 at 01:09:15PM +0200, Arnd Bergmann wrote:
> On Wednesday 18 August 2010, Stephen Boyd wrote:
> > So the only sticking point now is that x86, parisc, and arm use warnings 
> > and errors but s390 only uses warnings. I guess I'll reword it to be:
> > 
> >         Enabling this option turns a certain set of sanity checks for
> >         user copy operations into compile time warnings/errors.
> > 
> >         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 where this
> >         option is a no-op, say N.
> > 
> > or I'll add a patch to make s390 trigger an error when this is enabled?
> 
> (Taking Martin and Heiko on Cc for s390)
> 
> I'd strongly suggest making the behavior the same for everyone. It should
> be fairly easy to make sure none of these warnings ever triggers
> on s390, because most of the Linux device driver code does not get build
> there anyway.

Please don't do that. An s390 allyesconfig still triggers 45 warnings and
I'm currently not willing to "patch" working code just to get rid of these
warnings which are most likely all false positives.
That's the reason why we currently don't error out and only generate
warnings.

[PATCH v2] ARM: uaccess: Implement strict user copy checks

[Heiko Carstens]

On Thu, Aug 19, 2010 at 01:09:15PM +0200, Arnd Bergmann wrote:
> On Wednesday 18 August 2010, Stephen Boyd wrote:
> > So the only sticking point now is that x86, parisc, and arm use warnings 
> > and errors but s390 only uses warnings. I guess I'll reword it to be:
> > 
> >         Enabling this option turns a certain set of sanity checks for
> >         user copy operations into compile time warnings/errors.
> > 
> >         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 where this
> >         option is a no-op, say N.
> > 
> > or I'll add a patch to make s390 trigger an error when this is enabled?
> 
> (Taking Martin and Heiko on Cc for s390)
> 
> I'd strongly suggest making the behavior the same for everyone. It should
> be fairly easy to make sure none of these warnings ever triggers
> on s390, because most of the Linux device driver code does not get build
> there anyway.

Please don't do that. An s390 allyesconfig still triggers 45 warnings and
I'm currently not willing to "patch" working code just to get rid of these
warnings which are most likely all false positives.
That's the reason why we currently don't error out and only generate
warnings.

Re: [PATCH v2] ARM: uaccess: Implement strict user copy checks

[Arnd Bergmann]

On Tuesday 24 August 2010, Heiko Carstens wrote:
> > (Taking Martin and Heiko on Cc for s390)
> > 
> > I'd strongly suggest making the behavior the same for everyone. It should
> > be fairly easy to make sure none of these warnings ever triggers
> > on s390, because most of the Linux device driver code does not get build
> > there anyway.
> 
> Please don't do that. An s390 allyesconfig still triggers 45 warnings and
> I'm currently not willing to "patch" working code just to get rid of these
> warnings which are most likely all false positives.
> That's the reason why we currently don't error out and only generate
> warnings.

Can't you just turn that option off then? Or are you worried about
allyesconfig builds?

The current state is confusing because on s390
CONFIG_DEBUG_STRICT_USER_COPY_CHECKS means that gcc will warn rather
than ignore the finding, while on all others, the same option turns
a warning into an error.

Test-building an allmodconfig on s390 showed these warnings only in
architecture independent code, and I agree that they are all false
positives.

	Arnd

[PATCH v2] ARM: uaccess: Implement strict user copy checks

[Arnd Bergmann]

On Tuesday 24 August 2010, Heiko Carstens wrote:
> > (Taking Martin and Heiko on Cc for s390)
> > 
> > I'd strongly suggest making the behavior the same for everyone. It should
> > be fairly easy to make sure none of these warnings ever triggers
> > on s390, because most of the Linux device driver code does not get build
> > there anyway.
> 
> Please don't do that. An s390 allyesconfig still triggers 45 warnings and
> I'm currently not willing to "patch" working code just to get rid of these
> warnings which are most likely all false positives.
> That's the reason why we currently don't error out and only generate
> warnings.

Can't you just turn that option off then? Or are you worried about
allyesconfig builds?

The current state is confusing because on s390
CONFIG_DEBUG_STRICT_USER_COPY_CHECKS means that gcc will warn rather
than ignore the finding, while on all others, the same option turns
a warning into an error.

Test-building an allmodconfig on s390 showed these warnings only in
architecture independent code, and I agree that they are all false
positives.

	Arnd

Re: [PATCH v2] ARM: uaccess: Implement strict user copy checks

[Heiko Carstens]

On Tue, Aug 24, 2010 at 05:26:08PM +0200, Arnd Bergmann wrote:
> On Tuesday 24 August 2010, Heiko Carstens wrote:
> > > (Taking Martin and Heiko on Cc for s390)
> > > 
> > > I'd strongly suggest making the behavior the same for everyone. It should
> > > be fairly easy to make sure none of these warnings ever triggers
> > > on s390, because most of the Linux device driver code does not get build
> > > there anyway.
> > 
> > Please don't do that. An s390 allyesconfig still triggers 45 warnings and
> > I'm currently not willing to "patch" working code just to get rid of these
> > warnings which are most likely all false positives.
> > That's the reason why we currently don't error out and only generate
> > warnings.
> 
> Can't you just turn that option off then? Or are you worried about
> allyesconfig builds?

I'd like to keep an allyesconfig compiling and booting.
With the proposed change we would never see a green entry at
http://kisskb.ellerman.id.au/kisskb/branch/9/ for s390's allyesconfig
build ;)
And it would make it a bit harder to find the usual !HAS_DMA and
!HAS_IOMEM build breakages we see quite frequently. No reason to make
it even more difficult to keep s390 compiling.

> The current state is confusing because on s390
> CONFIG_DEBUG_STRICT_USER_COPY_CHECKS means that gcc will warn rather
> than ignore the finding, while on all others, the same option turns
> a warning into an error.

Then maybe add a "choice" Kconfig option in a way that both allyesconfig
as well as allnoconfig will build?

[PATCH v2] ARM: uaccess: Implement strict user copy checks

[Heiko Carstens]

On Tue, Aug 24, 2010 at 05:26:08PM +0200, Arnd Bergmann wrote:
> On Tuesday 24 August 2010, Heiko Carstens wrote:
> > > (Taking Martin and Heiko on Cc for s390)
> > > 
> > > I'd strongly suggest making the behavior the same for everyone. It should
> > > be fairly easy to make sure none of these warnings ever triggers
> > > on s390, because most of the Linux device driver code does not get build
> > > there anyway.
> > 
> > Please don't do that. An s390 allyesconfig still triggers 45 warnings and
> > I'm currently not willing to "patch" working code just to get rid of these
> > warnings which are most likely all false positives.
> > That's the reason why we currently don't error out and only generate
> > warnings.
> 
> Can't you just turn that option off then? Or are you worried about
> allyesconfig builds?

I'd like to keep an allyesconfig compiling and booting.
With the proposed change we would never see a green entry at
http://kisskb.ellerman.id.au/kisskb/branch/9/ for s390's allyesconfig
build ;)
And it would make it a bit harder to find the usual !HAS_DMA and
!HAS_IOMEM build breakages we see quite frequently. No reason to make
it even more difficult to keep s390 compiling.

> The current state is confusing because on s390
> CONFIG_DEBUG_STRICT_USER_COPY_CHECKS means that gcc will warn rather
> than ignore the finding, while on all others, the same option turns
> a warning into an error.

Then maybe add a "choice" Kconfig option in a way that both allyesconfig
as well as allnoconfig will build?

Re: [PATCH v2] ARM: uaccess: Implement strict user copy checks

[Arnd Bergmann]

On Tuesday 24 August 2010, Heiko Carstens wrote:
> On Tue, Aug 24, 2010 at 05:26:08PM +0200, Arnd Bergmann wrote:
> > Can't you just turn that option off then? Or are you worried about
> > allyesconfig builds?
> 
> I'd like to keep an allyesconfig compiling and booting.
> With the proposed change we would never see a green entry at
> http://kisskb.ellerman.id.au/kisskb/branch/9/ for s390's allyesconfig
> build ;)

Yes, that makes sense.

> Then maybe add a "choice" Kconfig option in a way that both allyesconfig
> as well as allnoconfig will build?

I think it would be easier to remove the config option entirely on s390
and just always warn. As I said earlier in this thread, I generally
don't think this particular warning is more important than a lot of
the other ones that we don't turn into errors.

I do think it would be helpful to optionally build parts of the kernel
with the much stronger '-Werror', which we already do for some
architectures. You could do that with inverted logic (bool "Disable -Werror
compile option) and fix all warnings in allnoconfig to make all of
allnoconfig, allyesconfig and defconfig build.

	Arnd

[PATCH v2] ARM: uaccess: Implement strict user copy checks

[Arnd Bergmann]

On Tuesday 24 August 2010, Heiko Carstens wrote:
> On Tue, Aug 24, 2010 at 05:26:08PM +0200, Arnd Bergmann wrote:
> > Can't you just turn that option off then? Or are you worried about
> > allyesconfig builds?
> 
> I'd like to keep an allyesconfig compiling and booting.
> With the proposed change we would never see a green entry at
> http://kisskb.ellerman.id.au/kisskb/branch/9/ for s390's allyesconfig
> build ;)

Yes, that makes sense.

> Then maybe add a "choice" Kconfig option in a way that both allyesconfig
> as well as allnoconfig will build?

I think it would be easier to remove the config option entirely on s390
and just always warn. As I said earlier in this thread, I generally
don't think this particular warning is more important than a lot of
the other ones that we don't turn into errors.

I do think it would be helpful to optionally build parts of the kernel
with the much stronger '-Werror', which we already do for some
architectures. You could do that with inverted logic (bool "Disable -Werror
compile option) and fix all warnings in allnoconfig to make all of
allnoconfig, allyesconfig and defconfig build.

	Arnd

Re: [PATCH v2] ARM: uaccess: Implement strict user copy checks

[Heiko Carstens]

On Wed, Aug 25, 2010 at 02:14:16PM +0200, Arnd Bergmann wrote:
> > Then maybe add a "choice" Kconfig option in a way that both allyesconfig
> > as well as allnoconfig will build?
> 
> I think it would be easier to remove the config option entirely on s390
> and just always warn. As I said earlier in this thread, I generally
> don't think this particular warning is more important than a lot of
> the other ones that we don't turn into errors.

I disagree: a default kernel build should compile without the noise of
tens of false positive warnings.
Nobody would look at new warnings and fix possible bugs.
That's why I want to have an option to turn the warnings off (default).

Or are you volunteering to "fix" all the false positives? :)

[PATCH v2] ARM: uaccess: Implement strict user copy checks

[Heiko Carstens]

On Wed, Aug 25, 2010 at 02:14:16PM +0200, Arnd Bergmann wrote:
> > Then maybe add a "choice" Kconfig option in a way that both allyesconfig
> > as well as allnoconfig will build?
> 
> I think it would be easier to remove the config option entirely on s390
> and just always warn. As I said earlier in this thread, I generally
> don't think this particular warning is more important than a lot of
> the other ones that we don't turn into errors.

I disagree: a default kernel build should compile without the noise of
tens of false positive warnings.
Nobody would look at new warnings and fix possible bugs.
That's why I want to have an option to turn the warnings off (default).

Or are you volunteering to "fix" all the false positives? :)

Re: [PATCH v2] ARM: uaccess: Implement strict user copy checks

[Arnd Bergmann]

On Wednesday 25 August 2010, Heiko Carstens wrote:
> 
> On Wed, Aug 25, 2010 at 02:14:16PM +0200, Arnd Bergmann wrote:
> > > Then maybe add a "choice" Kconfig option in a way that both allyesconfig
> > > as well as allnoconfig will build?
> > 
> > I think it would be easier to remove the config option entirely on s390
> > and just always warn. As I said earlier in this thread, I generally
> > don't think this particular warning is more important than a lot of
> > the other ones that we don't turn into errors.
> 
> I disagree: a default kernel build should compile without the noise of
> tens of false positive warnings.
> Nobody would look at new warnings and fix possible bugs.
> That's why I want to have an option to turn the warnings off (default).
> 
> Or are you volunteering to "fix" all the false positives? :)

If you don't want to see the warnings, then just remove the strict checks.
We already concluded that there is little value in them on s390 since it only
shows false postives.

Maybe the easiest way would be to rename the option on s390 and move all
the other ones into a common place.

	Arnd

[PATCH v2] ARM: uaccess: Implement strict user copy checks

[Arnd Bergmann]

On Wednesday 25 August 2010, Heiko Carstens wrote:
> 
> On Wed, Aug 25, 2010 at 02:14:16PM +0200, Arnd Bergmann wrote:
> > > Then maybe add a "choice" Kconfig option in a way that both allyesconfig
> > > as well as allnoconfig will build?
> > 
> > I think it would be easier to remove the config option entirely on s390
> > and just always warn. As I said earlier in this thread, I generally
> > don't think this particular warning is more important than a lot of
> > the other ones that we don't turn into errors.
> 
> I disagree: a default kernel build should compile without the noise of
> tens of false positive warnings.
> Nobody would look at new warnings and fix possible bugs.
> That's why I want to have an option to turn the warnings off (default).
> 
> Or are you volunteering to "fix" all the false positives? :)

If you don't want to see the warnings, then just remove the strict checks.
We already concluded that there is little value in them on s390 since it only
shows false postives.

Maybe the easiest way would be to rename the option on s390 and move all
the other ones into a common place.

	Arnd

Re: [PATCH v2] ARM: uaccess: Implement strict user copy checks

[Heiko Carstens]

On Wed, Aug 25, 2010 at 03:55:53PM +0200, Arnd Bergmann wrote:
> On Wednesday 25 August 2010, Heiko Carstens wrote:
> > > I think it would be easier to remove the config option entirely on s390
> > > and just always warn. As I said earlier in this thread, I generally
> > > don't think this particular warning is more important than a lot of
> > > the other ones that we don't turn into errors.
> > 
> > I disagree: a default kernel build should compile without the noise of
> > tens of false positive warnings.
> > Nobody would look at new warnings and fix possible bugs.
> > That's why I want to have an option to turn the warnings off (default).
> > 
> > Or are you volunteering to "fix" all the false positives? :)
> 
> If you don't want to see the warnings, then just remove the strict checks.
> We already concluded that there is little value in them on s390 since it only
> shows false postives.
> 
> Maybe the easiest way would be to rename the option on s390 and move all
> the other ones into a common place.

Yes, feel free to do that.

[PATCH v2] ARM: uaccess: Implement strict user copy checks

[Heiko Carstens]

On Wed, Aug 25, 2010 at 03:55:53PM +0200, Arnd Bergmann wrote:
> On Wednesday 25 August 2010, Heiko Carstens wrote:
> > > I think it would be easier to remove the config option entirely on s390
> > > and just always warn. As I said earlier in this thread, I generally
> > > don't think this particular warning is more important than a lot of
> > > the other ones that we don't turn into errors.
> > 
> > I disagree: a default kernel build should compile without the noise of
> > tens of false positive warnings.
> > Nobody would look at new warnings and fix possible bugs.
> > That's why I want to have an option to turn the warnings off (default).
> > 
> > Or are you volunteering to "fix" all the false positives? :)
> 
> If you don't want to see the warnings, then just remove the strict checks.
> We already concluded that there is little value in them on s390 since it only
> shows false postives.
> 
> Maybe the easiest way would be to rename the option on s390 and move all
> the other ones into a common place.

Yes, feel free to do that.

Re: [PATCH v2] ARM: uaccess: Implement strict user copy checks

[Stephen Boyd]

On 08/25/2010 07:40 AM, Heiko Carstens wrote:
> On Wed, Aug 25, 2010 at 03:55:53PM +0200, Arnd Bergmann wrote:
>>
>> If you don't want to see the warnings, then just remove the strict checks.
>> We already concluded that there is little value in them on s390 since it only
>> shows false postives.
>>
>> Maybe the easiest way would be to rename the option on s390 and move all
>> the other ones into a common place.
> 
> Yes, feel free to do that.
> 

Can you put up the false positives somewhere? I don't have easy access
to an s390 toolchain to test build with and I'm interested to see how
bad the false positives are.

I'm slightly concerned that we'll just have this problem again when
another arch comes along with false positives. But ignoring that issue
is probably fine. I'll respin with a patch to move s390 to something
like DEBUG_WARN_USER_COPY_CHECKS.

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH v2] ARM: uaccess: Implement strict user copy checks

[Stephen Boyd]

On 08/25/2010 07:40 AM, Heiko Carstens wrote:
> On Wed, Aug 25, 2010 at 03:55:53PM +0200, Arnd Bergmann wrote:
>>
>> If you don't want to see the warnings, then just remove the strict checks.
>> We already concluded that there is little value in them on s390 since it only
>> shows false postives.
>>
>> Maybe the easiest way would be to rename the option on s390 and move all
>> the other ones into a common place.
> 
> Yes, feel free to do that.
> 

Can you put up the false positives somewhere? I don't have easy access
to an s390 toolchain to test build with and I'm interested to see how
bad the false positives are.

I'm slightly concerned that we'll just have this problem again when
another arch comes along with false positives. But ignoring that issue
is probably fine. I'll respin with a patch to move s390 to something
like DEBUG_WARN_USER_COPY_CHECKS.

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH v2] ARM: uaccess: Implement strict user copy checks

[Heiko Carstens]

On Fri, Aug 27, 2010 at 06:35:16PM -0700, Stephen Boyd wrote:
> On 08/25/2010 07:40 AM, Heiko Carstens wrote:
> > On Wed, Aug 25, 2010 at 03:55:53PM +0200, Arnd Bergmann wrote:
> >>
> >> If you don't want to see the warnings, then just remove the strict checks.
> >> We already concluded that there is little value in them on s390 since it only
> >> shows false postives.
> >>
> >> Maybe the easiest way would be to rename the option on s390 and move all
> >> the other ones into a common place.
> > 
> > Yes, feel free to do that.
> 
> Can you put up the false positives somewhere? I don't have easy access
> to an s390 toolchain to test build with and I'm interested to see how
> bad the false positives are.
> 
> I'm slightly concerned that we'll just have this problem again when
> another arch comes along with false positives. But ignoring that issue
> is probably fine. I'll respin with a patch to move s390 to something
> like DEBUG_WARN_USER_COPY_CHECKS.

Sure:

In function 'copy_from_user',
    inlined from 'write_enabled_file_bool' at kernel/kprobes.c:1973:
/linux-2.6/arch/s390/include/asm/uaccess.h:297: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
In function 'copy_from_user',
    inlined from 'perf_copy_attr' at kernel/perf_event.c:4988,
    inlined from 'SYSC_perf_event_open' at kernel/perf_event.c:5092,
    inlined from 'SyS_perf_event_open' at kernel/perf_event.c:5077:
/linux-2.6/arch/s390/include/asm/uaccess.h:297: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
In function 'copy_from_user',
    inlined from '__tun_chr_ioctl' at drivers/net/tun.c:1200:
/linux-2.6/arch/s390/include/asm/uaccess.h:297: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
In function 'copy_from_user',
    inlined from 'pktgen_if_write' at net/core/pktgen.c:880:
/linux-2.6/arch/s390/include/asm/uaccess.h:297: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
In function 'copy_from_user',
    inlined from 'pktgen_if_write' at net/core/pktgen.c:1143:
/linux-2.6/arch/s390/include/asm/uaccess.h:297: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
In function 'copy_from_user',
    inlined from 'pktgen_if_write' at net/core/pktgen.c:1250:
/linux-2.6/arch/s390/include/asm/uaccess.h:297: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
In function 'copy_from_user',
    inlined from 'pktgen_if_write' at net/core/pktgen.c:1272:
/linux-2.6/arch/s390/include/asm/uaccess.h:297: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
In function 'copy_from_user',
    inlined from 'pktgen_if_write' at net/core/pktgen.c:1296:
/linux-2.6/arch/s390/include/asm/uaccess.h:297: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
In function 'copy_from_user',
    inlined from 'pktgen_if_write' at net/core/pktgen.c:1319:
/linux-2.6/arch/s390/include/asm/uaccess.h:297: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
In function 'copy_from_user',
    inlined from 'pktgen_if_write' at net/core/pktgen.c:1342:
/linux-2.6/arch/s390/include/asm/uaccess.h:297: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
In function 'copy_from_user',
    inlined from 'pktgen_if_write' at net/core/pktgen.c:1363:
/linux-2.6/arch/s390/include/asm/uaccess.h:297: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
In function 'copy_from_user',
    inlined from 'pktgen_if_write' at net/core/pktgen.c:1384:
/linux-2.6/arch/s390/include/asm/uaccess.h:297: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
In function 'copy_from_user',
    inlined from 'pktgen_if_write' at net/core/pktgen.c:1405:
/linux-2.6/arch/s390/include/asm/uaccess.h:297: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
In function 'copy_from_user',
    inlined from 'pktgen_if_write' at net/core/pktgen.c:1432:
/linux-2.6/arch/s390/include/asm/uaccess.h:297: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
In function 'copy_from_user',
    inlined from 'pktgen_if_write' at net/core/pktgen.c:1468:
/linux-2.6/arch/s390/include/asm/uaccess.h:297: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
In function 'copy_from_user',
    inlined from 'pktgen_thread_write' at net/core/pktgen.c:1792:
/linux-2.6/arch/s390/include/asm/uaccess.h:297: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
In function 'copy_from_user',
    inlined from 'pktgen_thread_write' at net/core/pktgen.c:1823:
/linux-2.6/arch/s390/include/asm/uaccess.h:297: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
In function 'copy_from_user',
    inlined from 'write_file_bool' at fs/debugfs/file.c:434:
/linux-2.6/arch/s390/include/asm/uaccess.h:297: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
In function 'copy_from_user',
    inlined from 'sg_proc_write_dressz' at drivers/scsi/sg.c:2396:
/linux-2.6/arch/s390/include/asm/uaccess.h:297: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
In function 'copy_from_user',
    inlined from 'sg_proc_write_adio' at drivers/scsi/sg.c:2373:
/linux-2.6/arch/s390/include/asm/uaccess.h:297: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
In function 'copy_from_user',
    inlined from 'packet_getsockopt' at net/packet/af_packet.c:2123:
/linux-2.6/arch/s390/include/asm/uaccess.h:297: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
In function 'copy_from_user',
    inlined from 'compat_sys_socketcall' at net/compat.c:783:
/linux-2.6/arch/s390/include/asm/uaccess.h:297: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct

[PATCH v2] ARM: uaccess: Implement strict user copy checks

[Heiko Carstens]

On Fri, Aug 27, 2010 at 06:35:16PM -0700, Stephen Boyd wrote:
> On 08/25/2010 07:40 AM, Heiko Carstens wrote:
> > On Wed, Aug 25, 2010 at 03:55:53PM +0200, Arnd Bergmann wrote:
> >>
> >> If you don't want to see the warnings, then just remove the strict checks.
> >> We already concluded that there is little value in them on s390 since it only
> >> shows false postives.
> >>
> >> Maybe the easiest way would be to rename the option on s390 and move all
> >> the other ones into a common place.
> > 
> > Yes, feel free to do that.
> 
> Can you put up the false positives somewhere? I don't have easy access
> to an s390 toolchain to test build with and I'm interested to see how
> bad the false positives are.
> 
> I'm slightly concerned that we'll just have this problem again when
> another arch comes along with false positives. But ignoring that issue
> is probably fine. I'll respin with a patch to move s390 to something
> like DEBUG_WARN_USER_COPY_CHECKS.

Sure:

In function 'copy_from_user',
    inlined from 'write_enabled_file_bool' at kernel/kprobes.c:1973:
/linux-2.6/arch/s390/include/asm/uaccess.h:297: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
In function 'copy_from_user',
    inlined from 'perf_copy_attr' at kernel/perf_event.c:4988,
    inlined from 'SYSC_perf_event_open' at kernel/perf_event.c:5092,
    inlined from 'SyS_perf_event_open' at kernel/perf_event.c:5077:
/linux-2.6/arch/s390/include/asm/uaccess.h:297: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
In function 'copy_from_user',
    inlined from '__tun_chr_ioctl' at drivers/net/tun.c:1200:
/linux-2.6/arch/s390/include/asm/uaccess.h:297: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
In function 'copy_from_user',
    inlined from 'pktgen_if_write' at net/core/pktgen.c:880:
/linux-2.6/arch/s390/include/asm/uaccess.h:297: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
In function 'copy_from_user',
    inlined from 'pktgen_if_write' at net/core/pktgen.c:1143:
/linux-2.6/arch/s390/include/asm/uaccess.h:297: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
In function 'copy_from_user',
    inlined from 'pktgen_if_write' at net/core/pktgen.c:1250:
/linux-2.6/arch/s390/include/asm/uaccess.h:297: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
In function 'copy_from_user',
    inlined from 'pktgen_if_write' at net/core/pktgen.c:1272:
/linux-2.6/arch/s390/include/asm/uaccess.h:297: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
In function 'copy_from_user',
    inlined from 'pktgen_if_write' at net/core/pktgen.c:1296:
/linux-2.6/arch/s390/include/asm/uaccess.h:297: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
In function 'copy_from_user',
    inlined from 'pktgen_if_write' at net/core/pktgen.c:1319:
/linux-2.6/arch/s390/include/asm/uaccess.h:297: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
In function 'copy_from_user',
    inlined from 'pktgen_if_write' at net/core/pktgen.c:1342:
/linux-2.6/arch/s390/include/asm/uaccess.h:297: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
In function 'copy_from_user',
    inlined from 'pktgen_if_write' at net/core/pktgen.c:1363:
/linux-2.6/arch/s390/include/asm/uaccess.h:297: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
In function 'copy_from_user',
    inlined from 'pktgen_if_write' at net/core/pktgen.c:1384:
/linux-2.6/arch/s390/include/asm/uaccess.h:297: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
In function 'copy_from_user',
    inlined from 'pktgen_if_write' at net/core/pktgen.c:1405:
/linux-2.6/arch/s390/include/asm/uaccess.h:297: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
In function 'copy_from_user',
    inlined from 'pktgen_if_write' at net/core/pktgen.c:1432:
/linux-2.6/arch/s390/include/asm/uaccess.h:297: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
In function 'copy_from_user',
    inlined from 'pktgen_if_write' at net/core/pktgen.c:1468:
/linux-2.6/arch/s390/include/asm/uaccess.h:297: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
In function 'copy_from_user',
    inlined from 'pktgen_thread_write' at net/core/pktgen.c:1792:
/linux-2.6/arch/s390/include/asm/uaccess.h:297: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
In function 'copy_from_user',
    inlined from 'pktgen_thread_write' at net/core/pktgen.c:1823:
/linux-2.6/arch/s390/include/asm/uaccess.h:297: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
In function 'copy_from_user',
    inlined from 'write_file_bool' at fs/debugfs/file.c:434:
/linux-2.6/arch/s390/include/asm/uaccess.h:297: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
In function 'copy_from_user',
    inlined from 'sg_proc_write_dressz' at drivers/scsi/sg.c:2396:
/linux-2.6/arch/s390/include/asm/uaccess.h:297: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
In function 'copy_from_user',
    inlined from 'sg_proc_write_adio' at drivers/scsi/sg.c:2373:
/linux-2.6/arch/s390/include/asm/uaccess.h:297: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
In function 'copy_from_user',
    inlined from 'packet_getsockopt' at net/packet/af_packet.c:2123:
/linux-2.6/arch/s390/include/asm/uaccess.h:297: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
In function 'copy_from_user',
    inlined from 'compat_sys_socketcall' at net/compat.c:783:
/linux-2.6/arch/s390/include/asm/uaccess.h:297: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct

Re: [PATCH v2] ARM: uaccess: Implement strict user copy checks

[Arnd Bergmann]

On Saturday 28 August 2010 09:43:04 Heiko Carstens wrote:
> In function 'copy_from_user',
>     inlined from '__tun_chr_ioctl' at drivers/net/tun.c:1200:
> /linux-2.6/arch/s390/include/asm/uaccess.h:297: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct

I wrote this one, and I can't think of an easy way to do fix
it without increasing the code complexity or size.

> In function 'copy_from_user',
>     inlined from 'write_file_bool' at fs/debugfs/file.c:434:
> /linux-2.6/arch/s390/include/asm/uaccess.h:297: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
> In function 'copy_from_user',
>     inlined from 'packet_getsockopt' at net/packet/af_packet.c:2123:
> /linux-2.6/arch/s390/include/asm/uaccess.h:297: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct

These look like the compiler is not smart enough. Both make sure
that we copy at most the size of the object, or less if the user
didn't pass all of it.

> In function 'copy_from_user',
>     inlined from 'compat_sys_socketcall' at net/compat.c:783:
> /linux-2.6/arch/s390/include/asm/uaccess.h:297: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct

I don't think the compiler has a chance to figure this one out.
However, I don't see the warning on x86. Maybe x86-gcc has
a bug.

	Arnd

[PATCH v2] ARM: uaccess: Implement strict user copy checks

[Arnd Bergmann]

On Saturday 28 August 2010 09:43:04 Heiko Carstens wrote:
> In function 'copy_from_user',
>     inlined from '__tun_chr_ioctl' at drivers/net/tun.c:1200:
> /linux-2.6/arch/s390/include/asm/uaccess.h:297: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct

I wrote this one, and I can't think of an easy way to do fix
it without increasing the code complexity or size.

> In function 'copy_from_user',
>     inlined from 'write_file_bool' at fs/debugfs/file.c:434:
> /linux-2.6/arch/s390/include/asm/uaccess.h:297: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
> In function 'copy_from_user',
>     inlined from 'packet_getsockopt' at net/packet/af_packet.c:2123:
> /linux-2.6/arch/s390/include/asm/uaccess.h:297: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct

These look like the compiler is not smart enough. Both make sure
that we copy at most the size of the object, or less if the user
didn't pass all of it.

> In function 'copy_from_user',
>     inlined from 'compat_sys_socketcall' at net/compat.c:783:
> /linux-2.6/arch/s390/include/asm/uaccess.h:297: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct

I don't think the compiler has a chance to figure this one out.
However, I don't see the warning on x86. Maybe x86-gcc has
a bug.

	Arnd

Re: [PATCH v2] ARM: uaccess: Implement strict user copy checks

[Stephen Boyd]

On 08/28/2010 12:43 AM, Heiko Carstens wrote:
> On Fri, Aug 27, 2010 at 06:35:16PM -0700, Stephen Boyd wrote:
>> On 08/25/2010 07:40 AM, Heiko Carstens wrote:
>>> On Wed, Aug 25, 2010 at 03:55:53PM +0200, Arnd Bergmann wrote:
>>>>
>>>> If you don't want to see the warnings, then just remove the strict checks.
>>>> We already concluded that there is little value in them on s390 since it only
>>>> shows false postives.
>>>>
>>>> Maybe the easiest way would be to rename the option on s390 and move all
>>>> the other ones into a common place.
>>>
>>> Yes, feel free to do that.
>>
>> Can you put up the false positives somewhere? I don't have easy access
>> to an s390 toolchain to test build with and I'm interested to see how
>> bad the false positives are.
>>
>> I'm slightly concerned that we'll just have this problem again when
>> another arch comes along with false positives. But ignoring that issue
>> is probably fine. I'll respin with a patch to move s390 to something
>> like DEBUG_WARN_USER_COPY_CHECKS.
> 
> Sure:
> 
> In function 'copy_from_user',
>     inlined from 'write_enabled_file_bool' at kernel/kprobes.c:1973:
> /linux-2.6/arch/s390/include/asm/uaccess.h:297: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
> In function 'copy_from_user',
>     inlined from 'perf_copy_attr' at kernel/perf_event.c:4988,
>     inlined from 'SYSC_perf_event_open' at kernel/perf_event.c:5092,
>     inlined from 'SyS_perf_event_open' at kernel/perf_event.c:5077:
[snip]
>     inlined from 'compat_sys_socketcall' at net/compat.c:783:
> /linux-2.6/arch/s390/include/asm/uaccess.h:297: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
> 

Thanks. I'm a bit confused now since these files are compiled on x86 too
and I don't see any warnings on that architecture. Which compiler is wrong?

Anyway, tile has joined the strict copy from user checks arena and it's
acting like s390 by only enabling warnings when the option is set. Sigh....

I would really like to just merge all this code. How about a config
DEBUG_USER_COPY_CHECKS which just does warnings, and then a config
DEBUG_STRICT_USER_COPY_CHECKS that depends on DEBUG_USER_COPY_CHECKS
that upgrades the warnings to errors? This would allow us to merge most
of the code and still be mostly backwards compatible.

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH v2] ARM: uaccess: Implement strict user copy checks

[Stephen Boyd]

On 08/28/2010 12:43 AM, Heiko Carstens wrote:
> On Fri, Aug 27, 2010 at 06:35:16PM -0700, Stephen Boyd wrote:
>> On 08/25/2010 07:40 AM, Heiko Carstens wrote:
>>> On Wed, Aug 25, 2010 at 03:55:53PM +0200, Arnd Bergmann wrote:
>>>>
>>>> If you don't want to see the warnings, then just remove the strict checks.
>>>> We already concluded that there is little value in them on s390 since it only
>>>> shows false postives.
>>>>
>>>> Maybe the easiest way would be to rename the option on s390 and move all
>>>> the other ones into a common place.
>>>
>>> Yes, feel free to do that.
>>
>> Can you put up the false positives somewhere? I don't have easy access
>> to an s390 toolchain to test build with and I'm interested to see how
>> bad the false positives are.
>>
>> I'm slightly concerned that we'll just have this problem again when
>> another arch comes along with false positives. But ignoring that issue
>> is probably fine. I'll respin with a patch to move s390 to something
>> like DEBUG_WARN_USER_COPY_CHECKS.
> 
> Sure:
> 
> In function 'copy_from_user',
>     inlined from 'write_enabled_file_bool' at kernel/kprobes.c:1973:
> /linux-2.6/arch/s390/include/asm/uaccess.h:297: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
> In function 'copy_from_user',
>     inlined from 'perf_copy_attr' at kernel/perf_event.c:4988,
>     inlined from 'SYSC_perf_event_open' at kernel/perf_event.c:5092,
>     inlined from 'SyS_perf_event_open' at kernel/perf_event.c:5077:
[snip]
>     inlined from 'compat_sys_socketcall' at net/compat.c:783:
> /linux-2.6/arch/s390/include/asm/uaccess.h:297: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
> 

Thanks. I'm a bit confused now since these files are compiled on x86 too
and I don't see any warnings on that architecture. Which compiler is wrong?

Anyway, tile has joined the strict copy from user checks arena and it's
acting like s390 by only enabling warnings when the option is set. Sigh....

I would really like to just merge all this code. How about a config
DEBUG_USER_COPY_CHECKS which just does warnings, and then a config
DEBUG_STRICT_USER_COPY_CHECKS that depends on DEBUG_USER_COPY_CHECKS
that upgrades the warnings to errors? This would allow us to merge most
of the code and still be mostly backwards compatible.

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH v2] ARM: uaccess: Implement strict user copy checks

[Stephen Boyd]

On 09/03/2010 09:49 PM, Stephen Boyd wrote:
> On 08/28/2010 12:43 AM, Heiko Carstens wrote:
>> On Fri, Aug 27, 2010 at 06:35:16PM -0700, Stephen Boyd wrote:
>>> On 08/25/2010 07:40 AM, Heiko Carstens wrote:
>>>> On Wed, Aug 25, 2010 at 03:55:53PM +0200, Arnd Bergmann wrote:
>>>>>
>>>>> If you don't want to see the warnings, then just remove the strict checks.
>>>>> We already concluded that there is little value in them on s390 since it only
>>>>> shows false postives.
>>>>>
>>>>> Maybe the easiest way would be to rename the option on s390 and move all
>>>>> the other ones into a common place.
>>>>
>>>> Yes, feel free to do that.
>>>
>>> Can you put up the false positives somewhere? I don't have easy access
>>> to an s390 toolchain to test build with and I'm interested to see how
>>> bad the false positives are.
>>>
>>> I'm slightly concerned that we'll just have this problem again when
>>> another arch comes along with false positives. But ignoring that issue
>>> is probably fine. I'll respin with a patch to move s390 to something
>>> like DEBUG_WARN_USER_COPY_CHECKS.
>>
>> Sure:
>>
>> In function 'copy_from_user',
>>     inlined from 'write_enabled_file_bool' at kernel/kprobes.c:1973:
>> /linux-2.6/arch/s390/include/asm/uaccess.h:297: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
>> In function 'copy_from_user',
>>     inlined from 'perf_copy_attr' at kernel/perf_event.c:4988,
>>     inlined from 'SYSC_perf_event_open' at kernel/perf_event.c:5092,
>>     inlined from 'SyS_perf_event_open' at kernel/perf_event.c:5077:
> [snip]
>>     inlined from 'compat_sys_socketcall' at net/compat.c:783:
>> /linux-2.6/arch/s390/include/asm/uaccess.h:297: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
>>
> 
> Thanks. I'm a bit confused now since these files are compiled on x86 too
> and I don't see any warnings on that architecture. Which compiler is wrong?
> 
> Anyway, tile has joined the strict copy from user checks arena and it's
> acting like s390 by only enabling warnings when the option is set. Sigh....
> 
> I would really like to just merge all this code. How about a config
> DEBUG_USER_COPY_CHECKS which just does warnings, and then a config
> DEBUG_STRICT_USER_COPY_CHECKS that depends on DEBUG_USER_COPY_CHECKS
> that upgrades the warnings to errors? This would allow us to merge most
> of the code and still be mostly backwards compatible.
> 

Ok, I wasn't aware that only x86_32 has support for user copy checks. So
I hacked support for that on x86_64 and then saw the kprobes.c failure.
Great! Now for the weird(?) part.

Changing the buf_size variable from an int to a size_t makes the warning
go away. Perhaps this is because gcc can't reliably eliminate the else
case when the lower bound isn't 0? Overflow? I'm not really sure. Does
the kernel/kprobes.c part of this patch work for you?

--->8----8<----

diff --git a/arch/x86/include/asm/uaccess_64.h b/arch/x86/include/asm/uaccess_64.h
index 316708d..904684b 100644
--- a/arch/x86/include/asm/uaccess_64.h
+++ b/arch/x86/include/asm/uaccess_64.h
@@ -44,6 +44,14 @@ _copy_from_user(void *to, const void __user *from, unsigned len);
 __must_check unsigned long
 copy_in_user(void __user *to, const void __user *from, unsigned len);
 
+extern void copy_from_user_overflow(void)
+#ifdef CONFIG_DEBUG_STRICT_USER_COPY_CHECKS
+	__compiletime_error("copy_from_user() buffer size is not provably correct")
+#else
+	__compiletime_warning("copy_from_user() buffer size is not provably correct")
+#endif
+;
+
 static inline unsigned long __must_check copy_from_user(void *to,
 					  const void __user *from,
 					  unsigned long n)
@@ -53,10 +61,8 @@ static inline unsigned long __must_check copy_from_user(void *to,
 	might_fault();
 	if (likely(sz == -1 || sz >= n))
 		n = _copy_from_user(to, from, n);
-#ifdef CONFIG_DEBUG_VM
 	else
-		WARN(1, "Buffer overflow detected!\n");
-#endif
+		copy_from_user_overflow();
 	return n;
 }
 
diff --git a/arch/x86/lib/usercopy_64.c b/arch/x86/lib/usercopy_64.c
index b7c2849..d7a5d9a 100644
--- a/arch/x86/lib/usercopy_64.c
+++ b/arch/x86/lib/usercopy_64.c
@@ -181,3 +181,9 @@ copy_user_handle_tail(char *to, char *from, unsigned len, unsigned zerorest)
 			break;
 	return len;
 }
+
+void copy_from_user_overflow(void)
+{
+	WARN(1, "Buffer overflow detected!\n");
+}
+EXPORT_SYMBOL(copy_from_user_overflow);
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 282035f..fa4e0f9 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -1967,7 +1967,7 @@ static ssize_t write_enabled_file_bool(struct file *file,
 	       const char __user *user_buf, size_t count, loff_t *ppos)
 {
 	char buf[32];
-	int buf_size;
+	size_t buf_size;
 
 	buf_size = min(count, (sizeof(buf)-1));
 	if (copy_from_user(buf, user_buf, buf_size))


-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH v2] ARM: uaccess: Implement strict user copy checks

[Stephen Boyd]

On 09/03/2010 09:49 PM, Stephen Boyd wrote:
> On 08/28/2010 12:43 AM, Heiko Carstens wrote:
>> On Fri, Aug 27, 2010 at 06:35:16PM -0700, Stephen Boyd wrote:
>>> On 08/25/2010 07:40 AM, Heiko Carstens wrote:
>>>> On Wed, Aug 25, 2010 at 03:55:53PM +0200, Arnd Bergmann wrote:
>>>>>
>>>>> If you don't want to see the warnings, then just remove the strict checks.
>>>>> We already concluded that there is little value in them on s390 since it only
>>>>> shows false postives.
>>>>>
>>>>> Maybe the easiest way would be to rename the option on s390 and move all
>>>>> the other ones into a common place.
>>>>
>>>> Yes, feel free to do that.
>>>
>>> Can you put up the false positives somewhere? I don't have easy access
>>> to an s390 toolchain to test build with and I'm interested to see how
>>> bad the false positives are.
>>>
>>> I'm slightly concerned that we'll just have this problem again when
>>> another arch comes along with false positives. But ignoring that issue
>>> is probably fine. I'll respin with a patch to move s390 to something
>>> like DEBUG_WARN_USER_COPY_CHECKS.
>>
>> Sure:
>>
>> In function 'copy_from_user',
>>     inlined from 'write_enabled_file_bool' at kernel/kprobes.c:1973:
>> /linux-2.6/arch/s390/include/asm/uaccess.h:297: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
>> In function 'copy_from_user',
>>     inlined from 'perf_copy_attr' at kernel/perf_event.c:4988,
>>     inlined from 'SYSC_perf_event_open' at kernel/perf_event.c:5092,
>>     inlined from 'SyS_perf_event_open' at kernel/perf_event.c:5077:
> [snip]
>>     inlined from 'compat_sys_socketcall' at net/compat.c:783:
>> /linux-2.6/arch/s390/include/asm/uaccess.h:297: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
>>
> 
> Thanks. I'm a bit confused now since these files are compiled on x86 too
> and I don't see any warnings on that architecture. Which compiler is wrong?
> 
> Anyway, tile has joined the strict copy from user checks arena and it's
> acting like s390 by only enabling warnings when the option is set. Sigh....
> 
> I would really like to just merge all this code. How about a config
> DEBUG_USER_COPY_CHECKS which just does warnings, and then a config
> DEBUG_STRICT_USER_COPY_CHECKS that depends on DEBUG_USER_COPY_CHECKS
> that upgrades the warnings to errors? This would allow us to merge most
> of the code and still be mostly backwards compatible.
> 

Ok, I wasn't aware that only x86_32 has support for user copy checks. So
I hacked support for that on x86_64 and then saw the kprobes.c failure.
Great! Now for the weird(?) part.

Changing the buf_size variable from an int to a size_t makes the warning
go away. Perhaps this is because gcc can't reliably eliminate the else
case when the lower bound isn't 0? Overflow? I'm not really sure. Does
the kernel/kprobes.c part of this patch work for you?

--->8----8<----

diff --git a/arch/x86/include/asm/uaccess_64.h b/arch/x86/include/asm/uaccess_64.h
index 316708d..904684b 100644
--- a/arch/x86/include/asm/uaccess_64.h
+++ b/arch/x86/include/asm/uaccess_64.h
@@ -44,6 +44,14 @@ _copy_from_user(void *to, const void __user *from, unsigned len);
 __must_check unsigned long
 copy_in_user(void __user *to, const void __user *from, unsigned len);
 
+extern void copy_from_user_overflow(void)
+#ifdef CONFIG_DEBUG_STRICT_USER_COPY_CHECKS
+	__compiletime_error("copy_from_user() buffer size is not provably correct")
+#else
+	__compiletime_warning("copy_from_user() buffer size is not provably correct")
+#endif
+;
+
 static inline unsigned long __must_check copy_from_user(void *to,
 					  const void __user *from,
 					  unsigned long n)
@@ -53,10 +61,8 @@ static inline unsigned long __must_check copy_from_user(void *to,
 	might_fault();
 	if (likely(sz == -1 || sz >= n))
 		n = _copy_from_user(to, from, n);
-#ifdef CONFIG_DEBUG_VM
 	else
-		WARN(1, "Buffer overflow detected!\n");
-#endif
+		copy_from_user_overflow();
 	return n;
 }
 
diff --git a/arch/x86/lib/usercopy_64.c b/arch/x86/lib/usercopy_64.c
index b7c2849..d7a5d9a 100644
--- a/arch/x86/lib/usercopy_64.c
+++ b/arch/x86/lib/usercopy_64.c
@@ -181,3 +181,9 @@ copy_user_handle_tail(char *to, char *from, unsigned len, unsigned zerorest)
 			break;
 	return len;
 }
+
+void copy_from_user_overflow(void)
+{
+	WARN(1, "Buffer overflow detected!\n");
+}
+EXPORT_SYMBOL(copy_from_user_overflow);
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 282035f..fa4e0f9 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -1967,7 +1967,7 @@ static ssize_t write_enabled_file_bool(struct file *file,
 	       const char __user *user_buf, size_t count, loff_t *ppos)
 {
 	char buf[32];
-	int buf_size;
+	size_t buf_size;
 
 	buf_size = min(count, (sizeof(buf)-1));
 	if (copy_from_user(buf, user_buf, buf_size))


-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH v2] ARM: uaccess: Implement strict user copy checks

[Heiko Carstens]

On Mon, Sep 13, 2010 at 08:07:52PM -0700, Stephen Boyd wrote:
> Ok, I wasn't aware that only x86_32 has support for user copy checks. So
> I hacked support for that on x86_64 and then saw the kprobes.c failure.
> Great! Now for the weird(?) part.
> 
> Changing the buf_size variable from an int to a size_t makes the warning
> go away. Perhaps this is because gcc can't reliably eliminate the else
> case when the lower bound isn't 0? Overflow? I'm not really sure. Does
> the kernel/kprobes.c part of this patch work for you?
> 
[...]
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index 282035f..fa4e0f9 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -1967,7 +1967,7 @@ static ssize_t write_enabled_file_bool(struct file *file,
>  	       const char __user *user_buf, size_t count, loff_t *ppos)
>  {
>  	char buf[32];
> -	int buf_size;
> +	size_t buf_size;
> 
>  	buf_size = min(count, (sizeof(buf)-1));
>  	if (copy_from_user(buf, user_buf, buf_size))

Yes, the warning goes away on s390 as well.

[PATCH v2] ARM: uaccess: Implement strict user copy checks

[Heiko Carstens]

On Mon, Sep 13, 2010 at 08:07:52PM -0700, Stephen Boyd wrote:
> Ok, I wasn't aware that only x86_32 has support for user copy checks. So
> I hacked support for that on x86_64 and then saw the kprobes.c failure.
> Great! Now for the weird(?) part.
> 
> Changing the buf_size variable from an int to a size_t makes the warning
> go away. Perhaps this is because gcc can't reliably eliminate the else
> case when the lower bound isn't 0? Overflow? I'm not really sure. Does
> the kernel/kprobes.c part of this patch work for you?
> 
[...]
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index 282035f..fa4e0f9 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -1967,7 +1967,7 @@ static ssize_t write_enabled_file_bool(struct file *file,
>  	       const char __user *user_buf, size_t count, loff_t *ppos)
>  {
>  	char buf[32];
> -	int buf_size;
> +	size_t buf_size;
> 
>  	buf_size = min(count, (sizeof(buf)-1));
>  	if (copy_from_user(buf, user_buf, buf_size))

Yes, the warning goes away on s390 as well.

Re: [PATCH v2] ARM: uaccess: Implement strict user copy checks

[Arnd Bergmann]

On Tuesday 14 September 2010, Heiko Carstens wrote:
> On Mon, Sep 13, 2010 at 08:07:52PM -0700, Stephen Boyd wrote:
> > Changing the buf_size variable from an int to a size_t makes the warning
> > go away. Perhaps this is because gcc can't reliably eliminate the else
> > case when the lower bound isn't 0? Overflow? I'm not really sure. Does
> > the kernel/kprobes.c part of this patch work for you?
> 
> Yes, the warning goes away on s390 as well.

Ok, great!

In that case, I think we should just apply this patch to fix all these
warnings for good. There are probably some more in an x86_64 allyesconfig
build, but this should make s390 build cleanly again.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 55f3a3e..0947e16 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -1185,7 +1185,7 @@ static int set_offload(struct net_device *dev, unsigned long arg)
 }
 
 static long __tun_chr_ioctl(struct file *file, unsigned int cmd,
-			    unsigned long arg, int ifreq_len)
+			    unsigned long arg, size_t ifreq_len)
 {
 	struct tun_file *tfile = file->private_data;
 	struct tun_struct *tun;
diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index 13776a7..6fff2f2 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -2388,7 +2388,7 @@ static ssize_t
 sg_proc_write_dressz(struct file *filp, const char __user *buffer,
 		     size_t count, loff_t *off)
 {
-	int num;
+	size_t num;
 	unsigned long k = ULONG_MAX;
 	char buff[11];
 
diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
index 1b4db2c..c085561 100644
--- a/fs/debugfs/file.c
+++ b/fs/debugfs/file.c
@@ -428,7 +428,7 @@ static ssize_t write_file_bool(struct file *file, const char __user *user_buf,
 			       size_t count, loff_t *ppos)
 {
 	char buf[32];
-	int buf_size;
+	size_t buf_size;
 	u32 *val = file->private_data;
 
 	buf_size = min(count, (sizeof(buf)-1));
diff --git a/net/compat.c b/net/compat.c
index 63d260e..4cc0ba9 100644
--- a/net/compat.c
+++ b/net/compat.c
@@ -774,7 +774,7 @@ asmlinkage long compat_sys_recvmmsg(int fd, struct compat_mmsghdr __user *mmsg,
 
 asmlinkage long compat_sys_socketcall(int call, u32 __user *args)
 {
-	int ret;
+	size_t ret;
 	u32 a[6];
 	u32 a0, a1;
 
diff --git a/net/core/pktgen.c b/net/core/pktgen.c
index 10a1ea7..d9ee8b9 100644
--- a/net/core/pktgen.c
+++ b/net/core/pktgen.c
@@ -848,7 +848,8 @@ static ssize_t pktgen_if_write(struct file *file,
 {
 	struct seq_file *seq = file->private_data;
 	struct pktgen_dev *pkt_dev = seq->private;
-	int i = 0, max, len;
+	int i = 0, max;
+	size_t len;
 	char name[16], valstr[32];
 	unsigned long value = 0;
 	char *pg_result = NULL;
diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index 9a17f28..fc1a4e0 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -2062,7 +2062,7 @@ packet_setsockopt(struct socket *sock, int level, int optname, char __user *optv
 static int packet_getsockopt(struct socket *sock, int level, int optname,
 			     char __user *optval, int __user *optlen)
 {
-	int len;
+	size_t len;
 	int val;
 	struct sock *sk = sock->sk;
 	struct packet_sock *po = pkt_sk(sk);

[PATCH v2] ARM: uaccess: Implement strict user copy checks

[Arnd Bergmann]

On Tuesday 14 September 2010, Heiko Carstens wrote:
> On Mon, Sep 13, 2010 at 08:07:52PM -0700, Stephen Boyd wrote:
> > Changing the buf_size variable from an int to a size_t makes the warning
> > go away. Perhaps this is because gcc can't reliably eliminate the else
> > case when the lower bound isn't 0? Overflow? I'm not really sure. Does
> > the kernel/kprobes.c part of this patch work for you?
> 
> Yes, the warning goes away on s390 as well.

Ok, great!

In that case, I think we should just apply this patch to fix all these
warnings for good. There are probably some more in an x86_64 allyesconfig
build, but this should make s390 build cleanly again.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 55f3a3e..0947e16 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -1185,7 +1185,7 @@ static int set_offload(struct net_device *dev, unsigned long arg)
 }
 
 static long __tun_chr_ioctl(struct file *file, unsigned int cmd,
-			    unsigned long arg, int ifreq_len)
+			    unsigned long arg, size_t ifreq_len)
 {
 	struct tun_file *tfile = file->private_data;
 	struct tun_struct *tun;
diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index 13776a7..6fff2f2 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -2388,7 +2388,7 @@ static ssize_t
 sg_proc_write_dressz(struct file *filp, const char __user *buffer,
 		     size_t count, loff_t *off)
 {
-	int num;
+	size_t num;
 	unsigned long k = ULONG_MAX;
 	char buff[11];
 
diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
index 1b4db2c..c085561 100644
--- a/fs/debugfs/file.c
+++ b/fs/debugfs/file.c
@@ -428,7 +428,7 @@ static ssize_t write_file_bool(struct file *file, const char __user *user_buf,
 			       size_t count, loff_t *ppos)
 {
 	char buf[32];
-	int buf_size;
+	size_t buf_size;
 	u32 *val = file->private_data;
 
 	buf_size = min(count, (sizeof(buf)-1));
diff --git a/net/compat.c b/net/compat.c
index 63d260e..4cc0ba9 100644
--- a/net/compat.c
+++ b/net/compat.c
@@ -774,7 +774,7 @@ asmlinkage long compat_sys_recvmmsg(int fd, struct compat_mmsghdr __user *mmsg,
 
 asmlinkage long compat_sys_socketcall(int call, u32 __user *args)
 {
-	int ret;
+	size_t ret;
 	u32 a[6];
 	u32 a0, a1;
 
diff --git a/net/core/pktgen.c b/net/core/pktgen.c
index 10a1ea7..d9ee8b9 100644
--- a/net/core/pktgen.c
+++ b/net/core/pktgen.c
@@ -848,7 +848,8 @@ static ssize_t pktgen_if_write(struct file *file,
 {
 	struct seq_file *seq = file->private_data;
 	struct pktgen_dev *pkt_dev = seq->private;
-	int i = 0, max, len;
+	int i = 0, max;
+	size_t len;
 	char name[16], valstr[32];
 	unsigned long value = 0;
 	char *pg_result = NULL;
diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index 9a17f28..fc1a4e0 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -2062,7 +2062,7 @@ packet_setsockopt(struct socket *sock, int level, int optname, char __user *optv
 static int packet_getsockopt(struct socket *sock, int level, int optname,
 			     char __user *optval, int __user *optlen)
 {
-	int len;
+	size_t len;
 	int val;
 	struct sock *sk = sock->sk;
 	struct packet_sock *po = pkt_sk(sk);

Re: [PATCH v2] ARM: uaccess: Implement strict user copy checks

[Heiko Carstens]

On Tue, Sep 14, 2010 at 03:10:43PM +0200, Arnd Bergmann wrote:
> On Tuesday 14 September 2010, Heiko Carstens wrote:
> > On Mon, Sep 13, 2010 at 08:07:52PM -0700, Stephen Boyd wrote:
> > > Changing the buf_size variable from an int to a size_t makes the warning
> > > go away. Perhaps this is because gcc can't reliably eliminate the else
> > > case when the lower bound isn't 0? Overflow? I'm not really sure. Does
> > > the kernel/kprobes.c part of this patch work for you?
> > 
> > Yes, the warning goes away on s390 as well.
> 
> Ok, great!
> 
> In that case, I think we should just apply this patch to fix all these
> warnings for good. There are probably some more in an x86_64 allyesconfig
> build, but this should make s390 build cleanly again.

Nah, that would be too easy.
allyesconfig with gcc 4.5.2 and both patches applied:

In file included from /home2/heicarst/linux-2.6/arch/s390/include/asm/mmu_context.h:13:0,
                 from /home2/heicarst/linux-2.6/arch/s390/include/asm/elf.h:133,
                 from include/linux/elf.h:7,
                 from include/linux/module.h:14,
                 from net/core/pktgen.c:123:
In function 'copy_from_user',
    inlined from 'pktgen_if_write' at net/core/pktgen.c:881:20:
/home2/heicarst/linux-2.6/arch/s390/include/asm/uaccess.h:297:26: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
In function 'copy_from_user',
    inlined from 'pktgen_if_write' at net/core/pktgen.c:1144:21:
/home2/heicarst/linux-2.6/arch/s390/include/asm/uaccess.h:297:26: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
In function 'copy_from_user',
    inlined from 'pktgen_if_write' at net/core/pktgen.c:1251:21:
/home2/heicarst/linux-2.6/arch/s390/include/asm/uaccess.h:297:26: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
In function 'copy_from_user',
    inlined from 'pktgen_if_write' at net/core/pktgen.c:1273:21:
/home2/heicarst/linux-2.6/arch/s390/include/asm/uaccess.h:297:26: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
In function 'copy_from_user',
    inlined from 'pktgen_if_write' at net/core/pktgen.c:1297:21:
/home2/heicarst/linux-2.6/arch/s390/include/asm/uaccess.h:297:26: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
In function 'copy_from_user',
    inlined from 'pktgen_if_write' at net/core/pktgen.c:1320:21:
/home2/heicarst/linux-2.6/arch/s390/include/asm/uaccess.h:297:26: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
In function 'copy_from_user',
    inlined from 'pktgen_if_write' at net/core/pktgen.c:1343:21:
/home2/heicarst/linux-2.6/arch/s390/include/asm/uaccess.h:297:26: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
In function 'copy_from_user',
    inlined from 'pktgen_if_write' at net/core/pktgen.c:1364:21:
/home2/heicarst/linux-2.6/arch/s390/include/asm/uaccess.h:297:26: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
In function 'copy_from_user',
    inlined from 'pktgen_if_write' at net/core/pktgen.c:1385:21:
/home2/heicarst/linux-2.6/arch/s390/include/asm/uaccess.h:297:26: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
In function 'copy_from_user',
    inlined from 'pktgen_if_write' at net/core/pktgen.c:1406:21:
/home2/heicarst/linux-2.6/arch/s390/include/asm/uaccess.h:297:26: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
In function 'copy_from_user',
    inlined from 'pktgen_if_write' at net/core/pktgen.c:1433:21:
/home2/heicarst/linux-2.6/arch/s390/include/asm/uaccess.h:297:26: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
In function 'copy_from_user',
    inlined from 'pktgen_if_write' at net/core/pktgen.c:1469:21:
/home2/heicarst/linux-2.6/arch/s390/include/asm/uaccess.h:297:26: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
In file included from /home2/heicarst/linux-2.6/arch/s390/include/asm/mmu_context.h:13:0,
                 from /home2/heicarst/linux-2.6/arch/s390/include/asm/elf.h:133,
                 from include/linux/elf.h:7,
                 from include/linux/module.h:14,
                 from net/core/pktgen.c:123:
In function 'copy_from_user',
    inlined from 'pktgen_thread_write' at net/core/pktgen.c:1793:20:
/home2/heicarst/linux-2.6/arch/s390/include/asm/uaccess.h:297:26: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
In function 'copy_from_user',
    inlined from 'pktgen_thread_write' at net/core/pktgen.c:1824:21:
/home2/heicarst/linux-2.6/arch/s390/include/asm/uaccess.h:297:26: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
In file included from /home2/heicarst/linux-2.6/arch/s390/include/asm/mmu_context.h:13:0,
                 from /home2/heicarst/linux-2.6/arch/s390/include/asm/elf.h:133,
                 from include/linux/elf.h:7,
                 from include/linux/module.h:14,
                 from drivers/net/tun.c:42:
In function 'copy_from_user',
    inlined from '__tun_chr_ioctl' at drivers/net/tun.c:1200:21:
/home2/heicarst/linux-2.6/arch/s390/include/asm/uaccess.h:297:26: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
In file included from /home2/heicarst/linux-2.6/arch/s390/include/asm/mmu_context.h:13:0,
                 from /home2/heicarst/linux-2.6/arch/s390/include/asm/elf.h:133,
                 from include/linux/elf.h:7,
                 from include/linux/module.h:14,
                 from include/linux/sysdev.h:25,
                 from include/linux/cpu.h:22,
                 from kernel/perf_event.c:14:
In function 'copy_from_user',
    inlined from 'perf_copy_attr' at kernel/perf_event.c:5006:22,
    inlined from 'SYSC_perf_event_open' at kernel/perf_event.c:5110:6,
    inlined from 'SyS_perf_event_open' at kernel/perf_event.c:5095:1:
/home2/heicarst/linux-2.6/arch/s390/include/asm/uaccess.h:297:26: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
In file included from /home2/heicarst/linux-2.6/arch/s390/include/asm/mmu_context.h:13:0,
                 from /home2/heicarst/linux-2.6/arch/s390/include/asm/elf.h:133,
                 from include/linux/elf.h:7,
                 from include/linux/module.h:14,
                 from drivers/scsi/sg.c:31:
In function 'copy_from_user',
    inlined from 'sg_proc_write_adio' at drivers/scsi/sg.c:2373:20:
/home2/heicarst/linux-2.6/arch/s390/include/asm/uaccess.h:297:26: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
In file included from /home2/heicarst/linux-2.6/arch/s390/include/asm/mmu_context.h:13:0,
                 from /home2/heicarst/linux-2.6/arch/s390/include/asm/elf.h:133,
                 from include/linux/elf.h:7,
                 from include/linux/module.h:14,
                 from include/linux/textsearch.h:7,
                 from include/linux/skbuff.h:27,
                 from include/linux/icmpv6.h:82,
                 from net/compat.c:19:
In function 'copy_from_user',
    inlined from 'compat_sys_socketcall' at net/compat.c:783:20:
/home2/heicarst/linux-2.6/arch/s390/include/asm/uaccess.h:297:26: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct

[PATCH v2] ARM: uaccess: Implement strict user copy checks

[Heiko Carstens]

On Tue, Sep 14, 2010 at 03:10:43PM +0200, Arnd Bergmann wrote:
> On Tuesday 14 September 2010, Heiko Carstens wrote:
> > On Mon, Sep 13, 2010 at 08:07:52PM -0700, Stephen Boyd wrote:
> > > Changing the buf_size variable from an int to a size_t makes the warning
> > > go away. Perhaps this is because gcc can't reliably eliminate the else
> > > case when the lower bound isn't 0? Overflow? I'm not really sure. Does
> > > the kernel/kprobes.c part of this patch work for you?
> > 
> > Yes, the warning goes away on s390 as well.
> 
> Ok, great!
> 
> In that case, I think we should just apply this patch to fix all these
> warnings for good. There are probably some more in an x86_64 allyesconfig
> build, but this should make s390 build cleanly again.

Nah, that would be too easy.
allyesconfig with gcc 4.5.2 and both patches applied:

In file included from /home2/heicarst/linux-2.6/arch/s390/include/asm/mmu_context.h:13:0,
                 from /home2/heicarst/linux-2.6/arch/s390/include/asm/elf.h:133,
                 from include/linux/elf.h:7,
                 from include/linux/module.h:14,
                 from net/core/pktgen.c:123:
In function 'copy_from_user',
    inlined from 'pktgen_if_write' at net/core/pktgen.c:881:20:
/home2/heicarst/linux-2.6/arch/s390/include/asm/uaccess.h:297:26: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
In function 'copy_from_user',
    inlined from 'pktgen_if_write' at net/core/pktgen.c:1144:21:
/home2/heicarst/linux-2.6/arch/s390/include/asm/uaccess.h:297:26: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
In function 'copy_from_user',
    inlined from 'pktgen_if_write' at net/core/pktgen.c:1251:21:
/home2/heicarst/linux-2.6/arch/s390/include/asm/uaccess.h:297:26: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
In function 'copy_from_user',
    inlined from 'pktgen_if_write' at net/core/pktgen.c:1273:21:
/home2/heicarst/linux-2.6/arch/s390/include/asm/uaccess.h:297:26: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
In function 'copy_from_user',
    inlined from 'pktgen_if_write' at net/core/pktgen.c:1297:21:
/home2/heicarst/linux-2.6/arch/s390/include/asm/uaccess.h:297:26: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
In function 'copy_from_user',
    inlined from 'pktgen_if_write' at net/core/pktgen.c:1320:21:
/home2/heicarst/linux-2.6/arch/s390/include/asm/uaccess.h:297:26: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
In function 'copy_from_user',
    inlined from 'pktgen_if_write' at net/core/pktgen.c:1343:21:
/home2/heicarst/linux-2.6/arch/s390/include/asm/uaccess.h:297:26: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
In function 'copy_from_user',
    inlined from 'pktgen_if_write' at net/core/pktgen.c:1364:21:
/home2/heicarst/linux-2.6/arch/s390/include/asm/uaccess.h:297:26: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
In function 'copy_from_user',
    inlined from 'pktgen_if_write' at net/core/pktgen.c:1385:21:
/home2/heicarst/linux-2.6/arch/s390/include/asm/uaccess.h:297:26: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
In function 'copy_from_user',
    inlined from 'pktgen_if_write' at net/core/pktgen.c:1406:21:
/home2/heicarst/linux-2.6/arch/s390/include/asm/uaccess.h:297:26: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
In function 'copy_from_user',
    inlined from 'pktgen_if_write' at net/core/pktgen.c:1433:21:
/home2/heicarst/linux-2.6/arch/s390/include/asm/uaccess.h:297:26: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
In function 'copy_from_user',
    inlined from 'pktgen_if_write' at net/core/pktgen.c:1469:21:
/home2/heicarst/linux-2.6/arch/s390/include/asm/uaccess.h:297:26: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
In file included from /home2/heicarst/linux-2.6/arch/s390/include/asm/mmu_context.h:13:0,
                 from /home2/heicarst/linux-2.6/arch/s390/include/asm/elf.h:133,
                 from include/linux/elf.h:7,
                 from include/linux/module.h:14,
                 from net/core/pktgen.c:123:
In function 'copy_from_user',
    inlined from 'pktgen_thread_write' at net/core/pktgen.c:1793:20:
/home2/heicarst/linux-2.6/arch/s390/include/asm/uaccess.h:297:26: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
In function 'copy_from_user',
    inlined from 'pktgen_thread_write' at net/core/pktgen.c:1824:21:
/home2/heicarst/linux-2.6/arch/s390/include/asm/uaccess.h:297:26: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
In file included from /home2/heicarst/linux-2.6/arch/s390/include/asm/mmu_context.h:13:0,
                 from /home2/heicarst/linux-2.6/arch/s390/include/asm/elf.h:133,
                 from include/linux/elf.h:7,
                 from include/linux/module.h:14,
                 from drivers/net/tun.c:42:
In function 'copy_from_user',
    inlined from '__tun_chr_ioctl' at drivers/net/tun.c:1200:21:
/home2/heicarst/linux-2.6/arch/s390/include/asm/uaccess.h:297:26: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
In file included from /home2/heicarst/linux-2.6/arch/s390/include/asm/mmu_context.h:13:0,
                 from /home2/heicarst/linux-2.6/arch/s390/include/asm/elf.h:133,
                 from include/linux/elf.h:7,
                 from include/linux/module.h:14,
                 from include/linux/sysdev.h:25,
                 from include/linux/cpu.h:22,
                 from kernel/perf_event.c:14:
In function 'copy_from_user',
    inlined from 'perf_copy_attr' at kernel/perf_event.c:5006:22,
    inlined from 'SYSC_perf_event_open' at kernel/perf_event.c:5110:6,
    inlined from 'SyS_perf_event_open' at kernel/perf_event.c:5095:1:
/home2/heicarst/linux-2.6/arch/s390/include/asm/uaccess.h:297:26: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
In file included from /home2/heicarst/linux-2.6/arch/s390/include/asm/mmu_context.h:13:0,
                 from /home2/heicarst/linux-2.6/arch/s390/include/asm/elf.h:133,
                 from include/linux/elf.h:7,
                 from include/linux/module.h:14,
                 from drivers/scsi/sg.c:31:
In function 'copy_from_user',
    inlined from 'sg_proc_write_adio' at drivers/scsi/sg.c:2373:20:
/home2/heicarst/linux-2.6/arch/s390/include/asm/uaccess.h:297:26: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
In file included from /home2/heicarst/linux-2.6/arch/s390/include/asm/mmu_context.h:13:0,
                 from /home2/heicarst/linux-2.6/arch/s390/include/asm/elf.h:133,
                 from include/linux/elf.h:7,
                 from include/linux/module.h:14,
                 from include/linux/textsearch.h:7,
                 from include/linux/skbuff.h:27,
                 from include/linux/icmpv6.h:82,
                 from net/compat.c:19:
In function 'copy_from_user',
    inlined from 'compat_sys_socketcall' at net/compat.c:783:20:
/home2/heicarst/linux-2.6/arch/s390/include/asm/uaccess.h:297:26: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct

[PATCHv2 2/1] Consolidate CONFIG_DEBUG_STRICT_USER_COPY_CHECKS

[Stephen Boyd]

The help text for this config is duplicated across the x86,
parisc, s390, and arm 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.

Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: x86@kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-s390@vger.kernel.org
Cc: linux-parisc@vger.kernel.org
Cc: Arjan van de Ven <arjan@linux.intel.com>
Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
Cc: Helge Deller <deller@gmx.de>
---

This depends on a patch sent to the arm mailing list adding
CONFIG_DEBUG_STRICT_USER_COPY_CHECKS to ARM.

LKML: http://lkml.org/lkml/2010/8/17/535

 arch/arm/Kconfig.debug    |   15 ++-------------
 arch/parisc/Kconfig.debug |   15 ++-------------
 arch/s390/Kconfig.debug   |   14 ++------------
 arch/x86/Kconfig.debug    |   15 ++-------------
 lib/Kconfig.debug         |   16 ++++++++++++++++
 5 files changed, 24 insertions(+), 51 deletions(-)

diff --git a/arch/arm/Kconfig.debug b/arch/arm/Kconfig.debug
index 64e33b8..326c7f1 100644
--- a/arch/arm/Kconfig.debug
+++ b/arch/arm/Kconfig.debug
@@ -128,18 +128,7 @@ config DEBUG_S3C_UART
 	  The uncompressor code port configuration is now handled
 	  by CONFIG_S3C_LOWLEVEL_UART_PORT.
 
-config DEBUG_STRICT_USER_COPY_CHECKS
-	bool "Strict user 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 errors.
-
-	  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 ARCH_HAS_DEBUG_STRICT_USER_COPY_CHECKS
+	def_bool y
 
 endmenu
diff --git a/arch/parisc/Kconfig.debug b/arch/parisc/Kconfig.debug
index 7305ac8..b13e8d0 100644
--- a/arch/parisc/Kconfig.debug
+++ b/arch/parisc/Kconfig.debug
@@ -12,18 +12,7 @@ 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.
+config ARCH_HAS_DEBUG_STRICT_USER_COPY_CHECKS
+	def_bool y
 
 endmenu
diff --git a/arch/s390/Kconfig.debug b/arch/s390/Kconfig.debug
index 45e0c61..6df8e30 100644
--- a/arch/s390/Kconfig.debug
+++ b/arch/s390/Kconfig.debug
@@ -6,17 +6,7 @@ config TRACE_IRQFLAGS_SUPPORT
 
 source "lib/Kconfig.debug"
 
-config DEBUG_STRICT_USER_COPY_CHECKS
-	bool "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 ARCH_HAS_DEBUG_STRICT_USER_COPY_CHECKS
+	def_bool y
 
 endmenu
diff --git a/arch/x86/Kconfig.debug b/arch/x86/Kconfig.debug
index 7508508..d24afa3 100644
--- a/arch/x86/Kconfig.debug
+++ b/arch/x86/Kconfig.debug
@@ -285,18 +285,7 @@ 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 ARCH_HAS_DEBUG_STRICT_USER_COPY_CHECKS
+	def_bool y
 
 endmenu
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 9e06b7f..bbb1ac5 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1013,6 +1013,22 @@ config SYSCTL_SYSCALL_CHECK
 	  to properly maintain and use. This enables checks that help
 	  you to keep things correct.
 
+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 warnings/errors.
+
+	  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 where this option
+	  is a no-op, say N.
+
 source mm/Kconfig.debug
 source kernel/trace/Kconfig
 
-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCHv2 2/1] Consolidate CONFIG_DEBUG_STRICT_USER_COPY_CHECKS

[Stephen Boyd]

The help text for this config is duplicated across the x86,
parisc, s390, and arm 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.

Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: x86 at kernel.org
Cc: linux-arm-kernel at lists.infradead.org
Cc: linux-s390 at vger.kernel.org
Cc: linux-parisc at vger.kernel.org
Cc: Arjan van de Ven <arjan@linux.intel.com>
Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
Cc: Helge Deller <deller@gmx.de>
---

This depends on a patch sent to the arm mailing list adding
CONFIG_DEBUG_STRICT_USER_COPY_CHECKS to ARM.

LKML: http://lkml.org/lkml/2010/8/17/535

 arch/arm/Kconfig.debug    |   15 ++-------------
 arch/parisc/Kconfig.debug |   15 ++-------------
 arch/s390/Kconfig.debug   |   14 ++------------
 arch/x86/Kconfig.debug    |   15 ++-------------
 lib/Kconfig.debug         |   16 ++++++++++++++++
 5 files changed, 24 insertions(+), 51 deletions(-)

diff --git a/arch/arm/Kconfig.debug b/arch/arm/Kconfig.debug
index 64e33b8..326c7f1 100644
--- a/arch/arm/Kconfig.debug
+++ b/arch/arm/Kconfig.debug
@@ -128,18 +128,7 @@ config DEBUG_S3C_UART
 	  The uncompressor code port configuration is now handled
 	  by CONFIG_S3C_LOWLEVEL_UART_PORT.
 
-config DEBUG_STRICT_USER_COPY_CHECKS
-	bool "Strict user 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 errors.
-
-	  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 ARCH_HAS_DEBUG_STRICT_USER_COPY_CHECKS
+	def_bool y
 
 endmenu
diff --git a/arch/parisc/Kconfig.debug b/arch/parisc/Kconfig.debug
index 7305ac8..b13e8d0 100644
--- a/arch/parisc/Kconfig.debug
+++ b/arch/parisc/Kconfig.debug
@@ -12,18 +12,7 @@ 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.
+config ARCH_HAS_DEBUG_STRICT_USER_COPY_CHECKS
+	def_bool y
 
 endmenu
diff --git a/arch/s390/Kconfig.debug b/arch/s390/Kconfig.debug
index 45e0c61..6df8e30 100644
--- a/arch/s390/Kconfig.debug
+++ b/arch/s390/Kconfig.debug
@@ -6,17 +6,7 @@ config TRACE_IRQFLAGS_SUPPORT
 
 source "lib/Kconfig.debug"
 
-config DEBUG_STRICT_USER_COPY_CHECKS
-	bool "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 ARCH_HAS_DEBUG_STRICT_USER_COPY_CHECKS
+	def_bool y
 
 endmenu
diff --git a/arch/x86/Kconfig.debug b/arch/x86/Kconfig.debug
index 7508508..d24afa3 100644
--- a/arch/x86/Kconfig.debug
+++ b/arch/x86/Kconfig.debug
@@ -285,18 +285,7 @@ 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 ARCH_HAS_DEBUG_STRICT_USER_COPY_CHECKS
+	def_bool y
 
 endmenu
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 9e06b7f..bbb1ac5 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1013,6 +1013,22 @@ config SYSCTL_SYSCALL_CHECK
 	  to properly maintain and use. This enables checks that help
 	  you to keep things correct.
 
+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 warnings/errors.
+
+	  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 where this option
+	  is a no-op, say N.
+
 source mm/Kconfig.debug
 source kernel/trace/Kconfig
 
-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCHv2 2/1] Consolidate CONFIG_DEBUG_STRICT_USER_COPY_CHECKS

[Arjan van de Ven]

On 8/18/2010 7:28 PM, Stephen Boyd wrote:
> The help text for this config is duplicated across the x86,
> parisc, s390, and arm 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.
>    


Acked-by: Arjan van de Ven <arjan@linux.intel.com>

[PATCHv2 2/1] Consolidate CONFIG_DEBUG_STRICT_USER_COPY_CHECKS

[Arjan van de Ven]

On 8/18/2010 7:28 PM, Stephen Boyd wrote:
> The help text for this config is duplicated across the x86,
> parisc, s390, and arm 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.
>    


Acked-by: Arjan van de Ven <arjan@linux.intel.com>

Re: [PATCHv2 2/1] Consolidate CONFIG_DEBUG_STRICT_USER_COPY_CHECKS

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 404 bytes --]

On Wed, 18 Aug 2010 19:28:18 -0700 Stephen Boyd <sboyd@codeaurora.org> wrote:
>
> +config ARCH_HAS_DEBUG_STRICT_USER_COPY_CHECKS
> +	def_bool y

Why not just have

config ARCH_HAS_DEBUG_STRICT_USER_COPY_CHECKS
	bool

in lib/Kconfig.debug

and select that in each arch that want it?

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 490 bytes --]

[PATCHv2 2/1] Consolidate CONFIG_DEBUG_STRICT_USER_COPY_CHECKS

[Stephen Rothwell]

On Wed, 18 Aug 2010 19:28:18 -0700 Stephen Boyd <sboyd@codeaurora.org> wrote:
>
> +config ARCH_HAS_DEBUG_STRICT_USER_COPY_CHECKS
> +	def_bool y

Why not just have

config ARCH_HAS_DEBUG_STRICT_USER_COPY_CHECKS
	bool

in lib/Kconfig.debug

and select that in each arch that want it?

-- 
Cheers,
Stephen Rothwell                    sfr at canb.auug.org.au
http://www.canb.auug.org.au/~sfr/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 490 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20100819/56db0070/attachment.sig>

Re: [PATCHv2 2/1] Consolidate CONFIG_DEBUG_STRICT_USER_COPY_CHECKS

[Arnd Bergmann]

On Thursday 19 August 2010, Stephen Rothwell wrote:
>   On Wed, 18 Aug 2010 19:28:18 -0700 Stephen Boyd <sboyd@codeaurora.org> wrote:
> >
> > +config ARCH_HAS_DEBUG_STRICT_USER_COPY_CHECKS
> > +     def_bool y
> 
> Why not just have
> 
> config ARCH_HAS_DEBUG_STRICT_USER_COPY_CHECKS
>         bool
> 
> in lib/Kconfig.debug
> 
> and select that in each arch that want it?
> 

Yes, that would be even easier, thanks for the suggestion!

	Arnd

[PATCHv2 2/1] Consolidate CONFIG_DEBUG_STRICT_USER_COPY_CHECKS

[Arnd Bergmann]

On Thursday 19 August 2010, Stephen Rothwell wrote:
>   On Wed, 18 Aug 2010 19:28:18 -0700 Stephen Boyd <sboyd@codeaurora.org> wrote:
> >
> > +config ARCH_HAS_DEBUG_STRICT_USER_COPY_CHECKS
> > +     def_bool y
> 
> Why not just have
> 
> config ARCH_HAS_DEBUG_STRICT_USER_COPY_CHECKS
>         bool
> 
> in lib/Kconfig.debug
> 
> and select that in each arch that want it?
> 

Yes, that would be even easier, thanks for the suggestion!

	Arnd

Re: [PATCH] ARM: uaccess: Implement strict user copy checks

[Arnd Bergmann]

On Wednesday 04 August 2010, Stephen Boyd wrote:
> +config DEBUG_STRICT_USER_COPY_CHECKS
> +       bool "Strict user copy size checks"
> +       depends on DEBUG_KERNEL
> +       help
> +         Enabling this option turns a certain set of sanity checks for user
> +         copy operations into compile time errors.
> +
> +         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.
> +

Do you actually need to disable this if running an older gcc? AFAICT, it
should just have no effect at all in that case, so the comment is slightly
misleading.

Also, why turn this specific warning into an error but not any of the other
warnings? Some architectures (alpha, sparc, mips, powerpc, sh) simply turn
on -Werror for architecture specific code in general, which seems very
useful. We can also make that a config option (probably arch independent)
that we turn on for defconfig files that we know build without warnings.

Unfortunately, there is a number of device drivers that have never been
warning-free, so we can't just enable -Werror for all code.

	Arnd

[PATCH] ARM: uaccess: Implement strict user copy checks

[Arnd Bergmann]

On Wednesday 04 August 2010, Stephen Boyd wrote:
> +config DEBUG_STRICT_USER_COPY_CHECKS
> +       bool "Strict user copy size checks"
> +       depends on DEBUG_KERNEL
> +       help
> +         Enabling this option turns a certain set of sanity checks for user
> +         copy operations into compile time errors.
> +
> +         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.
> +

Do you actually need to disable this if running an older gcc? AFAICT, it
should just have no effect at all in that case, so the comment is slightly
misleading.

Also, why turn this specific warning into an error but not any of the other
warnings? Some architectures (alpha, sparc, mips, powerpc, sh) simply turn
on -Werror for architecture specific code in general, which seems very
useful. We can also make that a config option (probably arch independent)
that we turn on for defconfig files that we know build without warnings.

Unfortunately, there is a number of device drivers that have never been
warning-free, so we can't just enable -Werror for all code.

	Arnd

Re: [PATCH] ARM: uaccess: Implement strict user copy checks

[Stephen Boyd]

On 08/10/2010 08:04 PM, Arnd Bergmann wrote:
>
> Do you actually need to disable this if running an older gcc? AFAICT, it
> should just have no effect at all in that case, so the comment is slightly
> misleading.

I blindly copied the help text from x86. Will fix to be less misleading.

>
> Also, why turn this specific warning into an error but not any of the other
> warnings? Some architectures (alpha, sparc, mips, powerpc, sh) simply turn
> on -Werror for architecture specific code in general, which seems very
> useful. We can also make that a config option (probably arch independent)
> that we turn on for defconfig files that we know build without warnings.
>
> Unfortunately, there is a number of device drivers that have never been
> warning-free, so we can't just enable -Werror for all code.
>

I'm following the x86 implementation. I suppose it's done this way since 
many drivers aren't warning free (as you mention) and turning on -Werror 
will make it more annoying to find these types of errors. Since there 
isn't any -Werror=user-copy this approach allows us to find this type of 
error easily without having to sift through noise.

Enabling -Werror in architecture specific code wouldn't help much here 
though right since this is going to be inlined into drivers and such?

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH] ARM: uaccess: Implement strict user copy checks

[Stephen Boyd]

On 08/10/2010 08:04 PM, Arnd Bergmann wrote:
>
> Do you actually need to disable this if running an older gcc? AFAICT, it
> should just have no effect at all in that case, so the comment is slightly
> misleading.

I blindly copied the help text from x86. Will fix to be less misleading.

>
> Also, why turn this specific warning into an error but not any of the other
> warnings? Some architectures (alpha, sparc, mips, powerpc, sh) simply turn
> on -Werror for architecture specific code in general, which seems very
> useful. We can also make that a config option (probably arch independent)
> that we turn on for defconfig files that we know build without warnings.
>
> Unfortunately, there is a number of device drivers that have never been
> warning-free, so we can't just enable -Werror for all code.
>

I'm following the x86 implementation. I suppose it's done this way since 
many drivers aren't warning free (as you mention) and turning on -Werror 
will make it more annoying to find these types of errors. Since there 
isn't any -Werror=user-copy this approach allows us to find this type of 
error easily without having to sift through noise.

Enabling -Werror in architecture specific code wouldn't help much here 
though right since this is going to be inlined into drivers and such?

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH] ARM: uaccess: Implement strict user copy checks

[Arnd Bergmann]

On Wednesday 11 August 2010, Stephen Boyd wrote:
> On 08/10/2010 08:04 PM, Arnd Bergmann wrote:
> >
> > Do you actually need to disable this if running an older gcc? AFAICT, it
> > should just have no effect at all in that case, so the comment is slightly
> > misleading.
> 
> I blindly copied the help text from x86. Will fix to be less misleading.

Ok, I didn't realize that x86 is also doing this as an optional error.
My comment was obviously not about your copy then but about the
comment in general. Since it would be good to diverge, please
leave the patch as it is then and do a new patch that fixes the
message on all architectures at the same time.

> > Also, why turn this specific warning into an error but not any of the other
> > warnings? Some architectures (alpha, sparc, mips, powerpc, sh) simply turn
> > on -Werror for architecture specific code in general, which seems very
> > useful. We can also make that a config option (probably arch independent)
> > that we turn on for defconfig files that we know build without warnings.
> >
> > Unfortunately, there is a number of device drivers that have never been
> > warning-free, so we can't just enable -Werror for all code.
> >
> 
> I'm following the x86 implementation. I suppose it's done this way since 
> many drivers aren't warning free (as you mention) and turning on -Werror 
> will make it more annoying to find these types of errors. Since there 
> isn't any -Werror=user-copy this approach allows us to find this type of 
> error easily without having to sift through noise.
> 
> Enabling -Werror in architecture specific code wouldn't help much here 
> though right since this is going to be inlined into drivers and such?

My point was that I don't think we should single out this particular
warning and make it an error, while there are other equally important
warnings.

Again, this is directed more at the original code from Arjan than your
copy for the ARM architecture. I agree that it may be helpful to turn
more warnings into errors, but I don't think we should do this on 
this level of detail (one Kconfig option per warning).

Your patch should just go in unmodified, but I'd also suggest generalizing
this a bit more on all architectures.

	Arnd

[PATCH] ARM: uaccess: Implement strict user copy checks

[Arnd Bergmann]

On Wednesday 11 August 2010, Stephen Boyd wrote:
> On 08/10/2010 08:04 PM, Arnd Bergmann wrote:
> >
> > Do you actually need to disable this if running an older gcc? AFAICT, it
> > should just have no effect at all in that case, so the comment is slightly
> > misleading.
> 
> I blindly copied the help text from x86. Will fix to be less misleading.

Ok, I didn't realize that x86 is also doing this as an optional error.
My comment was obviously not about your copy then but about the
comment in general. Since it would be good to diverge, please
leave the patch as it is then and do a new patch that fixes the
message on all architectures at the same time.

> > Also, why turn this specific warning into an error but not any of the other
> > warnings? Some architectures (alpha, sparc, mips, powerpc, sh) simply turn
> > on -Werror for architecture specific code in general, which seems very
> > useful. We can also make that a config option (probably arch independent)
> > that we turn on for defconfig files that we know build without warnings.
> >
> > Unfortunately, there is a number of device drivers that have never been
> > warning-free, so we can't just enable -Werror for all code.
> >
> 
> I'm following the x86 implementation. I suppose it's done this way since 
> many drivers aren't warning free (as you mention) and turning on -Werror 
> will make it more annoying to find these types of errors. Since there 
> isn't any -Werror=user-copy this approach allows us to find this type of 
> error easily without having to sift through noise.
> 
> Enabling -Werror in architecture specific code wouldn't help much here 
> though right since this is going to be inlined into drivers and such?

My point was that I don't think we should single out this particular
warning and make it an error, while there are other equally important
warnings.

Again, this is directed more at the original code from Arjan than your
copy for the ARM architecture. I agree that it may be helpful to turn
more warnings into errors, but I don't think we should do this on 
this level of detail (one Kconfig option per warning).

Your patch should just go in unmodified, but I'd also suggest generalizing
this a bit more on all architectures.

	Arnd