Re: [kernel-hardening] Re: [RFC v2] implement SL*B and stack usercopy runtime checks

Vasiliy Kulikov
2011-07-18T19:33:38Z

On Mon, Jul 18, 2011 at 11:52 -0700, Andrew Morton wrote:
> On Mon, 18 Jul 2011 22:39:51 +0400
> Vasiliy Kulikov <segoon@openwall.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