[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <1463685359.16149.35.camel@redhat.com>
Date: Thu, 19 May 2016 15:15:59 -0400
From: Rik van Riel <riel@...hat.com>
To: kernel-hardening@...ts.openwall.com
Cc: Casey Schaufler <casey.schaufler@...el.com>
Subject: Re: [PATCH] Subject: [RFC PATCH] mm: Hardened
usercopy
On Thu, 2016-05-19 at 09:14 -0700, casey.schaufler@...el.com wrote:
I have a bunch of nitpicks for this patch, mostly
to do with code readability and documentation.
> +++ b/arch/x86/include/asm/uaccess_32.h
> @@ -43,6 +43,9 @@ unsigned long __must_check
> __copy_from_user_ll_nocache_nozero
> static __always_inline unsigned long __must_check
> __copy_to_user_inatomic(void __user *to, const void *from, unsigned
> long n)
> {
> +#ifdef CONFIG_HARDUSERCOPY
> + check_object_size(from, n, true);
> +#endif
> if (__builtin_constant_p(n)) {
> unsigned long ret;
Would it be possible to leave out those ifdefs,
and move all the ifdeffery to the include file?
+++ b/include/linux/something.h
#ifdef CONFIG_HARDUSERCOPY
... what you have now
#else
static inline void check_object_size(const void *ptr, unsigned long n,
bool to_user) { }
#endif
> +++ b/fs/exec.c
> @@ -1749,3 +1749,128 @@ COMPAT_SYSCALL_DEFINE5(execveat, int, fd,
> argv, envp, flags);
> }
> #endif
> +
> +#ifdef CONFIG_HARDUSERCOPY
> +/*
Maybe add "Check whether an object is inside the stack" ?
> + * 0: not at all,
> + * 1: fully,
> + * 2: fully inside frame,
> + * -1: partially (implies an error)
> + */
> +static noinline int check_stack_object(const void *obj, unsigned
> long len)
> +{
> + const void * const stack = task_stack_page(current);
> + const void * const stackend = stack + THREAD_SIZE;
> +
> +#if defined(CONFIG_FRAME_POINTER) && defined(CONFIG_X86)
> + const void *frame = NULL;
> + const void *oldframe;
> +#endif
> +
> + if (obj + len < obj)
> + return -1;
> +
> + if (obj + len <= stack || stackend <= obj)
> + return 0;
> +
> + if (obj < stack || stackend < obj + len)
> + return -1;
> +
> +#if defined(CONFIG_FRAME_POINTER) && defined(CONFIG_X86)
> + oldframe = __builtin_frame_address(1);
> + if (oldframe)
> + frame = __builtin_frame_address(2);
> + /*
> + low ----------------------------------------------> high
> + [saved bp][saved ip][args][local vars][saved bp][saved ip]
> + ^----------------^
> + allow copies only within here
> + */
> + 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)
> + return obj >= oldframe + 2 * sizeof(void *)
> ? 2 : -1;
> + oldframe = frame;
> + frame = *(const void * const *)frame;
> + }
> + return -1;
> +#else
> + return 1;
> +#endif
> +}
> +static inline bool check_kernel_text_object(unsigned long low,
> + unsigned long high)
> +{
> + unsigned long textlow = (unsigned long)_stext;
> + unsigned long texthigh = (unsigned long)_etext;
> +
> +#ifdef CONFIG_X86_64
> + /* check against linear mapping as well */
> + if (high > (unsigned long)__va(__pa(textlow)) &&
> + low < (unsigned long)__va(__pa(texthigh)))
> + return true;
> +#endif
> +
> + if (high <= textlow || low >= texthigh)
> + return false;
> + else
> + return true;
Could this be simplified to the following?
/* Object overlaps with kernel text */
return (high > textlow && low < texthigh);
The following function could use a bunch of documentation,
too.
/* Is ptr a valid location for copy_from_user of size n? */
> +void __check_object_size(const void *ptr, unsigned long n, bool
> to_user,
> + bool const_size)
> +{
> + const char *type;
> +
> +#if !defined(CONFIG_STACK_GROWSUP) && !defined(CONFIG_X86_64)
> + unsigned long stackstart = (unsigned
> long)task_stack_page(current);
> + unsigned long currentsp = (unsigned long)&stackstart;
> + if (unlikely((currentsp < stackstart + 512 ||
> + currentsp >= stackstart + THREAD_SIZE) &&
> !in_interrupt()))
> + BUG();
> +#endif
> +
> + if (!n)
> + return;
> +
> + type = check_heap_object(ptr, n);
Maybe rename "type" to "err", to indicate that if
check_heap_object returned non-zero, there already
is an error detected, and it makes no sense to check
the rest?
The following seems a little convoluted. This appears
to be for good reasons, but could use some comments.
> + if (!type) {
> + int ret = check_stack_object(ptr, n);
> + if (ret == 1 || ret == 2)
> + return;
> + if (ret == 0) {
/*
* Object is not on the stack. Make sure
* it is not inside the kernel text.
*/
> + if (check_kernel_text_object((unsigned
> long)ptr,
> + (unsigned long)ptr +
> n))
Why does check_kernel_text_object() not take "ptr, n" as
its arguments, like all the other functions here?
Some consistency may make it easier for people to see
what is going on.
> + type = "<kernel text>";
> + else
/* Valid copy_from_user object */
> + return;
> + } else
> + type = "<process stack>";
> + }
> +
> + report_hardusercopy(ptr, n, to_user, type);
> +
> +}
>
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index b7b9501..048eea8 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -757,6 +757,9 @@ struct signal_struct {
> #ifdef CONFIG_TASKSTATS
> struct taskstats *stats;
> #endif
> +#ifdef CONFIG_HARDUSERCOPY
> + u32 curr_ip;
> +#endif
This is not big enough to hold an instruction
pointer on a 64 bit architecture.
However, it also appears to be unused in your
patch, so you may be able to just get rid of it,
and the code that checks for it in report_hardusercopy()
> +++ b/include/linux/thread_info.h
> @@ -145,6 +145,17 @@ static inline bool
> test_and_clear_restore_sigmask(void)
> #error "no set_restore_sigmask() provided and default one won't
> work"
> #endif
>
> +#ifdef CONFIG_HARDUSERCOPY
> +extern void __check_object_size(const void *ptr, unsigned long n,
> + bool to_user, bool
> const_size);
> +
> +static inline void check_object_size(const void *ptr, unsigned long
> n,
> + bool to_user)
> +{
> + __check_object_size(ptr, n, to_user,
> __builtin_constant_p(n));
> +}
#else
static inline void check_object_size(const void *ptr, unsigned long
n,
bool to_user) {}
> +#endif /* CONFIG_HARDUSERCOPY */
> +
> #endif /* __KERNEL__ */
>
> #endif /* _LINUX_THREAD_INFO_H */
Here is another function that could use some documentation.
I wrote up a comment documenting what it does, but I am not
sure why copy_from_user is not allowed to vmalloced kernel
objects. Is there a policy reason for this, or is it just a
happy coincidence that it does not break anything currently?
> +#ifdef CONFIG_HARDUSERCOPY
/*
* Ensure that ptr is a valid copy_to_user destination.
* Disallow copies to slab caches without SLAB_USERCOPY
* set, that overflow slab objects, or to vmalloc areas.
* There are additional checks in __check_object_size for
* errors not detected here.
*/
> +const char *check_heap_object(const void *ptr, unsigned long n)
> +{
> + struct page *page;
> + struct kmem_cache *cachep;
> + unsigned int objnr;
> + unsigned long offset;
> +
> + if (ZERO_OR_NULL_PTR(ptr))
> + return "<null>";
> +
> + if (!virt_addr_valid(ptr))
> + return NULL;
> +
> + page = virt_to_head_page(ptr);
> +
> + if (!PageSlab(page))
> + return NULL;
> +
> + cachep = page->slab_cache;
> + if (!(cachep->flags & SLAB_USERCOPY))
> + return cachep->name;
> +
> + objnr = obj_to_index(cachep, page, ptr);
> + BUG_ON(objnr >= cachep->num);
> + offset = ptr - index_to_obj(cachep, page, objnr) -
> obj_offset(cachep);
> + if (offset <= cachep->object_size && n <= cachep-
> >object_size - offset)
> + return NULL;
> +
> + return cachep->name;
> +}
> +#endif /* CONFIG_HARDUSERCOPY */
> +
> /**
> * ksize - get the actual amount of memory allocated for a given
> object
> * @objp: Pointer to the object
> diff --git a/mm/slab_common.c b/mm/slab_common.c
> index 5ce4fae..655cc17d 100644
> --- a/mm/slab_common.c
> +++ b/mm/slab_common.c
> @@ -43,7 +43,11 @@ struct kmem_cache *kmem_cache;
> * Merge control. If this is set then no merging of slab caches will
> occur.
> * (Could be removed. This was introduced to pacify the merge
> skeptics.)
> */
> +#ifdef CONFIG_HARDUSERCOPY_SLABS
> +static int slab_nomerge = 1;
> +#else
> static int slab_nomerge;
> +#endif
This disables merging of all slabs just because there
may be a few slabs with SLAB_USERCOPY set. This seems
like overkill.
Simply adding SLAB_USERCOPY to "SLAB_MERGE_SAME" should
prevent the merging of slabs with SLAB_USERCOPY set with
slab caches that do not have that flag set, and should
result in the same security benefits without losing
functionality.
The following could use a comment:
> @@ -805,6 +814,11 @@ struct kmem_cache *kmalloc_slab(size_t size,
> gfp_t flags)
> return kmalloc_dma_caches[index];
>
> #endif
> +#ifdef CONFIG_HARDUSERCOPY_SLABS
/* Separate slab caches for copy_from_user */
> + if (unlikely((flags & GFP_USERCOPY)))
> + return kmalloc_usercopy_caches[index];
> +#endif /* CONFIG_HARDUSERCOPY_SLABS */
> +
> return kmalloc_caches[index];
> }
The code added to slub.c and slob.c could use similar
commenting to the slab.c comments above.
> +++ b/mm/slub.c
> @@ -3475,6 +3475,36 @@ void *__kmalloc_node(size_t size, gfp_t flags,
> int node)
> EXPORT_SYMBOL(__kmalloc_node);
> #endif
>
> +#ifdef CONFIG_HARDUSERCOPY
> +const char *check_heap_object(const void *ptr, unsigned long n)
> +{
> + struct page *page;
> + struct kmem_cache *s;
> + unsigned long offset;
> +
> + if (ZERO_OR_NULL_PTR(ptr))
> + return "<null>";
> +
> + if (!virt_addr_valid(ptr))
> + return NULL;
> +
> + page = virt_to_head_page(ptr);
Wait a moment. Everything above the
if (!PageSlab(page)) code is identical between
slab, slob, and slub.
Would it make sense to move that common code into
__check_object_size, and have a check_slab_object
check that is called if (PageSlab(page)) ?
That way slab.c, slob.c, and slub.c only have their
own unique code, and not this replication between them.
Having the common code in __check_object_size is likely
to end up more readable, too.
> + if (!PageSlab(page))
> + return NULL;
> +
> + s = page->slab_cache;
Additionally, having the SLAB_USERCOPY flag set on any
slab caches at all is a function of CONFIG_HARDUSERCOPY_SLABS.
By not having the check below under an ifdef, does the code
always disallow copy_from_user to slab caches when that
config option is not set?
In other words, does disabling CONFIG_HARDUSERCOPY_SLABS
result in stricter controls (and possibly a brokne system)?
> + if (!(s->flags & SLAB_USERCOPY))
> + return s->name;
> +
> + offset = (ptr - page_address(page)) % s->size;
> + if (offset <= s->object_size && n <= s->object_size -
> offset)
> + return NULL;
> +
> + return s->name;
> +}
> +#endif /* CONFIG_HARDUSERCOPY */
> +
> static size_t __ksize(const void *object)
> {
> struct page *page;
--
All rights reversed
Download attachment "signature.asc" of type "application/pgp-signature" (474 bytes)
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.
