Message-Id: <1247212107.23169.25.camel@penberg-laptop>
Date:	Fri, 10 Jul 2009 10:48:27 +0300
From:	Pekka Enberg <penberg@...helsinki.fi>
To:	Janboe Ye <yuan-bo.ye@...orola.com>
Cc:	linux-arm-kernel@...ts.arm.linux.org.uk,
	linux-kernel <linux-kernel@...r.kernel.org>,
	vegard.nossum@...il.com, graydon@...hat.com, fche@...hat.com,
	mingo@...e.hu, cl@...ux-foundation.org
Subject: Re: [RFC][PATCH] Check write to slab memory which freed already
 using mudflap v2

Hi Janboe!

On Thu, 2009-07-09 at 18:26 -0500, Janboe Ye wrote:
> +#include<linux/kernel.h>
> +
> +#define LOOKUP_CACHE_MASK_DFL 1023
> +#define LOOKUP_CACHE_SIZE_MAX 65536 /* Allows max CACHE_MASK 0xFFFF */
> +#define LOOKUP_CACHE_SHIFT_DFL 2

These are probably too generic names if this is going to be included by
default. Maybe you could just add a KMUDFLAP prefix or something?

> +
> +typedef void *__mf_ptr_t;
> +typedef unsigned int __mf_uintptr_t;
> +struct __mf_cache { __mf_uintptr_t low; __mf_uintptr_t high; };
> +
> +void __mf_check(void *ptr, unsigned int sz, int type, const char *location);
> +void __mf_register(void *ptr, size_t sz, int type, const char *name);
> +void __mf_init(void);
> +void __mf_unregister(void *ptr, size_t sz, int type);
> +
> +#ifdef _MUDFLAP
> +#pragma redefine_extname memcpy __mfwrap_memcpy
> +#pragma redefine_extname memset __mfwrap_memset
> +#pragma redefine_extname strcpy __mfwrap_strcpy
> +#pragma redefine_extname strncpy __mfwrap_strncpy
> +
> +#ifdef CONFIG_ARM
> +#pragma redefine_extname __memzero __mfwrap_memzero
> +#endif
> +
> +#endif /* _MUDFLAP */
> +
> +#endif
> diff --git a/kernel/Makefile b/kernel/Makefile
> index 2093a69..c4c2402 100644
> --- a/kernel/Makefile
> +++ b/kernel/Makefile
> @@ -22,7 +22,7 @@ CFLAGS_REMOVE_rtmutex-debug.o = -pg
>  CFLAGS_REMOVE_cgroup-debug.o = -pg
>  CFLAGS_REMOVE_sched_clock.o = -pg
>  endif
> -
> +obj-$(CONFIG_MUDFLAP) += mudflap.o
>  obj-$(CONFIG_FREEZER) += freezer.o
>  obj-$(CONFIG_PROFILING) += profile.o
>  obj-$(CONFIG_SYSCTL_SYSCALL_CHECK) += sysctl_check.o
> diff --git a/kernel/mudflap.c b/kernel/mudflap.c
> new file mode 100644
> index 0000000..5734dff
> --- /dev/null
> +++ b/kernel/mudflap.c
> @@ -0,0 +1,132 @@
> +/*
> + * kernel/mudflap.c
> + *
> + * Copyright (C) 2009 Motorola Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * General Public License for more details.
> + */
> +
> +#include<linux/kernel.h>
> +#include<linux/slab.h>
> +#include<linux/module.h>
> +#include<linux/mm.h>
> +#undef DEBUG
> +
> +#ifdef DEBUG
> +#define DPRINTK(fmt, args...) printk(fmt, ## args)
> +#else
> +#define DPRINTK(fmt, args...)
> +#endif

This is core kernel code so this DEBUG thing needs to go. Check the
previous email for suggestions how to do it.

> +
> +/* Codes to describe the type of access to check */
> +#define __MF_CHECK_READ 0
> +#define __MF_CHECK_WRITE 1
> +
> +typedef void *__mf_ptr_t;
> +typedef unsigned int __mf_uintptr_t;
> +struct __mf_cache { __mf_uintptr_t low; __mf_uintptr_t high; };
> +
> +#define LOOKUP_CACHE_MASK_DFL 1023
> +#define LOOKUP_CACHE_SIZE_MAX 65536 /* Allows max CACHE_MASK 0xFFFF */
> +#define LOOKUP_CACHE_SHIFT_DFL 2

Why are we redefining these here?

> +
> +struct __mf_cache __mf_lookup_cache[LOOKUP_CACHE_SIZE_MAX];
> +EXPORT_SYMBOL(__mf_lookup_cache);
> +
> +uintptr_t __mf_lc_mask = LOOKUP_CACHE_MASK_DFL;
> +EXPORT_SYMBOL(__mf_lc_mask);
> +
> +unsigned char __mf_lc_shift = LOOKUP_CACHE_SHIFT_DFL;
> +EXPORT_SYMBOL(__mf_lc_shift);
> +
> +void check_slab_write(void *ptr, unsigned int sz, const char *location);

This needs to go into <linux/slab_def.h> and <linux/slub_def.h> and
probably renamed to "slab_check_write()"

> +static inline int verify_ptr(unsigned long ptr)
> +{
> +	if (ptr < PAGE_OFFSET ||
> +		(ptr > (unsigned long)high_memory && high_memory != 0))

s/0/NULL/

> +		return -EFAULT;
> +
> +	return 0;
> +}

OTOH, can't we use virt_addr_valid() instead? It should be safe to do
virt_to_head_page() if it passes and the PageSlab check in
check_slab_write() will take care of the rest.

Furthermore, you could just move all this processing to
check_slab_write() and nuke verity_ptr() completely.

> +
> +void __mf_check(void *ptr, unsigned int sz, int type, const char *location)
> +{
> +	if (verify_ptr((unsigned long)ptr))
> +		return;
> +	if (type) /* write */
> +		check_slab_write(ptr, sz, location);
> +}
> +EXPORT_SYMBOL(__mf_check);
> +
> +void *__mfwrap_memset(void *s, int c, size_t count)

Is the __mfwrap prefix mandated by mudflap? Can we use kmudflap or
similar instead?

> +{
> +	__mf_check(s, count, __MF_CHECK_WRITE, "memset dest");
> +	return memset(s, c, count);
> +}
> +EXPORT_SYMBOL(__mfwrap_memset);
> +
> +void *__mfwrap_memcpy(void *dest, const void *src, size_t count)
> +{
> +	__mf_check(dest, count, __MF_CHECK_WRITE, "memcpy dest");
> +	return memcpy(dest, src, count);
> +}
> +EXPORT_SYMBOL(__mfwrap_memcpy);
> +
> +char *__mfwrap_strcpy(char *dest, const char *src)
> +{
> +	size_t n = strlen(src);
> +	 __mf_check(dest, n, __MF_CHECK_WRITE, "strcpy dest");
> +	return strcpy(dest, src);
> +}
> +EXPORT_SYMBOL(__mfwrap_strcpy);
> +
> +void *__mfwrap_strncpy(char *dest, const char *src, size_t count)
> +{
> +	size_t len = strnlen(src, count);
> +	 __mf_check(dest, len, __MF_CHECK_WRITE, "strncpy dest");
> +	return strncpy(dest, src, count);
> +}
> +EXPORT_SYMBOL(__mfwrap_strncpy);
> +
> +#ifdef CONFIG_ARM

Like I said, this doesn't really belong into generic kernel code. Can we
just move this function under arch/arm/kernel?

> +void __mfwrap_memzero(void *ptr, __kernel_size_t n)
> +{
> +	__mf_check(ptr, n, __MF_CHECK_WRITE, "memzero dest");
> +	return __memzero(ptr, n);
> +}
> +EXPORT_SYMBOL(__mfwrap_memzero);
> +#endif
> +
> +void
> +__mf_register(void *ptr, size_t sz, int type, const char *name)
> +{
> +	DPRINTK("ptr: %p, sz: %d, type: %d, %s\n", ptr, sz, type, name);
> +}
> +EXPORT_SYMBOL(__mf_register);
> +
> +void
> +__mf_unregister(void *ptr, size_t sz, int type)
> +{
> +	DPRINTK("ptr: %p, sz: %d, type: %d\n", ptr, sz, type);
> +}
> +EXPORT_SYMBOL(__mf_unregister);
> +
> +void __mf_init(void)
> +{
> +}
> +EXPORT_SYMBOL(__mf_init);
> +
> +int
> +__mf_set_options(const char *optstr)
> +{
> +	return 0;
> +}
> +EXPORT_SYMBOL(__mf_set_options);
> diff --git a/mm/slab.c b/mm/slab.c
> index 7b5d4de..f4433d4 100644
> --- a/mm/slab.c
> +++ b/mm/slab.c
> @@ -4503,3 +4503,41 @@ size_t ksize(const void *objp)
>  	return obj_size(virt_to_cache(objp));
>  }
>  EXPORT_SYMBOL(ksize);
> +
> +#ifdef CONFIG_MUDFLAP
> +void check_slab_write(void *ptr, unsigned int sz, const char *location)

s/sz/size/ and use size_t

> +{
> +	struct page *page;
> +	struct kmem_cache *cachep;
> +	struct slab *slabp;
> +	char *realobj;
> +	void *objp;
> +	int objnr, size;

s/cachep/cache/ and same for slabp and objp too, please.

> +
> +	page = virt_to_head_page(ptr);
> +	if (!PageSlab(page))
> +		return;
> +
> +	cachep = page_get_cache(page);
> +	slabp = page_get_slab(page);
> +	objnr = (unsigned)(ptr - slabp->s_mem) / cachep->buffer_size;

Cast to unsigned long and make obnr unsigned long too?

> +	objp = index_to_obj(cachep, slabp, objnr);
> +	realobj = (char *)objp + obj_offset(cachep);

The char pointer casting looks unnecessary here.

> +	size = obj_size(cachep);
> +	if (slab_bufctl(slabp)[objnr] == BUFCTL_FREE) {

You could do:

  if (slab_bufctl(slabp)[objnr] != BUFCTL_FREE)
          return;

to reduce nesting here.

> +		printk(KERN_ERR "ptr: %p, sz: %d, location: %s\n",
> +				ptr, sz, location);
> +		printk(KERN_ERR "write to freed object, size %d\n",
> +				size);
> +		if (cachep->flags & SLAB_STORE_USER) {
> +			printk(KERN_ERR "Last user: [<%p>]",
> +					*dbg_userword(cachep, objp));
> +			print_symbol("(%s)",
> +				(unsigned long)*dbg_userword(cachep, objp));
> +			printk("\n");
> +		}

It would be nice to clean this up a bit. See below for comments on how
to do it for SLUB.

> +
> +		dump_stack();
> +	}
> +}

don't we need

  EXPORT_SYMBOL(check_slab_write);

here?

> +#endif
> diff --git a/mm/slub.c b/mm/slub.c
> index a9201d8..b3fdd93 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -4729,3 +4729,32 @@ static int __init slab_proc_init(void)
>  }
>  module_init(slab_proc_init);
>  #endif /* CONFIG_SLABINFO */
> +
> +#ifdef CONFIG_MUDFLAP
> +void check_slab_write(void *ptr, unsigned int sz, const char *location)
> +{

s/sz/size/g and use size_t

> +	struct page *page;
> +	struct kmem_cache *cachep;

  struct kmem_cache *s;

please

> +	struct slab *slabp;

Hmm? SLUB has no "struct slab"

> +	char *realobj;
> +	void *objp, *start;

s/objp/obj/

> +	int objnr, size;
> +
> +	page = virt_to_head_page(ptr);
> +	if (!PageSlab(page))
> +		return;
> +
> +	cachep = page->slab;
> +	start = page_address(page);
> +	objnr = (unsigned)(ptr - start) / cachep->size;

This looks wee bit fishy to me. You probably want to cast to "(unsigned
long)" and make "objnr" unsigned long.

> +	objp = start + cachep->size * objnr;
> +	if (on_freelist(cachep, page, objp)) {

You could do

  if (!on_freelist(cachep, page, objp))
       return;

to reduce nesting here.

> +		printk(KERN_ERR "ptr: %p, sz: %d, location: %s\n",
> +				ptr, sz, location);
> +		printk(KERN_ERR "write to freed object, size %d\n",
> +				cachep->size);

Can we make this error message more readable? Something like

  printk(KERN_ERR "%s: Write to free'd object %p (size=%d) in %s\n",
         s->name, ptr, size, location);

> +		print_tracking(cachep, objp);
> +		dump_stack();
> +	}
> +}

don't we need

  EXPORT_SYMBOL(check_slab_write);

here?

> +#endif

			Pekka

--
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