Re: [PATCH v2] lkdtm: Add tests for LIST_POISON and ZERO_SIZE_PTR

[Kees Cook]

On Thu, Dec 1, 2016 at 8:22 PM, Michael Ellerman <mpe@xxxxxxxxxxxxxx> wrote:
> This adds two tests, to check that a read or write to LIST_POISON1 and
> ZERO_SIZE_PTR are blocked.
>
> The default values for both (256 and 16) typically fall in the range
> of valid user space addresses. However in general mmap_min_addr is 64K,
> which prevents user space from mapping anything at those addresses.
>
> However it's feasible that an attacker will be able to find a way to
> cause an access at an offset from either value, and if that offset is
> greater than 64K then they can access user space again.
>
> To simulate that case, in the test we create a user mapping at
> approximately mmap_min_addr, and offset the pointer by that amount. This
> gives the test the greatest chance of failing (ie. an access
> succeeding). We don't actually use mmap_min_addr, because that would
> require exporting it to modules, instead we use the default value at
> compile time as a reasonable approximation.
>
> Signed-off-by: Michael Ellerman <mpe@xxxxxxxxxxxxxx>

Thanks for this! I like it. :)

Acked-by: Kees Cook <keescook@xxxxxxxxxxxx>

Greg, can you take this into your tree?

-Kees

> ---
>  drivers/misc/lkdtm.h      |  2 ++
>  drivers/misc/lkdtm_bugs.c | 48 +++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/misc/lkdtm_core.c |  2 ++
>  3 files changed, 52 insertions(+)
>
> v2: Fix 32-bit compile errors by using int not long.
>     Avoid need to export mmap_min_addr by using CONFIG_DEFAULT_MMAP_MIN_ADDR.
>
> diff --git a/drivers/misc/lkdtm.h b/drivers/misc/lkdtm.h
> index fdf954c2107f..cc207f7824f9 100644
> --- a/drivers/misc/lkdtm.h
> +++ b/drivers/misc/lkdtm.h
> @@ -21,6 +21,8 @@ void lkdtm_SPINLOCKUP(void);
>  void lkdtm_HUNG_TASK(void);
>  void lkdtm_ATOMIC_UNDERFLOW(void);
>  void lkdtm_ATOMIC_OVERFLOW(void);
> +void lkdtm_ACCESS_LIST_POISON(void);
> +void lkdtm_ACCESS_ZERO_SIZE_PTR(void);
>
>  /* lkdtm_heap.c */
>  void lkdtm_OVERWRITE_ALLOCATION(void);
> diff --git a/drivers/misc/lkdtm_bugs.c b/drivers/misc/lkdtm_bugs.c
> index 30e62dd7e7ca..9ae079ac9a93 100644
> --- a/drivers/misc/lkdtm_bugs.c
> +++ b/drivers/misc/lkdtm_bugs.c
> @@ -5,7 +5,10 @@
>   * test source files.
>   */
>  #include "lkdtm.h"
> +#include <linux/mman.h>
>  #include <linux/sched.h>
> +#include <linux/security.h>
> +#include <linux/slab.h>
>
>  /*
>   * Make sure our attempts to over run the kernel stack doesn't trigger
> @@ -147,3 +150,48 @@ void lkdtm_ATOMIC_OVERFLOW(void)
>         pr_info("attempting bad atomic overflow\n");
>         atomic_inc(&over);
>  }
> +
> +static void test_poison_ptr(void *base, const char *desc)
> +{
> +       unsigned int *ptr, bias, val;
> +       unsigned long uaddr;
> +
> +       /* We'd rather not export mmap_min_addr, so use the default instead */
> +       bias = PAGE_ALIGN(CONFIG_DEFAULT_MMAP_MIN_ADDR);
> +
> +       uaddr = vm_mmap(NULL, bias, PAGE_SIZE, PROT_READ | PROT_WRITE,
> +                       MAP_ANONYMOUS | MAP_PRIVATE | MAP_FIXED, 0);
> +       if (uaddr >= TASK_SIZE) {
> +               pr_warn("Failed to allocate user memory, can't perform test.\n");
> +               return;
> +       }
> +
> +       /*
> +        * Creating a mapping and adding mmap_min_addr to the value is cheating
> +        * in a way. But it simulates the case where an attacker is able to
> +        * cause an access at a small offset from the base value, leading to a
> +        * user space access. If an arch doesn't define CONFIG_ILLEGAL_POINTER_VALUE
> +        * then it's likely this will work in the absence of other protections.
> +        */
> +       ptr = bias + base;
> +
> +       pr_info("attempting read of %s %p\n", desc, ptr);
> +       val = *ptr;
> +       pr_info("FAIL: Was able to read %s! Got 0x%x\n", desc, val);
> +
> +       pr_info("attempting write of %s %p\n", desc, ptr);
> +       *ptr = 0xdeadbeefU;
> +       pr_info("FAIL: Was able to write %s! Now = 0x%x\n", desc, *ptr);
> +
> +       vm_munmap(uaddr, PAGE_SIZE);
> +}
> +
> +void lkdtm_ACCESS_LIST_POISON(void)
> +{
> +       test_poison_ptr(LIST_POISON1, "LIST_POISON");
> +}
> +
> +void lkdtm_ACCESS_ZERO_SIZE_PTR(void)
> +{
> +       test_poison_ptr(ZERO_SIZE_PTR, "ZERO_SIZE_PTR");
> +}
> diff --git a/drivers/misc/lkdtm_core.c b/drivers/misc/lkdtm_core.c
> index f9154b8d67f6..025a0ee8d8ee 100644
> --- a/drivers/misc/lkdtm_core.c
> +++ b/drivers/misc/lkdtm_core.c
> @@ -220,6 +220,8 @@ struct crashtype crashtypes[] = {
>         CRASHTYPE(WRITE_KERN),
>         CRASHTYPE(ATOMIC_UNDERFLOW),
>         CRASHTYPE(ATOMIC_OVERFLOW),
> +       CRASHTYPE(ACCESS_LIST_POISON),
> +       CRASHTYPE(ACCESS_ZERO_SIZE_PTR),
>         CRASHTYPE(USERCOPY_HEAP_SIZE_TO),
>         CRASHTYPE(USERCOPY_HEAP_SIZE_FROM),
>         CRASHTYPE(USERCOPY_HEAP_FLAG_TO),
> --
> 2.7.4
>



-- 
Kees Cook
Nexus Security