Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1710233 > unrolled thread

Re: [kernel-hardening] [PATCH v5 04/10] arm64: Add __flush_tlb_one()

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [kernel-hardening] [PATCH v5 04/10] arm64: Add __flush_tlb_one() Mark Rutland <mark.rutland@arm.com> - 2017-08-12 13:30 +0200
    Re: [kernel-hardening] [PATCH v5 04/10] arm64: Add __flush_tlb_one() Tycho Andersen <tycho@docker.com> - 2017-08-14 18:40 +0200
      Re: [kernel-hardening] [PATCH v5 04/10] arm64: Add __flush_tlb_one() Mark Rutland <mark.rutland@arm.com> - 2017-08-14 19:00 +0200
        Re: [kernel-hardening] [PATCH v5 04/10] arm64: Add __flush_tlb_one() Tycho Andersen <tycho@docker.com> - 2017-08-14 19:10 +0200
        Re: [kernel-hardening] [PATCH v5 04/10] arm64: Add __flush_tlb_one() Tycho Andersen <tycho@docker.com> - 2017-08-23 19:00 +0200
          Re: [kernel-hardening] [PATCH v5 04/10] arm64: Add __flush_tlb_one() Mark Rutland <mark.rutland@arm.com> - 2017-08-23 19:10 +0200
            Re: [kernel-hardening] [PATCH v5 04/10] arm64: Add __flush_tlb_one() Tycho Andersen <tycho@docker.com> - 2017-08-23 19:20 +0200
              Re: [kernel-hardening] [PATCH v5 04/10] arm64: Add __flush_tlb_one() Mark Rutland <mark.rutland@arm.com> - 2017-08-24 17:50 +0200

#1710233 — Re: [kernel-hardening] [PATCH v5 04/10] arm64: Add __flush_tlb_one()

On Wed, Aug 09, 2017 at 02:07:49PM -0600, Tycho Andersen wrote:
> From: Juerg Haefliger <juerg.haefliger@hpe.com>
> 
> Add a hook for flushing a single TLB entry on arm64.
> 
> Signed-off-by: Juerg Haefliger <juerg.haefliger@canonical.com>
> Tested-by: Tycho Andersen <tycho@docker.com>
> ---
>  arch/arm64/include/asm/tlbflush.h | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/arch/arm64/include/asm/tlbflush.h b/arch/arm64/include/asm/tlbflush.h
> index af1c76981911..8e0c49105d3e 100644
> --- a/arch/arm64/include/asm/tlbflush.h
> +++ b/arch/arm64/include/asm/tlbflush.h
> @@ -184,6 +184,14 @@ static inline void flush_tlb_kernel_range(unsigned long start, unsigned long end
>  	isb();
>  }
>  
> +static inline void __flush_tlb_one(unsigned long addr)
> +{
> +	dsb(ishst);
> +	__tlbi(vaae1is, addr >> 12);
> +	dsb(ish);
> +	isb();
> +}

Is this going to be called by generic code?

It would be nice if we could drop 'kernel' into the name, to make it clear this
is intended to affect the kernel mappings, which have different maintenance
requirements to user mappings.

We should be able to implement this more simply as:

flush_tlb_kernel_page(unsigned long addr)
{
	flush_tlb_kernel_range(addr, addr + PAGE_SIZE);
}

Thanks,
Mark.

[toc] | [next] | [standalone]

#1711202

Hi Mark,

On Sat, Aug 12, 2017 at 12:26:03PM +0100, Mark Rutland wrote:
> On Wed, Aug 09, 2017 at 02:07:49PM -0600, Tycho Andersen wrote:
> > From: Juerg Haefliger <juerg.haefliger@hpe.com>
> > 
> > Add a hook for flushing a single TLB entry on arm64.
> > 
> > Signed-off-by: Juerg Haefliger <juerg.haefliger@canonical.com>
> > Tested-by: Tycho Andersen <tycho@docker.com>
> > ---
> >  arch/arm64/include/asm/tlbflush.h | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/arch/arm64/include/asm/tlbflush.h b/arch/arm64/include/asm/tlbflush.h
> > index af1c76981911..8e0c49105d3e 100644
> > --- a/arch/arm64/include/asm/tlbflush.h
> > +++ b/arch/arm64/include/asm/tlbflush.h
> > @@ -184,6 +184,14 @@ static inline void flush_tlb_kernel_range(unsigned long start, unsigned long end
> >  	isb();
> >  }
> >  
> > +static inline void __flush_tlb_one(unsigned long addr)
> > +{
> > +	dsb(ishst);
> > +	__tlbi(vaae1is, addr >> 12);
> > +	dsb(ish);
> > +	isb();
> > +}
> 
> Is this going to be called by generic code?

Yes, it's called in mm/xpfo.c:xpfo_kunmap.

> It would be nice if we could drop 'kernel' into the name, to make it clear this
> is intended to affect the kernel mappings, which have different maintenance
> requirements to user mappings.
> 
> We should be able to implement this more simply as:
> 
> flush_tlb_kernel_page(unsigned long addr)
> {
> 	flush_tlb_kernel_range(addr, addr + PAGE_SIZE);
> }

It's named __flush_tlb_one after the x86 (and a few other arches)
function of the same name. I can change it to flush_tlb_kernel_page,
but then we'll need some x86-specific code to map the name as well.

Maybe since it's called from generic code that's warranted though?
I'll change the implementation for now, let me know what you want to
do about the name.

Cheers,

Tycho

[toc] | [prev] | [next] | [standalone]

#1711254

On Mon, Aug 14, 2017 at 10:35:36AM -0600, Tycho Andersen wrote:
> Hi Mark,
> 
> On Sat, Aug 12, 2017 at 12:26:03PM +0100, Mark Rutland wrote:
> > On Wed, Aug 09, 2017 at 02:07:49PM -0600, Tycho Andersen wrote:
> > > +static inline void __flush_tlb_one(unsigned long addr)
> > > +{
> > > +	dsb(ishst);
> > > +	__tlbi(vaae1is, addr >> 12);
> > > +	dsb(ish);
> > > +	isb();
> > > +}
> > 
> > Is this going to be called by generic code?
> 
> Yes, it's called in mm/xpfo.c:xpfo_kunmap.
> 
> > It would be nice if we could drop 'kernel' into the name, to make it clear this
> > is intended to affect the kernel mappings, which have different maintenance
> > requirements to user mappings.

> It's named __flush_tlb_one after the x86 (and a few other arches)
> function of the same name. I can change it to flush_tlb_kernel_page,
> but then we'll need some x86-specific code to map the name as well.
> 
> Maybe since it's called from generic code that's warranted though?
> I'll change the implementation for now, let me know what you want to
> do about the name.

I think it would be preferable to do so, to align with 
flush_tlb_kernel_range(), which is an existing generic interface.

That said, is there any reason not to use flush_tlb_kernel_range()
directly?

Thanks,
Mark.

[toc] | [prev] | [next] | [standalone]

#1711260

On Mon, Aug 14, 2017 at 05:50:47PM +0100, Mark Rutland wrote:
> On Mon, Aug 14, 2017 at 10:35:36AM -0600, Tycho Andersen wrote:
> > Hi Mark,
> > 
> > On Sat, Aug 12, 2017 at 12:26:03PM +0100, Mark Rutland wrote:
> > > On Wed, Aug 09, 2017 at 02:07:49PM -0600, Tycho Andersen wrote:
> > > > +static inline void __flush_tlb_one(unsigned long addr)
> > > > +{
> > > > +	dsb(ishst);
> > > > +	__tlbi(vaae1is, addr >> 12);
> > > > +	dsb(ish);
> > > > +	isb();
> > > > +}
> > > 
> > > Is this going to be called by generic code?
> > 
> > Yes, it's called in mm/xpfo.c:xpfo_kunmap.
> > 
> > > It would be nice if we could drop 'kernel' into the name, to make it clear this
> > > is intended to affect the kernel mappings, which have different maintenance
> > > requirements to user mappings.
> 
> > It's named __flush_tlb_one after the x86 (and a few other arches)
> > function of the same name. I can change it to flush_tlb_kernel_page,
> > but then we'll need some x86-specific code to map the name as well.
> > 
> > Maybe since it's called from generic code that's warranted though?
> > I'll change the implementation for now, let me know what you want to
> > do about the name.
> 
> I think it would be preferable to do so, to align with 
> flush_tlb_kernel_range(), which is an existing generic interface.
> 
> That said, is there any reason not to use flush_tlb_kernel_range()
> directly?

I don't think so, I'll change the generic code to that and drop this
patch.

Thanks!

Tycho

[toc] | [prev] | [next] | [standalone]

#1718527

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

As you might expect, this is quite a performance hit (at least under kvm), I
ran a little kernbench:

# __flush_tlb_one
Wed Aug 23 15:47:33 UTC 2017
4.13.0-rc5+
Average Half load -j 2 Run (std deviation):
Elapsed Time 50.3233 (1.82716)
User Time 87.1233 (1.26871)
System Time 15.36 (0.500899)
Percent CPU 203.667 (4.04145)
Context Switches 7350.33 (1339.65)
Sleeps 16008.3 (980.362)

Average Optimal load -j 4 Run (std deviation):
Elapsed Time 27.4267 (0.215019)
User Time 88.6983 (1.91501)
System Time 13.1933 (2.39488)
Percent CPU 286.333 (90.6083)
Context Switches 11393 (4509.14)
Sleeps 15764.7 (698.048)

# flush_tlb_kernel_range()
Wed Aug 23 16:00:03 UTC 2017
4.13.0-rc5+
Average Half load -j 2 Run (std deviation):
Elapsed Time 86.57 (1.06099)
User Time 103.25 (1.85475)
System Time 75.4433 (0.415852)
Percent CPU 205.667 (3.21455)
Context Switches 9363.33 (1361.57)
Sleeps 14703.3 (1439.12)

Average Optimal load -j 4 Run (std deviation):
Elapsed Time 51.27 (0.615873)
User Time 110.328 (7.93884)
System Time 74.06 (1.55788)
Percent CPU 288 (90.2197)
Context Switches 16557.5 (7930.01)
Sleeps 14774.7 (921.746)

So, I think we need to keep something like __flush_tlb_one around.
I'll call it flush_one_local_tlb() for now, and will cc x86@ on the
next version to see if they have any insight.

Cheers,

Tycho

[toc] | [prev] | [next] | [standalone]

#1718528

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.

Thanks,
Mark.

[toc] | [prev] | [next] | [standalone]

#1718534

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.

Tycho


From 1d1b0a18d56cf1634072096231bfbaa96cb2aa16 Mon Sep 17 00:00:00 2001
From: Tycho Andersen <tycho@docker.com>
Date: Tue, 22 Aug 2017 18:07:12 -0600
Subject: [PATCH] add XPFO_SMP test

Signed-off-by: Tycho Andersen <tycho@docker.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

[toc] | [prev] | [next] | [standalone]

#1719350

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@docker.com>
> Date: Tue, 22 Aug 2017 18:07:12 -0600
> Subject: [PATCH] add XPFO_SMP test
> 
> Signed-off-by: Tycho Andersen <tycho@docker.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
> 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web