Message-ID: <20170824154518.GB29665@leverpostej>
Date:   Thu, 24 Aug 2017 16:45:19 +0100
From:   Mark Rutland <mark.rutland@....com>
To:     Tycho Andersen <tycho@...ker.com>
Cc:     linux-kernel@...r.kernel.org, linux-mm@...ck.org,
        kernel-hardening@...ts.openwall.com,
        Marco Benatto <marco.antonio.780@...il.com>,
        Juerg Haefliger <juerg.haefliger@...onical.com>
Subject: Re: [kernel-hardening] [PATCH v5 04/10] arm64: Add __flush_tlb_one()

On Wed, Aug 23, 2017 at 11:13:02AM -0600, Tycho Andersen wrote:
> On Wed, Aug 23, 2017 at 06:04:43PM +0100, Mark Rutland wrote:
> > On Wed, Aug 23, 2017 at 10:58:42AM -0600, Tycho Andersen wrote:
> > > Hi Mark,
> > > 
> > > On Mon, Aug 14, 2017 at 05:50:47PM +0100, Mark Rutland wrote:
> > > > That said, is there any reason not to use flush_tlb_kernel_range()
> > > > directly?
> > > 
> > > So it turns out that there is a difference between __flush_tlb_one() and
> > > flush_tlb_kernel_range() on x86: flush_tlb_kernel_range() flushes all the TLBs
> > > via on_each_cpu(), where as __flush_tlb_one() only flushes the local TLB (which
> > > I think is enough here).
> > 
> > That sounds suspicious; I don't think that __flush_tlb_one() is
> > sufficient.
> > 
> > If you only do local TLB maintenance, then the page is left accessible
> > to other CPUs via the (stale) kernel mappings. i.e. the page isn't
> > exclusively mapped by userspace.
> 
> I thought so too, so I tried to test it with something like the patch
> below. But it correctly failed for me when using __flush_tlb_one(). I
> suppose I'm doing something wrong in the test, but I'm not sure what.

I suspect the issue is that you use a completion to synchronise the
mapping.

The reader thread will block (i.e. it we go into schedule() and
something else will run), and I guess that on x86, that the
context-switch this entails upon completion happens to invalidate the
TLBs.

Instead, you could serialise the update with the reader doing:

	/* spin until address is published to us */
	addr = smp_cond_load_acquire(arg->virt_addr, VAL != NULL);

	read_map(addr);

... and the writer doing:

	user_addr = do_map(...)

	...

	smp_store_release(arg->virt_addr, user_addr);

There would still be a chance of a context-switch, but it wouldn't be
mandatory.

As an aside, it looks like DEBUG_PAGEALLOC on x86 has the problem w.r.t.
under-invalidating, juding by the comments in x86's
__kernel_map_pages(). It only invalidates the local TLBs, even though
it should do it on all CPUs.

Thanks,
Mark.

> 
> Tycho
> 
> 
> From 1d1b0a18d56cf1634072096231bfbaa96cb2aa16 Mon Sep 17 00:00:00 2001
> From: Tycho Andersen <tycho@...ker.com>
> Date: Tue, 22 Aug 2017 18:07:12 -0600
> Subject: [PATCH] add XPFO_SMP test
> 
> Signed-off-by: Tycho Andersen <tycho@...ker.com>
> ---
>  drivers/misc/lkdtm.h      |   1 +
>  drivers/misc/lkdtm_core.c |   1 +
>  drivers/misc/lkdtm_xpfo.c | 139 ++++++++++++++++++++++++++++++++++++++++++----
>  3 files changed, 130 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/misc/lkdtm.h b/drivers/misc/lkdtm.h
> index fc53546113c1..34a6ee37f216 100644
> --- a/drivers/misc/lkdtm.h
> +++ b/drivers/misc/lkdtm.h
> @@ -67,5 +67,6 @@ 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 164bc404f416..9544e329de4b 100644
> --- a/drivers/misc/lkdtm_core.c
> +++ b/drivers/misc/lkdtm_core.c
> @@ -237,6 +237,7 @@ struct crashtype crashtypes[] = {
>  	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
> index c72509128eb3..7600fdcae22f 100644
> --- a/drivers/misc/lkdtm_xpfo.c
> +++ b/drivers/misc/lkdtm_xpfo.c
> @@ -4,22 +4,27 @@
>  
>  #include "lkdtm.h"
>  
> +#include <linux/cpumask.h>
>  #include <linux/mman.h>
>  #include <linux/uaccess.h>
>  #include <linux/xpfo.h>
> +#include <linux/kthread.h>
>  
> -void read_user_with_flags(unsigned long flags)
> +#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 = 0xdeadbeef;
> -	phys_addr_t phys_addr;
> -	void *virt_addr;
> +	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;
> +		return 0;
>  	}
>  
>  	if (copy_to_user((void __user *)user_addr, &user_data,
> @@ -28,25 +33,61 @@ void read_user_with_flags(unsigned long flags)
>  		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");
> -		goto free_user;
> +		return 0;
>  	}
>  
>  	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");
> -		goto free_user;
> +		return 0;
>  	}
>  
> +	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 == user_data)
> -		pr_info("Huh? Bad read succeeded?!\n");
> +	if (*(unsigned long *)virt_addr == XPFO_DATA)
> +		pr_err("FAIL: Bad read succeeded?!\n");
>  	else
> -		pr_info("Huh? Bad read didn't fail but data is incorrect?!\n");
> +		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:
> +free_user:
>  	vm_munmap(user_addr, PAGE_SIZE);
>  }
>  
> @@ -60,3 +101,79 @@ void lkdtm_XPFO_READ_USER_HUGE(void)
>  {
>  	read_user_with_flags(MAP_PRIVATE | MAP_ANONYMOUS | MAP_HUGETLB);
>  }
> +
> +struct smp_arg {
> +	struct completion map_done;
> +	unsigned long *virt_addr;
> +	unsigned int cpu;
> +};
> +
> +static int smp_reader(void *parg)
> +{
> +	struct smp_arg *arg = parg;
> +
> +	if (arg->cpu != smp_processor_id()) {
> +		pr_err("FAIL: scheduled on wrong CPU?\n");
> +		return 0;
> +	}
> +
> +	wait_for_completion(&arg->map_done);
> +
> +	if (arg->virt_addr)
> +		read_map(arg->virt_addr);
> +
> +	return 0;
> +}
> +
> +/* The idea here is to read from the kernel's map on a different thread than
> + * 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;
> +	struct task_struct *thread;
> +	int ret;
> +	struct smp_arg arg;
> +
> +	init_completion(&arg.map_done);
> +
> +	if (num_online_cpus() < 2) {
> +		pr_err("not enough to do a multi cpu test\n");
> +		return;
> +	}
> +
> +	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) {
> +		arg.virt_addr = user_to_kernel(user_addr);
> +		/* child thread checks for failure */
> +	}
> +
> +	complete(&arg.map_done);
> +
> +	/* there must be a better way to do this. */
> +	while (1) {
> +		if (thread->exit_state)
> +			break;
> +		msleep_interruptible(100);
> +	}
> +
> +	ret = kthread_stop(thread);
> +	if (ret != SIGKILL)
> +		pr_err("FAIL: thread wasn't killed: %d\n", ret);
> +	put_task_struct(thread);
> +
> +	if (user_addr)
> +		vm_munmap(user_addr, PAGE_SIZE);
> +}
> -- 
> 2.11.0
> 

Powered by blists - more mailing lists