[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Mon, 18 Jul 2011 23:33:38 +0400
From: Vasiliy Kulikov <segoon@...nwall.com>
To: kernel-hardening@...ts.openwall.com
Cc: Linus Torvalds <torvalds@...ux-foundation.org>,
Thomas Gleixner <tglx@...utronix.de>,
Ingo Molnar <mingo@...hat.com>, "H. Peter Anvin" <hpa@...or.com>,
x86@...nel.org, Arnd Bergmann <arnd@...db.de>,
Christoph Lameter <cl@...ux-foundation.org>,
Pekka Enberg <penberg@...nel.org>, Matt Mackall <mpm@...enic.com>,
linux-kernel@...r.kernel.org, linux-arch@...r.kernel.org,
linux-mm@...ck.org
Subject: Re: Re: [RFC v2] implement SL*B and stack
usercopy runtime checks
On Mon, Jul 18, 2011 at 11:52 -0700, Andrew Morton wrote:
> On Mon, 18 Jul 2011 22:39:51 +0400
> Vasiliy Kulikov <segoon@...nwall.com> wrote:
>
> > */
> > #define access_ok(type, addr, size) (likely(__range_not_ok(addr, size) == 0))
> >
> > +#if defined(CONFIG_FRAME_POINTER)
>
> #ifdef is conventional in this case
OK.
> > +/*
> > + * MUST be always_inline to correctly count stack frame numbers.
> > + *
> > + * low ----------------------------------------------> high
> > + * [saved bp][saved ip][args][local vars][saved bp][saved ip]
> > + * ^----------------^
> > + * allow copies only within here
> > +*/
> > +#undef arch_check_object_on_stack_frame
> > +inline static __attribute__((always_inline))
>
> static inline __always_inline
OK.
> > +bool arch_check_object_on_stack_frame(const void *stack,
> > + const void *stackend, const void *obj, unsigned long len)
> > +{
> > + const void *frame = NULL;
> > + const void *oldframe;
> > +
> > + /*
> > + * Get the kernel_access_ok() caller frame.
> > + * __builtin_frame_address(0) returns kernel_access_ok() frame
> > + * as arch_ and stack_ are inline and kernel_ is noinline.
> > + */
> > + oldframe = __builtin_frame_address(0);
> > + if (oldframe)
> > + frame = __builtin_frame_address(1);
> > +
> > + while (stack <= frame && frame < stackend) {
> > + /*
> > + * If obj + len extends past the last frame, this
> > + * check won't pass and the next frame will be 0,
> > + * causing us to bail out and correctly report
> > + * the copy as invalid.
> > + */
> > + if (obj + len <= frame) {
> > + /* EBP + EIP */
> > + int protected_regs_size = 2*sizeof(void *);
>
> size_t?
Yes, it looks better here.
> > +static inline unsigned long __must_check copy_from_user_uncheched(void *to,
>
> typo
Oops, sure.
> > diff --git a/arch/x86/include/asm/uaccess_64.h b/arch/x86/include/asm/uaccess_64.h
> > index 1c66d30..10c5a0a 100644
> > --- a/arch/x86/include/asm/uaccess_64.h
> > +++ b/arch/x86/include/asm/uaccess_64.h
> > @@ -50,8 +50,10 @@ static inline unsigned long __must_check copy_from_user(void *to,
> > int sz = __compiletime_object_size(to);
>
> size_t? (ssize_t?)
It doesn't touch my patch, however, ssize_t seems reasonable here.
> > might_fault();
> > - if (likely(sz == -1 || sz >= n))
> > - n = _copy_from_user(to, from, n);
> > + if (likely(sz == -1 || sz >= n)) {
> > + if (kernel_access_ok(to, n))
> > + n = _copy_from_user(to, from, n);
> > + }
> > #ifdef CONFIG_DEBUG_VM
> > else
> > WARN(1, "Buffer overflow detected!\n");
> >
> > ...
> >
> > --- a/mm/maccess.c
> > +++ b/mm/maccess.c
> > @@ -3,8 +3,11 @@
> > */
> > #include <linux/module.h>
> > #include <linux/mm.h>
> > +#include <linux/sched.h>
> > #include <linux/uaccess.h>
> >
> > +extern bool slab_access_ok(const void *ptr, unsigned long len);
>
> no externs in .c - use a header
I thought it would make less noise. OK, will do.
> > +noinline bool __kernel_access_ok(const void *ptr, unsigned long len)
>
> noinline seems unneeded
It is needed here because arch_check_object_on_stack_frame() needs the
precise number of frames it should skip.
Thank you!
--
Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
