Path: csiph.com!news.redatomik.org!aioe.org!bofh.it!news.nic.it!robomod
From: Kees Cook <keescook@chromium.org>
Newsgroups: linux.kernel
Subject: Re: [PATCH v6 11/11] lkdtm: Add test for XPFO
Date: Thu, 07 Sep 2017 21:10:02 +0200
Message-ID: <unalA-4i2-3@gated-at.bofh.it>
References: <un8Wu-3e6-13@gated-at.bofh.it> <un8Wu-3e6-21@gated-at.bofh.it>
MIME-Version: 1.0
Content-Type: text/plain; charset="UTF-8"
Sender: robomod@news.nic.it
Approved: robomod@news.nic.it
Lines: 291
Organization: linux.* mail to news gateway
Xref: csiph.com linux.kernel:1728393

On Thu, Sep 7, 2017 at 10:36 AM, Tycho Andersen <tycho@docker.com> wrote:
> From: Juerg Haefliger <juerg.haefliger@canonical.com>
>
> This test simply reads from userspace memory via the kernel's linear
> map.
>
> v6: * drop an #ifdef, just let the test fail if XPFO is not supported
>     * add XPFO_SMP test to try and test the case when one CPU does an xpfo
>       unmap of an address, that it can't be used accidentally by other
>       CPUs.

This is very close! Thanks for the updates. :) Notes below...

>
> Signed-off-by: Juerg Haefliger <juerg.haefliger@canonical.com>
> Signed-off-by: Tycho Andersen <tycho@docker.com>
> Tested-by: Marco Benatto <marco.antonio.780@gmail.com>
> ---
>  drivers/misc/Makefile     |   1 +
>  drivers/misc/lkdtm.h      |   5 ++
>  drivers/misc/lkdtm_core.c |   3 +
>  drivers/misc/lkdtm_xpfo.c | 194 ++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 203 insertions(+)
>
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index b0b766416306..8447b42a447d 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -62,6 +62,7 @@ lkdtm-$(CONFIG_LKDTM)         += lkdtm_heap.o
>  lkdtm-$(CONFIG_LKDTM)          += lkdtm_perms.o
>  lkdtm-$(CONFIG_LKDTM)          += lkdtm_rodata_objcopy.o
>  lkdtm-$(CONFIG_LKDTM)          += lkdtm_usercopy.o
> +lkdtm-$(CONFIG_LKDTM)          += lkdtm_xpfo.o
>
>  KCOV_INSTRUMENT_lkdtm_rodata.o := n
>
> diff --git a/drivers/misc/lkdtm.h b/drivers/misc/lkdtm.h
> index 3b4976396ec4..34a6ee37f216 100644
> --- a/drivers/misc/lkdtm.h
> +++ b/drivers/misc/lkdtm.h
> @@ -64,4 +64,9 @@ void lkdtm_USERCOPY_STACK_FRAME_FROM(void);
>  void lkdtm_USERCOPY_STACK_BEYOND(void);
>  void lkdtm_USERCOPY_KERNEL(void);
>
> +/* lkdtm_xpfo.c */
> +void lkdtm_XPFO_READ_USER(void);
> +void lkdtm_XPFO_READ_USER_HUGE(void);
> +void lkdtm_XPFO_SMP(void);
> +
>  #endif
> diff --git a/drivers/misc/lkdtm_core.c b/drivers/misc/lkdtm_core.c
> index 42d2b8e31e6b..9544e329de4b 100644
> --- a/drivers/misc/lkdtm_core.c
> +++ b/drivers/misc/lkdtm_core.c
> @@ -235,6 +235,9 @@ struct crashtype crashtypes[] = {
>         CRASHTYPE(USERCOPY_STACK_FRAME_FROM),
>         CRASHTYPE(USERCOPY_STACK_BEYOND),
>         CRASHTYPE(USERCOPY_KERNEL),
> +       CRASHTYPE(XPFO_READ_USER),
> +       CRASHTYPE(XPFO_READ_USER_HUGE),
> +       CRASHTYPE(XPFO_SMP),
>  };
>
>
> diff --git a/drivers/misc/lkdtm_xpfo.c b/drivers/misc/lkdtm_xpfo.c
> new file mode 100644
> index 000000000000..d903063bdd0b
> --- /dev/null
> +++ b/drivers/misc/lkdtm_xpfo.c
> @@ -0,0 +1,194 @@
> +/*
> + * This is for all the tests related to XPFO (eXclusive Page Frame Ownership).
> + */
> +
> +#include "lkdtm.h"
> +
> +#include <linux/cpumask.h>
> +#include <linux/mman.h>
> +#include <linux/uaccess.h>
> +#include <linux/xpfo.h>
> +#include <linux/kthread.h>
> +
> +#include <linux/delay.h>
> +#include <linux/sched/task.h>
> +
> +#define XPFO_DATA 0xdeadbeef
> +
> +static unsigned long do_map(unsigned long flags)
> +{
> +       unsigned long user_addr, user_data = XPFO_DATA;
> +
> +       user_addr = vm_mmap(NULL, 0, PAGE_SIZE,
> +                           PROT_READ | PROT_WRITE | PROT_EXEC,
> +                           flags, 0);
> +       if (user_addr >= TASK_SIZE) {
> +               pr_warn("Failed to allocate user memory\n");
> +               return 0;
> +       }
> +
> +       if (copy_to_user((void __user *)user_addr, &user_data,
> +                        sizeof(user_data))) {
> +               pr_warn("copy_to_user failed\n");
> +               goto free_user;
> +       }
> +
> +       return user_addr;
> +
> +free_user:
> +       vm_munmap(user_addr, PAGE_SIZE);
> +       return 0;
> +}
> +
> +static unsigned long *user_to_kernel(unsigned long user_addr)
> +{
> +       phys_addr_t phys_addr;
> +       void *virt_addr;
> +
> +       phys_addr = user_virt_to_phys(user_addr);
> +       if (!phys_addr) {
> +               pr_warn("Failed to get physical address of user memory\n");
> +               return NULL;
> +       }
> +
> +       virt_addr = phys_to_virt(phys_addr);
> +       if (phys_addr != virt_to_phys(virt_addr)) {
> +               pr_warn("Physical address of user memory seems incorrect\n");
> +               return NULL;
> +       }
> +
> +       return virt_addr;
> +}
> +
> +static void read_map(unsigned long *virt_addr)
> +{
> +       pr_info("Attempting bad read from kernel address %p\n", virt_addr);
> +       if (*(unsigned long *)virt_addr == XPFO_DATA)
> +               pr_err("FAIL: Bad read succeeded?!\n");
> +       else
> +               pr_err("FAIL: Bad read didn't fail but data is incorrect?!\n");
> +}
> +
> +static void read_user_with_flags(unsigned long flags)
> +{
> +       unsigned long user_addr, *kernel;
> +
> +       user_addr = do_map(flags);
> +       if (!user_addr) {
> +               pr_err("FAIL: map failed\n");
> +               return;
> +       }
> +
> +       kernel = user_to_kernel(user_addr);
> +       if (!kernel) {
> +               pr_err("FAIL: user to kernel conversion failed\n");
> +               goto free_user;
> +       }
> +
> +       read_map(kernel);
> +
> +free_user:
> +       vm_munmap(user_addr, PAGE_SIZE);
> +}
> +
> +/* Read from userspace via the kernel's linear map. */
> +void lkdtm_XPFO_READ_USER(void)
> +{
> +       read_user_with_flags(MAP_PRIVATE | MAP_ANONYMOUS);
> +}
> +
> +void lkdtm_XPFO_READ_USER_HUGE(void)
> +{
> +       read_user_with_flags(MAP_PRIVATE | MAP_ANONYMOUS | MAP_HUGETLB);
> +}
> +
> +struct smp_arg {
> +       unsigned long *virt_addr;
> +       unsigned int cpu;
> +};
> +
> +static int smp_reader(void *parg)
> +{
> +       struct smp_arg *arg = parg;
> +       unsigned long *virt_addr;
> +
> +       if (arg->cpu != smp_processor_id()) {
> +               pr_err("FAIL: scheduled on wrong CPU?\n");
> +               return 0;
> +       }
> +
> +       virt_addr = smp_cond_load_acquire(&arg->virt_addr, VAL != NULL);
> +       read_map(virt_addr);
> +
> +       return 0;
> +}
> +
> +#ifdef CONFIG_X86
> +#define XPFO_SMP_KILLED SIGKILL
> +#elif CONFIG_ARM64
> +#define XPFO_SMP_KILLED SIGSEGV
> +#else
> +#error unsupported arch
> +#endif

This will fail the build for other architectures, so I would just do
this as an single if/else:

#ifdef CONFIG_ARM64
# define XPFO_SMP_KILLED SIGSEGV
#else
# define XPFO_SMP_KILLED SIGKILL
#endif

> +
> +/* The idea here is to read from the kernel's map on a different thread than

Comment style nit: leading /*\n please...

> + * did the mapping (and thus the TLB flushing), to make sure that the page
> + * faults on other cores too.
> + */
> +void lkdtm_XPFO_SMP(void)
> +{
> +       unsigned long user_addr, *virt_addr;
> +       struct task_struct *thread;
> +       int ret;
> +       struct smp_arg arg;
> +
> +       if (num_online_cpus() < 2) {
> +               pr_err("not enough to do a multi cpu test\n");
> +               return;
> +       }
> +
> +       arg.virt_addr = NULL;
> +       arg.cpu = (smp_processor_id() + 1) % num_online_cpus();
> +       thread = kthread_create(smp_reader, &arg, "lkdtm_xpfo_test");
> +       if (IS_ERR(thread)) {
> +               pr_err("couldn't create kthread? %ld\n", PTR_ERR(thread));
> +               return;
> +       }
> +
> +       kthread_bind(thread, arg.cpu);
> +       get_task_struct(thread);
> +       wake_up_process(thread);
> +
> +       user_addr = do_map(MAP_PRIVATE | MAP_ANONYMOUS);
> +       if (!user_addr)
> +               goto kill_thread;
> +
> +       virt_addr = user_to_kernel(user_addr);
> +       if (!virt_addr) {
> +               /*
> +                * let's store something that will fail, so we can unblock the
> +                * thread
> +                */
> +               smp_store_release(&arg.virt_addr, &arg);
> +               goto free_user;
> +       }
> +
> +       smp_store_release(&arg.virt_addr, virt_addr);
> +
> +       /* there must be a better way to do this. */
> +       while (1) {
> +               if (thread->exit_state)
> +                       break;
> +               msleep_interruptible(100);
> +       }

I don't like infinite loops. How about giving this a 1 second max runtime?

> +
> +free_user:
> +       if (user_addr)
> +               vm_munmap(user_addr, PAGE_SIZE);
> +
> +kill_thread:
> +       ret = kthread_stop(thread);
> +       if (ret != XPFO_SMP_KILLED)
> +               pr_err("FAIL: thread wasn't killed: %d\n", ret);
> +       put_task_struct(thread);
> +}
> --
> 2.11.0
>

Otherwise it looks great, thanks!

-Kees

-- 
Kees Cook
Pixel Security