login
Search
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 ...
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. --
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? --
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. --
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 && ...
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 --
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. --
(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 --
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 --
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 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 --
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 --
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. --
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 ...I wrote this one, and I can't think of an easy way to do fix 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 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 --
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. --
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 ...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 ...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 ...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 ...
Acked-by: Arjan van de Ven <arjan@linux.intel.com> --
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/
Yes, that would be even easier, thanks for the suggestion! Arnd --
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 --
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. --
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 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 --
| Greg KH | Og dreams of kernels |
| Jens Axboe | [PATCH 31/33] Fusion: sg chaining support |
| Arnd Bergmann | Re: finding your own dead "CONFIG_" variables |
| Mark Brown | [PATCH 2/2] Subject: natsemi: Allow users to disable workaround for DspCfg reset |
| Tony Breeds | [LGUEST] Look in object dir for .config |
git: | |
| Brian Downing | Re: Git in a Nutshell guide |
| John Benes | Re: master has some toys |
| Matthias Lederhofer | [PATCH 4/7] introduce GIT_WORK_TREE to specify the work tree |
| Alexander Sulfrian | [RFC/PATCH] RE: git calls SSH_ASKPASS even if DISPLAY is not set |
| Junio C Hamano |