linux-kernel - Re: [PATCH v2 2/2] kasan: add kasan_double

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CACT4Y+Zwv6J+8ovnXAb4EbsHWf4J-8cKr-h25Ucxq5T3kjSn=A@mail.gmail.com>
Date:	Mon, 9 May 2016 07:35:08 +0200
From:	Dmitry Vyukov <dvyukov@...gle.com>
To:	Kuthonuzo Luruo <kuthonuzo.luruo@....com>
Cc:	Andrey Ryabinin <aryabinin@...tuozzo.com>,
	Alexander Potapenko <glider@...gle.com>,
	Christoph Lameter <cl@...ux.com>,
	Pekka Enberg <penberg@...nel.org>,
	David Rientjes <rientjes@...gle.com>,
	Joonsoo Kim <iamjoonsoo.kim@....com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	kasan-dev <kasan-dev@...glegroups.com>,
	LKML <linux-kernel@...r.kernel.org>,
	"linux-mm@...ck.org" <linux-mm@...ck.org>
Subject: Re: [PATCH v2 2/2] kasan: add kasan_double_free() test

On Fri, May 6, 2016 at 1:50 PM, Kuthonuzo Luruo <kuthonuzo.luruo@....com> wrote:
> This patch adds a new 'test_kasan' test for KASAN double-free error
> detection when the same slab object is concurrently deallocated.
>
> Signed-off-by: Kuthonuzo Luruo <kuthonuzo.luruo@....com>
> ---
> Changes in v2:
> - This patch is new for v2.
> ---
>  lib/test_kasan.c |   79 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 79 insertions(+), 0 deletions(-)
>
> diff --git a/lib/test_kasan.c b/lib/test_kasan.c
> index bd75a03..dec5f74 100644
> --- a/lib/test_kasan.c
> +++ b/lib/test_kasan.c
> @@ -16,6 +16,7 @@
>  #include <linux/slab.h>
>  #include <linux/string.h>
>  #include <linux/module.h>
> +#include <linux/kthread.h>
>
>  static noinline void __init kmalloc_oob_right(void)
>  {
> @@ -389,6 +390,83 @@ static noinline void __init ksize_unpoisons_memory(void)
>         kfree(ptr);
>  }
>
> +#ifdef CONFIG_SLAB
> +#ifdef CONFIG_SMP

Will it fail without CONFIG_SMP if we create more than 1 kthread? If
it does not fail, then please remove the ifdef.
Also see below.


> +static DECLARE_COMPLETION(starting_gun);
> +static DECLARE_COMPLETION(finish_line);
> +
> +static int try_free(void *p)
> +{
> +       wait_for_completion(&starting_gun);
> +       kfree(p);
> +       complete(&finish_line);
> +       return 0;
> +}
> +
> +/*
> + * allocs an object; then all cpus concurrently attempt to free the
> + * same object.
> + */
> +static noinline void __init kasan_double_free(void)
> +{
> +       char *p;
> +       int cpu;
> +       struct task_struct **tasks;
> +       size_t size = (KMALLOC_MAX_CACHE_SIZE/4 + 1);

Is it important to use such tricky size calculation here? If it is not
important, then please replace it with some small constant.
There are some tests that calculate size based on
KMALLOC_MAX_CACHE_SIZE, but that's important for them.



> +       /*
> +        * max slab size instrumented by KASAN is KMALLOC_MAX_CACHE_SIZE/2.
> +        * Do not increase size beyond this: slab corruption from double-free
> +        * may ensue.
> +        */
> +       pr_info("concurrent double-free test\n");
> +       init_completion(&starting_gun);
> +       init_completion(&finish_line);
> +       tasks = kzalloc((sizeof(tasks) * nr_cpu_ids), GFP_KERNEL);
> +       if (!tasks) {
> +               pr_err("Allocation failed\n");
> +               return;
> +       }
> +       p = kmalloc(size, GFP_KERNEL);
> +       if (!p) {
> +               pr_err("Allocation failed\n");
> +               return;
> +       }
> +
> +       for_each_online_cpu(cpu) {


Won't the test fail with 1 cpu?
By failing I mean that it won't detect the double-free. Soon we will
start automatically ensuring that a double-free test in fact detects a
double-free.
I think it will be much simpler to use just, say, 4 threads. It will
eliminate kzmalloc, kfree, allocation failure tests, memory leaks and
also fix !CONFIG_SMP.



> +               tasks[cpu] = kthread_create(try_free, (void *)p, "try_free%d",
> +                               cpu);
> +               if (IS_ERR(tasks[cpu])) {
> +                       WARN(1, "kthread_create failed.\n");
> +                       return;
> +               }
> +               kthread_bind(tasks[cpu], cpu);
> +               wake_up_process(tasks[cpu]);
> +       }
> +
> +       complete_all(&starting_gun);
> +       for_each_online_cpu(cpu)
> +               wait_for_completion(&finish_line);
> +       kfree(tasks);
> +}
> +#else
> +static noinline void __init kasan_double_free(void)

This test should work with CONFIG_SLAB as well.
Please name the tests differently (e.g. kasan_double_free and
kasan_double_free_threaded), and run kasan_double_free always.
If kasan_double_free_threaded fails, but kasan_double_free does not,
that's already some useful info. And if both fail, then it's always
better to have a simpler reproducer.


> +{
> +       char *p;
> +       size_t size = 2049;
> +
> +       pr_info("double-free test\n");
> +       p = kmalloc(size, GFP_KERNEL);
> +       if (!p) {
> +               pr_err("Allocation failed\n");
> +               return;
> +       }
> +       kfree(p);
> +       kfree(p);
> +}
> +#endif
> +#endif
> +
>  static int __init kmalloc_tests_init(void)
>  {
>         kmalloc_oob_right();
> @@ -414,6 +492,7 @@ static int __init kmalloc_tests_init(void)
>         kasan_global_oob();
>  #ifdef CONFIG_SLAB
>         kasan_quarantine_cache();
> +       kasan_double_free();
>  #endif
>         ksize_unpoisons_memory();
>         return -EAGAIN;
> --
> 1.7.1
>