LinuxLists.cc - [PATCH] ARM: uaccess: Implement strict user copy checks

[permalink] [raw]

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

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 <[email protected]>
>

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.

2010-08-10 22:55:40

by Russell King - ARM Linux

[permalink] [raw]

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

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 <[email protected]>
>>
>
> 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?

2010-08-11 00:27:08

[permalink] [raw]

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

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.

2010-08-11 03:05:18

[permalink] [raw]

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

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

2010-08-11 18:46:56

[permalink] [raw]

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

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.

2010-08-12 15:00:53

[permalink] [raw]

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

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

2010-08-18 01:29:20

[permalink] [raw]

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

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 <[email protected]>
Cc: Arnd Bergmann <[email protected]>
Cc: Russell King <[email protected]>
---

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.

2010-08-18 12:28:42

[permalink] [raw]

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

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 [email protected] 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

2010-08-18 19:48:34

[permalink] [raw]

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

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 [email protected] 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.

2010-08-19 02:28:24

[permalink] [raw]

Subject: [PATCHv2 2/1] Consolidate CONFIG_DEBUG_STRICT_USER_COPY_CHECKS

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 <[email protected]>
Cc: Arnd Bergmann <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: Arjan van de Ven <[email protected]>
Cc: Heiko Carstens <[email protected]>
Cc: Helge Deller <[email protected]>
---

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.

2010-08-19 04:38:39

by Arjan van de Ven

[permalink] [raw]

Subject: Re: [PATCHv2 2/1] Consolidate CONFIG_DEBUG_STRICT_USER_COPY_CHECKS

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 <[email protected]>

2010-08-19 04:47:31

by Stephen Rothwell

[permalink] [raw]

Subject: Re: [PATCHv2 2/1] Consolidate CONFIG_DEBUG_STRICT_USER_COPY_CHECKS

On Wed, 18 Aug 2010 19:28:18 -0700 Stephen Boyd <[email protected]> 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 [email protected]
http://www.canb.auug.org.au/~sfr/

Attachments:

(No filename) (386.00 B)
(No filename) (490.00 B)
Download all attachments

2010-08-19 11:04:12

[permalink] [raw]

Subject: Re: [PATCHv2 2/1] Consolidate CONFIG_DEBUG_STRICT_USER_COPY_CHECKS

On Thursday 19 August 2010, Stephen Rothwell wrote:
> On Wed, 18 Aug 2010 19:28:18 -0700 Stephen Boyd <[email protected]> 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

2010-08-19 11:09:08

[permalink] [raw]

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

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

2010-08-24 15:05:02

[permalink] [raw]

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

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.

2010-08-24 15:26:20

[permalink] [raw]

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

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

2010-08-24 15:45:51

[permalink] [raw]

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

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?

2010-08-25 12:14:29

[permalink] [raw]

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

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

2010-08-25 12:52:59

[permalink] [raw]

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

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? :)

2010-08-25 13:56:18

[permalink] [raw]

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

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

2010-08-25 14:38:47

[permalink] [raw]

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

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.

2010-08-28 01:35:18

[permalink] [raw]

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

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.

2010-08-28 07:41:14

[permalink] [raw]

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

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

2010-08-28 09:57:09