Hi Arjan,
I was just about to port the strict copy_from_user checks to s390, but
I have some issues with it:
Is there a reason why there isn't a generic infrastructure that simply
can be 'selected' by each architecure? I guess there isn't ;)
x86_32 and x86_64 have different copy_from_user wrappers where only the
32 bit version will generate compile warnings. Is that intentional or was
the 64 bit version just forgotten when updating?
x86 and sparc return -EFAULT in copy_from_user instead of the number of
not copied bytes as it should in case of a detected buffer overflow.
That might have unwanted side effects. I would guess that is a bug.
Warnings cannot be switched off anymore as it was the case in your first
version. However gcc seems to report quite a few false positives so
it would be good if it could be turned off again.
E.g. this one with gcc 4.4.0:
In file included from /home2/heicarst/linux-2.6/arch/s390/include/asm/mmu_context.h:13,
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 kernel/kprobes.c:39:
In function 'copy_from_user',
inlined from 'write_enabled_file_bool' at kernel/kprobes.c:1527:
/home2/heicarst/linux-2.6/arch/s390/include/asm/uaccess.h:295: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
Even though I'm wondering why the reported function isn't simply using
get_user(). But that is a different story.
Instead of going the easy way and implementing the 3rd arch specific version
I also could address all of these issues :)
On Mon, 4 Jan 2010 16:43:45 +0100
Heiko Carstens <[email protected]> wrote:
> Hi Arjan,
>
> I was just about to port the strict copy_from_user checks to s390, but
> I have some issues with it:
>
> Is there a reason why there isn't a generic infrastructure that simply
> can be 'selected' by each architecure? I guess there isn't ;)
the compiler.h side is already generic; just that the copy from user
itself is different between architectures.
> x86 and sparc return -EFAULT in copy_from_user instead of the number
> of not copied bytes as it should in case of a detected buffer
> overflow. That might have unwanted side effects. I would guess that
> is a bug.
killing the bad guy in case of a real buffer overflow is appropriate..
this should never trigger for legitimate users.
>
> Warnings cannot be switched off anymore as it was the case in your
> first version. However gcc seems to report quite a few false
> positives so it would be good if it could be turned off again.
hmm I thought most got fixed.. I'd be surprised if this part is
architecture specific.....
I rather fix the few cases left than disable the warning to be honest.
It's not many, at least not on x86.
--
Arjan van de Ven Intel Open Source Technology Centre
For development, discussion and tips for power savings,
visit http://www.lesswatts.org
* Arjan van de Ven <[email protected]> wrote:
> > Warnings cannot be switched off anymore as it was the case in your first
> > version. However gcc seems to report quite a few false positives so it
> > would be good if it could be turned off again.
>
> hmm I thought most got fixed.. I'd be surprised if this part is architecture
> specific..... I rather fix the few cases left than disable the warning to be
> honest. It's not many, at least not on x86.
Would be nice to see the warnings reported.
Thanks,
Ingo
On Mon, Jan 04, 2010 at 05:43:08PM -0800, Arjan van de Ven wrote:
> On Mon, 4 Jan 2010 16:43:45 +0100
> Heiko Carstens <[email protected]> wrote:
> > x86 and sparc return -EFAULT in copy_from_user instead of the number
> > of not copied bytes as it should in case of a detected buffer
> > overflow. That might have unwanted side effects. I would guess that
> > is a bug.
>
> killing the bad guy in case of a real buffer overflow is appropriate..
> this should never trigger for legitimate users.
The point I tried to make is that no caller of copy_from_user can assume
that it would ever return -EFAULT. And if any caller does so it is broken.
But then again it probably doesn't matter in this case as long as something
!= 0 is returned.
> > Warnings cannot be switched off anymore as it was the case in your
> > first version. However gcc seems to report quite a few false
> > positives so it would be good if it could be turned off again.
>
> hmm I thought most got fixed.. I'd be surprised if this part is
> architecture specific.....
> I rather fix the few cases left than disable the warning to be honest.
> It's not many, at least not on x86.
An allyesconfig triggers 52 warnings on s390 (see below). I just checked
a few but all of them looked like false positives.
This is the patch I currently have for s390. Only differences to x86 and
sparc are the return code handling and that an allyesconfig will trigger
warnings instead of compile breakages.
---
arch/s390/Kconfig.debug | 25 +++++++++++++++++++++++++
arch/s390/include/asm/uaccess.h | 14 ++++++++++++++
arch/s390/lib/Makefile | 2 +-
arch/s390/lib/usercopy.c | 8 ++++++++
4 files changed, 48 insertions(+), 1 deletion(-)
--- a/arch/s390/include/asm/uaccess.h
+++ b/arch/s390/include/asm/uaccess.h
@@ -265,6 +265,14 @@ __copy_from_user(void *to, const void __
return uaccess.copy_from_user(n, from, to);
}
+extern void copy_from_user_overflow(void)
+#ifdef CONFIG_DEBUG_STRICT_USER_COPY_CHECKS
+__compiletime_warning("copy_from_user() buffer size is not provably correct")
+#else
+__compiletime_error("copy_from_user() buffer size is not provably correct")
+#endif
+;
+
/**
* copy_from_user: - Copy a block of data from user space.
* @to: Destination address, in kernel space.
@@ -284,7 +292,13 @@ __copy_from_user(void *to, const void __
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);
+
might_fault();
+ 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
--- a/arch/s390/Kconfig.debug
+++ b/arch/s390/Kconfig.debug
@@ -6,4 +6,29 @@ config TRACE_IRQFLAGS_SUPPORT
source "lib/Kconfig.debug"
+choice
+ prompt "Strict copy size checks"
+ depends on DEBUG_KERNEL && !TRACE_BRANCH_PROFILING
+
+config DEBUG_STRICT_USER_COPY_CHECKS
+ bool "Enabled - compile time warnings"
+ ---help---
+ Enabling this option adds a certain set of sanity checks for user
+ copy operations.
+
+ 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 gcc cannot prove that security checks are sufficient runtime
+ checks will be added.
+
+config DEBUG_STRICT_USER_COPY_CHECKS_WARN
+ bool "Enabled - compile time errors"
+ ---help---
+ Enabling this option turns a certain set of sanity checks for user
+ copy operations into compile time warnings.
+
+endchoice
+
endmenu
--- a/arch/s390/lib/Makefile
+++ b/arch/s390/lib/Makefile
@@ -2,7 +2,7 @@
# Makefile for s390-specific library files..
#
-lib-y += delay.o string.o uaccess_std.o uaccess_pt.o
+lib-y += delay.o string.o uaccess_std.o uaccess_pt.o usercopy.o
obj-$(CONFIG_32BIT) += div64.o qrnnd.o ucmpdi2.o
lib-$(CONFIG_64BIT) += uaccess_mvcos.o
lib-$(CONFIG_SMP) += spinlock.o
--- /dev/null
+++ b/arch/s390/lib/usercopy.c
@@ -0,0 +1,8 @@
+#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);
Warnings generated with an allyesconfig build:
In file included from /home2/heicarst/cfu/arch/s390/include/asm/mmu_context.h:13,
from /home2/heicarst/cfu/arch/s390/include/asm/elf.h:133,
from include/linux/elf.h:7,
from include/linux/module.h:14,
from fs/debugfs/file.c:16:
In function 'copy_from_user',
inlined from 'write_file_bool' at fs/debugfs/file.c:415:
/home2/heicarst/cfu/arch/s390/include/asm/uaccess.h:299: 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/cfu/arch/s390/include/asm/mmu_context.h:13,
from /home2/heicarst/cfu/arch/s390/include/asm/elf.h:133,
from include/linux/elf.h:7,
from include/linux/module.h:14,
from kernel/kprobes.c:39:
In function 'copy_from_user',
inlined from 'write_enabled_file_bool' at kernel/kprobes.c:1527:
/home2/heicarst/cfu/arch/s390/include/asm/uaccess.h:299: 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/cfu/arch/s390/include/asm/mmu_context.h:13,
from /home2/heicarst/cfu/arch/s390/include/asm/elf.h:133,
from include/linux/elf.h:7,
from include/linux/module.h:14,
from drivers/s390/crypto/zcrypt_api.c:30:
In function 'copy_from_user',
inlined from 'zcrypt_rsa_crt' at drivers/s390/crypto/zcrypt_api.c:401:
/home2/heicarst/cfu/arch/s390/include/asm/uaccess.h:299: 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/cfu/arch/s390/include/asm/mmu_context.h:13,
from /home2/heicarst/cfu/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:4563,
inlined from 'SYSC_perf_event_open' at kernel/perf_event.c:4668,
inlined from 'SyS_perf_event_open' at kernel/perf_event.c:4653:
/home2/heicarst/cfu/arch/s390/include/asm/uaccess.h:299: 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/cfu/arch/s390/include/asm/mmu_context.h:13,
from /home2/heicarst/cfu/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:1124:
/home2/heicarst/cfu/arch/s390/include/asm/uaccess.h:299: 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/cfu/arch/s390/include/asm/mmu_context.h:13,
from /home2/heicarst/cfu/arch/s390/include/asm/elf.h:133,
from include/linux/elf.h:7,
from include/linux/module.h:14,
from net/can/raw.c:44:
In function 'copy_from_user',
inlined from 'raw_setsockopt' at net/can/raw.c:447:
/home2/heicarst/cfu/arch/s390/include/asm/uaccess.h:299: 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/cfu/arch/s390/include/asm/mmu_context.h:13,
from /home2/heicarst/cfu/arch/s390/include/asm/elf.h:133,
from include/linux/elf.h:7,
from include/linux/module.h:14,
from net/core/pktgen.c:120:
In function 'copy_from_user',
inlined from 'pktgen_if_write' at net/core/pktgen.c:866:
/home2/heicarst/cfu/arch/s390/include/asm/uaccess.h:299: 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:1084:
/home2/heicarst/cfu/arch/s390/include/asm/uaccess.h:299: 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:1185:
/home2/heicarst/cfu/arch/s390/include/asm/uaccess.h:299: 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:1207:
/home2/heicarst/cfu/arch/s390/include/asm/uaccess.h:299: 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:1231:
/home2/heicarst/cfu/arch/s390/include/asm/uaccess.h:299: 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:1254:
/home2/heicarst/cfu/arch/s390/include/asm/uaccess.h:299: 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:1277:
/home2/heicarst/cfu/arch/s390/include/asm/uaccess.h:299: 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:1298:
/home2/heicarst/cfu/arch/s390/include/asm/uaccess.h:299: 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:
/home2/heicarst/cfu/arch/s390/include/asm/uaccess.h:299: 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:1340:
/home2/heicarst/cfu/arch/s390/include/asm/uaccess.h:299: 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:1367:
/home2/heicarst/cfu/arch/s390/include/asm/uaccess.h:299: 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:1409:
/home2/heicarst/cfu/arch/s390/include/asm/uaccess.h:299: 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:1739:
/home2/heicarst/cfu/arch/s390/include/asm/uaccess.h:299: 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:1770:
/home2/heicarst/cfu/arch/s390/include/asm/uaccess.h:299: 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/cfu/arch/s390/include/asm/mmu_context.h:13,
from /home2/heicarst/cfu/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_dressz' at drivers/scsi/sg.c:2381:
/home2/heicarst/cfu/arch/s390/include/asm/uaccess.h:299: 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:2358:
/home2/heicarst/cfu/arch/s390/include/asm/uaccess.h:299: 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/cfu/arch/s390/include/asm/mmu_context.h:13,
from /home2/heicarst/cfu/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/if_ether.h:124,
from include/linux/netdevice.h:29,
from net/packet/af_packet.c:58:
In function 'copy_from_user',
inlined from 'packet_getsockopt' at net/packet/af_packet.c:1880:
/home2/heicarst/cfu/arch/s390/include/asm/uaccess.h:299: 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/cfu/arch/s390/include/asm/mmu_context.h:13,
from /home2/heicarst/cfu/arch/s390/include/asm/elf.h:133,
from include/linux/elf.h:7,
from include/linux/module.h:14,
from net/netfilter/ipvs/ip_vs_ctl.c:24:
In function 'copy_from_user',
inlined from 'do_ip_vs_get_ctl' at net/netfilter/ipvs/ip_vs_ctl.c:2365:
/home2/heicarst/cfu/arch/s390/include/asm/uaccess.h:299: 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 'do_ip_vs_set_ctl' at net/netfilter/ipvs/ip_vs_ctl.c:2086:
/home2/heicarst/cfu/arch/s390/include/asm/uaccess.h:299: 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/cfu/arch/s390/include/asm/mmu_context.h:13,
from /home2/heicarst/cfu/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:18:
In function 'copy_from_user',
inlined from 'compat_sys_socketcall' at net/compat.c:785:
/home2/heicarst/cfu/arch/s390/include/asm/uaccess.h:299: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct
On Tuesday 05 January 2010, Heiko Carstens wrote:
> On Mon, Jan 04, 2010 at 05:43:08PM -0800, Arjan van de Ven wrote:
> > On Mon, 4 Jan 2010 16:43:45 +0100
> > Heiko Carstens <[email protected]> wrote:
> > > x86 and sparc return -EFAULT in copy_from_user instead of the number
> > > of not copied bytes as it should in case of a detected buffer
> > > overflow. That might have unwanted side effects. I would guess that
> > > is a bug.
> >
> > killing the bad guy in case of a real buffer overflow is appropriate..
> > this should never trigger for legitimate users.
>
> The point I tried to make is that no caller of copy_from_user can assume
> that it would ever return -EFAULT. And if any caller does so it is broken.
> But then again it probably doesn't matter in this case as long as something
> != 0 is returned.
To quote simple_read_from_buffer():
size_t ret;
...
ret = copy_to_user(to, from + pos, count);
if (ret == count)
return -EFAULT;
count -= ret;
*ppos = pos + count;
return count;
If copy_from_user() returns a negative value, bad things happen to f_pos
and to the value returned from the syscall. Many read() file_operations
do this similarly, and I wouldn't be surprised if this could be turned
into a security exploit for one of them (not simple_read_from_buffer
probably).
Arnd
On Tue, Jan 05, 2010 at 01:47:20PM +0100, Arnd Bergmann wrote:
> On Tuesday 05 January 2010, Heiko Carstens wrote:
> > On Mon, Jan 04, 2010 at 05:43:08PM -0800, Arjan van de Ven wrote:
> > > On Mon, 4 Jan 2010 16:43:45 +0100
> > > Heiko Carstens <[email protected]> wrote:
> > > > x86 and sparc return -EFAULT in copy_from_user instead of the number
> > > > of not copied bytes as it should in case of a detected buffer
> > > > overflow. That might have unwanted side effects. I would guess that
> > > > is a bug.
> > >
> > > killing the bad guy in case of a real buffer overflow is appropriate..
> > > this should never trigger for legitimate users.
> >
> > The point I tried to make is that no caller of copy_from_user can assume
> > that it would ever return -EFAULT. And if any caller does so it is broken.
> > But then again it probably doesn't matter in this case as long as something
> > != 0 is returned.
>
> To quote simple_read_from_buffer():
>
> size_t ret;
> ...
> ret = copy_to_user(to, from + pos, count);
> if (ret == count)
> return -EFAULT;
> count -= ret;
> *ppos = pos + count;
> return count;
>
> If copy_from_user() returns a negative value, bad things happen to f_pos
> and to the value returned from the syscall. Many read() file_operations
> do this similarly, and I wouldn't be surprised if this could be turned
> into a security exploit for one of them (not simple_read_from_buffer
> probably).
Thanks Arnd. I was looking for such an example. That's why I was about to
send the patch below (untested).
Subject: [PATCH] x86: copy_from_user() should not return -EFAULT
From: Heiko Carstens <[email protected]>
Callers of copy_from_user() expect it to return the number of bytes
it could not copy. In no case it is supposed to return -EFAULT.
In case of a detected buffer overflow just return the requested
length. In addition one could think of a memset that would clear
the size of the target object.
Signed-off-by: Heiko Carstens <[email protected]>
---
arch/x86/include/asm/uaccess_32.h | 5 ++---
arch/x86/include/asm/uaccess_64.h | 5 ++---
2 files changed, 4 insertions(+), 6 deletions(-)
--- a/arch/x86/include/asm/uaccess_32.h
+++ b/arch/x86/include/asm/uaccess_32.h
@@ -205,14 +205,13 @@ static inline unsigned long __must_check
unsigned long n)
{
int sz = __compiletime_object_size(to);
- int ret = -EFAULT;
if (likely(sz == -1 || sz >= n))
- ret = _copy_from_user(to, from, n);
+ n = _copy_from_user(to, from, n);
else
copy_from_user_overflow();
- return ret;
+ return n;
}
long __must_check strncpy_from_user(char *dst, const char __user *src,
--- a/arch/x86/include/asm/uaccess_64.h
+++ b/arch/x86/include/asm/uaccess_64.h
@@ -30,16 +30,15 @@ static inline unsigned long __must_check
unsigned long n)
{
int sz = __compiletime_object_size(to);
- int ret = -EFAULT;
might_fault();
if (likely(sz == -1 || sz >= n))
- ret = _copy_from_user(to, from, n);
+ n = _copy_from_user(to, from, n);
#ifdef CONFIG_DEBUG_VM
else
WARN(1, "Buffer overflow detected!\n");
#endif
- return ret;
+ return n;
}
static __always_inline __must_check
On Tue, 5 Jan 2010 14:19:11 +0100
Heiko Carstens <[email protected]> wrote:
> On Tue, Jan 05, 2010 at 01:47:20PM +0100, Arnd Bergmann wrote:
> > On Tuesday 05 January 2010, Heiko Carstens wrote:
> > > On Mon, Jan 04, 2010 at 05:43:08PM -0800, Arjan van de Ven wrote:
> > > > On Mon, 4 Jan 2010 16:43:45 +0100
> > > > Heiko Carstens <[email protected]> wrote:
> > > > > x86 and sparc return -EFAULT in copy_from_user instead of the
> > > > > number of not copied bytes as it should in case of a detected
> > > > > buffer overflow. That might have unwanted side effects. I
> > > > > would guess that is a bug.
> > > >
> > > > killing the bad guy in case of a real buffer overflow is
> > > > appropriate.. this should never trigger for legitimate users.
> > >
> > > The point I tried to make is that no caller of copy_from_user can
> > > assume that it would ever return -EFAULT. And if any caller does
> > > so it is broken. But then again it probably doesn't matter in
> > > this case as long as something != 0 is returned.
> >
> > To quote simple_read_from_buffer():
> >
> > size_t ret;
> > ...
> > ret = copy_to_user(to, from + pos, count);
> > if (ret == count)
> > return -EFAULT;
> > count -= ret;
> > *ppos = pos + count;
> > return count;
> >
> > If copy_from_user() returns a negative value, bad things happen to
> > f_pos and to the value returned from the syscall. Many read()
> > file_operations do this similarly, and I wouldn't be surprised if
> > this could be turned into a security exploit for one of them (not
> > simple_read_from_buffer probably).
>
> Thanks Arnd. I was looking for such an example. That's why I was
> about to send the patch below (untested).
Acked-by: Arjan van de Ven <[email protected]>
--
Arjan van de Ven Intel Open Source Technology Centre
For development, discussion and tips for power savings,
visit http://www.lesswatts.org
On Tue, 5 Jan 2010 10:48:57 +0100
Heiko Carstens <[email protected]> wrote:
>
> An allyesconfig triggers 52 warnings on s390 (see below). I just
> checked a few but all of them looked like false positives.
hmm I wonder why s390 gcc is so different.... if the s390 gcc isn't
so good at proving things, maybe it's wrong to warn on s390?
> In file included
> from /home2/heicarst/cfu/arch/s390/include/asm/mmu_context.h:13,
> from /home2/heicarst/cfu/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:1124:
> /home2/heicarst/cfu/arch/s390/include/asm/uaccess.h:299: warning:
> call to 'copy_from_user_overflow' declared with attribute warning:
> copy_from_user() buffer size is not provably correct
this one is ... interesting btw... I have trouble myself finding where
the check is done... so I can understand gcc having trouble too.
--
Arjan van de Ven Intel Open Source Technology Centre
For development, discussion and tips for power savings,
visit http://www.lesswatts.org
On Tue, 5 Jan 2010 05:34:43 -0800
Arjan van de Ven <[email protected]> wrote:
>
> this one is ... interesting btw... I have trouble myself finding where
> the check is done... so I can understand gcc having trouble too.
hmm I guess that's just because it's 5:30am here .. at 5:35am I do see
it.
Wonder if on x86 the function gets inlined, at which point gcc sees the
check, while on s390 it doesn't ?
--
Arjan van de Ven Intel Open Source Technology Centre
For development, discussion and tips for power savings,
visit http://www.lesswatts.org
On Tuesday 05 January 2010, Arjan van de Ven wrote:
> > In file included
> > from /home2/heicarst/cfu/arch/s390/include/asm/mmu_context.h:13,
> > from /home2/heicarst/cfu/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:1124:
> > /home2/heicarst/cfu/arch/s390/include/asm/uaccess.h:299: warning:
> > call to 'copy_from_user_overflow' declared with attribute warning:
> > copy_from_user() buffer size is not provably correct
>
> this one is ... interesting btw... I have trouble myself finding where
> the check is done... so I can understand gcc having trouble too.
>
I think it will get inlined on 32 bit machines or without CONFIG_COMPAT,
but not when CONFIG_COMPAT is enabled, because then there are two
call-sites.
The tun_chr_compat_ioctl was only merged in 2.6.33-rc1, so 2.6.32 could
still inline the function all the time.
If the compiler is really smart (haven't tried), it can optimize away
tun_chr_compat_ioctl entirely on i386 and make it an alias to
tun_chr_ioctl, but not on s390 because that uses a nontrivial compat_ptr()
function.
Arnd
On Tue, 5 Jan 2010 14:45:25 +0100
Arnd Bergmann <[email protected]> wrote:
>
> I think it will get inlined on 32 bit machines or without
> CONFIG_COMPAT, but not when CONFIG_COMPAT is enabled, because then
> there are two call-sites.
one of them is buggy it seems;
it passes in a shorter length, but there is no code in sight that makes
sure that the end of the structure (the difference between the shorter
and full length one) gets initialized to, say, zeros rather than stack
garbage. So looks like there is at least a bug there.
Would be nice if the copy (+ clear) would be pulled to the two callers
I suspect... at which point the warning will go away too as a side
effect.
--
Arjan van de Ven Intel Open Source Technology Centre
For development, discussion and tips for power savings,
visit http://www.lesswatts.org
On Tuesday 05 January 2010, Arjan van de Ven wrote:
> On Tue, 5 Jan 2010 14:45:25 +0100
> Arnd Bergmann <[email protected]> wrote:
> >
> > I think it will get inlined on 32 bit machines or without
> > CONFIG_COMPAT, but not when CONFIG_COMPAT is enabled, because then
> > there are two call-sites.
>
> one of them is buggy it seems;
> it passes in a shorter length, but there is no code in sight that makes
> sure that the end of the structure (the difference between the shorter
> and full length one) gets initialized to, say, zeros rather than stack
> garbage. So looks like there is at least a bug there.
The old code (until 2.6.32) always copied sizeof (struct ifreq), which
I changed in order not corrupt the user space stack.
I checked that no code in the tun driver accesses the incompatible
parts of the structure, which would require conversion rather
than simply doing a short read, so it looks correct to me, or am I
missing something?
> Would be nice if the copy (+ clear) would be pulled to the two callers
> I suspect... at which point the warning will go away too as a side
> effect.
You mean like this?
It adds some complexity and about 200 bytes of object code,
I'm not sure it's worth it.
--
[UNTESTED PATCH] tun: avoid copy_from_user size warning
For 32 bit compat code, the tun driver only copies the
fields it needs using a short length, which copy_from_user
now warns about. Moving the copy operation outside of the
main ioctl function lets us avoid the warning.
Signed-off-by: Arnd Bergmann <[email protected]>
---
drivers/net/tun.c | 104 +++++++++++++++++++++++++++++++----------------------
1 files changed, 61 insertions(+), 43 deletions(-)
diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 01e99f2..8eb9f38 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -1111,19 +1111,14 @@ 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, struct ifreq *ifr)
{
struct tun_file *tfile = file->private_data;
struct tun_struct *tun;
void __user* argp = (void __user*)arg;
- struct ifreq ifr;
int sndbuf;
int ret;
- if (cmd == TUNSETIFF || _IOC_TYPE(cmd) == 0x89)
- if (copy_from_user(&ifr, argp, ifreq_len))
- return -EFAULT;
-
if (cmd == TUNGETFEATURES) {
/* Currently this just means: "what IFF flags are valid?".
* This is needed because we never checked for invalid flags on
@@ -1136,34 +1131,21 @@ static long __tun_chr_ioctl(struct file *file, unsigned int cmd,
rtnl_lock();
tun = __tun_get(tfile);
- if (cmd == TUNSETIFF && !tun) {
- ifr.ifr_name[IFNAMSIZ-1] = '\0';
-
- ret = tun_set_iff(tfile->net, file, &ifr);
-
- if (ret)
- goto unlock;
-
- if (copy_to_user(argp, &ifr, ifreq_len))
- ret = -EFAULT;
+ if (!tun) {
+ ret = -EBADFD;
+ if (cmd == TUNSETIFF) {
+ ifr->ifr_name[IFNAMSIZ-1] = '\0';
+ ret = tun_set_iff(tfile->net, file, ifr);
+ }
goto unlock;
}
- ret = -EBADFD;
- if (!tun)
- goto unlock;
-
DBG(KERN_INFO "%s: tun_chr_ioctl cmd %d\n", tun->dev->name, cmd);
ret = 0;
switch (cmd) {
case TUNGETIFF:
- ret = tun_get_iff(current->nsproxy->net_ns, tun, &ifr);
- if (ret)
- break;
-
- if (copy_to_user(argp, &ifr, ifreq_len))
- ret = -EFAULT;
+ ret = tun_get_iff(current->nsproxy->net_ns, tun, ifr);
break;
case TUNSETNOCSUM:
@@ -1234,18 +1216,16 @@ static long __tun_chr_ioctl(struct file *file, unsigned int cmd,
case SIOCGIFHWADDR:
/* Get hw addres */
- memcpy(ifr.ifr_hwaddr.sa_data, tun->dev->dev_addr, ETH_ALEN);
- ifr.ifr_hwaddr.sa_family = tun->dev->type;
- if (copy_to_user(argp, &ifr, ifreq_len))
- ret = -EFAULT;
+ memcpy(ifr->ifr_hwaddr.sa_data, tun->dev->dev_addr, ETH_ALEN);
+ ifr->ifr_hwaddr.sa_family = tun->dev->type;
break;
case SIOCSIFHWADDR:
/* Set hw address */
DBG(KERN_DEBUG "%s: set hw address: %pM\n",
- tun->dev->name, ifr.ifr_hwaddr.sa_data);
+ tun->dev->name, ifr->ifr_hwaddr.sa_data);
- ret = dev_set_mac_address(tun->dev, &ifr.ifr_hwaddr);
+ ret = dev_set_mac_address(tun->dev, &ifr->ifr_hwaddr);
break;
case TUNGETSNDBUF:
@@ -1278,35 +1258,73 @@ unlock:
static long tun_chr_ioctl(struct file *file,
unsigned int cmd, unsigned long arg)
{
- return __tun_chr_ioctl(file, cmd, arg, sizeof (struct ifreq));
+ struct ifreq ifr;
+ struct ifreq __user *uifr = (void *)arg;
+ int ret;
+
+ switch (cmd) {
+ case TUNSETIFF:
+ case TUNGETIFF:
+ case SIOCGIFHWADDR:
+ case SIOCSIFHWADDR:
+ if (copy_from_user(&ifr, uifr, sizeof (ifr)))
+ return -EFAULT;
+
+ ret = __tun_chr_ioctl(file, cmd, arg, &ifr);
+ if (ret < 0)
+ return ret;
+
+ if (copy_to_user(uifr, &ifr, sizeof (ifr)))
+ return -EFAULT;
+
+ return ret;
+ }
+
+ return __tun_chr_ioctl(file, cmd, arg, NULL);
}
#ifdef CONFIG_COMPAT
static long tun_chr_compat_ioctl(struct file *file,
unsigned int cmd, unsigned long arg)
{
+ struct ifreq ifr;
+ struct compat_ifreq __user *uifr = compat_ptr(arg);
+ int ret;
+
switch (cmd) {
case TUNSETIFF:
case TUNGETIFF:
+ case SIOCGIFHWADDR:
+ case SIOCSIFHWADDR:
+ /*
+ * compat_ifreq is shorter than ifreq, so we must not access beyond
+ * the end of that structure. All fields that are used in this
+ * driver are compatible though, we don't need to convert the
+ * contents.
+ */
+ memset(&ifr, 0, sizeof (ifr));
+ if (copy_from_user((struct compat_ifreq *)&ifr, uifr, sizeof (*uifr)))
+ return -EFAULT;
+
+ ret = __tun_chr_ioctl(file, cmd, 0, &ifr);
+ if (ret)
+ return ret;
+
+ if (copy_to_user(uifr, (struct compat_ifreq *)&ifr, sizeof (*uifr)))
+ return -EFAULT;
+ return ret;
+ break;
+
case TUNSETTXFILTER:
case TUNGETSNDBUF:
case TUNSETSNDBUF:
- case SIOCGIFHWADDR:
- case SIOCSIFHWADDR:
arg = (unsigned long)compat_ptr(arg);
break;
default:
arg = (compat_ulong_t)arg;
break;
}
-
- /*
- * compat_ifreq is shorter than ifreq, so we must not access beyond
- * the end of that structure. All fields that are used in this
- * driver are compatible though, we don't need to convert the
- * contents.
- */
- return __tun_chr_ioctl(file, cmd, arg, sizeof(struct compat_ifreq));
+ return __tun_chr_ioctl(file, cmd, arg, NULL);
}
#endif /* CONFIG_COMPAT */
Subject: [PATCH] sparc: copy_from_user() should not return -EFAULT
From: Heiko Carstens <[email protected]>
Callers of copy_from_user() expect it to return the number of bytes
it could not copy. In no case it is supposed to return -EFAULT.
In case of a detected buffer overflow just return the requested
length. In addition one could think of a memset that would clear
the size of the target object.
Signed-off-by: Heiko Carstens <[email protected]>
---
arch/sparc/include/asm/uaccess_32.h | 2 +-
arch/sparc/include/asm/uaccess_64.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
--- a/arch/sparc/include/asm/uaccess_32.h
+++ b/arch/sparc/include/asm/uaccess_32.h
@@ -274,7 +274,7 @@ static inline unsigned long copy_from_us
if (unlikely(sz != -1 && sz < n)) {
copy_from_user_overflow();
- return -EFAULT;
+ return n;
}
if (n && __access_ok((unsigned long) from, n))
--- a/arch/sparc/include/asm/uaccess_64.h
+++ b/arch/sparc/include/asm/uaccess_64.h
@@ -221,8 +221,8 @@ extern unsigned long copy_from_user_fixu
static inline unsigned long __must_check
copy_from_user(void *to, const void __user *from, unsigned long size)
{
- unsigned long ret = (unsigned long) -EFAULT;
int sz = __compiletime_object_size(to);
+ unsigned long ret = size;
if (likely(sz == -1 || sz >= size)) {
ret = ___copy_from_user(to, from, size);
Heiko Carstens <[email protected]> writes:
> Subject: [PATCH] sparc: copy_from_user() should not return -EFAULT
>
> From: Heiko Carstens <[email protected]>
>
> Callers of copy_from_user() expect it to return the number of bytes
> it could not copy. In no case it is supposed to return -EFAULT.
>
> In case of a detected buffer overflow just return the requested
> length. In addition one could think of a memset that would clear
> the size of the target object.
Ouch! I would expect this is likely exploitable, e.g. in mount
-Andi
--
[email protected] -- Speaking for myself only.
On Tuesday 05 January 2010, Heiko Carstens wrote:
> Subject: [PATCH] sparc: copy_from_user() should not return -EFAULT
>
> From: Heiko Carstens <[email protected]>
>
> Callers of copy_from_user() expect it to return the number of bytes
> it could not copy. In no case it is supposed to return -EFAULT.
>
> In case of a detected buffer overflow just return the requested
> length. In addition one could think of a memset that would clear
> the size of the target object.
>
> Signed-off-by: Heiko Carstens <[email protected]>
The xtensa clear_user function seems to have a similar issue,
it should return size on failure, not -EFAULT.
Arnd
From: Andi Kleen <[email protected]>
Date: Tue, 05 Jan 2010 18:27:18 +0100
> Heiko Carstens <[email protected]> writes:
>
>> Subject: [PATCH] sparc: copy_from_user() should not return -EFAULT
>>
>> From: Heiko Carstens <[email protected]>
>>
>> Callers of copy_from_user() expect it to return the number of bytes
>> it could not copy. In no case it is supposed to return -EFAULT.
>>
>> In case of a detected buffer overflow just return the requested
>> length. In addition one could think of a memset that would clear
>> the size of the target object.
>
> Ouch! I would expect this is likely exploitable, e.g. in mount
You can rest easy as the problem only exists in 2.6.33-rcX, it got
introduced when I ported over the compile time length validation bits
from x86.
On 01/05/2010 07:20 AM, Arnd Bergmann wrote:
>
> You mean like this?
>
> It adds some complexity and about 200 bytes of object code,
> I'm not sure it's worth it.
>
What's much worse is that it adds churn to an otherwise-tested code path.
We almost need a copy_from/to_user_audited() to override the warning.
Not that errors can't creap back in...
> --
> [UNTESTED PATCH] tun: avoid copy_from_user size warning
>
> For 32 bit compat code, the tun driver only copies the
> fields it needs using a short length, which copy_from_user
> now warns about. Moving the copy operation outside of the
> main ioctl function lets us avoid the warning.
>
> Signed-off-by: Arnd Bergmann <[email protected]>
-hpa
Commit-ID: 409d02ef6d74f5e91f5ea4c587b2ee1375f106fc
Gitweb: http://git.kernel.org/tip/409d02ef6d74f5e91f5ea4c587b2ee1375f106fc
Author: Heiko Carstens <[email protected]>
AuthorDate: Tue, 5 Jan 2010 14:19:11 +0100
Committer: H. Peter Anvin <[email protected]>
CommitDate: Tue, 5 Jan 2010 13:45:06 -0800
x86: copy_from_user() should not return -EFAULT
Callers of copy_from_user() expect it to return the number of bytes
it could not copy. In no case it is supposed to return -EFAULT.
In case of a detected buffer overflow just return the requested
length. In addition one could think of a memset that would clear
the size of the target object.
[ hpa: code is not in .32 so not needed for -stable ]
Signed-off-by: Heiko Carstens <[email protected]>
Acked-by: Arjan van de Ven <[email protected]>
LKML-Reference: <[email protected]>
Signed-off-by: H. Peter Anvin <[email protected]>
---
arch/x86/include/asm/uaccess_32.h | 5 ++---
arch/x86/include/asm/uaccess_64.h | 5 ++---
2 files changed, 4 insertions(+), 6 deletions(-)
diff --git a/arch/x86/include/asm/uaccess_32.h b/arch/x86/include/asm/uaccess_32.h
index 0c9825e..088d09f 100644
--- a/arch/x86/include/asm/uaccess_32.h
+++ b/arch/x86/include/asm/uaccess_32.h
@@ -205,14 +205,13 @@ static inline unsigned long __must_check copy_from_user(void *to,
unsigned long n)
{
int sz = __compiletime_object_size(to);
- int ret = -EFAULT;
if (likely(sz == -1 || sz >= n))
- ret = _copy_from_user(to, from, n);
+ n = _copy_from_user(to, from, n);
else
copy_from_user_overflow();
- return ret;
+ return n;
}
long __must_check strncpy_from_user(char *dst, const char __user *src,
diff --git a/arch/x86/include/asm/uaccess_64.h b/arch/x86/include/asm/uaccess_64.h
index 46324c6..535e421 100644
--- a/arch/x86/include/asm/uaccess_64.h
+++ b/arch/x86/include/asm/uaccess_64.h
@@ -30,16 +30,15 @@ static inline unsigned long __must_check copy_from_user(void *to,
unsigned long n)
{
int sz = __compiletime_object_size(to);
- int ret = -EFAULT;
might_fault();
if (likely(sz == -1 || sz >= n))
- ret = _copy_from_user(to, from, n);
+ n = _copy_from_user(to, from, n);
#ifdef CONFIG_DEBUG_VM
else
WARN(1, "Buffer overflow detected!\n");
#endif
- return ret;
+ return n;
}
static __always_inline __must_check
On Tue, 05 Jan 2010 18:27:18 +0100
Andi Kleen <[email protected]> wrote:
> Heiko Carstens <[email protected]> writes:
>
> > Subject: [PATCH] sparc: copy_from_user() should not return -EFAULT
> >
> > From: Heiko Carstens <[email protected]>
> >
> > Callers of copy_from_user() expect it to return the number of bytes
> > it could not copy. In no case it is supposed to return -EFAULT.
> >
> > In case of a detected buffer overflow just return the requested
> > length. In addition one could think of a memset that would clear
> > the size of the target object.
>
> Ouch! I would expect this is likely exploitable, e.g. in mount
yeah once you have the buffer overflow there might be another exploit
instead.. so yes needs to be fixed.
--
Arjan van de Ven Intel Open Source Technology Centre
For development, discussion and tips for power savings,
visit http://www.lesswatts.org
From: Heiko Carstens <[email protected]>
Date: Tue, 5 Jan 2010 16:22:15 +0100
> Subject: [PATCH] sparc: copy_from_user() should not return -EFAULT
>
> From: Heiko Carstens <[email protected]>
>
> Callers of copy_from_user() expect it to return the number of bytes
> it could not copy. In no case it is supposed to return -EFAULT.
>
> In case of a detected buffer overflow just return the requested
> length. In addition one could think of a memset that would clear
> the size of the target object.
>
> Signed-off-by: Heiko Carstens <[email protected]>
Applied, thanks again for catching this.
On Tuesday 05 January 2010, H. Peter Anvin wrote:
> What's much worse is that it adds churn to an otherwise-tested code path.
>
> We almost need a copy_from/to_user_audited() to override the warning.
> Not that errors can't creap back in...
>
Maybe just splitting it up into access_ok() and __copy_from_user(),
plus a comment is enough? That way we don't need to add another interface
for the rare case.
On a related topic, one interface that may actually be worth adding is
a get_user/put_user variant that can operate on full data structures
and return -EFAULT on failure rather than the number of remaining
bytes that 99% of the code never need.
Arnd
On 01/07/2010 06:02 AM, Arnd Bergmann wrote:
> On Tuesday 05 January 2010, H. Peter Anvin wrote:
>> What's much worse is that it adds churn to an otherwise-tested code path.
>>
>> We almost need a copy_from/to_user_audited() to override the warning.
>> Not that errors can't creap back in...
>>
>
> Maybe just splitting it up into access_ok() and __copy_from_user(),
> plus a comment is enough? That way we don't need to add another interface
> for the rare case.
>
Adding a named interface makes it clear *what* you are doing and
*why*... just open-coding the implementation does neither.
> On a related topic, one interface that may actually be worth adding is
> a get_user/put_user variant that can operate on full data structures
> and return -EFAULT on failure rather than the number of remaining
> bytes that 99% of the code never need.
What is wrong with checking for zero?
-hpa
On Friday 08 January 2010 00:57:51 H. Peter Anvin wrote:
> On 01/07/2010 06:02 AM, Arnd Bergmann wrote:
>
> > On a related topic, one interface that may actually be worth adding is
> > a get_user/put_user variant that can operate on full data structures
> > and return -EFAULT on failure rather than the number of remaining
> > bytes that 99% of the code never need.
>
> What is wrong with checking for zero?
It's counterintuitive. Everyone who is around long enough should know about
the copy_from_user calling conventions, but the fact that Arjan submitted
a patch returning EFAULT from copy_from_user and Ingo and Dave both added
this to their trees tells me that it's less than ideal.
Also, the calling conventions require you to write slightly more when
you want to pass down an error value, e.g.
return copy_to_user(uptr, &data, sizeof(data)) ? -EFAULT : 0;
instead of
return put_user(data, uptr);
The latter form requires a macro instead of a function for the user copy,
but we now have that anyway because of the size check.
Arnd
On 01/08/2010 04:07 PM, Arnd Bergmann wrote:
>
> Also, the calling conventions require you to write slightly more when
> you want to pass down an error value, e.g.
>
> return copy_to_user(uptr, &data, sizeof(data)) ? -EFAULT : 0;
>
> instead of
>
> return put_user(data, uptr);
>
> The latter form requires a macro instead of a function for the user copy,
> but we now have that anyway because of the size check.
>
Well... we already have the latter form?
-hpa
On Saturday 09 January 2010, H. Peter Anvin wrote:
> > return put_user(data, uptr);
> >
> > The latter form requires a macro instead of a function for the user copy,
> > but we now have that anyway because of the size check.
> >
>
> Well... we already have the latter form?
Right, but my suggestion was to extend that do data structures in
addition to the scalars we are supporting now.
Arnd
On 01/09/2010 12:01 AM, Arnd Bergmann wrote:
> On Saturday 09 January 2010, H. Peter Anvin wrote:
>>> return put_user(data, uptr);
>>>
>>> The latter form requires a macro instead of a function for the user copy,
>>> but we now have that anyway because of the size check.
>>>
>>
>> Well... we already have the latter form?
>
> Right, but my suggestion was to extend that do data structures in
> addition to the scalars we are supporting now.
>
> Arnd
Structures, as in "struct"? That would seem to be a good idea. Arrays,
which can be dynamically sized, are trickier.
-hpa
--
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel. I don't speak on their behalf.