| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20090926141525.GA540@elte.hu>
Date: Sat, 26 Sep 2009 16:15:25 +0200
From: Ingo Molnar <mingo@...e.hu>
To: Arjan van de Ven <arjan@...radead.org>
Cc: Linus Torvalds <torvalds@...ux-foundation.org>,
linux-kernel@...r.kernel.org, tglx@...x.de, hpa@...or.com
Subject: Re: [PATCH] x86: Use __builtin_object_size to validate the buffer
size for copy_from_user
* Arjan van de Ven <arjan@...radead.org> wrote:
> On Sat, 26 Sep 2009 14:41:51 +0200
> Ingo Molnar <mingo@...e.hu> wrote:
>
> >
> > * Arjan van de Ven <arjan@...radead.org> wrote:
> >
> > > From 524a1da3c45683cec77480acc6cab1d33ae8d5cb Mon Sep 17 00:00:00
> > > 2001 From: Arjan van de Ven <arjan@...ux.intel.com>
> > > Date: Sat, 26 Sep 2009 12:36:21 +0200
> > > Subject: [PATCH] x86: Use __builtin_object_size to validate the
> > > buffer size for copy_from_user
> > >
> > > gcc (4.x) supports the __builtin_object_size() builtin, which
> > > reports the size of an object that a pointer point to, when known
> > > at compile time. If the buffer size is not known at compile time, a
> > > constant -1 is returned.
> > >
> > > This patch uses this feature to add a sanity check to
> > > copy_from_user(); if the target buffer is known to be smaller than
> > > the copy size, the copy is aborted and a WARNing is emitted in
> > > memory debug mode.
> > >
> > > These extra checks compile away when the object size is not known,
> > > or if both the buffer size and the copy length are constants.
> > >
> > > Signed-off-by: Arjan van de Ven <arjan@...ux.intel.com>
> > > Reviewed-by: Ingo Molnar <mingo@...e.hu>
> > > ---
> > > arch/x86/include/asm/uaccess_32.h | 19 ++++++++++++++++++-
> > > arch/x86/include/asm/uaccess_64.h | 19 ++++++++++++++++++-
> > > arch/x86/kernel/x8664_ksyms_64.c | 2 +-
> > > arch/x86/lib/copy_user_64.S | 4 ++--
> > > arch/x86/lib/usercopy_32.c | 4 ++--
> > > include/linux/compiler-gcc4.h | 2 ++
> > > include/linux/compiler.h | 4 ++++
> > > 7 files changed, 47 insertions(+), 7 deletions(-)
> >
> > I have tested this on a buffer overflow and it caught it:
> >
> > [ 87.056952] ------------[ cut here ]------------
> > [ 87.061628] WARNING:
> > at /home/mingo/linux/arch/x86/include/asm/uaccess_64.h:35
> > sys_perf_counter_open+0x112/0x65b() [ 87.072600] Hardware name:
> > System Product Name [ 87.077072] Buffer overflow detected!
> > [ 87.080762] Modules linked in: [ 87.083858] Pid: 2670, comm:
> > exploit Not tainted 2.6.31 #17235 [ 87.089708] Call Trace:
> > [ 87.092180] [<ffffffff810a3241>] ?
> > sys_perf_counter_open+0x112/0x65b [ 87.098654]
> > [<ffffffff8104303c>] warn_slowpath_common+0x77/0xa4 [ 87.104684]
> > [<ffffffff810430b6>] warn_slowpath_fmt+0x3c/0x3e [ 87.110458]
> > [<ffffffff810e41c3>] ? putname+0x30/0x39 [ 87.115570]
> > [<ffffffff810a3241>] sys_perf_counter_open+0x112/0x65b
> > [ 87.121880] [<ffffffff8105b6df>] ? up_read+0x9/0xb
> > [ 87.126802] [<ffffffff8100ba6b>] system_call_fastpath+0x16/0x1b
> > [ 87.132851] ---[ end trace 7469dba2cd3cfea8 ]---
> >
> >
> > > +static inline unsigned long __must_check copy_from_user(void *to,
> > > + const void __user *from,
> > > + 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);
> > > +#ifdef CONFIG_DEBUG_VM
> > > + else
> > > + WARN(1, "Buffer overflow detected!\n");
> > > +#endif
> > > + return ret;
> > > +}
> >
> > This is pretty optimal in the !CONFIG_DEBUG_VM case. Would be nice to
> > see precisely how optimal - how many new instructions in the default
> > !CONFIG_DEBUG_VM case?
> >
>
> a test ->write method:
>
> static ssize_t test_write(struct file *fp, const char __user *buf,
> size_t len, loff_t *off)
> {
> char buffer[10];
> int ret;
>
> ret = copy_from_user(&buffer, buf, len);
>
> return ret;
> }
>
> with the patch turns into
>
> 0: 55 push %ebp
>
> * 1: b8 f2 ff ff ff mov $0xfffffff2,%eax
> 6: 89 e5 mov %esp,%ebp
> 8: 83 ec 0c sub $0xc,%esp
> * b: 83 f9 0a cmp $0xa,%ecx
> * e: 77 08 ja 18 <test_write+0x18>
> 10: 8d 45 f6 lea -0xa(%ebp),%eax
> 13: e8 fc ff ff ff call 14 <_copy_from_user>
> 18: c9 leave
> 19: c3 ret
>
> while without it gets
>
> 0: 55 push %ebp
> 1: 89 e5 mov %esp,%ebp
> 3: 83 ec 0c sub $0xc,%esp
> 6: 8d 45 f6 lea -0xa(%ebp),%eax
> 9: e8 fc ff ff ff call <copy_from_user>
> e: c9 leave
> f: c3 ret
>
> This is for the case where you have a known stack variable, but
> variable copy size.
> If you have either an unknown target size and/or a fixed sized copy,
> the code goes away entirely.
That's pretty convincing.
Linus, any objections?
Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists